From a45d8e0bc33d1ff0633266d0c39a5fe12cc8f4fa Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 15 Mar 2021 19:59:50 -0400 Subject: [PATCH 1/3] Avoid forcing parsing of DefaultRequestHeaders In the past, this was done because every request would parse all headers as part of sending them, so it was better to parse once then on every request. But we changed the behavior for sending in SocketsHttpHandler to actually respect TryAddWithoutValidation, such that we wouldn't forcefully validate unless the developer asked us to. This makes DefaultRequestHeaders consistent with that. --- .../System/Net/Http/Headers/HttpHeaders.cs | 24 ++++++------------- .../FunctionalTests/SocketsHttpHandlerTest.cs | 19 +++++++++++++++ 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index edb3077d6a0143..d7f47893a57e5b 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -554,18 +554,7 @@ internal virtual void AddHeaders(HttpHeaders sourceHeaders) object sourceValue = header.Value; if (sourceValue is HeaderStoreItemInfo info) { - if (!sourceHeaders.ParseRawHeaderValues(header.Key, info, removeEmptyHeader: false)) - { - // If after trying to parse source header values no value is left (i.e. all values contain - // invalid newline chars), delete it and skip to the next header. ParseRawHeaderValues takes - // a lock, and it'll only ever return false once for one thread, so we don't need to be - // concerned about concurrent removals on the HttpClient.DefaultRequestHeaders source. - sourceHeadersStore.Remove(header.Key); - } - else - { - AddHeaderInfo(header.Key, info); - } + AddHeaderInfo(header.Key, info); } else { @@ -580,18 +569,19 @@ private void AddHeaderInfo(HeaderDescriptor descriptor, HeaderStoreItemInfo sour { HeaderStoreItemInfo destinationInfo = CreateAndAddHeaderToStore(descriptor); - // We have custom header values. The parsed values are strings. + // Always copy raw values + destinationInfo.RawValue = CloneStringHeaderInfoValues(sourceInfo.RawValue); + if (descriptor.Parser == null) { - Debug.Assert((sourceInfo.RawValue == null) && (sourceInfo.InvalidValue == null), - "No raw or invalid values expected for custom headers."); - + // We have custom header values. The parsed values are strings. // Custom header values are always stored as string or list of strings. + Debug.Assert(sourceInfo.InvalidValue == null, "No invalid values expected for custom headers."); destinationInfo.ParsedValue = CloneStringHeaderInfoValues(sourceInfo.ParsedValue); } else { - // We have a parser, so we have to copy invalid values and clone parsed values. + // We have a parser, so we also have to copy invalid values and clone parsed values. // Invalid values are always strings. Strings are immutable. So we only have to clone the // collection (if there is one). diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index a3359f238f9d6e..55ac1293eae1da 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -112,6 +112,25 @@ private sealed class SetOnFinalized public sealed class SocketsHttpHandler_HttpProtocolTests : HttpProtocolTests { public SocketsHttpHandler_HttpProtocolTests(ITestOutputHelper output) : base(output) { } + + [Fact] + public async Task DefaultRequestHeaders_Invalid_SentUnparsed() + { + await LoopbackServer.CreateClientAndServerAsync(async uri => + { + using (HttpClient client = CreateHttpClient()) + { + client.DefaultRequestHeaders.TryAddWithoutValidation("Accept-Language", "en-US,en;q=0.5"); // validation would add spaces + + var m = new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }; + (await client.SendAsync(TestAsync, m)).Dispose(); + } + }, async server => + { + List headers = await server.AcceptConnectionSendResponseAndCloseAsync(); + Assert.Contains(headers, header => header.Contains("en-US,en;q=0.5")); + }); + } } [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))] From 1b3a8e13ebfc70ce362961654d7ad16359fd1397 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 15 Mar 2021 21:01:03 -0400 Subject: [PATCH 2/3] Avoid forcing parsing when ToString()'ing request and response messages This not only can change behavior in the debugger just by hovering over a message, it can change behavior at runtime due to the request/response messages getting logged. --- .../Net/Http/Headers/HeaderUtilities.cs | 2 +- .../System/Net/Http/Headers/HttpHeaders.cs | 54 ++++++++++++------- .../FunctionalTests/HttpRequestMessageTest.cs | 23 ++++---- .../FunctionalTests/SocketsHttpHandlerTest.cs | 2 +- 4 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs index 91967beab099c4..febc456e2fdfb4 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs @@ -357,7 +357,7 @@ internal static void DumpHeaders(StringBuilder sb, params HttpHeaders?[] headers { if (headers[i] is HttpHeaders hh) { - foreach (KeyValuePair> header in hh) + foreach (KeyValuePair header in hh.EnumerateWithoutValidation()) { foreach (string headerValue in header.Value) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index d7f47893a57e5b..5d459603fed302 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -279,28 +279,29 @@ internal IEnumerable> GetHeaderStrings() } } - internal string GetHeaderString(HeaderDescriptor descriptor, object? exclude = null) => - TryGetHeaderValue(descriptor, out object? info) ? - GetHeaderString(descriptor, info, exclude) : - string.Empty; - - private string GetHeaderString(HeaderDescriptor descriptor, object info, object? exclude = null) + internal string GetHeaderString(HeaderDescriptor descriptor, object? exclude = null) { - string[] values = GetValuesAsStrings(descriptor, info, exclude); - - if (values.Length == 1) + if (TryGetHeaderValue(descriptor, out object? info)) { - return values[0]; - } + string[] values = GetValuesAsStrings(descriptor, info, exclude); - // Note that if we get multiple values for a header that doesn't support multiple values, we'll - // just separate the values using a comma (default separator). - string? separator = HttpHeaderParser.DefaultSeparator; - if ((descriptor.Parser != null) && (descriptor.Parser.SupportsMultipleValues)) - { - separator = descriptor.Parser.Separator; + if (values.Length == 1) + { + return values[0]; + } + + // Note that if we get multiple values for a header that doesn't support multiple values, we'll + // just separate the values using a comma (default separator). + string? separator = HttpHeaderParser.DefaultSeparator; + if (descriptor.Parser != null && descriptor.Parser.SupportsMultipleValues) + { + separator = descriptor.Parser.Separator; + } + + return string.Join(separator, values); } - return string.Join(separator, values); + + return string.Empty; } #region IEnumerable>> Members @@ -348,6 +349,23 @@ private IEnumerator>> GetEnumeratorCore } } + internal IEnumerable> EnumerateWithoutValidation() + { + if (_headerStore == null) + { + yield break; + } + + foreach (KeyValuePair header in _headerStore) + { + string[] values = TryGetHeaderValue(header.Key, out object? info) ? + GetValuesAsStrings(header.Key, info) : + Array.Empty(); + + yield return new KeyValuePair(header.Key.Name, values); + } + } + #endregion #region IEnumerable Members diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs index 2929a612e79d7d..167d5a0504c962 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs @@ -204,18 +204,23 @@ public void ToString_DefaultAndNonDefaultInstance_DumpAllFields() rm.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("text/plain", 0.2)); rm.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("text/xml", 0.1)); + rm.Headers.TryAddWithoutValidation("Accept-Language", "en-US,en;q=0.5"); // validate this remains unparsed rm.Headers.Add("Custom-Request-Header", "value1"); rm.Content.Headers.Add("Custom-Content-Header", "value2"); - Assert.Equal( - "Method: PUT, RequestUri: 'http://a.com/', Version: 1.0, Content: " + typeof(StringContent).ToString() + ", Headers:" + Environment.NewLine + - "{" + Environment.NewLine + - " Accept: text/plain; q=0.2" + Environment.NewLine + - " Accept: text/xml; q=0.1" + Environment.NewLine + - " Custom-Request-Header: value1" + Environment.NewLine + - " Content-Type: text/plain; charset=utf-8" + Environment.NewLine + - " Custom-Content-Header: value2" + Environment.NewLine + - "}", rm.ToString()); + for (int i = 0; i < 2; i++) // make sure ToString() doesn't impact subsequent use + { + Assert.Equal( + "Method: PUT, RequestUri: 'http://a.com/', Version: 1.0, Content: " + typeof(StringContent).ToString() + ", Headers:" + Environment.NewLine + + "{" + Environment.NewLine + + " Accept: text/plain; q=0.2" + Environment.NewLine + + " Accept: text/xml; q=0.1" + Environment.NewLine + + " Accept-Language: en-US,en;q=0.5" + Environment.NewLine + + " Custom-Request-Header: value1" + Environment.NewLine + + " Content-Type: text/plain; charset=utf-8" + Environment.NewLine + + " Custom-Content-Header: value2" + Environment.NewLine + + "}", rm.ToString()); + } } [Theory] diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 55ac1293eae1da..37b3ce785bfd7c 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -114,7 +114,7 @@ public sealed class SocketsHttpHandler_HttpProtocolTests : HttpProtocolTests public SocketsHttpHandler_HttpProtocolTests(ITestOutputHelper output) : base(output) { } [Fact] - public async Task DefaultRequestHeaders_Invalid_SentUnparsed() + public async Task DefaultRequestHeaders_SentUnparsed() { await LoopbackServer.CreateClientAndServerAsync(async uri => { From 94df2acdad31a2f22d8d4a88062db2fd0967e58b Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 16 Mar 2021 07:19:44 -0400 Subject: [PATCH 3/3] Add another test --- .../tests/FunctionalTests/SocketsHttpHandlerTest.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 37b3ce785bfd7c..21e2b9c78a70dc 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -121,6 +121,7 @@ await LoopbackServer.CreateClientAndServerAsync(async uri => using (HttpClient client = CreateHttpClient()) { client.DefaultRequestHeaders.TryAddWithoutValidation("Accept-Language", "en-US,en;q=0.5"); // validation would add spaces + client.DefaultRequestHeaders.TryAddWithoutValidation("From", "invalidemail"); // would fail to parse if validated var m = new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }; (await client.SendAsync(TestAsync, m)).Dispose(); @@ -128,7 +129,8 @@ await LoopbackServer.CreateClientAndServerAsync(async uri => }, async server => { List headers = await server.AcceptConnectionSendResponseAndCloseAsync(); - Assert.Contains(headers, header => header.Contains("en-US,en;q=0.5")); + Assert.Contains(headers, header => header.Contains("Accept-Language: en-US,en;q=0.5")); + Assert.Contains(headers, header => header.Contains("From: invalidemail")); }); } }