WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#54331 closed enhancement (fixed)

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

Reported by: xknown Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: good-first-bug has-patch has-unit-tests commit needs-dev-note
Focuses: Cc:

Description

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.

Change History (21)

#1 @johnbillion
5 weeks ago

  • Keywords needs-patch needs-unit-tests good-first-bug added

This ticket was mentioned in PR #1790 on WordPress/wordpress-develop by aaronjorbin.


5 weeks ago

  • Keywords has-patch added; needs-patch removed

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

#3 @jorbin
5 weeks ago

  • Milestone changed from Awaiting Review to 5.9

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.

#4 @costdev
4 weeks ago

@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.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


4 weeks ago

#6 @costdev
4 weeks ago

  • Keywords 2nd-opinion dev-feedback added

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.

#7 @dd32
4 weeks ago

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).

#8 @costdev
4 weeks ago

  • Keywords 2nd-opinion dev-feedback removed

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!

This ticket was mentioned in PR #1826 on WordPress/wordpress-develop by costdev.


4 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

#10 @costdev
4 weeks ago

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.

Last edited 4 weeks ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core-test by costdev. View the logs.


3 weeks ago

#12 @costdev
3 weeks ago

@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.

This ticket was mentioned in Slack in #core by costdev. View the logs.


3 weeks ago

#14 @costdev
3 weeks ago

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

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 weeks ago

#16 @hellofromTonya
3 weeks ago

  • Keywords commit added

The patch and tests look good. Marking for commit.

#17 @hellofromTonya
3 weeks ago

  • Keywords needs-dev-note added

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

#18 @hellofromTonya
3 weeks ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Assigning to me for commit.

#19 @hellofromTonya
3 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#21 @hellofromTonya
3 weeks ago

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.

Note: See TracTickets for help on using tickets.