Make WordPress Core

Opened 6 months ago

Closed 5 months ago

Last modified 5 months 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
6 months 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.


6 months 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
6 months 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
6 months 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
6 months 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
5 months 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
5 months ago

  • Milestone changed from Awaiting Review to 6.6

#8 @audrasjb
5 months 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
5 months 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.