-
Notifications
You must be signed in to change notification settings - Fork 4.9k
SocketsHttpHandler: Add a switch to allow non-ascii headers #42978
SocketsHttpHandler: Add a switch to allow non-ascii headers #42978
Conversation
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackEncoder.cs
Show resolved
Hide resolved
Done |
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
Show resolved
Hide resolved
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/StaticHttpSettings.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/StaticHttpSettings.cs
Show resolved
Hide resolved
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/StaticHttpSettings.cs
Outdated
Show resolved
Hide resolved
src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs
Outdated
Show resolved
Hide resolved
src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs
Outdated
Show resolved
Hide resolved
src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs
Outdated
Show resolved
Hide resolved
src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs
Show resolved
Hide resolved
src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs
Show resolved
Hide resolved
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackEncoder.cs
Outdated
Show resolved
Hide resolved
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
Show resolved
Hide resolved
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/StaticHttpSettings.cs
Outdated
Show resolved
Hide resolved
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.
Change LGTM
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
Outdated
Show resolved
Hide resolved
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.
LGTM, thanks!
No test failures are related to the change. ( |
@danmosemsft can you help pushing this forward for servicing approval? |
@antonfirsov usual procedure - create a port PR, @karelz supports it, let me know.. |
Oops, this is the PR. Got used to seeing 3.1 in the title. |
src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs
Show resolved
Hide resolved
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.
Please double check that tests that set appcontext/set environment variables aren't affecting other tests in the same process.
All tests are isolated using |
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 see it - thanks
@danmosemsft now that it is approved -- can we merge it, or should we wait for someone to tell us to do that? |
You can go ahead and merge 5.0 PR's when marked servicing-approved plus the usual requirements - code reviewed and tests are good. |
I hope this is also true for 3.1-only servicing PR-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.
Little nits, otherwise LGTM, thanks!
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
Show resolved
Hide resolved
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
Show resolved
Hide resolved
Oops, my bad. For 3.1/2.1, we normally have @Anipik do the merge, as he knows when the branch is open. Let him know if this is ready to merge. |
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.
No Packaging changes required here. We can merge this once the feedback is addressed the ci is green
Ah yes that's the other reason it's good for @Anipik to merge the 2.1/3.1 changes -- because he helps check the packaging was done correctly - something we've gotten wrong more than once. |
@Anipik this PR is ready to merge: |
Fixes 2 issues: - HTTP/2 is enabled by default since 3.1, the docs did not reflect this change - Document the AllowLatin1Headers switch introduced in dotnet/corefx#42978
.NET Core's
SocketHttpHandler
does not allow sending non-ASCII characters in HTTP headers according to RFC, however in reality services accept/require other characters/encodings.In dotnet/runtime#38711 we are introducing a generic solution for .NET 5. This is a 3.1-only workaround exposing two switches to relax header validation globally before the first usage of
HttpClient
, enabling advanced users to send Latin-1 encoded headers:System.Net.Http.SocketsHttpHandler.AllowLatin1Headers
DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_ALLOWLATIN1HEADERS
Customer Impact
Unblock migration to .NET Core 3.1 for customers who need to send Latin-1 headers.
Testing
The PR adds tests (and minimal necessary test infrastructure changes) based on the ones in dotnet/runtime#39468 and dotnet/runtime#39169 to make sure these scenarios do not regress.
Validation: we sent private builds to the partner asking for this feature, and they confirmed that the change fixes their issue.
Risk
Low. The workaround is hidden behind a switch, our default behavior remains unchanged.