Skip to content

Strip client Authorization on upstreamswap custom header#5661

Open
jerm-dro wants to merge 1 commit into
mainfrom
jerm/fix-upstreamswap-custom-header-leak
Open

Strip client Authorization on upstreamswap custom header#5661
jerm-dro wants to merge 1 commit into
mainfrom
jerm/fix-upstreamswap-custom-header-leak

Conversation

@jerm-dro

Copy link
Copy Markdown
Contributor

Summary

With header_strategy: custom, the upstreamswap auth middleware injected the upstream IdP token into a custom header but left the client's original Authorization header (the ToolHive-issued JWT) in place, forwarding it to the backend MCP server. That JWT is minted for the proxy, not the upstream — passing it through is credential passthrough of a token the backend was never meant to receive.

  • The replace strategy avoids this implicitly: it calls Set("Authorization", ...), overwriting the client JWT with the upstream token.
  • The strip-auth middleware from Allow MCPRemoteProxy to work without upstream or client auth #4168 (pkg/transport/middleware/strip_auth.go) does remove Authorization, but it is only wired in on the DisableUpstreamTokenInjection path, so it never ran on the upstreamswap custom path.
  • Fix: the custom injector now strips Authorization after setting the custom header, matching replace. When the custom header name is itself Authorization (case-insensitive, via http.CanonicalHeaderKey), the Set already replaced the JWT with the upstream token, so the strip is skipped.

Scoped to Authorization only (not Cookie/Proxy-Authorization) to mirror the replace reference path the issue identifies as non-leaking.

Fixes #5504

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix) — clean for pkg/auth/upstreamswap/...

Added a regression test that fails on the buggy code (Should be empty, but was Bearer toolhive-jwt) and passes after the fix, plus a case-insensitive edge-case test for a custom header named Authorization. Updated two existing tests that had been asserting the leaked-JWT behavior. Verified red→green by stashing the production change and re-running.

go test ./pkg/auth/upstreamswap/...   # ok
go build ./...                         # clean

Does this introduce a user-facing change?

Yes. When header_strategy: custom is configured, the backend MCP server no longer receives the client's ToolHive-issued JWT in the Authorization header. The upstream token is still delivered in the configured custom header. Backends that (incorrectly) relied on the leaked JWT will no longer see it.

Special notes for reviewers

Flagging this as a security-hardening fix for auth review — it closes a credential-passthrough path. Worth scrutinizing: the Authorization-as-custom-header edge case (must NOT strip there), and the decision to scope the strip to Authorization only to stay consistent with replace rather than adopting the broader strip-auth header set (Cookie, Proxy-Authorization).

Generated with Claude Code

The upstreamswap "custom" header strategy injected the upstream IdP
token into a custom header but left the client's original Authorization
header (the ToolHive-issued JWT) in place, forwarding it to the backend.
That token is minted for the proxy, not the upstream, so passing it
through is credential passthrough (#5504). The "replace" strategy avoids
this implicitly because it overwrites Authorization; the strip-auth
middleware from #4168 only runs on the DisableUpstreamTokenInjection
path, so it never covered this case.

Strip Authorization in the custom injector after setting the custom
header, matching the "replace" behavior. When the custom header name is
itself Authorization (case-insensitive), the Set already replaced the
JWT with the upstream token, so the strip is skipped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jun 26, 2026
@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.34%. Comparing base (799b422) to head (8838f7e).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5661      +/-   ##
==========================================
+ Coverage   69.93%   70.34%   +0.40%     
==========================================
  Files         650      649       -1     
  Lines       66289    66188     -101     
==========================================
+ Hits        46358    46558     +200     
+ Misses      16588    16276     -312     
- Partials     3343     3354      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

upstreamswap custom header strategy forwards the client ToolHive JWT upstream

1 participant