Conversation
|
|
8 tasks
Collaborator
Author
This was referenced May 4, 2026
Merged
Pratham-Mishra04
changed the title
Contributor
Confidence Score: 4/5Core OAuth fixes are correct and safe to merge; the demo server has open issues from prior review rounds. The three primary bug fixes are well-implemented: the re-auth upsert now correctly surfaces DB errors rather than silently swallowing them, the examples/mcps/oauth-demo-server/main.go — two findings flagged in prior review rounds remain unresolved (invalid startup-log config block and refresh-token TOCTOU race). Important Files Changed
Reviews (7): Last reviewed commit: "feat: add refresh token expiry reauth fl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an exported sentinel ErrMCPReconnectNotApplicable, treats expired per‑user OAuth tokens like missing tokens to trigger re‑auth, wraps the per‑user OAuth reconnect early return with that sentinel, upserts per‑user OAuth sessions, extends token/session DB fields, validates/sanitizes MCP external URLs, surfaces redirect_uri warnings in docs/UI, and adds an OAuth+MCP demo server. ChangesPer-user OAuth / Reconnect
Config Validation, Docs & UI
OAuth Demo Server (example)
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Bifrost as Bifrost
participant OAuth as OAuth Server
participant User as User
Client->>Bifrost: Request needing per-user auth
Bifrost->>Bifrost: ResolvePerUserOAuthToken
Bifrost->>OAuth: Attempt token refresh/validation
OAuth-->>Bifrost: ErrOAuth2TokenExpired or token not found
Bifrost->>Bifrost: InitiateUserOAuthFlow (upsert session, rotate PKCE/state)
Bifrost->>User: Redirect to OAuth /authorize (PKCE)
User->>OAuth: Authorize (login/consent)
OAuth->>User: Redirect back with code
User->>Bifrost: Callback with code
Bifrost->>OAuth: Exchange code -> /token
OAuth-->>Bifrost: access_token + refresh_token
Bifrost->>Client: Proceed with MCP request handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/mcps/oauth-demo-server/go.mod`:
- Around line 5-10: The go.mod currently requires github.com/mark3labs/mcp-go
v0.43.2 which pulls in the vulnerable github.com/buger/jsonparser v1.1.1; fix by
either updating the mcp-go requirement to v0.49.0 (or later) in go.mod (replace
the existing github.com/mark3labs/mcp-go version) or add a replace directive
that pins github.com/buger/jsonparser to v1.1.2 or newer; ensure you run go mod
tidy and verify the transitive dependency no longer resolves to v1.1.1.
In `@examples/mcps/oauth-demo-server/main.go`:
- Around line 568-605: The refresh rotation is not atomic: the code loads old
under st.mu, then releases the lock and later deletes/rotates, allowing two
concurrent refreshes to both succeed; fix by doing the lookup, expiry check and
the invalidation/rotation atomically under the same mutex. Specifically, in the
refresh handler keep st.mu locked across reading st.refreshTokens[rt], checking
time.Now().After(old.RefreshExpiresAt), deleting st.refreshTokens[rt] and
st.accessTokens[old.AccessToken], then call issueTokens (or modify issueTokens
so it can be safely invoked while holding the lock) and insert the new rec into
st.refreshTokens/st.accessTokens before unlocking, using the same rt/old
identifiers to ensure single-use semantics for rt.
In `@framework/oauth2/main.go`:
- Around line 859-871: The lookup error from
p.configStore.GetOauthUserSessionBySessionToken must not be ignored: check the
returned err immediately after calling GetOauthUserSessionBySessionToken (not
just existing), and if err != nil return/wrap that error instead of proceeding
to the insert path; only proceed to update existing (via
p.configStore.UpdateOauthUserSession) when existing != nil and err == nil,
otherwise propagate the retrieval error so transient read failures aren't masked
and you avoid creating misleading create/unique-index errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82d6d6e1-5c51-474b-986c-084aae702dd9
⛔ Files ignored due to path filters (1)
examples/mcps/oauth-demo-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
core/mcp/clientmanager.gocore/mcp/utils/utils.gocore/schemas/mcp.godocs/mcp/gateway-url.mdxdocs/mcp/oauth.mdxexamples/mcps/oauth-demo-server/go.modexamples/mcps/oauth-demo-server/main.goframework/oauth2/main.gotransports/bifrost-http/handlers/mcp.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
Pratham-Mishra04
force-pushed
the
04-30-fix_semantic_cache_fixes
branch
from
76e9d94 to
2ee69fd
Compare
Pratham-Mishra04
force-pushed
the
05-04-feat_add_refresh_token_expiry_reauth_flow_for_per_user_mcp_oauth
branch
from
09c4f02 to
bc19ce7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 670-683: ValidateBaseURL currently parses the raw input but
BuildBaseURL first normalizes by trimming whitespace and trailing slashes,
causing mismatched validation; update ValidateBaseURL to mirror BuildBaseURL
normalization: trim surrounding whitespace and any trailing slash(es) before
checking for empty and before calling url.Parse, then validate parsed.Scheme and
parsed.Host and return the same error message if invalid (reference:
ValidateBaseURL and BuildBaseURL).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bae884fc-20be-4bc7-a0fd-babb2eea7d96
⛔ Files ignored due to path filters (1)
examples/mcps/oauth-demo-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
core/mcp/clientmanager.gocore/mcp/utils/utils.gocore/schemas/mcp.godocs/mcp/gateway-url.mdxdocs/mcp/oauth.mdxexamples/mcps/oauth-demo-server/go.modexamples/mcps/oauth-demo-server/main.goframework/oauth2/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
✅ Files skipped from review due to trivial changes (5)
- docs/mcp/gateway-url.mdx
- core/schemas/mcp.go
- docs/mcp/oauth.mdx
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- ui/app/workspace/config/views/mcpView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/mcps/oauth-demo-server/go.mod
- framework/oauth2/main.go
Pratham-Mishra04
force-pushed
the
05-04-feat_add_refresh_token_expiry_reauth_flow_for_per_user_mcp_oauth
branch
from
bc19ce7 to
c092d13
Compare
Pratham-Mishra04
force-pushed
the
04-30-fix_semantic_cache_fixes
branch
from
2ee69fd to
59459da
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/oauth2/main.go`:
- Around line 853-894: This code performs a read-then-write TOCTOU around the
session_token unique index: concurrent requests can both see no existing row and
one will fail CreateOauthUserSession; fix by making the write atomic in
p.configStore — add and call a store-level upsert/claim method (e.g.,
UpsertOauthUserSession or ClaimOauthUserSession) that performs an INSERT ... ON
CONFLICT (session_token) DO UPDATE (or equivalent transactional lock) to
create-or-update the row; if you cannot change the store API, implement a retry
path in this function: call CreateOauthUserSession, and on unique-constraint
error reload with GetOauthUserSessionBySessionToken and then
UpdateOauthUserSession (or loop few times) to avoid the race. Ensure you
reference p.configStore, GetOauthUserSessionBySessionToken,
CreateOauthUserSession, UpdateOauthUserSession and the session_token unique
constraint when implementing.
- Around line 1128-1147: The delete of the stale token
(p.configStore.DeleteOauthUserToken) is load-bearing and must gate the "re-auth
required" sentinel; change the flow so that if DeleteOauthUserToken returns an
error you log and return that delete error (wrapped) immediately and do NOT
proceed to return schemas.ErrOAuth2TokenExpired or update the session; only when
the delete succeeds should you continue to perform the best-effort session
lookup (p.configStore.GetOauthUserSessionBySessionToken) and
UpdateOauthUserSession and finally return the wrapped ErrOAuth2TokenExpired
sentinel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: feb58271-bec3-4ec5-adc5-d5208c994b7f
⛔ Files ignored due to path filters (1)
examples/mcps/oauth-demo-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
core/mcp/clientmanager.gocore/mcp/utils/utils.gocore/schemas/mcp.godocs/mcp/gateway-url.mdxdocs/mcp/oauth.mdxexamples/mcps/oauth-demo-server/go.modexamples/mcps/oauth-demo-server/main.goframework/oauth2/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
✅ Files skipped from review due to trivial changes (6)
- examples/mcps/oauth-demo-server/go.mod
- transports/bifrost-http/lib/ctx.go
- core/schemas/mcp.go
- core/mcp/utils/utils.go
- docs/mcp/oauth.mdx
- ui/app/workspace/config/views/mcpView.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/mcp/gateway-url.mdx
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/mcp.go
- transports/bifrost-http/handlers/config.go
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- examples/mcps/oauth-demo-server/main.go
Pratham-Mishra04
changed the base branch from
04-30-fix_semantic_cache_fixes
to
graphite-base/3211
This was referenced May 5, 2026
Pratham-Mishra04
force-pushed
the
05-04-feat_add_refresh_token_expiry_reauth_flow_for_per_user_mcp_oauth
branch
from
c092d13 to
e34323a
Compare
Pratham-Mishra04
force-pushed
the
graphite-base/3211
branch
from
59459da to
a9d6459
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
transports/bifrost-http/lib/ctx.go (1)
624-637:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
ValidateBaseURLinput to matchBuildBaseURL.
ValidateBaseURLvalidates the raw string, butBuildBaseURLtrims whitespace/trailing slashes first. This can reject values that runtime later accepts.Suggested fix
func ValidateBaseURL(val string) error { - if val == "" { + normalized := strings.TrimRight(strings.TrimSpace(val), "/") + if normalized == "" { return nil } - parsed, err := url.Parse(val) + parsed, err := url.Parse(normalized) if err != nil || parsed.Scheme == "" || parsed.Host == "" { return fmt.Errorf("must be a fully-qualified URL with scheme and host (e.g. https://proxy.example.com), got %q", val) } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/ctx.go` around lines 624 - 637, ValidateBaseURL currently parses the raw val directly, which can reject strings that BuildBaseURL would accept; normalize the input first by trimming surrounding whitespace and any trailing slashes (same normalization BuildBaseURL uses), treat the trimmed-empty string as allowed, then parse the normalized value and validate parsed.Scheme and parsed.Host; update ValidateBaseURL to operate on the normalized value and keep the same error message and behavior otherwise (referencing the ValidateBaseURL and BuildBaseURL functions).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/handlers/config.go`:
- Around line 431-438: Run lib.ValidateBaseURL on
payload.ClientConfig.MCPExternalServerURL.GetValue() and
payload.ClientConfig.MCPExternalClientURL.GetValue() before any live mutations
(e.g., before calling UpdateDropExcessRequests, performing the MCP tool-manager
reload, compat plugin reload, or updating the in-memory MCP config). If either
validation fails, immediately SendError with StatusBadRequest and return so no
side effects are applied; move the existing validation block to the very start
of the handler (prior to the code that triggers those reloads/updates).
In `@transports/bifrost-http/lib/config.go`:
- Around line 707-712: The warning currently logged by ValidateBaseURL for
configData.Client.MCPExternalServerURL and MCPExternalClientURL does not prevent
those invalid values from being persisted; change the code so that when
ValidateBaseURL(...) returns an error you both log the warning and clear the
override (e.g., set client.MCPExternalServerURL = nil and
client.MCPExternalClientURL = nil) so the invalid values are not kept when
assigning config.ClientConfig = configData.Client; extract this logic into a
small helper (e.g., sanitizeMCPExternalOAuthURLs(client
*configstore.ClientConfig)) and call it for the client before assigning to
config.ClientConfig to ensure invalid overrides are discarded.
---
Duplicate comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 624-637: ValidateBaseURL currently parses the raw val directly,
which can reject strings that BuildBaseURL would accept; normalize the input
first by trimming surrounding whitespace and any trailing slashes (same
normalization BuildBaseURL uses), treat the trimmed-empty string as allowed,
then parse the normalized value and validate parsed.Scheme and parsed.Host;
update ValidateBaseURL to operate on the normalized value and keep the same
error message and behavior otherwise (referencing the ValidateBaseURL and
BuildBaseURL functions).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c785bae6-bf1c-4be3-8aa7-a091e9ab2d72
⛔ Files ignored due to path filters (1)
examples/mcps/oauth-demo-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
core/mcp/clientmanager.gocore/mcp/utils/utils.gocore/schemas/mcp.godocs/mcp/gateway-url.mdxdocs/mcp/oauth.mdxexamples/mcps/oauth-demo-server/go.modexamples/mcps/oauth-demo-server/main.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
- core/mcp/clientmanager.go
- framework/oauth2/main.go
Pratham-Mishra04
force-pushed
the
graphite-base/3211
branch
from
a9d6459 to
2525d89
Compare
Pratham-Mishra04
force-pushed
the
05-04-feat_add_refresh_token_expiry_reauth_flow_for_per_user_mcp_oauth
branch
from
e34323a to
553a797
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
transports/bifrost-http/lib/ctx.go (1)
632-637:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
ValidateBaseURLinput to matchBuildBaseURLbehavior.
ValidateBaseURLvalidates raw input, whileBuildBaseURLnormalizes withTrimSpace+ trailing-slash trim first. This can reject values that runtime would accept.Suggested fix
func ValidateBaseURL(val string) error { - if val == "" { + normalized := strings.TrimRight(strings.TrimSpace(val), "/") + if normalized == "" { return nil } - parsed, err := url.Parse(val) + parsed, err := url.Parse(normalized) if err != nil || parsed.Scheme == "" || parsed.Host == "" { return fmt.Errorf("must be a fully-qualified URL with scheme and host (e.g. https://proxy.example.com)") } return nil }#!/bin/bash # Verify normalization mismatch between ValidateBaseURL and BuildBaseURL rg -n -A6 -B2 'func ValidateBaseURL|func BuildBaseURL|TrimRight\(strings.TrimSpace|url.Parse\(' transports/bifrost-http/lib/ctx.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/ctx.go` around lines 632 - 637, ValidateBaseURL currently parses raw input directly; make it normalize the input exactly like BuildBaseURL by applying strings.TrimSpace and trimming trailing slashes (e.g., strings.TrimRight(..., "/")) before calling url.Parse. Update the ValidateBaseURL function to use the normalized value for parsing and for the subsequent checks (parsed.Scheme and parsed.Host) so inputs accepted at runtime by BuildBaseURL are also validated successfully.examples/mcps/oauth-demo-server/main.go (1)
568-605:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake refresh-token rotation atomic.
oldis loaded under one lock and revoked under another. Two concurrentgrant_type=refresh_tokenrequests can both pass the existence/expiry checks and each mint a fresh token pair, so the demo stops enforcing single-use refresh tokens and won't reliably reproduceinvalid_granton replay. Keep lookup, expiry check, deletion, and new-token insertion in one critical section, or splitissueTokensinto a locked helper for the refresh path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/mcps/oauth-demo-server/main.go` around lines 568 - 605, The refresh-token rotation is non-atomic: `old` is read under st.mu then revoked under a separate lock, allowing concurrent `grant_type=refresh_token` requests to both succeed; fix by performing lookup, expiry check (time.Now().After(old.RefreshExpiresAt)), deletion from `st.refreshTokens` and `st.accessTokens`, and insertion of the new token record returned by `issueTokens` while holding the same `st.mu` (or refactor `issueTokens` into a locked helper used only for the refresh path), then release the lock and emit logs; ensure `rt`, `old`, `rec`, `st.refreshTokens`, `st.accessTokens`, and `issueTokens` are the referenced symbols when implementing the atomic critical section.transports/bifrost-http/lib/config.go (1)
699-709:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't nil out file-backed MCP external URL overrides here.
These fields are mutated in-place on the same
configData.Clientobject that is later assigned intoconfig.ClientConfigand persisted on the no-DB/hash-mismatch paths. If the value was anenv.VARreference that is temporarily unset or invalid at startup, Lines 705 and 709 permanently erase that reference, so a later restart can no longer recover when the env var is fixed.💡 Minimal fix
func sanitizeMCPExternalOAuthURLs(client *configstore.ClientConfig) { if client == nil { return } if err := ValidateBaseURL(client.MCPExternalServerURL.GetValue()); err != nil { logger.Warn("mcp_external_server_url %v; override will be ignored and OAuth URLs will fall back to the request Host header", err) - client.MCPExternalServerURL = nil } if err := ValidateBaseURL(client.MCPExternalClientURL.GetValue()); err != nil { logger.Warn("mcp_external_client_url %v; override will be ignored and OAuth URLs will fall back to the request Host header", err) - client.MCPExternalClientURL = nil } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 699 - 709, sanitizeMCPExternalOAuthURLs currently mutates the passed configstore.ClientConfig by setting client.MCPExternalServerURL and client.MCPExternalClientURL to nil on validation failure, permanently erasing file/env-backed references; instead, stop mutating those fields in-place — leave client.MCPExternalServerURL and client.MCPExternalClientURL untouched when ValidateBaseURL returns an error and only treat the value as "ignored" at runtime (e.g., by returning an error/boolean, using a local copy, or setting an ephemeral in-memory flag) so that configData.Client later assigned into config.ClientConfig retains the original file/env reference and can recover on restart; update sanitizeMCPExternalOAuthURLs to implement this non-destructive behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/tables/oauth.go`:
- Around line 18-34: The BeforeSave hook in TableOauthConfig should set
EncryptionStatus = "encrypted" whenever encrypt.IsEnabled() is true (even if
ClientSecret/CodeVerifier are empty or unchanged) so startup batch encryption
won't repeatedly revisit discovery/public-client rows; update the
TableOauthConfig.BeforeSave method to check encrypt.IsEnabled() and
unconditionally set EncryptionStatus = "encrypted" in that branch (and set to
"plain_text" when encryption is disabled) while preserving existing encryption
logic for ClientSecret and CodeVerifier.
In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx`:
- Around line 291-331: The tooltip wrapper span should get a single computed
"disabled" flag so pointer-events-none is applied whenever the Button is
disabled for any reason (isPerUserOAuth, c.config.disabled,
reconnectingClients.includes(c.config.client_id), or !hasUpdateMCPClientAccess);
compute e.g. const reconnectDisabled = isPerUserOAuth || c.config.disabled ||
reconnectingClients.includes(c.config.client_id) || !hasUpdateMCPClientAccess
and use that for the span's className (pointer-events-none when true) while
keeping the Button's disabled prop set to the same reconnectDisabled and keeping
existing aria-label, onClick, and tooltip content logic (symbols to update: the
wrapping <span>, the className, and the Button disabled prop; refer to
reconnectingClients, isPerUserOAuth, c.config.disabled,
hasUpdateMCPClientAccess, handleReconnect).
---
Duplicate comments:
In `@examples/mcps/oauth-demo-server/main.go`:
- Around line 568-605: The refresh-token rotation is non-atomic: `old` is read
under st.mu then revoked under a separate lock, allowing concurrent
`grant_type=refresh_token` requests to both succeed; fix by performing lookup,
expiry check (time.Now().After(old.RefreshExpiresAt)), deletion from
`st.refreshTokens` and `st.accessTokens`, and insertion of the new token record
returned by `issueTokens` while holding the same `st.mu` (or refactor
`issueTokens` into a locked helper used only for the refresh path), then release
the lock and emit logs; ensure `rt`, `old`, `rec`, `st.refreshTokens`,
`st.accessTokens`, and `issueTokens` are the referenced symbols when
implementing the atomic critical section.
In `@transports/bifrost-http/lib/config.go`:
- Around line 699-709: sanitizeMCPExternalOAuthURLs currently mutates the passed
configstore.ClientConfig by setting client.MCPExternalServerURL and
client.MCPExternalClientURL to nil on validation failure, permanently erasing
file/env-backed references; instead, stop mutating those fields in-place — leave
client.MCPExternalServerURL and client.MCPExternalClientURL untouched when
ValidateBaseURL returns an error and only treat the value as "ignored" at
runtime (e.g., by returning an error/boolean, using a local copy, or setting an
ephemeral in-memory flag) so that configData.Client later assigned into
config.ClientConfig retains the original file/env reference and can recover on
restart; update sanitizeMCPExternalOAuthURLs to implement this non-destructive
behavior.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 632-637: ValidateBaseURL currently parses raw input directly; make
it normalize the input exactly like BuildBaseURL by applying strings.TrimSpace
and trimming trailing slashes (e.g., strings.TrimRight(..., "/")) before calling
url.Parse. Update the ValidateBaseURL function to use the normalized value for
parsing and for the subsequent checks (parsed.Scheme and parsed.Host) so inputs
accepted at runtime by BuildBaseURL are also validated successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42757af4-8f8f-48bc-b456-9a0172b0ba21
⛔ Files ignored due to path filters (1)
examples/mcps/oauth-demo-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
core/mcp/clientmanager.gocore/mcp/utils/utils.gocore/schemas/mcp.godocs/mcp/gateway-url.mdxdocs/mcp/oauth.mdxexamples/mcps/oauth-demo-server/go.modexamples/mcps/oauth-demo-server/main.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/mcps/oauth-demo-server/go.mod
Pratham-Mishra04
force-pushed
the
graphite-base/3211
branch
from
2525d89 to
d45fb1b
Compare
Pratham-Mishra04
force-pushed
the
05-04-feat_add_refresh_token_expiry_reauth_flow_for_per_user_mcp_oauth
branch
from
553a797 to
825ea3b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx (1)
307-314:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one
isReconnectDisabledflag for bothdisabledand pointer-event passthrough.The button is disabled for four conditions, but
pointer-events-noneonly covers two. Tooltip hover can still fail forreconnecting/ RBAC-disabled states.💡 Suggested fix
+const isReconnectDisabled = + isPerUserOAuth || + c.config.disabled || + reconnectingClients.includes(c.config.client_id) || + !hasUpdateMCPClientAccess; + <Button @@ - disabled={ - isPerUserOAuth || - c.config.disabled || - reconnectingClients.includes(c.config.client_id) || - !hasUpdateMCPClientAccess - } - className={isPerUserOAuth || c.config.disabled ? "pointer-events-none" : undefined} + disabled={isReconnectDisabled} + className={isReconnectDisabled ? "pointer-events-none" : undefined} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx` around lines 307 - 314, Create a single boolean flag (e.g., isReconnectDisabled) that combines the four conditions currently used to set disabled (isPerUserOAuth || c.config.disabled || reconnectingClients.includes(c.config.client_id) || !hasUpdateMCPClientAccess) and use that one flag for both the button's disabled prop and the className ternary that applies "pointer-events-none"; locate where the button is rendered (references: isPerUserOAuth, c.config.disabled, reconnectingClients.includes(c.config.client_id), hasUpdateMCPClientAccess) and replace the repeated condition with the new isReconnectDisabled variable so tooltip/pointer behavior is consistent across all disabling reasons.transports/bifrost-http/lib/config.go (1)
703-709:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not clear
mcp_external_*_urlon validation failure; preserve EnvVar references.Setting
client.MCPExternalServerURL = nil/client.MCPExternalClientURL = nildropsenv.VARreferences when startup validation fails transiently (e.g., env not yet available), so later restarts cannot recover from the original reference.💡 Suggested fix
func sanitizeMCPExternalOAuthURLs(client *configstore.ClientConfig) { if client == nil { return } if err := ValidateBaseURL(client.MCPExternalServerURL.GetValue()); err != nil { logger.Warn("mcp_external_server_url %v; override will be ignored and OAuth URLs will fall back to the request Host header", err) - client.MCPExternalServerURL = nil } if err := ValidateBaseURL(client.MCPExternalClientURL.GetValue()); err != nil { logger.Warn("mcp_external_client_url %v; override will be ignored and OAuth URLs will fall back to the request Host header", err) - client.MCPExternalClientURL = nil } }Based on learnings: “
mcp_external_server_urlandmcp_external_client_url… intentionally warn but keep the originalEnvVarvalue rather than clearing it … Do not flag or suggest clearing these fields on validation failure.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 703 - 709, The validation code currently clears client.MCPExternalServerURL and client.MCPExternalClientURL on ValidateBaseURL failure which drops EnvVar references; instead remove the assignments that set those fields to nil and only emit the warning via logger.Warn so the original EnvVar-backed values remain intact for later retries; locate the ValidateBaseURL checks around client.MCPExternalServerURL.GetValue() and client.MCPExternalClientURL.GetValue() and delete the lines that assign nil while keeping the warning behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/oauth2/main.go`:
- Around line 863-878: The current flow fetches a session via
GetOauthUserSessionBySessionToken and then blindly mutates fields like
existing.MCPClientID, OauthConfigID, State, RedirectURI, etc., which can
reassign a session to a different MCP client; change the logic in the block
handling existing (the code that sets existing.MCPClientID,
existing.OauthConfigID, existing.State, existing.RedirectURI,
existing.CodeVerifier, existing.VirtualKeyID, existing.UserID, existing.Status,
existing.ExpiresAt and calls UpdateOauthUserSession) to first verify the session
belongs to the same MCP client (compare existing.MCPClientID to mcpClientID and
existing.OauthConfigID to oauthConfigID), and if they differ, do not update the
row—instead return an error (or create/return a new isolated session token) so
you never repoint an existing session to a different MCP client; ensure you use
GetOauthUserSessionBySessionToken and UpdateOauthUserSession as the modification
points.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 703-709: The validation code currently clears
client.MCPExternalServerURL and client.MCPExternalClientURL on ValidateBaseURL
failure which drops EnvVar references; instead remove the assignments that set
those fields to nil and only emit the warning via logger.Warn so the original
EnvVar-backed values remain intact for later retries; locate the ValidateBaseURL
checks around client.MCPExternalServerURL.GetValue() and
client.MCPExternalClientURL.GetValue() and delete the lines that assign nil
while keeping the warning behavior.
In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx`:
- Around line 307-314: Create a single boolean flag (e.g., isReconnectDisabled)
that combines the four conditions currently used to set disabled (isPerUserOAuth
|| c.config.disabled || reconnectingClients.includes(c.config.client_id) ||
!hasUpdateMCPClientAccess) and use that one flag for both the button's disabled
prop and the className ternary that applies "pointer-events-none"; locate where
the button is rendered (references: isPerUserOAuth, c.config.disabled,
reconnectingClients.includes(c.config.client_id), hasUpdateMCPClientAccess) and
replace the repeated condition with the new isReconnectDisabled variable so
tooltip/pointer behavior is consistent across all disabling reasons.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a047924b-5562-4a18-805b-16b128dbbbe4
⛔ Files ignored due to path filters (1)
examples/mcps/oauth-demo-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
core/mcp/clientmanager.gocore/mcp/utils/utils.gocore/schemas/mcp.godocs/mcp/gateway-url.mdxdocs/mcp/oauth.mdxexamples/mcps/oauth-demo-server/go.modexamples/mcps/oauth-demo-server/main.goframework/configstore/tables/oauth.goframework/oauth2/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
Contributor
Merge activity
|
akshaydeo
deleted the
05-04-feat_add_refresh_token_expiry_reauth_flow_for_per_user_mcp_oauth
branch
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
3 participants
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.
Summary
This PR improves the robustness and observability of Bifrost's per-user OAuth flow. It fixes a re-authentication bug where a user whose refresh token was permanently rejected (or whose token row was purged) could not re-initiate the OAuth flow due to a unique constraint violation on the session token. It also ensures that
ErrOAuth2TokenExpiredis treated as a re-auth signal alongsideErrOAuth2TokenNotFound, and adds a self-contained OAuth demo server for end-to-end testing of the full per-user OAuth path including token refresh.Changes
InitiateUserOAuthFlownow checks for an existing session row by session token before inserting. If one exists, it updates the row in place (rotating state, PKCE verifier, and expiry) rather than inserting a duplicate, avoiding unique constraint failures on re-authentication.RefreshUserAccessTokennow detects permanent upstream rejections (HTTP 401, or RFC 6749invalid_grant/unauthorized_client400s), purges the dead token row, marks the session asneeds_reauth, and returnsErrOAuth2TokenExpiredso the caller surfaces an inline auth URL.ErrOAuth2TokenExpiredas re-auth sentinel:ResolvePerUserOAuthTokennow treats bothErrOAuth2TokenNotFoundandErrOAuth2TokenExpiredas signals to fall through to the re-auth branch, rather than surfacing them as errors.ErrMCPReconnectNotApplicablesentinel: A new typed error distinguishes "reconnect is not meaningful for this client type" from a generic failure. The HTTP handler returns 400 (not 500) when this error is encountered.ErrMCPReconnectNotApplicablewith a descriptive message explaining why no shared connection exists.examples/mcps/oauth-demo-server) implements RFC 9728, RFC 8414, RFC 7591, and the authorization code + PKCE + refresh token flow with a deliberately short (30s) access token TTL to make the refresh path easy to observe.Tooltipinstead of a nativetitleattribute, so the explanation is visible even though the button is disabled.mcp_external_client_urlwarning: A warning is added to the config UI explaining that changing this URL after clients have completed OAuth will break them due to the lockedredirect_uri.redirect_urilock warnings: Warnings are added togateway-url.mdxandoauth.mdxexplaining that theredirect_uriis locked at Dynamic Client Registration time and how to recover if the URL changes.RevokeTokensuccess log is demoted fromInfotoDebugto reduce noise.Type of change
Affected areas
How to test
End-to-end per-user OAuth with refresh:
Re-auth after token purge:
Reconnect API for per-user OAuth:
curl -X POST /api/mcp/clients/{id}/reconnect # Expect: 400 Bad Request with descriptive message, not 500UI:
cd ui pnpm i pnpm buildmcp_external_client_urlfield shows the redirect URI warning.Breaking changes
The reconnect endpoint now returns 400 instead of 500 for per-user OAuth clients. Callers that were matching on 500 for this case should be updated to handle 400.
Security considerations
Checklist
docs/contributing/README.mdand followed the guidelines