-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix various HTTP header parsing issues #116323
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses several HTTP header parsing issues by improving invalid character detection and correcting percent-decoding logic, along with adding new tests to validate these changes.
- Fixes in the Cookie, Host, and Alt-Svc header parsers
- Additional test cases to assert invalid inputs, including new-line characters and disallowed host characters
- Updates to fuzzing support and minor documentation corrections
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/libraries/System.Net.Http/tests/UnitTests/System.Net.Http.Unit.Tests.csproj | Added Cookie header parser tests |
src/libraries/System.Net.Http/tests/UnitTests/HttpRuleParserTest.cs | Added tests for invalid quoted pairs including new line characters |
src/libraries/System.Net.Http/tests/UnitTests/Headers/GenericHeaderParserTest/HostParserTest.cs | Expanded invalid host tests with more test cases |
src/libraries/System.Net.Http/tests/UnitTests/Headers/CookieHeaderParserTest.cs | Introduced new tests for Cookie header parsing |
src/libraries/System.Net.Http/tests/UnitTests/Headers/AltSvcHeaderParserTest.cs | Updated tests for Alt-Svc header percent decoding and invalid inputs |
src/libraries/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs | Added disallowed characters for host parsing and refined new line checks |
src/libraries/System.Net.Http/src/System/Net/Http/Headers/CookieHeaderParser.cs | Updated Cookie header parser to block new line characters |
src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs | Fixed percent-decoding logic by correcting the bit shift and index usage |
src/libraries/Fuzzing/README.md | Minor documentation updates for build instructions |
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs | Expanded header validations in the fuzzer |
src/libraries/Fuzzing/.gitignore | Extended ignore patterns for fuzzing-related artifacts |
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Http/tests/UnitTests/Headers/GenericHeaderParserTest/HostParserTest.cs:58
- Please add a comment explaining why the expected starting index is 1 for input " host\r\n ", to clarify the test intent for future maintainers.
CheckInvalidParsedValue(" host\r\n ", 1);
src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs:269
- The correction from using '<< 8' to '<< 4' properly shifts the high nibble, fixing the percent-decoding logic. Confirm that this change aligns with the intended behavior for all valid hexadecimal values.
builder.Append((char)((hi << 4) | lo));
Tagging subscribers to this area: @dotnet/ncl |
// If so, check whether the character is in the range 0-127. If not, it's an invalid value. | ||
if ((startIndex + 2 > input.Length) || (input[startIndex + 1] > 127)) | ||
// If so, check whether the character is in the range 0-127 and not a new line. Otherwise, it's an invalid value. | ||
if ((startIndex + 2 > input.Length) || (input[startIndex + 1] is > (char)127 or '\r' or '\n')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or '\0' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not currently blocking null specifically elsewhere. If we want to start also filtering those out, yes, this would be one of the places to update.
But there are more places we'd also have to update (essentially anywhere we're checking for new lines right now). I'd leave that to a follow-up PR.
We could choose to move such validation to a more central place that runs for every header instead of having it split and hoping that all of the implementations are doing similar things.
Cookie
parser to block new line chars"Hello\\nWorld"
)Host
parser to block more invalid hosts.$"http://{host}/"
, but weren't blocking characters like'?'
or'#'
that would end the host partAlt-Svc
:%
wasn't the first character. E.g.A0A%AA
decoded intoA0A\n
(high * 256) + low
instead of* 16
, breaking all values with a non-zero high nibble