From 265fb626dba7e719d81f035de41e1c110325389c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 21 Oct 2026 01:58:23 -0700 Subject: [PATCH 1/3] feat(hosted-mcp): register a confidential client when the vendor offers no public one TASK-248. gitlab.com and api.supabase.com list only client_secret_basic and client_secret_post, so the DCR registry refused them before any POST and neither entry could connect (#2331, #2332 wait on this). - When `none` is not listed, register with client_secret_basic, or client_secret_post when that is the only one; the answer must carry a secret and register that method. - The secret is stored encrypted in the connector-secret store, owned by the registration (new kind hosted-mcp-dcr-client-secret); the row keeps only the ref, the method and the expiry. - Exchange, refresh and revocation authenticate with it; PKCE still rides. endpointAuthentication recognises a confidential DCR client by its secret, so callers that pass only clientId/clientSecret still send it. - A secret lapsing within 24 h is replaced at the next start; a refused registration's secret is deleted at once; a race loser deletes its own. - The orphan sweep keeps a registration-owned secret while its registration exists, is not refused and still names it. - The connectors page names client_registration_unsupported and client_registration_refused. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4 --- .../service/hostedMcpDcrRegistry.test.js | 239 +++++++++++++++++- .../installableInstallationService.test.js | 56 +++- backend/models/HostedMcpDcrRegistration.ts | 18 +- backend/services/connectorSecretKinds.ts | 35 ++- backend/services/hostedMcpDcrRegistry.ts | 176 +++++++++++-- backend/services/hostedMcpIntakeService.ts | 63 +++-- .../installable/installableReconciler.ts | 20 +- docs/plans/hosted-mcp-connection-scope.md | 20 +- .../v2/__tests__/V2ConnectorsPage.test.tsx | 2 + .../src/v2/components/hostedMcpErrorNotice.ts | 4 + 10 files changed, 576 insertions(+), 57 deletions(-) diff --git a/backend/__tests__/service/hostedMcpDcrRegistry.test.js b/backend/__tests__/service/hostedMcpDcrRegistry.test.js index 76e8b0940e..d4525426f7 100644 --- a/backend/__tests__/service/hostedMcpDcrRegistry.test.js +++ b/backend/__tests__/service/hostedMcpDcrRegistry.test.js @@ -1,4 +1,6 @@ process.env.PG_HOST = ''; +process.env.CONNECTOR_SECRET_KEYS = `dcr-test:${Buffer.alloc(32, 7).toString('base64')}`; +process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'dcr-test'; const Model = require('../../models/HostedMcpDcrRegistration'); const { @@ -7,10 +9,20 @@ const { refuseDcrClient, } = require('../../services/hostedMcpDcrRegistry'); const { setupMongoDb, clearMongoDb, closeMongoDb } = require('../utils/testUtils'); +const ConnectorSecret = require('../../models/ConnectorSecret'); +const connectorSecrets = require('../../services/connectorSecrets'); +const { + buildTokenExchangeRequest, buildRefreshRequest, buildRevocationRequest, +} = require('../../services/hostedMcpIntakeService'); const entry = { - id: 'close', title: 'Close', client: 'dcr', issuer: 'https://auth.close.com', - resource: 'https://mcp.close.com/mcp', scopes: ['mcp.read'], tools: [], + id: 'close', + title: 'Close', + client: 'dcr', + issuer: 'https://auth.close.com', + resource: 'https://mcp.close.com/mcp', + scopes: ['mcp.read'], + tools: [], }; const callback = 'https://api.commonly.me/api/integrations/connect/hosted-mcp/close/callback'; const metadata = { @@ -34,7 +46,7 @@ describe('instance-scoped hosted MCP DCR registry', () => { beforeAll(async () => { await setupMongoDb(); }); - beforeEach(async () => { await Model.deleteMany({}); }); + beforeEach(async () => { await Model.deleteMany({}); await ConnectorSecret.deleteMany({}); }); afterAll(async () => { await clearMongoDb(); await closeMongoDb(); }); test('first start registers a public client once and a second reuses it', async () => { @@ -114,19 +126,228 @@ describe('instance-scoped hosted MCP DCR registry', () => { expect(await Model.findOne({ clientId: 'old' })).toMatchObject({ current: false }); }); - test('refuses AS metadata without none before registration and refuses a secret-bearing answer', async () => { + // TASK-248 inverted the first half of this arm: a vendor listing only + // client_secret_* methods now gets a confidential client (below). What stays + // refused before any POST is a vendor naming no method this registry + // supports, or naming none at all. + test('refuses before any POST when the vendor names no supported registration method', async () => { const fetch = registrationFetch([]); - await expect(ensureDcrClient(entry, { - ...metadata, token_endpoint_auth_methods_supported: ['client_secret_basic'], - }, callback, fetch)).rejects.toMatchObject({ code: 'client_registration_unsupported' }); + for (const supported of [['private_key_jwt'], [], undefined]) { + // eslint-disable-next-line no-await-in-loop + await expect(ensureDcrClient(entry, { + ...metadata, token_endpoint_auth_methods_supported: supported, + }, callback, fetch)).rejects.toMatchObject({ code: 'client_registration_unsupported' }); + } expect(fetch).not.toHaveBeenCalled(); + expect(await Model.countDocuments({})).toBe(0); + }); + + test('a public registration answered with a secret is still refused, and nothing is stored', async () => { const secretAnswer = jest.fn(async () => new Response(JSON.stringify({ - client_id: 'private', client_secret: 'must-not-store', - token_endpoint_auth_method: 'client_secret_basic', redirect_uris: [callback], + client_id: 'private', + client_secret: 'must-not-store', + token_endpoint_auth_method: 'client_secret_basic', + redirect_uris: [callback], }), { status: 201 })); await expect(ensureDcrClient(entry, metadata, callback, secretAnswer)) .rejects.toMatchObject({ code: 'client_registration_unsupported' }); expect(await Model.countDocuments({})).toBe(0); + expect(await ConnectorSecret.countDocuments({})).toBe(0); + }); + + describe('a vendor that offers only confidential registration (TASK-248)', () => { + const confidential = { ...metadata, token_endpoint_auth_methods_supported: ['client_secret_basic', 'client_secret_post'] }; + const confidentialAnswer = (seen, overrides = {}) => jest.fn(async (_url, init) => { + seen.push(JSON.parse(init.body)); + return new Response(JSON.stringify({ + client_id: 'conf-1', + client_secret: 'issued-secret-1', + token_endpoint_auth_method: 'client_secret_basic', + redirect_uris: [callback], + client_secret_expires_at: 0, + registration_access_token: 'never-stored', + ...overrides, + }), { status: 201 }); + }); + + test('registers with the vendor\'s listed method, keeps the secret encrypted and owned by the row, and reuses it', async () => { + const sent = []; + const fetch = confidentialAnswer(sent); + const first = await ensureDcrClient(entry, confidential, callback, fetch); + const second = await ensureDcrClient(entry, confidential, callback, fetch); + expect(first).toEqual({ clientId: 'conf-1', clientSecret: 'issued-secret-1', tokenEndpointAuthMethod: 'client_secret_basic' }); + expect(second).toEqual(first); + expect(fetch).toHaveBeenCalledTimes(1); + expect(sent[0]).toMatchObject({ token_endpoint_auth_method: 'client_secret_basic', redirect_uris: [callback] }); + + const row = (await Model.findOne({})).toObject(); + expect(row.tokenEndpointAuthMethod).toBe('client_secret_basic'); + expect(JSON.stringify(row)).not.toMatch(/issued-secret-1|never-stored/); + const secret = await ConnectorSecret.findById(row.clientSecretRef).lean(); + expect(String(secret.integrationId)).toBe(String(row._id)); + expect(secret.kind).toBe('hosted-mcp-dcr-client-secret'); + expect(secret.ciphertext).not.toMatch(/issued-secret-1/); + expect(await connectorSecrets.get(row.clientSecretRef)).toBe('issued-secret-1'); + }); + + test('two concurrent first starts keep only the winner\'s secret, and both use the winner\'s client', async () => { + await Model.init(); + let release; + const bothPosted = new Promise((resolve) => { release = resolve; }); + let posts = 0; + const fetch = jest.fn(async () => { + posts += 1; + const n = posts; + if (posts === 2) release(); + await bothPosted; + return new Response(JSON.stringify({ + client_id: `conf-race-${n}`, + client_secret: `race-secret-${n}`, + token_endpoint_auth_method: 'client_secret_basic', + redirect_uris: [callback], + }), { status: 201 }); + }); + const [a, b] = await Promise.all([ + ensureDcrClient(entry, confidential, callback, fetch), + ensureDcrClient(entry, confidential, callback, fetch), + ]); + expect(posts).toBe(2); + expect(a).toEqual(b); + const winner = await Model.findOne({ current: true }).lean(); + expect(a.clientId).toBe(winner.clientId); + // The loser's secret is deleted; only the stored registration's remains. + const secrets = await ConnectorSecret.find({}).lean(); + expect(secrets.map((secret) => String(secret._id))).toEqual([winner.clientSecretRef]); + }); + + test('client_secret_post when that is the only secret method the vendor lists', async () => { + const sent = []; + const client = await ensureDcrClient( + entry, + { ...metadata, token_endpoint_auth_methods_supported: ['client_secret_post'] }, + callback, + confidentialAnswer(sent, { token_endpoint_auth_method: 'client_secret_post' }), + ); + expect(sent[0].token_endpoint_auth_method).toBe('client_secret_post'); + expect(client.tokenEndpointAuthMethod).toBe('client_secret_post'); + }); + + test('an answer without a secret, or with another auth method, is refused and stores nothing', async () => { + const postOnly = { ...metadata, token_endpoint_auth_methods_supported: ['client_secret_post'] }; + for (const [vendor, overrides] of [ + [confidential, { client_secret: undefined }], + [confidential, { client_secret: '' }], + [confidential, { token_endpoint_auth_method: 'client_secret_post' }], + // RFC 7591: an omitted method means client_secret_basic, a mismatch for a post-only vendor. + [postOnly, { token_endpoint_auth_method: undefined }], + ]) { + // eslint-disable-next-line no-await-in-loop + await expect(ensureDcrClient(entry, vendor, callback, confidentialAnswer([], overrides))) + .rejects.toMatchObject({ code: 'client_registration_unsupported' }); + } + expect(await Model.countDocuments({})).toBe(0); + expect(await ConnectorSecret.countDocuments({})).toBe(0); + }); + + test('a secret that lapses is replaced by registering again before it does', async () => { + const now = new Date('2026-10-21T12:00:00Z'); + // Each secret the vendor issues lapses two days after the first start. + const day = 24 * 60 * 60 * 1000; + let n = 0; + const fetch = jest.fn(async () => { + n += 1; + return new Response(JSON.stringify({ + client_id: `conf-${n}`, + client_secret: `secret-${n}`, + token_endpoint_auth_method: 'client_secret_basic', + redirect_uris: [callback], + client_secret_expires_at: Math.floor((now.getTime() + n * 2 * day) / 1000), + }), { status: 201 }); + }); + const first = await ensureDcrClient(entry, confidential, callback, fetch, now); + expect(first.clientId).toBe('conf-1'); + const later = new Date(now.getTime() + 36 * 60 * 60 * 1000); // inside the 24 h margin + const renewed = await ensureDcrClient(entry, confidential, callback, fetch, later); + expect(renewed.clientId).toBe('conf-2'); + expect(await Model.countDocuments({ current: true })).toBe(1); + // The retired client still serves rows it minted, with its own secret. + expect(await dcrClientForRow(entry, 'conf-1')).toEqual({ + clientId: 'conf-1', clientSecret: 'secret-1', tokenEndpointAuthMethod: 'client_secret_basic', + }); + }); + + test('an already-lapsing secret in the answer is refused', async () => { + const now = new Date('2026-10-21T12:00:00Z'); + await expect(ensureDcrClient( + entry, + confidential, + callback, + confidentialAnswer([], { client_secret_expires_at: Math.floor(now.getTime() / 1000) + 60 }), + now, + )) + .rejects.toMatchObject({ code: 'client_registration_refused' }); + }); + + test('a refused client\'s secret is deleted with it', async () => { + await ensureDcrClient(entry, confidential, callback, confidentialAnswer([])); + const { clientSecretRef } = await Model.findOne({}).lean(); + await refuseDcrClient(entry, 'conf-1'); + expect(await ConnectorSecret.findById(clientSecretRef)).toBeNull(); + expect(await dcrClientForRow(entry, 'conf-1')).toBeNull(); + }); + + test('a registration whose secret can no longer be read is replaced on the next start', async () => { + await ensureDcrClient(entry, confidential, callback, confidentialAnswer([], { client_id: 'conf-old' })); + await ConnectorSecret.deleteMany({}); + const client = await ensureDcrClient(entry, confidential, callback, confidentialAnswer([], { client_id: 'conf-new' })); + expect(client.clientId).toBe('conf-new'); + expect(await dcrClientForRow(entry, 'conf-old')).toBeNull(); + }); + + test('the token and revocation calls authenticate with the registered method, and PKCE still rides', async () => { + const client = await ensureDcrClient(entry, confidential, callback, confidentialAnswer([])); + const exchange = buildTokenExchangeRequest(entry, confidential, client, { + code: 'code-1', codeVerifier: 'verifier-1', redirectUri: callback, + }); + expect(exchange.headers.Authorization).toBe(`Basic ${Buffer.from('conf-1:issued-secret-1').toString('base64')}`); + expect(exchange.body.get('code_verifier')).toBe('verifier-1'); + expect(exchange.body.get('client_secret')).toBeNull(); + + // A post-registered client revokes with post where the vendor lists it... + const post = { ...client, tokenEndpointAuthMethod: 'client_secret_post' }; + const listed = { ...confidential, revocation_endpoint_auth_methods_supported: ['client_secret_basic', 'client_secret_post'] }; + const revoke = buildRevocationRequest(entry, listed, post, { token: 'tok', tokenTypeHint: 'refresh_token' }); + expect(revoke.headers.Authorization).toBeUndefined(); + expect(revoke.body.get('client_id')).toBe('conf-1'); + expect(revoke.body.get('client_secret')).toBe('issued-secret-1'); + // ...and with basic where the list is omitted, which RFC 8414 defaults to basic. + const defaulted = buildRevocationRequest(entry, confidential, post, { token: 'tok', tokenTypeHint: 'refresh_token' }); + expect(defaulted.headers.Authorization).toMatch(/^Basic /); + expect(defaulted.body.get('client_secret')).toBeNull(); + }); + + test('refresh still sends the secret when the caller copies only clientId and clientSecret', async () => { + // hostedMcpCredentialService and connectionRemovalService pass the client on as + // { clientId, clientSecret }; without the secret the vendor answers invalid_client + // and the registration is poisoned for every row it minted. + const { clientId, clientSecret } = await ensureDcrClient(entry, confidential, callback, confidentialAnswer([])); + const refresh = buildRefreshRequest(entry, confidential, { clientId, clientSecret }, { refreshToken: 'rt-1' }); + expect(refresh.headers.Authorization).toBe(`Basic ${Buffer.from('conf-1:issued-secret-1').toString('base64')}`); + + const postOnly = { ...metadata, token_endpoint_auth_methods_supported: ['client_secret_post'] }; + const viaPost = buildRefreshRequest(entry, postOnly, { clientId, clientSecret }, { refreshToken: 'rt-1' }); + expect(viaPost.body.get('client_secret')).toBe('issued-secret-1'); + }); + + test('a public DCR client still sends only its client_id', async () => { + const client = await ensureDcrClient(entry, metadata, callback, registrationFetch([])); + const exchange = buildTokenExchangeRequest(entry, metadata, client, { + code: 'code-1', codeVerifier: 'verifier-1', redirectUri: callback, + }); + expect(exchange.headers.Authorization).toBeUndefined(); + expect(exchange.body.get('client_id')).toBe('registered-1'); + expect(exchange.body.get('client_secret')).toBeNull(); + }); }); test('a CIMD entry never touches the registry or the AS', async () => { diff --git a/backend/__tests__/unit/services/installableInstallationService.test.js b/backend/__tests__/unit/services/installableInstallationService.test.js index 88c7c22646..76f9a29dff 100644 --- a/backend/__tests__/unit/services/installableInstallationService.test.js +++ b/backend/__tests__/unit/services/installableInstallationService.test.js @@ -22,7 +22,10 @@ const { const { sweep } = require('../../../services/installable/installableReconciler'); const { TELEGRAM_CONNECTOR, SLACK_CONNECTOR } = require('../../../scripts/seed-builtin-connectors'); const { put } = require('../../../services/connectorSecrets'); -const { DISCORD_WEBHOOK_URL, SLACK_BOT_TOKEN } = require('../../../services/connectorSecretKinds'); +const { + DISCORD_WEBHOOK_URL, HOSTED_MCP_DCR_CLIENT_SECRET, SLACK_BOT_TOKEN, +} = require('../../../services/connectorSecretKinds'); +const HostedMcpDcrRegistration = require('../../../models/HostedMcpDcrRegistration'); const telegramService = require('../../../services/telegramService'); const { relayTelegramMessageToPod } = require('../../../services/telegramBridgeService'); const { @@ -944,6 +947,57 @@ describe('installable connector projection', () => { expect(integration.errorMessageUserFacing).toBe(true); }); + // TASK-248: a confidential hosted-MCP client's secret is owned by its + // registration, not by an Integration row. Read only through the Integration + // paths, the sweep would delete it ten minutes after registration, and every + // connection that client minted would fail its next refresh. + const seedDcrRegistrationWithSecret = async (overrides = {}) => { + const registrationId = new mongoose.Types.ObjectId(); + const ref = await put(String(registrationId), HOSTED_MCP_DCR_CLIENT_SECRET, 'client-secret'); + await HostedMcpDcrRegistration.create({ + _id: registrationId, + entryId: 'gitlab', + issuer: 'https://gitlab.com', + clientId: `client-${registrationId}`, + redirectUri: 'https://api.example.test/callback', + tokenEndpointAuthMethod: 'client_secret_basic', + clientSecretRef: ref, + current: true, + ...overrides, + }); + return { registrationId, ref }; + }; + + it('keeps a hosted-MCP client secret past the grace window while its registration names it, retired or not', async () => { + const current = await seedDcrRegistrationWithSecret(); + const retired = await seedDcrRegistrationWithSecret({ current: false, retiredAt: new Date() }); + await backdateSecret(current.ref); + const now = await backdateSecret(retired.ref); + + const swept = await sweep(now); + + expect(swept.orphanedSecrets).toBe(0); + expect(await ConnectorSecret.findById(current.ref)).not.toBeNull(); + expect(await ConnectorSecret.findById(retired.ref)).not.toBeNull(); + }); + + it('revokes a hosted-MCP client secret once its registration is refused, gone, or names another', async () => { + const refused = await seedDcrRegistrationWithSecret({ current: false, refusedAt: new Date() }); + const orphan = await put(new mongoose.Types.ObjectId().toString(), HOSTED_MCP_DCR_CLIENT_SECRET, 'lost'); + const repointed = await seedDcrRegistrationWithSecret({ current: false }); + await HostedMcpDcrRegistration.updateOne({ _id: repointed.registrationId }, { $set: { clientSecretRef: 'other' } }); + await backdateSecret(refused.ref); + await backdateSecret(orphan); + const now = await backdateSecret(repointed.ref); + + const swept = await sweep(now); + + expect(swept.orphanedSecrets).toBe(3); + expect(await ConnectorSecret.findById(refused.ref)).toBeNull(); + expect(await ConnectorSecret.findById(orphan)).toBeNull(); + expect(await ConnectorSecret.findById(repointed.ref)).toBeNull(); + }); + it('keeps a secret whose kind it cannot place instead of deleting the ref it never looked for', async () => { // Fail-closed on an unrecognized kind: this build does not know where that // ref lives, and "the sweep found no reference" is not evidence that none diff --git a/backend/models/HostedMcpDcrRegistration.ts b/backend/models/HostedMcpDcrRegistration.ts index 6c494f5408..ef32105a0f 100644 --- a/backend/models/HostedMcpDcrRegistration.ts +++ b/backend/models/HostedMcpDcrRegistration.ts @@ -1,11 +1,22 @@ import mongoose, { Document, Model, Schema } from 'mongoose'; -/** One vendor-issued public OAuth client for an instance and catalogue entry. */ +/** + * One vendor-issued OAuth client for an instance and catalogue entry: public + * (`token_endpoint_auth_method: 'none'`) when the vendor offers that, otherwise + * confidential, with its secret kept encrypted in the connector-secret store + * and owned by this row (TASK-248). + */ export interface IHostedMcpDcrRegistration extends Document { entryId: string; issuer: string; clientId: string; redirectUri: string; + /** How this client authenticates at the token and revocation endpoints. */ + tokenEndpointAuthMethod?: 'none' | 'client_secret_basic' | 'client_secret_post'; + /** connectorSecrets ref of the client secret; set only for a confidential client. */ + clientSecretRef?: string | null; + /** When the vendor said the secret lapses; null means it never does. */ + clientSecretExpiresAt?: Date | null; current: boolean; retiredAt?: Date | null; refusedAt?: Date | null; @@ -18,6 +29,11 @@ const HostedMcpDcrRegistrationSchema = new Schema({ issuer: { type: String, required: true }, clientId: { type: String, required: true }, redirectUri: { type: String, required: true }, + tokenEndpointAuthMethod: { + type: String, enum: ['none', 'client_secret_basic', 'client_secret_post'], default: 'none', + }, + clientSecretRef: { type: String, default: null }, + clientSecretExpiresAt: { type: Date, default: null }, current: { type: Boolean, required: true }, retiredAt: { type: Date, default: null }, refusedAt: { type: Date, default: null }, diff --git a/backend/services/connectorSecretKinds.ts b/backend/services/connectorSecretKinds.ts index a2bc57e3e5..d0c2e27a1a 100644 --- a/backend/services/connectorSecretKinds.ts +++ b/backend/services/connectorSecretKinds.ts @@ -40,6 +40,13 @@ export interface ConnectorSecretKindSpec { refPaths: readonly string[]; /** The sentence written for the person reading the Connectors page. */ unavailableReason: string; + /** + * The row that holds this kind's ref. Absent means an `Integration` row, as + * for every kind before TASK-248; `refPaths` are then paths on that row. A + * hosted-MCP client registration is not an Integration, so its secret is + * owned by the registration and the sweeps look it up there. + */ + owner?: 'integration' | 'hosted-mcp-dcr-registration'; } /** The Slack bot token: the ref lives on the row the OAuth bind writes. */ @@ -85,19 +92,43 @@ export const HOSTED_MCP_REFRESH_TOKEN: ConnectorSecretKindSpec = { unavailableReason: 'Hosted MCP connection refresh token is unavailable', }; +/** + * A confidential hosted-MCP client's secret (TASK-248). When a vendor offers + * no public-client registration (gitlab.com and api.supabase.com list only + * client_secret_basic and client_secret_post), the instance registers a + * confidential client and keeps the issued secret here. It belongs to the + * registration, not to any one person's connection: every row minted by that + * client authenticates its refresh and revocation with it. + */ +export const HOSTED_MCP_DCR_CLIENT_SECRET: ConnectorSecretKindSpec = { + kind: 'hosted-mcp-dcr-client-secret', + provider: 'hosted-mcp', + refPaths: ['clientSecretRef'], + unavailableReason: 'Hosted MCP client registration secret is unavailable', + owner: 'hosted-mcp-dcr-registration', +}; + export const CONNECTOR_SECRET_KINDS: readonly ConnectorSecretKindSpec[] = [ SLACK_BOT_TOKEN, DISCORD_WEBHOOK_URL, HOSTED_MCP_ACCESS_TOKEN, HOSTED_MCP_REFRESH_TOKEN, + HOSTED_MCP_DCR_CLIENT_SECRET, ]; +/** Whether a kind's ref lives on an Integration row (the default owner). */ +export const ownedByIntegration = (spec: ConnectorSecretKindSpec): boolean => ( + (spec.owner ?? 'integration') === 'integration' +); + export const kindSpec = (kind: string): ConnectorSecretKindSpec | undefined => ( CONNECTOR_SECRET_KINDS.find((spec) => spec.kind === kind) ); -/** Every ref path the sweeps must inspect, across all kinds. */ -export const allRefPaths = (): string[] => CONNECTOR_SECRET_KINDS.flatMap((spec) => [...spec.refPaths]); +/** Every ref path on an Integration row the sweeps must inspect, across kinds. */ +export const allRefPaths = (): string[] => CONNECTOR_SECRET_KINDS + .filter(ownedByIntegration) + .flatMap((spec) => [...spec.refPaths]); const valueAtPath = (row: unknown, path: string): unknown => path .split('.') diff --git a/backend/services/hostedMcpDcrRegistry.ts b/backend/services/hostedMcpDcrRegistry.ts index 0171cc6e39..9ee39d64b8 100644 --- a/backend/services/hostedMcpDcrRegistry.ts +++ b/backend/services/hostedMcpDcrRegistry.ts @@ -1,13 +1,41 @@ -/** Instance-scoped Dynamic Client Registration for hosted MCP public clients. */ -import HostedMcpDcrRegistration from '../models/HostedMcpDcrRegistration'; +/** + * Instance-scoped Dynamic Client Registration for hosted MCP clients. + * + * A vendor that offers public-client registration (`none` in its token + * endpoint auth methods) gets a public client, as before. One that offers only + * client_secret_basic or client_secret_post gets a confidential client + * (TASK-248, Wren's scope 2026-10-19): gitlab.com and api.supabase.com list + * only those two, so neither could connect. The issued secret is kept + * encrypted in the connector-secret store, owned by the registration row (the + * orphan sweep knows that owner), and authenticates the token, refresh and + * revocation calls; PKCE S256 is still sent. No registration_access_token is + * stored. A secret the vendor says will lapse is replaced by registering again + * before it does. + */ +import { Types } from 'mongoose'; +import HostedMcpDcrRegistration, { IHostedMcpDcrRegistration } from '../models/HostedMcpDcrRegistration'; import type { HostedMcpEntry } from './hostedMcpEntryService'; import { + CONFIDENTIAL_CLIENT_METHODS, HostedMcpClientError, type HostedMcpAuthorizationServer, type HostedMcpClientConfiguration, } from './hostedMcpIntakeService'; import { upstreamFetch } from './upstreamFetch'; import { hostedVendorFetch } from './hostedVendorFetch'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +const connectorSecrets = require('./connectorSecrets'); +// eslint-disable-next-line @typescript-eslint/no-require-imports +const { HOSTED_MCP_DCR_CLIENT_SECRET } = require('./connectorSecretKinds'); + +type ConfidentialMethod = typeof CONFIDENTIAL_CLIENT_METHODS[number]; +type RegistrationMethod = 'none' | ConfidentialMethod; + +/** + * A secret that lapses within this margin is treated as lapsed, so a start + * never hands the vendor a client about to stop working mid-flow. + */ +export const CLIENT_SECRET_RENEW_MARGIN_MS = 24 * 60 * 60 * 1000; const currentFilter = (entry: HostedMcpEntry) => ({ entryId: entry.id, @@ -15,7 +43,47 @@ const currentFilter = (entry: HostedMcpEntry) => ({ current: true, }); -const asClient = (clientId: string): HostedMcpClientConfiguration => ({ clientId }); +type RegistrationRow = Pick; + +const secretLapsing = (row: RegistrationRow, now: Date): boolean => Boolean( + row.clientSecretExpiresAt + && new Date(row.clientSecretExpiresAt).getTime() - now.getTime() <= CLIENT_SECRET_RENEW_MARGIN_MS, +); + +/** + * The client a stored registration describes, with its secret decrypted for a + * confidential one. Null when a confidential client's secret can't be read + * (deleted, or its key left the ring): that registration can't authenticate. + */ +const clientFromRow = async (row: RegistrationRow): Promise => { + const method = (row.tokenEndpointAuthMethod || 'none') as RegistrationMethod; + if (method === 'none') return { clientId: row.clientId }; + if (!row.clientSecretRef) return null; + try { + const clientSecret = await connectorSecrets.get(row.clientSecretRef); + return clientSecret ? { clientId: row.clientId, clientSecret, tokenEndpointAuthMethod: method } : null; + } catch { + return null; + } +}; + +/** Which method to register with, or a refusal before any POST. */ +const registrationMethod = (entry: HostedMcpEntry, metadata: HostedMcpAuthorizationServer): RegistrationMethod => { + const supported = metadata.token_endpoint_auth_methods_supported; + // RFC 8414 defaults an omitted list to client_secret_basic, but this registry + // registers only a method the vendor names: never POST hoping it accepts one. + if (Array.isArray(supported)) { + if (supported.includes('none')) return 'none'; + // Basic, then post: the order the token calls choose by (hostedMcpIntakeService). + const confidential = CONFIDENTIAL_CLIENT_METHODS.find((method) => supported.includes(method)); + if (confidential) return confidential; + } + throw new HostedMcpClientError( + 'client_registration_unsupported', + `authorization server advertises no client registration this instance supports for ${entry.id}`, + ); +}; /** * A registration has one callback. Changing the instance host retires the old @@ -26,6 +94,7 @@ export const ensureDcrClient = async ( metadata: HostedMcpAuthorizationServer, redirectUri: string, fetchImpl: typeof fetch = hostedVendorFetch, + now: Date = new Date(), ): Promise => { if (entry.client !== 'dcr') { throw new HostedMcpClientError('client_registration_unsupported', 'entry does not use DCR'); @@ -34,24 +103,24 @@ export const ensureDcrClient = async ( // The upsert is safe only once the unique current-registration index exists. await HostedMcpDcrRegistration.init(); const existing = await HostedMcpDcrRegistration.findOne(currentFilter(entry)); - if (existing?.redirectUri === redirectUri && !existing.refusedAt) return asClient(existing.clientId); + if (existing?.redirectUri === redirectUri && !existing.refusedAt && !secretLapsing(existing, now)) { + const reused = await clientFromRow(existing); + if (reused) return reused; + } if (existing) { await HostedMcpDcrRegistration.updateOne( { _id: existing._id, current: true }, - { $set: { current: false, retiredAt: new Date() } }, + { $set: { current: false, retiredAt: now } }, ); } - // RFC 8414 defaults an omitted auth-method list to client_secret_basic. - // These entries use public clients only; never POST hoping the AS accepts one. - if (!Array.isArray(metadata.token_endpoint_auth_methods_supported) - || !metadata.token_endpoint_auth_methods_supported.includes('none') - || !metadata.registration_endpoint) { + if (!metadata.registration_endpoint) { throw new HostedMcpClientError( 'client_registration_unsupported', - `authorization server does not advertise public-client registration for ${entry.id}`, + `authorization server advertises no registration endpoint for ${entry.id}`, ); } + const method = registrationMethod(entry, metadata); let registrationUrl: URL; try { registrationUrl = new URL(metadata.registration_endpoint); @@ -70,7 +139,7 @@ export const ensureDcrClient = async ( redirect_uris: [redirectUri], grant_types: ['authorization_code', 'refresh_token'], response_types: ['code'], - token_endpoint_auth_method: 'none', + token_endpoint_auth_method: method, scope: entry.scopes.join(' '), }), }, fetchImpl); @@ -80,43 +149,100 @@ export const ensureDcrClient = async ( const issued = await response.json().catch(() => null) as { client_id?: unknown; client_secret?: unknown; + client_secret_expires_at?: unknown; token_endpoint_auth_method?: unknown; redirect_uris?: unknown; } | null; - if (!issued || typeof issued.client_id !== 'string' || !issued.client_id - || issued.client_secret !== undefined - || issued.token_endpoint_auth_method !== 'none' - || !Array.isArray(issued.redirect_uris) - || issued.redirect_uris.length !== 1 - || issued.redirect_uris[0] !== redirectUri) { + // RFC 7591 §2: an omitted token_endpoint_auth_method means client_secret_basic. + const issuedMethod = issued?.token_endpoint_auth_method === undefined + ? 'client_secret_basic' + : issued.token_endpoint_auth_method; + const confirmsCallback = Array.isArray(issued?.redirect_uris) + && issued.redirect_uris.length === 1 + && issued.redirect_uris[0] === redirectUri; + if (!issued || typeof issued.client_id !== 'string' || !issued.client_id || !confirmsCallback) { + throw new HostedMcpClientError( + 'client_registration_unsupported', + 'registration answer did not confirm the client and its sole callback', + ); + } + if (method === 'none') { + if (issued.client_secret !== undefined || issued.token_endpoint_auth_method !== 'none') { + throw new HostedMcpClientError( + 'client_registration_unsupported', + 'registration answer did not confirm the public client and sole callback', + ); + } + } else if (typeof issued.client_secret !== 'string' || !issued.client_secret || issuedMethod !== method) { throw new HostedMcpClientError( 'client_registration_unsupported', - 'registration answer did not confirm the public client and sole callback', + 'registration answer did not confirm the confidential client, its secret and its auth method', ); } + // RFC 7591 §3.2.1: 0 means the secret never lapses; any other value is seconds since the epoch. + let clientSecretExpiresAt: Date | null = null; + if (method !== 'none' && issued.client_secret_expires_at !== undefined && issued.client_secret_expires_at !== 0) { + const seconds = Number(issued.client_secret_expires_at); + if (!Number.isFinite(seconds) || seconds <= 0) { + throw new HostedMcpClientError( + 'client_registration_unsupported', + 'registration answer carried an unreadable secret expiry', + ); + } + clientSecretExpiresAt = new Date(seconds * 1000); + if (secretLapsing({ clientId: issued.client_id, clientSecretExpiresAt }, now)) { + throw new HostedMcpClientError( + 'client_registration_refused', + 'registration answer carried a secret that lapses too soon', + ); + } + } + + // The secret is stored before the row so the row is never written without + // it; it is owned by the row's id, chosen here. A start that loses the + // insert race below deletes its own secret and uses the winner's client. + const registrationId = new Types.ObjectId(); + const clientSecretRef = method === 'none' + ? null + : await connectorSecrets.put(String(registrationId), HOSTED_MCP_DCR_CLIENT_SECRET, issued.client_secret as string); + const discardOwnSecret = async () => { + if (clientSecretRef) await connectorSecrets.revoke(clientSecretRef).catch(() => {}); + }; // Two first starts may both POST. The unique current index admits only one - // registration; both starts then authorize with the stored winner's id. + // registration; both starts then authorize with the stored winner's client. try { const stored = await HostedMcpDcrRegistration.findOneAndUpdate( currentFilter(entry), { $setOnInsert: { + _id: registrationId, entryId: entry.id, issuer: entry.issuer, clientId: issued.client_id, redirectUri, + tokenEndpointAuthMethod: method, + clientSecretRef, + clientSecretExpiresAt, current: true, } }, { upsert: true, new: true, setDefaultsOnInsert: true }, ); if (!stored || stored.redirectUri !== redirectUri || stored.refusedAt) { + await discardOwnSecret(); throw new HostedMcpClientError('client_registration_refused', 'current registration changed during start'); } - return asClient(stored.clientId); + if (String(stored._id) !== String(registrationId)) await discardOwnSecret(); + const client = await clientFromRow(stored); + if (!client) { + throw new HostedMcpClientError('client_registration_refused', 'current registration has no usable secret'); + } + return client; } catch (error) { if ((error as { code?: number })?.code !== 11000) throw error; + await discardOwnSecret(); const winner = await HostedMcpDcrRegistration.findOne(currentFilter(entry)); - if (winner?.redirectUri === redirectUri && !winner.refusedAt) return asClient(winner.clientId); + const client = winner?.redirectUri === redirectUri && !winner.refusedAt ? await clientFromRow(winner) : null; + if (client) return client; throw new HostedMcpClientError('client_registration_refused', 'concurrent registration changed'); } }; @@ -133,15 +259,17 @@ export const dcrClientForRow = async ( clientId: recordedClientId, refusedAt: null, }); - return registration ? asClient(registration.clientId) : null; + return registration ? clientFromRow(registration) : null; }; /** Invalid client on token/revoke poisons this registration, not every row. */ export const refuseDcrClient = async (entry: HostedMcpEntry, clientId: string): Promise => { if (entry.client !== 'dcr') return; const now = new Date(); - await HostedMcpDcrRegistration.updateOne( + const refused = await HostedMcpDcrRegistration.findOneAndUpdate( { entryId: entry.id, issuer: entry.issuer, clientId, refusedAt: null }, { $set: { refusedAt: now, retiredAt: now, current: false } }, ); + // A refused client's secret is useless; delete it now rather than at the sweep. + if (refused?.clientSecretRef) await connectorSecrets.revoke(refused.clientSecretRef).catch(() => {}); }; diff --git a/backend/services/hostedMcpIntakeService.ts b/backend/services/hostedMcpIntakeService.ts index e5fc29c079..87f3f60d6b 100644 --- a/backend/services/hostedMcpIntakeService.ts +++ b/backend/services/hostedMcpIntakeService.ts @@ -137,8 +137,17 @@ export class HostedMcpClientError extends Error { export interface HostedMcpClientConfiguration { clientId: string; - /** Present only for an instance-configured pre-registered confidential client. */ + /** + * Present for a confidential client: an instance-configured pre-registered + * one, or one this instance registered by DCR (TASK-248). + */ clientSecret?: string; + /** + * The method a DCR-registered confidential client registered with; it + * authenticates the token, refresh and revocation calls that way. Absent for + * a pre-registered client, which follows the vendor's advertised methods. + */ + tokenEndpointAuthMethod?: 'client_secret_basic' | 'client_secret_post'; } export type HostedMcpEndpointKind = 'token' | 'revocation'; @@ -389,13 +398,41 @@ const formEncode = (value: string): string => ( new URLSearchParams([['value', value]]).toString().slice('value='.length) ); +/** Client-secret methods this instance can use, in preference order. */ +export const CONFIDENTIAL_CLIENT_METHODS = ['client_secret_basic', 'client_secret_post'] as const; + +/** RFC 6749 §2.3.1: the client's secret in the Basic header, or in the form body for post. */ +const confidentialAuthentication = ( + client: HostedMcpClientConfiguration, + method: 'client_secret_basic' | 'client_secret_post', +): ClientEndpointAuthentication => { + const secret = String(client.clientSecret || ''); + if (method === 'client_secret_post') { + return { + headers: {}, + body: { client_id: client.clientId, client_secret: secret }, + }; + } + const basic = `${formEncode(client.clientId)}:${formEncode(secret)}`; + return { + headers: { Authorization: `Basic ${Buffer.from(basic).toString('base64')}` }, + body: {}, + }; +}; + const endpointAuthentication = ( entry: HostedMcpEntry, metadata: Partial, client: HostedMcpClientConfiguration, endpoint: HostedMcpEndpointKind, ): ClientEndpointAuthentication => { - if (entry.client !== 'pre-registered') { + // A DCR client is confidential when this instance registered it with a + // secret (TASK-248). It is recognised by the secret itself, not by the + // method field, so a caller that copies only clientId and clientSecret (the + // refresh and removal paths) still sends the secret. + const confidential = entry.client === 'pre-registered' + || (entry.client === 'dcr' && Boolean(client.clientSecret)); + if (!confidential) { return { headers: {}, body: { client_id: client.clientId } }; } if (!client.clientSecret) { @@ -414,25 +451,21 @@ const endpointAuthentication = ( const methods = supported === undefined ? ['client_secret_basic'] : (Array.isArray(supported) ? supported : []); - const method = ['client_secret_basic', 'client_secret_post'] - .find((candidate) => methods.includes(candidate)); + // The method a DCR client registered with is tried first. Without it the + // order is basic, then post: the same order registration chose by, so the + // answer matches while the vendor's list is unchanged. + const registered = client.tokenEndpointAuthMethod; + const preference = registered + ? [registered, ...CONFIDENTIAL_CLIENT_METHODS.filter((candidate) => candidate !== registered)] + : [...CONFIDENTIAL_CLIENT_METHODS]; + const method = preference.find((candidate) => methods.includes(candidate)); if (!method) { throw new HostedMcpClientError( 'client_auth_unsupported', `authorization server advertises no supported ${endpoint} client-secret method for ${entry.id}`, ); } - if (method === 'client_secret_post') { - return { - headers: {}, - body: { client_id: client.clientId, client_secret: client.clientSecret }, - }; - } - const basic = `${formEncode(client.clientId)}:${formEncode(client.clientSecret)}`; - return { - headers: { Authorization: `Basic ${Buffer.from(basic).toString('base64')}` }, - body: {}, - }; + return confidentialAuthentication(client, method as 'client_secret_basic' | 'client_secret_post'); }; /** Authorization-code exchange: PKCE and resource survive every client kind. */ diff --git a/backend/services/installable/installableReconciler.ts b/backend/services/installable/installableReconciler.ts index 61be8908cb..06fc9d603b 100644 --- a/backend/services/installable/installableReconciler.ts +++ b/backend/services/installable/installableReconciler.ts @@ -17,7 +17,9 @@ const connectorSecrets = require('../connectorSecrets'); // Discord webhook secret ten minutes after it was written (vera 74130), and the // unavailable sweep named Slack for any provider (vera 74131). // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const { allRefPaths, kindSpec, rowReferencesSecret } = require('../connectorSecretKinds'); +const { + allRefPaths, kindSpec, ownedByIntegration, rowReferencesSecret, +} = require('../connectorSecretKinds'); // The user-facing half of the error state: this module marks a row it cannot // decrypt, and the reason is written for the person reading the Connectors page // — the same flag the delivery-failure flip sets, so it comes from that service @@ -295,9 +297,23 @@ const sweepOrphanedConnectorSecrets = async (now: Date): Promise => { ); continue; } + const ref = String(secret._id); + if (!ownedByIntegration(spec)) { + // A hosted-MCP client registration's secret (TASK-248). It is kept while + // its registration exists, has not been refused, and still names it: + // a RETIRED registration keeps serving the rows it minted, so retirement + // alone is not a reason to delete the secret those rows refresh with. + // eslint-disable-next-line global-require, @typescript-eslint/no-require-imports + const HostedMcpDcrRegistration = require('../../models/HostedMcpDcrRegistration'); + const registration = await HostedMcpDcrRegistration.findById(secret.integrationId) + .select('refusedAt clientSecretRef').lean() as Record | null; + if (registration && !registration.refusedAt && rowReferencesSecret(registration, spec, ref)) continue; + await connectorSecrets.revoke(ref); + revoked += 1; + continue; + } const integration = await Integration.findById(secret.integrationId) .select(select).lean() as Record | null; - const ref = String(secret._id); if (integration?.isActive === true && rowReferencesSecret(integration, spec, ref)) continue; await connectorSecrets.revoke(ref); revoked += 1; diff --git a/docs/plans/hosted-mcp-connection-scope.md b/docs/plans/hosted-mcp-connection-scope.md index 417fe30c31..b3707bd3de 100644 --- a/docs/plans/hosted-mcp-connection-scope.md +++ b/docs/plans/hosted-mcp-connection-scope.md @@ -200,9 +200,17 @@ The callback reads the subject after TASK-193's scope check and before it stores **A pre-registered client is instance config, never entry data** (76042). Its id and secret are held by the instance, through ESO on the hosted one, and neither appears in an entry or a repo file. The secret appears on no row either. A row keeps only a snapshot of the id that minted its pair (§2), and instance config stays the id's source: every exchange, refresh and revocation reads the id from there, never from a row. The id already arrives that way, as the entry's own `_CLIENT_ID` (`resolvedClientId`, `services/hostedMcpIntakeService.ts:145`), and the secret arrives the same way. An instance that lacks either does not offer the entry. The entry's readiness answers `not_configured`, as GitHub's does without its App's credentials (`services/installable/toolInstallables.ts:74–76`), so the catalogue projection lists it as unavailable and the page offers no Connect (Vera 76113). A `cimd` entry still has nothing to set, so its readiness stays constant. The instance also treats the entry as one the catalogue no longer has (§3): its rows are refused, and their removal finishes with the row's copied page (§9). The start route's `client_not_configured` stays as the backstop. It is a confidential client: it authenticates with its secret at the token and revocation endpoints, by a method the AS's metadata lists, and it still sends PKCE `S256`. The entry's callback is registered with the vendor as a redirect URI. Google is the first such vendor. Its setup page for its MCP servers says to create a "Web application" client and copy its "Client ID and Client Secret" (developers.google.com/workspace/guides/configure-mcp-servers, updated 2026-09-18). `accounts.google.com`'s metadata lists no registration endpoint and no CIMD support, and names `client_secret_post` and `client_secret_basic` as its token endpoint's auth methods (fetched 2026-10-01). -**A `dcr` entry registers one public client per instance, per entry and issuer, at its first start** (connector-ops 77008 (f), Wren 77045, the TASK-197 row). Six vendors offer DCR and no CIMD: Close, Otter, Buildkite, Miro, Calendly and ClickUp (fetched 2026-10-14). The start registers at the AS's `registration_endpoint` with `token_endpoint_auth_method: none`, the `authorization_code` and `refresh_token` grants, the `code` response type, the entry's scopes, and the entry's callback on this instance as the only redirect URI. It sends PKCE `S256`, as every flow does. Before it sends anything, it refuses `client_registration_unsupported` when the metadata names no https `registration_endpoint`, or when its `token_endpoint_auth_methods_supported` lacks `none`. An omitted list lacks it, since RFC 8414 §2 defaults that list to `client_secret_basic`. It refuses the answer with the same code when the answer registers another method, carries a `client_secret`, or does not echo the one redirect URI. An omitted method is another method, since RFC 7591 §2 defaults it to `client_secret_basic`. An AS that refuses the request ends the start `client_registration_refused`. No `registration_access_token` is stored, and nothing is sent to RFC 7592's management endpoint. +**A `dcr` entry registers one public client per instance, per entry and issuer, at its first start** (connector-ops 77008 (f), Wren 77045, the TASK-197 row). Six vendors offer DCR and no CIMD: Close, Otter, Buildkite, Miro, Calendly and ClickUp (fetched 2026-10-14). The start registers at the AS's `registration_endpoint` with `token_endpoint_auth_method: none`, the `authorization_code` and `refresh_token` grants, the `code` response type, the entry's scopes, and the entry's callback on this instance as the only redirect URI. It sends PKCE `S256`, as every flow does. Before it sends anything, it refuses `client_registration_unsupported` when the metadata names no https `registration_endpoint`, or when its `token_endpoint_auth_methods_supported` names neither `none` nor a client-secret method (amended 2026-10-21 by TASK-248; see the next paragraph). An omitted list names none of them: RFC 8414 §2 defaults it to `client_secret_basic`, but the registry registers only a method the vendor actually lists. It refuses the answer with the same code when the answer registers another method, carries a `client_secret`, or does not echo the one redirect URI. An omitted method is another method, since RFC 7591 §2 defaults it to `client_secret_basic`. An AS that refuses the request ends the start `client_registration_refused`. No `registration_access_token` is stored, and nothing is sent to RFC 7592's management endpoint. -Miro's AS lists only `client_secret_post` and `client_secret_basic` (fetched 2026-10-14), so Miro's PR brings a confidential registration and teaches the orphan sweep who owns its secret. The sweep deletes a secret that no active row names once a 10-minute grace has passed (`services/installable/installableReconciler.ts:272`, `:30`), and a registration is not a row. +Miro's AS lists only `client_secret_post` and `client_secret_basic` (fetched 2026-10-14), and so do gitlab.com and api.supabase.com (fetched 2026-10-19). Miro left Wave 1, so the confidential registration came with TASK-248 (amended 2026-10-21), before GitLab (#2331) and Supabase (#2332): + +- **When `none` is not listed,** the start registers with `client_secret_basic`, or `client_secret_post` when that is the only one listed. Basic comes first, the same order the token calls choose by. The answer must carry a non-empty `client_secret` and register that same method; an omitted method counts as `client_secret_basic`, as above. +- **The secret is stored before the registration.** It goes into the connector-secret store (`hosted-mcp-dcr-client-secret`, AES-GCM like the tokens), owned by the registration's id. The registration holds only `clientSecretRef`, the method and `clientSecretExpiresAt`. A start that loses the insert race deletes its own secret. +- **The secret authenticates every token call.** The exchange, refresh and revocation authenticate with it, and PKCE `S256` is still sent. `endpointAuthentication` recognises a confidential DCR client by its secret, so the refresh and removal paths, which pass only `clientId` and `clientSecret`, still send it. +- **A secret is replaced before it lapses.** A `client_secret_expires_at` within 24 hours retires the registration at the next start, which registers anew. An answer whose secret already lapses within that margin is refused `client_registration_refused`. Zero means it never lapses (RFC 7591 §3.2.1). +- **The sweep knows the owner.** A secret is kept while its registration exists, has not been refused, and still names it, current or retired. A refused registration's secret is deleted at once. A registration whose secret can no longer be read is replaced at the next start, and its rows go to reconnect. + +The sweep needed this because it deletes a secret that no active row names once a 10-minute grace has passed (`services/installable/installableReconciler.ts:272`, `:30`), and a registration is not a row. **A registration is a record of its own, never a row's.** It holds the entry, the issuer, `clientId`, `redirectUri`, `createdAt`, `retiredAt`, `refusedAt`, and a required `current` flag that every insert writes. A unique index allows one current registration per entry and issuer. It is partial on `current: true`, an equality on a field every insert writes (Vera 77059, #1976). A second unique index keeps a `clientId` to one record per entry and issuer. Two starts at once may both register: the store keeps one, both flows authorize with it, and the other registration stays unused at the AS. A host move changes the callback, so the next start retires the current registration and registers anew, and the rows minted before the move keep their own client (§2). @@ -488,7 +496,13 @@ Each step can be tested without the vendor, except the live measurements in step - `a pre-registered client authenticates at the token and revocation endpoints, and still sends PKCE` - Before the first DCR entry: the registration store (TASK-197, §4). - `a dcr start registers a public client once, and two concurrent starts store one registration and authorize with it`, with the indexes built before the race, so it goes red when the partial index is dropped - - `an AS that lists no none, or an answer that registers another method, is refused client_registration_unsupported before any token is requested` + - `an AS that names no supported method, or an answer that registers another method, is refused client_registration_unsupported before any token is requested` (amended by TASK-248: an AS listing only client-secret methods now registers a confidential client) + - With TASK-248: confidential registration (§4). + - `registers with the vendor's listed method, keeps the secret encrypted and owned by the row, and reuses it` + - `two concurrent first starts keep only the winner's secret` + - `a secret that lapses is replaced by registering again before it does` + - `refresh still sends the secret when the caller copies only clientId and clientSecret` + - `keeps a hosted-MCP client secret past the grace window while its registration names it` and `revokes it once its registration is refused, gone, or names another` - `the callback exchanges with the client its start recorded` - `a row on a retired registration still refreshes with its own client`, which goes red if the lookup compares against the current registration - `a host move registers anew while older rows refresh` diff --git a/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx b/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx index 8c7bc5c71a..3a667367f3 100644 --- a/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx +++ b/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx @@ -701,6 +701,8 @@ describe('V2ConnectorsPage', () => { ['issuer_metadata_incomplete', 'hostedMcpIssuerUnavailable'], ['client_not_configured', 'hostedMcpClientSetup'], ['client_registration_required', 'hostedMcpClientSetup'], + ['client_registration_unsupported', 'hostedMcpClientSetup'], + ['client_registration_refused', 'hostedMcpClientSetup'], ['client_auth_unsupported', 'hostedMcpClientSetup'], ['client_auth_refused', 'hostedMcpClientSetup'], ['authorization_parameter_conflict', 'hostedMcpClientSetup'], diff --git a/frontend/src/v2/components/hostedMcpErrorNotice.ts b/frontend/src/v2/components/hostedMcpErrorNotice.ts index 828491f280..038d1f143f 100644 --- a/frontend/src/v2/components/hostedMcpErrorNotice.ts +++ b/frontend/src/v2/components/hostedMcpErrorNotice.ts @@ -16,6 +16,10 @@ const noticeKeyByCode: Record = { issuer_metadata_incomplete: 'hostedMcpIssuerUnavailable', client_not_configured: 'hostedMcpClientSetup', client_registration_required: 'hostedMcpClientSetup', + // The vendor offers no registration this instance supports, or refused the + // one it asked for (TASK-248 named both before the first DCR entry ships). + client_registration_unsupported: 'hostedMcpClientSetup', + client_registration_refused: 'hostedMcpClientSetup', client_auth_unsupported: 'hostedMcpClientSetup', client_auth_refused: 'hostedMcpClientSetup', authorization_parameter_conflict: 'hostedMcpClientSetup', From fc61adbbecb104b9a771ce945579d072b691f266 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 21 Oct 2026 03:20:07 -0700 Subject: [PATCH 2/3] fix(hosted-mcp): a DCR client authenticates exactly as it registered, on every path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vera's HOLD on #2377, both items. 1. The registration's method reached the exchange and nothing else. The refresh and removal paths passed only clientId and clientSecret, and endpointAuthentication filtered a method against the advertised list; a dcr removal passes no metadata, so RFC 8414's omitted-list default (basic) turned a post-registered client into Basic, and its invalid_client would refuse the registration every row shares. Now the method is carried through refresh (credentialFor → refreshAtVendor) and removal (removeConnection → revokeAtVendor → revokeTokenAtVendor), and a recorded method is sent as recorded with no list consulted. The vendor refresh is lifted out as refreshTokenAtVendor so the credential it sends is testable, like the removal side's revokeTokenAtVendor. 2. The unreadable-secret paths now each have an arm: a secret row deleted, a registration with no secret ref, and a secret whose key left the ring all re-register rather than return a client. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4 --- .../service/hostedMcpDcrRegistry.test.js | 47 ++++++-- .../unit/services/connectionRemoval.test.js | 35 ++++++ .../unit/services/hostedMcpCredential.test.js | 44 +++++++ backend/services/connectionRemovalService.ts | 13 +- .../services/hostedMcpCredentialService.ts | 112 +++++++++++------- backend/services/hostedMcpIntakeService.ts | 24 ++-- docs/plans/hosted-mcp-connection-scope.md | 4 +- 7 files changed, 209 insertions(+), 70 deletions(-) diff --git a/backend/__tests__/service/hostedMcpDcrRegistry.test.js b/backend/__tests__/service/hostedMcpDcrRegistry.test.js index d4525426f7..c0a730282e 100644 --- a/backend/__tests__/service/hostedMcpDcrRegistry.test.js +++ b/backend/__tests__/service/hostedMcpDcrRegistry.test.js @@ -296,6 +296,30 @@ describe('instance-scoped hosted MCP DCR registry', () => { expect(await dcrClientForRow(entry, 'conf-1')).toBeNull(); }); + test('a confidential registration with no secret ref is replaced, never used without its secret', async () => { + await ensureDcrClient(entry, confidential, callback, confidentialAnswer([], { client_id: 'conf-noref' })); + await Model.updateOne({ clientId: 'conf-noref' }, { $set: { clientSecretRef: null } }); + expect(await dcrClientForRow(entry, 'conf-noref')).toBeNull(); + const client = await ensureDcrClient(entry, confidential, callback, confidentialAnswer([], { client_id: 'conf-fresh' })); + expect(client.clientId).toBe('conf-fresh'); + }); + + test('a registration whose secret key left the ring is replaced on the next start', async () => { + await ensureDcrClient(entry, confidential, callback, confidentialAnswer([], { client_id: 'conf-oldkey' })); + const keys = process.env.CONNECTOR_SECRET_KEYS; + const active = process.env.CONNECTOR_SECRET_ACTIVE_KEY; + process.env.CONNECTOR_SECRET_KEYS = `dcr-rotated:${Buffer.alloc(32, 9).toString('base64')}`; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'dcr-rotated'; + try { + expect(await dcrClientForRow(entry, 'conf-oldkey')).toBeNull(); + const client = await ensureDcrClient(entry, confidential, callback, confidentialAnswer([], { client_id: 'conf-newkey' })); + expect(client).toMatchObject({ clientId: 'conf-newkey', clientSecret: 'issued-secret-1' }); + } finally { + process.env.CONNECTOR_SECRET_KEYS = keys; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = active; + } + }); + test('a registration whose secret can no longer be read is replaced on the next start', async () => { await ensureDcrClient(entry, confidential, callback, confidentialAnswer([], { client_id: 'conf-old' })); await ConnectorSecret.deleteMany({}); @@ -313,17 +337,20 @@ describe('instance-scoped hosted MCP DCR registry', () => { expect(exchange.body.get('code_verifier')).toBe('verifier-1'); expect(exchange.body.get('client_secret')).toBeNull(); - // A post-registered client revokes with post where the vendor lists it... + // Vera on #2377: a recorded method is sent as recorded, never filtered by an + // advertised list. The removal passes `{}` for a dcr entry, and RFC 8414 + // defaults an omitted list to basic, which turned a post-registered client + // into Basic. Held on both inputs: no revocation metadata at all, and a + // discovered document that omits the revocation list. const post = { ...client, tokenEndpointAuthMethod: 'client_secret_post' }; - const listed = { ...confidential, revocation_endpoint_auth_methods_supported: ['client_secret_basic', 'client_secret_post'] }; - const revoke = buildRevocationRequest(entry, listed, post, { token: 'tok', tokenTypeHint: 'refresh_token' }); - expect(revoke.headers.Authorization).toBeUndefined(); - expect(revoke.body.get('client_id')).toBe('conf-1'); - expect(revoke.body.get('client_secret')).toBe('issued-secret-1'); - // ...and with basic where the list is omitted, which RFC 8414 defaults to basic. - const defaulted = buildRevocationRequest(entry, confidential, post, { token: 'tok', tokenTypeHint: 'refresh_token' }); - expect(defaulted.headers.Authorization).toMatch(/^Basic /); - expect(defaulted.body.get('client_secret')).toBeNull(); + for (const revocationMetadata of [{}, confidential]) { + const revoke = buildRevocationRequest(entry, revocationMetadata, post, { token: 'tok', tokenTypeHint: 'refresh_token' }); + expect(revoke.headers.Authorization).toBeUndefined(); + expect(revoke.body.get('client_id')).toBe('conf-1'); + expect(revoke.body.get('client_secret')).toBe('issued-secret-1'); + const refresh = buildRefreshRequest(entry, revocationMetadata, post, { refreshToken: 'rt-1' }); + expect(refresh.body.get('client_secret')).toBe('issued-secret-1'); + } }); test('refresh still sends the secret when the caller copies only clientId and clientSecret', async () => { diff --git a/backend/__tests__/unit/services/connectionRemoval.test.js b/backend/__tests__/unit/services/connectionRemoval.test.js index 31b4b09f79..28742a1fed 100644 --- a/backend/__tests__/unit/services/connectionRemoval.test.js +++ b/backend/__tests__/unit/services/connectionRemoval.test.js @@ -101,6 +101,22 @@ describe('removeConnection (a hosted-MCP row)', () => { expect(deps.clientIdFor).not.toHaveBeenCalled(); }); + it('revokes a DCR confidential row with the method it registered (TASK-248, Vera #2377)', async () => { + const dcrEntry = { ...ENTRY, client: 'dcr' }; + const { deps, revokeGrants } = recorder({ entryFor: () => dcrEntry }); + deps.dcrClientForRow = jest.fn(async () => ({ + clientId: 'conf-1', clientSecret: 'registered-secret', tokenEndpointAuthMethod: 'client_secret_post', + })); + const row = hostedRow({ config: { + entryId: 'linear', clientId: 'conf-1', credentialRef: 'access-ref', refreshTokenRef: 'refresh-ref', + } }); + const result = await remove(row, deps, revokeGrants); + expect(result.removed).toBe(true); + expect(deps.revokeAtVendor).toHaveBeenCalledWith(expect.objectContaining({ + clientId: 'conf-1', clientSecret: 'registered-secret', tokenEndpointAuthMethod: 'client_secret_post', + })); + }); + it('removes a refused DCR row locally and returns the vendor page without sending its token', async () => { const dcrEntry = { ...ENTRY, client: 'dcr' }; const { deps, revokeGrants } = recorder({ entryFor: () => dcrEntry }); @@ -542,6 +558,25 @@ describe('revokeTokenAtVendor (RFC 7009)', () => { refuse.mockRestore(); } }); + it('a post-registered DCR client revokes with post, though a dcr removal reads no metadata', async () => { + const dcrEntry = { ...ENTRY, client: 'dcr' }; + const fetchImpl = jest.fn(async () => ({ ok: true, status: 200, json: async () => ({}) })); + await revokeTokenAtVendor({ + entry: dcrEntry, + clientId: 'conf-1', + clientSecret: 'registered-secret', + tokenEndpointAuthMethod: 'client_secret_post', + token: 'held-token', + tokenTypeHint: 'refresh_token', + }, fetchImpl); + expect(fetchImpl).toHaveBeenCalledTimes(1); + const [, init] = fetchImpl.mock.calls[0]; + expect(init.headers.Authorization).toBeUndefined(); + const body = new URLSearchParams(init.body); + expect(body.get('client_id')).toBe('conf-1'); + expect(body.get('client_secret')).toBe('registered-secret'); + }); + const call = (response) => { const fetchImpl = jest.fn(async () => response); return { diff --git a/backend/__tests__/unit/services/hostedMcpCredential.test.js b/backend/__tests__/unit/services/hostedMcpCredential.test.js index 32ee4a35b1..ab1fa178b9 100644 --- a/backend/__tests__/unit/services/hostedMcpCredential.test.js +++ b/backend/__tests__/unit/services/hostedMcpCredential.test.js @@ -28,6 +28,7 @@ const { EXPIRY_SKEW_MS, REFRESH_LEASE_MS, WINNER_WAIT_MS, + refreshTokenAtVendor, } = require('../../../services/hostedMcpCredentialService'); const { undeclaredPaths } = require('../../utils/schemaPathGuard'); @@ -222,6 +223,49 @@ describe('the fast path', () => { expect(h.deps.clientIdFor).not.toHaveBeenCalled(); }); + test('a DCR confidential client refreshes with the method it registered (TASK-248, Vera #2377)', async () => { + const dcrEntry = { ...ENTRY, client: 'dcr' }; + const row = stale({ clientId: 'conf-1' }); + const h = harness({ row, entryFor: () => dcrEntry }); + h.deps.dcrClientForRow = jest.fn(async () => ({ + clientId: 'conf-1', clientSecret: 'registered-secret', tokenEndpointAuthMethod: 'client_secret_post', + })); + await credentialFor(row, h.deps); + expect(h.refreshAtVendor).toHaveBeenCalledWith(expect.objectContaining({ + clientId: 'conf-1', clientSecret: 'registered-secret', tokenEndpointAuthMethod: 'client_secret_post', + })); + }); + + test('the vendor refresh sends a DCR client\'s recorded method, whatever the vendor lists', async () => { + // Discovery lists basic first, so a list lookup would pick Basic; the + // registration said post, and post is what must go out (Vera, #2377). + const dcrEntry = { ...ENTRY, client: 'dcr' }; + const metadata = { + issuer: dcrEntry.issuer, + authorization_endpoint: `${dcrEntry.issuer}/authorize`, + token_endpoint: `${dcrEntry.issuer}/token`, + token_endpoint_auth_methods_supported: ['client_secret_basic', 'client_secret_post'], + }; + const fetchImpl = jest.fn() + .mockResolvedValueOnce({ ok: true, status: 200, json: async () => metadata }) + .mockResolvedValueOnce({ ok: true, status: 200, json: async () => ({ access_token: 'new-access', expires_in: 3600 }) }); + const result = await refreshTokenAtVendor({ + entry: dcrEntry, + clientId: 'conf-1', + clientSecret: 'registered-secret', + tokenEndpointAuthMethod: 'client_secret_post', + refreshToken: 'old-refresh', + }, fetchImpl); + expect(result.accessToken).toBe('new-access'); + const [url, init] = fetchImpl.mock.calls[1]; + expect(url).toBe(metadata.token_endpoint); + expect(init.headers.Authorization).toBeUndefined(); + const body = new URLSearchParams(init.body); + expect(body.get('client_id')).toBe('conf-1'); + expect(body.get('client_secret')).toBe('registered-secret'); + expect(body.get('refresh_token')).toBe('old-refresh'); + }); + test('a refused or unknown DCR client requires reconnect before serving any token', async () => { const dcrEntry = { ...ENTRY, client: 'dcr' }; const row = live({ clientId: 'refused-client' }); diff --git a/backend/services/connectionRemovalService.ts b/backend/services/connectionRemovalService.ts index 4854195b8e..7ec59126ba 100644 --- a/backend/services/connectionRemovalService.ts +++ b/backend/services/connectionRemovalService.ts @@ -55,6 +55,7 @@ import { discoverAuthorizationServer, resolvedClientId, resolvedClientSecret, + type HostedMcpClientConfiguration, } from './hostedMcpIntakeService'; import * as connectorSecrets from './connectorSecrets'; import { upstreamFetch } from './upstreamFetch'; @@ -110,6 +111,8 @@ export interface RemovalDeps { entry: HostedMcpEntry; clientId: string; clientSecret?: string; + /** A DCR confidential client's registered method (TASK-248); sent as recorded. */ + tokenEndpointAuthMethod?: HostedMcpClientConfiguration['tokenEndpointAuthMethod']; token: string; tokenTypeHint: 'refresh_token' | 'access_token'; }) => Promise; @@ -165,6 +168,7 @@ export const revokeTokenAtVendor = async (input: { entry: HostedMcpEntry; clientId: string; clientSecret?: string; + tokenEndpointAuthMethod?: HostedMcpClientConfiguration['tokenEndpointAuthMethod']; token: string; tokenTypeHint: 'refresh_token' | 'access_token'; }, fetchImpl: typeof fetch = hostedVendorFetch): Promise => { @@ -184,7 +188,11 @@ export const revokeTokenAtVendor = async (input: { const request = buildRevocationRequest( input.entry, metadata, - { clientId: input.clientId, ...(input.clientSecret ? { clientSecret: input.clientSecret } : {}) }, + { + clientId: input.clientId, + ...(input.clientSecret ? { clientSecret: input.clientSecret } : {}), + ...(input.tokenEndpointAuthMethod ? { tokenEndpointAuthMethod: input.tokenEndpointAuthMethod } : {}), + }, { token: input.token, tokenTypeHint: input.tokenTypeHint }, ); const response = await upstreamFetch(target.endpoint, { @@ -320,6 +328,7 @@ export const removeConnection = async (options: { const recordedClientId = connection?.config?.clientId; let clientId: string | undefined; let clientSecret: string | undefined; + let tokenEndpointAuthMethod: HostedMcpClientConfiguration['tokenEndpointAuthMethod']; let clientMatches = true; try { if (entry.client === 'dcr') { @@ -327,6 +336,7 @@ export const removeConnection = async (options: { clientMatches = Boolean(recorded); clientId = recorded?.clientId; clientSecret = recorded?.clientSecret; + tokenEndpointAuthMethod = recorded?.tokenEndpointAuthMethod; } else { clientId = deps.clientIdFor(entry); // During the first rollout legacy rows have no snapshot. The backfill @@ -389,6 +399,7 @@ export const removeConnection = async (options: { entry, clientId: clientId as string, ...(clientSecret ? { clientSecret } : {}), + ...(tokenEndpointAuthMethod ? { tokenEndpointAuthMethod } : {}), token, tokenTypeHint, }); diff --git a/backend/services/hostedMcpCredentialService.ts b/backend/services/hostedMcpCredentialService.ts index ba301438e4..6d63d9e3f1 100644 --- a/backend/services/hostedMcpCredentialService.ts +++ b/backend/services/hostedMcpCredentialService.ts @@ -54,6 +54,7 @@ import { discoverAuthorizationServer, resolvedClientId, resolvedClientSecret, + type HostedMcpClientConfiguration, } from './hostedMcpIntakeService'; import * as connectorSecrets from './connectorSecrets'; import { HOSTED_MCP_ACCESS_TOKEN, HOSTED_MCP_REFRESH_TOKEN } from './connectorSecretKinds'; @@ -120,6 +121,8 @@ interface CredentialDeps { entry: HostedMcpEntry; clientId: string; clientSecret?: string; + /** A DCR confidential client's registered method (TASK-248); sent as recorded. */ + tokenEndpointAuthMethod?: HostedMcpClientConfiguration['tokenEndpointAuthMethod']; refreshToken: string; }) => Promise; secrets: Pick; @@ -206,6 +209,64 @@ export const defaultRowDeps = (): CredentialDeps['row'] => ({ }, }); +/** + * The vendor refresh: discover the token endpoint, authenticate as the row's + * own client, and read the answer. Exported, like the removal side's + * revokeTokenAtVendor, so the credential it actually sends is testable with a + * fetch double (a DCR client's registered method must reach this call). + */ +export const refreshTokenAtVendor = async ({ + entry, clientId, clientSecret, tokenEndpointAuthMethod, refreshToken, +}: Parameters[0], fetchImpl: typeof fetch = fetch): Promise => { + const metadata = await discoverAuthorizationServer(entry.issuer, fetchImpl); + const client = { + clientId, + ...(clientSecret ? { clientSecret } : {}), + ...(tokenEndpointAuthMethod ? { tokenEndpointAuthMethod } : {}), + }; + const tokenRequest = buildRefreshRequest(entry, metadata, client, { + refreshToken, + }); + const response = await upstreamFetch(metadata.token_endpoint, { + method: 'POST', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + Accept: 'application/json', + ...tokenRequest.headers, + }, + body: tokenRequest.body, + }, fetchImpl); + const body = await response.json().catch(() => ({} as Record)) as Record; + if (!response.ok) { + const code = String(body.error || ''); + if (code === 'invalid_client' && entry.client === 'dcr') { + await refuseDcrClient(entry, clientId); + throw new HostedMcpCredentialError( + 'reconnect_required', + 'OAuth client was refused by the provider; reconnect to continue', + ); + } + // `invalid_grant` is the vendor saying the refresh token itself is done: + // revoked from the vendor's side, or run out its own lifetime. Everything + // else — a 500, a 429, a body we cannot read — is the vendor having a bad + // moment, and the next call should try again with the same pair. + const definitive = code === 'invalid_grant' || response.status === 400 || response.status === 401; + throw new HostedMcpCredentialError( + definitive ? 'connection_error' : 'refresh_unreachable', + `refresh refused (${response.status}${code ? ` ${code}` : ''})`, + !definitive, + ); + } + if (!body.access_token) { + throw new HostedMcpCredentialError('refresh_unreachable', 'refresh returned no access token', true); + } + return { + accessToken: String(body.access_token), + refreshToken: body.refresh_token ? String(body.refresh_token) : undefined, + expiresIn: typeof body.expires_in === 'number' ? body.expires_in : undefined, + }; +}; + const defaultDeps = (): CredentialDeps => ({ now: () => new Date(), entryFor: (row) => { @@ -231,50 +292,7 @@ const defaultDeps = (): CredentialDeps => ({ { new: true }, ); }, - refreshAtVendor: async ({ entry, clientId, clientSecret, refreshToken }) => { - const metadata = await discoverAuthorizationServer(entry.issuer); - const tokenRequest = buildRefreshRequest(entry, metadata, { clientId, ...(clientSecret ? { clientSecret } : {}) }, { - refreshToken, - }); - const response = await upstreamFetch(metadata.token_endpoint, { - method: 'POST', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - Accept: 'application/json', - ...tokenRequest.headers, - }, - body: tokenRequest.body, - }); - const body = await response.json().catch(() => ({} as Record)) as Record; - if (!response.ok) { - const code = String(body.error || ''); - if (code === 'invalid_client' && entry.client === 'dcr') { - await refuseDcrClient(entry, clientId); - throw new HostedMcpCredentialError( - 'reconnect_required', - 'OAuth client was refused by the provider; reconnect to continue', - ); - } - // `invalid_grant` is the vendor saying the refresh token itself is done: - // revoked from the vendor's side, or run out its own lifetime. Everything - // else — a 500, a 429, a body we cannot read — is the vendor having a bad - // moment, and the next call should try again with the same pair. - const definitive = code === 'invalid_grant' || response.status === 400 || response.status === 401; - throw new HostedMcpCredentialError( - definitive ? 'connection_error' : 'refresh_unreachable', - `refresh refused (${response.status}${code ? ` ${code}` : ''})`, - !definitive, - ); - } - if (!body.access_token) { - throw new HostedMcpCredentialError('refresh_unreachable', 'refresh returned no access token', true); - } - return { - accessToken: String(body.access_token), - refreshToken: body.refresh_token ? String(body.refresh_token) : undefined, - expiresIn: typeof body.expires_in === 'number' ? body.expires_in : undefined, - }; - }, + refreshAtVendor: (input) => refreshTokenAtVendor(input), secrets: connectorSecrets, row: defaultRowDeps(), sleep: (ms) => new Promise((resolve) => { setTimeout(resolve, ms); }), @@ -349,6 +367,7 @@ export const credentialFor = async ( const recordedClientId = row.config?.clientId; let clientId: string; let clientSecret: string | undefined; + let tokenEndpointAuthMethod: HostedMcpClientConfiguration['tokenEndpointAuthMethod']; try { if (entry.client === 'dcr') { const recorded = await deps.dcrClientForRow(entry, recordedClientId); @@ -359,6 +378,7 @@ export const credentialFor = async ( } clientId = recorded.clientId; clientSecret = recorded.clientSecret; + tokenEndpointAuthMethod = recorded.tokenEndpointAuthMethod; } else { clientId = deps.clientIdFor(entry); clientSecret = deps.clientSecretFor(entry); @@ -427,7 +447,9 @@ export const credentialFor = async ( let refreshed: RefreshResult; try { - refreshed = await deps.refreshAtVendor({ entry, clientId, clientSecret, refreshToken }); + refreshed = await deps.refreshAtVendor({ + entry, clientId, clientSecret, ...(tokenEndpointAuthMethod ? { tokenEndpointAuthMethod } : {}), refreshToken, + }); } catch (error) { const failure = error instanceof HostedMcpCredentialError ? error diff --git a/backend/services/hostedMcpIntakeService.ts b/backend/services/hostedMcpIntakeService.ts index 87f3f60d6b..02d4ac4d6e 100644 --- a/backend/services/hostedMcpIntakeService.ts +++ b/backend/services/hostedMcpIntakeService.ts @@ -426,10 +426,17 @@ const endpointAuthentication = ( client: HostedMcpClientConfiguration, endpoint: HostedMcpEndpointKind, ): ClientEndpointAuthentication => { - // A DCR client is confidential when this instance registered it with a - // secret (TASK-248). It is recognised by the secret itself, not by the - // method field, so a caller that copies only clientId and clientSecret (the - // refresh and removal paths) still sends the secret. + // A confidential client this instance registered (TASK-248) authenticates + // exactly as it registered, at every endpoint. No advertised list is read: + // the removal path passes no metadata for a dcr entry, and RFC 8414's + // omitted-list default (basic) would otherwise turn a post-registered client + // into Basic, and its invalid_client would refuse the registration every + // row shares (Vera, #2377). + if (entry.client === 'dcr' && client.clientSecret && client.tokenEndpointAuthMethod) { + return confidentialAuthentication(client, client.tokenEndpointAuthMethod); + } + // A DCR client with a secret but no recorded method still sends the secret, + // choosing from the advertised list like a pre-registered client. const confidential = entry.client === 'pre-registered' || (entry.client === 'dcr' && Boolean(client.clientSecret)); if (!confidential) { @@ -451,14 +458,7 @@ const endpointAuthentication = ( const methods = supported === undefined ? ['client_secret_basic'] : (Array.isArray(supported) ? supported : []); - // The method a DCR client registered with is tried first. Without it the - // order is basic, then post: the same order registration chose by, so the - // answer matches while the vendor's list is unchanged. - const registered = client.tokenEndpointAuthMethod; - const preference = registered - ? [registered, ...CONFIDENTIAL_CLIENT_METHODS.filter((candidate) => candidate !== registered)] - : [...CONFIDENTIAL_CLIENT_METHODS]; - const method = preference.find((candidate) => methods.includes(candidate)); + const method = CONFIDENTIAL_CLIENT_METHODS.find((candidate) => methods.includes(candidate)); if (!method) { throw new HostedMcpClientError( 'client_auth_unsupported', diff --git a/docs/plans/hosted-mcp-connection-scope.md b/docs/plans/hosted-mcp-connection-scope.md index b3707bd3de..0c8c911fa0 100644 --- a/docs/plans/hosted-mcp-connection-scope.md +++ b/docs/plans/hosted-mcp-connection-scope.md @@ -206,9 +206,9 @@ Miro's AS lists only `client_secret_post` and `client_secret_basic` (fetched 202 - **When `none` is not listed,** the start registers with `client_secret_basic`, or `client_secret_post` when that is the only one listed. Basic comes first, the same order the token calls choose by. The answer must carry a non-empty `client_secret` and register that same method; an omitted method counts as `client_secret_basic`, as above. - **The secret is stored before the registration.** It goes into the connector-secret store (`hosted-mcp-dcr-client-secret`, AES-GCM like the tokens), owned by the registration's id. The registration holds only `clientSecretRef`, the method and `clientSecretExpiresAt`. A start that loses the insert race deletes its own secret. -- **The secret authenticates every token call.** The exchange, refresh and revocation authenticate with it, and PKCE `S256` is still sent. `endpointAuthentication` recognises a confidential DCR client by its secret, so the refresh and removal paths, which pass only `clientId` and `clientSecret`, still send it. +- **The secret authenticates every token call, by the method it registered.** The exchange, refresh and revocation authenticate with it, and PKCE `S256` is still sent. The recorded method is carried through the refresh and removal paths and sent as recorded, with no advertised list consulted. A dcr removal reads no metadata, and RFC 8414's omitted-list default (basic) would otherwise turn a post-registered client into Basic, whose `invalid_client` would refuse the registration every row shares (Vera, #2377). - **A secret is replaced before it lapses.** A `client_secret_expires_at` within 24 hours retires the registration at the next start, which registers anew. An answer whose secret already lapses within that margin is refused `client_registration_refused`. Zero means it never lapses (RFC 7591 §3.2.1). -- **The sweep knows the owner.** A secret is kept while its registration exists, has not been refused, and still names it, current or retired. A refused registration's secret is deleted at once. A registration whose secret can no longer be read is replaced at the next start, and its rows go to reconnect. +- **The sweep knows the owner.** A secret is kept while its registration exists, has not been refused, and still names it, current or retired. A refused registration's secret is deleted at once. A registration whose secret can no longer be read (its row is gone, its key left the ring, or it holds no ref) is replaced at the next start, and its rows go to reconnect. The sweep needed this because it deletes a secret that no active row names once a 10-minute grace has passed (`services/installable/installableReconciler.ts:272`, `:30`), and a registration is not a row. From bd6ba2262aa03a1b72d8e0d1f471637e736dfcf9 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:56:39 -0700 Subject: [PATCH 3/3] fix(hosted-mcp): the vendor refresh defaults to the guarded fetch refreshTokenAtVendor took `fetchImpl = fetch`, so a refresh with no injected fetch reached the vendor through the platform fetch and skipped the outbound guard. It now defaults to hostedVendorFetch, as revokeTokenAtVendor and discoverAuthorizationServer already do. hostedVendorFetch.wiring.test.js caught it on CI after the rebase. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4 --- backend/services/hostedMcpCredentialService.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/backend/services/hostedMcpCredentialService.ts b/backend/services/hostedMcpCredentialService.ts index 6d63d9e3f1..ef578c1a5c 100644 --- a/backend/services/hostedMcpCredentialService.ts +++ b/backend/services/hostedMcpCredentialService.ts @@ -59,6 +59,7 @@ import { import * as connectorSecrets from './connectorSecrets'; import { HOSTED_MCP_ACCESS_TOKEN, HOSTED_MCP_REFRESH_TOKEN } from './connectorSecretKinds'; import { upstreamFetch } from './upstreamFetch'; +import { hostedVendorFetch } from './hostedVendorFetch'; import { dcrClientForRow, refuseDcrClient } from './hostedMcpDcrRegistry'; import { HostedMcpCredentialError } from './hostedMcpCredentialError'; export { HostedMcpCredentialError, type HostedMcpCredentialErrorCode } from './hostedMcpCredentialError'; @@ -217,7 +218,7 @@ export const defaultRowDeps = (): CredentialDeps['row'] => ({ */ export const refreshTokenAtVendor = async ({ entry, clientId, clientSecret, tokenEndpointAuthMethod, refreshToken, -}: Parameters[0], fetchImpl: typeof fetch = fetch): Promise => { +}: Parameters[0], fetchImpl: typeof fetch = hostedVendorFetch): Promise => { const metadata = await discoverAuthorizationServer(entry.issuer, fetchImpl); const client = { clientId,