Skip to content

Commit e557330

Browse files
antonfirsovantonfirsov
committed
Harmonize redaction:
- Use a shared switch to opt-out from redaction entirely (System.Net.Http.DisableUriRedaction) - Simplify and optimize redaction logic, always omit Fragment when redacting - Adjust and consolidate tests
1 parent e711ec9 commit e557330

2 files changed

Lines changed: 61 additions & 92 deletions

File tree

‎src/libraries/Microsoft.Extensions.Http/src/Logging/LogHelper.cs‎

Lines changed: 21 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace Microsoft.Extensions.Http.Logging
1414
internal static class LogHelper
1515
{
1616
private static readonly LogDefineOptions s_skipEnabledCheckLogDefineOptions = new LogDefineOptions() { SkipEnabledCheck = true };
17-
private static readonly bool s_disableUriQueryRedaction = GetDisableUriQueryRedactionSettingValue();
17+
private static readonly bool s_disableUriRedaction = GetDisableUriRedactionSettingValue();
1818

1919
private static class EventIds
2020
{
@@ -54,14 +54,14 @@ private static class EventIds
5454
EventIds.PipelineEnd,
5555
"End processing HTTP request after {ElapsedMilliseconds}ms - {StatusCode}");
5656

57-
private static bool GetDisableUriQueryRedactionSettingValue()
57+
private static bool GetDisableUriRedactionSettingValue()
5858
{
59-
if (AppContext.TryGetSwitch("Microsoft.Extensions.Http.DisableUriQueryRedaction", out bool value))
59+
if (AppContext.TryGetSwitch("System.Net.Http.DisableUriRedaction", out bool value))
6060
{
6161
return value;
6262
}
6363

64-
string? envVar = Environment.GetEnvironmentVariable("DOTNET_MICROSOFT_EXTENSIONS_HTTP_DISABLEURIQUERYREDACTION");
64+
string? envVar = Environment.GetEnvironmentVariable("DOTNET_SYSTEM_NET_HTTP_DISABLEURIREDACTION");
6565

6666
if (bool.TryParse(envVar, out value))
6767
{
@@ -151,6 +151,11 @@ public static void LogRequestPipelineEnd(this ILogger logger, HttpResponseMessag
151151
return null;
152152
}
153153

154+
if (s_disableUriRedaction)
155+
{
156+
return uri.IsAbsoluteUri ? uri.AbsoluteUri : uri.ToString();
157+
}
158+
154159
if (!uri.IsAbsoluteUri)
155160
{
156161
// We cannot guarantee the redaction of UserInfo for relative Uris without implementing some subset of Uri parsing in this package.
@@ -160,45 +165,25 @@ public static void LogRequestPipelineEnd(this ILogger logger, HttpResponseMessag
160165
return "*";
161166
}
162167

163-
string uriString = uri.GetComponents(UriComponents.AbsoluteUri & ~UriComponents.UserInfo, UriFormat.UriEscaped);
168+
string pathAndQuery = uri.PathAndQuery;
169+
int queryIndex = pathAndQuery.IndexOf('?');
164170

165-
if (s_disableUriQueryRedaction)
166-
{
167-
return uriString;
168-
}
171+
bool redactQuery = queryIndex >= 0 && // Query is present.
172+
queryIndex < pathAndQuery.Length - 1; // Query is not empty.
169173

170-
int fragmentOffset = uriString.IndexOf('#');
171-
int queryOffset = uriString.IndexOf('?');
172-
if (fragmentOffset >= 0 && queryOffset > fragmentOffset)
174+
return (redactQuery, uri.IsDefaultPort) switch
173175
{
174-
// Not a query delimiter, but a '?' in the fragment.
175-
queryOffset = -1;
176-
}
177-
178-
if (queryOffset < 0 || queryOffset == uriString.Length - 1 || queryOffset == fragmentOffset - 1)
179-
{
180-
// No query or empty query.
181-
return uriString;
182-
}
176+
(true, true) => $"{uri.Scheme}://{uri.Host}{GetPath(pathAndQuery, queryIndex)}*",
177+
(true, false) => $"{uri.Scheme}://{uri.Host}:{uri.Port}{GetPath(pathAndQuery, queryIndex)}*",
178+
(false, true) => $"{uri.Scheme}://{uri.Host}{pathAndQuery}",
179+
(false, false) => $"{uri.Scheme}://{uri.Host}:{uri.Port}{pathAndQuery}"
180+
};
183181

184-
if (fragmentOffset < 0)
185-
{
186-
#if NET
187-
return $"{uriString.AsSpan(0, queryOffset + 1)}*";
188-
#else
189-
return $"{uriString.Substring(0, queryOffset + 1)}*";
190-
#endif
191-
}
192-
else
193-
{
194182
#if NET
195-
ReadOnlySpan<char> uriSpan = uriString.AsSpan();
196-
return $"{uriSpan[..(queryOffset + 1)]}*{uriSpan[fragmentOffset..]}";
183+
static ReadOnlySpan<char> GetPath(string pathAndQuery, int queryIndex) => pathAndQuery.AsSpan(0, queryIndex + 1);
197184
#else
198-
return $"{uriString.Substring(0, queryOffset + 1)}*{uriString.Substring(fragmentOffset)}";
185+
static string GetPath(string pathAndQuery, int queryIndex) => pathAndQuery.Substring(0, queryIndex + 1);
199186
#endif
200-
}
201-
202187
}
203188
}
204189
}

‎src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/Logging/LoggingUriOutputTests.cs‎

Lines changed: 40 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -29,69 +29,68 @@ private static class EventIds
2929
{
3030
{ null, null },
3131
{ "http://q.app/foo", "http://q.app/foo" },
32+
{ "http://q.app:123/foo", "http://q.app:123/foo" },
3233
{ "http://user:xxx@q.app/foo", "http://q.app/foo" }, // has user info
3334
{ "http://q.app/foo?", "http://q.app/foo?" },
3435
{ "http://q.app/foo?XXX", "http://q.app/foo?*" },
3536
{ "http://q.app/a/b/c?a=b%20c&x=1", "http://q.app/a/b/c?*" },
36-
{ "http://q.app/#", "http://q.app/#" }, // Has empty fragment.
37-
{ "http://q.app#f", "http://q.app/#f" }, // Has fragment.
38-
{ "http://q.app#f?a=b", "http://q.app/#f?a=b" }, // Has fragment with a '?'.
39-
{ "http://q.app/?a=b#f?a=b", "http://q.app/?*#f?a=b" }, // Has query and fragment with a '?'.
40-
{ "http://q.app?#f", "http://q.app/?#f" }, // Has empty query and fragment.
37+
{ "http://q.app:4242/a/b/c?a=b%20c&x=1", "http://q.app:4242/a/b/c?*" },
4138
{ "/cat/1/2", "*" }, // Relative Uris are fully redacted.
4239
{ "/cat/1/2?a=b%20c&x=1", "*" },
4340
};
4441

4542
[Theory]
4643
[MemberData(nameof(GetRedactedUriString_Data))]
47-
public void GetRedactedUriString(string original, string expected)
44+
public void GetRedactedUriString_RedactsUriByDefault(string original, string expected)
4845
{
4946
Uri? uri = original != null ? new Uri(original, UriKind.RelativeOrAbsolute) : null;
5047
string? actual = LogHelper.GetRedactedUriString(uri);
5148

5249
Assert.Equal(expected, actual);
5350
}
5451

55-
public static TheoryData<bool, bool, string> Handlers_LogExpectedUri_Data()
52+
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
53+
[InlineData("AppCtx")] // AppContext switch System.Net.Http.DisableUriRedaction = true
54+
[InlineData("EnvVar1")] // Env. var DOTNET_SYSTEM_NET_DISABLEURIREDACTION = "1"
55+
[InlineData("EnvVarTrue")] // Env. var DOTNET_SYSTEM_NET_DISABLEURIREDACTION = "true"
56+
public void GetRedactedUriString_DisableUriRedaction_DoesNotRedactUri(string queryRedactionDisabler)
5657
{
57-
TheoryData<bool, bool, string> result = new();
58-
bool[] booleans = { true, false };
59-
bool[] syncApiVals =
60-
#if NET
61-
booleans;
62-
#else
63-
{ false };
64-
#endif
65-
foreach (bool syncApi in syncApiVals)
58+
RemoteExecutor.Invoke(static queryRedactionDisabler =>
6659
{
67-
foreach (bool scopeHandler in booleans)
60+
switch (queryRedactionDisabler)
6861
{
69-
// valid values for logQueryStringEnabler:
70-
// "" - Do not enable query string logging.
71-
// "AppCtx" - Enable via AppContext switch.
72-
// "EnvVarTrue" - Enable by setting the environment *_DISABLEURIQUERYREDACTION variable to 'true'.
73-
// "EnvVar1" - Enable by setting the environment *DISABLEURIQUERYREDACTION variable to '1'.
74-
string[] lqs = ["", "AppCtx"];
75-
foreach (string queryRedactionDisabler in lqs)
76-
{
77-
result.Add(syncApi, scopeHandler, queryRedactionDisabler);
78-
}
62+
case "AppCtx":
63+
AppContext.SetSwitch("System.Net.Http.DisableUriRedaction", true);
64+
break;
65+
case "EnvVarTrue":
66+
Environment.SetEnvironmentVariable("DOTNET_SYSTEM_NET_HTTP_DISABLEURIREDACTION", "true");
67+
break;
68+
case "EnvVar1":
69+
Environment.SetEnvironmentVariable("DOTNET_SYSTEM_NET_HTTP_DISABLEURIREDACTION", "1");
70+
break;
7971
}
80-
}
8172

82-
result.Add(false, false, "EnvVarTrue");
83-
result.Add(false, false, "EnvVar1");
84-
result.Add(false, true, "EnvVarTrue");
85-
result.Add(false, true, "EnvVar1");
73+
Uri[] uris = GetRedactedUriString_Data.Select(a => a[0] == null ? null : new Uri((string)a[0], UriKind.RelativeOrAbsolute)).ToArray();
8674

87-
return result;
75+
foreach (Uri uri in uris)
76+
{
77+
string? expected = uri != null ? uri.IsAbsoluteUri ? uri.AbsoluteUri : uri.ToString() : null;
78+
string? actual = LogHelper.GetRedactedUriString(uri);
79+
Assert.Equal(expected, actual);
80+
}
81+
}, queryRedactionDisabler).Dispose();
8882
}
8983

9084
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
91-
[MemberData(nameof(Handlers_LogExpectedUri_Data))]
92-
public async Task Handlers_LogExpectedUri(bool syncApi, bool scopeHandler, string queryRedactionDisabler)
85+
[InlineData(false, false)]
86+
[InlineData(false, true)]
87+
#if NET
88+
[InlineData(true, false)]
89+
[InlineData(true, true)]
90+
#endif
91+
public async Task Handlers_LogExpectedUri(bool syncApi, bool scopeHandler)
9392
{
94-
await RemoteExecutor.Invoke(static async (syncApiStr, scopeHandlerStr, queryRedactionDisabler) =>
93+
await RemoteExecutor.Invoke(static async (syncApiStr, scopeHandlerStr) =>
9594
{
9695
bool syncApi = bool.Parse(syncApiStr);
9796
bool scopeHandler = bool.Parse(scopeHandlerStr);
@@ -100,19 +99,6 @@ await RemoteExecutor.Invoke(static async (syncApiStr, scopeHandlerStr, queryReda
10099
const string queryString = "term=Western%20Australia";
101100
string destinationUri = $"{baseUri}?{queryString}";
102101

103-
switch (queryRedactionDisabler)
104-
{
105-
case "AppCtx":
106-
AppContext.SetSwitch("Microsoft.Extensions.Http.DisableUriQueryRedaction", true);
107-
break;
108-
case "EnvVarTrue":
109-
Environment.SetEnvironmentVariable("DOTNET_MICROSOFT_EXTENSIONS_HTTP_DISABLEURIQUERYREDACTION", "True");
110-
break;
111-
case "EnvVar1":
112-
Environment.SetEnvironmentVariable("DOTNET_MICROSOFT_EXTENSIONS_HTTP_DISABLEURIQUERYREDACTION", "1");
113-
break;
114-
}
115-
116102
var sink = new TestSink();
117103
var logger = new TestLogger("test", sink, enabled: true);
118104

@@ -134,20 +120,18 @@ await RemoteExecutor.Invoke(static async (syncApiStr, scopeHandlerStr, queryReda
134120
_ = await invoker.SendAsync(request, default);
135121
}
136122

137-
string expectedUri = !string.IsNullOrEmpty(queryRedactionDisabler) ? destinationUri : $"{baseUri}?*";
138-
139123
if (scopeHandler)
140124
{
141125
var pipelineStartMessage = Assert.Single(sink.Writes.Where(m => m.EventId == EventIds.PipelineStart));
142-
Assert.Equal($"HTTP GET {expectedUri}", pipelineStartMessage.Scope.ToString());
143-
Assert.Equal($"Start processing HTTP request GET {expectedUri}", pipelineStartMessage.Message);
126+
Assert.Equal($"HTTP GET {baseUri}?*", pipelineStartMessage.Scope.ToString());
127+
Assert.Equal($"Start processing HTTP request GET {baseUri}?*", pipelineStartMessage.Message);
144128
}
145129
else
146130
{
147131
var requestStartMessage = Assert.Single(sink.Writes.Where(m => m.EventId == EventIds.RequestStart));
148-
Assert.Equal($"Sending HTTP request GET {expectedUri}", requestStartMessage.Message);
132+
Assert.Equal($"Sending HTTP request GET {baseUri}?*", requestStartMessage.Message);
149133
}
150-
}, syncApi.ToString(), scopeHandler.ToString(), queryRedactionDisabler).DisposeAsync();
134+
}, syncApi.ToString(), scopeHandler.ToString()).DisposeAsync();
151135
}
152136

153137
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
@@ -170,7 +154,7 @@ await RemoteExecutor.Invoke(static async (syncApiStr, disableUriQueryRedactionSt
170154

171155
if (disableUriQueryRedaction)
172156
{
173-
AppContext.SetSwitch("Microsoft.Extensions.Http.DisableUriQueryRedaction", true);
157+
AppContext.SetSwitch("System.Net.Http.DisableUriRedaction", true);
174158
}
175159

176160
var sink = new TestSink();

0 commit comments

Comments
 (0)