Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Networking: Implement gethostbyname #400

Open
adamziel opened this issue May 21, 2023 · 2 comments
Open

Networking: Implement gethostbyname #400

adamziel opened this issue May 21, 2023 · 2 comments

Comments

@adamziel
Copy link
Collaborator

adamziel commented May 21, 2023

Problem

As seen in #396, calling gethostbyname in PHP generates a random local IP address. Emscripten then re-routes it to the correct location, but the fact we even have local IP in the mix causes wp_safe_remote_get to fail since it refuses to connect to a local IP address.

A workaround discussed in that issue silences the warning, which works in a dev environment, but for production use we need a proper fix that doesn't open up a possibility of local network traversal.

Proposed solution

Hardcoding host resolution 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

Related info

This is also solved by enabling the networking access by adding networking=yes to the URL, for example:

https://playground.wordpress.net/?plugin=create-block-theme&url=/wp-admin/admin.php?page=create-block-theme&networking=yes

Or with the following Blueprint:

{
    "features": {
        "networking": true
    }
}

cc @akirk @dmsnell @danielbachhuber

@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Feature] PHP.wasm [Aspect] Networking Production use labels May 21, 2023
@adamziel adamziel changed the title Networking: Synchronously call OS gethostbyname function Networking: Implement gethostbyname May 21, 2023
@adamziel adamziel mentioned this issue Oct 4, 2023
10 tasks
@adamziel
Copy link
Collaborator Author

Actually, this is only a problem when networking is disabled. Native PHP returns the original input in that scenario:

php > var_dump(gethostbyname('www.example.com'));
string(13) "93.184.216.34"
php > var_dump(gethostbyname('www.example.com'));
string(15) "www.example.com"

The WASM version of gethostname should do the same. At the same time, this seems like a superficial difference since the actual network call won't work anyway unless networking is enabled.

@adamziel adamziel added [Type] Enhancement New feature or request and removed [Type] Bug An existing feature does not function as intended labels Jan 31, 2024
@adamziel
Copy link
Collaborator Author

gethostbyname is still relevant because of the following validation code in wp-includes/http.php:

		if ( preg_match( '#^(([1-9]?\d|1\d\d|25[0-5]|2[0-4]\d)\.){3}([1-9]?\d|1\d\d|25[0-5]|2[0-4]\d)$#', $host ) ) {
			$ip = $host;
		} else {
			$ip = gethostbyname( $host );
			if ( $ip === $host ) { // Error condition for gethostbyname().
				return false;
			}
		}
		if ( $ip ) {
			$parts = array_map( 'intval', explode( '.', $ip ) );
			if ( 127 === $parts[0] || 10 === $parts[0] || 0 === $parts[0]
				|| ( 172 === $parts[0] && 16 <= $parts[1] && 31 >= $parts[1] )
				|| ( 192 === $parts[0] && 168 === $parts[1] )
			) {
				// If host appears local, reject unless specifically allowed.
				if ( ! apply_filters( 'http_request_host_is_external', false, $host, $url ) ) {
					return false;
				}
			}
		}

The local IP address that gethostbyname("s.w.org") returns by default fails the validation and causes WordPress to give up without issuing a request. As a result, the new 6.5 Google Fonts feature doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant