Skip to content

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

Merged
merged 1 commit into from
Jun 5, 2025

Conversation

MihaZupan
Copy link
Member

  • Fix Cookie parser to block new line chars
  • Block escaped new lines in quoted strings (e.g. "Hello\\nWorld")
  • Improve the Host parser to block more invalid hosts.
    • We are creating a test uri string like $"http://{host}/", but weren't blocking characters like '?' or '#' that would end the host part
  • Fixed the broken percent decoding logic in Alt-Svc:
    • The offsets used were wrong if % wasn't the first character. E.g. A0A%AA decoded into A0A\n
    • Decoding used (high * 256) + low instead of * 16, breaking all values with a non-zero high nibble
    • Block decoding new line chars
  • Improved the headers fuzzer that discovered all of these ^

@MihaZupan MihaZupan added this to the 10.0.0 milestone Jun 4, 2025
@MihaZupan MihaZupan requested review from ManickaP, a team and Copilot June 4, 2025 21:40
Copy link
Contributor

@Copilot Copilot AI left a 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));

Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

// 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'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or '\0' ?

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants