Don't validate/materialize HTTP headers when logging them#132010
Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/Microsoft.Extensions.Http/src/Logging/HttpHeadersLogValue.cs:91
- Under
#if NET,headers.NonValidatedalready materializes values as either a single string or astring[]viaHttpHeaders.GetStoreValuesAsStringOrStringArray(...). The current implementation allocates a newstring[]and copies all values again, which is redundant per-header allocation/copy on the Trace logging path.
Consider storing the HeaderStringValues directly (it remains non-validating and enumerates the same raw strings) to avoid the extra array allocation and copy.
string[] headerValues = new string[kvp.Value.Count];
int i = 0;
foreach (string value in kvp.Value)
{
headerValues[i++] = value;
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/libraries/Microsoft.Extensions.Http/src/Logging/HttpHeadersLogValue.cs:85
- Even when a header name should be redacted,
AddHeaderscurrently stores the real header value inValues. BecauseHttpHeadersLogValueis used as anILoggerstate object, structured loggers can serialize these values regardless of theToString()implementation.
Consider passing the redaction predicate into AddHeaders and storing "*" for redacted headers (and in the NET case, avoid calling HeaderStringValues.ToString() when redacted).
// Enumerate the headers without triggering validation/parsing of the values, so that logging
// doesn't alter how the headers are subsequently serialized on the wire.
private static void AddHeaders(List<KeyValuePair<string, object>> values, HttpHeaders headers)
{
#if NET
foreach (KeyValuePair<string, HeaderStringValues> kvp in headers.NonValidated)
{
values.Add(new KeyValuePair<string, object>(kvp.Key, kvp.Value.ToString()));
}
#else
foreach (KeyValuePair<string, IEnumerable<string>> kvp in headers)
{
values.Add(new KeyValuePair<string, object>(kvp.Key, kvp.Value));
}
#endif
}
src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/Logging/HttpHeadersLogValueTest.cs:70
- The existing redaction test only validates
HttpHeadersLogValue.ToString(). Since this type is passed asILoggerstate, structured loggers (e.g., JSON console) can enumerate the state and emit the key/value pairs directly. To ensureRedactLoggedHeadersactually redacts in logs, add an assertion/test that enumeratingHttpHeadersLogValueyields"*"for redacted headers (not the original value).
#if NET
[Fact]
public void HttpHeadersLogValue_DoesNotValidateHeaderValues()
{
var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com");
request.Headers.TryAddWithoutValidation("Accept", "application/vnd.example+json;version=1");
var httpHeadersLogValue = new HttpHeadersLogValue(HttpHeadersLogValue.Kind.Request, request.Headers, contentHeaders: null, _ => false);
Assert.Equal(
"Request Headers:" + Environment.NewLine +
"Accept: application/vnd.example+json;version=1" + Environment.NewLine,
httpHeadersLogValue.ToString());
Assert.True(request.Headers.NonValidated.TryGetValues("Accept", out HeaderStringValues values));
Assert.Equal("application/vnd.example+json;version=1", Assert.Single(values));
}
#endif
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/Microsoft.Extensions.Http/src/Logging/HttpHeadersLogValue.cs:81
- This change also alters the structured logging payload: in NET builds the per-header Value is now always a single joined string (or "*") rather than an IEnumerable. Providers/consumers that previously treated these as arrays (or cast to IEnumerable) will observe different structured data even though the formatted message stays the same. If preserving the enumerable shape is important, the implementation likely needs an alternative representation (e.g., keep IEnumerable for structured state and only join when formatting).
// Enumerate the headers without triggering validation/parsing of the values, so that logging
// doesn't alter how the headers are subsequently serialized on the wire.
private void AddHeaders(List<KeyValuePair<string, object>> values, HttpHeaders headers)
{
#if NET
foreach (KeyValuePair<string, HeaderStringValues> kvp in headers.NonValidated)
{
object value = _shouldRedactHeaderValue(kvp.Key) ? RedactedValue : kvp.Value.ToString();
values.Add(new KeyValuePair<string, object>(kvp.Key, value));
}
src/libraries/Microsoft.Extensions.Http/src/Logging/HttpHeadersLogValue.cs:79
- In the NET path, using HeaderStringValues.ToString() will join multiple header values using the header-specific separator (e.g., Cookie uses "; "), whereas the previous implementation always joined with ", ". That means the formatted log text can change for multi-valued headers like Cookie, which seems to contradict the PR goal/description of keeping rendered log output unchanged.
foreach (KeyValuePair<string, HeaderStringValues> kvp in headers.NonValidated)
{
object value = _shouldRedactHeaderValue(kvp.Key) ? RedactedValue : kvp.Value.ToString();
values.Add(new KeyValuePair<string, object>(kvp.Key, value));
…TFMs Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/Microsoft.Extensions.Http/src/Logging/HttpHeadersLogValue.cs:90
- This change makes the structured logging payload store each header value as a single joined
string(and the ToString path now casts(string)kvp.Value). Previously the payload exposedIEnumerable<string>per header, so structured-log consumers could access individual values without re-parsing the formatted string. This is a user-observable behavior change for providers that capture state, and it could break consumers that cast the value toIEnumerable<string>.
Consider preserving the previous structured shape (e.g., store HeaderStringValues / IEnumerable<string> as the object value and only join during formatting) while still enumerating via headers.NonValidated to avoid validation.
string value = _shouldRedactHeaderValue(kvp.Key)
? RedactedValue
#if NET
: kvp.Value.ToString();
#else
: string.Join(", ", kvp.Value);
#endif
values.Add(new KeyValuePair<string, object>(kvp.Key, value));
}
src/libraries/Microsoft.Extensions.Http/src/Logging/HttpHeadersLogValue.cs:88
- On NET,
HeaderStringValues.ToString()joins multi-values using the header-specific separator (HeaderStringValues.cs:45-50), which isn't always", "(e.g.,User-Agent/Serveruse whitespace perProductInfoHeaderParser.cs:64-65). That means this change can alter the rendered log text for multi-valued headers, contradicting the PR description that the formatted output is unchanged. If keeping the exact previous formatting is required, join with", "explicitly when constructing the per-header string.
This issue also appears on line 82 of the same file.
string value = _shouldRedactHeaderValue(kvp.Key)
? RedactedValue
#if NET
: kvp.Value.ToString();
#else
: string.Join(", ", kvp.Value);
#endif
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
HttpHeadersLogValueenumeratedHttpHeadersdirectly, which forces lazy parsing of known headers. WithLogLevel.Traceenabled forMicrosoft.Extensions.Http, merely logging a request re-serialized values added viaTryAddWithoutValidation:Changes
HttpHeaders.NonValidated— header enumeration moves into anAddHeadershelper that uses the non-validated view, so logging no longer triggers parsing.netstandard2.0/net462keep the previous enumeration under#else, sinceNonValidatedis .NET 5+.HeaderStringValues.ToString()per header — it already joins multiple values with", ", matching the previousAppendJoin(", ", ...)output, so the rendered log text is unchanged.HttpHeadersNonValidated.Count.HttpHeadersLogValueTestasserting both the formatted output and thatNonValidatedstill returns the raw value after formatting.Note for reviewers
On .NET, the
objectvalue in the exposedIReadOnlyList<KeyValuePair<string, object>>changes fromIEnumerable<string>to a single joinedstring. Message text is identical, but a structured-logging consumer casting the value toIEnumerable<string>would be affected. Happy to preserve the enumerable shape instead if that tradeoff isn't acceptable.