WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16888 closed enhancement (wontfix)

WP HTTP Curl transport is only handling some but not all redirect status codes.

Reported by: hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: HTTP API Keywords: has-patch
Focuses: Cc:

Description

That's violating the specs by namely processing 301 and 302 only:

HTTP status codes are extensible. HTTP applications are not required
to understand the meaning of all registered status codes, though such
understanding is obviously desirable. However, applications MUST
understand the class of any status code, as indicated by the first
digit, and treat any unrecognized response as being equivalent to the
x00 status code of that class, with the exception that an
unrecognized response MUST NOT be cached.

from: 6.1.1 Status Code and Reason Phrase

For 3xx responses, the location SHOULD indicate the
server's preferred URI for automatic redirection to the resource. The
field value consists of a single absolute URI.

from: 14.30 Location

Attachments (1)

16888.patch (707 bytes) - added by hakre 3 years ago.
3xx instead of 301 and 302 only.

Download all attachments as: .zip

Change History (15)

hakre3 years ago

3xx instead of 301 and 302 only.

comment:1 hakre3 years ago

Related: #16889

comment:2 hakre3 years ago

  • Summary changed from WP HTTP Layer with Curl is only handling some but not all redirects. to WP HTTP Curl transport is only handling some but not all redirect status codes.

comment:3 follow-up: sivel3 years ago

Not all 3xx codes indicate a redirection. For example 304 is a not modified and has nothing to do with redirection. Returning a WP_Error on a 304 would be incorrect behavior, as someone may be expecting to get additional information when a 304 is received.

comment:4 follow-up: dd323 years ago

  • Component changed from General to HTTP

What other status codes are used with a Redirect (other than 301/302)?

One option here is to redirect on any Location header, but in the case of the cURL library, IIRC, Redirections are handled internally by cURL, the modified code in the patch is mearly for error reporting purposes (If the final page is a 301/302, cURL gave up redirecting and returned a redirect response, If it falls past that, it should hit another error condition and return the raw error)

comment:5 in reply to: ↑ 3 hakre3 years ago

Replying to sivel:

Not all 3xx codes indicate a redirection. For example 304 is a not modified and has nothing to do with redirection.

The RFC 2616 speaks another language (6.1.1 Status Code and Reason Phrase):

    The first digit of the Status-Code defines the class of response.
    [...]
      - 3xx: Redirection - Further action must be taken in order to
        complete the request

And further on in specific of the 304 status (which is specified):

   If the client has performed a conditional GET request and access is
   allowed, but the document has not been modified, the server SHOULD
   respond with this status code. The 304 response MUST NOT contain a
   message-body, and thus is always terminated by the first empty line
   after the header fields.

Returning a WP_Error on a 304 would be incorrect behavior, as someone may be expecting to get additional information when a 304 is received.

If redirects are limited and the request results in a going-over-that-limit, then I would expect the function to return an error. Per all specification documents related to the context of HTTP the classification of the 3xx codes as being all redirects per RFC make me think so.

If you can provide any document with some authority on the subject specifying the 304 status not being classified as redirect, please link it, I would be interested.

If you're setting the redirection parameter to control a follow-location-header behavior, an additional parameter might be more flexible to actually control something (and prevent error return values): #16855 (, #16889).

Last edited 3 years ago by hakre (previous) (diff)

comment:6 in reply to: ↑ 4 ; follow-up: hakre3 years ago

Replying to dd32:

What other status codes are used with a Redirect (other than 301/302)?

All 3xx codes are classified as redirect per RFC (see my last comment)

One option here is to redirect on any Location header

I will make my mind about this, I'll check the location header for this.

, but in the case of the cURL library, IIRC, Redirections are handled internally by cURL, the modified code in the patch is mearly for error reporting purposes (If the final page is a 301/302, cURL gave up redirecting and returned a redirect response, If it falls past that, it should hit another error condition and return the raw error)

I have the feeling that cUrl is well developed (to say at least) and it's worth to test with it and look into it's detailed documentation to get a well-thought interface to http.

I'll review the 3xx codes in conjunction with location response headers as well.

comment:7 in reply to: ↑ 6 ; follow-up: hakre3 years ago

Replying to hakre:

I'll review the 3xx codes in conjunction with location response headers as well.

Done: HTTP Redirect Codes (3xx) and the Location Field - You'll find an overview-table for the use I could find specified with details linked.

comment:8 hakre3 years ago

We have a list of status codes as data: Related: #13955

Version 0, edited 3 years ago by hakre (next)

comment:9 voyagerfan57613 years ago

  • Cc WordPress@… added

comment:10 in reply to: ↑ 7 ; follow-up: dd323 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

Replying to hakre:

Done: HTTP Redirect Codes (3xx) and the Location Field

Looks like you've inverted all your logic there. By your logic we shouldn't redirect in every case we currently do.

Summarizing here for the purpose of keeping things in one place: http://tools.ietf.org/html/rfc2616

10.3.1 300 Multiple Choices
   The requested resource corresponds to any one of a set of
   representations, each with its own specific location, and agent-
   driven negotiation information.
   If the server has a preferred choice of representation, it SHOULD
   include the specific URI for that representation in the Location
   field; user agents MAY use the Location field value for automatic
   redirection.

10.3.4 303 See Other
   The response to the request can be found under a different URI and
    SHOULD be retrieved using a GET method on that resource.

10.3.6 305 Use Proxy
   The requested resource MUST be accessed through the proxy given by
   the Location field. The Location field gives the URI of the proxy.
   The recipient is expected to repeat this single request via the
   proxy.

10.3.8 307 Temporary Redirect
   The requested resource resides temporarily under a different URI.
   The temporary URI SHOULD be given by the Location field in the
   response. 
   If the 307 status code is received in response to a request other
   than GET or HEAD, the user agent MUST NOT automatically redirect the
   request unless it can be confirmed by the user, since this might
   change the conditions under which the request was issued.

Selectively copied segments which is of use to this ticket, both 303 and 307 specify that many HTTP 1.0 clients do not understand them, and a 302 is recommended for interoperability. 300 can be treated like a 301/302 (As we do not do user-agent negotiation, if the servers preferred method is there, take it)

I'm throwing this into Future Release as a enhancement, The WP_HTTP class currently supports the most common redirection methods in use on the web today (I'd challenge anyone to find anyone actively using them..)

comment:11 in reply to: ↑ 10 hakre3 years ago

Replying to dd32:

I have no problem discussing the RFC 2626, but let's stick a bit on topic and not head that far away from this ticket.

It's as much not about removing redirects as it is about performing additional automatic follow-up-requests because of location headers.

The article linked on my blog was only a write-up to summarize the use of the location header in the 3xx class, it's not entirely related for this ticket here so I put it apart.

I opened this ticket to classify any status in the 3xx redirect class as being a redirect so they get counted for the error condition.

16888.patch

comment:12 follow-up: dd323 years ago

on the back of #16889, it might be best to consider handling a location header being present instead of handling it by response code.

comment:13 in reply to: ↑ 12 hakre3 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

Replying to dd32:

on the back of #16889, it might be best to consider handling a location header being present instead of handling it by response code.

For 201 this makes sense. We didn't cover it before as well, right.

Let's consider this done for the moment.

comment:14 ocean903 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.