-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[HTTP] Stricter header value validation #116634
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 implements stricter validation for HTTP header values by rejecting header values containing CR, LF, or NUL characters, in accordance with RFC9110. It also improves related error messages and adds tests for the new validation rules.
- Updated header validation logic throughout the HTTP and Cookie handlers.
- Improved error messages with clearer, RFC-compliant text.
- Added both negative and positive tests covering various header types and dangerous characters.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs | Added NUL character to reserved characters and updated value checks in cookies. |
src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs | Extended tests with additional dangerous header values. |
src/libraries/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs | Updated new-line check to include NUL characters. |
src/libraries/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs | Updated reason phrase validation to include NUL. |
src/libraries/System.Net.Http/src/System/Net/Http/Headers/*.cs | Replaced calls to CheckContainsNewLine with calls that also check for NUL. |
src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/*.cs | Adjusted WinHTTP header handling to throw more precise exceptions. |
src/libraries/System.Net.Http.WinHttpHandler/src/Resources/Strings.resx | Updated error message resource to reflect validation of NUL characters. |
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs | Added extensive tests for forbidden and valid header characters. |
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs:1130
- [nitpick] The method name 'ContainsNewLineOrNull' may be misleading since it does not return a boolean value but instead validates the header value by throwing an exception when a forbidden character is found. Consider renaming it to 'EnsureNoNewLineOrNul' or a similar name that better reflects its behavior.
internal static void ContainsNewLineOrNull(string? value)
Tagging subscribers to this area: @dotnet/ncl |
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.
I'm in favour of blocking null as you suggest.
Can you please also update the fuzzer to look for nulls?
https://github.com/dotnet/runtime/blob/main/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs
Outdated
Show resolved
Hide resolved
e4501b1
to
3299dda
Compare
da9d61e
to
e567445
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/ba-g unrelated and/or known issues |
Based on RFC:
Cookie
as well since that results in request headers.Changes here are up for discussion, this is just my opinionated solution to the problem.
cc @MihaZupan