Make WordPress Core

Opened 7 weeks ago

Closed 8 days ago

Last modified 7 days ago

#61092 closed defect (bug) (fixed)

Improving documentation of wp_remote_safe_* and wp_http_validate_url

Reported by: benjaminpick's profile benjaminpick Owned by: audrasjb's profile audrasjb
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: has-patch
Focuses: docs Cc:

Description

The documentation of wp_safe_remote_get says

"The URL is validated to avoid redirection and request forgery attacks."

However, there is no code preventing redirects - it is "only" validating the request URL.

P.S. - Oh, maybe the sentence is worded ambigiously. It could be read as:

The URL is validated to avoid

  • redirection and
  • request

forgery attacks.

But also as (and that's how I read it):

The URL is validated to avoid

  • redirection and
  • request forgery attacks.

May I suggest to elaborate the documentation, e.g. as in #60934:
"This is intended to protect against SSRF attacks, in which an application is 'tricked' to request non-public resources and expose them publicly through the accessible endpoint. We additionally protect against redirection attacks used to start a SSRF attack."

Change History (10)

#1 @audrasjb
7 weeks ago

  • Component changed from General to HTTP API
  • Focuses docs added
  • Keywords needs-patch added

This ticket was mentioned in PR #6469 on WordPress/wordpress-develop by benjaminpick.


7 weeks ago
#2

  • Keywords has-patch added; needs-patch removed

Documentation seems to be ambigous and needs to be more specific.
In particular, "The URL is validated to avoid redirection and request forgery attacks." can be understood as "... to avoid redirection ..." which is not what it is doing.

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

#3 @benjaminpick
7 weeks ago

  • Keywords needs-patch added; has-patch removed

This my first take on this, could you please add your feedback?
Of course, the same wording will be used for the other wp_safe_remote_* functions as well.

#4 @audrasjb
7 weeks ago

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

Thanks for the patch! It looks good at a glance. Just left a small typo feedback but I think this can be extended on the other similar functions as well 👍

#5 @benjaminpick
7 weeks ago

  • Keywords has-patch added; needs-patch removed
  • Summary changed from Does wp_safe_remote_get really disable redirects? (Documentation) to Improving documentation of wp_remote_safe_* and wp_http_validate_url

I'm glad! I have removed the typo and copied the docs to the rest of the functions.

#6 @benjaminpick
2 weeks ago

Anything I can do to move this ticket forward? Maybe, should the unit tests include all the examples that I have written into the doc? They are copied or are a variation of what is already in the unit tests.

#7 @audrasjb
2 weeks ago

  • Milestone changed from Awaiting Review to 6.6

#8 @audrasjb
8 days ago

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

In 58384:

Docs: Improve wp_remote_safe_* and wp_http_validate_url docblocks.

Props benjaminpick, audrasjb.
Fixes #61092.

#10 @SergeyBiryukov
7 days ago

In 58388:

Docs: Further improve the wp_http_validate_url() DocBlock.

Follow-up to [58384].

See #61092.

Note: See TracTickets for help on using tickets.