Fix DownloadHeader for real servers - #8
Merged
Merged
Conversation
DownloadHeader threw a FormatException for practically every server. It built the header dictionary with `v.Value.ToString()` on an IEnumerable<string>, which yields "System.String[]", and HttpHeader then tried to parse that as the Date header. TryDownloadHeader swallowed the exception and returned false. - read the values of the headers instead of the type name; join several values with ", " - also read the content headers (Content-Length, Content-Type, Last-Modified, ...); they are not part of the response headers, so these properties were never set - look header names up case-insensitively in HttpHeader (HTTP/2 sends them in lower case); RawDictionary still is the dictionary that was passed in - dispose the response Tests - DownloadHeaderTest and HttpHeaderTest use a fake server, no internet needed - HttpChannelTest gets two tests against a real server (network trait) - tests that use the process-wide SharedHttpClient run in one xunit collection so they cannot race
AddHeaders joined every header with several values with ", ". That is not valid for Set-Cookie (RFC 6265, section 3; RFC 9110, section 5.3): a comma can be part of a cookie, e.g. in "Expires=Wed, 21 Oct 2015 07:28:00 GMT", so the boundary between two cookies got lost. - several Set-Cookie values are joined with the new HttpHeader.SetCookieSeparator (a line feed, which can never be part of a header value); this applies to SetCookie and to RawDictionary - add HttpHeader.SetCookies, one entry per cookie (a new member of the class, IHttpHeader is unchanged) - every other header with several values is still joined with ", "
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HttpChannelExt.DownloadHeaderthrew aFormatExceptionfor practically every real server, so it never worked outside of tests with hand-made dictionaries.TryDownloadHeaderswallowed the exception and returnedfalse.Cause
The header dictionary was built with
v.Value.ToString()on anIEnumerable<string>, which yields"System.String[]".HttpHeaderthen parsed that text as theDateheader and threw.Changes
", ".Content-Length,Content-TypeandLast-Modifiedare part ofContent.Headers, not of the response headers, so these properties were never set.HttpHeaderlooks header names up case-insensitively (HTTP/2 sends them in lower case).RawDictionaryis still the dictionary that was passed in.The public API is unchanged.
Tests
DownloadHeaderTestandHttpHeaderTestuse a fake server, no internet needed.HttpChannelTestgets two tests against a real server (Category=Network, non-blocking in CI).SharedHttpClientrun in one xunit collection (SharedHttpClientCollection) so they cannot race.Verification
FormatException,TryDownloadHeaderreturnsfalse); with the fix they passnet10.0andnet8.0(local, macOS), 25 repeated runs without a failureNot part of this PR
DownloadToStringignores itsencodingparameter and does not check the status code. Both change results, so they are left for the network work package of the 13.0 plan.