Skip to content

Add AppContext switch in patch release to opt-out of breaking behavior change in ForwardedHeaders middleware #62690

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

Open
wants to merge 2 commits into
base: release/2.3
Choose a base branch
from

Conversation

BrennanConroy
Copy link
Member

Add AppContext switch in patch release to opt-out of breaking behavior change in ForwardedHeaders middleware.

Description

We previously fixed a bug where KnownProxies and KnownNetworks weren't being applied in common cases. We didn't realize at the time this would break many customers apps.

The workaround is to configure KnownProxies and KnownNetworks, 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?

  • Yes
  • No

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

  • High
  • Medium
  • Low

Just adding an app context switch. Test coverage added for the switch as well.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

@Copilot Copilot AI review requested due to automatic review settings July 11, 2025 22:49
@BrennanConroy BrennanConroy added the Servicing-consider Shiproom approval is required for the issue label Jul 11, 2025
@BrennanConroy BrennanConroy requested review from Tratcher and a team as code owners July 11, 2025 22:49
Copy link
Contributor

@Copilot Copilot AI left a 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 an opt-out switch to restore previous proxy-filtering behavior in ForwardedHeadersMiddleware and includes the HttpOverrides package in the patch release.

  • Introduce _ignoreUnknownProxiesWithoutFor flag, wired to an AppContext switch and an environment variable
  • Guard the KnownProxies/KnownNetworks check in ApplyForwarders based on the new flag
  • Update log level from Debug to Warning for unknown proxies
  • Add HttpOverrides package to the 2.3.5 patch configuration

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs Add and consume Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor switch; update conditional logic and log level
eng/PatchConfig.props Include Microsoft.AspNetCore.HttpOverrides in the 2.3.5 patch release
Comments suppressed due to low confidence (3)

src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs:94

  • Add unit tests to cover both the AppContext switch and the environment variable path to ensure _ignoreUnknownProxiesWithoutFor behaves correctly under each scenario.
            if (AppContext.TryGetSwitch("Microsoft.AspNetCore.HttpOverrides.IgnoreUnknownProxiesWithoutFor", out var enabled)

src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs:252

  • [nitpick] Raising unknown proxy messages to Warning may flood production logs during normal operation. Consider reverting to Debug or making the severity configurable.
                        _logger.LogWarning(1, $"Unknown proxy: {currentValues.RemoteIpAndPort}");

eng/PatchConfig.props:41

  • Defining a second <PackagesInPatch> inside a new <PropertyGroup> will override the default package list rather than appending. Verify that this doesn’t unintentionally drop other packages from the patch, or merge into the existing element instead.
    <PackagesInPatch>

Copy link
Contributor

Hi @@BrennanConroy. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Servicing-consider Shiproom approval is required for the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant