feat(mcp): support pre-registered OAuth credentials for non-DCR providers - #5614
Open
adhyaay-karnwal wants to merge 1 commit into
Open
feat(mcp): support pre-registered OAuth credentials for non-DCR providers#5614adhyaay-karnwal wants to merge 1 commit into
adhyaay-karnwal wants to merge 1 commit into
Conversation
…ders MCP servers whose authorization server doesn't support Dynamic Client Registration (e.g. HubSpot) couldn't be connected: the OAuth flow always attempted DCR, which has no registration_endpoint to POST to. Let users supply an optional client_id/client_secret on the Add-server dialog (and edit them via a new configure dialog), persist the secret encrypted on the server record, and consult them before falling back to DCR. Fixes every non-DCR provider without vendor-specific code.
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughMCP servers now support optional pre-registered OAuth client IDs and secrets. The database stores encrypted secrets, and APIs validate, update, clear, and report credential metadata. OAuth authorization uses stored credentials before provider or dynamic registration and preserves them during token exchange. The settings interface adds credential entry, secret visibility controls, and configuration dialogs. Tests cover validation, response mapping, persistence behavior, and OAuth client selection. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/features/settings/Integrations.tsx`:
- Around line 119-131: Update handleSubmit and the Add control in Integrations
so a non-empty clientSecretValue without a clientIdValue cannot submit or call
addMutation.mutate. Return a form error or disable Add for this invalid pair,
while preserving both credential fields when clientIdValue is present and
providing feedback instead of silently discarding the secret.
- Around line 314-326: Update the Integrations save flow around handleSave to
track explicit clearing of an existing stored secret, adding a “Clear stored
secret” control near the client-secret input. When selected, include
client_secret as an empty string in updateMutation.mutate even when the client
ID is retained; otherwise preserve the current omission behavior for untouched
empty input.
In `@crates/mcp_client/src/inbound/axum_router.rs`:
- Around line 272-274: Update the API annotations for both add_server and
update_server to declare the HTTP 400 response used for OAuthRejected,
MalformedCallback, and InvalidCredentials errors, then regenerate the API
specifications so POST /mcp/servers and PUT /mcp/servers expose the validation
response.
🪄 Autofix
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: 85633a28-2be9-4cef-997f-553bf9ca13ac
⛔ Files ignored due to path filters (14)
.sqlx/query-69895e1fdfef3998c26e8b62496504f96dc5d617be95ef79dd541acca264ddb4.jsonis excluded by!**/.sqlx/**.sqlx/query-94c66b7510d0a9f791b7a00521ae9975e965670662132acfc60e18667c53370d.jsonis excluded by!**/.sqlx/**.sqlx/query-aaa8d664de60cdcea278ee2e013ef2270b53cbc0c7b13ff2bae7617ff201ee2d.jsonis excluded by!**/.sqlx/**.sqlx/query-d90855304ae7f4a8ca06018d4dfd88b138c1b95d46a31c63b9e93ac2198b037c.jsonis excluded by!**/.sqlx/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/addServerRequest.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/addServerRequestClientId.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/addServerRequestClientSecret.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/index.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/serverResponse.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/serverResponseClientId.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/updateServerRequest.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/updateServerRequestClientId.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**apps/web/src/lib/service-clients/service-cognition/generated/schemas/updateServerRequestClientSecret.tsis excluded by!**/generated/**,!apps/web/src/lib/service-clients/**/generated/**packages/sdk/generated/cognition/types.gen.tsis excluded by!**/generated/**,!**/*.gen.ts
📒 Files selected for processing (13)
apps/web/src/features/settings/Integrations.tsxapps/web/src/lib/service-clients/service-cognition/openapi.jsoncrates/macro_db_client/migrations/20260812191214_add_mcp_servers_oauth_credentials.down.sqlcrates/macro_db_client/migrations/20260812191214_add_mcp_servers_oauth_credentials.sqlcrates/mcp_client/src/domain/models/server.rscrates/mcp_client/src/domain/service/persisting_credential_store/test.rscrates/mcp_client/src/inbound/axum_router.rscrates/mcp_client/src/inbound/axum_router/test.rscrates/mcp_client/src/outbound/oauth.rscrates/mcp_client/src/outbound/oauth/test.rscrates/mcp_client/src/outbound/pg_server_repo.rscrates/onboarding/src/domain/service/test.rspackages/sdk/specs/cognition.json
Comment on lines
119
to
+131
| addMutation.mutate( | ||
| { server_name: n, url: u }, | ||
| { | ||
| server_name: n, | ||
| url: u, | ||
| ...(clientIdValue | ||
| ? { | ||
| client_id: clientIdValue, | ||
| ...(clientSecretValue | ||
| ? { client_secret: clientSecretValue } | ||
| : {}), | ||
| } | ||
| : {}), | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Block submission when a client secret has no client ID.
The UI displays an error for this state, but handleSubmit silently omits clientSecretValue when clientIdValue is empty. It then adds the server and starts authorization.
Disable Add for this invalid credential pair, or return a form error before the mutation. Do not discard entered secret data without feedback.
🤖 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 `@apps/web/src/features/settings/Integrations.tsx` around lines 119 - 131,
Update handleSubmit and the Add control in Integrations so a non-empty
clientSecretValue without a clientIdValue cannot submit or call
addMutation.mutate. Return a form error or disable Add for this invalid pair,
while preserving both credential fields when clientIdValue is present and
providing feedback instead of silently discarding the secret.
Comment on lines
+314
to
+326
| const handleSave = () => { | ||
| const cid = clientId().trim(); | ||
| const csecret = clientSecret().trim(); | ||
| // Clearing the id also clears the stored secret, so we never leave a | ||
| // secret behind without an id (which the backend rejects). | ||
| const clearedId = cid === '' && (props.server.client_id ?? '') !== ''; | ||
|
|
||
| updateMutation.mutate( | ||
| { | ||
| url: props.server.url, | ||
| client_id: cid, | ||
| ...(csecret !== '' || clearedId ? { client_secret: csecret } : {}), | ||
| }, |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add an explicit stored-secret clear action.
clientSecret() starts empty when a secret already exists. Line 325 omits client_secret for an empty value, so Save preserves the existing secret. A user cannot remove the stored secret while retaining the client ID.
Add explicit clear state, such as a “Clear stored secret” control. Send client_secret: '' when that control is selected.
🤖 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 `@apps/web/src/features/settings/Integrations.tsx` around lines 314 - 326,
Update the Integrations save flow around handleSave to track explicit clearing
of an existing stored secret, adding a “Clear stored secret” control near the
client-secret input. When selected, include client_secret as an empty string in
updateMutation.mutate even when the client ID is retained; otherwise preserve
the current omission behavior for untouched empty input.
Comment on lines
+272
to
+274
| McpHandlerErr::OAuthRejected(_) | ||
| | McpHandlerErr::MalformedCallback | ||
| | McpHandlerErr::InvalidCredentials(_) => StatusCode::BAD_REQUEST, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document the HTTP 400 validation response.
Lines 272-274 make add_server and update_server return HTTP 400 when credential validation fails. The generated specifications do not declare HTTP 400 for POST /mcp/servers or PUT /mcp/servers.
Add the error response to both handler API annotations and regenerate the specifications. This keeps generated clients and API consumers aligned with runtime behavior.
🤖 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 `@crates/mcp_client/src/inbound/axum_router.rs` around lines 272 - 274, Update
the API annotations for both add_server and update_server to declare the HTTP
400 response used for OAuthRejected, MalformedCallback, and InvalidCredentials
errors, then regenerate the API specifications so POST /mcp/servers and PUT
/mcp/servers expose the validation response.
|
|
||
| impl PgServerRepo { | ||
| #[tracing::instrument(skip_all, err)] | ||
| #[expect(clippy::too_many_arguments, reason = "maps a DB row into a record")] |
Member
There was a problem hiding this comment.
nit: prefer creating a CreateRecordArgs struct that will contain all db arguments to create the row.
Comment on lines
+225
to
+230
| let client_secret = client_secret | ||
| .map(|b| { | ||
| let plaintext = self.decrypt_bytes(&b)?; | ||
| String::from_utf8(plaintext).map_err(|e| sqlx::Error::Decode(Box::new(e))) | ||
| }) | ||
| .transpose()?; |
Member
There was a problem hiding this comment.
Please decode client_secret the same way we do for credentials
let credentials = credentials.map(|c| self.decrypt(&c)).transpose()?;
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
2 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
Fixes #5598 — MCP servers whose authorization server doesn't support Dynamic Client Registration (e.g. HubSpot) previously couldn't be connected. The OAuth flow always attempted DCR, which has no
registration_endpointto POST to, so authorization failed before the user ever reached a consent screen.This adds optional per-server OAuth credentials (client ID + client secret) so any non-DCR provider is connectable without vendor-specific code or hosted env vars.
What changed
Backend (
mcp_client)McpServerRecordgainsclient_id/client_secretfields; the secret is#[serde(skip)]and never leaves the service.PgServerRepostores the client id as plaintext and encrypts the client secret with AES-256-GCM at rest (the same scheme used for the OAuth grant), via a new migration addingmcp_servers.client_id/mcp_servers.client_secret.OAuthServiceconsults per-server credentials before the provider registry and DCR:A client secret supplied without a client id is rejected with a 400.
add_server/update_serveracceptclient_id/client_secret; re-adding a server preserves stored credentials, and an empty string clears them.ServerResponseexposesclient_idand ahas_client_secretboolean (never the secret).Frontend (
apps/web)Generated artifacts
apps/web) and openapi-ts (packages/sdk) clients from the updated OpenAPI spec, and refreshed the.sqlxoffline query cache.Testing
cargo test -p mcp_client— 23 passed (includes new tests: pre-registered credentials skip DCR; credential normalization/validation; response mapping).cargo test -p onboarding— 6 passed.cargo clippy -p mcp_client --all-features— clean.cargo check --all-features(viacargo sqlx prepare --workspace) — clean.tsctype-check andbiome— clean.