Fix exception when parsing HTTP/2 status line#15203
Conversation
davidsh
added
area-System.Net.Http
os-linux
|
cc @justinvp who wrote the original tests ;) |
I believe the tests were originally written by @kapilash (I just moved/tweaked them at some point). |
Is this a valid syntax for http protocol? In the RFC 7230 the protocol describes the syntax should be, |
|
That's the RFC for HTTP/1.1.
HTTP/2 actually doesn't have that textual representation at all, because it's a binary format. Instead, the status line is replaced with a ":status" pseudo-header, see https://tools.ietf.org/html/rfc7540 section 8.1.2.4.
I assume that for ease of backwards compatibility, cURL is converting the HTTP/2 response into a HTTP/1.x-like format, which is then what System.Net.Http is parsing.
Therefore, this isn't so much "what does the spec say" as much as "what does curl do", because this is now completely outside of the HTTP spec.
|
There was a problem hiding this comment.
I believe this PR should be OK to merge unless you strictly require a unit test. In this area, I'm unsure as to how to modify CurlResponseParseUtilsTest and the MemberData provided to the test in order to create new test cases for HTTP/2 for all tests. Can I get a hand with including HTTP/2 unit tests?
You can add the test by having a new StatusCodeMajorVersionOnlyFormat = HTTP/{0} 200 OK", and create memberdata for this format, through a method that generates major version from 1 to 10, and then write a new test like ReadStatusLine_ValidStatusCodeLine_ResponseMessageVersionSet test method with only major number.
Extend existing tests to cover status line with major version only. HttpResponseMessage.Version should still have a value of (2, 0) to ensure compatibility with Windows (see HttpVersionInternal) and full .NET Framework, not a value of just (2).
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the improvement and the tests to go with it.
|
@dotnet-bot test outerloop Ubuntu14.04 Debug please |
|
+1 on thanks for the improvement! |
|
Thanks, everyone! |
* Fix exception when parsing HTTP/2 status line. * Extend curl tests to cover "HTTP/2" status line Extend existing tests to cover status line with major version only. HttpResponseMessage.Version should still have a value of (2, 0) to ensure compatibility with Windows (see HttpVersionInternal) and full .NET Framework, not a value of just (2). Commit migrated from dotnet/corefx@8ddccab
8 participants
Related issue: #14715
It appears that
CurlResponseHeaderReaderand it's tests always assume that the HTTP status line starts with a stringHTTP/x.ywherexandyare the HTTP major and minor version numbers, respectively.For HTTP 2, the status line is
HTTP/2followed by a space and then the rest of the line. There is no.character to delimit between major and minor versions.I believe this PR should be OK to merge unless you strictly require a unit test. In this area, I'm unsure as to how to modify
CurlResponseParseUtilsTestand theMemberDataprovided to the test in order to create new test cases forHTTP/2for all tests. Can I get a hand with including HTTP/2 unit tests?Thanks.