Make WordPress Core

Opened 13 years ago

Closed 4 years ago

#17010 closed defect (bug) (wontfix)

Inconsistent handling of HTTP response codes

Reported by: sivel's profile sivel Owned by:
Milestone: Priority: lowest
Severity: normal Version: 3.1
Component: HTTP API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

I noticed during some testing to see what would happen if a server returned a 404 when streaming to a file,and found some inconsistent handling of response codes between transports.

The below test checks to see if the transport returns a WP_Error or if it returns an actual response. If the second column is an integer matching the first column then the transport returned a response, if the second column is 'wp_error' then the transport returned WP_Error for that response code.

STREAMS:

100 => 100
101 => 101
102 => 102
200 => 200
201 => 201
202 => 202
203 => 203
204 => 204
205 => 205
206 => 206
207 => 207
226 => 226
300 => 300
301 => 301
302 => 302
303 => 303
304 => 304
305 => 305
306 => 306
307 => 307
400 => 400
401 => 401
402 => 402
403 => 403
404 => 404
405 => 405
406 => 406
407 => 407
408 => 408
409 => 409
410 => 410
411 => 411
412 => 412
413 => 413
414 => 414
415 => 415
416 => 416
417 => 417
422 => 422
423 => 423
424 => 424
426 => 426
500 => 500
501 => 501
502 => 502
503 => 503
504 => 504
505 => 505
506 => 506
507 => 507
510 => 510

FSOCKOPEN:

100 => 100
101 => 101
102 => 102
200 => 200
201 => 201
202 => 202
203 => 203
204 => 204
205 => 205
206 => 206
207 => 207
226 => 226
300 => 300
301 => 301
302 => 302
303 => 303
304 => 304
305 => 305
306 => 306
307 => 307
400 => wp_error
401 => wp_error
402 => wp_error
403 => wp_error
404 => wp_error
405 => wp_error
406 => wp_error
407 => wp_error
408 => wp_error
409 => wp_error
410 => wp_error
411 => wp_error
412 => wp_error
413 => wp_error
414 => wp_error
415 => wp_error
416 => wp_error
417 => wp_error
422 => wp_error
423 => wp_error
424 => wp_error
426 => wp_error
500 => 500
501 => 501
502 => 502
503 => 503
504 => 504
505 => 505
506 => 506
507 => 507
510 => 510

EXTHTTP:

100 => wp_error
101 => wp_error
102 => wp_error
200 => 200
201 => 201
202 => 202
203 => 203
204 => 204
205 => 205
206 => 206
207 => 207
226 => 226
300 => 300
301 => 301
302 => 302
303 => 303
304 => 304
305 => 305
306 => 306
307 => 307
400 => 400
401 => 401
402 => 402
403 => 403
404 => 404
405 => 405
406 => 406
407 => 407
408 => 408
409 => 409
410 => 410
411 => 411
412 => 412
413 => 413
414 => 414
415 => 415
416 => 416
417 => 417
422 => 422
423 => 423
424 => 424
426 => 426
500 => 500
501 => 501
502 => 502
503 => 503
504 => 504
505 => 505
506 => 506
507 => 507
510 => 510

CURL:

100 => wp_error
101 => wp_error
102 => wp_error
200 => 200
201 => 201
202 => 202
203 => 203
204 => 204
205 => 205
206 => 206
207 => 207
226 => 226
300 => 300
301 => 301
302 => 302
303 => 303
304 => 304
305 => 305
306 => 306
307 => 307
400 => 400
401 => 401
402 => 402
403 => 403
404 => 404
405 => 405
406 => 406
407 => 407
408 => 408
409 => 409
410 => 410
411 => 411
412 => 412
413 => 413
414 => 414
415 => 415
416 => 416
417 => 417
422 => 422
423 => 423
424 => 424
426 => 426
500 => 500
501 => 501
502 => 502
503 => 503
504 => 504
505 => 505
506 => 506
507 => 507
510 => 510

We need to define when we want to return a WP_Error, if ever. I am thinking that returning a WP_Error is not very useful if we get a response from the server. The response codes tested were obtained from get_status_header_desc().

Attachments (2)

17010.diff (2.2 KB) - added by johnbillion 8 years ago.
status.php (1.8 KB) - added by johnbillion 8 years ago.

Download all attachments as: .zip

Change History (17)

#1 @dd32
13 years ago

The 1xx errors will be being raised by cURL/http internally, we currently check to see if the error it has raised is a redirect and leave it at that.

If 1xx errors are pretty much invalid final responses (Pretty sure they are, they expect a next response correct?) then perhaps we can standardise on returning an error for those cases.

fsockopen is out on it's own there, so that WP_Error response set for 4xx should probably be canned.

I've said it elsewhere that a WP_Error response from the API should be considered a failed request - either network based or a 404(or similar) being returned - based on fsockopen()'s responses.

#2 follow-up: @sivel
13 years ago

I believe the only time that we should return WP_Error is if the transport failed, not due to the status of the request as defined by the response code.

We should always return the response, and only return WP_Error for an actual transport failure.

Last edited 13 years ago by sivel (previous) (diff)

#3 @dd32
13 years ago

It looks like the 100 error is being translated to a empty response, in the wild this is caused by: http://the-stickman.com/web-development/php-and-curl-disabling-100-continue-header/

The HTTP Extension is based internally on cURL which is why it has the same response pattern - I don't think the Extension offers enough of a API to catch this particular issue; I personally question the value the HTTP Extension brings given it's either a cURL wrapper or a fopen() wrapper depending on the configuration.

100 responses shouldn't be delivered to the client IMO, They're a informational reply from the server that states the actual response is about to come through. I'm not too sure what would happen with a legit 101 response (With the Upgrade header set to HTTP/1.1 on a HTTP/1.0 request)

#4 in reply to: ↑ 2 @gingerbread
13 years ago

Replying to sivel:

I believe the only time that we should return WP_Error is if the transport failed, not due to the status of the request as defined by the response code.

We should always return the response, and only return WP_Error for an actual transport failure.

I like that approach.

And as an additional idea: As long as a (partial) response has been even given while an error occurred of such type that an WP_Error is considered to be returned, we should add the (partial) response data to the WP_Error object.

#5 @dd32
13 years ago

(In [17597]) Return 4xx errors as a standard response from WP_HTTP, Brings it in line with the rest of the transports for 4xx error handling. See #17010

#6 @dd32
13 years ago

It looks like cURL only returns a WP_Error if the HTTP code is 100 AND the content is blank.

#7 @dd32
13 years ago

The specific error code is 52: (from http://curl.haxx.se/libcurl/c/libcurl-errors.html)

CURLE_GOT_NOTHING (52) 

Nothing was returned from the server, and under the circumstances, getting nothing is considered an error.

If we want to standardise on returning a normal response for 1xx errors, I can't find a way to reliable catch it.

I've tried changing the error check to check for both an empty body, error 53, and headers are present.. but that resulted in timeouts not being caught properly.

It looks like cURL only returns a WP_Error if the HTTP code is 100 AND the content is blank.

Scratch that, Its a error condition internally to curl, there's nothing to avoid that it seems, cURL will wait for a non-1xx response following a 1xx response.. which is pretty much what I expected.

A 1xx response "requires" a followup request response, from what I can see, it's never mean't to be returned by itself.


Lets looks at some rfc's now: http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

10.1.1 100 Continue

 The client SHOULD continue with its request. This interim response is used to inform the client that the initial part of the request has been received and has not yet been rejected by the server. The client SHOULD continue by sending the remainder of the request or, if the request has already been completed, ignore this response. The server MUST send a final response after the request has been completed. See section 8.2.3 for detailed discussion of the use and handling of this status code.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.3

Requirements for HTTP/1.1 clients: 
      - If a client will wait for a 100 (Continue) response before
        sending the request body, it MUST send an Expect request-header
        field (section 14.20) with the "100-continue" expectation.
      - A client MUST NOT send an Expect request-header field (section
        14.20) with the "100-continue" expectation if it does not intend
        to send a request body.

Requirements for HTTP/1.1 origin servers:
...<snip>...
      - An origin server that sends a 100 (Continue) response MUST
        ultimately send a final status code, once the request body is
        received and processed, unless it terminates the transport
        connection prematurely.

From that I conclude:

  • cURL is sending an Expect: 100-continue; header, in doing so it's allowing servers to respond with 100 messages
  • Servers should NOT just return a 100 code, if it does, and doesnt follow up with a valid response (ie. 200) It's breaking the spec and client should ignore the request
  • cURL is acting correctly here, Streams and fsockopen() shouldn't be accepting the 100 code like they are
  • 101 responses are to tell the client to make the request over a non-HTTP supported method, this is effectively a code which can be returned (the Upgrade header should be checked) however it's highly unlikely that any usage of the WP_HTTP API within WordPress is ever going to come accross a real-world usage of such a redirection (Stop using HTTP! Switch to FooBar procol version 1337 for continued usage!).

IMO, lets change streams and fsockopen to bail on 1xx responses.

#8 @sivel
13 years ago

I am fine with whatever for the 1xx response codes. My testing was pretty simplistic and may not have fully tested the 1xx response codes in their proper context anyway.

#9 follow-up: @dd32
13 years ago

My testing was pretty simplistic and may not have fully tested the 1xx response codes in their proper context anyway.

I literally, cannot test them in their correct context, PHP doesn't let me :)

I'm going to make Streams/fsockopen return the same as curl.

#10 @dd32
11 years ago

  • Keywords needs-patch added; dev-feedback removed

#11 @chriscct7
9 years ago

@dd32 any progress on that?

#12 @johnbillion
8 years ago

  • Keywords needs-unit-tests added
  • Owner set to johnbillion
  • Status changed from new to reviewing

@johnbillion
8 years ago

@johnbillion
8 years ago

#13 in reply to: ↑ 9 @johnbillion
8 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
  • Priority changed from normal to lowest

Replying to dd32:

I'm going to make Streams/fsockopen return the same as curl.

17010.diff brings the behaviour of Streams into line with cURL, by forcibly returning a WP_Error if the HTTP status code is a 1xx.

The unit test is part of the external-http group. Place the status.php file somewhere accessible and change the URL in the test_http_response_code_handling() test as necessary.

#14 @johnbillion
8 years ago

  • Owner johnbillion deleted

#15 @johnbillion
4 years ago

  • Resolution set to wontfix
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.