Opened 14 years ago
Closed 5 years ago
#17010 closed defect (bug) (wontfix)
Inconsistent handling of HTTP response codes
| Reported by: |
|
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)
Change History (17)
#2
follow-up:
↓ 4
@
14 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.
#3
@
14 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
@
14 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.
#6
@
14 years ago
It looks like cURL only returns a WP_Error if the HTTP code is 100 AND the content is blank.
#7
@
14 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
@
14 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:
↓ 13
@
14 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.
#12
@
9 years ago
- Keywords needs-unit-tests added
- Owner set to johnbillion
- Status changed from new to reviewing
#13
in reply to:
↑ 9
@
9 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.
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.