Fix fragment handling in HttpClient #27360
Conversation
SocketsHttpHandler isn't sending fragments, nor is it properly inheriting the fragment from the original request URI into the redirect location URI when the original URI had one and the redirect URI did not, even though RFC 7231 says it must. This commit fixes that for SocketsHttpHandler. WinHttpHandler also isn't handling this inheritance according to the RFC. It appears that the logic for WinHttpHandler would actually need to be changed in WINHTTP itself, or else WinHttpHandler would need to be changed to do the redirects itself. Neither CurlHandler or NetFxHandler send fragments at all. This commit also fixes the test to correctly compare the expected and actual Uris... apparently Uri equality doesn't factor in fragments, so they're first converted to strings. It also updates the test to also validate that the server received the URI with the fragment included.
davidsh
added
area-System.Net.Http
area-System.Net.Http.SocketsHttpHandler
and removed
area-System.Net.Http
labels
| Assert.NotEmpty(secondRequest.Result); | ||
| string[] statusLineParts = secondRequest.Result[0].Split(' '); | ||
| Assert.Equal(3, statusLineParts.Length); | ||
| Assert.Equal(expectedUrl.GetComponents(UriComponents.PathAndQuery | UriComponents.Fragment, UriFormat.SafeUnescaped), statusLineParts[1]); |
There was a problem hiding this comment.
Nit: shouldn't matter for this test, but shouldn't this technically use UriFormat.Escaped?
|
LGTM, one minor nit above |
|
@davidsh, why did you remove the System.Net.Http label? This applies to all handlers. There's a fix in SocketsHttpHandler, but the test is for all of them, and the others all suffer from the same or similar issues. |
|
@dotnet-bot test Outerloop Windows x64 Debug Build please |
I based the label on where the produce code changes were made primarily which was SocketsHttpHandler. Doing this helps group all the PRs related to changes in SocketNetHttpHandler. I don't have a strong opinion on this, so if you want, the label can be changed to just the generic "Http" label. |
Apparently it sometimes doesn't do the "right thing" even in other cases.
|
@dotnet-bot test Outerloop Windows x64 Debug Build please please |
* Fix fragment handling in HttpClient SocketsHttpHandler isn't sending fragments, nor is it properly inheriting the fragment from the original request URI into the redirect location URI when the original URI had one and the redirect URI did not, even though RFC 7231 says it must. This commit fixes that for SocketsHttpHandler. WinHttpHandler also isn't handling this inheritance according to the RFC. It appears that the logic for WinHttpHandler would actually need to be changed in WINHTTP itself, or else WinHttpHandler would need to be changed to do the redirects itself. Neither CurlHandler or NetFxHandler send fragments at all. This commit also fixes the test to correctly compare the expected and actual Uris... apparently Uri equality doesn't factor in fragments, so they're first converted to strings. It also updates the test to also validate that the server received the URI with the fragment included. * Disable the fragments test on WinHttpHandler Apparently it sometimes doesn't do the "right thing" even in other cases. Commit migrated from dotnet/corefx@a273be6
SocketsHttpHandler isn't sending fragments, nor is it properly inheriting the fragment from the original request URI into the redirect location URI when the original URI had one and the redirect URI did not, even though RFC 7231 says it must. This commit fixes that for SocketsHttpHandler.
WinHttpHandler also isn't handling this inheritance according to the RFC. It appears that the logic for WinHttpHandler would actually need to be changed in WINHTTP itself, or else WinHttpHandler would need to be changed to do the redirects itself.
Neither CurlHandler or NetFxHandler send fragments at all.
This commit also fixes the test to correctly compare the expected and actual Uris... apparently Uri equality doesn't factor in fragments, so they're first converted to strings. It also updates the test to also validate that the server received the URI with the fragment included.
Closes https://github.com/dotnet/corefx/issues/27305
cc: @geoffkizer, @davidsh, @wfurt, @rmkerr