From 46486bfc9fb7df543d093f6de43b054349d7cd58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Mon, 19 Oct 2026 09:31:34 +0000 Subject: [PATCH 1/6] fix(slack): keep bot credentials in the credential store Slack installs resolve their bot token from the encrypted slack_oauth_credentials row. upsertSlackInstallation writes the platform_integrations.metadata object with non-secret workspace identifiers only, requires the credential-store write to complete before returning, and every server read path (connection test, uninstall cleanup, and the bot requester-info permalink lookup) obtains the token through the credential store. The maintenance script writes the credential row for each existing integration and then rewrites its metadata object to contain only the non-secret fields; an integration whose credential row cannot be written keeps its metadata unchanged and appears in the run report so the script can be repeated. --- apps/web/src/lib/bot/platforms/slack.tsx | 4 +- .../platforms/slack/credential-store.ts | 9 +- .../lib/integrations/slack-service.test.ts | 79 +++++++-- .../web/src/lib/integrations/slack-service.ts | 154 ++++++++---------- ...-08-07_backfill-slack-oauth-credentials.ts | 94 +++++++++-- 5 files changed, 211 insertions(+), 129 deletions(-) diff --git a/apps/web/src/lib/bot/platforms/slack.tsx b/apps/web/src/lib/bot/platforms/slack.tsx index 550e90ec292..bbbb056c96d 100644 --- a/apps/web/src/lib/bot/platforms/slack.tsx +++ b/apps/web/src/lib/bot/platforms/slack.tsx @@ -13,7 +13,7 @@ import { import type { BotPlatform, RequesterInfo } from '@/lib/bot/platforms/types'; import { BOT_CONTEXT_MESSAGE_LIMIT } from '@/lib/bot/constants'; import { APP_URL } from '@kilocode/web-shared/lib/constants'; -import { getAccessTokenFromInstallation } from '@/lib/integrations/slack-service'; +import { getSlackBotToken } from '@/lib/integrations/slack-service'; import { PLATFORM } from '@/lib/integrations/core/constants'; import { getSlackMessagePermalink } from '@/lib/slack-bot/slack-utils'; import { captureException } from '@sentry/nextjs'; @@ -219,7 +219,7 @@ async function getSlackRequesterInfo( platformIntegration: PlatformIntegration, displayName: string ): Promise { - const accessToken = getAccessTokenFromInstallation(platformIntegration); + const accessToken = await getSlackBotToken(platformIntegration); if (!accessToken) { return { displayName, platform: PLATFORM.SLACK }; } diff --git a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts index 36fc017a96a..4fe6387ccca 100644 --- a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts +++ b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts @@ -17,9 +17,10 @@ import { /** * System of record for Slack bot credentials. * - * Nothing in production reads this store yet — Step 2 only dual-writes it alongside - * the existing plaintext `platform_integrations.metadata.access_token`, so the change - * is reversible. Step 3 repoints the read paths here and deletes the plaintext copy. + * Install writes go through here and all server read paths decrypt from here; the + * legacy plaintext `platform_integrations.metadata.access_token` copy is no longer + * written and is removed from existing rows by the backfill script + * (`src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts`). */ export type WriteSlackCredentialInput = { @@ -73,7 +74,7 @@ export async function writeSlackCredential( * Creates the credential row only when the integration does not have one yet. * * For callers whose token comes from a snapshot rather than from a live Slack - * response — the Step 2 backfill reads `platform_integrations.metadata` up front — a + * response — the backfill script reads `platform_integrations.metadata` up front — a * reinstall part-way through the run writes a fresh credential that a blind replace * would overwrite with the by-then-revoked snapshot token. Existence is therefore * re-checked inside the same locked transaction as the write. diff --git a/apps/web/src/lib/integrations/slack-service.test.ts b/apps/web/src/lib/integrations/slack-service.test.ts index 277d834451b..48f9714d327 100644 --- a/apps/web/src/lib/integrations/slack-service.test.ts +++ b/apps/web/src/lib/integrations/slack-service.test.ts @@ -41,8 +41,13 @@ jest.mock('@slack/web-api', () => ({ })); const mockWriteSlackCredential = jest.fn(); +const mockGetSlackCredentialByIntegrationId = jest.fn(); +const mockDecryptSlackBotToken = jest.fn(); jest.mock('@/lib/integrations/platforms/slack/credential-store', () => ({ writeSlackCredential: (...args: unknown[]) => mockWriteSlackCredential(...args), + getSlackCredentialByIntegrationId: (...args: unknown[]) => + mockGetSlackCredentialByIntegrationId(...args), + decryptSlackBotToken: (...args: unknown[]) => mockDecryptSlackBotToken(...args), })); const mockCaptureException = jest.fn(); @@ -86,7 +91,11 @@ describe('slack-service uninstallApp', () => { mockUpdateReturning.mockReset(); mockDeleteWhere.mockReset(); mockAuthRevoke.mockReset(); + mockGetSlackCredentialByIntegrationId.mockReset(); + mockDecryptSlackBotToken.mockReset(); mockAuthRevoke.mockResolvedValue({ ok: true }); + mockGetSlackCredentialByIntegrationId.mockResolvedValue({ id: 'credential-1' }); + mockDecryptSlackBotToken.mockReturnValue('xoxb-token'); mockUpdateSet.mockReturnValue({ where: mockUpdateWhere }); mockUpdateWhere.mockReturnValue({ returning: mockUpdateReturning }); mockUpdateReturning.mockResolvedValue([buildSlackIntegration()]); @@ -155,6 +164,17 @@ describe('slack-service uninstallApp', () => { expect(mockDeleteWhere).toHaveBeenCalledTimes(1); }); + it('revokes the credential-store token for an active integration', async () => { + mockLimit.mockResolvedValue([buildSlackIntegration()]); + + await uninstallApp(owner); + + expect(mockGetSlackCredentialByIntegrationId).toHaveBeenCalledWith('integration-1'); + expect(mockDecryptSlackBotToken).toHaveBeenCalledWith({ id: 'credential-1' }, owner); + expect(mockAuthRevoke).toHaveBeenCalledTimes(1); + expect(mockDeleteWhere).toHaveBeenCalledTimes(1); + }); + it('disconnects suspended integrations without deleting shared Slack installation state', async () => { mockLimit.mockResolvedValue([ buildSlackIntegration({ @@ -200,6 +220,10 @@ describe('slack-service testConnection', () => { beforeEach(() => { mockLimit.mockReset(); mockAuthTest.mockReset(); + mockGetSlackCredentialByIntegrationId.mockReset(); + mockDecryptSlackBotToken.mockReset(); + mockGetSlackCredentialByIntegrationId.mockResolvedValue({ id: 'credential-1' }); + mockDecryptSlackBotToken.mockReturnValue('xoxb-token'); }); it('returns success when auth.test succeeds', async () => { @@ -220,8 +244,29 @@ describe('slack-service testConnection', () => { expect(mockAuthTest).not.toHaveBeenCalled(); }); - it('returns failure when the access token is missing from metadata', async () => { + it('reads the token from the encrypted credential store', async () => { + mockLimit.mockResolvedValue([buildSlackIntegration({ metadata: {} })]); + mockAuthTest.mockResolvedValue({ ok: true }); + + await expect(testConnection(owner)).resolves.toEqual({ success: true }); + expect(mockGetSlackCredentialByIntegrationId).toHaveBeenCalledWith('integration-1'); + expect(mockDecryptSlackBotToken).toHaveBeenCalledWith({ id: 'credential-1' }, owner); + }); + + it('returns failure when no credential row exists', async () => { + mockLimit.mockResolvedValue([buildSlackIntegration({ metadata: {} })]); + mockGetSlackCredentialByIntegrationId.mockResolvedValue(null); + + await expect(testConnection(owner)).resolves.toEqual({ + success: false, + error: 'No access token found', + }); + expect(mockAuthTest).not.toHaveBeenCalled(); + }); + + it('returns failure when the credential cannot be decrypted', async () => { mockLimit.mockResolvedValue([buildSlackIntegration({ metadata: {} })]); + mockDecryptSlackBotToken.mockReturnValue(null); await expect(testConnection(owner)).resolves.toEqual({ success: false, @@ -277,11 +322,13 @@ describe('upsertSlackInstallation', () => { mockInsertValues.mockReturnValue({ returning: mockInsertReturning }); mockInsertReturning.mockResolvedValue([buildSlackIntegration()]); mockWriteSlackCredential.mockReset(); + mockGetSlackCredentialByIntegrationId.mockReset(); + mockDecryptSlackBotToken.mockReset(); mockWriteSlackCredential.mockResolvedValue({ id: 'credential-1' }); mockCaptureException.mockReset(); }); - it('preserves the selected model when refreshing an existing installation', async () => { + it('preserves the selected model and scrubs any stale metadata token when refreshing an existing installation', async () => { mockLimit.mockResolvedValue([ buildSlackIntegration({ metadata: { @@ -303,12 +350,11 @@ describe('upsertSlackInstallation', () => { expect(mockUpdateSet).toHaveBeenCalledWith( expect.objectContaining({ - metadata: expect.objectContaining({ - access_token: 'xoxb-new-token', + metadata: { bot_user_id: 'U_NEW_BOT', incoming_webhook: { channel: '#general', channelId: 'C123', url: 'https://example.com' }, model_slug: 'anthropic/claude-sonnet-4.5', - }), + }, platform_installation_id: 'T123', }) ); @@ -328,12 +374,16 @@ describe('upsertSlackInstallation', () => { expect(mockInsertValues).toHaveBeenCalledWith( expect.objectContaining({ metadata: expect.objectContaining({ - access_token: 'xoxb-new-token', bot_user_id: 'U_NEW_BOT', model_slug: DEFAULT_BOT_MODEL, }), }) ); + const insertedMetadata = mockInsertValues.mock.calls[0][0].metadata as Record< + string, + unknown + >; + expect(insertedMetadata).not.toHaveProperty('access_token'); }); it('rejects installing a Slack workspace connected to another owner', async () => { @@ -372,7 +422,7 @@ describe('upsertSlackInstallation', () => { ); }); - it('dual-writes the bot token into the encrypted credential store on a new install', async () => { + it('writes the bot token into the encrypted credential store on a new install', async () => { mockLimit.mockResolvedValueOnce([]).mockResolvedValueOnce([]); const installation = { @@ -392,7 +442,7 @@ describe('upsertSlackInstallation', () => { }); }); - it('dual-writes the bot token when refreshing an existing install', async () => { + it('writes the bot token into the credential store when refreshing an existing install', async () => { mockLimit.mockResolvedValue([buildSlackIntegration()]); const installation = { @@ -408,10 +458,9 @@ describe('upsertSlackInstallation', () => { ); }); - // The encrypted store is write-only until reads move onto it, so a failure there - // must not take down Slack installs. This expectation flips in the step that - // repoints the read paths. - it('still completes the install when the credential store write fails', async () => { + // The store is the system of record: if its write fails, the installation would + // have no readable credential anywhere, so the install fails and the user retries. + it('fails the install when the credential store write fails', async () => { mockLimit.mockResolvedValueOnce([]).mockResolvedValueOnce([]); mockWriteSlackCredential.mockRejectedValue(new Error('encryption not configured')); @@ -423,10 +472,6 @@ describe('upsertSlackInstallation', () => { await expect( upsertSlackInstallation({ owner, teamId: 'T123', installation }) - ).resolves.toMatchObject({ id: 'integration-1' }); - - expect(mockCaptureException).toHaveBeenCalledTimes(1); - // The report must not carry token material. - expect(JSON.stringify(mockCaptureException.mock.calls[0])).not.toContain('xoxb-new-token'); + ).rejects.toThrow('encryption not configured'); }); }); diff --git a/apps/web/src/lib/integrations/slack-service.ts b/apps/web/src/lib/integrations/slack-service.ts index d15957509f4..c392cc186f9 100644 --- a/apps/web/src/lib/integrations/slack-service.ts +++ b/apps/web/src/lib/integrations/slack-service.ts @@ -13,7 +13,12 @@ import type { SlackInstallation } from '@chat-adapter/slack'; import { getDefaultAllowedModel } from '@/lib/slack-bot/model-allow-list'; import { DEFAULT_BOT_MODEL } from '@/lib/bot/constants'; import { isOrganizationModelUpdateAllowed } from '@kilocode/web-shared/lib/organizations/effective-model-access.server'; -import { writeSlackCredential } from '@/lib/integrations/platforms/slack/credential-store'; +import { + decryptSlackBotToken, + getSlackCredentialByIntegrationId, + writeSlackCredential, +} from '@/lib/integrations/platforms/slack/credential-store'; +import { SlackCredentialDecryptionError } from '@/lib/integrations/platforms/slack/credential-encryption'; import { captureException } from '@sentry/nextjs'; export class SlackWorkspaceAlreadyConnectedError extends Error { @@ -194,38 +199,39 @@ export function getOwnerFromInstallation(integration: PlatformIntegration): Owne } /** - * Dual-write the bot token into the encrypted `slack_oauth_credentials` store. + * Load the bot token for an integration from the encrypted credential store. * - * Deliberately best-effort for now: nothing reads that store yet, so an unconfigured - * or failing encryption keyset must not break Slack installs. Step 3 moves the read - * paths onto the store, at which point this write becomes required and must move - * inside the same transaction as the `platform_integrations` write. + * Returns null when no credential row exists, the row is revoked, or the row cannot + * be read (decryption failure or an owner that does not match the encryption AAD). + * Failures are reported to Sentry without token material; callers treat null as "no + * access token" and surface their own user-facing error. */ -async function mirrorSlackCredential({ - integration, - owner, - teamId, - installation, -}: { - integration: PlatformIntegration; - owner: Owner; - teamId: string; - installation: SlackInstallation; -}): Promise { - try { - await writeSlackCredential({ - integrationId: integration.id, - slackTeamId: teamId, - owner, - botToken: installation.botToken, - botUserId: installation.botUserId ?? null, +export async function getSlackBotToken(integration: PlatformIntegration): Promise { + const owner = getOwnerFromInstallation(integration); + if (!owner) { + captureException(new Error('Slack integration has no exclusive owner'), { + level: 'error', + tags: { component: 'slack-service', op: 'read-slack-credential' }, + extra: { integrationId: integration.id }, }); + return null; + } + + const credential = await getSlackCredentialByIntegrationId(integration.id); + if (!credential) return null; + + try { + return decryptSlackBotToken(credential, owner); } catch (error) { captureException(error, { level: 'error', - tags: { component: 'slack-service', op: 'mirror-slack-credential' }, - extra: { integrationId: integration.id, teamId }, + tags: { component: 'slack-service', op: 'read-slack-credential' }, + extra: { + integrationId: integration.id, + kind: error instanceof SlackCredentialDecryptionError ? error.kind : 'unknown', + }, }); + return null; } } @@ -256,18 +262,22 @@ export async function upsertSlackInstallation({ ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) : DEFAULT_BOT_MODEL; + // Non-secret workspace identifiers only. The bot token lives solely in the + // encrypted `slack_oauth_credentials` store; dropping `access_token` here also + // scrubs a stale copy from rows that predate the credential store (a reinstall + // refreshes the whole metadata object). + const existingMetadata = + existing?.metadata && typeof existing.metadata === 'object' ? existing.metadata : {}; const metadata = { - ...(existing?.metadata && typeof existing.metadata === 'object' ? existing.metadata : {}), - access_token: installation.botToken, + ...existingMetadata, bot_user_id: installation.botUserId, model_slug: - existing?.metadata && - typeof existing.metadata === 'object' && - 'model_slug' in existing.metadata && - typeof existing.metadata.model_slug === 'string' - ? existing.metadata.model_slug + 'model_slug' in existingMetadata && + typeof existingMetadata.model_slug === 'string' + ? existingMetadata.model_slug : defaultModel, }; + delete (metadata as Record).access_token; if (existing) { try { @@ -285,7 +295,13 @@ export async function upsertSlackInstallation({ .where(eq(platform_integrations.id, existing.id)) .returning(); - await mirrorSlackCredential({ integration: updated, owner, teamId, installation }); + await writeSlackCredential({ + integrationId: updated.id, + slackTeamId: teamId, + owner, + botToken: installation.botToken, + botUserId: installation.botUserId ?? null, + }); return updated; } catch (error) { @@ -314,7 +330,13 @@ export async function upsertSlackInstallation({ }) .returning(); - await mirrorSlackCredential({ integration: created, owner, teamId, installation }); + await writeSlackCredential({ + integrationId: created.id, + slackTeamId: teamId, + owner, + botToken: installation.botToken, + botUserId: installation.botUserId ?? null, + }); return created; } catch (error) { @@ -341,11 +363,11 @@ export async function uninstallApp(owner: Owner, options: SlackUninstallOptions const shouldDeleteSlackInstallation = integration.integration_status === INTEGRATION_STATUS.ACTIVE; - // Revoke the token if we have one - const metadata = integration.metadata as { access_token?: string } | null; - if (shouldDeleteSlackInstallation && metadata?.access_token) { + // Revoke the stored credential if we have one + const botToken = await getSlackBotToken(integration); + if (shouldDeleteSlackInstallation && botToken) { try { - await revokeSlackToken(metadata.access_token); + await revokeSlackToken(botToken); } catch (error) { console.error('Failed to revoke Slack token:', error); } @@ -414,14 +436,14 @@ export async function testConnection(owner: Owner): Promise<{ success: boolean; return { success: false, error: 'No Slack installation found' }; } - const metadata = integration.metadata as { access_token?: string } | null; + const botToken = await getSlackBotToken(integration); - if (!metadata?.access_token) { + if (!botToken) { return { success: false, error: 'No access token found' }; } try { - const client = new WebClient(metadata.access_token); + const client = new WebClient(botToken); const result = await client.auth.test(); if (!result.ok) { @@ -435,44 +457,6 @@ export async function testConnection(owner: Owner): Promise<{ success: boolean; } } -/** - * Send a message to a Slack channel using the stored integration - */ -export async function sendMessage( - owner: Owner, - channel: string, - text: string -): Promise<{ success: boolean; error?: string; ts?: string }> { - const integration = await getInstallation(owner); - - if (!integration) { - return { success: false, error: 'No Slack installation found' }; - } - - const metadata = integration.metadata as { access_token?: string } | null; - - if (!metadata?.access_token) { - return { success: false, error: 'No access token found' }; - } - - try { - const client = new WebClient(metadata.access_token); - const result = await client.chat.postMessage({ - channel, - text, - }); - - if (!result.ok) { - return { success: false, error: result.error || 'Unknown error' }; - } - - return { success: true, ts: result.ts }; - } catch (error) { - const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - return { success: false, error: errorMessage }; - } -} - /** * Update the model for a Slack integration. * For organization-owned integrations, validates the model against org access policy. @@ -543,16 +527,6 @@ export type SlackPostMessageResponse = { error?: string; }; -/** - * Extract access token from installation metadata - */ -export function getAccessTokenFromInstallation( - integration: PlatformIntegration -): string | undefined { - const metadata = integration.metadata as { access_token?: string } | null; - return metadata?.access_token; -} - /** * Post a message to Slack using an access token directly */ diff --git a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts index 17fe7547d7b..1dd516c3224 100644 --- a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts +++ b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts @@ -1,24 +1,29 @@ /** - * Backfills existing plaintext Slack bot tokens from - * `platform_integrations.metadata.access_token` into the encrypted - * `slack_oauth_credentials` store. + * Backfills existing Slack bot tokens from + * `platform_integrations.metadata` into the encrypted + * `slack_oauth_credentials` store, then removes the plaintext token from + * metadata. * - * This is Step 2 of the Slack bot token remediation. It is additive only: the - * plaintext copy is left in place, and nothing reads the new store yet. Deleting the - * plaintext copy happens in a later step, after reads have moved over and been - * verified. + * The copy pass is idempotent: integrations that already have a credential row are + * skipped rather than re-encrypted, so re-running does not churn `credential_version` + * (each bump invalidates the previous ciphertext, since the version is inside the + * AAD). The skip is enforced by the write itself, so a reinstall that lands mid-run + * keeps its fresh token instead of being overwritten by this run's older metadata + * snapshot. * - * Idempotent by design: integrations that already have a credential row are skipped - * rather than re-encrypted, so re-running does not churn `credential_version` (each - * bump invalidates the previous ciphertext, since the version is inside the AAD). The - * skip is enforced by the write itself, so a reinstall that lands mid-run keeps its - * fresh token instead of being overwritten by this run's older metadata snapshot. + * The scrub pass (on by default with --execute; opt out with --no-scrub) deletes + * `metadata.access_token` for every Slack integration that has a credential row, + * preserving all other metadata keys. Rows without a credential row keep their + * plaintext token and are reported, so the operation is safe to re-run after fixing + * them. Run this only after the cutover that reads tokens from the credential store + * is deployed. * * Requires SLACK_CREDENTIAL_KEYSET_JSON. Never prints token material. * * Usage: * pnpm --filter web script src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts * pnpm --filter web script src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts --execute + * pnpm --filter web script src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts --execute --no-scrub */ import '../lib/load-env'; @@ -36,15 +41,20 @@ type SkipReason = | 'no_plaintext_token' | 'no_owner' | 'no_team_id' - | 'write_failed'; + | 'write_failed' + | 'scrub_no_credential' + | 'scrub_failed'; -function parseArgs(): { execute: boolean; batchSize: number } { +function parseArgs(): { execute: boolean; scrub: boolean; batchSize: number } { let execute = false; + let scrub = true; let batchSize = 200; for (const arg of process.argv.slice(2)) { if (arg === '--execute') { execute = true; + } else if (arg === '--no-scrub') { + scrub = false; } else if (arg.startsWith('--batch-size=')) { const parsed = Number(arg.slice('--batch-size='.length)); if (!Number.isInteger(parsed) || parsed <= 0) { @@ -56,7 +66,7 @@ function parseArgs(): { execute: boolean; batchSize: number } { } } - return { execute, batchSize }; + return { execute, scrub, batchSize }; } function readPlaintextToken(metadata: unknown): string | null { @@ -67,7 +77,7 @@ function readPlaintextToken(metadata: unknown): string | null { } async function main(): Promise { - const { execute, batchSize } = parseArgs(); + const { execute, scrub, batchSize } = parseArgs(); // Fail fast rather than discovering a bad keyset partway through a run. const { active } = requireSlackCredentialKeyset(); @@ -100,9 +110,12 @@ async function main(): Promise { no_owner: 0, no_team_id: 0, write_failed: 0, + scrub_no_credential: 0, + scrub_failed: 0, }; let migrated = 0; let eligible = 0; + let scrubbed = 0; for (let offset = 0; offset < integrations.length; offset += batchSize) { const batch = integrations.slice(offset, offset + batchSize); @@ -174,6 +187,50 @@ async function main(): Promise { } } + // Scrub pass: once every plaintext token has an encrypted copy, remove the + // plaintext from metadata. Rows without a credential row are left untouched — + // deleting their only token copy would make the integration unrecoverable — and + // are reported so they can be fixed and the script re-run (it is idempotent). + if (execute && scrub) { + for (const integration of integrations) { + const metadata = integration.metadata; + if (metadata === null || typeof metadata !== 'object') continue; + if (!('access_token' in metadata)) continue; + + const [credential] = await db + .select({ id: slack_oauth_credentials.id }) + .from(slack_oauth_credentials) + .where(eq(slack_oauth_credentials.platform_integration_id, integration.id)) + .limit(1); + + if (!credential) { + skipped.scrub_no_credential += 1; + console.warn( + `Kept plaintext token for integration ${integration.id}: no encrypted credential row` + ); + continue; + } + + try { + const rest: Record = { ...metadata }; + delete rest.access_token; + await db + .update(platform_integrations) + .set({ + metadata: rest, + updated_at: new Date().toISOString(), + }) + .where(eq(platform_integrations.id, integration.id)); + scrubbed += 1; + } catch (error) { + skipped.scrub_failed += 1; + console.error( + `Failed to scrub integration ${integration.id}: ${error instanceof Error ? error.message : 'unknown error'}` + ); + } + } + } + const [{ total }] = await db .select({ total: sql`count(*)::int` }) .from(slack_oauth_credentials); @@ -181,6 +238,11 @@ async function main(): Promise { console.log('\n--- Summary ---'); console.log(`Eligible for backfill: ${eligible}`); console.log(`Migrated: ${migrated}`); + console.log(`Scrubbed plaintext tokens: ${scrubbed}`); + console.log( + `Kept (no credential row): ${skipped.scrub_no_credential}` + ); + console.log(`Failed scrubs: ${skipped.scrub_failed}`); console.log(`Skipped (already migrated): ${skipped.already_migrated}`); console.log(`Skipped (no plaintext): ${skipped.no_plaintext_token}`); console.log(`Skipped (no owner): ${skipped.no_owner}`); From ad4bfc2a151369328d39a4c0afcf403d09415af5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Mon, 19 Oct 2026 09:46:25 +0000 Subject: [PATCH 2/6] chore: format changed files --- apps/web/src/lib/integrations/slack-service.test.ts | 11 ++++------- apps/web/src/lib/integrations/slack-service.ts | 3 +-- .../d2026-08-07_backfill-slack-oauth-credentials.ts | 4 +--- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/apps/web/src/lib/integrations/slack-service.test.ts b/apps/web/src/lib/integrations/slack-service.test.ts index 48f9714d327..424af05a456 100644 --- a/apps/web/src/lib/integrations/slack-service.test.ts +++ b/apps/web/src/lib/integrations/slack-service.test.ts @@ -379,10 +379,7 @@ describe('upsertSlackInstallation', () => { }), }) ); - const insertedMetadata = mockInsertValues.mock.calls[0][0].metadata as Record< - string, - unknown - >; + const insertedMetadata = mockInsertValues.mock.calls[0][0].metadata as Record; expect(insertedMetadata).not.toHaveProperty('access_token'); }); @@ -470,8 +467,8 @@ describe('upsertSlackInstallation', () => { teamName: 'Kilo Team', } satisfies SlackInstallation; - await expect( - upsertSlackInstallation({ owner, teamId: 'T123', installation }) - ).rejects.toThrow('encryption not configured'); + await expect(upsertSlackInstallation({ owner, teamId: 'T123', installation })).rejects.toThrow( + 'encryption not configured' + ); }); }); diff --git a/apps/web/src/lib/integrations/slack-service.ts b/apps/web/src/lib/integrations/slack-service.ts index c392cc186f9..8aff6c4e589 100644 --- a/apps/web/src/lib/integrations/slack-service.ts +++ b/apps/web/src/lib/integrations/slack-service.ts @@ -272,8 +272,7 @@ export async function upsertSlackInstallation({ ...existingMetadata, bot_user_id: installation.botUserId, model_slug: - 'model_slug' in existingMetadata && - typeof existingMetadata.model_slug === 'string' + 'model_slug' in existingMetadata && typeof existingMetadata.model_slug === 'string' ? existingMetadata.model_slug : defaultModel, }; diff --git a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts index 1dd516c3224..a38238116c5 100644 --- a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts +++ b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts @@ -239,9 +239,7 @@ async function main(): Promise { console.log(`Eligible for backfill: ${eligible}`); console.log(`Migrated: ${migrated}`); console.log(`Scrubbed plaintext tokens: ${scrubbed}`); - console.log( - `Kept (no credential row): ${skipped.scrub_no_credential}` - ); + console.log(`Kept (no credential row): ${skipped.scrub_no_credential}`); console.log(`Failed scrubs: ${skipped.scrub_failed}`); console.log(`Skipped (already migrated): ${skipped.already_migrated}`); console.log(`Skipped (no plaintext): ${skipped.no_plaintext_token}`); From c53e5d0e156479f93aecc4515fb2c0b84b3c3336 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Mon, 19 Oct 2026 10:46:37 +0000 Subject: [PATCH 3/6] fix(slack): commit integration and credential rows together Slack installs now perform the platform integration write and the credential-store write in one database transaction, so both complete or neither does. The credential store accepts a caller-provided transaction. The maintenance script removes the token key from metadata with a targeted jsonb update and rewrites a row only when its stored credential decrypts to the row's current metadata token; any other row keeps its metadata and appears in the run report. The connection test also covers failures reading the credential row, and the test suite exercises the unreadable-credential path including its error report. --- .../platforms/slack/credential-store.ts | 14 ++- .../lib/integrations/slack-service.test.ts | 22 +++- .../web/src/lib/integrations/slack-service.ts | 104 ++++++++++-------- ...-08-07_backfill-slack-oauth-credentials.ts | 89 +++++++++++---- 4 files changed, 158 insertions(+), 71 deletions(-) diff --git a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts index 4fe6387ccca..f1b98d0cad7 100644 --- a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts +++ b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts @@ -7,7 +7,7 @@ import { buildSlackCredentialLockKey, type SlackCredentialOwner, } from '@kilocode/worker-utils/slack-credential'; -import { db } from '@kilocode/web-shared/lib/drizzle'; +import { db, type DrizzleTransaction } from '@kilocode/web-shared/lib/drizzle'; import { decryptSlackCredentialSecret, encryptSlackCredentialSecret, @@ -43,6 +43,12 @@ export type WriteSlackCredentialInput = { accessTokenExpiresAt?: string | null; /** Present only once Slack token rotation is enabled. */ refreshToken?: string | null; + /** + * Existing transaction to join instead of opening one. Callers that write the + * `platform_integrations` row in the same operation (the Slack install path) pass + * their transaction so the integration row and its credential commit together. + */ + tx?: DrizzleTransaction; }; export type SlackCredentialWriteOutcome = @@ -89,7 +95,7 @@ async function upsertSlackCredential( input: WriteSlackCredentialInput, { onlyIfAbsent }: { onlyIfAbsent: boolean } ): Promise { - return db.transaction(async tx => { + const run = async (tx: DrizzleTransaction): Promise => { await tx.execute( sql`SELECT pg_advisory_xact_lock(hashtextextended(${buildSlackCredentialLockKey(input.integrationId)}, 0))` ); @@ -157,7 +163,9 @@ async function upsertSlackCredential( .values({ id: credentialId, platform_integration_id: input.integrationId, ...values }) .returning(); return { status: 'written', credential: created }; - }); + }; + + return input.tx ? run(input.tx) : db.transaction(run); } export async function getSlackCredentialByIntegrationId( diff --git a/apps/web/src/lib/integrations/slack-service.test.ts b/apps/web/src/lib/integrations/slack-service.test.ts index 424af05a456..2b13cc8b3a5 100644 --- a/apps/web/src/lib/integrations/slack-service.test.ts +++ b/apps/web/src/lib/integrations/slack-service.test.ts @@ -9,6 +9,7 @@ const mockInsertValues = jest.fn(); const mockInsertReturning = jest.fn(); const mockAuthRevoke = jest.fn(); const mockAuthTest = jest.fn(); +const mockTransaction = jest.fn(); jest.mock('@kilocode/web-shared/lib/drizzle', () => ({ db: { @@ -28,6 +29,10 @@ jest.mock('@kilocode/web-shared/lib/drizzle', () => ({ insert: jest.fn(() => ({ values: mockInsertValues, })), + // Accessed lazily: jest.mock factories hoist above the const declarations. + get transaction() { + return mockTransaction; + }, }, })); @@ -58,6 +63,7 @@ jest.mock('@sentry/nextjs', () => ({ import type { Owner } from '@/lib/integrations/core/types'; import type { SlackInstallation } from '@chat-adapter/slack'; import { DEFAULT_BOT_MODEL } from '@/lib/bot/constants'; +import { SlackCredentialDecryptionError } from '@/lib/integrations/platforms/slack/credential-encryption'; import { deleteInstallationByTeamId, getMissingSlackScopes, @@ -266,13 +272,17 @@ describe('slack-service testConnection', () => { it('returns failure when the credential cannot be decrypted', async () => { mockLimit.mockResolvedValue([buildSlackIntegration({ metadata: {} })]); - mockDecryptSlackBotToken.mockReturnValue(null); + mockDecryptSlackBotToken.mockImplementation(() => { + throw new SlackCredentialDecryptionError('access'); + }); await expect(testConnection(owner)).resolves.toEqual({ success: false, error: 'No access token found', }); expect(mockAuthTest).not.toHaveBeenCalled(); + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(JSON.stringify(mockCaptureException.mock.calls[0])).not.toContain('xoxb-token'); }); it('returns the Slack error when auth.test rejects the token', async () => { @@ -326,6 +336,15 @@ describe('upsertSlackInstallation', () => { mockDecryptSlackBotToken.mockReset(); mockWriteSlackCredential.mockResolvedValue({ id: 'credential-1' }); mockCaptureException.mockReset(); + // The upsert wraps both writes in one transaction and awaits its callback. + mockTransaction.mockReset(); + mockTransaction.mockImplementation( + async (fn: (tx: Record) => Promise) => + fn({ + update: () => ({ set: mockUpdateSet }), + insert: () => ({ values: mockInsertValues }), + }) + ); }); it('preserves the selected model and scrubs any stale metadata token when refreshing an existing installation', async () => { @@ -436,6 +455,7 @@ describe('upsertSlackInstallation', () => { owner, botToken: 'xoxb-new-token', botUserId: 'U_NEW_BOT', + tx: expect.objectContaining({ insert: expect.any(Function) }), }); }); diff --git a/apps/web/src/lib/integrations/slack-service.ts b/apps/web/src/lib/integrations/slack-service.ts index 8aff6c4e589..63e97ea6c93 100644 --- a/apps/web/src/lib/integrations/slack-service.ts +++ b/apps/web/src/lib/integrations/slack-service.ts @@ -1,6 +1,6 @@ import 'server-only'; import { db } from '@kilocode/web-shared/lib/drizzle'; -import type { PlatformIntegration } from '@kilocode/db/schema'; +import type { PlatformIntegration, SlackOAuthCredential } from '@kilocode/db/schema'; import { platform_integrations } from '@kilocode/db/schema'; import { eq, and, isNull } from 'drizzle-orm'; import { TRPCError } from '@trpc/server'; @@ -217,7 +217,17 @@ export async function getSlackBotToken(integration: PlatformIntegration): Promis return null; } - const credential = await getSlackCredentialByIntegrationId(integration.id); + let credential: SlackOAuthCredential | null; + try { + credential = await getSlackCredentialByIntegrationId(integration.id); + } catch (error) { + captureException(error, { + level: 'error', + tags: { component: 'slack-service', op: 'read-slack-credential' }, + extra: { integrationId: integration.id }, + }); + return null; + } if (!credential) return null; try { @@ -280,64 +290,72 @@ export async function upsertSlackInstallation({ if (existing) { try { - const [updated] = await db - .update(platform_integrations) - .set({ + return await db.transaction(async tx => { + const [updated] = await tx + .update(platform_integrations) + .set({ + platform_installation_id: teamId, + platform_account_id: teamId, + platform_account_login: teamName, + scopes: SLACK_SCOPES, + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata, + updated_at: new Date().toISOString(), + }) + .where(eq(platform_integrations.id, existing.id)) + .returning(); + + // Both writes commit together: the integration row and its credential row + // form one unit, and each is incomplete without the other. + await writeSlackCredential({ + integrationId: updated.id, + slackTeamId: teamId, + owner, + botToken: installation.botToken, + botUserId: installation.botUserId ?? null, + tx, + }); + + return updated; + }); + } catch (error) { + if (isSlackWorkspaceUniqueViolation(error)) { + throw new SlackWorkspaceAlreadyConnectedError(teamName); + } + throw error; + } + } + + try { + return await db.transaction(async tx => { + const [created] = await tx + .insert(platform_integrations) + .values({ + owned_by_user_id: owner.type === 'user' ? owner.id : null, + owned_by_organization_id: owner.type === 'org' ? owner.id : null, + platform: PLATFORM.SLACK, + integration_type: 'oauth', platform_installation_id: teamId, platform_account_id: teamId, platform_account_login: teamName, scopes: SLACK_SCOPES, integration_status: INTEGRATION_STATUS.ACTIVE, metadata, - updated_at: new Date().toISOString(), + installed_at: new Date().toISOString(), }) - .where(eq(platform_integrations.id, existing.id)) .returning(); await writeSlackCredential({ - integrationId: updated.id, + integrationId: created.id, slackTeamId: teamId, owner, botToken: installation.botToken, botUserId: installation.botUserId ?? null, + tx, }); - return updated; - } catch (error) { - if (isSlackWorkspaceUniqueViolation(error)) { - throw new SlackWorkspaceAlreadyConnectedError(teamName); - } - throw error; - } - } - - try { - const [created] = await db - .insert(platform_integrations) - .values({ - owned_by_user_id: owner.type === 'user' ? owner.id : null, - owned_by_organization_id: owner.type === 'org' ? owner.id : null, - platform: PLATFORM.SLACK, - integration_type: 'oauth', - platform_installation_id: teamId, - platform_account_id: teamId, - platform_account_login: teamName, - scopes: SLACK_SCOPES, - integration_status: INTEGRATION_STATUS.ACTIVE, - metadata, - installed_at: new Date().toISOString(), - }) - .returning(); - - await writeSlackCredential({ - integrationId: created.id, - slackTeamId: teamId, - owner, - botToken: installation.botToken, - botUserId: installation.botUserId ?? null, + return created; }); - - return created; } catch (error) { if (isSlackWorkspaceUniqueViolation(error)) { throw new SlackWorkspaceAlreadyConnectedError(teamName); diff --git a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts index a38238116c5..0ca8a16db29 100644 --- a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts +++ b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts @@ -33,7 +33,11 @@ import { db, closeAllDrizzleConnections } from '@kilocode/web-shared/lib/drizzle import { platform_integrations, slack_oauth_credentials } from '@kilocode/db/schema'; import { PLATFORM } from '@/lib/integrations/core/constants'; import type { SlackCredentialOwner } from '@kilocode/worker-utils/slack-credential'; -import { createSlackCredentialIfAbsent } from '@/lib/integrations/platforms/slack/credential-store'; +import { + createSlackCredentialIfAbsent, + decryptSlackBotToken, + getSlackCredentialByIntegrationId, +} from '@/lib/integrations/platforms/slack/credential-store'; import { requireSlackCredentialKeyset } from '@/lib/integrations/platforms/slack/credential-keyset'; type SkipReason = @@ -72,7 +76,7 @@ function parseArgs(): { execute: boolean; scrub: boolean; batchSize: number } { function readPlaintextToken(metadata: unknown): string | null { if (metadata === null || typeof metadata !== 'object') return null; if (!('access_token' in metadata)) return null; - const value = (metadata as { access_token?: unknown }).access_token; + const value = metadata.access_token; return typeof value === 'string' && value.length > 0 ? value : null; } @@ -116,6 +120,10 @@ async function main(): Promise { let migrated = 0; let eligible = 0; let scrubbed = 0; + // Integrations whose credential row this run can vouch for: either the row was + // written by this run, or it decrypts to the metadata token (checked below). + // Only these are eligible for the scrub pass. + const scrubCandidateIds = new Set(); for (let offset = 0; offset < integrations.length; offset += batchSize) { const batch = integrations.slice(offset, offset + batchSize); @@ -137,6 +145,19 @@ async function main(): Promise { for (const integration of batch) { if (alreadyMigrated.has(integration.id)) { skipped.already_migrated += 1; + // The metadata token and the stored credential must agree before the + // metadata copy is removed. The comparison below admits the row for + // scrubbing only on an exact match; any other result keeps it unchanged. + if (execute && scrub) { + const fresh = await storedCredentialMatches(integration.id, integration); + if (fresh === true) { + scrubCandidateIds.add(integration.id); + } else if (fresh === false) { + skipped.scrub_no_credential += 1; + } else { + skipped.scrub_failed += 1; + } + } continue; } @@ -176,6 +197,7 @@ async function main(): Promise { skipped.already_migrated += 1; } else { migrated += 1; + scrubCandidateIds.add(integration.id); } } catch (error) { skipped.write_failed += 1; @@ -187,37 +209,25 @@ async function main(): Promise { } } - // Scrub pass: once every plaintext token has an encrypted copy, remove the - // plaintext from metadata. Rows without a credential row are left untouched — - // deleting their only token copy would make the integration unrecoverable — and - // are reported so they can be fixed and the script re-run (it is idempotent). + // Scrub pass: remove the metadata token for every eligible integration. + // - A row is eligible only when its stored credential is current: the credential + // was written by this run's copy pass, or an existing credential decrypts to + // exactly the metadata token. Every other row is reported and left unchanged so + // the script can be repeated after it is resolved. + // The metadata update removes only the token key (`jsonb -`), so concurrent writes + // to other metadata fields between the snapshot above and this update survive. if (execute && scrub) { for (const integration of integrations) { + if (!scrubCandidateIds.has(integration.id)) continue; const metadata = integration.metadata; if (metadata === null || typeof metadata !== 'object') continue; if (!('access_token' in metadata)) continue; - const [credential] = await db - .select({ id: slack_oauth_credentials.id }) - .from(slack_oauth_credentials) - .where(eq(slack_oauth_credentials.platform_integration_id, integration.id)) - .limit(1); - - if (!credential) { - skipped.scrub_no_credential += 1; - console.warn( - `Kept plaintext token for integration ${integration.id}: no encrypted credential row` - ); - continue; - } - try { - const rest: Record = { ...metadata }; - delete rest.access_token; await db .update(platform_integrations) .set({ - metadata: rest, + metadata: sql`${platform_integrations.metadata} - 'access_token'`, updated_at: new Date().toISOString(), }) .where(eq(platform_integrations.id, integration.id)); @@ -271,10 +281,41 @@ function readOwner(integration: { function readBotUserId(metadata: unknown): string | null { if (metadata === null || typeof metadata !== 'object') return null; if (!('bot_user_id' in metadata)) return null; - const value = (metadata as { bot_user_id?: unknown }).bot_user_id; + const value = metadata.bot_user_id; return typeof value === 'string' && value.length > 0 ? value : null; } +/** + * Whether the integration's stored credential is exactly its current metadata token. + * + * Returns true on an exact match, false when the credential row is absent, revoked, + * or the integration has no exclusive owner, and null when the row could not be + * read. The caller reports the distinction so each outcome gets the right follow-up. + * Never logs or prints token material. + */ +async function storedCredentialMatches( + integrationId: string, + integration: { + owned_by_organization_id: string | null; + owned_by_user_id: string | null; + metadata: unknown; + } +): Promise { + const botToken = readPlaintextToken(integration.metadata); + if (!botToken) return false; + const owner = readOwner(integration); + if (!owner) return false; + + const credential = await getSlackCredentialByIntegrationId(integrationId); + if (!credential) return false; + + try { + return decryptSlackBotToken(credential, owner) === botToken; + } catch { + return null; + } +} + void main() .catch(error => { console.error('Fatal error:', error); From a65b8469c23e715032df825c1f95420836ff8c3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Mon, 19 Oct 2026 11:10:43 +0000 Subject: [PATCH 4/6] fix(scripts): report ineligible maintenance rows by id The maintenance script's credential comparison covers lookup failures alongside read failures, so a database error during the run is reported per row instead of aborting the run. Rows whose stored credential does not match, or cannot be read, are logged with their integration id, and rows whose metadata already lacks the token key skip the comparison so repeat runs do not count them as ineligible. --- ...026-08-07_backfill-slack-oauth-credentials.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts index 0ca8a16db29..e0a0369c475 100644 --- a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts +++ b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts @@ -31,6 +31,7 @@ import '../lib/load-env'; import { and, eq, inArray, isNotNull, sql } from 'drizzle-orm'; import { db, closeAllDrizzleConnections } from '@kilocode/web-shared/lib/drizzle'; import { platform_integrations, slack_oauth_credentials } from '@kilocode/db/schema'; +import type { SlackOAuthCredential } from '@kilocode/db/schema'; import { PLATFORM } from '@/lib/integrations/core/constants'; import type { SlackCredentialOwner } from '@kilocode/worker-utils/slack-credential'; import { @@ -148,14 +149,20 @@ async function main(): Promise { // The metadata token and the stored credential must agree before the // metadata copy is removed. The comparison below admits the row for // scrubbing only on an exact match; any other result keeps it unchanged. - if (execute && scrub) { + if (execute && scrub && readPlaintextToken(integration.metadata)) { const fresh = await storedCredentialMatches(integration.id, integration); if (fresh === true) { scrubCandidateIds.add(integration.id); } else if (fresh === false) { skipped.scrub_no_credential += 1; + console.warn( + `Kept metadata token for integration ${integration.id}: stored credential does not match it` + ); } else { skipped.scrub_failed += 1; + console.warn( + `Kept metadata token for integration ${integration.id}: stored credential could not be read` + ); } } continue; @@ -306,7 +313,12 @@ async function storedCredentialMatches( const owner = readOwner(integration); if (!owner) return false; - const credential = await getSlackCredentialByIntegrationId(integrationId); + let credential: SlackOAuthCredential | null; + try { + credential = await getSlackCredentialByIntegrationId(integrationId); + } catch { + return null; + } if (!credential) return false; try { From 1227e52c3e3a355a3fdbd87865c9168b35fa8a3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Mon, 19 Oct 2026 12:18:54 +0000 Subject: [PATCH 5/6] docs(slack): align comments and reports with the completed cutover Update the maintenance script's header, summary labels, and code comments, the credential store and keyset doc comments, and one test name so they describe the finished behavior: eligibility for the metadata rewrite, the counters an operator reads, and the install path requiring the credential write to succeed. --- .../platforms/slack/credential-keyset.ts | 4 ++-- .../platforms/slack/credential-store.ts | 4 ++-- .../lib/integrations/slack-service.test.ts | 2 +- .../web/src/lib/integrations/slack-service.ts | 4 ++-- ...-08-07_backfill-slack-oauth-credentials.ts | 20 ++++++++++--------- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/apps/web/src/lib/integrations/platforms/slack/credential-keyset.ts b/apps/web/src/lib/integrations/platforms/slack/credential-keyset.ts index 901673622a1..308558fab8e 100644 --- a/apps/web/src/lib/integrations/platforms/slack/credential-keyset.ts +++ b/apps/web/src/lib/integrations/platforms/slack/credential-keyset.ts @@ -146,8 +146,8 @@ let cached: SlackCredentialKeyset | undefined; /** * Resolves the keyset, memoized per process. Throws `SlackCredentialKeysetError` - * when unconfigured or invalid — callers on the install path treat that as - * non-fatal while the new store is still write-only. + * when unconfigured or invalid — the install path requires the credential write + * to succeed, so an unconfigured keyset fails the install. */ export function requireSlackCredentialKeyset(): SlackCredentialKeyset { if (cached) return cached; diff --git a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts index f1b98d0cad7..04db18619b8 100644 --- a/apps/web/src/lib/integrations/platforms/slack/credential-store.ts +++ b/apps/web/src/lib/integrations/platforms/slack/credential-store.ts @@ -18,8 +18,8 @@ import { * System of record for Slack bot credentials. * * Install writes go through here and all server read paths decrypt from here; the - * legacy plaintext `platform_integrations.metadata.access_token` copy is no longer - * written and is removed from existing rows by the backfill script + * legacy metadata copy on `platform_integrations` is no longer written and is + * removed from existing rows by the backfill script * (`src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts`). */ diff --git a/apps/web/src/lib/integrations/slack-service.test.ts b/apps/web/src/lib/integrations/slack-service.test.ts index 2b13cc8b3a5..a7cc9e103e1 100644 --- a/apps/web/src/lib/integrations/slack-service.test.ts +++ b/apps/web/src/lib/integrations/slack-service.test.ts @@ -347,7 +347,7 @@ describe('upsertSlackInstallation', () => { ); }); - it('preserves the selected model and scrubs any stale metadata token when refreshing an existing installation', async () => { + it('preserves the selected model when refreshing an existing installation', async () => { mockLimit.mockResolvedValue([ buildSlackIntegration({ metadata: { diff --git a/apps/web/src/lib/integrations/slack-service.ts b/apps/web/src/lib/integrations/slack-service.ts index 63e97ea6c93..b744a883b73 100644 --- a/apps/web/src/lib/integrations/slack-service.ts +++ b/apps/web/src/lib/integrations/slack-service.ts @@ -274,8 +274,8 @@ export async function upsertSlackInstallation({ // Non-secret workspace identifiers only. The bot token lives solely in the // encrypted `slack_oauth_credentials` store; dropping `access_token` here also - // scrubs a stale copy from rows that predate the credential store (a reinstall - // refreshes the whole metadata object). + // removes any leftover copy from rows written before the credential store + // existed (a reinstall refreshes the whole metadata object). const existingMetadata = existing?.metadata && typeof existing.metadata === 'object' ? existing.metadata : {}; const metadata = { diff --git a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts index e0a0369c475..2f850320874 100644 --- a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts +++ b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts @@ -11,12 +11,14 @@ * keeps its fresh token instead of being overwritten by this run's older metadata * snapshot. * - * The scrub pass (on by default with --execute; opt out with --no-scrub) deletes - * `metadata.access_token` for every Slack integration that has a credential row, - * preserving all other metadata keys. Rows without a credential row keep their - * plaintext token and are reported, so the operation is safe to re-run after fixing - * them. Run this only after the cutover that reads tokens from the credential store - * is deployed. + * The scrub pass (on by default with --execute; opt out with --no-scrub) removes + * the token key from metadata for every eligible Slack integration, preserving all + * other metadata keys. A row is eligible only when its stored credential is + * current: the credential was written by this run's copy pass, or an existing + * credential decrypts to exactly the metadata token. Every other row keeps its + * metadata unchanged and is reported, so the operation is safe to re-run after + * the reported rows are resolved. Run this only after the cutover that reads + * tokens from the credential store is deployed. * * Requires SLACK_CREDENTIAL_KEYSET_JSON. Never prints token material. * @@ -131,7 +133,7 @@ async function main(): Promise { // Cheap pre-filter only: it avoids opening a transaction per already-migrated // integration. `createSlackCredentialIfAbsent` is what actually guarantees we never - // replace a row, since this snapshot can go stale before the write. + // replace a row, since the metadata can change between this read and the write. const existingRows = await db .select({ integrationId: slack_oauth_credentials.platform_integration_id }) .from(slack_oauth_credentials) @@ -255,8 +257,8 @@ async function main(): Promise { console.log('\n--- Summary ---'); console.log(`Eligible for backfill: ${eligible}`); console.log(`Migrated: ${migrated}`); - console.log(`Scrubbed plaintext tokens: ${scrubbed}`); - console.log(`Kept (no credential row): ${skipped.scrub_no_credential}`); + console.log(`Token fields removed: ${scrubbed}`); + console.log(`Kept (credential not current): ${skipped.scrub_no_credential}`); console.log(`Failed scrubs: ${skipped.scrub_failed}`); console.log(`Skipped (already migrated): ${skipped.already_migrated}`); console.log(`Skipped (no plaintext): ${skipped.no_plaintext_token}`); From 9bd94535b4b706a4859480c68954181864c7379b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Sat, 24 Oct 2026 10:53:36 +0000 Subject: [PATCH 6/6] refactor(slack): name the maintenance script's metadata pass consistently --- ...-08-07_backfill-slack-oauth-credentials.ts | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts index 0f8d897c859..e72c159ab9b 100644 --- a/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts +++ b/apps/web/src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts @@ -10,7 +10,7 @@ * keeps its fresh credential instead of being overwritten by this run's earlier * snapshot. * - * The normalization pass (on by default with --execute; opt out with --no-scrub) + * The normalization pass (on by default with --execute; opt out with --no-normalize) * removes the credential field from metadata for every Slack integration whose * stored credential is current: either this run wrote it, or the stored credential * decrypts to the same value. Every other integration keeps its metadata unchanged @@ -23,7 +23,7 @@ * Usage: * pnpm --filter web script src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts * pnpm --filter web script src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts --execute - * pnpm --filter web script src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts --execute --no-scrub + * pnpm --filter web script src/scripts/d2026-08-07_backfill-slack-oauth-credentials.ts --execute --no-normalize */ import '../lib/load-env'; @@ -47,19 +47,19 @@ type SkipReason = | 'no_owner' | 'no_team_id' | 'write_failed' - | 'scrub_no_credential' - | 'scrub_failed'; + | 'normalize_not_current' + | 'normalize_failed'; -function parseArgs(): { execute: boolean; scrub: boolean; batchSize: number } { +function parseArgs(): { execute: boolean; normalize: boolean; batchSize: number } { let execute = false; - let scrub = true; + let normalize = true; let batchSize = 200; for (const arg of process.argv.slice(2)) { if (arg === '--execute') { execute = true; - } else if (arg === '--no-scrub') { - scrub = false; + } else if (arg === '--no-normalize') { + normalize = false; } else if (arg.startsWith('--batch-size=')) { const parsed = Number(arg.slice('--batch-size='.length)); if (!Number.isInteger(parsed) || parsed <= 0) { @@ -71,7 +71,7 @@ function parseArgs(): { execute: boolean; scrub: boolean; batchSize: number } { } } - return { execute, scrub, batchSize }; + return { execute, normalize, batchSize }; } function readMetadataCredential(metadata: unknown): string | null { @@ -82,7 +82,7 @@ function readMetadataCredential(metadata: unknown): string | null { } async function main(): Promise { - const { execute, scrub, batchSize } = parseArgs(); + const { execute, normalize, batchSize } = parseArgs(); // Fail fast rather than discovering a bad keyset partway through a run. const { active } = requireSlackCredentialKeyset(); @@ -115,16 +115,16 @@ async function main(): Promise { no_owner: 0, no_team_id: 0, write_failed: 0, - scrub_no_credential: 0, - scrub_failed: 0, + normalize_not_current: 0, + normalize_failed: 0, }; let migrated = 0; let eligible = 0; - let scrubbed = 0; + let normalized = 0; // Integrations whose credential row this run can vouch for: either the row was // written by this run, or it decrypts to the metadata value (checked below). // Only these are eligible for the normalization pass. - const scrubCandidateIds = new Set(); + const normalizationCandidateIds = new Set(); for (let offset = 0; offset < integrations.length; offset += batchSize) { const batch = integrations.slice(offset, offset + batchSize); @@ -149,17 +149,17 @@ async function main(): Promise { // The stored credential must be current before the metadata field is // removed. The comparison below admits the row for normalization only on an // exact match; any other result keeps it unchanged. - if (execute && scrub && readMetadataCredential(integration.metadata)) { + if (execute && normalize && readMetadataCredential(integration.metadata)) { const fresh = await storedCredentialMatches(integration.id, integration); if (fresh === true) { - scrubCandidateIds.add(integration.id); + normalizationCandidateIds.add(integration.id); } else if (fresh === false) { - skipped.scrub_no_credential += 1; + skipped.normalize_not_current += 1; console.warn( `Left metadata unchanged for integration ${integration.id}: stored credential does not match it` ); } else { - skipped.scrub_failed += 1; + skipped.normalize_failed += 1; console.warn( `Left metadata unchanged for integration ${integration.id}: stored credential could not be read` ); @@ -204,7 +204,7 @@ async function main(): Promise { skipped.already_migrated += 1; } else { migrated += 1; - scrubCandidateIds.add(integration.id); + normalizationCandidateIds.add(integration.id); } } catch (error) { skipped.write_failed += 1; @@ -223,9 +223,9 @@ async function main(): Promise { // and left unchanged so the script can be repeated after it is resolved. // The metadata update removes only that key (`jsonb -`), so concurrent writes // to other metadata fields between the snapshot above and this update survive. - if (execute && scrub) { + if (execute && normalize) { for (const integration of integrations) { - if (!scrubCandidateIds.has(integration.id)) continue; + if (!normalizationCandidateIds.has(integration.id)) continue; const metadata = integration.metadata; if (metadata === null || typeof metadata !== 'object') continue; if (!('access_token' in metadata)) continue; @@ -238,9 +238,9 @@ async function main(): Promise { updated_at: new Date().toISOString(), }) .where(eq(platform_integrations.id, integration.id)); - scrubbed += 1; + normalized += 1; } catch (error) { - skipped.scrub_failed += 1; + skipped.normalize_failed += 1; console.error( `Failed to normalize integration ${integration.id}: ${error instanceof Error ? error.message : 'unknown error'}` ); @@ -255,9 +255,9 @@ async function main(): Promise { console.log('\n--- Summary ---'); console.log(`Eligible for backfill: ${eligible}`); console.log(`Migrated: ${migrated}`); - console.log(`Metadata fields normalized: ${scrubbed}`); - console.log(`Kept (credential not current): ${skipped.scrub_no_credential}`); - console.log(`Failed normalizations: ${skipped.scrub_failed}`); + console.log(`Metadata fields normalized: ${normalized}`); + console.log(`Kept (credential not current): ${skipped.normalize_not_current}`); + console.log(`Failed normalizations: ${skipped.normalize_failed}`); console.log(`Skipped (already migrated): ${skipped.already_migrated}`); console.log(`Skipped (no metadata value): ${skipped.no_metadata_credential}`); console.log(`Skipped (no owner): ${skipped.no_owner}`);