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..1d24837aa2 --- /dev/null +++ b/backend/__tests__/unit/scripts/encryptDiscordWebhookUrl.test.ts @@ -0,0 +1,262 @@ +// @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('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, 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 } }); + + 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..e5b4c59347 --- /dev/null +++ b/backend/scripts/encrypt-discord-webhook-url.ts @@ -0,0 +1,205 @@ +#!/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. + * + * 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: `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 + * 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(); + + 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; + + // 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, + 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;