feat: add pending_verification state and initiate-verification endpoint for config.json shared-OAuth MCP clients - #3871
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds pending OAuth configuration and encrypted persistence for MCP clients. Adds a pending-verification lifecycle, preserves server-side OAuth state during reconciliation, and adds shared-OAuth verification initiation and completion flows. ChangesMCP Shared-OAuth Pending Verification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPHandler
participant OAuthFlow
participant ConfigStore
Client->>MCPHandler: Initiate MCP client verification
MCPHandler->>ConfigStore: Load pending shared-OAuth client
MCPHandler->>OAuthFlow: Start OAuth bootstrap
OAuthFlow-->>MCPHandler: Return authorization metadata
MCPHandler->>ConfigStore: Link OAuth configuration ID
Client->>MCPHandler: Complete OAuth authorization
MCPHandler->>ConfigStore: Resolve client and clear pending configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
|
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (6): Last reviewed commit: "feat: support for shared oauth mcp serve..." | Re-trigger Greptile |
90587d2 to
f63648d
Compare
634594d to
bd51a12
Compare
f63648d to
73722bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@core/schemas/mcp.go`:
- Around line 326-342: Config.RedactMCPClientConfig currently leaves
PendingOAuthConfig untouched, so the inline oauth_config block can leak
plaintext secrets. Update the redaction path for MCP client configs to handle
PendingOAuthConfig the same way as the sibling OAuth secret fields, using the
existing OAuth2Config redaction/encryption logic so OAuth2Config.ClientSecret is
not returned verbatim.
In `@framework/configstore/clientconfig.go`:
- Around line 1449-1455: GenerateMCPClientHash should not rely only on
PendingOAuthConfigJSON for the oauth_config contribution, because it can be nil
on freshly built rows before BeforeSave runs. Update the hash logic in
GenerateMCPClientHash to fall back to hashing the runtime PendingOAuthConfig
when PendingOAuthConfigJSON is empty, while preserving the existing
deterministic JSON path when the persisted mirror is present.
In `@framework/configstore/tables/mcp.go`:
- Around line 205-218: PendingOAuthConfigJSON is being serialized in plain text
while the same hook encrypts other sensitive fields. Update the save logic in
the MCP table hook around c.PendingOAuthConfig / c.PendingOAuthConfigJSON to
route this field through the existing encrypt.IsEnabled() handling alongside
ConnectionString and HeadersJSON, and add the corresponding decrypt/rehydration
path in AfterFind so PendingOAuthConfig is restored correctly. If OAuth2Config
includes a secret, ensure the encrypted persistence path covers it consistently
with the other protected fields.
- Around line 50-59: `PendingOAuthConfigJSON` is missing an explicit GORM column
mapping, so it may not resolve to the intended database field. Update the
`PendingOAuthConfigJSON` field in `mcp.go` to pin it to
`pending_oauth_config_json` with a column tag, keeping it aligned with the
migration and any raw `Updates()` usage. Use the `PendingOAuthConfigJSON` struct
field as the anchor for the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1230292-6009-4af4-a826-bcac11f81c28
📒 Files selected for processing (10)
core/mcp/clientmanager.gocore/schemas/mcp.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.go
73722bd to
697b86f
Compare
77274df to
78aaeec
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…ootstrap cleanup failure - initiateMCPClientVerification now checks h.store.ConfigStore for nil before dereferencing, matching every other handler in this file. - pendingOAuthConfigToRequest now carries the config.json oauth_config's Resource (RFC 8707) into the OAuth initiation request instead of silently dropping it. - completeMCPClientOAuth now reports failure (mirroring the existing [PARTIAL SUCCESS] pattern used for the VK-assignment case) when clearing a config.json client's pending-bootstrap stash fails, instead of returning success while DB state can regress the client to pending_verification after a restart.
78aaeec to
30b6653
Compare
Merge activity
|

Summary
Shared-OAuth MCP clients declared in
config.json(i.e.auth_type: oauthwith an inlineoauth_configblock but no pre-existingoauth_configsrow or token) previously had no path to complete authorization — attempting to connect would fail immediately inConnectionHeaders → GetAccessToken. This PR introduces apending_verificationstate and a newPOST /api/mcp/client/{id}/initiate-verificationendpoint so an admin can trigger the browser OAuth flow for these config-file-originated clients after the server starts.Changes
New
MCPConnectionStatePendingVerificationstate: clients withPendingOAuthConfigset are parked in this state on startup instead of attempting a connection that would fail. Once the OAuth callback marks the linkedoauth_configsrow authorized, the stash is cleared and the next reconnect takes the normal connect path.PendingOAuthConfigfield onMCPClientConfigandTableMCPClient: holds the inlineoauth_configblock fromconfig.json. Serialized to/from a newpending_oauth_config_jsoncolumn viaBeforeSave/AfterFindhooks. Cleared byClearMCPClientPendingOAuthConfigonce authorization succeeds.DB migration (
migrationAddMCPClientPendingOAuthConfigJSONColumn): adds the nullablepending_oauth_config_jsontext column toconfig_mcp_clients.POST /api/mcp/client/{id}/initiate-verificationendpoint: reads the persistedPendingOAuthConfigstash, runs the same OAuth bootstrap dance as the UI Create flow (via the new sharedrunOAuthBootstraphelper), links the freshly createdoauth_configsrow to the MCP client viaUpdateMCPClientOAuthConfigID, and returns the authorize URL.runOAuthBootstraphelper: extracts the shared OAuth initiation logic (redirect URI construction,InitiateOAuthFlowcall) fromaddMCPClientso both the UI Create flow and the config-bootstrap flow use identical code paths and cannot drift.completeMCPClientOAuthconfig-bootstrap fallback: whenmcp_client_config_jsonon theoauth_configsrow is nil (config.json-originated clients don't populate it), the handler now resolves the MCP client byoauth_config_idinstead of returning 404. After authorization,ClearMCPClientPendingOAuthConfigdrops the stash so the client reconnects normally.New
ConfigStoremethods:GetMCPClientByOauthConfigID,UpdateMCPClientOAuthConfigID,ClearMCPClientPendingOAuthConfig.Hash coverage:
GenerateMCPClientHashnow includesPendingOAuthConfigJSONso edits to the inlineoauth_configblock inconfig.jsontrigger reconciliation.pendingOAuthConfigToRequestconverter: maps the plain-stringOAuth2Configfields to the*EnvVar-typedOAuthConfigRequestshape expected byInitiateOAuthFlow.Type of change
Affected areas
How to test
config.jsonwithauth_type: oauthand an inlineoauth_configblock (nooauth_config_id).pending_verificationstate rather than erroring.POST /api/mcp/client/{id}/initiate-verificationas an admin. Expect a200response containingauthorize_url,oauth_config_id, andexpires_at.authorize_url.pending_verificationand intoconnected.pending_verification).go test ./...Breaking changes
Security considerations
PendingOAuthConfigstores OAuth client credentials as plaintext in thepending_oauth_config_jsoncolumn. This mirrors the existing behavior for other credential columns in the same table (which are encrypted at rest via the existingEncryptionStatusmechanism). Env-var reference resolution is intentionally not applied to fields inside this block, consistent with the documented behavior.completeMCPClientOAuthfallback path gates onauth_type='oauth'and a non-nullPendingOAuthConfigJSONto prevent replay attacks and cross-type misuse, returning409 Conflictif the bootstrap has already been completed.Checklist
docs/contributing/README.mdand followed the guidelines