fix: warn callers not to truncate the #t= temp-token fragment on MCP inline-auth links - #5104
Conversation
…inline-auth links When MCPEnableTempTokenAuth is on, per-user MCP auth flows (OAuth and headers) mint a one-time temp token embedded as a `#t=<token>` URL fragment on the auth link returned to the caller. Fragments never reach the server, so the token can't leak into access logs or get forwarded as a Referer header — but an AI agent relaying the link to a human frequently drops everything after the `#`, treating it as a non-essential anchor, which breaks the link. Adds schemas.MCPAuthURLHasTempTokenFragment and schemas.MCPAuthTempTokenReminder, and uses them to conditionally append a reminder to the inline-auth Message wherever it's built, only when the URL actually carries the fragment (the mint is best-effort and skipped for MCPAuthModeUser flows). Affected packages: - core: schemas.MCPAuthURLHasTempTokenFragment/MCPAuthTempTokenReminder, per-user OAuth and per-user headers credstore resolvers - transports: bifrost-http's own MCP server tool-call error path Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe MCP auth flow now detects MCP temp-token auth reminders
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant perUserOAuthResolver
participant MCPAuthURLHasTempTokenFragment
participant MCPToolHandler
MCPClient->>perUserOAuthResolver: Request connection headers
perUserOAuthResolver->>MCPAuthURLHasTempTokenFragment: Check authorize URL
MCPAuthURLHasTempTokenFragment-->>perUserOAuthResolver: Fragment present or absent
perUserOAuthResolver-->>MCPClient: MCPAuthRequiredError with conditional reminder
MCPClient->>MCPToolHandler: Execute MCP tool
MCPToolHandler->>MCPAuthURLHasTempTokenFragment: Check auth URL
MCPAuthURLHasTempTokenFragment-->>MCPToolHandler: Fragment present or absent
MCPToolHandler-->>MCPClient: Tool error with conditional reminder
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Merge branch 'dev' into 07-11-fix_mcp_au..." | Re-trigger Greptile |
Signed-off-by: Marcus <MarcusPeng@users.noreply.github.com>
* origin/dev: (79 commits) chore: add `helm-update` Claude skill for syncing Helm chart with `config.schema.json` (maximhq#5144) fix: web search options to google search mapping in gemini api (maximhq#5139) feat: add `postgresql.external.port` string support and `bifrost.mcp.toolGroups[*].id` to Helm chart (maximhq#5143) fix: parse `SecretVar` JSON with `ref`/`env_var` fields even when `value` is absent (maximhq#5146) Revert "fix: less strict unmarshalling for secret var (maximhq#4723)" (maximhq#5145) fix: max reasoning effort in openai (maximhq#5130) chore: replace manual `helm registry login` steps with `step-security/docker-login-action` (maximhq#5132) fix: support GA transcription-type sessions in POST /v1/realtime/client_secrets (maximhq#5092) community: add Xquik to MCP library (maximhq#5069) fix: warn callers not to truncate the #t= temp-token fragment on MCP inline-auth links (maximhq#5104) chore: build fix in core (maximhq#5129) fix: never persist masked provider key previews (maximhq#5106) Filter out provider-level keys from selector in prompt manager (maximhq#5018) fix: show user popover when `userInfo` exists and include `preferred_username` as display name fallback (maximhq#5098) fix: use `AutoMigrate` and add `runner_id`/`created_by_user_id` columns to sidekiq table migration (maximhq#5085) dds new harness skill and updates based on merged PRs (maximhq#5126) dds new harness skill and updates based on merged PRs (maximhq#5123) Add Trendshift badge to README (maximhq#5124) fix: make tracing span lookup nil-safe to prevent panic on streaming errors (maximhq#4896) Revert "fix: synthesize per-query rerank usage for Bedrock and Vertex (maximhq#4322)" (maximhq#5122) ...
…inline-auth links (#5104) * fix: warn callers not to truncate the #t= temp-token fragment on MCP inline-auth links When MCPEnableTempTokenAuth is on, per-user MCP auth flows (OAuth and headers) mint a one-time temp token embedded as a `#t=<token>` URL fragment on the auth link returned to the caller. Fragments never reach the server, so the token can't leak into access logs or get forwarded as a Referer header — but an AI agent relaying the link to a human frequently drops everything after the `#`, treating it as a non-essential anchor, which breaks the link. Adds schemas.MCPAuthURLHasTempTokenFragment and schemas.MCPAuthTempTokenReminder, and uses them to conditionally append a reminder to the inline-auth Message wherever it's built, only when the URL actually carries the fragment (the mint is best-effort and skipped for MCPAuthModeUser flows). Affected packages: - core: schemas.MCPAuthURLHasTempTokenFragment/MCPAuthTempTokenReminder, per-user OAuth and per-user headers credstore resolvers - transports: bifrost-http's own MCP server tool-call error path Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: include mcpserver.go temp-token reminder change (missed in prior push) --------- Signed-off-by: Marcus <MarcusPeng@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…inline-auth links (maximhq#5104) * fix: warn callers not to truncate the #t= temp-token fragment on MCP inline-auth links When MCPEnableTempTokenAuth is on, per-user MCP auth flows (OAuth and headers) mint a one-time temp token embedded as a `#t=<token>` URL fragment on the auth link returned to the caller. Fragments never reach the server, so the token can't leak into access logs or get forwarded as a Referer header — but an AI agent relaying the link to a human frequently drops everything after the `#`, treating it as a non-essential anchor, which breaks the link. Adds schemas.MCPAuthURLHasTempTokenFragment and schemas.MCPAuthTempTokenReminder, and uses them to conditionally append a reminder to the inline-auth Message wherever it's built, only when the URL actually carries the fragment (the mint is best-effort and skipped for MCPAuthModeUser flows). Affected packages: - core: schemas.MCPAuthURLHasTempTokenFragment/MCPAuthTempTokenReminder, per-user OAuth and per-user headers credstore resolvers - transports: bifrost-http's own MCP server tool-call error path Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: include mcpserver.go temp-token reminder change (missed in prior push) --------- Signed-off-by: Marcus <MarcusPeng@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…inline-auth links (maximhq#5104) * fix: warn callers not to truncate the #t= temp-token fragment on MCP inline-auth links When MCPEnableTempTokenAuth is on, per-user MCP auth flows (OAuth and headers) mint a one-time temp token embedded as a `#t=<token>` URL fragment on the auth link returned to the caller. Fragments never reach the server, so the token can't leak into access logs or get forwarded as a Referer header — but an AI agent relaying the link to a human frequently drops everything after the `#`, treating it as a non-essential anchor, which breaks the link. Adds schemas.MCPAuthURLHasTempTokenFragment and schemas.MCPAuthTempTokenReminder, and uses them to conditionally append a reminder to the inline-auth Message wherever it's built, only when the URL actually carries the fragment (the mint is best-effort and skipped for MCPAuthModeUser flows). Affected packages: - core: schemas.MCPAuthURLHasTempTokenFragment/MCPAuthTempTokenReminder, per-user OAuth and per-user headers credstore resolvers - transports: bifrost-http's own MCP server tool-call error path Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: include mcpserver.go temp-token reminder change (missed in prior push) --------- Signed-off-by: Marcus <MarcusPeng@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Summary
When
MCPEnableTempTokenAuthis on, per-user MCP auth flows (OAuth and headers) mint a one-time temp token embedded as a#t=<token>URL fragment on the auth link returned to the caller. Fragments never reach the server, so the token can't leak into access logs or get forwarded as a Referer header — but an AI agent relaying the link to a human frequently drops everything after the#, treating it as a non-essential anchor, which breaks the link.Changes
schemas.MCPAuthURLHasTempTokenFragmentandschemas.MCPAuthTempTokenReminderMessagewherever it's built (per-user OAuth resolver, per-user headers resolver, bifrost-http's MCP server tool-call error path), only appending the reminder when the URL actually carries the#t=fragmentType of change
Affected areas
How to test
go test ./core/schemas/... ./core/mcp/credstore/... ./transports/bifrost-http/handlers/...Breaking changes
Security considerations
None — the fragment was already never sent to the server; this only changes the accompanying human-readable message text.