fix: oauth issues and add tokenResponseMapping for non-standard providers#4009
fix: oauth issues and add tokenResponseMapping for non-standard providers#4009jhrozek merged 15 commits intostacklok:mainfrom
Conversation
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>
ce82035 to
fd618d5
Compare
6890954 to
e0b3876
Compare
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>
jhrozek
left a comment
There was a problem hiding this comment.
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! |
There was a problem hiding this comment.
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 transformationAlternative:
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.
eh, go away bot |
|
@aron-muon I should have ran CI earlier, sorry would you mind running |
All set! |
There was a problem hiding this comment.
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 transformationAlternative:
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 Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
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.gobeforeviper.BindEnv("debug", "TOOLHIVE_DEBUG")was called incommands.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 mainthvbinary 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
UserClaimsstruct to avoid parameter-ordering mistakes insession.New(), and addedNameClaimKey/EmailClaimKeyconstants 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/mcpwere forwarded tohttps://mcp.asana.com/mcpinstead ofhttps://mcp.asana.com/v2/mcp, causing 401 errors from Asana and Atlassian because the endpoint doesn't exist at the wrong path.Added
WithRemoteBasePathoption 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_tokenunderauthed_user.access_token, causing Go's oauth2 library to fail with"server response missing access_token".Added a
tokenResponseMappingfield toOAuth2UpstreamConfigthat configures dot-notation paths (via gjson, already a dependency) for extracting token fields from non-standard response formats. Implemented as a response-rewritinghttp.RoundTripperthat 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 viaTokenSource). Thetoken_typeis always normalized to"Bearer"since non-standard providers may use different values (e.g., GovSlack uses"user").Example CRD usage:
Review feedback addressed
session.UserClaimsstruct to avoid swappable string parameters insession.New()NameClaimKey/EmailClaimKeyconstants for consistency withTokenSessionIDClaimKeyandClientIDClaimKeyTokenTypePathfield —token_typeis always normalized to"Bearer", so the field was dead configpathOrDefaultcalls to use standard field names as defaults ("refresh_token","expires_in","scope") instead of empty stringsio.LimitReaderfor token response body reads to prevent unbounded memory allocationContent-Lengthheader handling: useHeader.Delinstead of setting to empty stringtask operator-generate,task operator-manifests, andtask crdref-gento regenerate deepcopy, CRD schemas, and API docsTOOLHIVE_DEBUGfix to the mainthvbinaryTest plan
Large PR Justification