wp_safe_remote_get request to an HTTPS site fails

[akirk]

Code to reproduce:

wp_safe_remote_get( 'https://alex.kirk.at/' );

returns

object(WP_Error)#1235 (3) {
    ["errors"]=> array(1) {
        ["http_request_failed"]=> array(1) {
            [0]=> string(29) "A valid URL was not provided."
        }
    }
    ["error_data"]=> array(0) { }
    ["additional_data":protected]=> array(0) { }
} 

I've identified the reason being that this arg is specified:

"sslcertificates" => "/var/www/html/wp-includes/certificates/ca-bundle.crt",

Maybe that path is not a valid one?
If I directly use

_wp_http_get_object()->request( 'https://alex.kirk.at/' );

Then it works.

[dmsnell]

that bundle should be in the standard WordPress install, but maybe it's not at the right WordPress path?

that default is literally ABSPATH . WPINC . '/certificates/ca-bundle.crt' so I'm not sure this fails, unless there is some problem with absolute paths generally, which I don't think there is 🤔

[akirk]

Sorry for the wrong lead, the ca-bundle file is fine. The function that fails when using wp_safe_remote_get() is actually wp_http_validate_url() and it fails because gethostbyname() returns an internal IP (172.29.2.0).

Thus a workaround is defining this filter:

add_filter('http_request_host_is_external', '__return_true' );

[dmsnell]

that's good @akirk - though I guess we could probably also implement that function so it returns false for the local routing?

[akirk]

The gethostbyname() either needs to return a non-local (i.e. the real) IP address, that particular host needs to be considered external, or we just don't do reject_unsafe_urls.

[adamziel]

Nice debugging, thank you ❤️

Sounds like the „hardcode localhost resolution” fix won’t cut it and we need to defer to the OS gethostbyname implementation.

The DNS.lookup() implementation that comes with node.js is asynchronous so there are two ways to go about it:

• Find a synchronous alternative, plug it into Emscripten, done
• Plug in the asynchronous version via Asyncify, which perhaps wouldn’t be that much harder but would create additional stack switching code paths

Unrelated trivia: I just learned that dns.lookup() pretends to be asynchronous, but in reality it calls a blocking function:

https://httptoolkit.com/blog/configuring-nodejs-dns/

The author discusses a cacheable-lookup library that exposes a synchronous lookup() function - perhaps it could „just work” for us?

https://www.npmjs.com/package/cacheable-lookup

[akirk]

Why not just add that filter via a mu-plugin? Accessing localhost (which I believe this code path is trying to prevent) inside the sandbox should not be a problem?

[adamziel]

Oh sorry I didn’t explain - this will be a problem for every site, e.g. in your case it was related to wp_safe_remote_get( 'https://alex.kirk.at/' );. I’m looking for a general fix

[adamziel]

About the filter add_filter('http_request_host_is_external', '__return_true' );, it will do the trick as long as we’re in a dev env. That might be okay for now, I just worry about future production use. Maybe I worry too much, though. Let’s file an issue to keep track of it and use a filter.

Edit: Issue created here: #400

[adamziel]

Technically, this filter:

add_filter('http_request_host_is_external', '__return_true' );

Can be added as an mu-plugin to Playground in WordPressPatcher (it prepares WordPress for being ran in Playground whenever it's loaded). One other mu-plugin is already added there:

wordpress-playground/packages/playground/remote/src/lib/web-wordpress-patches/index.ts

Line 10 in 4ff89bf

import showAdminCredentialsOnWpLogin from './wp-content/mu-plugins/1-show-admin-credentials-on-wp-login.php?raw';

[adamziel]

This works as expected with networking enabled (via ?networking=yes). Without networking, adding the filter mentioned above simply leads to a different error:

object(WP_Error)#1318 (3) { ["errors"]=> array(1) { ["http_request_failed"]=> array(1) { [0]=> string(29) "Missing header/body separator" } } ["error_data"]=> array(0) { } ["additional_data":protected]=> array(0) { } }

@akirk what would you expect to happen instead in that scenario?

[adamziel]

I'm going to close this since I'm not sure if there's an actual problem that needs solving. I'm happy to be wrong here. @akirk feel free to reopen if there's still something that needs to be addressed here.