Skip to content
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

Avoid forcing parsing of DefaultRequestHeaders and from message.ToString() #49673

Merged
merged 3 commits into from
Mar 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ internal static void DumpHeaders(StringBuilder sb, params HttpHeaders?[] headers
{
if (headers[i] is HttpHeaders hh)
{
foreach (KeyValuePair<string, IEnumerable<string>> header in hh)
foreach (KeyValuePair<string, string[]> header in hh.EnumerateWithoutValidation())
{
foreach (string headerValue in header.Value)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,28 +279,29 @@ internal IEnumerable<KeyValuePair<string, string>> 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<KeyValuePair<string, IEnumerable<string>>> Members
Expand Down Expand Up @@ -348,6 +349,23 @@ private IEnumerator<KeyValuePair<string, IEnumerable<string>>> GetEnumeratorCore
}
}

internal IEnumerable<KeyValuePair<string, string[]>> EnumerateWithoutValidation()
{
if (_headerStore == null)
{
yield break;
}

foreach (KeyValuePair<HeaderDescriptor, object> header in _headerStore)
{
string[] values = TryGetHeaderValue(header.Key, out object? info) ?
GetValuesAsStrings(header.Key, info) :
Array.Empty<string>();

yield return new KeyValuePair<string, string[]>(header.Key.Name, values);
}
}

#endregion

#region IEnumerable Members
Expand Down Expand Up @@ -554,18 +572,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
{
Expand All @@ -580,18 +587,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.
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
// 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).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,27 @@ private sealed class SetOnFinalized
public sealed class SocketsHttpHandler_HttpProtocolTests : HttpProtocolTests
{
public SocketsHttpHandler_HttpProtocolTests(ITestOutputHelper output) : base(output) { }

[Fact]
public async Task DefaultRequestHeaders_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
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();
}
}, async server =>
{
List<string> headers = await server.AcceptConnectionSendResponseAndCloseAsync();
Assert.Contains(headers, header => header.Contains("Accept-Language: en-US,en;q=0.5"));
Assert.Contains(headers, header => header.Contains("From: invalidemail"));
});
}
}

[ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
Expand Down