WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#10624 closed defect (bug) (fixed)

Redirection should be disabled for HEAD requests

Reported by: dd32 Owned by: dd32
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: HTTP API Keywords: has-patch
Focuses: Cc:

Description (last modified by dd32)

Updated:

Currently, HEAD requests will follow redirections in some(or all) transports. HEAD requests should not follow redirects.


Original:

Currently if you request a URL that attempts to redirect past your redirection limit, you'll get a object similar to this:

object(WP_Error)#106 (2) {
  ["errors"]=>
  array(1) {
    ["http_request_failed"]=>
    array(1) {
      [0]=>
      string(30) "Maximum (0) redirects followed"
    }
  }
  ["error_data"]=>
  array(0) {
  }
}

The major problem with this is that its impossible to retrieve the location its being redirected to.

Right now, I'm using wp_remote_head() to check if its redirecting to a different location, Unfortunately thats not possible...

I'll add a patch which adds the location header to the error data objects

But i'm also tempted to suggest, that if its a HEAD request (Or really, any request) and redirects are set to 0, then it shouldnt attempt to redirect at all, and simply return the headers as is (ie. as a 301 with a location header and possibly empty body)

Attachments (2)

10624.diff (2.2 KB) - added by dd32 6 years ago.
10624.2.diff (2.7 KB) - added by dd32 5 years ago.

Download all attachments as: .zip

Change History (15)

@dd326 years ago

comment:1 @dd326 years ago

This might actually be impossible..

Half the classes do not support redirections very well, You cant catch the error with some (So cannot tell why the request failed), Not all abide by the max-redirections, and end up returning multiple headers(WP_Http_Fopen at least).

Only really the Curl and FSockopen seem to support redirects well.

I'm going to attach a patch which modifies Curl and FSockopen to return as a normal request, but with a 301/2 error if the max-redirects is 0..

Note, The switching of the Curl error handling order was to allow for the custom 30x error message to have priority over Curl's own internal error. The $parts.. line was also redundant, not being used at all by anything.

comment:2 @ryan5 years ago

  • Milestone changed from 2.9 to Future Release

comment:3 @dd325 years ago

any dev feedback on this, Or close wontfix?

comment:4 @westi5 years ago

  • Cc westi added
  • Keywords dev-feedback removed

I am surprised that HEAD requests follow the redirects and I would expect to get the HEAD results back and read the information out.

Do you want to put together a patch for that.

As for returning the redirected url - which of the many urls would you return?

The last one. The original requested one.

It feels like we should have a consistent WP_Error for redirection limit exceeded and allow people to use wp_remote_head to do single requests to work out why if they want.

comment:5 @dd325 years ago

  • Description modified (diff)
  • Keywords needs-patch added
  • Summary changed from Upon redirection limit being hit return redirected URL in wp_error data fields to Redirection should be disabled for HEAD requests

I am surprised that HEAD requests follow the redirects and I would expect to get the HEAD results back and read the information out.

Me too, I'll write in a patch for that - Damn, it'll create conflicts with all my other HTTP patches from this evening :)

As for returning the redirected url - which of the many urls would you return?

The last one. However, You're right i think, Its not really a typical use-case.. It'd be a pretty rare occurance.

Lets skip returning the URL, and stick to preventing redirects for Head requests.

comment:6 @dd325 years ago

  • Status changed from new to accepted

@dd325 years ago

comment:7 @dd325 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 3.0

attachment 10624.2.diff added

  • Disables redirects on HEAD requests.
  • Slight bug fix, Turns out that the Streams transport is not returning error-conditional pages, Adding 'ignore_errors' => true fixed that. (Note: a 302 was classified as a error if it was set to not redirect). Result was that it was throwing the error "Could not open handle"
  • Disabled HEAD requests on one of the transports that cant handle HEAD's. See #11613 for a bug related to that, If a Request is made before it using GET, the result of the test for HEAD will fail as the GET request was cached.

comment:8 @dd325 years ago

One more addition:

  • the PHP HTTP Extension is untested, As i dont have access to it. I'm going off the docs on that change, and its mentioned in a comment on that line. Ideally, Once i got to test that transport properly that comment would be removed...

comment:9 @hakre5 years ago

Fore a certain transport method to test it (here curl) via plugin: #11499/11499-curlforce.php

comment:10 @dd325 years ago

While I remember about it, Theres a import-related function which uses HEAD requests and expects it to follow requests. That'll need to be updated to reflect any changes here.

comment:11 @dd325 years ago

(In [13149]) Disable Redirection on HEAD requests. See #10624

comment:12 @dd325 years ago

(In [13151]) Update wp_get_http() to handle redirections on HEAD requests. Un-deprecate $red header to track total redirections. See #10624

comment:13 @dd325 years ago

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

From what i can see, That was the only function which was expecting it to follow redirects on HEAD requests.

Re-open if anyone notices anything broken.

Note: See TracTickets for help on using tickets.