Skip to content

fix: oauth issues and add tokenResponseMapping for non-standard providers#4009

Merged
jhrozek merged 15 commits intostacklok:mainfrom
aron-muon:aron/fix-oauth-issues
Mar 6, 2026
Merged

fix: oauth issues and add tokenResponseMapping for non-standard providers#4009
jhrozek merged 15 commits intostacklok:mainfrom
aron-muon:aron/fix-oauth-issues

Conversation

@aron-muon
Copy link
Contributor

@aron-muon aron-muon commented Mar 5, 2026

Summary

Fixes for OAuth-related issues discovered while deploying MCPRemoteProxy with embeddedAuthServer against Asana, Atlassian, and GovSlack, plus a new feature for non-standard token response formats.

Changes

Bug Fixes

1. Fix TOOLHIVE_DEBUG env var not enabling debug logging (cmd/thv-proxyrunner/main.go, cmd/thv/main.go)

The logger was initialized in main.go before viper.BindEnv("debug", "TOOLHIVE_DEBUG") was called in commands.go, so the env var had no effect on log level. Moved the env var binding before logger initialization. Applied the same fix to the main thv binary which was also missing the env var binding.

2. Propagate upstream user name and email into JWT claims (pkg/authserver/)

The embedded auth server resolved user identity (name, email) from the upstream IDP via the userInfo endpoint but only stored the subject claim in the JWT. This caused audit logs to show "user":"anonymous" despite successful authentication. Now propagates name and email as standard OIDC claims (per OIDC Core Section 5.1).

Introduced a UserClaims struct to avoid parameter-ordering mistakes in session.New(), and added NameClaimKey/EmailClaimKey constants for consistency with existing claim key constants. Claims extraction from ID tokens now logs a warning on failure instead of silently ignoring the error.

3. Fix remote URL path not forwarded to backend server (pkg/transport/)

When the remote URL has a path (e.g., https://mcp.asana.com/v2/mcp), the proxy stripped it and only used the scheme+host as the target. Client requests to /mcp were forwarded to https://mcp.asana.com/mcp instead of https://mcp.asana.com/v2/mcp, causing 401 errors from Asana and Atlassian because the endpoint doesn't exist at the wrong path.

Added WithRemoteBasePath option to the transparent proxy that rewrites incoming request paths to the remote server's configured path.

Feature

4. Add tokenResponseMapping for non-standard OAuth token responses (pkg/authserver/upstream/)

Some OAuth providers (e.g., GovSlack) nest token fields under non-standard paths. GovSlack returns access_token under authed_user.access_token, causing Go's oauth2 library to fail with "server response missing access_token".

Added a tokenResponseMapping field to OAuth2UpstreamConfig that configures dot-notation paths (via gjson, already a dependency) for extracting token fields from non-standard response formats. Implemented as a response-rewriting http.RoundTripper that normalizes the response JSON before the oauth2 library parses it — keeping all of Go's oauth2 RFC 6749 compliance (error handling, token type validation, refresh via TokenSource). The token_type is always normalized to "Bearer" since non-standard providers may use different values (e.g., GovSlack uses "user").

Example CRD usage:

oauth2Config:
  tokenEndpoint: "https://slack-gov.com/api/oauth.v2.access"
  tokenResponseMapping:
    accessTokenPath: "authed_user.access_token"
    scopePath: "authed_user.scope"

Review feedback addressed

  • Introduced session.UserClaims struct to avoid swappable string parameters in session.New()
  • Added NameClaimKey/EmailClaimKey constants for consistency with TokenSessionIDClaimKey and ClientIDClaimKey
  • Log warning when optional ID token claims extraction fails (instead of silently ignoring)
  • Removed unused TokenTypePath field — token_type is always normalized to "Bearer", so the field was dead config
  • Fixed pathOrDefault calls to use standard field names as defaults ("refresh_token", "expires_in", "scope") instead of empty strings
  • Used io.LimitReader for token response body reads to prevent unbounded memory allocation
  • Fixed Content-Length header handling: use Header.Del instead of setting to empty string
  • Ran task operator-generate, task operator-manifests, and task crdref-gen to regenerate deepcopy, CRD schemas, and API docs
  • Applied the same TOOLHIVE_DEBUG fix to the main thv binary

Test plan

  • All existing tests pass
  • New tests for remote path forwarding (5 test cases: Asana, Atlassian, GitHub, base path, full path scenarios)
  • New tests for token response mapping (rewrite unit tests, RoundTripper integration tests, error passthrough, nil mapping, validation)
  • Session tests updated for name/email JWT claims with UserClaims struct
  • Verified in production: Asana, Atlassian, and GovSlack all authenticate and forward requests successfully
  • Full build clean, golangci-lint clean

Large PR Justification

  • This is a large feature

aron-muon and others added 5 commits March 5, 2026 11:52
The logger was initialized in main.go before viper.BindEnv was called
in commands.go, so TOOLHIVE_DEBUG had no effect on log level. Move the
env var binding before the logger initialization.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The embedded auth server resolved user identity (name, email) from the
upstream IDP via the userInfo endpoint but only stored the subject
claim in the JWT. This caused audit logs to show "anonymous" for the
user field despite successful authentication.

Propagate name and email from the upstream Identity through to the
session's JWT claims as standard OIDC claims (name, email per OIDC
Core Section 5.1). The auth middleware's claimsToIdentity function
already reads these claims, so the audit middleware will now display
the actual user name.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the remote URL has a path (e.g., https://mcp.asana.com/v2/mcp),
the proxy stripped it and only used the scheme+host as the target.
Client requests to /mcp were forwarded to https://mcp.asana.com/mcp
instead of https://mcp.asana.com/v2/mcp, causing Asana to return
401 invalid_token because the endpoint doesn't exist at /mcp.

Extract the remote URL's path and pass it to the transparent proxy
via WithRemoteBasePath. The proxy's Director rewrites incoming
request paths to the remote server's configured path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Some OAuth providers (e.g., GovSlack) nest token fields under
non-standard paths instead of returning them at the top level.
GovSlack returns access_token under authed_user.access_token,
causing the standard oauth2 library to fail with "response missing
access_token".

Add a tokenResponseMapping field to OAuth2UpstreamConfig that
configures dot-notation paths for extracting token fields from
non-standard response formats. When set, the token exchange bypasses
golang.org/x/oauth2 and makes the HTTP POST directly, extracting
fields using gjson (already a dependency).

Example usage:
  tokenResponseMapping:
    accessTokenPath: "authed_user.access_token"
    tokenTypePath: "authed_user.token_type"

Changes span the full config pipeline: CRD types, RunConfig,
operator conversion, runtime resolution, and the upstream provider.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Run `task operator-manifests` and `crd-ref-docs` to update the
CRD schema with the new tokenResponseMapping field on
OAuth2UpstreamConfig and regenerate the API reference docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@aron-muon aron-muon force-pushed the aron/fix-oauth-issues branch from ce82035 to fd618d5 Compare March 5, 2026 12:40
@aron-muon aron-muon changed the title Fix OAuth issues: remote path forwarding, JWT claims, debug logging, and token response mapping Fix OAuth issues and add tokenResponseMapping for non-standard providers Mar 5, 2026
@aron-muon aron-muon changed the title Fix OAuth issues and add tokenResponseMapping for non-standard providers fix: OAuth issues and add tokenResponseMapping for non-standard providers Mar 5, 2026
@aron-muon aron-muon changed the title fix: OAuth issues and add tokenResponseMapping for non-standard providers fix: oauth issues and add tokenResponseMapping for non-standard providers Mar 5, 2026
@aron-muon aron-muon force-pushed the aron/fix-oauth-issues branch from 6890954 to e0b3876 Compare March 5, 2026 12:50
@aron-muon aron-muon marked this pull request as ready for review March 5, 2026 12:52
GovSlack returns token_type "user" which the oauth2 library rejects
since it only accepts "Bearer". Since the rewriter already handles
non-standard responses, always normalize token_type to "Bearer" so
the oauth2 library accepts it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

This is great work. I left some comments inline, the only that really need to be done is to re-run the generate task target to fix the deepcopy files.

the rest of the comments are mostly nits and questions.

Thanks a lot of for the PR!

@aron-muon
Copy link
Contributor Author

This is great work. I left some comments inline, the only that really need to be done is to re-run the generate task target to fix the deepcopy files.

the rest of the comments are mostly nits and questions.

Thanks a lot of for the PR!

Thanks for the review! I appreciate the work ya'll have put into this - it works pretty well in our PoC!

jhrozek
jhrozek previously approved these changes Mar 6, 2026
@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Mar 6, 2026
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

@jhrozek
Copy link
Contributor

jhrozek commented Mar 6, 2026

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

eh, go away bot

@jhrozek jhrozek dismissed github-actions[bot]’s stale review March 6, 2026 10:02

bad bot, no cookie

@jhrozek
Copy link
Contributor

jhrozek commented Mar 6, 2026

@aron-muon I should have ran CI earlier, sorry would you mind running task docs, submitting the changes it produces and pushing again?

@aron-muon
Copy link
Contributor Author

@aron-muon I should have ran CI earlier, sorry would you mind running task docs, submitting the changes it produces and pushing again?

All set!

@aron-muon aron-muon requested a review from jhrozek March 6, 2026 10:12
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 6, 2026
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 71.05263% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.66%. Comparing base (3aefbc8) to head (11e85fd).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/transport/http.go 0.00% 9 Missing ⚠️
cmd/thv-operator/pkg/controllerutil/authserver.go 0.00% 6 Missing and 1 partial ⚠️
pkg/authserver/runner/embeddedauthserver.go 25.00% 5 Missing and 1 partial ⚠️
pkg/authserver/upstream/token_exchange.go 88.67% 3 Missing and 3 partials ⚠️
pkg/authserver/upstream/oidc.go 55.55% 3 Missing and 1 partial ⚠️
pkg/authserver/server/handlers/callback.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4009      +/-   ##
==========================================
+ Coverage   68.62%   68.66%   +0.03%     
==========================================
  Files         444      445       +1     
  Lines       45222    45323     +101     
==========================================
+ Hits        31034    31120      +86     
- Misses      11791    11798       +7     
- Partials     2397     2405       +8     

☔ View full report in Codecov by Sentry.
📢 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.

@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 6, 2026
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 6, 2026
@jhrozek jhrozek merged commit bc9b534 into stacklok:main Mar 6, 2026
33 checks passed
@aron-muon aron-muon deleted the aron/fix-oauth-issues branch March 6, 2026 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants