diff --git a/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts b/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts index 2b9ebf843db..c987ce4f4f3 100644 --- a/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts +++ b/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts @@ -38,13 +38,20 @@ jest.mock('@kilocode/web-shared/routers/organizations/utils', () => ({ jest.mock('@/lib/slack-bot/model-allow-list', () => ({ getDefaultAllowedModel: jest.fn(async () => 'test/default-model'), })); -jest.mock('@kilocode/web-shared/lib/config.server', () => ({ - ...jest.requireActual('@kilocode/web-shared/lib/config.server'), - LINEAR_CLIENT_ID: 'test-linear-client-id', - LINEAR_CLIENT_SECRET: 'test-linear-client-secret', - SLACK_CLIENT_ID: 'test-slack-client-id', - SLACK_CLIENT_SECRET: 'test-slack-client-secret', -})); +jest.mock('@kilocode/web-shared/lib/config.server', () => { + // Test-only envelope keyset; a jest factory cannot import the module above. + const slackTestKeyset = jest.requireActual( + '@/lib/integrations/platforms/slack/credential-test-keyset' + ) as { TEST_SLACK_CREDENTIAL_KEYSET_JSON: string }; + return { + ...jest.requireActual('@kilocode/web-shared/lib/config.server'), + LINEAR_CLIENT_ID: 'test-linear-client-id', + LINEAR_CLIENT_SECRET: 'test-linear-client-secret', + SLACK_CLIENT_ID: 'test-slack-client-id', + SLACK_CLIENT_SECRET: 'test-slack-client-secret', + SLACK_CREDENTIAL_KEYSET_JSON: slackTestKeyset.TEST_SLACK_CREDENTIAL_KEYSET_JSON, + }; +}); jest.mock('@sentry/nextjs', () => ({ captureException: jest.fn(), captureMessage: jest.fn(), diff --git a/apps/web/src/app/api/integrations/slack/callback/route.test.ts b/apps/web/src/app/api/integrations/slack/callback/route.test.ts index 88fec2871de..46337773e59 100644 --- a/apps/web/src/app/api/integrations/slack/callback/route.test.ts +++ b/apps/web/src/app/api/integrations/slack/callback/route.test.ts @@ -2,7 +2,11 @@ import { beforeEach, describe, expect, test } from '@jest/globals'; import { NextRequest } from 'next/server'; import { eq } from 'drizzle-orm'; import { db } from '@kilocode/web-shared/lib/drizzle'; -import { kilocode_users, platform_integrations } from '@kilocode/db/schema'; +import { + kilocode_users, + platform_integrations, + slack_oauth_credentials, +} from '@kilocode/db/schema'; import { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; import { createOAuthState } from '@/lib/integrations/oauth-state'; @@ -45,6 +49,16 @@ jest.mock('@sentry/nextjs', () => ({ captureException: jest.fn(), captureMessage: jest.fn(), })); +jest.mock('@kilocode/web-shared/lib/config.server', () => { + // Test-only envelope keyset; a jest factory cannot import the module above. + const slackTestKeyset = jest.requireActual( + '@/lib/integrations/platforms/slack/credential-test-keyset' + ) as { TEST_SLACK_CREDENTIAL_KEYSET_JSON: string }; + return { + ...jest.requireActual('@kilocode/web-shared/lib/config.server'), + SLACK_CREDENTIAL_KEYSET_JSON: slackTestKeyset.TEST_SLACK_CREDENTIAL_KEYSET_JSON, + }; +}); const mockedGetUserFromAuth = jest.mocked(getUserFromAuth); const mockedBotGetAdapter = jest.mocked(bot.getAdapter); @@ -149,8 +163,14 @@ describe('GET /api/integrations/slack/callback', () => { platform: 'slack', platform_installation_id: TEAM_ID, platform_account_login: 'Acme Workspace', - metadata: expect.objectContaining({ bot_user_id: 'U0BOT', access_token: 'xoxb-new-token' }), + metadata: expect.objectContaining({ bot_user_id: 'U0BOT' }), }); + expect((stored.metadata as Record).access_token).toBeUndefined(); + const credentialRows = await db + .select() + .from(slack_oauth_credentials) + .where(eq(slack_oauth_credentials.platform_integration_id, stored.id)); + expect(credentialRows).toHaveLength(1); } finally { mockedUpsertSlackInstallation.mockReset(); await db.delete(kilocode_users).where(eq(kilocode_users.id, USER_ID)); 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-keyset.ts b/apps/web/src/lib/integrations/platforms/slack/credential-keyset.ts index 72934d0332d..70c4d2de02a 100644 --- a/apps/web/src/lib/integrations/platforms/slack/credential-keyset.ts +++ b/apps/web/src/lib/integrations/platforms/slack/credential-keyset.ts @@ -144,8 +144,7 @@ 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 the keyset is unconfigured or invalid. */ 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 1eabd085ee3..4295aa4a772 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,7 @@ 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 and every server read path of the bot credential go through here. */ export type WriteSlackCredentialInput = { @@ -73,11 +71,9 @@ 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 - * 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. + * A caller whose token comes from a snapshot rather than from a live Slack response + * needs the current row to win: existence is re-checked inside the same locked + * transaction as the write, so a credential written after the snapshot is kept. */ export async function createSlackCredentialIfAbsent( input: WriteSlackCredentialInput diff --git a/apps/web/src/lib/integrations/platforms/slack/credential-test-keyset.ts b/apps/web/src/lib/integrations/platforms/slack/credential-test-keyset.ts new file mode 100644 index 00000000000..b4d691781c3 --- /dev/null +++ b/apps/web/src/lib/integrations/platforms/slack/credential-test-keyset.ts @@ -0,0 +1,20 @@ +import { generateKeyPairSync } from 'node:crypto'; + +const KEY_ID = 'slack-credential-test-key'; + +const { publicKey, privateKey } = generateKeyPairSync('rsa', { + modulusLength: 2048, + publicKeyEncoding: { type: 'spki', format: 'pem' }, + privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, +}); + +/** + * Envelope keyset for tests that run the real Slack credential store. + * + * Test-only material, generated per test module registry. Use it where a test + * must exercise the install path end to end instead of mocking the store. + */ +export const TEST_SLACK_CREDENTIAL_KEYSET_JSON = JSON.stringify({ + active: { keyId: KEY_ID, publicKeyPem: publicKey }, + decrypt: [{ keyId: KEY_ID, privateKeyPem: privateKey }], +}); diff --git a/apps/web/src/lib/integrations/slack-service.test.ts b/apps/web/src/lib/integrations/slack-service.test.ts index 7817849a633..8ed1596c0d1 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; + }, }, })); @@ -41,8 +46,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 mockGetDefaultAllowedModel = jest.fn(); @@ -58,6 +68,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, @@ -93,7 +104,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()]); @@ -162,6 +177,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({ @@ -207,6 +233,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 () => { @@ -227,14 +257,39 @@ 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.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 () => { @@ -284,8 +339,19 @@ describe('upsertSlackInstallation', () => { mockInsertValues.mockReturnValue({ returning: mockInsertReturning }); mockInsertReturning.mockResolvedValue([buildSlackIntegration()]); mockWriteSlackCredential.mockReset(); + mockGetSlackCredentialByIntegrationId.mockReset(); + 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 }), + }) + ); mockGetDefaultAllowedModel.mockReset(); }); @@ -311,12 +377,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', }) ); @@ -336,12 +401,13 @@ 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; + expect(insertedMetadata).not.toHaveProperty('access_token'); }); it('stores no model for a new organization installation when the policy allows none', async () => { @@ -359,10 +425,7 @@ describe('upsertSlackInstallation', () => { expect(mockGetDefaultAllowedModel).toHaveBeenCalledWith(orgOwner.id, DEFAULT_BOT_MODEL); const inserted = mockInsertValues.mock.calls[0][0] as { metadata: Record }; expect('model_slug' in inserted.metadata).toBe(false); - expect(inserted.metadata).toMatchObject({ - access_token: 'xoxb-new-token', - bot_user_id: 'U_NEW_BOT', - }); + expect(inserted.metadata).toEqual({ bot_user_id: 'U_NEW_BOT' }); }); it('stores the allowed model for a new organization installation', async () => { @@ -401,7 +464,7 @@ describe('upsertSlackInstallation', () => { const updated = mockUpdateSet.mock.calls[0][0] as { metadata: Record }; expect('model_slug' in updated.metadata).toBe(false); - expect(updated.metadata.access_token).toBe('xoxb-new-token'); + expect('access_token' in updated.metadata).toBe(false); }); it('preserves the selected model on reinstall', async () => { @@ -462,12 +525,8 @@ 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 () => { + it('writes the bot token into the encrypted credential store on a new install', async () => { mockLimit.mockResolvedValueOnce([]).mockResolvedValueOnce([]); - mockWriteSlackCredential.mockRejectedValue(new Error('encryption not configured')); const installation = { botToken: 'xoxb-new-token', @@ -475,13 +534,51 @@ describe('upsertSlackInstallation', () => { teamName: 'Kilo Team', } satisfies SlackInstallation; - await expect( - upsertSlackInstallation({ owner, teamId: 'T123', installation }) - ).resolves.toMatchObject({ id: 'integration-1' }); + await upsertSlackInstallation({ owner, teamId: 'T123', installation }); - expect(mockCaptureException).toHaveBeenCalledTimes(1); - // The report must not carry token material. - expect(JSON.stringify(mockCaptureException.mock.calls[0])).not.toContain('xoxb-new-token'); + expect(mockWriteSlackCredential).toHaveBeenCalledWith( + { + integrationId: 'integration-1', + slackTeamId: 'T123', + owner, + botToken: 'xoxb-new-token', + botUserId: 'U_NEW_BOT', + }, + expect.objectContaining({ insert: expect.any(Function) }) + ); + }); + + it('writes the bot token into the credential store when refreshing an existing install', async () => { + mockLimit.mockResolvedValue([buildSlackIntegration()]); + + const installation = { + botToken: 'xoxb-rotated-token', + botUserId: 'U_NEW_BOT', + teamName: 'Kilo Team', + } satisfies SlackInstallation; + + await upsertSlackInstallation({ owner, teamId: 'T123', installation }); + + expect(mockWriteSlackCredential).toHaveBeenCalledWith( + expect.objectContaining({ botToken: 'xoxb-rotated-token', slackTeamId: 'T123' }), + expect.objectContaining({ update: expect.any(Function) }) + ); + }); + + // A failed credential write fails the install. + it('fails the install when the credential store write fails', async () => { + mockLimit.mockResolvedValueOnce([]).mockResolvedValueOnce([]); + mockWriteSlackCredential.mockRejectedValue(new Error('encryption not configured')); + + const installation = { + botToken: 'xoxb-new-token', + botUserId: 'U_NEW_BOT', + teamName: 'Kilo Team', + } satisfies SlackInstallation; + + 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 475ef3a716f..f0efefbe7fb 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, type DrizzleTransaction } 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'; @@ -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 { @@ -266,43 +271,48 @@ 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 while readers still use installation metadata. A - * supplied installation transaction also contains this write, with a savepoint - * keeping a credential failure from aborting the installation. + * Returns null when no credential row exists, the row is revoked, or the row cannot + * be read. Failures are reported to the error tracker with the integration id only; + * 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; - }, - database: typeof db | DrizzleTransaction -): Promise { +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; + } + + let credential: SlackOAuthCredential | null; try { - await writeSlackCredential( - { - integrationId: integration.id, - slackTeamId: teamId, - owner, - botToken: installation.botToken, - botUserId: installation.botUserId ?? null, - }, - database - ); + 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 { + 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; } } @@ -375,7 +385,6 @@ export async function upsertSlackInstallation( const metadata: Record = { ...(existing?.metadata && typeof existing.metadata === 'object' ? existing.metadata : {}), - access_token: installation.botToken, bot_user_id: installation.botUserId, }; if (modelSlug) { @@ -383,28 +392,42 @@ export async function upsertSlackInstallation( } else { delete metadata.model_slug; } + delete metadata.access_token; if (existing) { try { - const [updated] = await database - .update(platform_integrations) - .set({ - platform_installation_id: teamId, - platform_account_id: teamId, - platform_account_login: teamName, - scopes: SLACK_SCOPES, - integration_status: INTEGRATION_STATUS.ACTIVE, - suspended_at: null, - suspended_by: null, - metadata, - updated_at: new Date().toISOString(), - }) - .where(eq(platform_integrations.id, existing.id)) - .returning(); - - await mirrorSlackCredential({ integration: updated, owner, teamId, installation }, database); - - return updated; + return await database.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, + suspended_at: null, + suspended_by: null, + 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); @@ -414,26 +437,37 @@ export async function upsertSlackInstallation( } try { - const [created] = await database - .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 mirrorSlackCredential({ integration: created, owner, teamId, installation }, database); - - return created; + return await database.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, + installed_at: new Date().toISOString(), + }) + .returning(); + + await writeSlackCredential( + { + integrationId: created.id, + slackTeamId: teamId, + owner, + botToken: installation.botToken, + botUserId: installation.botUserId ?? null, + }, + tx + ); + + return created; + }); } catch (error) { if (isSlackWorkspaceUniqueViolation(error)) { throw new SlackWorkspaceAlreadyConnectedError(teamName); @@ -458,11 +492,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); } @@ -531,14 +565,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) { @@ -552,44 +586,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. @@ -660,16 +656,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..2f499524a43 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,26 @@ /** - * Backfills existing plaintext Slack bot tokens from - * `platform_integrations.metadata.access_token` into the encrypted - * `slack_oauth_credentials` store. + * Brings existing Slack integrations onto the encrypted `slack_oauth_credentials` + * store. * - * 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. + * Pass one writes a credential row for integrations that have none. It 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 credential written mid-run is kept. * - * 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. + * Pass two runs by default with --execute (opt out with --no-normalize) and applies + * only to integrations whose stored credential is current: either this run wrote it, + * or it decrypts to the value the integration records. Anything else is reported and + * left unchanged, so the operation is safe to re-run after the reported rows are + * resolved. Run this only after the release whose read paths use the credential + * store. * - * Requires SLACK_CREDENTIAL_KEYSET_JSON. Never prints token material. + * Requires SLACK_CREDENTIAL_KEYSET_JSON. Never prints credential 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-normalize */ import '../lib/load-env'; @@ -26,25 +28,35 @@ 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 { 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 = | 'already_migrated' - | 'no_plaintext_token' + | 'no_metadata_credential' | 'no_owner' | 'no_team_id' - | 'write_failed'; + | 'write_failed' + | 'normalize_not_current' + | 'normalize_failed'; -function parseArgs(): { execute: boolean; batchSize: number } { +function parseArgs(): { execute: boolean; normalize: boolean; batchSize: number } { let execute = false; + let normalize = true; let batchSize = 200; for (const arg of process.argv.slice(2)) { if (arg === '--execute') { execute = true; + } 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) { @@ -56,18 +68,18 @@ function parseArgs(): { execute: boolean; batchSize: number } { } } - return { execute, batchSize }; + return { execute, normalize, batchSize }; } -function readPlaintextToken(metadata: unknown): string | null { +function readMetadataCredential(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; } async function main(): Promise { - const { execute, batchSize } = parseArgs(); + const { execute, normalize, batchSize } = parseArgs(); // Fail fast rather than discovering a bad keyset partway through a run. const { active } = requireSlackCredentialKeyset(); @@ -96,20 +108,27 @@ async function main(): Promise { const skipped: Record = { already_migrated: 0, - no_plaintext_token: 0, + no_metadata_credential: 0, no_owner: 0, no_team_id: 0, write_failed: 0, + normalize_not_current: 0, + normalize_failed: 0, }; let migrated = 0; let eligible = 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 normalizationCandidateIds = new Set(); for (let offset = 0; offset < integrations.length; offset += batchSize) { const batch = integrations.slice(offset, offset + batchSize); // 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) @@ -124,12 +143,31 @@ async function main(): Promise { for (const integration of batch) { if (alreadyMigrated.has(integration.id)) { skipped.already_migrated += 1; + // Only rows whose stored credential is current qualify for the second pass. + // The comparison below admits the row on an exact match; any other result + // keeps it unchanged. + if (execute && normalize && readMetadataCredential(integration.metadata)) { + const fresh = await storedCredentialMatches(integration.id, integration); + if (fresh === true) { + normalizationCandidateIds.add(integration.id); + } else if (fresh === false) { + skipped.normalize_not_current += 1; + console.warn( + `Left metadata unchanged for integration ${integration.id}: stored credential does not match it` + ); + } else { + skipped.normalize_failed += 1; + console.warn( + `Left metadata unchanged for integration ${integration.id}: stored credential could not be read` + ); + } + } continue; } - const botToken = readPlaintextToken(integration.metadata); + const botToken = readMetadataCredential(integration.metadata); if (!botToken) { - skipped.no_plaintext_token += 1; + skipped.no_metadata_credential += 1; continue; } @@ -163,6 +201,7 @@ async function main(): Promise { skipped.already_migrated += 1; } else { migrated += 1; + normalizationCandidateIds.add(integration.id); } } catch (error) { skipped.write_failed += 1; @@ -174,6 +213,37 @@ async function main(): Promise { } } + // Pass two: finalize the integration row for every eligible integration. + // - A row is eligible only when its stored credential is current: this run wrote it, + // or the stored credential decrypts to the same value. Every other row is reported + // and left unchanged so the script can be repeated after it is resolved. + // The update changes only that key (`jsonb -`), so concurrent writes to other + // metadata fields between the snapshot above and this update survive. + if (execute && normalize) { + for (const integration of integrations) { + if (!normalizationCandidateIds.has(integration.id)) continue; + const metadata = integration.metadata; + if (metadata === null || typeof metadata !== 'object') continue; + if (!('access_token' in metadata)) continue; + + try { + await db + .update(platform_integrations) + .set({ + metadata: sql`${platform_integrations.metadata} - 'access_token'`, + updated_at: new Date().toISOString(), + }) + .where(eq(platform_integrations.id, integration.id)); + normalized += 1; + } catch (error) { + skipped.normalize_failed += 1; + console.error( + `Failed to normalize integration ${integration.id}: ${error instanceof Error ? error.message : 'unknown error'}` + ); + } + } + } + const [{ total }] = await db .select({ total: sql`count(*)::int` }) .from(slack_oauth_credentials); @@ -181,8 +251,11 @@ async function main(): Promise { console.log('\n--- Summary ---'); console.log(`Eligible for backfill: ${eligible}`); console.log(`Migrated: ${migrated}`); + 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 plaintext): ${skipped.no_plaintext_token}`); + console.log(`Skipped (no metadata value): ${skipped.no_metadata_credential}`); console.log(`Skipped (no owner): ${skipped.no_owner}`); console.log(`Skipped (no team ID): ${skipped.no_team_id}`); console.log(`Failed writes: ${skipped.write_failed}`); @@ -211,10 +284,46 @@ 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 value. + * + * 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 credential material. + */ +async function storedCredentialMatches( + integrationId: string, + integration: { + owned_by_organization_id: string | null; + owned_by_user_id: string | null; + metadata: unknown; + } +): Promise { + const botToken = readMetadataCredential(integration.metadata); + if (!botToken) return false; + const owner = readOwner(integration); + if (!owner) return false; + + let credential: SlackOAuthCredential | null; + try { + credential = await getSlackCredentialByIntegrationId(integrationId); + } catch { + return null; + } + if (!credential) return false; + + try { + return decryptSlackBotToken(credential, owner) === botToken; + } catch { + return null; + } +} + void main() .catch(error => { console.error('Fatal error:', error);