From e6d4077ca2cfb381df05d07f86309416edbc72f2 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 13:54:20 -0700 Subject: [PATCH 1/3] feat(discord): encrypt the webhook URL at rest, keyed by (integrationId, kind) (TASK-124 part 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `discord_integrations.webhookUrl` was the last connector credential stored in plaintext outside the ESO path. The URL embeds the webhook's own token, so holding it is posting AS that channel. Writers now put it in the connector-secret envelope (AES-256-GCM, ring from CONNECTOR_SECRET_KEYS) and the row keeps only an opaque ref at `Integration.config.webhookUrlRef`; readers go through `utils/discordWebhookUrl`. Wren's ruling: option A, keyed with `kind`. The same change fixes the three blockers the design exposed, which is what makes the encryption safe to turn on rather than a new way to lose a live credential: 1. `sweepOrphanedConnectorSecrets` kept a secret only on a Slack `botTokenRef`/`pendingBind.botTokenRef` pair, so it revoked a Discord secret ten minutes after it was written (`ORPHAN_SECRET_GRACE_MS`). Its keep-condition is now drawn from the kinds registry. 2. `sweepUnavailableSlackSecretKeys` marked ANY row with a Slack-named reason, and since #1878/#1881 that string is user-facing — a Discord owner read "Slack connector secret key is unavailable". Renamed and made kind-aware; Slack's sentence is byte-identical. 3. `upsertEncrypted` filtered on `{ integrationId }` alone, on both the upsert and the 11000 retry, so a second kind did not collide: it MATCHED the first kind's row and $set the new ciphertext onto it, leaving the first ref valid and decrypting to the second secret. The filter now carries `kind`, the model declares ONE index on the pair, and `services/connectorSecretKinds` is the single definition all four sites read. Also: `webhookUrl` and `webhookUrlRef` join `SERVER_OWNED_CONFIG_KEYS` (the URL is derived by the server on both writers, and a planted ref would point a row at another row's secret), and `webhookUrlRef` is listed in `INTEGRATION_SECRET_CONFIG_KEYS` so the pointer never rides out either. The migration is a separate gated step: `scripts/encrypt-discord-webhook-url` (dry-run by default, `--apply` to write, digests not URLs, idempotent, reads both plaintext stores and unsets both, refuses to write at all when the key ring is unusable). Run it after this deploys — the old image reads the plaintext field directly. Evidence: full backend tier 446 suites / 4167 tests green; tsc 0; eslint .ts 0 errors at the 1210-warning baseline. Eight-arm mutation ledger, no survivors: each arm reverts one piece and every arm reddens a named test. --- .../routes/discordBindingContainment.test.js | 61 +++++- .../integrations.discordTokenCopy.test.js | 25 ++- .../routes/slackOAuth.installables.test.js | 8 +- .../scripts/encryptDiscordWebhookUrl.test.ts | 201 ++++++++++++++++++ .../unit/services/connectorSecrets.test.js | 47 +++- .../unit/services/discordService.test.js | 96 +++++++++ .../installableInstallationService.test.js | 97 ++++++++- .../unit/utils/discordWebhookUrl.test.js | 92 ++++++++ .../integrations/providers/discordProvider.ts | 20 +- backend/models/ConnectorSecret.ts | 21 +- backend/models/DiscordIntegration.ts | 23 +- backend/models/Integration.ts | 9 + backend/models/integrationPublicConfig.ts | 3 + backend/routes/installables.ts | 4 +- backend/routes/integrations.ts | 31 ++- .../scripts/encrypt-discord-webhook-url.ts | 197 +++++++++++++++++ backend/services/connectorSecretKinds.ts | 93 ++++++++ backend/services/connectorSecrets.ts | 31 ++- backend/services/discordService.ts | 45 +++- .../installable/installableReconciler.ts | 53 +++-- backend/utils/discordWebhookUrl.ts | 55 +++++ 21 files changed, 1146 insertions(+), 66 deletions(-) create mode 100644 backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts create mode 100644 backend/__tests__/unit/utils/discordWebhookUrl.test.js create mode 100644 backend/scripts/encrypt-discord-webhook-url.ts create mode 100644 backend/services/connectorSecretKinds.ts create mode 100644 backend/utils/discordWebhookUrl.ts diff --git a/backend/__tests__/unit/routes/discordBindingContainment.test.js b/backend/__tests__/unit/routes/discordBindingContainment.test.js index fd28ddc8bc..0b2315736a 100644 --- a/backend/__tests__/unit/routes/discordBindingContainment.test.js +++ b/backend/__tests__/unit/routes/discordBindingContainment.test.js @@ -47,7 +47,7 @@ jest.mock('../../../models/Integration', () => { function Integration(data) { Object.assign(this, data); - this._id = data._id || 'integration-new'; + this._id = data._id || '6a8f6de1a1dccf2e02f31459'; this.save = jest.fn().mockResolvedValue(this); lastInstance = this; } @@ -94,6 +94,18 @@ jest.mock('../../../services/discordMultiCommandService', () => ({ runDiscordCommandForIntegrations: jest.fn(), })); +// The create path encrypts the webhook URL. This suite is about binding +// containment, not the envelope, so the envelope is stubbed here — leaving it +// real would have the route await a Mongo write against no connection (the +// suites that own that behaviour are integration.discordTokenCopy and the +// connectorSecrets unit file). +jest.mock('../../../services/connectorSecrets', () => ({ + put: jest.fn(async () => 'secret-ref-1'), + get: jest.fn(), + revoke: jest.fn(), + listWithUnavailableKey: jest.fn(), +})); + const axios = require('axios'); const Pod = require('../../../models/Pod'); const User = require('../../../models/User'); @@ -140,12 +152,24 @@ const podIdsQuery = (rows) => { }; describe('Discord binding containment (TASK-123 a)', () => { + const savedKeys = process.env.CONNECTOR_SECRET_KEYS; + const savedActiveKey = process.env.CONNECTOR_SECRET_ACTIVE_KEY; + + afterAll(() => { + process.env.CONNECTOR_SECRET_KEYS = savedKeys; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = savedActiveKey; + }); + beforeEach(() => { jest.clearAllMocks(); process.env.FRONTEND_URL = 'https://app.example.test'; process.env.DISCORD_BOT_TOKEN = 'bot-secret'; process.env.DISCORD_CLIENT_ID = 'client-id'; process.env.DISCORD_CLIENT_SECRET = 'client-secret'; + // The create path encrypts the webhook URL (TASK-124 part 2), so a connector + // secret ring is part of the deployment this route runs against. + process.env.CONNECTOR_SECRET_KEYS = `k1:${Buffer.alloc(32, 1).toString('base64')}`; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'k1'; podIdsQuery([{ _id: 'pod-1' }]); Pod.findById.mockResolvedValue({ _id: 'pod-1', members: [MEMBER], createdBy: MEMBER }); User.findById.mockResolvedValue({ _id: MEMBER, role: 'member' }); @@ -333,6 +357,12 @@ describe('Discord binding containment (TASK-123 a)', () => { expect(res.status).toBe(201); expect(Integration.__getLastInstance().config).not.toHaveProperty('botToken'); + // The URL the create path derived is encrypted, not written to the row: the + // strip above must not have eaten the ref the reader resolves through. + expect(Integration.findByIdAndUpdate).toHaveBeenCalledWith( + expect.anything(), + { $set: { 'config.webhookUrlRef': 'secret-ref-1' } }, + ); expect(axios.post).toHaveBeenCalledWith( `https://discord.com/api/channels/${CHANNEL}/webhooks`, { name: 'Commonly Bot', avatar: null }, @@ -410,8 +440,37 @@ describe('Discord binding containment (TASK-123 a)', () => { expect(stored.config).not.toHaveProperty('botToken'); expect(stored.config.channelId).toBe('C012345'); }); + + it('drops a caller-supplied webhookUrl and webhookUrlRef from the stored config', async () => { + // Both names join the server-owned list with this change: the URL is a + // bearer credential the server derives from the Discord API on both + // writers, and the ref is a pointer a caller must not be able to aim at + // another row's secret. A planted `config.webhookUrl` would also be picked + // up as the resolver's legacy fallback. + const res = await post({ + podId: 'pod-1', + type: 'discord', + config: { + serverId: GUILD, + channelId: CHANNEL, + webhookUrl: 'https://discord.com/api/webhooks/999/caller-token', + webhookUrlRef: 'somebody-elses-ref', + }, + }); + + expect(res.status).toBe(201); + const stored = Integration.__getLastInstance(); + expect(stored.config).not.toHaveProperty('webhookUrl'); + expect(stored.config).not.toHaveProperty('webhookUrlRef'); + // The ref on the row is the one the server wrote from the envelope. + expect(Integration.findByIdAndUpdate).toHaveBeenCalledWith( + expect.anything(), + { $set: { 'config.webhookUrlRef': 'secret-ref-1' } }, + ); + }); }); + describe('PATCH /api/integrations/:id', () => { const patch = (body) => request(app) .patch('/api/integrations/64b64c7f8a9e2f0012345678') diff --git a/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js b/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js index df0570fff5..e8ad1f12cc 100644 --- a/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js +++ b/backend/__tests__/unit/routes/integrations.discordTokenCopy.test.js @@ -43,6 +43,7 @@ const DiscordIntegration = require('../../../models/DiscordIntegration'); const Pod = require('../../../models/Pod'); const User = require('../../../models/User'); const integrationRoutes = require('../../../routes/integrations'); +const { get: getConnectorSecret } = require('../../../services/connectorSecrets'); const GUILD = '123456789012345678'; const CHANNEL = '123456789012345679'; @@ -57,6 +58,8 @@ describe('discord connector stores no bot token copy (TASK-124)', () => { let pod; let creator; const savedEnv = process.env.DISCORD_BOT_TOKEN; + const savedKeys = process.env.CONNECTOR_SECRET_KEYS; + const savedActiveKey = process.env.CONNECTOR_SECRET_ACTIVE_KEY; const post = (body) => request(app) .post('/api/integrations') @@ -77,6 +80,10 @@ describe('discord connector stores no bot token copy (TASK-124)', () => { beforeEach(async () => { process.env.DISCORD_BOT_TOKEN = ENV_TOKEN; + // The create path now encrypts the webhook URL, so the key ring is part of + // the deployment it runs against — the same dependency the Slack bind has. + process.env.CONNECTOR_SECRET_KEYS = `k1:${Buffer.alloc(32, 1).toString('base64')}`; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'k1'; await Integration.deleteMany({}); await DiscordIntegration.deleteMany({}); axios.post.mockReset(); @@ -88,6 +95,11 @@ describe('discord connector stores no bot token copy (TASK-124)', () => { else process.env.DISCORD_BOT_TOKEN = savedEnv; }); + afterAll(() => { + process.env.CONNECTOR_SECRET_KEYS = savedKeys; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = savedActiveKey; + }); + it('creates the connector without persisting the token, and the row still saves', async () => { const res = await post({ podId: String(pod._id), @@ -104,8 +116,16 @@ describe('discord connector stores no bot token copy (TASK-124)', () => { // so the absence assertion below cannot pass on a row that was never made. expect(row).not.toBeNull(); expect(row.webhookId).toBe('wh-1'); - expect(row.webhookUrl).toContain('/wh-1/'); expect(row.serverId).toBe(GUILD); + // The claim this change adds: the URL the create path derived is encrypted, + // not stored. No plaintext on the platform row, the ref on the Integration + // row, and the ref resolves back to the URL the Discord API returned — so + // "it is gone from the row" is not the same as "it is gone". + expect(row.webhookUrl).toBeUndefined(); + const integrationRow = await Integration.findById(row.integrationId).lean(); + expect(integrationRow.config.webhookUrlRef).toBeTruthy(); + expect(await getConnectorSecret(integrationRow.config.webhookUrlRef)) + .toBe('https://discord.com/api/webhooks/wh-1/wh-token'); // The claim: no copy of the instance-wide token is stored on the row. expect(row.botToken).toBeUndefined(); }); @@ -122,6 +142,9 @@ describe('discord connector stores no bot token copy (TASK-124)', () => { expect(res.status).toBe(201); expect(res.body.platformIntegration).not.toHaveProperty('botToken'); expect(res.body.platformIntegration).not.toHaveProperty('webhookUrl'); + // The pointer to the encrypted URL is a secret-config key, so it does not + // ride out either. + expect(res.body.integration.config).not.toHaveProperty('webhookUrlRef'); }); it('still requires a token to connect: with the env var absent the create is refused', async () => { diff --git a/backend/__tests__/unit/routes/slackOAuth.installables.test.js b/backend/__tests__/unit/routes/slackOAuth.installables.test.js index 6a3873e6b4..cd1e4d4b57 100644 --- a/backend/__tests__/unit/routes/slackOAuth.installables.test.js +++ b/backend/__tests__/unit/routes/slackOAuth.installables.test.js @@ -185,7 +185,13 @@ describe('Slack installable OAuth routes', () => { expect(response.status).toBe(302); expect(response.headers.location).toBe(`${TEST_PUBLIC_APP_URL}/v2/connectors?slack=pending`); expect(slackOAuth.exchangeCode).toHaveBeenCalledWith('slack-code'); - expect(connectorSecrets.put).toHaveBeenCalledWith('integration-1', 'slack', 'xoxb-never-store-on-integration'); + // The kind spec, not a bare provider string: it is the one definition of what + // this secret is and where its ref lives (TASK-124 part 2). + expect(connectorSecrets.put).toHaveBeenCalledWith( + 'integration-1', + expect.objectContaining({ kind: 'slack-bot-token', provider: 'slack' }), + 'xoxb-never-store-on-integration', + ); const [, commit] = Integration.findOneAndUpdate.mock.calls[1]; expect(commit.$set['config.pendingBind']).toMatchObject({ teamId: 'T1', chatId: 'D1', botTokenRef: 'secret-ref' }); expect(JSON.stringify(commit)).not.toContain('xoxb-never-store-on-integration'); diff --git a/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts b/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts new file mode 100644 index 0000000000..89da101d99 --- /dev/null +++ b/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts @@ -0,0 +1,201 @@ +// @ts-nocheck + +// The migration suite needs testUtils' Mongo harness, not JWT behavior. +jest.mock('jsonwebtoken', () => ({})); + +import mongoose from 'mongoose'; + +const { + encryptDiscordWebhookUrls, + formatReport, +} = require('../../../scripts/encrypt-discord-webhook-url'); +const { get } = require('../../../services/connectorSecrets'); +const { + setupMongoDb, + closeMongoDb, + clearMongoDb, +} = require('../../utils/testUtils'); + +const URL_A = 'https://discord.com/api/webhooks/111/token-aaa'; +const URL_B = 'https://discord.com/api/webhooks/222/token-bbb'; +const URL_C = 'https://example.test/not-a-discord-webhook'; + +const key = (fill) => Buffer.alloc(32, fill).toString('base64'); + +const discordRows = () => mongoose.connection.collection('discord_integrations'); +const integrationRows = () => mongoose.connection.collection('integrations'); + +const seedDiscordRow = (overrides = {}) => ({ + integrationId: new mongoose.Types.ObjectId(), + serverId: 'srv', + serverName: 'Server', + channelId: 'chan', + channelName: 'channel', + webhookId: 'wh1', + ...overrides, +}); + +/** A platform row plus the Integration row it belongs to. */ +const seedPair = async ({ platform = {}, config = {} } = {}) => { + const integrationId = new mongoose.Types.ObjectId(); + const platformRow = seedDiscordRow({ integrationId, ...platform }); + await discordRows().insertOne(platformRow); + await integrationRows().insertOne({ + _id: integrationId, + type: 'discord', + isActive: true, + config, + }); + return { integrationId, platformId: platformRow._id }; +}; + +describe('encrypt-discord-webhook-url', () => { + const originalKeys = process.env.CONNECTOR_SECRET_KEYS; + const originalActiveKey = process.env.CONNECTOR_SECRET_ACTIVE_KEY; + + beforeAll(async () => setupMongoDb()); + afterAll(async () => { + await closeMongoDb(); + process.env.CONNECTOR_SECRET_KEYS = originalKeys; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = originalActiveKey; + }); + beforeEach(async () => { + await clearMongoDb(); + process.env.CONNECTOR_SECRET_KEYS = `k1:${key(1)}`; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'k1'; + }); + + it('replaces the plaintext platform copy with a ref that resolves to it, and clears both stores', async () => { + const { integrationId, platformId } = await seedPair({ + platform: { webhookUrl: URL_A }, + config: { channelId: 'chan', webhookUrl: '' }, + }); + + const result = await encryptDiscordWebhookUrls({ apply: true }); + + expect(result).toMatchObject({ rows: 1, candidates: 1, migrated: 1, alreadyEncrypted: 0 }); + const integration = await integrationRows().findOne({ _id: integrationId }); + expect(integration.config.webhookUrlRef).toBeTruthy(); + expect(integration.config).not.toHaveProperty('webhookUrl'); + expect(await get(integration.config.webhookUrlRef)).toBe(URL_A); + const platform = await discordRows().findOne({ _id: platformId }); + expect(platform).not.toHaveProperty('webhookUrl'); + expect(platform.webhookId).toBe('wh1'); // not a secret; untouched + }); + + it('encrypts the legacy Integration.config.webhookUrl store too, and reports it apart', async () => { + // The second store: a row whose platform document carries no URL. wren 74127 + // is why this exists — `Integration.config.webhookUrl` is a Discord store, not + // registry config, and the migration that read only the platform document + // would have left this value in the clear. + const { integrationId } = await seedPair({ + config: { channelId: 'chan', webhookUrl: URL_B }, + }); + + const result = await encryptDiscordWebhookUrls({ apply: true }); + + expect(result).toMatchObject({ candidates: 1, migrated: 1, configCopies: 1 }); + const integration = await integrationRows().findOne({ _id: integrationId }); + expect(integration.config).not.toHaveProperty('webhookUrl'); + expect(await get(integration.config.webhookUrlRef)).toBe(URL_B); + }); + + it('is idempotent: a second apply finds nothing to migrate', async () => { + await seedPair({ platform: { webhookUrl: URL_A } }); + + const first = await encryptDiscordWebhookUrls({ apply: true }); + const second = await encryptDiscordWebhookUrls({ apply: true }); + + expect(first.migrated).toBe(1); + expect(second).toMatchObject({ candidates: 0, migrated: 0, alreadyEncrypted: 1 }); + }); + + it('writes nothing on a dry run but still names the work', async () => { + const { integrationId, platformId } = await seedPair({ platform: { webhookUrl: URL_A } }); + + const result = await encryptDiscordWebhookUrls({ apply: false }); + + expect(result).toMatchObject({ candidates: 1, migrated: 0 }); + const integration = await integrationRows().findOne({ _id: integrationId }); + expect(integration.config).not.toHaveProperty('webhookUrlRef'); + expect((await discordRows().findOne({ _id: platformId })).webhookUrl).toBe(URL_A); + }); + + it('keeps a non-Discord-shaped value out of the report but still encrypts it', async () => { + const { integrationId } = await seedPair({ platform: { webhookUrl: URL_C } }); + + const result = await encryptDiscordWebhookUrls({ apply: true }); + + expect(result).toMatchObject({ candidates: 1, migrated: 1, unexpectedFormat: 1 }); + const integration = await integrationRows().findOne({ _id: integrationId }); + expect(await get(integration.config.webhookUrlRef)).toBe(URL_C); + }); + + it('refuses to write anything when the key ring is unusable', async () => { + // The destructive half of this migration is the $unset. A run that cannot + // encrypt must not reach it: losing the plaintext without storing a ref + // leaves the live credential unrecoverable. + process.env.CONNECTOR_SECRET_KEYS = ''; + const { integrationId, platformId } = await seedPair({ platform: { webhookUrl: URL_A } }); + + await expect(encryptDiscordWebhookUrls({ apply: true })).rejects.toThrow(); + + const integration = await integrationRows().findOne({ _id: integrationId }); + expect(integration.config).not.toHaveProperty('webhookUrlRef'); + expect((await discordRows().findOne({ _id: platformId })).webhookUrl).toBe(URL_A); + }); + + it('identifies each URL without reproducing it', async () => { + await seedPair({ platform: { webhookUrl: URL_A } }); + await seedPair({ platform: { webhookUrl: URL_B } }); + + const result = await encryptDiscordWebhookUrls({ apply: false }); + + result.digests.forEach((entry) => { + expect(entry).toMatch(/^[0-9a-f]{12} \(\d+ chars\)$/); + }); + const logged = result.digests.join(' '); + expect(logged).not.toContain(URL_A); + expect(logged).not.toContain('token-aaa'); + expect(new Set(result.digests).size).toBe(2); + }); + + it('labels every count it prints, including the two that are not actions', () => { + const lines = formatReport({ + rows: 3, + candidates: 1, + migrated: 1, + alreadyEncrypted: 1, + withoutWebhook: 1, + unexpectedFormat: 1, + configCopies: 1, + digests: ['abcdef123456 (45 chars)'], + }, true); + + expect(lines[0]).toBe( + '[encrypt-discord-webhook-url] APPLY rows=3 candidates=1 migrated=1', + ); + expect(lines[1]).toBe(' plaintext webhook abcdef123456 (45 chars)'); + expect(lines).toContain('[encrypt-discord-webhook-url] already encrypted (ref, no plaintext): 1'); + expect(lines).toContain('[encrypt-discord-webhook-url] no webhook at all: 1'); + expect(lines).toContain( + '[encrypt-discord-webhook-url] plaintext found in Integration.config.webhookUrl ' + + '(the legacy second store): 1', + ); + expect(lines).toContain( + '[encrypt-discord-webhook-url] candidates not matching the Discord webhook URL shape ' + + '(encrypted anyway): 1', + ); + expect(lines).not.toContain('DRY RUN — nothing written. Re-run with --apply.'); + expect(formatReport({ + rows: 0, + candidates: 0, + migrated: 0, + alreadyEncrypted: 0, + withoutWebhook: 0, + unexpectedFormat: 0, + configCopies: 0, + digests: [], + }, false)).toContain('DRY RUN — nothing written. Re-run with --apply.'); + }); +}); diff --git a/backend/__tests__/unit/services/connectorSecrets.test.js b/backend/__tests__/unit/services/connectorSecrets.test.js index 5dade6108b..987eff20e8 100644 --- a/backend/__tests__/unit/services/connectorSecrets.test.js +++ b/backend/__tests__/unit/services/connectorSecrets.test.js @@ -8,6 +8,10 @@ const { rewrap, revoke, } = require('../../../services/connectorSecrets'); +const { + DISCORD_WEBHOOK_URL, + SLACK_BOT_TOKEN, +} = require('../../../services/connectorSecretKinds'); const { setupMongoDb, closeMongoDb, clearMongoDb } = require('../../utils/testUtils'); const key = (fill) => Buffer.alloc(32, fill).toString('base64'); @@ -35,26 +39,55 @@ describe('connectorSecrets', () => { it('encrypts with a fresh IV and keeps material out of the document', async () => { const integrationId = new mongoose.Types.ObjectId().toString(); - const first = await put(integrationId, 'slack', 'xoxb-secret'); - const second = await put(integrationId, 'slack', 'xoxb-secret'); + const first = await put(integrationId, SLACK_BOT_TOKEN, 'xoxb-secret'); + const second = await put(integrationId, SLACK_BOT_TOKEN, 'xoxb-secret'); expect(second).toBe(first); const stored = await ConnectorSecret.findById(first).lean(); - expect(stored).toMatchObject({ provider: 'slack', keyId: 'k1' }); + expect(stored).toMatchObject({ provider: 'slack', kind: 'slack-bot-token', keyId: 'k1' }); expect(stored.ciphertext).not.toContain('xoxb-secret'); expect(stored.iv).toHaveLength(16); // base64 of a fresh 96-bit IV expect(await get(first)).toBe('xoxb-secret'); }); + it('keys a secret by the PAIR (integrationId, kind), so a second kind does not overwrite the first', async () => { + // The corruption this guards: with a filter on `integrationId` alone the + // second put does not collide — it matches the first kind's document and + // $sets the new ciphertext onto it, so the first ref stays valid and starts + // decrypting to the second secret (vera 74141). Varying the kind is the + // whole test; the refs and both plaintexts must differ. + const integrationId = new mongoose.Types.ObjectId().toString(); + const slackRef = await put(integrationId, SLACK_BOT_TOKEN, 'xoxb-secret'); + const webhook = 'https://discord.com/api/webhooks/1/token'; + const discordRef = await put(integrationId, DISCORD_WEBHOOK_URL, webhook); + + expect(discordRef).not.toBe(slackRef); + expect(await ConnectorSecret.countDocuments({ integrationId })).toBe(2); + expect(await get(slackRef)).toBe('xoxb-secret'); + expect(await get(discordRef)).toBe(webhook); + }); + + it('declares exactly one index, on the pair — a leftover single-field unique would still be enforced at boot', async () => { + // `syncIndexes()` keeps every declared index, so a `unique: true` left on + // `integrationId` while the compound index is added leaves the OLD key + // enforced: the second kind is rejected by an index whose name no longer + // says what it does (wren 74127). + const declared = ConnectorSecret.schema.indexes(); + expect(declared).toHaveLength(1); + const [spec, options] = declared[0]; + expect(spec).toEqual({ integrationId: 1, kind: 1 }); + expect(options).toMatchObject({ unique: true }); + }); + it('fails closed when ciphertext authentication fails', async () => { - const ref = await put(new mongoose.Types.ObjectId().toString(), 'slack', 'xoxb-secret'); + const ref = await put(new mongoose.Types.ObjectId().toString(), SLACK_BOT_TOKEN, 'xoxb-secret'); await ConnectorSecret.updateOne({ _id: ref }, { $set: { tag: Buffer.alloc(16, 9).toString('base64') } }); await expect(get(ref)).rejects.toThrow(); }); it('rewraps under the active key before the former key is removed', async () => { - const ref = await put(new mongoose.Types.ObjectId().toString(), 'slack', 'xoxb-secret'); + const ref = await put(new mongoose.Types.ObjectId().toString(), SLACK_BOT_TOKEN, 'xoxb-secret'); process.env.CONNECTOR_SECRET_KEYS = `k1:${key(1)},k2:${key(2)}`; process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'k2'; @@ -64,7 +97,7 @@ describe('connectorSecrets', () => { }); it('raises a typed error for a key removed without a rewrap', async () => { - const ref = await put(new mongoose.Types.ObjectId().toString(), 'slack', 'xoxb-secret'); + const ref = await put(new mongoose.Types.ObjectId().toString(), SLACK_BOT_TOKEN, 'xoxb-secret'); process.env.CONNECTOR_SECRET_KEYS = `k2:${key(2)}`; process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'k2'; @@ -72,7 +105,7 @@ describe('connectorSecrets', () => { }); it('deletes the secret by its opaque reference', async () => { - const ref = await put(new mongoose.Types.ObjectId().toString(), 'slack', 'xoxb-secret'); + const ref = await put(new mongoose.Types.ObjectId().toString(), SLACK_BOT_TOKEN, 'xoxb-secret'); await revoke(ref); expect(await ConnectorSecret.findById(ref)).toBeNull(); }); diff --git a/backend/__tests__/unit/services/discordService.test.js b/backend/__tests__/unit/services/discordService.test.js index e4fb04de7b..ee62a88c2f 100644 --- a/backend/__tests__/unit/services/discordService.test.js +++ b/backend/__tests__/unit/services/discordService.test.js @@ -25,6 +25,14 @@ jest.mock('../../../models/DiscordIntegration', () => ({ jest.mock('../../../models/DiscordSummaryHistory', () => jest.fn().mockImplementation(() => ({ save: jest.fn(), }))); +// The envelope is stubbed: this suite witnesses the CALL, not the cryptography +// (connectorSecrets' own unit file and the resolver's pin that). +jest.mock('../../../services/connectorSecrets', () => ({ + get: jest.fn(), + put: jest.fn(), + revoke: jest.fn(), + listWithUnavailableKey: jest.fn(), +})); jest.mock('../../../services/discordCommandService'); jest.mock('../../../services/agentEventService', () => ({ enqueue: jest.fn() })); jest.mock('../../../config/discord'); @@ -33,6 +41,8 @@ jest.mock('axios'); const axios = require('axios'); const DiscordService = require('../../../services/discordService'); const Integration = require('../../../models/Integration'); +const DiscordIntegration = require('../../../models/DiscordIntegration'); +const connectorSecrets = require('../../../services/connectorSecrets'); const DiscordCommandService = require('../../../services/discordCommandService'); const AgentEventService = require('../../../services/agentEventService'); @@ -41,6 +51,11 @@ jest.mock('../../../config/discord', () => ({ botToken: 'test-bot-token', clientId: 'test-client-id', applicationId: 'test-app-id', + webhookName: 'Commonly Bot', + webhookAvatar: null, + // Needed by sendMessage, which the TASK-124 part 2 cases below exercise. + messageRateLimit: { timeWindow: 60_000, maxMessages: 10 }, + errors: { RATE_LIMITED: 'Rate limited' }, })); describe('DiscordService', () => { @@ -151,6 +166,35 @@ describe('DiscordService', () => { ); }); + it('backfills a missing webhook as an encrypted ref, not as a plaintext URL', async () => { + // The second writer of the URL (TASK-124 part 2). It used to $set the URL + // straight onto the platform row, which is the plaintext-at-rest state the + // migration removes — so this path had to move with the connect path. + connectorSecrets.put.mockResolvedValue('ref-backfill'); + discordService.integration = { + ...mockIntegration, + config: { ...mockIntegration.config }, + platformIntegration: { serverId: 'guild123', channelId: 'channel123', _id: 'platform-1' }, + }; + discordService.createWebhook = jest.fn() + .mockResolvedValue({ url: 'https://discord.com/api/webhooks/5/token', id: 'wh-5' }); + + await expect(discordService.connect()).resolves.toBe(true); + + expect(connectorSecrets.put).toHaveBeenCalledWith( + 'integration123', + expect.objectContaining({ kind: 'discord-webhook-url' }), + 'https://discord.com/api/webhooks/5/token', + ); + expect(Integration.findByIdAndUpdate).toHaveBeenCalledWith( + 'integration123', + { $set: { 'config.webhookUrlRef': 'ref-backfill' } }, + ); + // Only the id reaches the platform document; it is not a secret. + expect(DiscordIntegration.findByIdAndUpdate) + .toHaveBeenCalledWith('platform-1', { webhookId: 'wh-5' }); + }); + it('should handle connection errors', async () => { discordService.client.guilds.fetch.mockRejectedValue( new Error('Guild not found'), @@ -499,4 +543,56 @@ describe('DiscordService', () => { fetchSpy.mockRestore(); }); }); + + describe('TASK-124 part 2 — the outbound URL is resolved, never the ref', () => { + const WEBHOOK_URL = 'https://discord.com/api/webhooks/999/resolved'; + + beforeEach(() => { + Integration.find = jest.fn().mockReturnValue({ count: jest.fn().mockResolvedValue(0) }); + global.fetch = jest.fn().mockResolvedValue({ ok: true, status: 200 }); + }); + + it('decrypts the ref and posts to the URL, not to the pointer', async () => { + connectorSecrets.get.mockResolvedValue(WEBHOOK_URL); + discordService.integration = { + ...mockIntegration, + config: { ...mockIntegration.config, webhookUrlRef: 'ref-1' }, + platformIntegration: { ...mockIntegration.platformIntegration, webhookUrl: undefined }, + }; + + await expect(discordService.sendMessage('hello')).resolves.toBe(true); + + expect(connectorSecrets.get).toHaveBeenCalledWith('ref-1'); + expect(global.fetch).toHaveBeenCalledWith(WEBHOOK_URL, expect.anything()); + }); + + it('still posts from the legacy plaintext copy when no ref exists', async () => { + discordService.integration = { ...mockIntegration, config: { ...mockIntegration.config } }; + + await expect(discordService.sendMessage('hello')).resolves.toBe(true); + + expect(global.fetch).toHaveBeenCalledWith( + mockIntegration.platformIntegration.webhookUrl, + expect.anything(), + ); + }); + + it('fails loudly on an unreadable ref instead of falling back to the plaintext copy', async () => { + // The substitution this guards: a revoked or rotated secret with a stale + // plaintext still on the row would keep sending to a URL nothing manages, + // and a wrong webhook URL does not fail — it authenticates as another + // connector (vera 74141). + connectorSecrets.get.mockRejectedValue(new Error('key ring unavailable')); + discordService.integration = { + ...mockIntegration, + config: { ...mockIntegration.config, webhookUrlRef: 'ref-1' }, + }; + + // sendMessage rethrows (its catch logs and re-raises), so the failure is + // visible to the caller rather than swallowed as a `false` return. + await expect(discordService.sendMessage('hello')).rejects.toThrow('key ring unavailable'); + + expect(global.fetch).not.toHaveBeenCalled(); + }); + }); }); diff --git a/backend/__tests__/unit/services/installableInstallationService.test.js b/backend/__tests__/unit/services/installableInstallationService.test.js index fa0b61295e..138d72f8bf 100644 --- a/backend/__tests__/unit/services/installableInstallationService.test.js +++ b/backend/__tests__/unit/services/installableInstallationService.test.js @@ -22,6 +22,7 @@ const { const { sweep } = require('../../../services/installable/installableReconciler'); const { TELEGRAM_CONNECTOR, SLACK_CONNECTOR } = require('../../../scripts/seed-builtin-connectors'); const { put } = require('../../../services/connectorSecrets'); +const { DISCORD_WEBHOOK_URL, SLACK_BOT_TOKEN } = require('../../../services/connectorSecretKinds'); const telegramService = require('../../../services/telegramService'); const { relayTelegramMessageToPod } = require('../../../services/telegramBridgeService'); const { @@ -480,7 +481,7 @@ describe('installable connector projection', () => { it('revokes Slack’s envelope secret when the installation is uninstalled', async () => { const { userId, podId } = ids(); const installed = await install({ installableId: 'slack', installedBy: userId, podId }); - const ref = await put(String(installed.integration._id), 'slack', 'xoxb-secret'); + const ref = await put(String(installed.integration._id), SLACK_BOT_TOKEN, 'xoxb-secret'); await Integration.updateOne( { _id: installed.integration._id }, { $set: { 'config.botTokenRef': ref } }, @@ -715,7 +716,7 @@ describe('installable connector projection', () => { it('expires a pending Slack bind, revokes its encrypted token, and leaves a retryable active card', async () => { const { userId, podId } = ids(); const installed = await install({ installableId: 'slack', installedBy: userId, podId }); - const ref = await put(String(installed.integration._id), 'slack', 'xoxb-secret'); + const ref = await put(String(installed.integration._id), SLACK_BOT_TOKEN, 'xoxb-secret'); await Integration.updateOne( { _id: installed.integration._id }, { @@ -741,7 +742,7 @@ describe('installable connector projection', () => { }); it('does not sweep a freshly written orphaned secret before a callback can commit its bind', async () => { - const ref = await put(new mongoose.Types.ObjectId().toString(), 'slack', 'xoxb-secret'); + const ref = await put(new mongoose.Types.ObjectId().toString(), SLACK_BOT_TOKEN, 'xoxb-secret'); const now = new Date(); const fresh = await sweep(now); @@ -760,7 +761,7 @@ describe('installable connector projection', () => { it('keeps a Slack projection visible but error-gated when its secret key disappears from the ring', async () => { const { userId, podId } = ids(); const installed = await install({ installableId: 'slack', installedBy: userId, podId }); - const ref = await put(String(installed.integration._id), 'slack', 'xoxb-secret'); + const ref = await put(String(installed.integration._id), SLACK_BOT_TOKEN, 'xoxb-secret'); await Integration.updateOne( { _id: installed.integration._id }, { @@ -779,13 +780,99 @@ describe('installable connector projection', () => { const integration = await Integration.findById(installed.integration._id); const parent = await InstallableInstallation.findById(installed.installation._id); - expect(reconciled.unavailableSlackSecretKeys).toBe(1); + expect(reconciled.unavailableConnectorSecretKeys).toBe(1); expect(integration.isActive).toBe(true); expect(integration.status).toBe('error'); expect(integration.errorMessage).toMatch(/secret key/i); expect(parent.components.every((component) => component.status === 'active')).toBe(true); }); + const seedDiscordConnectorWithSecret = async () => { + const integration = await Integration.create({ + podId: new mongoose.Types.ObjectId(), + type: 'discord', + status: 'connected', + isActive: true, + createdBy: new mongoose.Types.ObjectId(), + config: { serverId: 'srv', channelId: 'chan' }, + }); + const ref = await put( + String(integration._id), + DISCORD_WEBHOOK_URL, + 'https://discord.com/api/webhooks/1/token', + ); + await Integration.updateOne( + { _id: integration._id }, + { $set: { 'config.webhookUrlRef': ref } }, + ); + return { integrationId: integration._id, ref }; + }; + + const backdateSecret = async (ref) => { + const now = new Date(); + await ConnectorSecret.collection.updateOne( + { _id: new mongoose.Types.ObjectId(ref) }, + { $set: { createdAt: new Date(now.getTime() - (10 * 60_000) - 1) } }, + ); + return now; + }; + + it('keeps a Discord webhook secret past the grace window while its row still points at it', async () => { + // Blocker 1 (vera 74130): the keep-condition was a literal `botTokenRef` + // pair, so this secret matched neither path, the sweep revoked it ten + // minutes after the write, and the connector's webhook stopped resolving. + const { ref } = await seedDiscordConnectorWithSecret(); + const now = await backdateSecret(ref); + + const swept = await sweep(now); + + expect(swept.orphanedSecrets).toBe(0); + expect(await ConnectorSecret.findById(ref)).not.toBeNull(); + }); + + it('names the Discord webhook, not a Slack key, when a Discord secret is unreadable', async () => { + // Blocker 2 (vera 74131): the reason string is written through + // `userFacingError` and rendered verbatim on the Connectors page, so a + // Slack-named sentence reached Discord owners. + const { integrationId, ref } = await seedDiscordConnectorWithSecret(); + process.env.CONNECTOR_SECRET_KEYS = `k2:${Buffer.alloc(32, 2).toString('base64')}`; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'k2'; + + const reconciled = await sweep(new Date()); + const integration = await Integration.findById(integrationId); + + expect(reconciled.unavailableConnectorSecretKeys).toBe(1); + expect(await ConnectorSecret.findById(ref)).not.toBeNull(); + expect(integration.status).toBe('error'); + expect(integration.errorMessage).toBe('Discord connector webhook is unavailable'); + expect(integration.errorMessageUserFacing).toBe(true); + }); + + it('keeps a secret whose kind it cannot place instead of deleting the ref it never looked for', async () => { + // Fail-closed on an unrecognized kind: this build does not know where that + // ref lives, and "the sweep found no reference" is not evidence that none + // exists — the reasoning that produced blocker 1 in the first place. + const integration = await Integration.create({ + podId: new mongoose.Types.ObjectId(), + type: 'discord', + status: 'connected', + isActive: true, + createdBy: new mongoose.Types.ObjectId(), + config: { serverId: 'srv', channelId: 'chan' }, + }); + const ref = await put( + String(integration._id), + { kind: 'future-credential', provider: 'future' }, + 'material', + ); + const now = await backdateSecret(ref); + + const swept = await sweep(now); + + expect(swept.orphanedSecrets).toBe(0); + expect(await ConnectorSecret.findById(ref)).not.toBeNull(); + }); + it('completes a stale activating row with its already-active, same-generation projection', async () => { const { userId, podId } = ids(); const result = await install({ installableId: 'telegram', installedBy: userId, podId }); diff --git a/backend/__tests__/unit/utils/discordWebhookUrl.test.js b/backend/__tests__/unit/utils/discordWebhookUrl.test.js new file mode 100644 index 0000000000..4261935d1d --- /dev/null +++ b/backend/__tests__/unit/utils/discordWebhookUrl.test.js @@ -0,0 +1,92 @@ +/** + * TASK-124 part 2 — the Discord webhook URL is stored encrypted, and this file + * pins the one definition of how it is resolved. + * + * The URL is a bearer credential (it embeds the webhook's own token), so the + * writers put it in the connector-secret envelope and the row keeps a ref. Two + * plaintext stores survive on rows connected before the migration — the platform + * document's `webhookUrl` and the legacy `Integration.config.webhookUrl` — and + * they are read here as FALLBACKS behind the ref. + * + * The case that carries the weight is the last one: a ref that cannot be read + * must THROW rather than fall through to a plaintext copy. Falling back would + * keep a connector sending to a URL nothing manages, and a webhook URL that is + * wrong does not fail authentication — it authenticates as a different + * connector. A missing secret is loud; a substituted one is not (vera 74141). + */ +const mongoose = require('mongoose'); + +const { resolveDiscordWebhookUrl, hasDiscordWebhookUrl } = require('../../../utils/discordWebhookUrl'); +const { put, revoke } = require('../../../services/connectorSecrets'); +const { DISCORD_WEBHOOK_URL } = require('../../../services/connectorSecretKinds'); +const { setupMongoDb, closeMongoDb, clearMongoDb } = require('../../utils/testUtils'); + +const URL_A = 'https://discord.com/api/webhooks/111/token-aaa'; +const URL_B = 'https://discord.com/api/webhooks/222/token-bbb'; +const URL_C = 'https://discord.com/api/webhooks/333/token-ccc'; + +const key = (fill) => Buffer.alloc(32, fill).toString('base64'); + +describe('resolveDiscordWebhookUrl', () => { + const savedKeys = process.env.CONNECTOR_SECRET_KEYS; + const savedActiveKey = process.env.CONNECTOR_SECRET_ACTIVE_KEY; + + beforeAll(async () => { + await setupMongoDb(); + }); + afterAll(async () => { + await closeMongoDb(); + process.env.CONNECTOR_SECRET_KEYS = savedKeys; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = savedActiveKey; + }); + beforeEach(async () => { + await clearMongoDb(); + process.env.CONNECTOR_SECRET_KEYS = `k1:${key(1)}`; + process.env.CONNECTOR_SECRET_ACTIVE_KEY = 'k1'; + }); + + it('reads through the encrypted ref, which wins over a plaintext copy beside it', async () => { + const ref = await put(new mongoose.Types.ObjectId().toString(), DISCORD_WEBHOOK_URL, URL_A); + + await expect(resolveDiscordWebhookUrl(ref, URL_B, URL_C)).resolves.toBe(URL_A); + }); + + it('falls back to the platform plaintext store, then to the legacy config one', async () => { + await expect(resolveDiscordWebhookUrl(undefined, URL_B, URL_C)).resolves.toBe(URL_B); + await expect(resolveDiscordWebhookUrl(undefined, undefined, URL_C)).resolves.toBe(URL_C); + }); + + it('treats an empty ref as absent, the same way the writers do', async () => { + await expect(resolveDiscordWebhookUrl('', URL_B)).resolves.toBe(URL_B); + }); + + it('answers undefined when there is no webhook anywhere', async () => { + await expect(resolveDiscordWebhookUrl(undefined, '', null)).resolves.toBeUndefined(); + }); + + it('THROWS on a ref it cannot read instead of using the plaintext copy behind it', async () => { + const ref = await put(new mongoose.Types.ObjectId().toString(), DISCORD_WEBHOOK_URL, URL_A); + await revoke(ref); + + await expect(resolveDiscordWebhookUrl(ref, URL_B)).rejects.toThrow(); + }); + + it('throws rather than returning an empty value when a stored secret resolves to nothing', async () => { + // Reachable through the trim: `put` refuses an empty string, not a blank one, + // so a blank stored value is the shape a corrupt or hand-edited row has. + const ref = await put(new mongoose.Types.ObjectId().toString(), DISCORD_WEBHOOK_URL, ' '); + + await expect(resolveDiscordWebhookUrl(ref)).rejects.toThrow(/empty value/); + }); + + it('hasDiscordWebhookUrl answers presence without decrypting or needing the ring', async () => { + const ref = await put(new mongoose.Types.ObjectId().toString(), DISCORD_WEBHOOK_URL, URL_A); + process.env.CONNECTOR_SECRET_KEYS = ''; + + // A ref whose key is gone still counts as present: the sweep's job is to mark + // the row unavailable, and a presence test is not the place to discover it. + expect(hasDiscordWebhookUrl(ref)).toBe(true); + expect(hasDiscordWebhookUrl(undefined, URL_B)).toBe(true); + expect(hasDiscordWebhookUrl(undefined, '', null)).toBe(false); + }); +}); diff --git a/backend/integrations/providers/discordProvider.ts b/backend/integrations/providers/discordProvider.ts index f4e6bff2b1..a5d76772f9 100644 --- a/backend/integrations/providers/discordProvider.ts +++ b/backend/integrations/providers/discordProvider.ts @@ -4,13 +4,16 @@ const DiscordService = require('../../services/discordService'); const { manifests } = require('../manifests'); // eslint-disable-next-line global-require const { resolveDiscordBotToken } = require('../../utils/discordBotToken'); +// eslint-disable-next-line global-require +const { resolveDiscordWebhookUrl } = require('../../utils/discordWebhookUrl'); interface IntegrationDoc { _id: unknown; - config?: Record; + config?: Record & { webhookUrlRef?: string; webhookUrl?: string }; platformIntegration?: { toObject?: () => Record; botToken?: string; + webhookUrl?: string; [key: string]: unknown; }; } @@ -67,8 +70,19 @@ function createDiscordProvider(integration: IntegrationDoc): DiscordProvider { async validateConfig() { try { validateRequiredConfig(config, manifests.discord); - if (config.webhookUrl) { - await DiscordService.validateConfig(config); + // The URL is encrypted, so it is resolved here rather than carried in + // `config`: on a migrated row the merged plaintext is gone and the ref + // is the only store, and a presence check on the merged value would + // silently skip validation altogether. Resolution can throw (an + // unreadable key ring, a revoked ref) and the catch below turns that + // into the ValidationError this method already speaks. + const webhookUrl = await resolveDiscordWebhookUrl( + integration?.config?.webhookUrlRef, + integration?.platformIntegration?.webhookUrl, + integration?.config?.webhookUrl, + ); + if (webhookUrl) { + await DiscordService.validateConfig({ ...config, webhookUrl }); } } catch (err) { const e = err as { message?: string }; diff --git a/backend/models/ConnectorSecret.ts b/backend/models/ConnectorSecret.ts index 91b8f1fb03..8951de64d3 100644 --- a/backend/models/ConnectorSecret.ts +++ b/backend/models/ConnectorSecret.ts @@ -3,11 +3,18 @@ import mongoose, { Document, Model, Schema, Types } from 'mongoose'; /** * Encrypted connector credential material. * - * Integration rows keep only this document's id in `config.botTokenRef`; the - * token itself must never be serialised through an Integration route. + * Integration rows keep only this document's id in a ref field + * (`config.botTokenRef`, `config.webhookUrlRef`); the material itself must never + * be serialised through an Integration route. + * + * A row is identified by `(integrationId, kind)`, not by `integrationId` alone: + * one connector can hold more than one kind, and the two-field key is what stops + * the second `put` from overwriting the first. `kind` and the ref paths that + * point back here are declared together in `services/connectorSecretKinds`. */ export interface IConnectorSecret extends Document { integrationId: Types.ObjectId; + kind: string; provider: string; ciphertext: string; iv: string; @@ -23,8 +30,10 @@ const ConnectorSecretSchema = new Schema( type: Schema.Types.ObjectId, ref: 'Integration', required: true, - unique: true, }, + // The credential's role on the row, so a connector holding two kinds keeps + // two secrets. Read from `services/connectorSecretKinds`; never a literal. + kind: { type: String, required: true }, provider: { type: String, required: true }, ciphertext: { type: String, required: true }, // Base64-encoded 96-bit AES-GCM nonce and authentication tag. @@ -35,7 +44,11 @@ const ConnectorSecretSchema = new Schema( { timestamps: true, collection: 'connector_secrets' }, ); -ConnectorSecretSchema.index({ integrationId: 1 }, { unique: true }); +// One index, on the pair. `integrationId` alone was declared unique twice here +// (a field-level `unique: true` and a schema index) and `syncIndexes()` kept the +// old one by name, so adding `kind` to only one of them would have left the boot +// path enforcing the single-field key (TASK-124, wren 74127). +ConnectorSecretSchema.index({ integrationId: 1, kind: 1 }, { unique: true }); const ConnectorSecret: Model = (mongoose.models.ConnectorSecret as Model) diff --git a/backend/models/DiscordIntegration.ts b/backend/models/DiscordIntegration.ts index 277e3e1d5f..32753a34c6 100644 --- a/backend/models/DiscordIntegration.ts +++ b/backend/models/DiscordIntegration.ts @@ -20,7 +20,8 @@ export interface IDiscordIntegration extends Document { serverName: string; channelId: string; channelName: string; - webhookUrl: string; + /** Legacy plaintext. Migrated rows carry only `config.webhookUrlRef`. */ + webhookUrl?: string; webhookId: string; botToken: string; permissions: DiscordPermission[]; @@ -39,7 +40,13 @@ const DiscordIntegrationSchema = new Schema( serverName: { type: String, required: true }, channelId: { type: String, required: true }, channelName: { type: String, required: true }, - webhookUrl: { type: String, required: true }, + // Optional since TASK-124: the URL is a bearer credential (it embeds the + // webhook's own token), so writers put it in the connector-secret envelope + // and store the ref at `Integration.config.webhookUrlRef`. This field stays + // declared because rows connected before the migration still carry one, and + // `utils/discordWebhookUrl` reads it as a FALLBACK behind the ref. The + // migration (`scripts/encrypt-discord-webhook-url`) unsets it. + webhookUrl: { type: String, required: false }, webhookId: { type: String, required: true }, // Optional: no writer sets it any more (TASK-124). Legacy rows still carry // a value, which is why the field stays declared rather than removed — @@ -73,6 +80,9 @@ DiscordIntegrationSchema.index({ integrationId: 1 }, { unique: true }); DiscordIntegrationSchema.index({ serverId: 1, channelId: 1 }); DiscordIntegrationSchema.index({ webhookId: 1 }); +// Fires only when a plaintext URL is present, so a migrated row (ref only) is +// not validated against a value it no longer stores. The ref is opaque on +// purpose and must never be format-checked here. DiscordIntegrationSchema.pre('save', function (next) { if (this.webhookUrl && !this.webhookUrl.includes('discord.com/api/webhooks/')) { return next(new Error('Invalid Discord webhook URL format')); @@ -89,9 +99,12 @@ DiscordIntegrationSchema.virtual('recentMessages').get(function (this: IDiscordI // The record joins onto Integration as `platformIntegration` and rides out // through every list that populates it (admin Apps list, pod list, create). // webhookUrl carries the webhook's secret and stays server-only in every JSON -// response; `botToken` is deleted for the same reason and because it is a -// legacy field no writer sets any more (TASK-124). Server code reads the token -// from the environment, never off this document's JSON. +// response — the plaintext copy where it survives the migration and the +// `config.webhookUrlRef` pointer to the encrypted one, which is stripped by +// `toPublicIntegrationConfig`; `botToken` is deleted for the same reason and +// because it is a legacy field no writer sets any more (TASK-124). Server code +// reads the token from the environment and the webhook URL through +// `utils/discordWebhookUrl`, never off this document's JSON. DiscordIntegrationSchema.set('toJSON', { virtuals: true, transform: (_doc: unknown, returned: Record) => { diff --git a/backend/models/Integration.ts b/backend/models/Integration.ts index e106b584ce..0bc0239d00 100644 --- a/backend/models/Integration.ts +++ b/backend/models/Integration.ts @@ -73,6 +73,9 @@ export interface IIntegration extends Document { channelName?: string; channelUrl?: string; webhookUrl?: string; + // The pointer to the encrypted copy, written by the Discord writers. `webhookUrl` + // beside it is the legacy plaintext the migration unsets. + webhookUrlRef?: string; botToken?: string; signingSecret?: string; secretToken?: string; @@ -216,6 +219,12 @@ const IntegrationSchema = new Schema( channelName: String, channelUrl: String, webhookUrl: String, + // The pointer to the encrypted webhook URL (the connector-secret envelope). + // Declared, not forgotten: `config` is a STRICT subdocument, so a `$set` of + // an undeclared path here is dropped in silence — the row would keep no ref + // at all and every reader would fall through to a plaintext field that the + // migration had already unset (TASK-124 part 2). + webhookUrlRef: String, botToken: String, signingSecret: String, secretToken: String, diff --git a/backend/models/integrationPublicConfig.ts b/backend/models/integrationPublicConfig.ts index 0eb12a43ae..d949634b4c 100644 --- a/backend/models/integrationPublicConfig.ts +++ b/backend/models/integrationPublicConfig.ts @@ -28,6 +28,9 @@ export const INTEGRATION_SECRET_CONFIG_KEYS = [ 'refreshToken', 'webhookUrl', 'botTokenRef', + // The pointer to the encrypted Discord webhook URL. Listed with the ref it + // parallels: a browser is given neither the secret nor its address. + 'webhookUrlRef', 'oauthStateNonce', 'oauthStateClaimId', ] as const; diff --git a/backend/routes/installables.ts b/backend/routes/installables.ts index 81c59357a1..65749772d6 100644 --- a/backend/routes/installables.ts +++ b/backend/routes/installables.ts @@ -18,6 +18,8 @@ const { mintConnectCode } = require('../services/telegramConnectCode'); // eslint-disable-next-line global-require const connectorSecrets = require('../services/connectorSecrets'); // eslint-disable-next-line global-require +const { SLACK_BOT_TOKEN } = require('../services/connectorSecretKinds'); +// eslint-disable-next-line global-require const connectorDeliveryFailures = require('../services/connectorDeliveryFailureService'); // eslint-disable-next-line global-require const { @@ -322,7 +324,7 @@ const slackOAuthCallback = async (req: AuthReq, res: Res) => { const { accessToken, ...binding } = await exchangeCode(code); const dm = await new SlackApi(accessToken).openConversation(binding.slackUserId); if (!dm.ok || !dm.channel?.id) throw new SlackOAuthExchangeError(); - const botTokenRef = await connectorSecrets.put(String(integration._id), 'slack', accessToken); + const botTokenRef = await connectorSecrets.put(String(integration._id), SLACK_BOT_TOKEN, accessToken); const committed = await Integration.findOneAndUpdate( { _id: integration._id, diff --git a/backend/routes/integrations.ts b/backend/routes/integrations.ts index d468d146cb..a5b666930e 100644 --- a/backend/routes/integrations.ts +++ b/backend/routes/integrations.ts @@ -15,6 +15,10 @@ const DiscordIntegration = require('../models/DiscordIntegration'); // eslint-disable-next-line global-require const DiscordService = require('../services/discordService'); // eslint-disable-next-line global-require +const connectorSecrets = require('../services/connectorSecrets'); +// eslint-disable-next-line global-require +const { DISCORD_WEBHOOK_URL } = require('../services/connectorSecretKinds'); +// eslint-disable-next-line global-require const Pod = require('../models/Pod'); // eslint-disable-next-line global-require const User = require('../models/User'); @@ -69,6 +73,15 @@ const SERVER_OWNED_CONFIG_KEYS = [ // run FIRST (they precede this strip on both routes), so a supplied Discord // token is a 400 rather than a silent 200. 'botToken', + // The Discord channel webhook URL is a bearer credential of its own — the URL + // embeds the webhook's token, so posting to it posts AS that channel — and the + // server derives it from the Discord API on both writers (`routes/integrations` + // at connect, `services/discordService` on a backfill). No browser sends it, and + // a caller-planted value would be read as the legacy fallback in + // `utils/discordWebhookUrl`. `webhookUrlRef` is the pointer to the encrypted + // copy: like the Slack ref above, accepting it from a body would let a caller + // point their row at another row's secret. + 'webhookUrl', 'webhookUrlRef', // An administrator's pause is projected from the parent installation. An // owner's normal config write must never lift that stop. 'adminPause', @@ -446,10 +459,20 @@ router.post('/', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res if (type === 'discord') { const webhookResponse = await axios.post(`https://discord.com/api/channels/${config.channelId}/webhooks`, { name: 'Commonly Bot', avatar: null }, { headers: { Authorization: `Bot ${process.env.DISCORD_BOT_TOKEN}`, 'Content-Type': 'application/json' } }); const webhook = webhookResponse.data as { id: string; token: string }; - // No botToken copy. The token is instance-wide and read from the - // environment on every use; storing it here is what made a rotation miss - // integrations that already existed (TASK-124). - platformIntegration = new DiscordIntegration({ integrationId: integration._id, serverId: config.serverId, serverName: config.serverName, channelId: config.channelId, channelName: config.channelName, webhookUrl: `https://discord.com/api/webhooks/${webhook.id}/${webhook.token}`, webhookId: webhook.id, permissions: config.permissions || ['read_messages', 'send_messages'] }); + // The webhook URL is a bearer credential (it embeds the token), so it goes + // into the connector-secret envelope and the row keeps only the ref — the + // shape the Slack bind already uses for its bot token. `webhookId` is not a + // secret and stays on the platform document. No botToken copy either: that + // token is instance-wide and read from the environment on every use, and + // storing it is what made a rotation miss integrations that already existed + // (TASK-124). + const webhookUrlRef = await connectorSecrets.put( + String(integration._id), + DISCORD_WEBHOOK_URL, + `https://discord.com/api/webhooks/${webhook.id}/${webhook.token}`, + ); + await Integration.findByIdAndUpdate(integration._id, { $set: { 'config.webhookUrlRef': webhookUrlRef } }); + platformIntegration = new DiscordIntegration({ integrationId: integration._id, serverId: config.serverId, serverName: config.serverName, channelId: config.channelId, channelName: config.channelName, webhookId: webhook.id, permissions: config.permissions || ['read_messages', 'send_messages'] }); await platformIntegration.save(); } else if (['slack', 'groupme', 'telegram', 'messenger', 'whatsapp', 'x', 'instagram'].includes(type)) { integration.status = isManifestComplete(type, nextConfig) ? 'connected' : 'pending'; diff --git a/backend/scripts/encrypt-discord-webhook-url.ts b/backend/scripts/encrypt-discord-webhook-url.ts new file mode 100644 index 0000000000..cd2404e69a --- /dev/null +++ b/backend/scripts/encrypt-discord-webhook-url.ts @@ -0,0 +1,197 @@ +#!/usr/bin/env node +/* + * Encrypt the per-row Discord webhook URLs at rest (TASK-124 part 2). + * + * `discord_integrations.webhookUrl` is a bearer credential: the URL embeds the + * webhook's own token, so anyone holding it can post into the channel as that + * webhook. It was the last connector credential stored in plaintext outside the + * ESO path — the Slack bot token has lived in `connector_secrets` (AES-256-GCM, + * key ring from `CONNECTOR_SECRET_KEYS` / `CONNECTOR_SECRET_ACTIVE_KEY`) since the + * Slack bind landed. Part 2 routes every writer through that envelope and stores + * only the ref, at `Integration.config.webhookUrlRef`, and every reader through + * `utils/discordWebhookUrl`. + * + * This script is the data half. It runs AFTER the deploy that reads through the + * resolver (the old image read the plaintext field directly, so clearing first + * would break it), and it is idempotent: a second run finds no plaintext left. + * + * Both plaintext stores are read, because a row connected before this change can + * hold either: the platform document's `webhookUrl` (the writer that derived it + * from the Discord API) and the legacy `Integration.config.webhookUrl` (which a + * caller's body could set with an empty string, and which the merged effective + * config surfaced). A non-empty value in either is a candidate; the run unsets + * both and writes one encrypted ref. + * + * Two reported counts are deliberately NOT actions: + * + * - `unexpectedFormat`: a value that is not a `discord.com/api/webhooks/` URL. + * It is still encrypted — the point of the migration is to stop the value + * being readable at rest, not to judge it, and encrypting preserves whatever + * the row was already doing. + * - `alreadyEncrypted`: a row that carries a ref and no plaintext, i.e. one + * already migrated (or written after the deploy). A second run should report + * all rows here and `migrated=0`. + * + * The default is a dry run that logs each distinct URL as a digest, never the URL + * itself. Run with `--apply` to write. Needs MONGO_URI and the connector-secret + * key ring in the environment; run it inside the backend pod, where both live. + */ + +/* eslint-disable no-console */ +const crypto = require('crypto'); +const mongoose = require('mongoose'); + +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const connectorSecrets = require('../services/connectorSecrets'); +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { DISCORD_WEBHOOK_URL } = require('../services/connectorSecretKinds'); + +const DISCORD_COLLECTION = 'discord_integrations'; +const INTEGRATION_COLLECTION = 'integrations'; + +export interface DiscordWebhookUrlEncryptResult { + /** Discord integration rows examined. */ + rows: number; + /** Rows holding a non-empty plaintext URL in either store. */ + candidates: number; + /** Rows whose plaintext was replaced by an encrypted ref (0 on a dry run). */ + migrated: number; + /** Rows already carrying a ref and no plaintext. */ + alreadyEncrypted: number; + /** Rows with neither a ref nor a plaintext URL — nothing to encrypt. */ + withoutWebhook: number; + /** Candidates whose value is not a `discord.com/api/webhooks/` URL; encrypted anyway, reported. */ + unexpectedFormat: number; + /** Candidates whose plaintext came from `Integration.config.webhookUrl` rather than the platform document. */ + configCopies: number; + /** One digest per candidate: identifies a URL without reproducing it. */ + digests: string[]; +} + +const describeUrl = (url: string): string => { + const digest = crypto.createHash('sha256').update(url).digest('hex').slice(0, 12); + return `${digest} (${url.length} chars)`; +}; + +export async function encryptDiscordWebhookUrls( + { apply = false }: { apply?: boolean } = {}, +): Promise { + const discord = mongoose.connection.collection(DISCORD_COLLECTION); + const integrations = mongoose.connection.collection(INTEGRATION_COLLECTION); + const rows = await discord + .find({}, { projection: { _id: 1, integrationId: 1, webhookUrl: 1 } }) + .toArray(); + + // Fail before the first write if the ring cannot encrypt anything: an --apply + // that unset a plaintext and could not store a ref would destroy the only copy + // of a live credential. + if (apply) await connectorSecrets.listWithUnavailableKey(); + + let candidates = 0; + let migrated = 0; + let alreadyEncrypted = 0; + let withoutWebhook = 0; + let unexpectedFormat = 0; + let configCopies = 0; + const digests: string[] = []; + + for (const row of rows) { + const integration = await integrations.findOne( + { _id: row.integrationId }, + { projection: { config: 1 } }, + ); + const storedPlaintext = String(row.webhookUrl || '').trim(); + const configPlaintext = String(integration?.config?.webhookUrl || '').trim(); + const existingRef = String(integration?.config?.webhookUrlRef || '').trim(); + + if (!storedPlaintext && !configPlaintext) { + if (existingRef) alreadyEncrypted += 1; + else withoutWebhook += 1; + continue; + } + + const url = storedPlaintext || configPlaintext; + candidates += 1; + if (!storedPlaintext) configCopies += 1; + if (!url.includes('discord.com/api/webhooks/')) unexpectedFormat += 1; + digests.push(describeUrl(url)); + + if (!apply) continue; + + const webhookUrlRef = await connectorSecrets.put( + String(row.integrationId), + DISCORD_WEBHOOK_URL, + url, + ); + await integrations.updateOne( + { _id: row.integrationId }, + { + $set: { 'config.webhookUrlRef': webhookUrlRef }, + $unset: { 'config.webhookUrl': '' }, + }, + ); + await discord.updateOne({ _id: row._id }, { $unset: { webhookUrl: '' } }); + migrated += 1; + } + + return { + rows: rows.length, + candidates, + migrated, + alreadyEncrypted, + withoutWebhook, + unexpectedFormat, + configCopies, + digests, + }; +} + +/** + * The operator-facing report, returned as lines rather than printed inline so the + * wording is witnessable — the same reason the token-clear script exports its own + * (`backend/__tests__/unit/scripts/clearDiscordBotToken.test.ts`). + */ +export const formatReport = ( + result: DiscordWebhookUrlEncryptResult, + apply: boolean, +): string[] => { + const lines = [ + `[encrypt-discord-webhook-url] ${apply ? 'APPLY' : 'DRY-RUN'} ` + + `rows=${result.rows} candidates=${result.candidates} migrated=${result.migrated}`, + ...result.digests.map((digest) => ` plaintext webhook ${digest}`), + '[encrypt-discord-webhook-url] already encrypted (ref, no plaintext): ' + + `${result.alreadyEncrypted}`, + '[encrypt-discord-webhook-url] no webhook at all: ' + + `${result.withoutWebhook}`, + '[encrypt-discord-webhook-url] plaintext found in Integration.config.webhookUrl ' + + `(the legacy second store): ${result.configCopies}`, + '[encrypt-discord-webhook-url] candidates not matching the Discord webhook URL shape ' + + `(encrypted anyway): ${result.unexpectedFormat}`, + ]; + if (!apply) lines.push('DRY RUN — nothing written. Re-run with --apply.'); + return lines; +}; + +async function main(): Promise { + const apply = process.argv.includes('--apply'); + const mongoUri = process.env.MONGO_URI; + if (!mongoUri) { + console.error('MONGO_URI is required'); + process.exit(1); + } + + await mongoose.connect(mongoUri); + try { + const result = await encryptDiscordWebhookUrls({ apply }); + formatReport(result, apply).forEach((line) => console.log(line)); + } finally { + await mongoose.disconnect(); + } +} + +if (require.main === module) { + main().catch((error) => { + console.error(error); + process.exit(1); + }); +} diff --git a/backend/services/connectorSecretKinds.ts b/backend/services/connectorSecretKinds.ts new file mode 100644 index 0000000000..bf8c1ad77d --- /dev/null +++ b/backend/services/connectorSecretKinds.ts @@ -0,0 +1,93 @@ +/** + * Every kind of encrypted connector credential, in one place. + * + * A secret's identity is the PAIR `(integrationId, kind)`: one connector row can + * hold more than one kind (a Slack connector keeps its bot token, a Discord + * connector its webhook URL), and each kind's ref lives at a different place on + * the owning row. Four sites encode that pair, and all four read THIS module — + * which is the point, because three of them fail in ways that do not raise: + * + * 1. `models/ConnectorSecret` — the compound unique index. Leave a + * single-field `integrationId` index beside it and boot's `syncIndexes()` + * keeps the old one, so the second kind is rejected by an index whose name + * no longer describes it. + * 2. `services/connectorSecrets` — the upsert filter *and* the 11000 retry's + * filter. Filtered on `integrationId` alone, the second `put` does not + * collide: it matches the first kind's row and `$set`s the new ciphertext + * onto it, so the first ref stays valid and starts decrypting to the second + * secret. A missing secret is loud; a substituted one is not (Vera 74141). + * 3. `installableReconciler.sweepOrphanedConnectorSecrets` — its keep-condition + * is `refPaths`. With a literal `botTokenRef` there, a Discord secret is + * revoked ten minutes after it is written (`ORPHAN_SECRET_GRACE_MS`), + * which is the defect this module exists for (Vera 74130). + * 4. `installableReconciler.sweepUnavailableConnectorSecretKeys` — the message + * it marks the row with is `unavailableReason`, so a Discord owner is not + * told a Slack secret is unavailable (Vera 74131). + * + * Adding a kind means adding a spec here and nothing else. + */ + +export interface ConnectorSecretKindSpec { + /** Stored on the secret row; the discriminator of the compound key. */ + kind: string; + /** Stored on the secret row; the `Integration.type` that owns this kind. */ + provider: string; + /** + * Dotted paths on the owning `Integration` row that hold this kind's ref. + * Read by the orphan sweep's keep-condition, so EVERY path a writer can set + * must be named here — a path this list omits is a secret the sweep deletes. + */ + refPaths: readonly string[]; + /** The sentence written for the person reading the Connectors page. */ + unavailableReason: string; +} + +/** The Slack bot token: the ref lives on the row the OAuth bind writes. */ +export const SLACK_BOT_TOKEN: ConnectorSecretKindSpec = { + kind: 'slack-bot-token', + provider: 'slack', + refPaths: ['config.botTokenRef', 'config.pendingBind.botTokenRef'], + unavailableReason: 'Slack connector secret key is unavailable', +}; + +/** + * The Discord channel webhook URL. It is a bearer credential on its own — the + * URL embeds the token that authorises posting to the channel — so it is stored + * the same way and read through `utils/discordWebhookUrl`. + */ +export const DISCORD_WEBHOOK_URL: ConnectorSecretKindSpec = { + kind: 'discord-webhook-url', + provider: 'discord', + refPaths: ['config.webhookUrlRef'], + unavailableReason: 'Discord connector webhook is unavailable', +}; + +export const CONNECTOR_SECRET_KINDS: readonly ConnectorSecretKindSpec[] = [ + SLACK_BOT_TOKEN, + DISCORD_WEBHOOK_URL, +]; + +export const kindSpec = (kind: string): ConnectorSecretKindSpec | undefined => ( + CONNECTOR_SECRET_KINDS.find((spec) => spec.kind === kind) +); + +/** Every ref path the sweeps must inspect, across all kinds. */ +export const allRefPaths = (): string[] => CONNECTOR_SECRET_KINDS.flatMap((spec) => [...spec.refPaths]); + +const valueAtPath = (row: unknown, path: string): unknown => path + .split('.') + .reduce((value, key) => ( + value && typeof value === 'object' ? (value as Record)[key] : undefined + ), row); + +/** + * Does this row still reference `ref` through one of `spec`'s paths? + * + * An unknown kind answers `false` for every path, which is why the caller must + * decide what to do with it rather than treating this as "unreferenced". + */ +export const rowReferencesSecret = ( + row: unknown, + spec: ConnectorSecretKindSpec, + ref: string, +): boolean => spec.refPaths.some((path) => String(valueAtPath(row, path) ?? '') === ref); diff --git a/backend/services/connectorSecrets.ts b/backend/services/connectorSecrets.ts index 7dff55a600..d73f52860c 100644 --- a/backend/services/connectorSecrets.ts +++ b/backend/services/connectorSecrets.ts @@ -6,6 +6,7 @@ import { import { Types } from 'mongoose'; import type { IConnectorSecret } from '../models/ConnectorSecret'; +import type { ConnectorSecretKindSpec } from './connectorSecretKinds'; // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const ConnectorSecret = require('../models/ConnectorSecret'); @@ -114,30 +115,38 @@ const asObjectId = (integrationId: string): Types.ObjectId => { const upsertEncrypted = async ( integrationId: Types.ObjectId, - provider: string, + spec: ConnectorSecretKindSpec, encrypted: Pick, ): Promise => { + // The filter carries `kind` as well as `integrationId`, on BOTH the upsert and + // the retry. Filtered on `integrationId` alone the second kind does not + // collide — it MATCHES the first kind's row and $sets this ciphertext onto it, + // so the first ref stays valid and silently starts decrypting to the second + // secret (vera 74141). `kind` is read from the caller's spec, and the index in + // `models/ConnectorSecret` is on the same pair. + const filter = { integrationId, kind: spec.kind }; + const update = { $set: { provider: spec.provider, ...encrypted } }; try { return await ConnectorSecret.findOneAndUpdate( - { integrationId }, - { $set: { provider, ...encrypted } }, + filter, + update, { new: true, upsert: true, setDefaultsOnInsert: true }, ) as IConnectorSecret; } catch (error) { // Concurrent OAuth callbacks may race the first upsert. The unique - // integration key makes the loser retry as an ordinary update. + // (integrationId, kind) key makes the loser retry as an ordinary update. if ((error as { code?: number }).code !== 11000) throw error; - return ConnectorSecret.findOneAndUpdate( - { integrationId }, - { $set: { provider, ...encrypted } }, - { new: true }, - ) as Promise; + return ConnectorSecret.findOneAndUpdate(filter, update, { new: true }) as Promise; } }; -export const put = async (integrationId: string, provider: string, material: string): Promise => { +export const put = async ( + integrationId: string, + spec: ConnectorSecretKindSpec, + material: string, +): Promise => { if (!material) throw new ConnectorSecretConfigurationError('Connector secret material must not be empty.'); - const secret = await upsertEncrypted(asObjectId(integrationId), provider, encrypt(material, parseKeyRing())); + const secret = await upsertEncrypted(asObjectId(integrationId), spec, encrypt(material, parseKeyRing())); return String(secret._id); }; diff --git a/backend/services/discordService.ts b/backend/services/discordService.ts index ef5304b466..b27e201584 100644 --- a/backend/services/discordService.ts +++ b/backend/services/discordService.ts @@ -14,6 +14,12 @@ const summarizerService = require('./summarizerService'); const config = require('../config/discord'); // eslint-disable-next-line global-require const { resolveDiscordBotToken } = require('../utils/discordBotToken'); +// eslint-disable-next-line global-require +const { resolveDiscordWebhookUrl, hasDiscordWebhookUrl } = require('../utils/discordWebhookUrl'); +// eslint-disable-next-line global-require +const connectorSecrets = require('./connectorSecrets'); +// eslint-disable-next-line global-require +const { DISCORD_WEBHOOK_URL } = require('./connectorSecretKinds'); interface FetchMessagesOptions { channelId?: string; @@ -50,6 +56,10 @@ interface IntegrationDoc { channelId?: string; botToken?: string; serverId?: string; + // The pointer to the encrypted webhook URL. `webhookUrl` beside it is the + // legacy plaintext copy; both are resolved by `utils/discordWebhookUrl`. + webhookUrlRef?: string; + webhookUrl?: string; webhookListenerEnabled?: boolean; messageBuffer?: BufferedMessage[]; lastSummaryAt?: Date; @@ -316,15 +326,28 @@ class DiscordService { throw new Error(config.errors.CHANNEL_NOT_FOUND); } - if (!(this.integration as IntegrationDoc).platformIntegration?.webhookUrl) { + if (!hasDiscordWebhookUrl( + (this.integration as IntegrationDoc).config?.webhookUrlRef, + (this.integration as IntegrationDoc).platformIntegration?.webhookUrl, + (this.integration as IntegrationDoc).config?.webhookUrl, + )) { const webhook = await this.createWebhook(channel.id) as { url: string; id: string }; + // The URL is a credential (it embeds the webhook's own token), so the + // backfill stores it exactly like the connect path does: encrypted in + // the connector-secret envelope, with only the ref on the row. The + // platform document keeps `webhookId`, which is not a secret. + const webhookUrlRef = await connectorSecrets.put( + String(this.integrationId), + DISCORD_WEBHOOK_URL, + webhook.url, + ); + await Integration.findByIdAndUpdate(this.integrationId, { + $set: { 'config.webhookUrlRef': webhookUrlRef }, + }); await DiscordIntegration.findByIdAndUpdate( (this.integration as IntegrationDoc).platformIntegration?._id, - { - webhookUrl: webhook.url, - webhookId: webhook.id, - }, + { webhookId: webhook.id }, ); } @@ -384,7 +407,15 @@ class DiscordService { async sendMessage(message: string): Promise { try { - if (!(this.integration as IntegrationDoc).platformIntegration?.webhookUrl) { + // Resolved through the one precedence definition: the encrypted ref wins, + // the two legacy plaintext stores are fallbacks. An unresolvable ref throws + // rather than falling back — see utils/discordWebhookUrl. + const webhookUrl = await resolveDiscordWebhookUrl( + (this.integration as IntegrationDoc).config?.webhookUrlRef, + (this.integration as IntegrationDoc).platformIntegration?.webhookUrl, + (this.integration as IntegrationDoc).config?.webhookUrl, + ); + if (!webhookUrl) { throw new Error('Webhook URL not found'); } @@ -401,7 +432,7 @@ class DiscordService { } const response = await fetch( - (this.integration as IntegrationDoc).platformIntegration?.webhookUrl as string, + webhookUrl, { method: 'POST', headers: { 'Content-Type': 'application/json' }, diff --git a/backend/services/installable/installableReconciler.ts b/backend/services/installable/installableReconciler.ts index 9d4d50d698..c71c10f444 100644 --- a/backend/services/installable/installableReconciler.ts +++ b/backend/services/installable/installableReconciler.ts @@ -11,6 +11,13 @@ const Pod = require('../../models/Pod'); const ConnectorSecret = require('../../models/ConnectorSecret'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const connectorSecrets = require('../connectorSecrets'); +// The kinds registry is the single definition of where each kind's ref lives and +// what its unavailability is called. Both sweeps below read it — the orphan +// sweep's keep-condition was a literal `botTokenRef` pair, which revoked a +// Discord webhook secret ten minutes after it was written (vera 74130), and the +// unavailable sweep named Slack for any provider (vera 74131). +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { allRefPaths, kindSpec, rowReferencesSecret } = require('../connectorSecretKinds'); // The user-facing half of the error state: this module marks a row it cannot // decrypt, and the reason is written for the person reading the Connectors page // — the same flag the delivery-failure flip sets, so it comes from that service @@ -255,48 +262,62 @@ const sweepExpiredSlackBinds = async (now: Date): Promise => { const sweepOrphanedConnectorSecrets = async (now: Date): Promise => { const secrets = await ConnectorSecret.find({ createdAt: { $lte: new Date(now.getTime() - ORPHAN_SECRET_GRACE_MS) }, - }).select('_id integrationId createdAt').lean() as Array<{ + }).select('_id integrationId kind createdAt').lean() as Array<{ _id: unknown; integrationId: unknown; + kind?: string; createdAt: Date; }>; + // Every path any kind can store a ref at, selected once: the keep-condition is + // data drawn from the kinds registry, not a field name written here. + const select = ['isActive', ...allRefPaths()].join(' '); let revoked = 0; for (const secret of secrets) { + const spec = kindSpec(String(secret.kind ?? '')); + if (!spec) { + // An unrecognized kind is KEPT, not revoked. Nothing here knows where its + // ref lives, and "the sweep cannot find a reference" is not evidence that + // none exists — deleting on that reasoning is exactly how a Discord secret + // was being deleted while its row still pointed at it. + console.warn( + `[installable-reconciler] connector secret ${String(secret._id)} has unrecognized kind ` + + `'${String(secret.kind)}'; kept`, + ); + continue; + } const integration = await Integration.findById(secret.integrationId) - .select('isActive config.botTokenRef config.pendingBind.botTokenRef').lean() as { - isActive?: boolean; - config?: { botTokenRef?: string; pendingBind?: { botTokenRef?: string } }; - } | null; + .select(select).lean() as Record | null; const ref = String(secret._id); - if ( - integration?.isActive === true - && (integration.config?.botTokenRef === ref || integration.config?.pendingBind?.botTokenRef === ref) - ) continue; + if (integration?.isActive === true && rowReferencesSecret(integration, spec, ref)) continue; await connectorSecrets.revoke(ref); revoked += 1; } return revoked; }; -const sweepUnavailableSlackSecretKeys = async (): Promise => { +const sweepUnavailableConnectorSecretKeys = async (): Promise => { const secretCount = await ConnectorSecret.countDocuments(); if (!secretCount) return 0; - let unavailable: Array<{ integrationId: unknown }>; + let unavailable: Array<{ integrationId: unknown; kind?: string }>; try { unavailable = await connectorSecrets.listWithUnavailableKey(); } catch (error) { // A malformed or wholly absent ring makes every stored secret unreadable. // That is an error state, not an invitation to keep attempting sends. console.error('[installable-reconciler] connector secret key ring unavailable:', (error as Error).message); - unavailable = await ConnectorSecret.find({}).select('integrationId').lean(); + unavailable = await ConnectorSecret.find({}).select('integrationId kind').lean(); } for (const secret of unavailable) { const integration = await Integration.findById(secret.integrationId) .select('installationId').lean() as { _id: unknown; installationId?: string } | null; if (integration) { + // The reason names THIS secret's kind, so a Discord owner is not told that + // a Slack secret is unavailable (the string is user-facing: it is written + // through `userFacingError` and rendered on the Connectors page). + const spec = kindSpec(String(secret.kind ?? '')); await connectorDeliveryFailures.markConnectorUnavailable( integration, - 'Slack connector secret key is unavailable', + spec ? spec.unavailableReason : 'Connector credential is unavailable', ); } } @@ -314,7 +335,7 @@ export const sweep = async (now: Date = new Date()): Promise<{ deactivated: number; expiredSlackBinds: number; orphanedSecrets: number; - unavailableSlackSecretKeys: number; + unavailableConnectorSecretKeys: number; }> => { const locks = await sweepStaleLocks(now); const active = await sweepActiveInstallations(); @@ -324,7 +345,7 @@ export const sweep = async (now: Date = new Date()): Promise<{ const deactivated = await sweepUninstalledInstallations(); const expiredSlackBinds = await sweepExpiredSlackBinds(now); const orphanedSecrets = await sweepOrphanedConnectorSecrets(now); - const unavailableSlackSecretKeys = await sweepUnavailableSlackSecretKeys(); + const unavailableConnectorSecretKeys = await sweepUnavailableConnectorSecretKeys(); const result = { ...locks, staleComponents: active.staleComponents, @@ -335,7 +356,7 @@ export const sweep = async (now: Date = new Date()): Promise<{ deactivated, expiredSlackBinds, orphanedSecrets, - unavailableSlackSecretKeys, + unavailableConnectorSecretKeys, }; console.log('[installable-reconciler] sweep', result); return result; diff --git a/backend/utils/discordWebhookUrl.ts b/backend/utils/discordWebhookUrl.ts new file mode 100644 index 0000000000..79ac1b994d --- /dev/null +++ b/backend/utils/discordWebhookUrl.ts @@ -0,0 +1,55 @@ +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const connectorSecrets = require('../services/connectorSecrets'); + +/** + * Discord webhook URL resolution — the ENCRYPTED REF is authoritative. + * + * The URL embeds the webhook's own token, so it is a bearer credential: posting + * to it is posting AS that channel. It therefore lives as connector-secret + * material (`services/connectorSecrets`, kind `discord-webhook-url`, declared in + * `services/connectorSecretKinds`) and the row keeps only an opaque ref in + * `config.webhookUrlRef` — the same shape Slack's bot token already had. + * + * Two plaintext stores are still declared and still read as FALLBACKS: + * `platformIntegration.webhookUrl` and the legacy `config.webhookUrl`. Rows + * connected before the encryption migration (`scripts/encrypt-discord-webhook-url`) + * carry one; it unsets both, so on a migrated row only the ref exists. + * + * A ref that cannot be resolved THROWS rather than falling through to a + * plaintext copy. The two are not equivalents: falling back would mean a row + * whose secret was rotated or revoked keeps sending to a URL nothing manages — + * and a wrong webhook URL authenticates as the wrong connector, silently. That + * substitution is the defect this encryption exists to prevent (vera 74141). + * + * `hasDiscordWebhookUrl` is the presence test for call sites that only need to + * know whether a webhook exists (it must not decrypt, and must not throw on an + * unavailable key ring). + */ +export const resolveDiscordWebhookUrl = async ( + ref: unknown, + ...legacy: unknown[] +): Promise => { + const trimmedRef = String(ref ?? '').trim(); + if (trimmedRef) { + const material = await connectorSecrets.get(trimmedRef); + const resolved = String(material ?? '').trim(); + if (!resolved) { + // A decryptable-but-empty secret is a corrupt row, not an absent one. + throw new Error(`Connector secret '${trimmedRef}' resolved to an empty value.`); + } + return resolved; + } + for (const candidate of legacy) { + const value = String(candidate ?? '').trim(); + if (value) return value; + } + return undefined; +}; + +/** Presence of a webhook, without decrypting: the ref OR a legacy copy. */ +export const hasDiscordWebhookUrl = (ref: unknown, ...legacy: unknown[]): boolean => ( + String(ref ?? '').trim().length > 0 + || legacy.some((candidate) => String(candidate ?? '').trim().length > 0) +); + +export default resolveDiscordWebhookUrl; From d015ad25b8e26384ccc935ef6215dac2070f5b8c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:05:23 -0700 Subject: [PATCH 2/3] test(discord): witness the migration's write-before-unset ordering, and drop the inert pre-flight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vera's hold at `e6d4077c`: the ordering that prevents data loss in `encrypt-discord-webhook-url` was unwitnessed. Moving both `$unset`s above the `connectorSecrets.put` (the arrangement where a failing put destroys the only copy) left the suite 8/8, and deleting the `listWithUnavailableKey()` pre-flight reddened nothing. Both observations were right, and the second one was the more interesting: `listWithUnavailableKey` returns refs whose `keyId` is not in the ring. It cannot fail. A call that cannot fail is not a guard, so it is gone — the script no longer claims to fail fast, and its comment now states the invariant that actually protects the data: per row, the `put` completes before either plaintext is cleared, so ANY failure leaves that row whole and a re-run reports it as still waiting. Two witnesses replace the claim: - a failed `put` leaves both plaintexts in place and reports nothing migrated (asserts the platform store, the legacy `config.webhookUrl` store, the absence of a ref, and that a following dry run still sees `candidates=1, migrated=0`); - clearing happens only after the row's own ref is stored, one row further in: the second row's `put` rejects, so the first must have completed end to end (the test cannot pass vacuously) and the second must be untouched. The empty-ring test keeps its place as the ring-shaped instance, with its comment corrected: what protects it is the ordering, not the deleted pre-flight. Mutation ledger, script only: vera's mutation reddens 3 named tests (both new witnesses + the empty-ring one); removing the dry-run guard reddens 1 as a control that the ledger sees this file at all; re-adding the deleted pre-flight is a SURVIVOR by design, which is the evidence for removing it rather than witnessing it. --- .../scripts/encryptDiscordWebhookUrl.test.ts | 67 +++++++++++++++++-- .../scripts/encrypt-discord-webhook-url.ts | 17 +++-- 2 files changed, 75 insertions(+), 9 deletions(-) diff --git a/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts b/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts index 89da101d99..3bfff5036d 100644 --- a/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts +++ b/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts @@ -131,10 +131,69 @@ describe('encrypt-discord-webhook-url', () => { expect(await get(integration.config.webhookUrlRef)).toBe(URL_C); }); - it('refuses to write anything when the key ring is unusable', async () => { - // The destructive half of this migration is the $unset. A run that cannot - // encrypt must not reach it: losing the plaintext without storing a ref - // leaves the live credential unrecoverable. + it('a failed put leaves both plaintexts in place and reports nothing migrated', async () => { + // vera's hold on #1898: the ordering that prevents data loss was unwitnessed. + // The destructive half of this migration is the $unset, and the only thing + // between a failed put and an unrecoverable credential is that the $unset + // comes second. This is the ordering witness in its simplest shape. + const secrets = require('../../../services/connectorSecrets'); + const { integrationId, platformId } = await seedPair({ + platform: { webhookUrl: URL_A }, + config: { webhookUrl: URL_A }, + }); + const putSpy = jest.spyOn(secrets, 'put') + .mockRejectedValue(new Error('ring exploded')); + + await expect(encryptDiscordWebhookUrls({ apply: true })).rejects.toThrow('ring exploded'); + + const integration = await integrationRows().findOne({ _id: integrationId }); + expect(integration.config).not.toHaveProperty('webhookUrlRef'); + expect(integration.config.webhookUrl).toBe(URL_A); + expect((await discordRows().findOne({ _id: platformId })).webhookUrl).toBe(URL_A); + // A re-run still sees exactly one candidate: the count of what actually moved + // is 0, so the operator sees the same work waiting rather than a lost row. + expect(await encryptDiscordWebhookUrls({ apply: false })) + .toMatchObject({ candidates: 1, migrated: 0, alreadyEncrypted: 0 }); + putSpy.mockRestore(); + }); + + it('clears a row only after its own ref is stored, so one failure cannot strand the rest', async () => { + // Same invariant, one row further in: the run must complete row 1 all the way + // (or the test would pass vacuously) and leave row 2 whole when its put fails. + const secrets = require('../../../services/connectorSecrets'); + const realPut = secrets.put; + const putSpy = jest.spyOn(secrets, 'put') + .mockImplementationOnce(realPut) + .mockRejectedValueOnce(new Error('ring exploded on the second row')); + const first = await seedPair({ platform: { webhookUrl: URL_A } }); + const second = await seedPair({ platform: { webhookUrl: URL_B } }); + + await expect(encryptDiscordWebhookUrls({ apply: true })) + .rejects.toThrow('ring exploded on the second row'); + + const rows = await Promise.all([first, second].map(async ({ integrationId, platformId }) => ({ + integration: await integrationRows().findOne({ _id: integrationId }), + platform: await discordRows().findOne({ _id: platformId }), + }))); + const moved = rows.filter((row) => row.integration.config.webhookUrlRef); + const whole = rows.filter((row) => !row.integration.config.webhookUrlRef); + + expect(moved).toHaveLength(1); + expect(moved[0].platform).not.toHaveProperty('webhookUrl'); + expect(await get(moved[0].integration.config.webhookUrlRef)) + .toMatch(/^https:\/\/discord\.com\/api\/webhooks\//); + // The other row is untouched: no ref, and its URL is exactly where it was. + expect(whole).toHaveLength(1); + expect(whole[0].platform.webhookUrl).toMatch(/^https:\/\/discord\.com\/api\/webhooks\//); + putSpy.mockRestore(); + }); + + it('an unusable key ring hits the same ordering, not a pre-flight', async () => { + // The ring-shaped instance of the property above. Note what does NOT protect + // this: the script used to call `listWithUnavailableKey()` first, which lists + // refs whose key is missing and cannot itself fail — deleting it reddens + // nothing, which is how vera found the ordering unwitnessed in the first + // place. The empty ring makes the real `put` throw, and the $unsets never run. process.env.CONNECTOR_SECRET_KEYS = ''; const { integrationId, platformId } = await seedPair({ platform: { webhookUrl: URL_A } }); diff --git a/backend/scripts/encrypt-discord-webhook-url.ts b/backend/scripts/encrypt-discord-webhook-url.ts index cd2404e69a..b807bba1d4 100644 --- a/backend/scripts/encrypt-discord-webhook-url.ts +++ b/backend/scripts/encrypt-discord-webhook-url.ts @@ -15,6 +15,15 @@ * resolver (the old image read the plaintext field directly, so clearing first * would break it), and it is idempotent: a second run finds no plaintext left. * + * The destructive half is the `$unset`, and the only thing that keeps it safe is + * its ORDER: per row, the `put` completes before either plaintext is cleared. So + * a failure of any kind — an unusable key ring, a transient write error — leaves + * that row's plaintext exactly where it was, and a re-run both retries it and + * reports what already moved as `alreadyEncrypted`. There is deliberately no + * separate ring pre-flight: `listWithUnavailableKey` lists refs whose key is + * missing from the ring, it cannot fail, and a call that cannot fail is not a + * guard. The ordering is, and two witnesses in the suite hold it there. + * * Both plaintext stores are read, because a row connected before this change can * hold either: the platform document's `webhookUrl` (the writer that derived it * from the Discord API) and the legacy `Integration.config.webhookUrl` (which a @@ -82,11 +91,6 @@ export async function encryptDiscordWebhookUrls( .find({}, { projection: { _id: 1, integrationId: 1, webhookUrl: 1 } }) .toArray(); - // Fail before the first write if the ring cannot encrypt anything: an --apply - // that unset a plaintext and could not store a ref would destroy the only copy - // of a live credential. - if (apply) await connectorSecrets.listWithUnavailableKey(); - let candidates = 0; let migrated = 0; let alreadyEncrypted = 0; @@ -118,6 +122,9 @@ export async function encryptDiscordWebhookUrls( if (!apply) continue; + // Order matters, and it is the whole of the protection: a `put` that throws + // leaves this row's plaintext in both stores for the next attempt. Never move + // an `$unset` above this line. const webhookUrlRef = await connectorSecrets.put( String(row.integrationId), DISCORD_WEBHOOK_URL, From 715e187c19e1a5a24cc227d1eaf2c6cd6dfd4aae Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:10:21 -0700 Subject: [PATCH 3/3] docs(discord): correct the migration comment's claim about the deleted pre-flight vera's correction at 74198, and she is right: `listWithUnavailableKey` CAN fail. Its first statement is `const { keys } = parseKeyRing()` (services/connectorSecrets.ts:171), and `parseKeyRing` throws `ConnectorSecretConfigurationError` on an unset, malformed or duplicated ring and `ConnectorSecretKeyMissing` when the active key is absent (:49-79). I wrote "it cannot fail" by quoting the find line and dropping the line above it. So the pre-flight was redundant, not inert: `put` computes `encrypt(material, parseKeyRing())` before `upsertEncrypted` touches anything (:138-146), so an unusable ring already fails the first row ahead of either `$unset`. Deleting it remains right; the reason recorded for the deletion was wrong, and on the one script in this lane whose damage is irreversible. The comment now says what is true, in vera's wording; the same false claim is corrected in the test comment that cited it. Comment-only, so the clearance at `d015ad25` stands: the only changed lines in this commit begin with `//`, `*` or `/**`. --- .../unit/scripts/encryptDiscordWebhookUrl.test.ts | 10 ++++++---- backend/scripts/encrypt-discord-webhook-url.ts | 7 ++++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts b/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts index 3bfff5036d..1d24837aa2 100644 --- a/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts +++ b/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts @@ -190,10 +190,12 @@ describe('encrypt-discord-webhook-url', () => { it('an unusable key ring hits the same ordering, not a pre-flight', async () => { // The ring-shaped instance of the property above. Note what does NOT protect - // this: the script used to call `listWithUnavailableKey()` first, which lists - // refs whose key is missing and cannot itself fail — deleting it reddens - // nothing, which is how vera found the ordering unwitnessed in the first - // place. The empty ring makes the real `put` throw, and the $unsets never run. + // this: the script used to call `listWithUnavailableKey()` first, and that + // call does parse the ring and would throw on an unusable one — but so does + // `put`, ahead of either `$unset`, so the pre-flight was redundant rather + // than protective, and deleting it reddens nothing (which is how vera found + // the ordering unwitnessed in the first place). The empty ring makes the real + // `put` throw, and the $unsets never run. process.env.CONNECTOR_SECRET_KEYS = ''; const { integrationId, platformId } = await seedPair({ platform: { webhookUrl: URL_A } }); diff --git a/backend/scripts/encrypt-discord-webhook-url.ts b/backend/scripts/encrypt-discord-webhook-url.ts index b807bba1d4..e5b4c59347 100644 --- a/backend/scripts/encrypt-discord-webhook-url.ts +++ b/backend/scripts/encrypt-discord-webhook-url.ts @@ -20,9 +20,10 @@ * a failure of any kind — an unusable key ring, a transient write error — leaves * that row's plaintext exactly where it was, and a re-run both retries it and * reports what already moved as `alreadyEncrypted`. There is deliberately no - * separate ring pre-flight: `listWithUnavailableKey` lists refs whose key is - * missing from the ring, it cannot fail, and a call that cannot fail is not a - * guard. The ordering is, and two witnesses in the suite hold it there. + * separate ring pre-flight: `put` parses the key ring before it writes anything, + * so an unusable ring already fails the first row ahead of either `$unset`. A + * second check would be redundant, not safer. The ordering is what protects the + * data, and two witnesses in the suite hold it there. * * Both plaintext stores are read, because a row connected before this change can * hold either: the platform document's `webhookUrl` (the writer that derived it