Skip to content

Comments

fix(security): remove proxy and cookie auth headers on insecure redirect#653

Open
MIchaelMainer wants to merge 9 commits intomainfrom
mmainer/redirect-sec
Open

fix(security): remove proxy and cookie auth headers on insecure redirect#653
MIchaelMainer wants to merge 9 commits intomainfrom
mmainer/redirect-sec

Conversation

@MIchaelMainer
Copy link
Member

@MIchaelMainer MIchaelMainer commented Feb 11, 2026

Current behavior is that RedirectHandler only removes Authorization header on redirects that change host and scheme.

This PR addresses the scenario where proxy authorization, cookie authorization, or API Key headers are present on redirect.

  • For proxy authorization, we want to remove headers if there isn't a configured proxy and redirect URL isn't on the bypass list.
  • For cookie authorization, we will remove all headers on redirects that change host and scheme.
  • For API key type scenarios, API owner will need to provide a collection of headers that should be removed on redirect.

As always, customers can create their own redirect handler to address their custom API scenarios.

@MIchaelMainer MIchaelMainer requested a review from a team as a code owner February 11, 2026 18:07
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
adrian05-ms
adrian05-ms previously approved these changes Feb 11, 2026
@github-project-automation github-project-automation bot moved this to In Progress 🚧 in Kiota Feb 11, 2026
gavinbarron
gavinbarron previously approved these changes Feb 11, 2026
!newRequest.RequestUri.Scheme.Equals(request.RequestUri?.Scheme))
{
newRequest.Headers.Authorization = null;
newRequest.Headers.ProxyAuthorization = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should remove the proxy authorization header here, there are a couple of reasons for that:

  • if we're in a context where proxies are required, it's most likely that they'll be required for any request/origin and not just a subset. So this subsequent request will most likely fail because of the proxy denying the request.
  • According to the RFC this header only applied to the next inbound proxy. It'll be dropped if no proxy is configured on the client.

Overall, I'm failing to understand how dropping this header is beneficial. Linking an issue with a detailed analysis would have been useful here.

And to be clear, I fully support dropping the cookies. Even though it's unlikely the API would have set any.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further reflection, there are a few cases where a leak of that header might happen.
In all cases, this requires a legitimate API to redirect the client to an illegitimate host, so the likelihood of this happening is low, but not impossible in theory:

  • The proxy auth header was manually set, but a proxy configuration was never applied. In which case the header was meaningless, already forwarded to the API, but ignored. It may contain sensitive information though.
  • The proxy auth header was set (either manually or through a proxy configuration), and the proxy configuration does not apply to the illegitimate origin or was removed in between requests. This is extremely unlikely, but in this scenario, it it in theory possible to leak credentials.

Now, the question is: is there ever a legitimate scenario when a legitimate API redirects to an unsecure location? And the answer to that is: yes. This is a common pattern for blob downloads where the API redirects to a direct download URI, and sometimes those are on a different origin, which does not support HTTPS. The reasons for not supporting HTTPS may vary from "host didn't want to pay for a certificate" (less legitimate those days with free certificate providers like lets encrypt) to "this is a lot of traffic to have to encrypt when we can instead do hash validation on the client side to validate nothing has been tempered with".

In that common scenario, removing the proxy auth header without checking whether a proxy is set first will lead to a broken client, unable to complete the request.

I think the code should be amended to look something like this

var proxyUri = handler.Proxy?.GetProxy(originalRequestUri);

bool proxyActive = handler.UseProxy &&
                   proxyUri != null &&
                   proxyUri != originalRequestUri;

if (!proxyActive)
{
    request.Headers.Remove("Proxy-Authorization");
}

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question regarding the proxy authorization

@MIchaelMainer MIchaelMainer changed the title fix: remove proxy and cookie auth headers on insecure redirect fix(security): remove proxy and cookie auth headers on insecure redirect Feb 12, 2026
ramsessanchez and others added 3 commits February 13, 2026 12:45
…t require a proxy

Remove ProxyAuthorization if:  No proxy is configured (header is meaningless without a proxy) OR proxy is configured but the redirect URL is bypassed (won't use the proxy); Keep ProxyAuthorization if proxy is configured AND the redirect URL will use the proxy
…r on redirect

This is useful for scenarios like API keys
MIchaelMainer and others added 3 commits February 19, 2026 15:32
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@sonarqubecloud
Copy link

/// This is useful for removing sensitive headers like API keys that should not be sent to different hosts.
/// The Authorization and Cookie headers are always removed on host/scheme change regardless of this setting.
/// </summary>
public ICollection<string> SensitiveHeaders { get; set; } = new List<string>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we say this would be a call back with the header name and the new origin? (And potentially the old one)

If not, it should at the very least be a hashset with ordinal case insensitive string comparer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress 🚧

Development

Successfully merging this pull request may close these issues.

5 participants