Skip to content

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 a team, ManickaP and Copilot June 4, 2025 21:40
Copy link
Contributor

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));

@dotnet-policy-service
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants