Add a hook in wp_http_validate_url to control which ports are allowed for remote requests

[xknown]

By default we only allow ports 80, 443, and 8080 to be used for (safe) remote requests.

https://core.trac.wordpress.org/browser/tags/5.8/src/wp-includes/http.php#L584

It'd be nice to have a way to remove these hardcode ports so we can better control which ports are allowed or not.

By default we only allow ports 80, 443, and 8080 to be used for (safe) remote requests. This adds a way to change these hardcode ports so we can better control which ports are allowed or not.

Trac ticket: https://core.trac.wordpress.org/ticket/54331

[jorbin]

Did a first pass at the filter, would be good to get some additional eyes on the docblock and filter name. The good-first-bug still applies as someone will need to do the unit test. I think an appropriate test would be one that makes sure wp_http_validate_url allows a new port when the filter is used and doesn't allow it when it is not.

[costdev]

@jorbin I've submitted a review on ​PR 1790.

New contributors:
If you're interested in working on the unit tests for this PR, check out the ​Writing PHPUnit Tests Handbook entry and leave a comment in this ticket to let us know you're working on the unit test. You may also find it useful to look in tests/phpunit/tests to find some unit tests for existing filters.

Feel free to ping me on Slack if you have any questions.

Bug gardeners:
If this ticket still needs-unit-tests as the 5.9 milestone approaches and you're about to punt this to a later release, feel free to ping me and I'll submit the tests so we can get this into 5.9.

[costdev]

When allowing extenders to decide which ports should be allowed, a plugin or theme could do this:

add_filter( 'http_allowed_safe_ports', 'remove_all_safe' );

function remove_all_safe( $ports ) {
	return array();
}

If this isn't accounted for, valid urls containing a port may be deemed invalid and vice versa. This may also create a vulnerability in Core. ​PR 1790 doesn't yet account for this.

If array() or any other invalid value is provided, I think that wp_http_validate_url() should have a fallback to the default array( 80, 443, 8080 ) and possibly throw a _doing_it_wrong() to ensure that the filter is used as expected.

Unit tests should probably be postponed until we've decided on the intended behaviour.

I have unit tests drafted that target all of wp_http_validate_url() including some minor refactoring. I'm holding off in case a new contributor wants to voice their interest in providing unit tests.

If we haven't heard any expressions of interest by the time we come to a decision on the implementation, I can finalize and submit the unit tests for wp_http_validate_url() that will also cover this filter.

Marking this as 2nd-opinion and dev-feedback to get thoughts on the best implementation.

[dd32]

If this isn't accounted for, valid urls containing a port may be deemed invalid and vice versa. This may also create a vulnerability in Core. PR 1790 doesn't yet account for this.

An integrator could also do something like add_filter( 'http_allowed_safe_ports', function( $ports, $host, $url) { shell_exec( $url ); }, 10, 3 );.

Some expectations of sanity should be applied to those writing filters, there's far worse things they can do than to return an empty array, just because they could do it wrong, we shouldn't have to expect them to do it wrong.

I would consider add_filter( 'http_allowed_safe_ports', '__return_empty_array' ); to mark all requests as unsafe, and add_filter( 'http_allowed_safe_ports', '__return_false' ); to throw a PHP Warning from the in_array() check (and ultimately mark it as unsafe).

[costdev]

Thanks for the feedback @dd32!

​PR 1790's current implementation results in __return_empty_array marking all requests as unsafe and __return_false throwing a PHP Warning, so it looks like the PR is good to go after the suggested changes are handled.

I'll submit the unit tests on Friday if we haven't seen an expression of interest from a new contributor by then.

Removing 2nd-opinion and dev-feedback. Thanks again!

wp_http_validate_url() lacked unit tests.

This PR:

• Adds unit tests for wp_http_validate_url().
• Improves validation in wp_http_validate_url().
• Removes an unnecessary branch to improve test coverage.
• Adds unit tests for the new http_allowed_safe_ports filter in wp_http_validate_url() ​PR 1790

These changes have been separated into different commits to allow a committer to cherry pick the changes they think are ready to go into core.

Trac ticket: https://core.trac.wordpress.org/ticket/54331

[costdev]

Unit tests and other improvements have been added for wp_http_validate_url().

Committers:
The final commit in ​PR 1826 adds tests for the filter added in ​PR 1790.
Once this filter is committed, the unit tests in ​PR 1826's final commit will pass.

In the meantime, ​PR 1826 is ready for review and three of the four commits can be reviewed (and hopefully considered for commit) before the filter if it isn't ready.

[costdev]

@jorbin Will you have time to check the review on your PR so that we can look at committing this filter before tomorrow's feature freeze?

I've also requested a review on my PR in core-test, but anyone reading this comment is invited to review to help get these changes ready for commit.

[costdev]

​PR 1790 has been merged into ​PR 1826, which is now ready for final review ahead of today's feature freeze.

[hellofromTonya]

The patch and tests look good. Marking for commit.

[hellofromTonya]

As this patch adds a new filter, it will need a call-out in the Misc Dev Note.

[hellofromTonya]

Assigning to me for commit.

[hellofromTonya]

In 52084:

HTTP API: Introduce 'http_allowed_safe_ports' filter in wp_http_validate_url().

Adds a new filter 'http_allowed_safe_ports' to control which ports are allowed for remote requests. By default, ports 80, 443, and 8080 are allowed for safe remote requests.

Adds tests.

Follow-up to [24480].

Props xknown, johnbillion, jorbin, costdev, dd32.
Fixes #54331.

Committed in changeset https://core.trac.wordpress.org/changeset/52084.

[hellofromTonya]

In 52085:

HTTP API: Ensure value returned from 'http_allowed_safe_ports' is an array to avoid PHP 8+ TypeError fatal error.

Adds an is_array() check before the in_array(). Why? in_array() requires a array for the haystack. Any other data type will cause a fatal error on PHP 8.0 or higher:

Fatal error: Uncaught TypeError: in_array(): Argument #2 ($haystack) must be of type array

As this is a new filter, this type check properly guards to avoid the fatal error.

Follow-up to [52084].

See #54331.

[audrasjb]

Dev note published: ​https://make.wordpress.org/core/2022/01/04/miscellaneous-core-changes-in-wordpress-5-9/

[PerS]

When I read "misc core changes in wp 5.9", I hoped #21077 was fixed. Alas, I was wrong.

Rolled into PR #1826 which was committed via https://core.trac.wordpress.org/changeset/52084 and https://core.trac.wordpress.org/changeset/52085.