diff --git a/backend/__tests__/service/hostedMcpDcrRegistry.test.js b/backend/__tests__/service/hostedMcpDcrRegistry.test.js index 76e8b0940e..c0a730282e 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,255 @@ 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 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({}); + 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(); + + // 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' }; + 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 () => { + // 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/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/__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/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/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/hostedMcpCredentialService.ts b/backend/services/hostedMcpCredentialService.ts index ba301438e4..ef578c1a5c 100644 --- a/backend/services/hostedMcpCredentialService.ts +++ b/backend/services/hostedMcpCredentialService.ts @@ -54,10 +54,12 @@ import { discoverAuthorizationServer, resolvedClientId, resolvedClientSecret, + type HostedMcpClientConfiguration, } from './hostedMcpIntakeService'; 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'; @@ -120,6 +122,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 +210,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 = hostedVendorFetch): 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 +293,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 +368,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 +379,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 +448,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/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..02d4ac4d6e 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,48 @@ 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 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) { return { headers: {}, body: { client_id: client.clientId } }; } if (!client.clientSecret) { @@ -414,25 +458,14 @@ 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)); + const method = CONFIDENTIAL_CLIENT_METHODS.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..0c8c911fa0 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, 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 (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. **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',