-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Add AppContext switch in patch release to opt-out of breaking behavior change in ForwardedHeaders middleware. #62687
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
Conversation
…r change in ForwardedHeaders middleware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new AppContext switch and matching environment variable so that users can opt out of the stricter proxy validation change in the ForwardedHeaders middleware, restoring the previous behavior when no X-Forwarded-For header is present.
- Introduces a private
_ignoreUnknownProxiesWithoutForflag initialized fromAppContext.SetSwitchor an environment variable. - Wraps the existing KnownProxies/KnownNetworks check in
ApplyForwardersto skip validation when the flag is enabled. - Adds two theory-based tests verifying both the AppContext switch and the environment variable override.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs | Added _ignoreUnknownProxiesWithoutFor field, switch/env loading, and conditional proxy check. |
| src/Middleware/HttpOverrides/test/ForwardedHeadersMiddlewareTest.cs | Added tests for the AppContext switch and environment variable opt-out scenarios. |
Comments suppressed due to low confidence (1)
src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs:67
- Add an XML doc comment on the middleware constructor or the switch key constant to describe the purpose of this AppContext switch and the corresponding environment variable, so that it's surfaced in public API documentation.
if (AppContext.TryGetSwitch("Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor", out var enabled)
| if (AppContext.TryGetSwitch("Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor", out var enabled) | ||
| && enabled) | ||
| { | ||
| _ignoreUnknownProxiesWithoutFor = true; | ||
| } | ||
|
|
||
| if (Environment.GetEnvironmentVariable("MICROSOFT_ASPNETCORE_HTTPOVERRIDES_IGNORE_UNKNOWN_PROXIES_WITHOUT_FOR") is string env |
Copilot
AI
Jul 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Extract the switch key string into a private const string (and reuse it for the environment-variable name) to avoid magic strings and ensure consistency.
| if (AppContext.TryGetSwitch("Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor", out var enabled) | |
| && enabled) | |
| { | |
| _ignoreUnknownProxiesWithoutFor = true; | |
| } | |
| if (Environment.GetEnvironmentVariable("MICROSOFT_ASPNETCORE_HTTPOVERRIDES_IGNORE_UNKNOWN_PROXIES_WITHOUT_FOR") is string env | |
| const string SwitchKey = "Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor"; | |
| const string EnvVarKey = "MICROSOFT_ASPNETCORE_HTTPOVERRIDES_IGNORE_UNKNOWN_PROXIES_WITHOUT_FOR"; | |
| if (AppContext.TryGetSwitch(SwitchKey, out var enabled) && enabled) | |
| { | |
| _ignoreUnknownProxiesWithoutFor = true; | |
| } | |
| if (Environment.GetEnvironmentVariable(EnvVarKey) is string env |
| } | ||
| } | ||
|
|
||
| [Theory] |
Copilot
AI
Jul 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The AppContext and environment-variable tests share nearly identical setup. Consider extracting the common host setup and assertion logic into a helper or combining them into a single parameterized test to reduce duplication.
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/16230613374 |
|
/backport to release/2.3 |
|
Started backporting to release/2.3: https://github.com/dotnet/aspnetcore/actions/runs/16230628317 |
|
@BrennanConroy backporting to "release/2.3" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Add AppContext switch in patch release to opt-out of breaking behavior change in ForwardedHeaders middleware.
Using index info to reconstruct a base tree...
M src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs
M src/Middleware/HttpOverrides/test/ForwardedHeadersMiddlewareTest.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Middleware/HttpOverrides/test/ForwardedHeadersMiddlewareTest.cs
CONFLICT (content): Merge conflict in src/Middleware/HttpOverrides/test/ForwardedHeadersMiddlewareTest.cs
Auto-merging src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs
CONFLICT (content): Merge conflict in src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Add AppContext switch in patch release to opt-out of breaking behavior change in ForwardedHeaders middleware.
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
| if (_logger.IsEnabled(LogLevel.Warning)) | ||
| { | ||
| _logger.LogWarning(1, "Unknown proxy: {RemoteIpAndPort}", currentValues.RemoteIpAndPort); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed from debug? Without MICROSOFT_ASPNETCORE_HTTPOVERRIDES_IGNORE_UNKNOWN_PROXIES_WITHOUT_FOR my logs are getting spammed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. And we didn't change anything about our configuration. We do have KnownNetworks/KnownProxies configured, but apparently we are losing some of that information now with Azure AGC. Odd too because we use option "All" which should've been unchanged... Looked into my logs today and saw tons of warnings and finally traced it to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we get rid of these warnings ? It started appearing as soon as we updated the version with no additional code change .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using the switch Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor or ENV MICROSOFT_ASPNETCORE_HTTPOVERRIDES_IGNORE_UNKNOWN_PROXIES_WITHOUT_FOR.
Was kinda hoping to have heard something from @BrennanConroy or @wtgodbe by now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all we're using at the moment:
app.UseForwardedHeaders(new ForwardedHeadersOptions
{
ForwardLimit = 10,
ForwardedHeaders = ForwardedHeaders.XForwardedProto | ForwardedHeaders.XForwardedHost
});I get a warning log entry every time I interact with the application behind a reverse proxy. Was this changed to a warning because it's been deemed to be more of a risk?
If so, would we need to add the following to accommodate tens of thousands of users' individual setups?
KnownNetworks.Add(new IPNetwork(IPAddress.Any, 0));
KnownNetworks.Add(new IPNetwork(IPAddress.IPv6Any, 0));
KnownProxies.Add(IPAddress.Any);
KnownProxies.Add(IPAddress.IPv6Any);Is the switch a better idea?
public static void SetSwitch("Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor", true);Or is there some other option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, got my logs spammed by that debug log switched to a warning log. That is not just a matter of properly configuring allowed proxy networks. We know them and configure them, but some of our routes allow to have one level of proxy less, and those routes then spam us with this change due to external proxies like enterprise proxies, ...
So, I am going to disable that warning specifically through logging configuration.
Something like this should do:
"Logging": {
"EventLog": {
"LogLevel": {
"Microsoft.AspNetCore.HttpOverrides.ForwardedHeadersMiddleware": "Error"
}
},
"EventSource": {
"LogLevel": {
"Microsoft.AspNetCore.HttpOverrides.ForwardedHeadersMiddleware": "Error"
}
}
}
(Disabling it in the general section does not work, because these two providers have a default configuration lowering the log level to Information for the "Microsoft" category.)
It does disable some other warnings of the middleware, though.
Actually there is an issue about promoting it to something else than a mere debug log, see #58461. So that issue is now fulfilled, but I do not think it is adequate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps #58461 could be alternatively addressed with a configuration that will enable Warning logging any untrusted proxies if users want to opt in to that? I understand that doesn't by default address the support burden for cases where KnownProxies/Networks are not configured appropriately, but it's unfortunate that this change also impacts correctly behaving uses where they are set up properly and are properly stopping at the first untrusted proxy but still getting a Warning log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #63801 for explicitly the Debug-Warning level change
Add AppContext switch in patch release to opt-out of breaking behavior change in ForwardedHeaders middleware.
Description
We previously fixed a bug where
KnownProxiesandKnownNetworksweren't being applied in common cases. We didn't realize at the time this would break many customers apps.The workaround is to configure
KnownProxiesandKnownNetworks, which is intended, and our docs do state that users should configure these. But due to the bug we recently fixed, users didn't need to configure those options for the app to work.We're adding an app context switch to opt-out of the breaking change and go back to the previous behavior. This gives users the option to update their app code at a more convenient time, e.g. when 10.0 releases.
Customer Impact
Customers have noticed that updating to the latest patch breaks scenarios like Https redirection and auth flows due to the X-Forwarded-Proto header not being applied anymore.
Regression?
2.3.4, 8.0.17, 9.0.6
Change was made on purpose to harden security, but didn't realize it would cause a regression.
Risk
Just adding an app context switch. Test coverage added for the switch as well.
Verification
Packaging changes reviewed?