From e1651cddfe984428e90c015443db252f11225411 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 01:59:56 -0800 Subject: [PATCH 1/7] feat(connectors): operator-key hosted-MCP mode, per-agent binding and paste-key intake (TASK-330) Ports Kai's draft #3043 (kai/task-330-latchkey-connector at 77dc4f54f, 11 commits) onto main 8e60a6652 as one commit. Conflicts resolved in hostedMcpEntryService.ts (main's tokenEndpointAuthMethod moves into the OAuth member of the union), grants.ts (import) and grants.mint.test.js (both sides' describes kept). Written by Kai, a Commonly agent; ported by the CTO builder session. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../hostedMcpAgentIndexMigration.test.js | 99 ++++++++ .../service/hostedMcpConnectionRow.test.js | 25 ++ .../__tests__/unit/routes/grants.mint.test.js | 50 ++++ .../unit/routes/hostedMcpConnect.test.js | 14 ++ .../routes/hostedMcpOperatorKey.route.test.js | 224 ++++++++++++++++++ .../hostedMcpCredential.operatorKey.test.js | 64 +++++ .../services/hostedMcpOperatorKey.test.js | 38 +++ .../hostedMcpOperatorKeyIntake.test.js | 85 +++++++ backend/integrations/hostedMcp/entries.ts | 3 +- backend/models/Integration.ts | 27 ++- backend/routes/grants.ts | 11 + backend/routes/hostedMcpConnect.ts | 29 ++- backend/routes/hostedMcpOperatorKey.ts | 121 ++++++++++ .../scripts/backfill-hosted-mcp-client-id.ts | 3 +- .../scripts/migrate-hosted-mcp-agent-index.js | 80 +++++++ backend/services/connectionRemovalService.ts | 15 +- backend/services/hostedMcpClientIdBackfill.ts | 7 +- .../services/hostedMcpCredentialService.ts | 19 +- backend/services/hostedMcpDcrRegistry.ts | 12 +- backend/services/hostedMcpEntryService.ts | 51 +++- backend/services/hostedMcpIntakeService.ts | 24 +- backend/services/hostedMcpOperatorKey.ts | 29 +++ .../services/hostedMcpOperatorKeyIntake.ts | 74 ++++++ backend/services/hostedMcpToolDefinitions.ts | 18 +- .../installable/installableCatalogService.ts | 8 +- backend/utils/serverOwnedConfigKeys.ts | 2 +- 26 files changed, 1069 insertions(+), 63 deletions(-) create mode 100644 backend/__tests__/service/hostedMcpAgentIndexMigration.test.js create mode 100644 backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js create mode 100644 backend/__tests__/unit/services/hostedMcpCredential.operatorKey.test.js create mode 100644 backend/__tests__/unit/services/hostedMcpOperatorKey.test.js create mode 100644 backend/__tests__/unit/services/hostedMcpOperatorKeyIntake.test.js create mode 100644 backend/routes/hostedMcpOperatorKey.ts create mode 100644 backend/scripts/migrate-hosted-mcp-agent-index.js create mode 100644 backend/services/hostedMcpOperatorKey.ts create mode 100644 backend/services/hostedMcpOperatorKeyIntake.ts diff --git a/backend/__tests__/service/hostedMcpAgentIndexMigration.test.js b/backend/__tests__/service/hostedMcpAgentIndexMigration.test.js new file mode 100644 index 000000000..b0f437230 --- /dev/null +++ b/backend/__tests__/service/hostedMcpAgentIndexMigration.test.js @@ -0,0 +1,99 @@ +/** + * The TASK-330 index migration (scripts/migrate-hosted-mcp-agent-index.js). + * Before it runs, the old `(createdBy, entryId)` index refuses a second agent's + * key with E11000. After it runs, that key saves, and the old index is gone. + * Run against a real mongod, because the failure is an index, not a mock. + * + * Every arm seeds its own pre-migration state (old index present, new index + * absent, agent-a's key saved) so no arm depends on another having run first. + */ +process.env.PG_HOST = ''; + +const mongoose = require('mongoose'); +const Integration = require('../../models/Integration'); +const { setupMongoDb, closeMongoDb, clearMongoDb } = require('../utils/testUtils'); +const { run, OLD_INDEX } = require('../../scripts/migrate-hosted-mcp-agent-index'); + +const OWNER = new mongoose.Types.ObjectId(); +const keyFor = (agentId) => ({ + scope: 'user', + type: 'hosted-mcp', + status: 'connected', + createdBy: OWNER, + config: { entryId: 'latchkey', intake: 'operator_key', agentId }, +}); + +const indexNames = async () => (await Integration.collection.indexes()).map((index) => index.name); + +/** Put the database in the production state: the old index present, the agent-scoped one absent. */ +const seedPreMigration = async () => { + const agentIndex = (await Integration.collection.indexes()) + .find((index) => index.key.createdBy === 1 && index.key['config.agentId'] === 1); + if (agentIndex) await Integration.collection.dropIndex(agentIndex.name); + if (!(await indexNames()).includes(OLD_INDEX)) { + await Integration.collection.createIndex( + { createdBy: 1, 'config.entryId': 1 }, + { unique: true, partialFilterExpression: { type: 'hosted-mcp' }, name: OLD_INDEX }, + ); + } +}; + +describe('the TASK-330 agent-scoped index migration', () => { + beforeAll(async () => { + await setupMongoDb(); + await Integration.init(); + }); + + beforeEach(async () => { + await Integration.deleteMany({}); + await seedPreMigration(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + afterAll(async () => { + await clearMongoDb(); + await closeMongoDb(); + }); + + test('before the migration, the old index refuses a second agent key for the same person and entry', async () => { + await Integration.create(keyFor('agent-a')); + await expect(Integration.create(keyFor('agent-b'))).rejects.toMatchObject({ code: 11000 }); + }); + + test('the migration builds the new index, drops the old one, and lets a second agent key save', async () => { + await Integration.create(keyFor('agent-a')); + + await run({ apply: true }); + + const names = await indexNames(); + expect(names).not.toContain(OLD_INDEX); + expect(names.some((name) => name.includes('config.agentId'))).toBe(true); + await expect(Integration.create(keyFor('agent-b'))).resolves.toBeDefined(); + // The migration deletes no rows: agent-a's key is still there. + expect(await Integration.countDocuments({ 'config.agentId': 'agent-a' })).toBe(1); + }); + + test('a failed build leaves the old index in place, so uniqueness is never lost', async () => { + jest.spyOn(Integration.collection, 'createIndex').mockRejectedValueOnce(new Error('build failed')); + + await expect(run({ apply: true })).rejects.toThrow('build failed'); + + expect(await indexNames()).toContain(OLD_INDEX); + await Integration.create(keyFor('agent-a')); + await expect(Integration.create(keyFor('agent-b'))).rejects.toMatchObject({ code: 11000 }); + }); + + test('a second run is a no-op rather than an error', async () => { + await run({ apply: true }); + await expect(run({ apply: true })).resolves.toBeUndefined(); + expect(await indexNames()).not.toContain(OLD_INDEX); + }); + + test('read-only by default: a dry run leaves the old index in place', async () => { + await run({ apply: false }); + expect(await indexNames()).toContain(OLD_INDEX); + }); +}); diff --git a/backend/__tests__/service/hostedMcpConnectionRow.test.js b/backend/__tests__/service/hostedMcpConnectionRow.test.js index 5777f8768..340ac2be6 100644 --- a/backend/__tests__/service/hostedMcpConnectionRow.test.js +++ b/backend/__tests__/service/hostedMcpConnectionRow.test.js @@ -43,6 +43,9 @@ const shape = (overrides = {}) => ({ // failure names the key that was dropped rather than the object that lost it. const RECORD_KEYS = { 'config.entryId': 'linear', + // The agent an operator key is bound to (TASK-330). Declared, so a $set the + // schema drops is caught here and not by a mint that refuses "not bound". + 'config.agentId': 'agent-a', 'config.intake': 'oauth', 'config.clientId': 'https://api.commonly.me/api/integrations/connect/hosted-mcp/linear/client-metadata', 'config.providerSubject': 'subject-1', @@ -158,6 +161,28 @@ describe('the hosted-mcp connection row', () => { await expect(Integration.create(shape({ createdBy: OWNER_B }))).resolves.toBeDefined(); }); + test('one owner holds one key per bound agent, and an OAuth row co-exists with them', async () => { + // TASK-330: an operator key is one agent's bearer, so the second agent's + // key for the same vendor is a second row, not an E11000. A second key for + // the SAME agent is still one row, and an OAuth row (no agentId) is its own + // row beside them. Each is per (person, entry, agent). + const keyFor = (agentId) => shape({ config: { entryId: ENTRY_A, intake: 'operator_key', agentId } }); + + await expect(Integration.create(keyFor('agent-a'))).resolves.toBeDefined(); + await expect(Integration.create(keyFor('agent-b'))).resolves.toBeDefined(); + await expect(Integration.create(keyFor('agent-a'))).rejects.toMatchObject({ code: 11000 }); + await expect(Integration.create(shape())).resolves.toBeDefined(); + await expect(Integration.create(shape())).rejects.toMatchObject({ code: 11000 }); + }); + + test('the declared index keys on the bound agent as well as the person and entry', () => { + const declared = Integration.schema.indexes().find(([keys, options]) => ( + keys.createdBy === 1 && options.partialFilterExpression?.type === 'hosted-mcp' + )); + expect(declared[0]).toEqual({ createdBy: 1, 'config.entryId': 1, 'config.agentId': 1 }); + expect(declared[1].unique).toBe(true); + }); + test('positive control: the index is PARTIAL, not a unique index on createdBy', async () => { // If the declaration lost `partialFilterExpression`, the unique key would // still be enforced on hosted-mcp rows and the arm above would keep diff --git a/backend/__tests__/unit/routes/grants.mint.test.js b/backend/__tests__/unit/routes/grants.mint.test.js index 9976ecd96..1306af3c6 100644 --- a/backend/__tests__/unit/routes/grants.mint.test.js +++ b/backend/__tests__/unit/routes/grants.mint.test.js @@ -500,3 +500,53 @@ describe('task-bound mint', () => { expect(await RoomGrant.countDocuments({})).toBe(0); }); }); + +/** + * TASK-330: an operator-key row is one agent's bearer, so its grant may name + * only that agent, and only alone. The helper has its own arms; these run the + * mint, so the test goes red if the grants.ts call is removed. + */ +describe('POST /api/grants on an operator-key hosted connection', () => { + const KEY_ENTRY = { ...ENTRY, id: 'latchkey', title: 'Latchkey', credentialMode: 'operator_key' }; + const KEY_ID = 'eeeeeeeeeeeeeeeeeeeeee02'; + const keyRow = (config = {}) => ({ + _id: KEY_ID, type: 'hosted-mcp', status: 'connected', createdBy: OWNER, + config: { entryId: 'latchkey', agentId: SEAT, credentialRef: 'secret-ref-key', ...config }, + }); + const keyMint = (body = {}) => mint({ connectionId: KEY_ID, tools: ['latchkey.list_issues'], ...body }); + + beforeEach(() => { + Integration.findById.mockResolvedValue(keyRow()); + }); + + test('an omitted audience is refused, not defaulted to every pod member', async () => { + const res = await withCatalog([KEY_ENTRY], () => keyMint()); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ error: 'invalid_audience' }); + expect(res.body.message).toMatch(/omitted audience/); + expect(await RoomGrant.countDocuments({})).toBe(0); + }); + + test('an audience naming another agent is refused', async () => { + const res = await withCatalog([KEY_ENTRY], () => keyMint({ audience: [OWNER] })); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'invalid_audience', message: `audience must be exactly [${SEAT}]` }); + expect(await RoomGrant.countDocuments({})).toBe(0); + }); + + test('an audience of the bound agent plus another member is refused', async () => { + const res = await withCatalog([KEY_ENTRY], () => keyMint({ audience: [SEAT, OWNER] })); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ error: 'invalid_audience' }); + expect(await RoomGrant.countDocuments({})).toBe(0); + }); + + test('an audience of exactly the bound agent passes the audience check', async () => { + const res = await withCatalog([KEY_ENTRY], () => keyMint({ audience: [SEAT] })); + // The mint then reaches the broker, which this harness does not stand up + // (see the 503 arm above), so the arm asserts what it names: not refused + // for its audience. + expect(res.body.error).not.toBe('invalid_audience'); + expect(res.status).not.toBe(400); + }); +}); diff --git a/backend/__tests__/unit/routes/hostedMcpConnect.test.js b/backend/__tests__/unit/routes/hostedMcpConnect.test.js index 06f4b57cf..fe2f6c603 100644 --- a/backend/__tests__/unit/routes/hostedMcpConnect.test.js +++ b/backend/__tests__/unit/routes/hostedMcpConnect.test.js @@ -368,6 +368,20 @@ describe('hosted-mcp connect: start', () => { entries.HOSTED_MCP_ENTRIES.pop(); }); + it('refuses an operator-key entry before any client, discovery or row read', async () => { + const entries = jest.requireMock('../../../integrations/hostedMcp/entries'); + entries.HOSTED_MCP_ENTRIES.push({ + ...FIXTURE_ENTRY, id: 'latchkey', credentialMode: 'operator_key', client: undefined, + }); + const res = await start('latchkey'); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'unknown_entry' }); + expect(intake.discoverAuthorizationServer).not.toHaveBeenCalled(); + expect(Integration.findOne).not.toHaveBeenCalled(); + expect(Integration.findOneAndUpdate).not.toHaveBeenCalled(); + entries.HOSTED_MCP_ENTRIES.pop(); + }); + it('requires a person, because it is the person who will be consenting', async () => { mockAuthedUser = undefined; const res = await start(); diff --git a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js new file mode 100644 index 000000000..60b9fefbd --- /dev/null +++ b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js @@ -0,0 +1,224 @@ +/** + * TASK-330 paste-key intake route. The row, the secret store, the vendor probe + * and the installation data are mocked at their module edges; the handler, the + * scope refusal, the intake probe and the sole-installer rule + * (services/runtimeIdentityOwnership.ts) are real. The key must never come back + * in a response or reach the row, and the scope the route reports must be a + * self-report whatever the body claims (Vera's review of 2d4e37fc7). + */ +const express = require('express'); +const request = require('supertest'); + +jest.mock('../../../models/Integration', () => ({ findOneAndUpdate: jest.fn(), updateOne: jest.fn() })); +jest.mock('../../../models/User', () => ({ findOne: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ AgentInstallation: { find: jest.fn() } })); +jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); +jest.mock('../../../services/connectorSecrets', () => ({ put: jest.fn() })); +jest.mock('../../../services/hostedMcpToolDefinitions', () => ({ + guardedFetch: jest.fn(), + listHostedMcpTools: jest.fn(), +})); + +const Integration = require('../../../models/Integration'); +const User = require('../../../models/User'); +const { AgentInstallation } = require('../../../models/AgentRegistry'); +const Pod = require('../../../models/Pod'); +const connectorSecrets = require('../../../services/connectorSecrets'); +const { listHostedMcpTools } = require('../../../services/hostedMcpToolDefinitions'); +const { HOSTED_MCP_ENTRIES } = require('../../../integrations/hostedMcp/entries'); +const { submitOperatorKey } = require('../../../routes/hostedMcpOperatorKey'); + +const OWNER = 'bbbbbbbbbbbbbbbbbbbbbb01'; +const OTHER = 'bbbbbbbbbbbbbbbbbbbbbb02'; +const AGENT = 'cccccccccccccccccccccc01'; +const ROW_ID = 'eeeeeeeeeeeeeeeeeeeeee02'; +const POD = 'aaaaaaaaaaaaaaaaaaaaaa01'; +const OTHER_POD = 'aaaaaaaaaaaaaaaaaaaaaa03'; +const DM_POD = 'aaaaaaaaaaaaaaaaaaaaaa02'; +const KEY = 'lk_live_SECRET_VALUE_DO_NOT_ECHO'; + +const KEY_ENTRY = { + id: 'latchkey', + title: 'Latchkey', + resource: 'https://latchkey.example/mcp', + credentialMode: 'operator_key', + scopes: [], + revoke: 'https://latchkey.example/keys', + tools: [{ + name: 'list_jobs', + upstreamName: 'list_jobs', + description: 'List jobs', + class: 'read', + inputSchema: { type: 'object' }, + }], +}; + +const app = express(); +app.use(express.json()); +app.post('/connect/hosted-mcp/:entryId/operator-key', (req, res, next) => { + const id = req.get('x-test-user'); + if (!id) return res.status(401).json({ error: 'unauthorized' }); + req.user = { id }; + return next(); +}, submitOperatorKey); + +const submit = (body = {}) => request(app) + .post('/connect/hosted-mcp/latchkey/operator-key') + .set('x-test-user', OWNER) + .send({ agentId: AGENT, key: KEY, ...body }); + +// The installations of the agent identity, and which of their pods are agent-dm. +let installations; +let dmPods; + +let shipped; +beforeEach(() => { + jest.clearAllMocks(); + shipped = HOSTED_MCP_ENTRIES.splice(0, HOSTED_MCP_ENTRIES.length, KEY_ENTRY); + User.findOne.mockReturnValue({ + select: () => ({ lean: async () => ({ _id: AGENT, botMetadata: { agentName: 'OpenClaw', instanceId: 'latch' } }) }), + }); + installations = [{ installedBy: OWNER, podId: POD }]; + dmPods = []; + AgentInstallation.find.mockImplementation(() => ({ + collation: () => ({ select: () => ({ lean: async () => installations }) }), + })); + Pod.find.mockImplementation(() => ({ select: () => ({ lean: async () => dmPods }) })); + listHostedMcpTools.mockResolvedValue([{ name: 'list_jobs' }]); + Integration.findOneAndUpdate.mockResolvedValue({ _id: ROW_ID, config: { authGeneration: 1 } }); + Integration.updateOne.mockResolvedValue({ acknowledged: true }); + connectorSecrets.put.mockResolvedValue('secret-ref-1'); +}); +afterEach(() => { + HOSTED_MCP_ENTRIES.splice(0, HOSTED_MCP_ENTRIES.length, ...shipped); +}); + +/** Every value the route handed to a model write, so a leak anywhere shows. */ +const everyWrittenValue = () => JSON.stringify([ + Integration.findOneAndUpdate.mock.calls, + Integration.updateOne.mock.calls, +]); + +describe('POST /connect/hosted-mcp/:entryId/operator-key', () => { + test('a refused scope among the operator picks is refused before any vendor call or row write', async () => { + const res = await submit({ scopePicks: ['read', 'jobs:approve'] }); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ error: 'scope_refused' }); + expect(res.body.message).toMatch(/jobs:approve/); + expect(listHostedMcpTools).not.toHaveBeenCalled(); + expect(Integration.findOneAndUpdate).not.toHaveBeenCalled(); + expect(connectorSecrets.put).not.toHaveBeenCalled(); + }); + + test('a vendor that rejects the key is refused, and no row or secret is written', async () => { + listHostedMcpTools.mockRejectedValue(Object.assign(new Error('nope'), { code: 'credential_rejected' })); + const res = await submit(); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'vendor_rejected', message: 'the vendor rejected this key' }); + expect(Integration.findOneAndUpdate).not.toHaveBeenCalled(); + expect(connectorSecrets.put).not.toHaveBeenCalled(); + }); + + test('an agentId that is not a bot is refused before the probe', async () => { + User.findOne.mockReturnValue({ select: () => ({ lean: async () => null }) }); + const res = await submit(); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ error: 'invalid_agent' }); + expect(listHostedMcpTools).not.toHaveBeenCalled(); + }); + + test('a bot with no agent identity is refused before the probe', async () => { + User.findOne.mockReturnValue({ select: () => ({ lean: async () => ({ _id: AGENT, botMetadata: {} }) }) }); + const res = await submit(); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ error: 'invalid_agent' }); + expect(AgentInstallation.find).not.toHaveBeenCalled(); + expect(listHostedMcpTools).not.toHaveBeenCalled(); + }); + + test('the sole owner of the agent passes, and the identity is read from the bot row, lowercased', async () => { + const res = await submit(); + expect(res.status).toBe(201); + expect(res.body).toEqual({ + integrationId: ROW_ID, status: 'connected', scopeCheck: 'self_report', toolCount: 1, + }); + expect(AgentInstallation.find).toHaveBeenCalledWith(expect.objectContaining({ agentName: 'openclaw', instanceId: 'latch' })); + }); + + test('a caller with no installation of the agent is refused 403 before any probe or write', async () => { + installations = []; + const res = await submit(); + expect(res.status).toBe(403); + expect(res.body).toEqual({ + error: 'agent_not_owned', + failure: 'owner_installation_missing', + message: 'you must be the sole installer of this agent', + }); + expect(listHostedMcpTools).not.toHaveBeenCalled(); + expect(Integration.findOneAndUpdate).not.toHaveBeenCalled(); + expect(connectorSecrets.put).not.toHaveBeenCalled(); + }); + + test('an agent with a second installer is refused 403, even when the caller also installed it', async () => { + installations = [ + { installedBy: OWNER, podId: POD }, + { installedBy: OTHER, podId: OTHER_POD }, + ]; + const res = await submit(); + expect(res.status).toBe(403); + expect(res.body).toMatchObject({ error: 'agent_not_owned', failure: 'another_installer' }); + expect(listHostedMcpTools).not.toHaveBeenCalled(); + expect(Integration.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + test('an agent-dm installation is a conversation, not ownership: the caller is refused', async () => { + installations = [{ installedBy: OWNER, podId: DM_POD }]; + dmPods = [{ _id: DM_POD }]; + const res = await submit(); + expect(res.status).toBe(403); + expect(res.body).toMatchObject({ error: 'agent_not_owned', failure: 'owner_installation_missing' }); + expect(listHostedMcpTools).not.toHaveBeenCalled(); + }); + + test('scopeCheck is a self-report even when the body claims a readback', async () => { + const res = await submit({ scopeCheck: 'read', reportedScopes: ['read'] }); + expect(res.status).toBe(201); + expect(res.body.scopeCheck).toBe('self_report'); + }); + + test('a valid key writes a pending row, stores the secret on it, then connects it', async () => { + const res = await submit(); + expect(res.status).toBe(201); + expect(Integration.findOneAndUpdate).toHaveBeenCalledWith( + { + type: 'hosted-mcp', createdBy: OWNER, 'config.entryId': 'latchkey', 'config.agentId': AGENT, + }, + expect.objectContaining({ $set: expect.objectContaining({ status: 'pending' }) }), + expect.objectContaining({ upsert: true }), + ); + expect(connectorSecrets.put).toHaveBeenCalledWith(ROW_ID, expect.anything(), KEY, expect.any(Number)); + expect(Integration.updateOne).toHaveBeenCalledWith( + { _id: ROW_ID }, + { $set: { status: 'connected', 'config.credentialRef': 'secret-ref-1' } }, + ); + }); + + test('the key appears in no response, no row write, and no thrown error text', async () => { + const ok = await submit(); + expect(JSON.stringify(ok.body)).not.toContain(KEY); + expect(everyWrittenValue()).not.toContain(KEY); + + listHostedMcpTools.mockRejectedValue(new Error(`upstream echoed ${KEY}`)); + const unreachable = await submit(); + expect(JSON.stringify(unreachable.body)).not.toContain(KEY); + }); + + test('a failed secret write leaves the row pending, never connected, and says so without the key', async () => { + connectorSecrets.put.mockRejectedValue(new Error(`store failed for ${KEY}`)); + const res = await submit(); + expect(res.status).toBe(500); + expect(res.body).toEqual({ error: 'key_store_failed', message: 'the key could not be stored; try again' }); + expect(Integration.updateOne).not.toHaveBeenCalled(); + expect(JSON.stringify(res.body)).not.toContain(KEY); + }); +}); diff --git a/backend/__tests__/unit/services/hostedMcpCredential.operatorKey.test.js b/backend/__tests__/unit/services/hostedMcpCredential.operatorKey.test.js new file mode 100644 index 000000000..4b93e9dc1 --- /dev/null +++ b/backend/__tests__/unit/services/hostedMcpCredential.operatorKey.test.js @@ -0,0 +1,64 @@ +/** + * `credentialFor` on an operator-key entry (TASK-330, Vera's read §3): the + * stored key is served as it stands, with no fence, no lease and no vendor + * refresh, and the freshness rule does not decide it. + */ +const { credentialFor, HostedMcpCredentialError } = require('../../../services/hostedMcpCredentialService'); + +const KEY_ENTRY = { id: 'latchkey', credentialMode: 'operator_key' }; +const ROW = { + _id: '68e1f2a4b5c6d7e8f9a0b1c2', + status: 'connected', + revokedAt: null, + config: { entryId: 'latchkey', agentId: 'a1', credentialRef: 'ref-key-1' }, +}; + +const deps = (overrides = {}) => { + const bumpGeneration = jest.fn(); + const secretsGet = jest.fn(async (ref) => `token-for-${ref}`); + return { + bumpGeneration, + secretsGet, + overrides: { + entryFor: () => KEY_ENTRY, + secrets: { get: secretsGet, put: jest.fn(), revoke: jest.fn() }, + row: { bumpGeneration, markClientMismatch: jest.fn() }, + now: () => new Date('2026-10-31T06:00:00.000Z'), + ...overrides, + }, + }; +}; + +describe('credentialFor on an operator-key entry', () => { + it('serves the stored key without touching the fence or the vendor', async () => { + const d = deps(); + const out = await credentialFor(ROW, d.overrides); + expect(out).toEqual({ token: 'token-for-ref-key-1', expiresAt: '' }); + expect(d.secretsGet).toHaveBeenCalledWith('ref-key-1'); + expect(d.bumpGeneration).not.toHaveBeenCalled(); + }); + + it('serves the key whatever its lifetime says, because lifetime is not its rule', async () => { + const d = deps(); + const expired = { ...ROW, config: { ...ROW.config, expiresAt: new Date('2000-01-01T00:00:00.000Z') } }; + await expect(credentialFor(expired, d.overrides)).resolves.toMatchObject({ token: 'token-for-ref-key-1' }); + }); + + it('refuses with reconnect_required when no key is stored', async () => { + const d = deps(); + const noKey = { ...ROW, config: { entryId: 'latchkey', agentId: 'a1' } }; + await expect(credentialFor(noKey, d.overrides)).rejects.toMatchObject({ + code: 'reconnect_required', + }); + await expect(credentialFor(noKey, d.overrides)).rejects.toBeInstanceOf(HostedMcpCredentialError); + expect(d.secretsGet).not.toHaveBeenCalled(); + }); + + it('refuses a removed connection before it reads the key', async () => { + const d = deps(); + await expect(credentialFor({ ...ROW, revokedAt: new Date() }, d.overrides)).rejects.toMatchObject({ + code: 'connection_mismatch', + }); + expect(d.secretsGet).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/__tests__/unit/services/hostedMcpOperatorKey.test.js b/backend/__tests__/unit/services/hostedMcpOperatorKey.test.js new file mode 100644 index 000000000..f8a960cb6 --- /dev/null +++ b/backend/__tests__/unit/services/hostedMcpOperatorKey.test.js @@ -0,0 +1,38 @@ +/** + * The operator-key mint binding (TASK-330, Vera's read §2): a key is one + * agent's, so a grant on its row may name that agent alone. Pure function, so + * these arms need no database. + */ +const { operatorKeyAudienceRefusal } = require('../../../services/hostedMcpOperatorKey'); + +const KEY_ENTRY = { credentialMode: 'operator_key' }; +const OAUTH_ENTRY = { credentialMode: undefined }; +const bound = (agentId) => ({ config: { agentId } }); + +describe('operatorKeyAudienceRefusal', () => { + it('leaves OAuth rows alone, so the member default still applies to them', () => { + expect(operatorKeyAudienceRefusal(OAUTH_ENTRY, bound('a1'), undefined)).toBeNull(); + expect(operatorKeyAudienceRefusal(undefined, bound('a1'), undefined)).toBeNull(); + }); + + it('names exactly the bound agent', () => { + expect(operatorKeyAudienceRefusal(KEY_ENTRY, bound('a1'), ['a1'])).toBeNull(); + }); + + it('refuses an omitted audience, which would otherwise default to every member', () => { + expect(operatorKeyAudienceRefusal(KEY_ENTRY, bound('a1'), undefined)).toMatch(/omitted audience/); + }); + + it('refuses another agent, and refuses the bound agent alongside another', () => { + expect(operatorKeyAudienceRefusal(KEY_ENTRY, bound('a1'), ['a2'])).toBe('audience must be exactly [a1]'); + expect(operatorKeyAudienceRefusal(KEY_ENTRY, bound('a1'), ['a1', 'a2'])).toBe('audience must be exactly [a1]'); + }); + + it('refuses an empty audience, which names nobody', () => { + expect(operatorKeyAudienceRefusal(KEY_ENTRY, bound('a1'), [])).toBe('audience must be exactly [a1]'); + }); + + it('refuses a row with no bound agent, since the key has no owner to name', () => { + expect(operatorKeyAudienceRefusal(KEY_ENTRY, { config: {} }, ['a1'])).toBe('this key is not bound to an agent'); + }); +}); diff --git a/backend/__tests__/unit/services/hostedMcpOperatorKeyIntake.test.js b/backend/__tests__/unit/services/hostedMcpOperatorKeyIntake.test.js new file mode 100644 index 000000000..04c4b5df1 --- /dev/null +++ b/backend/__tests__/unit/services/hostedMcpOperatorKeyIntake.test.js @@ -0,0 +1,85 @@ +/** + * Operator-key intake (TASK-330, Vera §4-§5): a pasted key is proved against + * the vendor before its row can go connected, a forbidden scope is refused, + * and the key never comes back in any result. The vendor client is injected, + * so these arms need no network. + */ +const { RoomGrantError } = require('../../../services/roomGrantService'); +const { + validateOperatorKey, + refusedOperatorKeyScopes, + OPERATOR_KEY_REFUSED_SCOPES, +} = require('../../../services/hostedMcpOperatorKeyIntake'); + +const KEY_ENTRY = { id: 'latchkey', credentialMode: 'operator_key', resource: 'https://latchkey.dev/mcp' }; +const SECRET = 'lk_live_THE_SECRET_VALUE'; +const TOOLS = [{ name: 'list_jobs' }]; +const fetcher = jest.fn(); + +const run = (overrides = {}) => validateOperatorKey({ + entry: KEY_ENTRY, + key: SECRET, + reportedScopes: ['read', 'jobs:run', 'dispatch'], + fetcher, + listTools: jest.fn(async () => TOOLS), + ...overrides, +}); + +describe('refusedOperatorKeyScopes', () => { + it('names exactly the three scopes a key may never carry', () => { + expect([...OPERATOR_KEY_REFUSED_SCOPES].sort()).toEqual(['jobs:approve', 'migration:apply', 'secrets:write']); + }); + + it('returns only the forbidden ones, and nothing for the walk scopes', () => { + expect(refusedOperatorKeyScopes(['read', 'jobs:run', 'dispatch'])).toEqual([]); + expect(refusedOperatorKeyScopes(['read', 'secrets:write', 'jobs:approve'])).toEqual(['secrets:write', 'jobs:approve']); + }); +}); + +describe('validateOperatorKey', () => { + it('accepts a key whose probe lists tools, and reports the read check', async () => { + const out = await run(); + expect(out).toEqual({ ok: true, tools: TOOLS, scopeCheck: 'read' }); + }); + + it('marks the check as a self-report when the vendor cannot report scopes', async () => { + const out = await run({ reportedScopes: null }); + expect(out).toMatchObject({ ok: true, scopeCheck: 'self_report' }); + }); + + it('refuses a forbidden scope before it probes the vendor', async () => { + const listTools = jest.fn(); + const out = await run({ reportedScopes: ['read', 'secrets:write'], listTools }); + expect(out).toMatchObject({ ok: false, code: 'scope_refused' }); + expect(out.message).toContain('secrets:write'); + expect(listTools).not.toHaveBeenCalled(); + }); + + it('refuses a missing key and a non-key entry', async () => { + await expect(run({ key: ' ' })).resolves.toMatchObject({ ok: false, code: 'key_missing' }); + await expect(run({ entry: { id: 'linear', resource: 'https://x' } })) + .resolves.toMatchObject({ ok: false, code: 'not_operator_key' }); + }); + + it('maps a vendor rejection to fixed copy', async () => { + const listTools = jest.fn(async () => { + throw new RoomGrantError('credential_rejected', 'the vendor rejected this connection’s credential', 403); + }); + const out = await run({ listTools }); + expect(out).toEqual({ ok: false, code: 'vendor_rejected', message: 'the vendor rejected this key' }); + }); + + it('never returns the key, even when the vendor error repeats it', async () => { + const listTools = jest.fn(async () => { + throw new RoomGrantError('provider_error', `vendor said ${SECRET} is wrong`, 502); + }); + const out = await run({ listTools }); + expect(out).toMatchObject({ ok: false, code: 'vendor_unreachable' }); + expect(JSON.stringify(out)).not.toContain(SECRET); + }); + + it('never returns the key on success or refusal', async () => { + expect(JSON.stringify(await run())).not.toContain(SECRET); + expect(JSON.stringify(await run({ reportedScopes: ['jobs:approve'] }))).not.toContain(SECRET); + }); +}); diff --git a/backend/integrations/hostedMcp/entries.ts b/backend/integrations/hostedMcp/entries.ts index fe2db399b..3f2e9b85d 100644 --- a/backend/integrations/hostedMcp/entries.ts +++ b/backend/integrations/hostedMcp/entries.ts @@ -11,6 +11,7 @@ * verify the URL, transport, auth mode, token lifetime, refresh and revocation, * and their pinned read tools claim `readOnlyHint: true` (§3, §11). */ +import { isOAuthEntry } from '../../services/hostedMcpEntryService'; import type { HostedMcpEntry } from '../../services/hostedMcpEntryService'; import { AIRTABLE_ENTRY } from './airtable'; import { ATLASSIAN_ENTRY } from './atlassian'; @@ -170,7 +171,7 @@ export const assertHostedMcpEntries = ( if (reservedIds.has(entry.id)) { throw new Error(`hosted-mcp entry id collides with a builtin tool installable: ${entry.id}`); } - if (entry.authorizationParams !== undefined) { + if (isOAuthEntry(entry) && entry.authorizationParams !== undefined) { if (!entry.authorizationParams || typeof entry.authorizationParams !== 'object' || Array.isArray(entry.authorizationParams)) { throw new Error(`hosted-mcp entry authorization parameters are not an object: ${entry.id}`); diff --git a/backend/models/Integration.ts b/backend/models/Integration.ts index adb47d3c9..ad8e2ecb2 100644 --- a/backend/models/Integration.ts +++ b/backend/models/Integration.ts @@ -175,6 +175,8 @@ export interface IIntegration extends Document { // this half is what stops a typed write from needing a cast to state the // same thing — a cast being the same silence one layer up (#1976 gate). entryId?: string; + /** The agent an operator key is bound to (TASK-330). Server-owned; see the schema. */ + agentId?: string; intake?: 'oauth'; clientId?: string; providerSubject?: string; @@ -378,6 +380,11 @@ const IntegrationSchema = new Schema( return this.type === 'hosted-mcp'; }, }, + // The agent an operator key is bound to (TASK-330). Set server-side at + // paste intake and never by a body. Absent on every OAuth row, which is + // why the unique index below can treat it as one more key without + // splitting an OAuth person's single row. + agentId: { type: String, trim: true }, intake: String, // 'oauth' is the only intake for this type clientId: String, // id that minted this pair; DCR rows resolve this against held registrations providerSubject: String, // the authorization server's stable id for the account @@ -470,14 +477,20 @@ IntegrationSchema.index({ podId: 1, type: 1 }); IntegrationSchema.index({ status: 1 }); IntegrationSchema.index({ createdBy: 1 }); IntegrationSchema.index({ installationId: 1 }, { unique: true, sparse: true }); -// One `hosted-mcp` row per (person, catalogue entry). The page cannot say which -// row a grant uses if a person holds two for one vendor, and the two would go -// through removal separately (scope §2). Partial, not sparse: `config.entryId` -// is absent on every other type, and every other type has no such key to -// collide on. A second connect by the same person REUSES the row through the -// §10.3 fence rather than inserting one. +// One `hosted-mcp` row per (person, catalogue entry, bound agent). An operator +// key is one agent's bearer (TASK-330), so one person may hold one row per +// agent for the same vendor. OAuth rows have no `config.agentId`, index as +// null, and so still get one row per person per entry, as before. Partial, not +// sparse: `config.entryId` is absent on every other type. A second OAuth connect +// by the same person REUSES the row through the §10.3 fence rather than +// inserting one. +// +// This replaces `createdBy_1_config.entryId_1`, which would refuse a second +// agent's key with E11000. Mongo does not swap an index in place: the old one +// must be dropped by scripts/migrate-hosted-mcp-agent-index.js before the new +// key is used on a database that already holds hosted rows. IntegrationSchema.index( - { createdBy: 1, 'config.entryId': 1 }, + { createdBy: 1, 'config.entryId': 1, 'config.agentId': 1 }, { unique: true, partialFilterExpression: { type: 'hosted-mcp' } }, ); IntegrationSchema.index({ 'ingestTokens.tokenHash': 1 }); diff --git a/backend/routes/grants.ts b/backend/routes/grants.ts index fc22be280..cf29ea482 100644 --- a/backend/routes/grants.ts +++ b/backend/routes/grants.ts @@ -7,6 +7,7 @@ import { Types } from 'mongoose'; import Integration from '../models/Integration'; import { isHostedMcpEntryHidden } from '../integrations/hostedMcp/visibility'; import { HOSTED_MCP_ENTRIES, findHostedMcpEntry } from '../integrations/hostedMcp/entries'; +import { operatorKeyAudienceRefusal } from '../services/hostedMcpOperatorKey'; import Pod from '../models/Pod'; import RoomGrant from '../models/RoomGrant'; import ToolCall from '../models/ToolCall'; @@ -397,6 +398,16 @@ router.post('/', grantRateLimit, auth, async (req: AuthenticatedRequest, res: ex return res.status(400).json({ error: 'invalid_target', message: 'target.kind must be pod' }); } const members = await ensureTargetAccess(target, userId); + // An operator-key connection names exactly its bound agent. This runs before + // the member default below, which is what would otherwise hand the key to + // every member (TASK-330). + if (hosted) { + const boundEntry = findHostedMcpEntry(HOSTED_MCP_ENTRIES, String(connection.config?.entryId || '')); + const boundRefusal = operatorKeyAudienceRefusal(boundEntry, connection, body.audience); + if (boundRefusal) { + return res.status(400).json({ error: 'invalid_audience', message: boundRefusal }); + } + } // Pod grants default to the current member snapshot. const audience = body.audience === undefined ? members : body.audience; const audienceValues = Array.isArray(audience) ? audience.map(String) : audience; diff --git a/backend/routes/hostedMcpConnect.ts b/backend/routes/hostedMcpConnect.ts index 21a195804..0e0309a16 100644 --- a/backend/routes/hostedMcpConnect.ts +++ b/backend/routes/hostedMcpConnect.ts @@ -16,7 +16,7 @@ const express = require('express'); import type { Request, Response } from 'express'; import type { HostedMcpAuthorizationServer } from '../services/hostedMcpIntakeService'; -import type { HostedMcpEntry } from '../services/hostedMcpEntryService'; +import type { HostedMcpOAuthEntry } from '../services/hostedMcpEntryService'; import rateLimit from 'express-rate-limit'; // eslint-disable-next-line global-require const auth = require('../middleware/auth'); @@ -26,6 +26,7 @@ import { upstreamFetch } from '../services/upstreamFetch'; import { dcrClientForRow, ensureDcrClient, refuseDcrClient } from '../services/hostedMcpDcrRegistry'; import { forgetHostedMcpDrift } from '../services/hostedMcpToolDefinitions'; import { publicAppUrl } from '../utils/publicAppUrl'; +import { submitOperatorKey } from './hostedMcpOperatorKey'; // eslint-disable-next-line global-require const Integration = require('../models/Integration'); // eslint-disable-next-line global-require @@ -36,9 +37,18 @@ const { HOSTED_MCP_ACCESS_TOKEN, HOSTED_MCP_REFRESH_TOKEN } = require('../servic // eslint-disable-next-line global-require const { revokeConnectionGrants } = require('../services/roomGrantService'); // eslint-disable-next-line global-require -const { revokeTokenAtVendor } = require('../services/connectionRemovalService'); +const { + revokeTokenAtVendor, +}: typeof import('../services/connectionRemovalService') = require('../services/connectionRemovalService'); // eslint-disable-next-line global-require -const { HOSTED_MCP_ENTRIES, findHostedMcpEntry, hostedMcpRevokeTarget } = require('../integrations/hostedMcp/entries'); +// Typed requires: tsc checks this route against the real module shapes, so an +// OAuth-only field read on the HostedMcpEntry union fails the build. +const { + HOSTED_MCP_ENTRIES, findHostedMcpEntry, hostedMcpRevokeTarget, +}: typeof import('../integrations/hostedMcp/entries') = require('../integrations/hostedMcp/entries'); +const { + isOAuthEntry, +}: typeof import('../services/hostedMcpEntryService') = require('../services/hostedMcpEntryService'); const { isHostedMcpEntryHidden } = require('../integrations/hostedMcp/visibility'); // eslint-disable-next-line global-require const { @@ -56,7 +66,7 @@ const { hostedMcpNonceCookiePath, readCookie, resolvedHostedMcpClient, -} = require('../services/hostedMcpIntakeService'); +}: typeof import('../services/hostedMcpIntakeService') = require('../services/hostedMcpIntakeService'); type AuthedRequest = Request & { user?: { id?: string; role?: string } }; @@ -97,7 +107,7 @@ const parseScopeSet = (scope: string): Set => new Set( /** A refused grant is never stored; revoke its refresh token when the entry can. */ const revokeRefusedGrant = async ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, client: { clientId: string; clientSecret?: string }, tokens: TokenResponse, ): Promise => { @@ -217,7 +227,7 @@ router.get('/:entryId/client-metadata', clientMetadataRateLimit, (req: Request, // still accept as a client with nowhere to send a code. return res.status(404).json({ error: 'unknown_entry' }); } - if (entry.client !== 'cimd') { + if (!isOAuthEntry(entry) || entry.client !== 'cimd') { return res.status(404).json({ error: 'client_metadata_not_used' }); } return res.json(buildClientMetadataDocument(entry)); @@ -240,7 +250,8 @@ const startConnect = async (req: AuthedRequest, res: Response) => { return res.status(401).json({ error: 'authentication_required' }); } const entry = findHostedMcpEntry(HOSTED_MCP_ENTRIES, req.params.entryId); - if (!entry) { + // An operator-key entry connects through its own paste route, never this flow. + if (!entry || !isOAuthEntry(entry)) { return res.status(404).json({ error: 'unknown_entry' }); } if (isHostedMcpEntryHidden(entry.id, process.env, userId)) return res.status(404).json({ error: 'unknown_entry' }); @@ -343,6 +354,8 @@ const startConnect = async (req: AuthedRequest, res: Response) => { }); }; +router.post('/:entryId/operator-key', writeIntegrationsRateLimit, auth, submitOperatorKey); + router.post('/:entryId/start', writeIntegrationsRateLimit, auth, async (req: AuthedRequest, res: Response) => { try { await startConnect(req, res); @@ -379,7 +392,7 @@ interface CallbackFlow { const finishCallback = async (req: Request, res: Response, flow: CallbackFlow) => { const entry = findHostedMcpEntry(HOSTED_MCP_ENTRIES, req.params.entryId); - if (!entry) { + if (!entry || !isOAuthEntry(entry)) { return callbackRedirect(res, 'error', 'unknown_entry'); } const state = typeof req.query.state === 'string' ? req.query.state : ''; diff --git a/backend/routes/hostedMcpOperatorKey.ts b/backend/routes/hostedMcpOperatorKey.ts new file mode 100644 index 000000000..87f20d5d6 --- /dev/null +++ b/backend/routes/hostedMcpOperatorKey.ts @@ -0,0 +1,121 @@ +import type { Request, Response } from 'express'; +import { HOSTED_MCP_ENTRIES, findHostedMcpEntry } from '../integrations/hostedMcp/entries'; +import { isHostedMcpEntryHidden } from '../integrations/hostedMcp/visibility'; +import { refusedOperatorKeyScopes, validateOperatorKey } from '../services/hostedMcpOperatorKeyIntake'; +import { guardedFetch } from '../services/hostedMcpToolDefinitions'; +import { secretWriteGeneration } from '../services/hostedMcpSecretGeneration'; +import { ownsRuntimeIdentity } from '../services/runtimeIdentityOwnership'; + +const Integration = require('../models/Integration'); +const User = require('../models/User'); +const connectorSecrets = require('../services/connectorSecrets'); +const { HOSTED_MCP_ACCESS_TOKEN } = require('../services/connectorSecretKinds'); + +type OperatorKeyRequest = Request & { user?: { id?: string } }; + +/** + * POST /connect/hosted-mcp/:entryId/operator-key (TASK-330, Vera §5). + * + * Takes one agent's pasted key and makes it a connected row bound to that + * agent. The key is proven by a live tools/list before the row can go + * `connected`, is stored as a ConnectorSecret on the row (never on a grant), + * and is never echoed in a response or written to a log. + * + * `scopeCheck` is always `self_report` here. Only a vendor readback of the + * key's scopes may set `reportedScopes`, and no readback exists yet, so the + * operator's scope picks are checked for refused scopes and nothing more. + */ +export const submitOperatorKey = async (req: OperatorKeyRequest, res: Response) => { + const userId = req.user?.id; + if (!userId) return res.status(401).json({ error: 'unauthorized' }); + + const entryId = String(req.params.entryId || ''); + const entry = findHostedMcpEntry(HOSTED_MCP_ENTRIES, entryId); + if (!entry || isHostedMcpEntryHidden(entryId, process.env, userId)) { + return res.status(404).json({ error: 'unknown_entry' }); + } + + const body = (req.body || {}) as Record; + const agentId = typeof body.agentId === 'string' ? body.agentId.trim() : ''; + if (!/^[a-f\d]{24}$/i.test(agentId)) { + return res.status(400).json({ error: 'invalid_agent', message: 'agentId must name an agent' }); + } + const agent = await User.findOne({ _id: agentId, isBot: true }).select('_id botMetadata').lean(); + // The identity comes from the bot's own row, never the body, and is lowercased + // as credentials.ts does. Refused before the probe, so a refused caller's key + // never reaches the vendor. + const agentName = String(agent?.botMetadata?.agentName || '').toLowerCase(); + const instanceId = String(agent?.botMetadata?.instanceId || ''); + if (!agent || !agentName || !instanceId) { + return res.status(400).json({ error: 'invalid_agent', message: 'agentId must name an agent' }); + } + // The key is a credential the agent presents, so it takes the same sole-installer + // rule as a runtime token. Admins are not exempt. + const ownership = await ownsRuntimeIdentity(userId, agentName, instanceId); + if (!ownership.owned) { + return res.status(403).json({ + error: 'agent_not_owned', + failure: ownership.failure, + message: 'you must be the sole installer of this agent', + }); + } + + const picks = Array.isArray(body.scopePicks) ? body.scopePicks.map(String) : []; + const refused = refusedOperatorKeyScopes(picks); + if (refused.length > 0) { + return res.status(400).json({ + error: 'scope_refused', + message: `this key carries ${refused.join(', ')}; create a key without those scopes`, + }); + } + + const key = typeof body.key === 'string' ? body.key : ''; + const intake = await validateOperatorKey({ + entry, + key, + reportedScopes: null, + fetcher: guardedFetch, + }); + if (!intake.ok) { + const status = intake.code === 'vendor_unreachable' ? 502 : 400; + return res.status(status).json({ error: intake.code, message: intake.message }); + } + + // The row is written only after the probe passes. It is upserted pending, + // then the secret lands on it, then it goes connected: a failed write leaves + // a pending row that grants refuse, never a connected row with no key. + const row = await Integration.findOneAndUpdate( + { type: 'hosted-mcp', createdBy: userId, 'config.entryId': entry.id, 'config.agentId': agentId }, + { + $inc: { 'config.authGeneration': 1 }, + $set: { status: 'pending', 'config.intake': 'operator_key' }, + $setOnInsert: { + type: 'hosted-mcp', + scope: 'user', + createdBy: userId, + 'config.entryId': entry.id, + 'config.agentId': agentId, + }, + }, + { upsert: true, new: true, setDefaultsOnInsert: true }, + ); + const writeGeneration = secretWriteGeneration(row.config?.authGeneration ?? 0, 0); + let credentialRef: string; + try { + credentialRef = await connectorSecrets.put(String(row._id), HOSTED_MCP_ACCESS_TOKEN, key, writeGeneration); + } catch { + // Fixed copy only: the store's error text stays out of the response. + return res.status(500).json({ error: 'key_store_failed', message: 'the key could not be stored; try again' }); + } + await Integration.updateOne( + { _id: row._id }, + { $set: { status: 'connected', 'config.credentialRef': credentialRef } }, + ); + + return res.status(201).json({ + integrationId: String(row._id), + status: 'connected', + scopeCheck: intake.scopeCheck, + toolCount: intake.tools.length, + }); +}; diff --git a/backend/scripts/backfill-hosted-mcp-client-id.ts b/backend/scripts/backfill-hosted-mcp-client-id.ts index a60365be9..ae6eef676 100644 --- a/backend/scripts/backfill-hosted-mcp-client-id.ts +++ b/backend/scripts/backfill-hosted-mcp-client-id.ts @@ -3,6 +3,7 @@ import mongoose from 'mongoose'; import Integration from '../models/Integration'; import { HOSTED_MCP_ENTRIES } from '../integrations/hostedMcp/entries'; +import { isOAuthEntry } from '../services/hostedMcpEntryService'; import { resolvedClientId } from '../services/hostedMcpIntakeService'; import { backfillHostedMcpClientIds, @@ -36,7 +37,7 @@ const main = async (): Promise => { .select('_id config.entryId config.clientId config.credentialRef config.refreshTokenRef') .lean(); - const entries = new Map(HOSTED_MCP_ENTRIES.map((entry) => [entry.id, entry])); + const entries = new Map(HOSTED_MCP_ENTRIES.filter(isOAuthEntry).map((entry) => [entry.id, entry])); const result = await backfillHostedMcpClientIds({ findLegacyRows: async () => rows as unknown as HostedMcpClientIdBackfillRow[], entryFor: (entryId) => (LEGACY_ENTRY_IDS.has(entryId) ? entries.get(entryId) : undefined), diff --git a/backend/scripts/migrate-hosted-mcp-agent-index.js b/backend/scripts/migrate-hosted-mcp-agent-index.js new file mode 100644 index 000000000..f22ae67a7 --- /dev/null +++ b/backend/scripts/migrate-hosted-mcp-agent-index.js @@ -0,0 +1,80 @@ +/* eslint-disable no-console, import/extensions, import/no-unresolved */ +/** + * TASK-330 rollout step. The hosted-mcp unique index moves from + * `(createdBy, config.entryId)` to `(createdBy, config.entryId, config.agentId)` + * so one person may hold one operator key per agent. Mongo does not replace an + * index in place, so the old one must go first, or the second agent's key + * still fails with E11000. + * + * Read-only by default. --apply builds the declared index first, then drops the + * old one. The order matters: dropping first would leave the collection with no + * uniqueness at all if the build then failed. It deletes no rows, so it cannot + * change which person holds which connection. If the old index is still present + * after --apply, stop: the new key is not safe to use yet. + * + * Run from the compiled output, like the other backend scripts: + * cd backend && node dist/scripts/migrate-hosted-mcp-agent-index.js # dry run + * cd backend && node dist/scripts/migrate-hosted-mcp-agent-index.js --apply + */ +const mongoose = require('mongoose'); + +const Integration = require('../models/Integration'); + +const OLD_INDEX = 'createdBy_1_config.entryId_1'; +const declared = Integration.schema.indexes().find(([keys, options]) => ( + keys.createdBy === 1 && keys['config.agentId'] === 1 && options.unique && options.partialFilterExpression +)); +if (!declared) throw new Error('the agent-scoped hosted-mcp index is not declared on Integration'); +const [keys, options] = declared; + +async function listIndexes() { + return Integration.collection.indexes().catch((error) => { + if (error.code === 26) return []; + throw error; + }); +} + +async function run({ apply }) { + const before = await listIndexes(); + const oldPresent = before.some((index) => index.name === OLD_INDEX); + const newPresent = before.some((index) => index.name === options.name + || JSON.stringify(index.key) === JSON.stringify(keys)); + console.log(JSON.stringify({ oldIndexPresent: oldPresent, newIndexPresent: newPresent, apply })); + if (!apply) return; + + // Build first: if the build fails, the old index still enforces uniqueness. + if (!newPresent) { + await Integration.collection.createIndex(keys, { ...options }); + console.log('created the agent-scoped hosted-mcp index'); + } + if (oldPresent) { + await Integration.collection.dropIndex(OLD_INDEX); + console.log(`dropped ${OLD_INDEX}`); + } + const after = await listIndexes(); + const stillOld = after.some((index) => index.name === OLD_INDEX); + if (stillOld) throw new Error(`${OLD_INDEX} is still present; do not issue operator keys`); + console.log('ok: old index gone, agent-scoped index present'); +} + +if (require.main === module) { + const apply = process.argv.includes('--apply'); + const uri = process.env.MONGO_URI; + if (!uri) { + console.error('MONGO_URI is required'); + process.exit(1); + } + // Only the CLI turns autoIndex off: a module-level set would leak into any + // process that requires this file, the tests included. + mongoose.set('autoIndex', false); + mongoose.connect(uri) + .then(() => run({ apply })) + .then(() => mongoose.disconnect()) + .catch(async (error) => { + console.error(error.message); + await mongoose.disconnect(); + process.exit(1); + }); +} + +module.exports = { run, OLD_INDEX }; diff --git a/backend/services/connectionRemovalService.ts b/backend/services/connectionRemovalService.ts index 5244396bc..dd8bae892 100644 --- a/backend/services/connectionRemovalService.ts +++ b/backend/services/connectionRemovalService.ts @@ -49,7 +49,8 @@ import { findHostedMcpEntry, hostedMcpRevokeTarget, } from '../integrations/hostedMcp/entries'; -import type { HostedMcpEntry } from './hostedMcpEntryService'; +import { isOAuthEntry } from './hostedMcpEntryService'; +import type { HostedMcpEntry, HostedMcpOAuthEntry } from './hostedMcpEntryService'; import { buildRevocationRequest, discoverAuthorizationServer, @@ -103,12 +104,12 @@ export interface RemovalDeps { /** Step 3b, between the provider revoke and any material going. Idempotent. */ markProviderRevoked: (id: unknown, at: Date) => Promise; entryFor: (connection: RemovableConnection) => HostedMcpEntry | null; - clientIdFor: (entry: HostedMcpEntry) => string; - clientSecretFor: (entry: HostedMcpEntry) => string | undefined; + clientIdFor: (entry: HostedMcpOAuthEntry) => string; + clientSecretFor: (entry: HostedMcpOAuthEntry) => string | undefined; dcrClientForRow: typeof dcrClientForRow; /** Step 3. Resolves when the authorization is gone; throws when it may still be live. */ revokeAtVendor: (input: { - entry: HostedMcpEntry; + entry: HostedMcpOAuthEntry; clientId: string; clientSecret?: string; /** A DCR confidential client's registered method (TASK-248); sent as recorded. */ @@ -177,7 +178,7 @@ export type RemovalResult = * endpoint that refused. */ export const revokeTokenAtVendor = async (input: { - entry: HostedMcpEntry; + entry: HostedMcpOAuthEntry; clientId: string; clientSecret?: string; tokenEndpointAuthMethod?: HostedMcpClientConfiguration['tokenEndpointAuthMethod']; @@ -342,7 +343,9 @@ export const removeConnection = async (options: { const alreadyRevokedAtVendor = Boolean(connection?.config?.[PROVIDER_REVOKED_MARK]); let providerRevoked = alreadyRevokedAtVendor; - if (entry && target?.endpoint && !alreadyRevokedAtVendor) { + // Only OAuth entries have a vendor revoke. An operator key is a static bearer + // with no refresh token, so it is never sent to an RFC 7009 endpoint. + if (entry && isOAuthEntry(entry) && target?.endpoint && !alreadyRevokedAtVendor) { // The row records which client minted this pair; the instance's current // configuration remains the source for client credentials. A recorded id // that differs from the current one must never send a held token under the diff --git a/backend/services/hostedMcpClientIdBackfill.ts b/backend/services/hostedMcpClientIdBackfill.ts index f6230b3d9..dd1c55284 100644 --- a/backend/services/hostedMcpClientIdBackfill.ts +++ b/backend/services/hostedMcpClientIdBackfill.ts @@ -1,4 +1,4 @@ -import type { HostedMcpEntry } from './hostedMcpEntryService'; +import type { HostedMcpOAuthEntry } from './hostedMcpEntryService'; export interface HostedMcpClientIdBackfillRow { _id: unknown; @@ -12,8 +12,9 @@ export interface HostedMcpClientIdBackfillRow { export interface HostedMcpClientIdBackfillDeps { findLegacyRows: () => Promise; - entryFor: (entryId: string) => HostedMcpEntry | undefined; - clientIdFor: (entry: HostedMcpEntry) => string; + /** Legacy rows hold a token pair, so only OAuth entries can be backfilled. */ + entryFor: (entryId: string) => HostedMcpOAuthEntry | undefined; + clientIdFor: (entry: HostedMcpOAuthEntry) => string; /** Must update only while `config.clientId` is still absent or blank. */ updateIfMissing: (row: HostedMcpClientIdBackfillRow, clientId: string) => Promise; } diff --git a/backend/services/hostedMcpCredentialService.ts b/backend/services/hostedMcpCredentialService.ts index 6b53e42b5..3b81a23f9 100644 --- a/backend/services/hostedMcpCredentialService.ts +++ b/backend/services/hostedMcpCredentialService.ts @@ -48,7 +48,7 @@ import Integration from '../models/Integration'; import { HOSTED_MCP_ENTRIES, findHostedMcpEntry } from '../integrations/hostedMcp/entries'; -import type { HostedMcpEntry } from './hostedMcpEntryService'; +import type { HostedMcpEntry, HostedMcpOAuthEntry } from './hostedMcpEntryService'; import { buildRefreshRequest, discoverAuthorizationServer, @@ -116,13 +116,13 @@ interface RefreshResult { interface CredentialDeps { now: () => Date; entryFor: (row: HostedMcpRow) => HostedMcpEntry; - clientIdFor: (entry: HostedMcpEntry) => string; - clientSecretFor: (entry: HostedMcpEntry) => string | undefined; + clientIdFor: (entry: HostedMcpOAuthEntry) => string; + clientSecretFor: (entry: HostedMcpOAuthEntry) => string | undefined; dcrClientForRow: typeof dcrClientForRow; /** Conditional on the client-id snapshot, so a concurrent reconnect wins. */ markClientMismatch: (id: unknown, recordedClientId: string | undefined, message: string) => Promise; refreshAtVendor: (input: { - entry: HostedMcpEntry; + entry: HostedMcpOAuthEntry; clientId: string; clientSecret?: string; /** A DCR confidential client's registered method (TASK-248); sent as recorded. */ @@ -396,6 +396,17 @@ export const credentialFor = async ( const id = rowId(row); const entry = deps.entryFor(row); + if (entry.credentialMode === 'operator_key') { + // A pasted key does not expire by a lifetime and cannot be refreshed, so + // the mode decides here, never a missing `expiresAt` (which would make an + // OAuth row with a lost lifetime serve a dead token). No fence, no lease. + assertUsable(row); + const keyRef = row.config?.credentialRef; + if (!keyRef) { + throw new HostedMcpCredentialError('reconnect_required', 'no key is stored for this connection'); + } + return { token: await deps.secrets.get(keyRef), expiresAt: '' }; + } const recordedClientId = row.config?.clientId; let clientId: string; let clientSecret: string | undefined; diff --git a/backend/services/hostedMcpDcrRegistry.ts b/backend/services/hostedMcpDcrRegistry.ts index 9ee39d64b..3a59ee38f 100644 --- a/backend/services/hostedMcpDcrRegistry.ts +++ b/backend/services/hostedMcpDcrRegistry.ts @@ -14,7 +14,7 @@ */ import { Types } from 'mongoose'; import HostedMcpDcrRegistration, { IHostedMcpDcrRegistration } from '../models/HostedMcpDcrRegistration'; -import type { HostedMcpEntry } from './hostedMcpEntryService'; +import type { HostedMcpOAuthEntry } from './hostedMcpEntryService'; import { CONFIDENTIAL_CLIENT_METHODS, HostedMcpClientError, @@ -37,7 +37,7 @@ type RegistrationMethod = 'none' | ConfidentialMethod; */ export const CLIENT_SECRET_RENEW_MARGIN_MS = 24 * 60 * 60 * 1000; -const currentFilter = (entry: HostedMcpEntry) => ({ +const currentFilter = (entry: HostedMcpOAuthEntry) => ({ entryId: entry.id, issuer: entry.issuer, current: true, @@ -69,7 +69,7 @@ const clientFromRow = async (row: RegistrationRow): Promise { +const registrationMethod = (entry: HostedMcpOAuthEntry, 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. @@ -90,7 +90,7 @@ const registrationMethod = (entry: HostedMcpEntry, metadata: HostedMcpAuthorizat * client for new starts; rows already minted by it continue to use it. */ export const ensureDcrClient = async ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, metadata: HostedMcpAuthorizationServer, redirectUri: string, fetchImpl: typeof fetch = hostedVendorFetch, @@ -249,7 +249,7 @@ export const ensureDcrClient = async ( /** A row may use a retired registration, but never one the vendor refused. */ export const dcrClientForRow = async ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, recordedClientId: string | undefined, ): Promise => { if (entry.client !== 'dcr' || !recordedClientId) return null; @@ -263,7 +263,7 @@ export const dcrClientForRow = async ( }; /** Invalid client on token/revoke poisons this registration, not every row. */ -export const refuseDcrClient = async (entry: HostedMcpEntry, clientId: string): Promise => { +export const refuseDcrClient = async (entry: HostedMcpOAuthEntry, clientId: string): Promise => { if (entry.client !== 'dcr') return; const now = new Date(); const refused = await HostedMcpDcrRegistration.findOneAndUpdate( diff --git a/backend/services/hostedMcpEntryService.ts b/backend/services/hostedMcpEntryService.ts index b30892aa1..eeefe2a56 100644 --- a/backend/services/hostedMcpEntryService.ts +++ b/backend/services/hostedMcpEntryService.ts @@ -61,7 +61,8 @@ export interface HostedMcpPinnedTool { annotations?: HostedMcpAnnotations; } -export interface HostedMcpEntry { +/** Fields every hosted entry carries, whatever its credential. */ +export interface HostedMcpEntryCommon { /** Namespaces every tool name (`.`) and names the row's app. */ id: string; title: string; @@ -77,17 +78,6 @@ export interface HostedMcpEntry { resource: string; /** The PRM resource identifier when it differs from the tools/call endpoint. */ audience?: string; - /** The authorization server the protected-resource metadata names. */ - issuer: string; - client: 'pre-registered' | 'cimd' | 'dcr'; - /** Entry-pinned exception when an AS omits its token auth-method list. */ - tokenEndpointAuthMethod?: 'client_secret_basic' | 'client_secret_post'; - scopes: string[]; - /** Only the vendor-specific authorization parameters supported by this code. */ - authorizationParams?: { - access_type?: string; - prompt?: string; - }; /** * Where the authorization dies, naming its kind because the URL cannot (§3). * Exactly one key: an `endpoint` is the RFC 7009 revocation endpoint the AS's @@ -105,6 +95,38 @@ export interface HostedMcpEntry { tools: HostedMcpPinnedTool[]; } +/** + * OAuth entries: every shipped one. The authorization server and the client + * shape belong to the OAuth flow, so they live only on this member. + */ +export interface HostedMcpOAuthEntry extends HostedMcpEntryCommon { + credentialMode?: 'oauth'; + /** The authorization server the protected-resource metadata names. */ + issuer: string; + client: 'pre-registered' | 'cimd' | 'dcr'; + /** Entry-pinned exception when an AS omits its token auth-method list. */ + tokenEndpointAuthMethod?: 'client_secret_basic' | 'client_secret_post'; + scopes: string[]; + /** Only the vendor-specific authorization parameters supported by this code. */ + authorizationParams?: { + access_type?: string; + prompt?: string; + }; +} + +/** + * Operator-key entries (TASK-330): a static bearer the operator pastes for one + * agent. It has no refresh, so `credentialFor` serves it without the freshness + * rule or the fence. No issuer, client or scopes exist for it, so those fields + * are absent from this member: reading one off the union is a compile error, + * which is the census check. + */ +export interface HostedMcpOperatorKeyEntry extends HostedMcpEntryCommon { + credentialMode: 'operator_key'; +} + +export type HostedMcpEntry = HostedMcpOAuthEntry | HostedMcpOperatorKeyEntry; + /** One tool as the vendor's `tools/list` reports it. */ export interface HostedMcpUpstreamTool { name: string; @@ -461,3 +483,8 @@ export const createHostedMcpDriftCache = (options: HostedMcpDriftCacheOptions): }, }; }; + +/** True for the OAuth members of the union. The OAuth flow and its readers narrow with this, not a cast. */ +export const isOAuthEntry = (entry: HostedMcpEntry): entry is HostedMcpOAuthEntry => ( + entry.credentialMode !== 'operator_key' +); diff --git a/backend/services/hostedMcpIntakeService.ts b/backend/services/hostedMcpIntakeService.ts index fac8e1ad6..3011755ec 100644 --- a/backend/services/hostedMcpIntakeService.ts +++ b/backend/services/hostedMcpIntakeService.ts @@ -24,7 +24,8 @@ * mix-up (§4). */ import { createHash, randomBytes, timingSafeEqual } from 'crypto'; -import type { HostedMcpEntry } from './hostedMcpEntryService'; +import { isOAuthEntry } from './hostedMcpEntryService'; +import type { HostedMcpEntry, HostedMcpOAuthEntry } from './hostedMcpEntryService'; import { upstreamFetch } from './upstreamFetch'; import { hostedVendorFetch } from './hostedVendorFetch'; @@ -106,7 +107,7 @@ export interface HostedMcpClientMetadataDocument { } export const buildClientMetadataDocument = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, apiBase: string = hostedMcpApiBase(), ): HostedMcpClientMetadataDocument => { const clientId = entryPath(apiBase, entry.id, CLIENT_METADATA_LEAF); @@ -173,7 +174,7 @@ export const hostedMcpClientConfigKeys = (entryId: string) => { * the authorization boundary, which is where it should be loud. */ export const resolvedClientId = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, env: NodeJS.ProcessEnv = process.env, ): string => { if (entry.client === 'cimd') return hostedMcpClientMetadataUrl(entry.id); @@ -196,7 +197,7 @@ export const resolvedClientId = ( /** A client secret exists only for the pre-registered confidential-client kind. */ export const resolvedClientSecret = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, env: NodeJS.ProcessEnv = process.env, ): string | undefined => { if (entry.client !== 'pre-registered') return undefined; @@ -213,7 +214,7 @@ export const resolvedClientSecret = ( /** Resolve all instance-held client material before offering or using the entry. */ export const resolvedHostedMcpClient = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, env: NodeJS.ProcessEnv = process.env, ): HostedMcpClientConfiguration => { const clientId = resolvedClientId(entry, env); @@ -225,6 +226,9 @@ export const isHostedMcpClientConfigured = ( entry: HostedMcpEntry, env: NodeJS.ProcessEnv = process.env, ): boolean => { + // An operator-key entry has no instance client: its key is per agent, and is + // proven at connect time, so there is nothing to preconfigure here. + if (!isOAuthEntry(entry)) return true; // DCR has no instance credential to preconfigure. Vendor support is checked // against discovered metadata on the first start, before any pending row. if (entry.client === 'dcr') return true; @@ -348,7 +352,7 @@ export const discoverAuthorizationServer = async ( * one function pair rather than by two call sites that could drift. */ export const buildAuthorizeUrl = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, authorizationEndpoint: string, params: { clientId: string; redirectUri: string; state: string; codeChallenge: string }, ): string => { @@ -421,7 +425,7 @@ const confidentialAuthentication = ( }; const endpointAuthentication = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, metadata: Partial, client: HostedMcpClientConfiguration, endpoint: HostedMcpEndpointKind, @@ -473,7 +477,7 @@ const endpointAuthentication = ( /** Authorization-code exchange: PKCE and resource survive every client kind. */ export const buildTokenExchangeRequest = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, metadata: Partial, client: HostedMcpClientConfiguration, params: { redirectUri: string; code: string; codeVerifier: string }, @@ -494,7 +498,7 @@ export const buildTokenExchangeRequest = ( /** Refresh uses the same client and endpoint-authentication rule as the exchange. */ export const buildRefreshRequest = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, metadata: Partial, client: HostedMcpClientConfiguration, params: { refreshToken: string }, @@ -513,7 +517,7 @@ export const buildRefreshRequest = ( /** RFC 7009 request: the same client must authenticate the revocation. */ export const buildRevocationRequest = ( - entry: HostedMcpEntry, + entry: HostedMcpOAuthEntry, metadata: Partial, client: HostedMcpClientConfiguration, params: { token: string; tokenTypeHint: 'refresh_token' | 'access_token' }, diff --git a/backend/services/hostedMcpOperatorKey.ts b/backend/services/hostedMcpOperatorKey.ts new file mode 100644 index 000000000..5c58b7cfc --- /dev/null +++ b/backend/services/hostedMcpOperatorKey.ts @@ -0,0 +1,29 @@ +import type { HostedMcpEntry } from './hostedMcpEntryService'; + +/** + * Why a grant on an operator-key connection may not name this audience, or + * null when it may (TASK-330, Vera's read §2). + * + * The key is one agent's bearer. The mint's default audience is every current + * pod member (grants.ts), so an omitted audience here would hand one key to + * all of them. Only the agent the row is bound to may be named, and only alone. + * This runs before the default is applied, so an omitted audience is refused + * rather than defaulted. + */ +export const operatorKeyAudienceRefusal = ( + entry: Pick | undefined, + connection: { config?: { agentId?: unknown } } | undefined, + audience: unknown, +): string | null => { + if (entry?.credentialMode !== 'operator_key') return null; + const agentId = String(connection?.config?.agentId || '').trim(); + if (!agentId) return 'this key is not bound to an agent'; + if (audience === undefined) { + return `name the bound agent in audience: an omitted audience reaches every pod member, ` + + `and this key is ${agentId}'s alone`; + } + if (!Array.isArray(audience) || audience.length !== 1 || String(audience[0]) !== agentId) { + return `audience must be exactly [${agentId}]`; + } + return null; +}; diff --git a/backend/services/hostedMcpOperatorKeyIntake.ts b/backend/services/hostedMcpOperatorKeyIntake.ts new file mode 100644 index 000000000..c9938273b --- /dev/null +++ b/backend/services/hostedMcpOperatorKeyIntake.ts @@ -0,0 +1,74 @@ +import type { HostedMcpEntry, HostedMcpUpstreamTool } from './hostedMcpEntryService'; +import { listHostedMcpTools } from './hostedMcpToolDefinitions'; + +/** + * Scopes an operator key may never carry (TASK-330 Decision 2). A key with any + * of them could approve its own jobs, rotate secrets or apply migrations, which + * is more authority than a seat should hold through a connector. + */ +export const OPERATOR_KEY_REFUSED_SCOPES = ['secrets:write', 'jobs:approve', 'migration:apply'] as const; + +export const refusedOperatorKeyScopes = (scopes: readonly string[]): string[] => ( + scopes.filter((scope) => (OPERATOR_KEY_REFUSED_SCOPES as readonly string[]).includes(scope)) +); + +export type OperatorKeyIntakeRefusal = + | { ok: false; code: 'not_operator_key' | 'key_missing' | 'scope_refused' | 'vendor_rejected' | 'vendor_unreachable'; message: string }; + +export type OperatorKeyIntakeResult = + | { ok: true; tools: HostedMcpUpstreamTool[]; scopeCheck: 'read' | 'self_report' } + | OperatorKeyIntakeRefusal; + +export interface OperatorKeyIntakeInput { + entry: HostedMcpEntry; + /** The pasted key. It is used for the probe and never returned, logged or echoed. */ + key: string; + /** + * The scopes Latchkey reports for this key, or null when it cannot report + * them. Null means the check is a self-report, which the caller must surface + * as such: a refused scope is then not a server guarantee (Vera §4). + */ + reportedScopes: readonly string[] | null; + fetcher: typeof fetch; + listTools?: typeof listHostedMcpTools; +} + +/** + * Prove a pasted key before its row can go `connected` (TASK-330, Vera §5). + * + * The probe is the same tools/list client the broker uses for calls, so a key + * that passes here is one the calls can use. Every refusal carries fixed copy: + * the vendor's own text and the key never leave this function, so a caller + * cannot relay the secret by mistake. + */ +export const validateOperatorKey = async ( + input: OperatorKeyIntakeInput, +): Promise => { + const { entry, key } = input; + if (entry.credentialMode !== 'operator_key') { + return { ok: false, code: 'not_operator_key', message: 'this connector does not take a pasted key' }; + } + const trimmed = typeof key === 'string' ? key.trim() : ''; + if (!trimmed) { + return { ok: false, code: 'key_missing', message: 'paste the key to continue' }; + } + const refused = input.reportedScopes ? refusedOperatorKeyScopes(input.reportedScopes) : []; + if (refused.length > 0) { + return { + ok: false, + code: 'scope_refused', + message: `this key carries ${refused.join(', ')}; create a key without those scopes`, + }; + } + const listTools = input.listTools ?? listHostedMcpTools; + try { + const tools = await listTools(entry, trimmed, input.fetcher); + return { ok: true, tools, scopeCheck: input.reportedScopes ? 'read' : 'self_report' }; + } catch (error) { + const status = (error as { code?: string })?.code; + if (status === 'credential_rejected') { + return { ok: false, code: 'vendor_rejected', message: 'the vendor rejected this key' }; + } + return { ok: false, code: 'vendor_unreachable', message: 'the vendor could not be reached to check this key' }; + } +}; diff --git a/backend/services/hostedMcpToolDefinitions.ts b/backend/services/hostedMcpToolDefinitions.ts index 223108525..e7335fab8 100644 --- a/backend/services/hostedMcpToolDefinitions.ts +++ b/backend/services/hostedMcpToolDefinitions.ts @@ -75,7 +75,7 @@ const messageOf = (error: unknown): string => ( // Loaded lazily, like the model and the credential service below: the frontend // glyph-catalogue test imports this module for its pure projection, and must // not pull the guard's node:dns and node:http transport in with it. -const guardedFetch: typeof fetch = (input, init) => { +export const guardedFetch: typeof fetch = (input, init) => { // eslint-disable-next-line global-require, @typescript-eslint/no-require-imports const { hostedVendorFetch } = require('./hostedVendorFetch'); return hostedVendorFetch(input, init); @@ -426,11 +426,25 @@ const listUpstreamTools = async ( const entry = findHostedMcpEntry(entries, String(row.config?.entryId || '')); if (!entry) throw new RoomGrantError('connection_mismatch', 'hosted-MCP entry is unavailable', 403); const { token } = await deps.credentialFor(row); + return listHostedMcpTools(entry, token, deps.fetcher); +}; + +/** + * The vendor's full tools/list for an entry under one bearer. Shared by the + * broker's drift check and by operator-key intake (TASK-330), which must prove + * a pasted key against the same client the calls will use. Errors are the + * broker's RoomGrantErrors, with any token already redacted from vendor text. + */ +export const listHostedMcpTools = async ( + entry: HostedMcpEntry, + token: string, + fetcher: typeof fetch, +): Promise => { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), HOSTED_MCP_CALL_TIMEOUT_MS); let session: Awaited> | undefined; try { - session = await openHostedMcpSession(entry.resource, token, deps.fetcher, controller.signal); + session = await openHostedMcpSession(entry.resource, token, fetcher, controller.signal); const allTools: HostedMcpUpstreamTool[] = []; const seenCursors = new Set(); let cursor: string | undefined; diff --git a/backend/services/installable/installableCatalogService.ts b/backend/services/installable/installableCatalogService.ts index 43a6541c9..cc31bc9d2 100644 --- a/backend/services/installable/installableCatalogService.ts +++ b/backend/services/installable/installableCatalogService.ts @@ -11,9 +11,13 @@ const { manifests } = require('../../integrations/manifests'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const { mcpComponentOf, projectTools, toolInstallableMetas } = require('./toolInstallables'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require -const { HOSTED_MCP_ENTRIES, findHostedMcpEntry } = require('../../integrations/hostedMcp/entries'); +const { + HOSTED_MCP_ENTRIES, findHostedMcpEntry, +}: typeof import('../../integrations/hostedMcp/entries') = require('../../integrations/hostedMcp/entries'); const { isHostedMcpEntryHidden } = require('../../integrations/hostedMcp/visibility'); -const { isHostedMcpClientConfigured } = require('../hostedMcpIntakeService'); +const { + isHostedMcpClientConfigured, +}: typeof import('../hostedMcpIntakeService') = require('../hostedMcpIntakeService'); type ProviderReadiness = { available: boolean; reason?: 'not_configured' }; diff --git a/backend/utils/serverOwnedConfigKeys.ts b/backend/utils/serverOwnedConfigKeys.ts index 976f9b89c..8b1beedf8 100644 --- a/backend/utils/serverOwnedConfigKeys.ts +++ b/backend/utils/serverOwnedConfigKeys.ts @@ -92,7 +92,7 @@ export const SERVER_OWNED_CONFIG_KEYS = [ // is a single-use nonce — a supplied one is a state the callback would accept // from whoever wrote it. `intake` and `credentialHint` are here for the same // reason at lower stakes: no browser decides how a credential is obtained. - 'entryId', 'intake', 'clientId', 'clientSecret', 'providerSubject', 'grantedScope', 'expiresAt', + 'entryId', 'agentId', 'intake', 'clientId', 'clientSecret', 'providerSubject', 'grantedScope', 'expiresAt', 'credentialRef', 'refreshTokenRef', 'refreshGeneration', 'authGeneration', 'credentialHint', 'pendingAuth', // The mark step 3b of the removal sequence records (`connectionRemovalService`) // after the provider revoke TAKES. A body that could write it would make a From 6020faa69c17b7224b7d8410969e5ccee3eafc38 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 02:12:15 -0800 Subject: [PATCH 2/7] fix(connectors): an operator key is spent only on its bound agent's turn, at call time (TASK-330) The mint holds an operator-key grant to exactly the row's agent, but the broker never read the binding: a grant from any other path, or one older than the mint rule, could spend agent A's key on agent B. resolveHosted- Connection now refuses connection_mismatch unless the grant's audience is exactly the row's agentId (a row marked operator_key with no agentId is refused too). Service test, real Mongo and ConnectorSecret store, real native hosted turns: only the bound granted seat is offered the tool; each seat reaches the vendor with its own key; a real leave empties the offer; a stray grant on A's row naming B is refused and A's key never leaves. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../hostedMcpOperatorKey.perAgent.test.js | 306 ++++++++++++++++++ backend/services/toolBrokerService.ts | 18 +- 2 files changed, 323 insertions(+), 1 deletion(-) create mode 100644 backend/__tests__/service/hostedMcpOperatorKey.perAgent.test.js diff --git a/backend/__tests__/service/hostedMcpOperatorKey.perAgent.test.js b/backend/__tests__/service/hostedMcpOperatorKey.perAgent.test.js new file mode 100644 index 000000000..bacd9270c --- /dev/null +++ b/backend/__tests__/service/hostedMcpOperatorKey.perAgent.test.js @@ -0,0 +1,306 @@ +/** + * TASK-330: an operator key is ONE agent's bearer. These arms run the real + * broker, the real native hosted turn, the real credential service and the real + * ConnectorSecret store on a real Mongo; only the model (LiteLLM, via + * axios.post) and the vendor's network edge (hostedVendorFetch) are stubbed. + * + * 1. per-agent offer — only the seat the key is bound to and granted for is + * offered the vendor's tool, read off its own model call. + * 2. per-agent bearer — each seat's call reaches the vendor carrying its OWN + * key, and never the other seat's. + * 3. leave empties it — after the bound seat leaves the pod (the product's + * removeAgentFromPod), its run offers no vendor tool. + */ +const { setupMongoDb, closeMongoDb } = require('../utils/testUtils'); + +process.env.CONNECTOR_SECRET_KEYS = `opkey-test:${Buffer.alloc(32, 5).toString('base64')}`; +process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'opkey-test'; + +jest.mock('axios', () => ({ __esModule: true, default: { post: jest.fn() } })); +jest.mock('../../models/pg/Message', () => ({ findByPodId: jest.fn().mockResolvedValue([]) })); +jest.mock('../../services/agentTypingService', () => ({ + emitAgentTypingStart: jest.fn(), emitAgentTypingStop: jest.fn(), +})); +jest.mock('../../services/agentMessageService', () => ({ postMessage: jest.fn().mockResolvedValue({ success: true }) })); +jest.mock('../../services/agentComputerBroker', () => ({ agentComputerBroker: {}, agentComputerEnabled: false })); +// The ToolCall ledger lives in PostgreSQL, which this tier does not run; the +// same in-memory stand-in teammate-approval.test.js uses. It decides nothing: +// the budget always admits, and the rows are only read back as a trail. +const mockLedger = []; +jest.mock('../../models/ToolCall', () => ({ + create: jest.fn(async (row) => { mockLedger.push(row); }), + digestArgs: (args) => require('crypto').createHash('sha256').update(JSON.stringify(args || {})).digest('hex'), + reserveBudgetLineage: jest.fn(async () => true), +})); +// The vendor's network edge. Every hosted-MCP request (tools/list for drift, +// tools/call for the call) goes through this one module. +const mockVendorRequests = []; +jest.mock('../../services/hostedVendorFetch', () => ({ + hostedVendorFetch: (...args) => mockVendorServer(...args), + HOSTED_VENDOR_MAX_RESPONSE_BYTES: 4 * 1024 * 1024, +})); + +const User = require('../../models/User'); +const Pod = require('../../models/Pod'); +const RoomGrant = require('../../models/RoomGrant'); +const AgentRun = require('../../models/AgentRun'); +const { AgentInstallation } = require('../../models/AgentRegistry'); +const Integration = require('../../models/Integration'); +const ConnectorSecret = require('../../models/ConnectorSecret'); +const connectorSecrets = require('../../services/connectorSecrets'); +const { HOSTED_MCP_ACCESS_TOKEN } = require('../../services/connectorSecretKinds'); +const { secretWriteGeneration } = require('../../services/hostedMcpSecretGeneration'); +const { HOSTED_MCP_ENTRIES, assertHostedMcpEntries } = require('../../integrations/hostedMcp/entries'); +const { runAgent } = require('../../services/nativeRuntimeService'); +const { GRANT_BROKER_ID } = require('../../services/installable/toolInstallables'); +const AgentIdentityService = require('../../services/agentIdentityService'); +const axios = require('axios').default; + +jest.setTimeout(60000); + +const READ_TOOL = { + name: 'list_jobs', + upstreamName: 'list_jobs', + description: 'List the jobs this key can see', + class: 'read', + annotations: { readOnlyHint: true }, + inputSchema: { type: 'object', properties: { limit: { type: 'number' } } }, +}; +const WRITE_TOOL = { + name: 'cancel_job', + upstreamName: 'cancel_job', + description: 'Cancel a job', + class: 'write', + annotations: { readOnlyHint: false, destructiveHint: true }, + inputSchema: { type: 'object', properties: { jobId: { type: 'string' } }, required: ['jobId'] }, +}; +const ENTRY = { + id: 'keyvendor', + title: 'Key Vendor', + resource: 'https://mcp.keyvendor.example/mcp', + credentialMode: 'operator_key', + revoke: { page: 'https://keyvendor.example/settings/keys' }, + tools: [READ_TOOL, WRITE_TOOL], +}; +const READ_NAME = 'keyvendor.list_jobs'; +const READ_FUNCTION = 'keyvendor_list_jobs'; + +const json = (body, headers = {}) => new Response(JSON.stringify(body), { + status: 200, headers: { 'content-type': 'application/json', ...headers }, +}); +// A Streamable-HTTP MCP server that answers the four messages the broker sends +// and records the bearer on every request, whatever its method. +async function mockVendorServer(url, init = {}) { + const auth = new Headers(init.headers).get('authorization'); + if (init.method === 'DELETE') { + mockVendorRequests.push({ auth, method: 'DELETE' }); + return new Response(null, { status: 204 }); + } + if (init.method === 'GET') return new Response(null, { status: 405 }); + const body = JSON.parse(String(init.body)); + mockVendorRequests.push({ auth, method: body.method }); + if (body.method === 'initialize') { + return json({ jsonrpc: '2.0', id: body.id, result: { + protocolVersion: '2025-11-25', capabilities: { tools: {} }, serverInfo: { name: 'keyvendor', version: '1' }, + } }, { 'mcp-session-id': 'keyvendor-session' }); + } + if (body.method === 'notifications/initialized') return new Response(null, { status: 202 }); + if (body.method === 'tools/list') { + return json({ jsonrpc: '2.0', id: body.id, result: { tools: [READ_TOOL, WRITE_TOOL].map((tool) => ({ + name: tool.upstreamName, description: tool.description, inputSchema: tool.inputSchema, annotations: tool.annotations, + })) } }); + } + if (body.method === 'tools/call') { + // Name the account by label, not by echoing the key: the broker redacts a + // bearer it finds in vendor text, so an echoed key would read [redacted]. + const account = { 'Bearer key-A': 'account-A', 'Bearer key-B': 'account-B' }[auth] || 'unknown-account'; + return json({ jsonrpc: '2.0', id: body.id, result: { content: [{ type: 'text', text: `jobs of ${account}` }] } }); + } + return json({ jsonrpc: '2.0', id: body.id, error: { code: -32601, message: 'method not found' } }); +} + +let shipped; +beforeAll(async () => { + assertHostedMcpEntries([ENTRY]); // the fixture is one the catalogue would load + shipped = HOSTED_MCP_ENTRIES.splice(0, HOSTED_MCP_ENTRIES.length, ENTRY); + await setupMongoDb(); +}); +afterAll(async () => { + HOSTED_MCP_ENTRIES.splice(0, HOSTED_MCP_ENTRIES.length, ...shipped); + await closeMongoDb(); +}); + +let owner; +let seatA; +let seatB; +let pod; +let previousEnv; + +const makeSeat = (agentName) => User.create({ + username: AgentIdentityService.buildAgentUsername(agentName, 'default'), + email: `${agentName}@agents.commonly.local`, + password: 'test-password', + isBot: true, + botType: 'agent', + botMetadata: { agentName, instanceId: 'default' }, +}); + +const HOSTED_NATIVE = { runtime: { runtimeType: 'hosted' }, hosted: { engine: 'native', provisionedAt: new Date() } }; + +beforeEach(async () => { + previousEnv = { ...process.env }; + process.env.LITELLM_BASE_URL = 'http://litellm.test'; + process.env.LITELLM_MASTER_KEY = 'test-only'; + delete process.env.HOSTED_TURNS_PER_DAY; + await Promise.all([User.deleteMany({}), Pod.deleteMany({}), RoomGrant.deleteMany({}), AgentRun.deleteMany({}), + Integration.deleteMany({}), ConnectorSecret.deleteMany({}), AgentInstallation.deleteMany({})]); + mockLedger.length = 0; + mockVendorRequests.length = 0; + owner = await User.create({ username: 'key-owner', email: 'key-owner@example.test', password: 'test-password' }); + seatA = await makeSeat('seat-a'); + seatB = await makeSeat('seat-b'); + pod = await Pod.create({ name: 'Operator key pod', type: 'team', createdBy: owner._id, + members: [owner._id, seatA._id, seatB._id], publicRead: false, joinPolicy: 'invite-only' }); + // Both seats are installed hosted-native, so the pod passes the broker's + // outsider-pod eligibility check for real (outsiderGrantPodEligibility.ts). + await AgentInstallation.create(['seat-a', 'seat-b'].map((agentName) => ({ + agentName, instanceId: 'default', podId: pod._id, version: '1', installedBy: owner._id, + status: 'active', config: HOSTED_NATIVE, + }))); + axios.post.mockReset(); +}); +afterEach(() => { process.env = previousEnv; }); + +/** + * The row the paste-key route writes (routes/hostedMcpOperatorKey.ts): bound to + * one agent, its key in the real ConnectorSecret store, then `connected`. + */ +const connectKey = async (seat, key) => { + const row = await Integration.create({ + type: 'hosted-mcp', scope: 'user', status: 'pending', createdBy: owner._id, + config: { entryId: ENTRY.id, agentId: String(seat._id), intake: 'operator_key', authGeneration: 1 }, + }); + const credentialRef = await connectorSecrets.put( + String(row._id), HOSTED_MCP_ACCESS_TOKEN, key, secretWriteGeneration(1, 0), + ); + await Integration.updateOne({ _id: row._id }, { $set: { status: 'connected', 'config.credentialRef': credentialRef } }); + return row; +}; + +const grantFor = (seat, row) => RoomGrant.create({ + grantId: `keyvendor-grant-${seat.botMetadata.agentName}`, + connectionId: String(row._id), installationId: String(row._id), + target: { kind: 'pod', id: String(pod._id) }, + audience: [String(seat._id)], tools: [READ_NAME], writeMode: 'read', + expiresAt: new Date(Date.now() + 86400000), brokerId: GRANT_BROKER_ID, +}); + +const run = (seat) => runAgent({ podId: pod._id, agentName: seat.botMetadata.agentName, instanceId: 'default', + installedBy: owner._id, displayName: seat.botMetadata.agentName, + config: HOSTED_NATIVE, +}, { type: 'chat.mention', payload: { content: 'List my jobs.' } }); + +const finalReply = { data: { + choices: [{ message: { role: 'assistant', content: 'Done.' } }], + usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 }, +} }; +// The model calls the vendor's read tool when it is offered, then finishes. +const modelCallsReadToolWhenOffered = () => axios.post.mockImplementation(async (_url, body) => { + const offered = (body.tools || []).map((tool) => tool.function.name); + const answered = (body.messages || []).some((message) => message.role === 'tool'); + if (offered.includes(READ_FUNCTION) && !answered) { + return { data: { + choices: [{ message: { role: 'assistant', content: null, tool_calls: [{ + id: 'call-1', type: 'function', function: { name: READ_FUNCTION, arguments: '{"limit":5}' }, + }] } }], + usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 }, + } }; + } + return finalReply; +}); +/** Function names offered on the run's FIRST model request. */ +const offeredOnFirstCall = () => axios.post.mock.calls[0][1].tools.map((tool) => tool.function.name); +/** The tool message the run fed back to the model after its broker call. */ +const toolResultSent = () => { + const last = axios.post.mock.calls[axios.post.mock.calls.length - 1][1]; + return last.messages.filter((message) => message.role === 'tool').map((message) => message.content).join('\n'); +}; + +describe('operator-key hosted MCP is per agent (TASK-330)', () => { + test('arm 1 per-agent offer: only the bound, granted seat is offered the vendor tool', async () => { + const rowA = await connectKey(seatA, 'key-A'); + await grantFor(seatA, rowA); + axios.post.mockResolvedValue(finalReply); + + expect(await run(seatA)).toMatchObject({ status: 'succeeded' }); + expect(offeredOnFirstCall()).toContain(READ_FUNCTION); + // Reads only on a hosted turn: the write pin is never offered. + expect(offeredOnFirstCall()).not.toContain('keyvendor_cancel_job'); + + axios.post.mockClear(); + expect(await run(seatB)).toMatchObject({ status: 'succeeded' }); + expect(offeredOnFirstCall().filter((name) => name.startsWith('keyvendor'))).toEqual([]); + }); + + test('arm 2 per-agent bearer: each seat reaches the vendor with its own key, never the other', async () => { + const rowA = await connectKey(seatA, 'key-A'); + const rowB = await connectKey(seatB, 'key-B'); + await grantFor(seatA, rowA); + await grantFor(seatB, rowB); + modelCallsReadToolWhenOffered(); + + expect(await run(seatA)).toMatchObject({ status: 'succeeded' }); + expect(toolResultSent()).toContain('jobs of account-A'); + const asA = mockVendorRequests.splice(0); + + axios.post.mockClear(); + expect(await run(seatB)).toMatchObject({ status: 'succeeded' }); + expect(toolResultSent()).toContain('jobs of account-B'); + const asB = mockVendorRequests.splice(0); + + for (const [requests, own, other] of [[asA, 'Bearer key-A', 'Bearer key-B'], [asB, 'Bearer key-B', 'Bearer key-A']]) { + expect(requests.map((request) => request.method)).toEqual(expect.arrayContaining(['initialize', 'tools/call'])); + expect(requests.filter((request) => request.method === 'tools/call')).toHaveLength(1); + expect(requests.every((request) => request.auth === own)).toBe(true); + expect(requests.some((request) => request.auth === other)).toBe(false); + } + }); + + test('arm 3 leave empties the audience: after the bound seat leaves, its run offers no vendor tool', async () => { + const rowA = await connectKey(seatA, 'key-A'); + await grantFor(seatA, rowA); + axios.post.mockResolvedValue(finalReply); + + expect(await run(seatA)).toMatchObject({ status: 'succeeded' }); + expect(offeredOnFirstCall()).toContain(READ_FUNCTION); // the positive control for this arm + + await AgentIdentityService.removeAgentFromPod('seat-a', pod._id, 'default'); + const after = await Pod.findById(pod._id).select('members').lean(); + expect(after.members.map(String)).not.toContain(String(seatA._id)); + + axios.post.mockClear(); + mockVendorRequests.length = 0; + expect(await run(seatA)).toMatchObject({ status: 'succeeded' }); + expect(offeredOnFirstCall().filter((name) => name.startsWith('keyvendor'))).toEqual([]); + expect(mockVendorRequests).toEqual([]); + }); + + // The mint refuses this grant (grants.ts, operatorKeyAudienceRefusal), so it + // is written straight to the store: a grant from any other path, or one that + // predates the mint rule, must still never spend A's key on B's turn. + test('arm 4 call-time binding: a grant on A\'s row that names B is refused, and A\'s key never leaves', async () => { + const rowA = await connectKey(seatA, 'key-A'); + await RoomGrant.create({ + grantId: 'keyvendor-grant-stray', connectionId: String(rowA._id), installationId: String(rowA._id), + target: { kind: 'pod', id: String(pod._id) }, + audience: [String(seatB._id)], tools: [READ_NAME], writeMode: 'read', + expiresAt: new Date(Date.now() + 86400000), brokerId: GRANT_BROKER_ID, + }); + modelCallsReadToolWhenOffered(); + + expect(await run(seatB)).toMatchObject({ status: 'succeeded' }); + expect(toolResultSent()).not.toContain('account-A'); + expect(toolResultSent()).toContain('connection_mismatch'); + expect(mockVendorRequests.some((request) => request.auth === 'Bearer key-A')).toBe(false); + }); +}); diff --git a/backend/services/toolBrokerService.ts b/backend/services/toolBrokerService.ts index 6f3cd817e..32cfd7c9c 100644 --- a/backend/services/toolBrokerService.ts +++ b/backend/services/toolBrokerService.ts @@ -605,7 +605,7 @@ const resolveHostedConnection = async ( revokedAt?: Date | null; createdBy?: unknown; createdAt?: unknown; - config?: { entryId?: unknown }; + config?: { entryId?: unknown; agentId?: unknown; intake?: unknown }; } | null; if (!row || row.type !== 'hosted-mcp' || row.status !== 'connected' || row.revokedAt) { throw new RoomGrantError( @@ -625,6 +625,22 @@ const resolveHostedConnection = async ( 403, ); } + // An operator-key row is ONE agent's bearer (TASK-330). The mint holds its + // grants to exactly that agent; this holds every grant whatever made it, so a + // stray or older grant can never spend the key on another agent's turn. The + // caller is already inside the grant's audience (`assertGrantUsable`), so an + // audience of only the bound agent means the caller is that agent. + const boundAgentId = row.config?.agentId ? String(row.config.agentId) : ''; + if (boundAgentId || row.config?.intake === 'operator_key') { + const audience = ((grant as { audience?: unknown }).audience || []) as unknown[]; + if (!boundAgentId || audience.length === 0 || audience.some((id) => String(id) !== boundAgentId)) { + throw new RoomGrantError( + 'connection_mismatch', + 'this connection\'s key belongs to one agent, and the grant names another', + 403, + ); + } + } const grantCreatedAt = createdAtMs((grant as { createdAt?: unknown }).createdAt); const rowCreatedAt = createdAtMs(row.createdAt); if (grantCreatedAt === null || rowCreatedAt === null) { From 35440ffa9f8fb710bfab0e1295dbff5a09963c7c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 02:18:34 -0800 Subject: [PATCH 3/7] fix(connectors): a pasted key is stored as proven, and a re-paste ends the old key's grants (TASK-330) From the F07 review round (two Opus reviewers, both HOLD on the first): - The route trims the key once and stores that value. Before, the probe used the trimmed key while the raw paste was stored, so a trailing newline still authenticated (fetch strips header whitespace) but the exact-match result redaction no longer matched an echoed key. - A re-paste on an existing row revokes its grants and forgets its drift cache before the new key is stored: a grant was decided against the key connected then, and a pasted key cannot prove it is the same account. - A paste clears revokedAt, errorMessage and the provider-revoked mark a half-finished removal left. - The connect step is conditional on this paste's generation; if a removal deleted the row mid-paste, the just-written secret is revoked and the paste refused 409 (a newer paste's secret is left alone). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../routes/hostedMcpOperatorKey.route.test.js | 63 +++++++++++++++++-- backend/routes/hostedMcpOperatorKey.ts | 38 +++++++++-- 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js index 60b9fefbd..7c1a5fbc7 100644 --- a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js +++ b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js @@ -9,14 +9,16 @@ const express = require('express'); const request = require('supertest'); -jest.mock('../../../models/Integration', () => ({ findOneAndUpdate: jest.fn(), updateOne: jest.fn() })); +jest.mock('../../../models/Integration', () => ({ findOneAndUpdate: jest.fn(), updateOne: jest.fn(), exists: jest.fn() })); jest.mock('../../../models/User', () => ({ findOne: jest.fn() })); jest.mock('../../../models/AgentRegistry', () => ({ AgentInstallation: { find: jest.fn() } })); jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); -jest.mock('../../../services/connectorSecrets', () => ({ put: jest.fn() })); +jest.mock('../../../services/connectorSecrets', () => ({ put: jest.fn(), revoke: jest.fn() })); +jest.mock('../../../services/roomGrantService', () => ({ revokeConnectionGrants: jest.fn() })); jest.mock('../../../services/hostedMcpToolDefinitions', () => ({ guardedFetch: jest.fn(), listHostedMcpTools: jest.fn(), + forgetHostedMcpDrift: jest.fn(), })); const Integration = require('../../../models/Integration'); @@ -24,7 +26,8 @@ const User = require('../../../models/User'); const { AgentInstallation } = require('../../../models/AgentRegistry'); const Pod = require('../../../models/Pod'); const connectorSecrets = require('../../../services/connectorSecrets'); -const { listHostedMcpTools } = require('../../../services/hostedMcpToolDefinitions'); +const { listHostedMcpTools, forgetHostedMcpDrift } = require('../../../services/hostedMcpToolDefinitions'); +const { revokeConnectionGrants } = require('../../../services/roomGrantService'); const { HOSTED_MCP_ENTRIES } = require('../../../integrations/hostedMcp/entries'); const { submitOperatorKey } = require('../../../routes/hostedMcpOperatorKey'); @@ -86,7 +89,8 @@ beforeEach(() => { Pod.find.mockImplementation(() => ({ select: () => ({ lean: async () => dmPods }) })); listHostedMcpTools.mockResolvedValue([{ name: 'list_jobs' }]); Integration.findOneAndUpdate.mockResolvedValue({ _id: ROW_ID, config: { authGeneration: 1 } }); - Integration.updateOne.mockResolvedValue({ acknowledged: true }); + Integration.updateOne.mockResolvedValue({ acknowledged: true, matchedCount: 1 }); + Integration.exists.mockResolvedValue({ _id: ROW_ID }); connectorSecrets.put.mockResolvedValue('secret-ref-1'); }); afterEach(() => { @@ -198,9 +202,58 @@ describe('POST /connect/hosted-mcp/:entryId/operator-key', () => { ); expect(connectorSecrets.put).toHaveBeenCalledWith(ROW_ID, expect.anything(), KEY, expect.any(Number)); expect(Integration.updateOne).toHaveBeenCalledWith( - { _id: ROW_ID }, + { _id: ROW_ID, 'config.authGeneration': 1 }, { $set: { status: 'connected', 'config.credentialRef': 'secret-ref-1' } }, ); + // A first paste has no grants to end. + expect(revokeConnectionGrants).not.toHaveBeenCalled(); + }); + + // Both F07 reviewers: fetch strips header whitespace, so a key pasted with a + // newline authenticates while the stored copy no longer matches what the + // vendor echoes, and redaction (an exact match) lets it through. + test('a key pasted with surrounding whitespace is stored as the trimmed value the probe proved', async () => { + const res = await submit({ key: ` ${KEY}\n` }); + expect(res.status).toBe(201); + expect(listHostedMcpTools).toHaveBeenCalledWith(expect.anything(), KEY, expect.anything()); + expect(connectorSecrets.put).toHaveBeenCalledWith(ROW_ID, expect.anything(), KEY, expect.any(Number)); + }); + + // A grant was decided against the key that was connected then; a pasted key + // cannot prove it is the same account, so a re-paste ends them, as the OAuth + // callback does for a subject it cannot compare (hostedMcpConnect.ts). + test('a re-paste on an existing row revokes its grants and forgets its drift before storing the new key', async () => { + const existing = { _id: ROW_ID, config: { authGeneration: 2 } }; + Integration.findOneAndUpdate.mockResolvedValue(existing); + const res = await submit(); + expect(res.status).toBe(201); + expect(revokeConnectionGrants).toHaveBeenCalledWith({ connection: existing, revokedBy: OWNER }); + expect(forgetHostedMcpDrift).toHaveBeenCalledWith(ROW_ID); + expect(revokeConnectionGrants.mock.invocationCallOrder[0]) + .toBeLessThan(connectorSecrets.put.mock.invocationCallOrder[0]); + }); + + test('a paste clears the row\'s revocation, error and provider-revoked mark', async () => { + await submit(); + const [, update] = Integration.findOneAndUpdate.mock.calls[0]; + expect(update.$set).toEqual(expect.objectContaining({ revokedAt: null, errorMessage: null })); + expect(update.$unset).toEqual({ 'config.providerRevokedAt': '' }); + }); + + test('a row removed while the key was being saved: the new secret is revoked and the paste refused', async () => { + Integration.updateOne.mockResolvedValue({ acknowledged: true, matchedCount: 0 }); + Integration.exists.mockResolvedValue(null); + const res = await submit(); + expect(res.status).toBe(409); + expect(res.body).toEqual({ error: 'connection_changed', message: 'the connection changed while the key was saved; paste it again' }); + expect(connectorSecrets.revoke).toHaveBeenCalledWith('secret-ref-1'); + }); + + test('a newer paste that won the row keeps its secret: the older paste is refused without revoking it', async () => { + Integration.updateOne.mockResolvedValue({ acknowledged: true, matchedCount: 0 }); + const res = await submit(); + expect(res.status).toBe(409); + expect(connectorSecrets.revoke).not.toHaveBeenCalled(); }); test('the key appears in no response, no row write, and no thrown error text', async () => { diff --git a/backend/routes/hostedMcpOperatorKey.ts b/backend/routes/hostedMcpOperatorKey.ts index 87f20d5d6..160d87c2a 100644 --- a/backend/routes/hostedMcpOperatorKey.ts +++ b/backend/routes/hostedMcpOperatorKey.ts @@ -2,7 +2,7 @@ import type { Request, Response } from 'express'; import { HOSTED_MCP_ENTRIES, findHostedMcpEntry } from '../integrations/hostedMcp/entries'; import { isHostedMcpEntryHidden } from '../integrations/hostedMcp/visibility'; import { refusedOperatorKeyScopes, validateOperatorKey } from '../services/hostedMcpOperatorKeyIntake'; -import { guardedFetch } from '../services/hostedMcpToolDefinitions'; +import { forgetHostedMcpDrift, guardedFetch } from '../services/hostedMcpToolDefinitions'; import { secretWriteGeneration } from '../services/hostedMcpSecretGeneration'; import { ownsRuntimeIdentity } from '../services/runtimeIdentityOwnership'; @@ -10,6 +10,7 @@ const Integration = require('../models/Integration'); const User = require('../models/User'); const connectorSecrets = require('../services/connectorSecrets'); const { HOSTED_MCP_ACCESS_TOKEN } = require('../services/connectorSecretKinds'); +const { revokeConnectionGrants } = require('../services/roomGrantService'); type OperatorKeyRequest = Request & { user?: { id?: string } }; @@ -69,7 +70,10 @@ export const submitOperatorKey = async (req: OperatorKeyRequest, res: Response) }); } - const key = typeof body.key === 'string' ? body.key : ''; + // Trimmed once, here: fetch strips header whitespace, so a pasted newline + // would still authenticate while the stored copy no longer matched what a + // vendor echoes, and the result redaction is an exact match. + const key = typeof body.key === 'string' ? body.key.trim() : ''; const intake = await validateOperatorKey({ entry, key, @@ -88,7 +92,11 @@ export const submitOperatorKey = async (req: OperatorKeyRequest, res: Response) { type: 'hosted-mcp', createdBy: userId, 'config.entryId': entry.id, 'config.agentId': agentId }, { $inc: { 'config.authGeneration': 1 }, - $set: { status: 'pending', 'config.intake': 'operator_key' }, + // A paste is a fresh connection: whatever a half-finished removal left on + // the row (its revocation, error, provider-revoked mark) is not about + // this key, and a surviving mark would let the next removal skip a step. + $set: { status: 'pending', 'config.intake': 'operator_key', revokedAt: null, errorMessage: null }, + $unset: { 'config.providerRevokedAt': '' }, $setOnInsert: { type: 'hosted-mcp', scope: 'user', @@ -99,7 +107,15 @@ export const submitOperatorKey = async (req: OperatorKeyRequest, res: Response) }, { upsert: true, new: true, setDefaultsOnInsert: true }, ); - const writeGeneration = secretWriteGeneration(row.config?.authGeneration ?? 0, 0); + const generation = row.config?.authGeneration ?? 0; + if (generation > 1) { + // A grant was decided against the key connected then, and a pasted key + // cannot prove it is the same account, so a re-paste ends them, as the OAuth + // callback does for a subject it cannot compare (hostedMcpConnect.ts). + await revokeConnectionGrants({ connection: row, revokedBy: userId }); + forgetHostedMcpDrift(String(row._id)); + } + const writeGeneration = secretWriteGeneration(generation, 0); let credentialRef: string; try { credentialRef = await connectorSecrets.put(String(row._id), HOSTED_MCP_ACCESS_TOKEN, key, writeGeneration); @@ -107,10 +123,20 @@ export const submitOperatorKey = async (req: OperatorKeyRequest, res: Response) // Fixed copy only: the store's error text stays out of the response. return res.status(500).json({ error: 'key_store_failed', message: 'the key could not be stored; try again' }); } - await Integration.updateOne( - { _id: row._id }, + const published = await Integration.updateOne( + { _id: row._id, 'config.authGeneration': generation }, { $set: { status: 'connected', 'config.credentialRef': credentialRef } }, ); + if (published?.matchedCount === 0) { + // A removal deleted the row mid-paste, or a newer paste took it. Only the + // first leaves this secret with nothing pointing at it; a newer paste wrote + // the same ref, so it is left alone. + if (!(await Integration.exists({ _id: row._id }))) await connectorSecrets.revoke(credentialRef); + return res.status(409).json({ + error: 'connection_changed', + message: 'the connection changed while the key was saved; paste it again', + }); + } return res.status(201).json({ integrationId: String(row._id), From abcdbad2a17960a32504a427e07406d4d2d920a6 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 02:20:14 -0800 Subject: [PATCH 4/7] test(connectors): an operator-key removal never calls the vendor and deletes the stored key (TASK-330) The entry under test names a revoke endpoint, so the credential mode is what keeps the vendor out. Red with the isOAuthEntry guard removed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../unit/services/connectionRemoval.test.js | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/backend/__tests__/unit/services/connectionRemoval.test.js b/backend/__tests__/unit/services/connectionRemoval.test.js index 2992e1196..3e3c60eb7 100644 --- a/backend/__tests__/unit/services/connectionRemoval.test.js +++ b/backend/__tests__/unit/services/connectionRemoval.test.js @@ -527,6 +527,35 @@ describe('an Atlassian removal (TASK-339)', () => { }); }); +describe('an operator-key removal (TASK-330)', () => { + // A pasted key has no refresh token and no RFC 7009 client, so removal never + // sends it anywhere: it ends our grants, deletes the stored key, and points + // the person at the vendor's key page. The entry here even names an endpoint, + // so it is the credential mode, not a missing URL, that keeps the vendor out. + const KEY_PAGE = 'https://latchkey.example/settings/api-keys'; + const KEY_ENTRY = { + id: 'keyvendor', + credentialMode: 'operator_key', + revoke: { page: KEY_PAGE, endpoint: 'https://latchkey.example/oauth/revoke' }, + resource: 'https://latchkey.example/mcp', + }; + + it('never calls the vendor, deletes the stored key, and hands over the key page', async () => { + const { calls, deps, revokeGrants } = recorder({ entryFor: () => KEY_ENTRY }); + const row = hostedRow({ config: { entryId: 'keyvendor', agentId: 'agent-1', intake: 'operator_key', credentialRef: 'key-ref' } }); + + const result = await remove(row, deps, revokeGrants); + + expect(result).toEqual({ removed: true, grantsRevoked: 2, revokeAt: KEY_PAGE, providerRevoked: false }); + expect(deps.revokeAtVendor).not.toHaveBeenCalled(); + expect(deps.secrets.get).not.toHaveBeenCalled(); + expect(deps.secrets.revoke).toHaveBeenCalledWith('key-ref'); + // The refresh slot is swept too and is empty (`connectorSecrets.revoke` + // returns on a missing ref), as on any row without a refresh token. + expect(calls).toEqual(['grants', 'row', 'material:undefined', 'material:key-ref', 'delete']); + }); +}); + describe('a row whose entry is gone', () => { const COPIED_PAGE = 'https://linear.app/settings/security/legacy'; From 809ae08e0f4d5e30d19107384611a534e9a94f3f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 02:44:59 -0800 Subject: [PATCH 5/7] test(connectors): reach the operator-key route through its production registration (TASK-330) The route test registered submitOperatorKey behind a stub with no limiter, so dropping auth or writeIntegrationsRateLimit from the router left every test green (CodeRabbit on #3213), and CodeQL read the test app as an unlimited route. The test now mounts routes/hostedMcpConnect with observable auth and limiter mocks, and a new arm asserts the limiter runs, then auth, and a caller with no session never reaches the handler. Removing auth, removing the limiter, or removing the route each turns it red. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../routes/hostedMcpOperatorKey.route.test.js | 39 +++++++++++++++---- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js index 7c1a5fbc7..5da81e752 100644 --- a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js +++ b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js @@ -20,6 +20,20 @@ jest.mock('../../../services/hostedMcpToolDefinitions', () => ({ listHostedMcpTools: jest.fn(), forgetHostedMcpDrift: jest.fn(), })); +// The route is reached through the production router, and auth and the write +// limiter are observable, so dropping either from the registration fails a test. +jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => { + const id = req.get('x-test-user'); + if (!id) return res.status(401).json({ error: 'unauthorized' }); + req.user = { id }; + return next(); +})); +jest.mock('../../../middleware/integrationRateLimit', () => ({ + writeIntegrationsRateLimit: jest.fn((_req, _res, next) => next()), + listIntegrationsRateLimit: (_req, _res, next) => next(), + ingestIngressRateLimit: (_req, _res, next) => next(), + ingestIntegrationsRateLimit: (_req, _res, next) => next(), +})); const Integration = require('../../../models/Integration'); const User = require('../../../models/User'); @@ -29,7 +43,9 @@ const connectorSecrets = require('../../../services/connectorSecrets'); const { listHostedMcpTools, forgetHostedMcpDrift } = require('../../../services/hostedMcpToolDefinitions'); const { revokeConnectionGrants } = require('../../../services/roomGrantService'); const { HOSTED_MCP_ENTRIES } = require('../../../integrations/hostedMcp/entries'); -const { submitOperatorKey } = require('../../../routes/hostedMcpOperatorKey'); +const auth = require('../../../middleware/auth'); +const { writeIntegrationsRateLimit } = require('../../../middleware/integrationRateLimit'); +const connectRoutes = require('../../../routes/hostedMcpConnect'); const OWNER = 'bbbbbbbbbbbbbbbbbbbbbb01'; const OTHER = 'bbbbbbbbbbbbbbbbbbbbbb02'; @@ -58,12 +74,7 @@ const KEY_ENTRY = { const app = express(); app.use(express.json()); -app.post('/connect/hosted-mcp/:entryId/operator-key', (req, res, next) => { - const id = req.get('x-test-user'); - if (!id) return res.status(401).json({ error: 'unauthorized' }); - req.user = { id }; - return next(); -}, submitOperatorKey); +app.use('/connect/hosted-mcp', connectRoutes); const submit = (body = {}) => request(app) .post('/connect/hosted-mcp/latchkey/operator-key') @@ -104,6 +115,20 @@ const everyWrittenValue = () => JSON.stringify([ ]); describe('POST /connect/hosted-mcp/:entryId/operator-key', () => { + test('the write limiter runs, then auth, and a caller with no session stops there', async () => { + const res = await request(app) + .post('/connect/hosted-mcp/latchkey/operator-key') + .send({ agentId: AGENT, key: KEY }); + + expect(res.status).toBe(401); + expect(writeIntegrationsRateLimit).toHaveBeenCalledTimes(1); + expect(auth).toHaveBeenCalledTimes(1); + expect(writeIntegrationsRateLimit.mock.invocationCallOrder[0]).toBeLessThan(auth.mock.invocationCallOrder[0]); + expect(User.findOne).not.toHaveBeenCalled(); + expect(listHostedMcpTools).not.toHaveBeenCalled(); + expect(connectorSecrets.put).not.toHaveBeenCalled(); + }); + test('a refused scope among the operator picks is refused before any vendor call or row write', async () => { const res = await submit({ scopePicks: ['read', 'jobs:approve'] }); expect(res.status).toBe(400); From 1a8145a93523f89be4dc48ebb783f7fb9e1c2514 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 03:03:39 -0800 Subject: [PATCH 6/7] fix(connectors): a paste never republishes a row a removal has marked Removal marks a row disconnected and revoked before it deletes it, and the operator-key publish fenced only on the row's generation. A paste that finished between the two put the row back to connected with revokedAt still set: a "connected" row the broker refuses, and one that stays that way if the removal stops before its delete. (CodeRabbit, body-only point.) The publish now also requires the row to be the pending, unrevoked row the paste wrote, the way the OAuth callback fences on its own interim status. On a miss the paste revokes its new secret when the row is gone or marked; a newer paste cleared the mark and wrote the same ref, so it keeps it. Reverse check: dropping the publish fence, or the revokedAt in the exists check, each turns the new test red. 465/465 across the 24 hosted-MCP, operator-key and removal suites; tsc clean. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../routes/hostedMcpOperatorKey.route.test.js | 23 ++++++++++++++++++- backend/routes/hostedMcpOperatorKey.ts | 18 +++++++++++---- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js index 5da81e752..3578c97b0 100644 --- a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js +++ b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js @@ -227,7 +227,9 @@ describe('POST /connect/hosted-mcp/:entryId/operator-key', () => { ); expect(connectorSecrets.put).toHaveBeenCalledWith(ROW_ID, expect.anything(), KEY, expect.any(Number)); expect(Integration.updateOne).toHaveBeenCalledWith( - { _id: ROW_ID, 'config.authGeneration': 1 }, + { + _id: ROW_ID, 'config.authGeneration': 1, status: 'pending', revokedAt: null, + }, { $set: { status: 'connected', 'config.credentialRef': 'secret-ref-1' } }, ); // A first paste has no grants to end. @@ -274,6 +276,25 @@ describe('POST /connect/hosted-mcp/:entryId/operator-key', () => { expect(connectorSecrets.revoke).toHaveBeenCalledWith('secret-ref-1'); }); + // Removal marks the row disconnected and revoked before it deletes it + // (connectionRemovalService), so a paste can finish between the two. That + // row must not go back to connected, and nothing will use the new secret. + test('a row marked by a removal mid-paste stays unpublished, and its new secret is revoked', async () => { + const marked = { + _id: ROW_ID, 'config.authGeneration': 1, status: 'disconnected', revokedAt: new Date(), + }; + const matches = (filter) => Object.entries(filter) + .every(([key, value]) => (value === null ? marked[key] == null : marked[key] === value)); + Integration.updateOne.mockImplementation(async (filter) => ({ + acknowledged: true, matchedCount: matches(filter) ? 1 : 0, + })); + Integration.exists.mockImplementation(async (filter) => (matches(filter) ? { _id: ROW_ID } : null)); + const res = await submit(); + expect(res.status).toBe(409); + expect(res.body.error).toBe('connection_changed'); + expect(connectorSecrets.revoke).toHaveBeenCalledWith('secret-ref-1'); + }); + test('a newer paste that won the row keeps its secret: the older paste is refused without revoking it', async () => { Integration.updateOne.mockResolvedValue({ acknowledged: true, matchedCount: 0 }); const res = await submit(); diff --git a/backend/routes/hostedMcpOperatorKey.ts b/backend/routes/hostedMcpOperatorKey.ts index 160d87c2a..e18e5520d 100644 --- a/backend/routes/hostedMcpOperatorKey.ts +++ b/backend/routes/hostedMcpOperatorKey.ts @@ -123,15 +123,23 @@ export const submitOperatorKey = async (req: OperatorKeyRequest, res: Response) // Fixed copy only: the store's error text stays out of the response. return res.status(500).json({ error: 'key_store_failed', message: 'the key could not be stored; try again' }); } + // Published only while the row is still the pending row this paste wrote. A + // removal marks the row disconnected and revoked before it deletes it, so + // the generation alone would let a paste that lands in between put that row + // back to connected. The OAuth callback fences on its own interim status the + // same way (hostedMcpConnect.ts). const published = await Integration.updateOne( - { _id: row._id, 'config.authGeneration': generation }, + { + _id: row._id, 'config.authGeneration': generation, status: 'pending', revokedAt: null, + }, { $set: { status: 'connected', 'config.credentialRef': credentialRef } }, ); if (published?.matchedCount === 0) { - // A removal deleted the row mid-paste, or a newer paste took it. Only the - // first leaves this secret with nothing pointing at it; a newer paste wrote - // the same ref, so it is left alone. - if (!(await Integration.exists({ _id: row._id }))) await connectorSecrets.revoke(credentialRef); + // A removal took the row mid-paste (deleted it, or marked it on the way), + // or a newer paste took it. Only a removal leaves this secret with nothing + // that will use it; a newer paste wrote the same ref and cleared the + // revocation, so its row still exists unrevoked and the secret is left alone. + if (!(await Integration.exists({ _id: row._id, revokedAt: null }))) await connectorSecrets.revoke(credentialRef); return res.status(409).json({ error: 'connection_changed', message: 'the connection changed while the key was saved; paste it again', From d4fac1b5f2fe60df91d028c4d0cfcc433a2cc0d3 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 03:25:55 -0800 Subject: [PATCH 7/7] fix(connectors): a paste's cleanup deletes only its own write of the key CodeRabbit on 1a8145a93, and right: the previous commit introduced it. A removal-marked row keeps its _id, so a newer paste reuses the same secret ref. The older paste looked the row up, saw the mark, and deleted by ref, and a newer paste that cleared the mark and wrote the ref in between lost its key while its row went connected. connectorSecrets.revokeOwnWrite(ref, writeGeneration) deletes only while the secret still holds that write. The route calls it on every missed publish and drops the row lookup: a newer paste's write carries a higher generation and survives, and a deleted row's successor gets a new ref anyway. Reverse check: an unfenced delete turns the store test red (real Mongo: an older write's cleanup must leave the newer key readable); deleting by ref in the route turns the three cleanup tests red. 476/476 across the 25 hosted-MCP, operator-key, removal and secret suites; tsc clean. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../routes/hostedMcpOperatorKey.route.test.js | 19 ++++++++++++------- .../unit/services/connectorSecrets.test.js | 16 ++++++++++++++++ backend/routes/hostedMcpOperatorKey.ts | 10 ++++++---- backend/services/connectorSecrets.ts | 12 ++++++++++++ 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js index 3578c97b0..1e0e1a948 100644 --- a/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js +++ b/backend/__tests__/unit/routes/hostedMcpOperatorKey.route.test.js @@ -13,7 +13,7 @@ jest.mock('../../../models/Integration', () => ({ findOneAndUpdate: jest.fn(), u jest.mock('../../../models/User', () => ({ findOne: jest.fn() })); jest.mock('../../../models/AgentRegistry', () => ({ AgentInstallation: { find: jest.fn() } })); jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); -jest.mock('../../../services/connectorSecrets', () => ({ put: jest.fn(), revoke: jest.fn() })); +jest.mock('../../../services/connectorSecrets', () => ({ put: jest.fn(), revoke: jest.fn(), revokeOwnWrite: jest.fn() })); jest.mock('../../../services/roomGrantService', () => ({ revokeConnectionGrants: jest.fn() })); jest.mock('../../../services/hostedMcpToolDefinitions', () => ({ guardedFetch: jest.fn(), @@ -101,7 +101,6 @@ beforeEach(() => { listHostedMcpTools.mockResolvedValue([{ name: 'list_jobs' }]); Integration.findOneAndUpdate.mockResolvedValue({ _id: ROW_ID, config: { authGeneration: 1 } }); Integration.updateOne.mockResolvedValue({ acknowledged: true, matchedCount: 1 }); - Integration.exists.mockResolvedValue({ _id: ROW_ID }); connectorSecrets.put.mockResolvedValue('secret-ref-1'); }); afterEach(() => { @@ -267,13 +266,16 @@ describe('POST /connect/hosted-mcp/:entryId/operator-key', () => { expect(update.$unset).toEqual({ 'config.providerRevokedAt': '' }); }); + // The paste's own write is generation 1 (authGeneration 1, refresh 0). + const OWN_WRITE = 1_000_000; + test('a row removed while the key was being saved: the new secret is revoked and the paste refused', async () => { Integration.updateOne.mockResolvedValue({ acknowledged: true, matchedCount: 0 }); - Integration.exists.mockResolvedValue(null); const res = await submit(); expect(res.status).toBe(409); expect(res.body).toEqual({ error: 'connection_changed', message: 'the connection changed while the key was saved; paste it again' }); - expect(connectorSecrets.revoke).toHaveBeenCalledWith('secret-ref-1'); + expect(connectorSecrets.revokeOwnWrite).toHaveBeenCalledWith('secret-ref-1', OWN_WRITE); + expect(connectorSecrets.revoke).not.toHaveBeenCalled(); }); // Removal marks the row disconnected and revoked before it deletes it @@ -288,18 +290,21 @@ describe('POST /connect/hosted-mcp/:entryId/operator-key', () => { Integration.updateOne.mockImplementation(async (filter) => ({ acknowledged: true, matchedCount: matches(filter) ? 1 : 0, })); - Integration.exists.mockImplementation(async (filter) => (matches(filter) ? { _id: ROW_ID } : null)); const res = await submit(); expect(res.status).toBe(409); expect(res.body.error).toBe('connection_changed'); - expect(connectorSecrets.revoke).toHaveBeenCalledWith('secret-ref-1'); + expect(connectorSecrets.revokeOwnWrite).toHaveBeenCalledWith('secret-ref-1', OWN_WRITE); }); - test('a newer paste that won the row keeps its secret: the older paste is refused without revoking it', async () => { + // The ref is reused, so a newer paste may own it by now. The delete is fenced + // on this paste's own write generation, which a newer write has replaced + // (the store's own test pins that), never a delete by ref alone. + test('a newer paste that won the row keeps its secret: the older paste deletes only its own write', async () => { Integration.updateOne.mockResolvedValue({ acknowledged: true, matchedCount: 0 }); const res = await submit(); expect(res.status).toBe(409); expect(connectorSecrets.revoke).not.toHaveBeenCalled(); + expect(connectorSecrets.revokeOwnWrite).toHaveBeenCalledWith('secret-ref-1', OWN_WRITE); }); test('the key appears in no response, no row write, and no thrown error text', async () => { diff --git a/backend/__tests__/unit/services/connectorSecrets.test.js b/backend/__tests__/unit/services/connectorSecrets.test.js index 690a5fec6..6ad0b30cc 100644 --- a/backend/__tests__/unit/services/connectorSecrets.test.js +++ b/backend/__tests__/unit/services/connectorSecrets.test.js @@ -9,6 +9,7 @@ const { putIfAbsent, rewrap, revoke, + revokeOwnWrite, } = require('../../../services/connectorSecrets'); const { DISCORD_WEBHOOK_URL, @@ -124,6 +125,21 @@ describe('connectorSecrets', () => { // TASK-539: the import path. Insert-only, so a secret a live edit stored // first is never replaced with an older copy. + // A writer cleaning up after itself: the ref is stable per (integration, + // kind), so a newer writer may already own it by the time the older one + // deletes, and a delete by ref alone would destroy that writer's key. + it('revokeOwnWrite deletes a secret only while it still holds that write', async () => { + const integrationId = new mongoose.Types.ObjectId().toString(); + const ref = await put(integrationId, SLACK_BOT_TOKEN, 'older', 1_000_000); + expect(await put(integrationId, SLACK_BOT_TOKEN, 'newer', 2_000_000)).toBe(ref); + + await revokeOwnWrite(ref, 1_000_000); + await expect(get(ref)).resolves.toBe('newer'); + + await revokeOwnWrite(ref, 2_000_000); + expect(await ConnectorSecret.findById(ref).lean()).toBeNull(); + }); + it('putIfAbsent inserts when the pair is free and reports it created', async () => { const integrationId = new mongoose.Types.ObjectId().toString(); const { ref, created } = await putIfAbsent(integrationId, GROUPME_ACCESS_TOKEN, 'imported'); diff --git a/backend/routes/hostedMcpOperatorKey.ts b/backend/routes/hostedMcpOperatorKey.ts index e18e5520d..04af9b783 100644 --- a/backend/routes/hostedMcpOperatorKey.ts +++ b/backend/routes/hostedMcpOperatorKey.ts @@ -136,10 +136,12 @@ export const submitOperatorKey = async (req: OperatorKeyRequest, res: Response) ); if (published?.matchedCount === 0) { // A removal took the row mid-paste (deleted it, or marked it on the way), - // or a newer paste took it. Only a removal leaves this secret with nothing - // that will use it; a newer paste wrote the same ref and cleared the - // revocation, so its row still exists unrevoked and the secret is left alone. - if (!(await Integration.exists({ _id: row._id, revokedAt: null }))) await connectorSecrets.revoke(credentialRef); + // or a newer paste took it. Either way nothing will use this paste's key. + // The ref is reused per row, so a newer paste may own it by now: delete + // only while it still holds this paste's own write. A row lookup here + // would race that newer paste, which can clear the mark and overwrite the + // ref between the lookup and the delete. + await connectorSecrets.revokeOwnWrite(credentialRef, writeGeneration); return res.status(409).json({ error: 'connection_changed', message: 'the connection changed while the key was saved; paste it again', diff --git a/backend/services/connectorSecrets.ts b/backend/services/connectorSecrets.ts index 68322515c..c41dd4ecd 100644 --- a/backend/services/connectorSecrets.ts +++ b/backend/services/connectorSecrets.ts @@ -213,6 +213,17 @@ export const revoke = async (ref: string | undefined): Promise => { await ConnectorSecret.deleteOne({ _id: ref }); }; +/** + * Delete a secret only while it still holds the material `writeGeneration` + * wrote. For a writer cleaning up after itself: the ref is stable per + * (integration, kind), so a newer writer may already own it, and a delete by + * ref alone would destroy that writer's key. + */ +export const revokeOwnWrite = async (ref: string | undefined, writeGeneration: number): Promise => { + if (!ref) return; + await ConnectorSecret.deleteOne({ _id: ref, writeGeneration }); +}; + export const rewrap = async (ref: string): Promise => { const secret = await ConnectorSecret.findById(ref) as IConnectorSecret | null; if (!secret) throw new ConnectorSecretNotFoundError(ref); @@ -237,6 +248,7 @@ module.exports = { putIfAbsent, get, revoke, + revokeOwnWrite, rewrap, listWithUnavailableKey, };