Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test HTTP parsing #5102

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@annevk
Copy link
Member

annevk commented Mar 9, 2017

Closes #5064.

@wpt-pr-bot wpt-pr-bot added the http label Mar 9, 2017

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 9, 2017

@mnot this is a mess. I guess I can change the pass conditions for a couple of these (though not entirely clear if they all should outright fail or do something else), but I wonder how much browsers are willing to change here. IETF should get a bit more serious about testing I think.

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented Mar 9, 2017

Result in Safari TP Release 24 (Safari 10.2, WebKit 12604.1.6)

Subtest Results Messages
OK
HTTP/0.7 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
HTTP/1.0 280 PASS
HTTP/1.11 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
HTTP/1.2 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
HTTP/1.7 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
HTTP/2.0 280 PASS
HTTP/5.garbage 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
splitonspace 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
http 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
HttP/1.1 280 PASS
HTTP/garbage 280 FAIL promise_test: Unhandled rejection with value: object "TypeError: Type error"
<div id=log></div>
<script>
[
"HTTP/0.7 280",

This comment has been minimized.

Copy link
@mnot

mnot Mar 9, 2017

Contributor

HTTP/0 is effectively HTTP/0.9, which doesn't have a status code. So, this should fail in some fashion.

"HTTP/1.11 280",
"HTTP/1.2 280",
"HTTP/1.7 280",
"HTTP/2.0 280",

This comment has been minimized.

Copy link
@mnot

mnot Mar 9, 2017

Contributor

HTTP/!1 is a wire-incompatible with HTTP/1.x, so all bets are off for this and HTTP/5, etc. These should be errors, as should be plain "http" and "splitonspace".

"HTTP/5.garbage 280",
"splitonspace 280",
"http 280",
"HttP/1.1 280",

This comment has been minimized.

Copy link
@mnot

mnot Mar 9, 2017

Contributor

http-version is case-sensitive; this should be an error.

<script>
[
"HTTP/0.7 280",
"HTTP/1.0 280",

This comment has been minimized.

Copy link
@mnot

mnot Mar 9, 2017

Contributor

syntactically, a reason-phrase is required; you might want to include them so you don't get false negatives.

@mnot

This comment has been minimized.

Copy link
Contributor

mnot commented Mar 9, 2017

There isn't any intention of introducing new versions of HTTP/1, because this phenomenon has been observed for a long time; there are lots of bits of software that get this wrong (not just browsers).

See also: https://tools.ietf.org/html/rfc2145#section-2

@mnot

This comment has been minimized.

Copy link
Contributor

mnot commented Mar 9, 2017

All of that said, I understand that there are some situations where browsers need to treat protocols specially, e.g., for ICECAST and a few others. @mcmanus might have some thoughts here.

@mcmanus

This comment has been minimized.

Copy link

mcmanus commented Mar 9, 2017

most of these non HTTP/1 cases are being interpreted as HTTP 0.9... which is fair enough - they aren't HTTP/1 and there is no version negotiation outside of the TCP stream. There is no response code in 0.9 so calling it 200 or parsing it and calling it 280 all seem fair game. Not a useful test.

The exception is the HTTP/2 case (in what is obviously not RFC 7540 HTTP2..).. as mark notes, that's just a bugwards accommodation for shoutcast to treat that as http/1. Similarly allowing mixed-case HttP is something that a receiver is just going to do for legacy interop. We've cleaned it up going forward

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 10, 2017

Right, it seems suspicious that all user agents support HTTP/2.0 and case-insensitive mostly worked.

There's also a difference between returning a network error and continuing to treat as HTTP/0.9 (the latter is what Firefox does and Edge maybe (although Edge uses the status code 520), the former is more like Safari and Chrome (although Chrome only does one scenario that way)).

So if some of this is required it would be good if the HTTP specification actually said so. Otherwise it's hard to come up with pass/fail criteria here and get browsers to align on how they handle pass/fail.

Test HTTP parsing
Closes #5064.

@annevk annevk force-pushed the annevk/http-versioning branch from 7dcde16 to 7606bc9 May 14, 2018

@wpt-pr-bot

This comment has been minimized.

Copy link
Collaborator

wpt-pr-bot commented May 14, 2018

There are no owners for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.