Ignore proxy forwarding headers by default for OAuth resource metadata#2550
Open
SamMorrowDrums wants to merge 1 commit into
Open
Ignore proxy forwarding headers by default for OAuth resource metadata#2550SamMorrowDrums wants to merge 1 commit into
SamMorrowDrums wants to merge 1 commit into
Conversation
X-Forwarded-Host and X-Forwarded-Proto were unconditionally honored when constructing OAuth resource metadata URLs. In HTTP-mode deployments that do not set --base-url and are not fronted by a proxy that strips these headers, this lets an on-path client influence the URL advertised in WWW-Authenticate and the /.well-known/oauth-protected-resource body. This is a hardening change rather than a true vulnerability — exploiting it requires HTTP without --base-url plus an attacker already positioned to inject the header — but the unsafe default is worth closing. Default behavior now derives host/scheme from r.Host and the TLS state. Setups that rely on a trusted internal forwarder (e.g. an in-cluster gateway that needs to preserve the originating hostname per request) can opt back in with --trust-proxy-headers / GITHUB_TRUST_PROXY_HEADERS=1. --base-url continues to take precedence in all cases. Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens OAuth protected-resource metadata URL construction by ignoring X-Forwarded-Host / X-Forwarded-Proto by default, with an explicit opt-in for trusted proxy deployments.
Changes:
- Add
TrustProxyHeadersconfig plumbing from CLI/server config into OAuth URL derivation. - Gate forwarded-header usage in
GetEffectiveHostAndSchemebehindTrustProxyHeaders. - Update tests + docs to reflect the new default and opt-in behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/http/server.go | Plumbs TrustProxyHeaders through ServerConfig into OAuth config. |
| pkg/http/oauth/oauth.go | Gates forwarded-header reads on TrustProxyHeaders. |
| pkg/http/oauth/oauth_test.go | Adds/updates test cases for default-ignore vs opt-in forwarded headers. |
| cmd/github-mcp-server/main.go | Adds --trust-proxy-headers flag and viper binding. |
| docs/streamable-http.md | Documents default behavior and trusted-proxy opt-in. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 1
omgitsads
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
By default, ignore
X-Forwarded-HostandX-Forwarded-Protowhen constructing the URLs that appear in theWWW-Authenticate: Bearer resource_metadata=...response header and the/.well-known/oauth-protected-resourcebody. Add an opt-in--trust-proxy-headersflag (andGITHUB_TRUST_PROXY_HEADERSenv) for deployments that sit behind a trusted internal forwarder.--base-urlcontinues to take precedence in all cases.Why
GetEffectiveHostAndSchemepreviously readX-Forwarded-Host/X-Forwarded-Protounconditionally. In an HTTP-mode self-host that does not set--base-urland is not fronted by a proxy that strips those headers, a client that can inject them can influence the URL we advertise to MCP clients for OAuth discovery.This is a hardening change rather than a true vulnerability — exploitation requires HTTP mode without
--base-urlplus an attacker already positioned to set request headers, which is usually a sign of a bigger problem — but the unsafe default is worth closing.Behavior
r.Host(thenlocalhost); effective scheme comes from TLS state (thenhttp).--base-urlset: unchanged, always wins.--trust-proxy-headers(orGITHUB_TRUST_PROXY_HEADERS=1): restores the previous header-honoring behavior for trusted internal forwarders that need to preserve the originating hostname per request.