-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Remove support for deprecated obs-fold in header values #53505
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsRFC7230 deprecated support for line-folding in header values (emphasis mine):
Previously headers like This PR removes obs-fold support from header validation, instead treating all new line characters as invalid. On the receiving side, servers
Kestrel will treat the whole request as invalid and close the connection when receiving such a header. Fixes #50597
|
What's the motivation for doing this? I'm concerned that we will break some customer that is relying on this. |
e35f6ec
to
24349a5
Compare
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.
Actually, I think this is missing some relevant changes.
HttpRuleParser.GetWhitespaceLength should be updated.
Note: the change may seem huge, but it's 95% test changes Product changes:
*after #52794, setting the |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Product changes LGTM, and I skimmed the tests. Do we have tests that validate all forms of newline are still allowed with TryAddWithoutValidation and NonValidated headers?
Yes, I added a bunch of new-line combinations to this runtime/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs Lines 332 to 334 in 3f3e304
|
I'm looking at the tests you referenced @MihaZupan and it looks like all forms of newline are NOT allowed with TryAddWithoutValidation. In particular it looks like the HeaderValuesWithNewLines enumerable is explicitly testing that we disallow these. Am I missing something here? I thought the idea was to continue to allow obs-fold when using TryAddWithoutValidation but to restrict it elsewhere. Is this broken now? |
@geoffkizer We continue to do no validation with TryAddWithoutValidation. The test name is bad, but we are testing that new lines added without validation will be present when we enumerate without validation. runtime/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs Line 344 in 3f3e304
It is also testing that accessing the header with validation will result in its removal. |
Why is it removed if it is a valid header with embedded obs-fold? |
If you access a header with validation and we consider it as invalid, it will be removed from the collection. This has been the existing behavior. The change is that now we treat any new lines as invalid - even if it would have been a valid obs fold. In other words, you can only work with obs-folds if you add them without validation and read them without validation. What behavior did you have in mind as desired here? |
RFC7230 deprecated support for line-folding in header values (emphasis mine):
Previously headers like
foo: a\r\n b
were considered valid (space following new line).This PR removes obs-fold support from header validation, instead treating all new line characters as invalid.
On the receiving side, servers
MAY
accept obs-fold as long as they replace new lines with spaces.Kestrel will treat the whole request as invalid and close the connection when receiving such a header.
HttpClient today does accept them in responses - this PR does not change that.
Fixes #50597