From f3ad69a0b0e96fbfc10498f0ce3ae51da12f4a9e 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: Tue, 20 Oct 2026 02:14:04 +0000 Subject: [PATCH 1/6] fix(integrations): check integration ownership before installing a workspace Workspace install callbacks now resolve the target workspace from the OAuth code, confirm that this owner holds it, and only then write the Chat SDK installation. A workspace connected to a different owner is rejected before any installation state is written. If a competing integration appears while the install is in flight, the previously stored installation is preserved. --- .../linear/callback/route.test.ts | 188 ++++++---- .../oauth-install-conflict.test.ts | 327 ++++++++++++++++++ .../integrations/slack/callback/route.test.ts | 207 +++++++++++ .../src/lib/integrations/linear-service.ts | 29 +- .../oauth/platforms/linear-callback.ts | 101 +++--- .../oauth/platforms/slack-callback.ts | 56 ++- .../web/src/lib/integrations/slack-service.ts | 63 +++- 7 files changed, 809 insertions(+), 162 deletions(-) create mode 100644 apps/web/src/app/api/integrations/oauth-install-conflict.test.ts create mode 100644 apps/web/src/app/api/integrations/slack/callback/route.test.ts diff --git a/apps/web/src/app/api/integrations/linear/callback/route.test.ts b/apps/web/src/app/api/integrations/linear/callback/route.test.ts index b99161340c..2f58507d6a 100644 --- a/apps/web/src/app/api/integrations/linear/callback/route.test.ts +++ b/apps/web/src/app/api/integrations/linear/callback/route.test.ts @@ -6,6 +6,7 @@ import { bot } from '@/lib/bot'; import { exchangeLinearOAuthCode, fetchLinearOAuthIdentity, + getConflictingLinearInstallation, LinearWorkspaceAlreadyConnectedError, revokeLinearToken, upsertLinearInstallation, @@ -20,18 +21,9 @@ import { PLATFORM } from '@/lib/integrations/core/constants'; import { createSignedToken } from '@/lib/signed-token'; const mockIsEnabledForBot = jest.fn(); -const mockHandleOAuthCallback = jest.fn(); const mockConsumeProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); const mockCancelProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); -const mockLinearOrganization = jest.fn(async () => ({ name: 'Acme Workspace', urlKey: 'acme' })); - -jest.mock('@linear/sdk', () => ({ - LinearClient: jest.fn().mockImplementation(() => ({ - get organization() { - return mockLinearOrganization(); - }, - })), -})); + jest.mock('@kilocode/web-shared/lib/user/server'); jest.mock('@/lib/bot', () => ({ bot: { @@ -47,6 +39,7 @@ jest.mock('@/lib/integrations/linear-service', () => { upsertLinearInstallation: jest.fn(), exchangeLinearOAuthCode: jest.fn(), fetchLinearOAuthIdentity: jest.fn(), + getConflictingLinearInstallation: jest.fn(), revokeLinearToken: jest.fn(async () => true), }; }); @@ -84,6 +77,7 @@ const mockedUpsertLinearInstallation = jest.mocked(upsertLinearInstallation); const mockedUnlinkTeamKiloUsers = jest.mocked(unlinkTeamKiloUsers); const mockedExchangeLinearOAuthCode = jest.mocked(exchangeLinearOAuthCode); const mockedFetchLinearOAuthIdentity = jest.mocked(fetchLinearOAuthIdentity); +const mockedGetConflictingLinearInstallation = jest.mocked(getConflictingLinearInstallation); const mockedRevokeLinearToken = jest.mocked(revokeLinearToken); const mockedLinkKiloUser = jest.mocked(linkKiloUser); const mockedVerifyLinearBotLinkState = jest.mocked(verifyLinearBotLinkState); @@ -119,6 +113,10 @@ async function callLinearCallback(request: NextRequest) { } describe('GET /api/integrations/linear/callback', () => { + let getInstallation: jest.Mock, [string]>; + let setInstallation: jest.Mock, [string, unknown]>; + let deleteInstallation: jest.Mock, [string]>; + beforeEach(() => { jest.clearAllMocks(); mockConsumeProviderOAuthAttempt.mockResolvedValue(true); @@ -134,21 +132,28 @@ describe('GET /api/integrations/linear/callback', () => { authFailedResponse: null, } as never); - mockHandleOAuthCallback.mockResolvedValue({ + mockedExchangeLinearOAuthCode.mockResolvedValue({ + accessToken: 'lin_app_tok', + refreshToken: null, + expiresIn: null, + scope: null, + }); + mockedFetchLinearOAuthIdentity.mockResolvedValue({ + viewerId: 'linear-bot-user', + viewerName: 'Kilo', organizationId: ORGANIZATION_ID, - installation: { - accessToken: 'tok', - botUserId: 'bot-1', - organizationId: ORGANIZATION_ID, - expiresAt: null, - }, + organizationName: 'Acme Workspace', }); + mockedGetConflictingLinearInstallation.mockResolvedValue(null); + + getInstallation = jest.fn, [string]>(async () => null); + setInstallation = jest.fn, [string, unknown]>(async () => undefined); + deleteInstallation = jest.fn, [string]>(async () => undefined); mockedBotGetAdapter.mockReturnValue({ - handleOAuthCallback: mockHandleOAuthCallback, - deleteInstallation: jest.fn(async () => undefined), + getInstallation, + setInstallation, + deleteInstallation, } as never); - - mockLinearOrganization.mockResolvedValue({ name: 'Acme Workspace', urlKey: 'acme' }); }); test('redirects to sign-in when user is unauthenticated', async () => { @@ -171,7 +176,7 @@ describe('GET /api/integrations/linear/callback', () => { expectRedirectLocation(response, '/integrations/linear?error=access_denied'); expect(mockCancelProviderOAuthAttempt).toHaveBeenCalledTimes(1); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); }); @@ -209,7 +214,7 @@ describe('GET /api/integrations/linear/callback', () => { expectRedirectLocation(response, '/integrations/linear?error=missing_code'); expect(mockCancelProviderOAuthAttempt).toHaveBeenCalledTimes(1); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); }); @@ -244,7 +249,7 @@ describe('GET /api/integrations/linear/callback', () => { ); expectRedirectLocation(response, '/integrations/linear?success=installed'); - expect(mockHandleOAuthCallback).toHaveBeenCalledTimes(1); + expect(mockedExchangeLinearOAuthCode).toHaveBeenCalledWith('abc'); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); } finally { @@ -262,7 +267,7 @@ describe('GET /api/integrations/linear/callback', () => { makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); expectRedirectLocation(response, '/integrations?error=invalid_state'); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); @@ -278,24 +283,39 @@ describe('GET /api/integrations/linear/callback', () => { makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); expectRedirectLocation(response, '/integrations?error=invalid_state'); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedExchangeLinearOAuthCode).not.toHaveBeenCalled(); expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); expect(mockConsumeProviderOAuthAttempt).not.toHaveBeenCalled(); expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); }); - test('invokes upsertLinearInstallation with the workspace name from Linear GraphQL', async () => { + test('persists the installation and the integration row for an unclaimed workspace', async () => { mockedUpsertLinearInstallation.mockResolvedValue({} as never); const state = createProviderState(); const response = await callLinearCallback( makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); + expect(mockedExchangeLinearOAuthCode).toHaveBeenCalledWith('abc'); + expect(mockedFetchLinearOAuthIdentity).toHaveBeenCalledWith('lin_app_tok'); + expect(mockedGetConflictingLinearInstallation).toHaveBeenCalledWith( + { type: 'user', id: USER_ID }, + ORGANIZATION_ID + ); + expect(setInstallation).toHaveBeenCalledTimes(1); + expect(setInstallation).toHaveBeenCalledWith(ORGANIZATION_ID, { + accessToken: 'lin_app_tok', + botUserId: 'linear-bot-user', + expiresAt: null, + organizationId: ORGANIZATION_ID, + }); + expect(mockedUpsertLinearInstallation).toHaveBeenCalledWith( expect.objectContaining({ owner: { type: 'user', id: USER_ID }, organizationId: ORGANIZATION_ID, organizationName: 'Acme Workspace', + botUserId: 'linear-bot-user', }), expect.objectContaining({ getChatSdkAccessToken: expect.any(Function), @@ -331,56 +351,84 @@ describe('GET /api/integrations/linear/callback', () => { ); }); - test('falls back to organizationId when the Linear GraphQL query fails', async () => { - mockLinearOrganization.mockRejectedValue(new Error('Unauthorized')); + test('carries the token lifetime and refresh token into the stored installation', async () => { + mockedExchangeLinearOAuthCode.mockResolvedValue({ + accessToken: 'lin_app_tok', + refreshToken: 'lin_refresh_tok', + expiresIn: 3600, + scope: null, + }); mockedUpsertLinearInstallation.mockResolvedValue({} as never); + + const before = Date.now(); const state = createProviderState(); await callLinearCallback( makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); + const after = Date.now(); - expect(mockedUpsertLinearInstallation).toHaveBeenCalledWith( - expect.objectContaining({ - organizationName: ORGANIZATION_ID, - }), - expect.anything() + const installation = setInstallation.mock.calls[0]?.[1] as { + expiresAt: number; + refreshToken?: string; + }; + expect(installation.refreshToken).toBe('lin_refresh_tok'); + expect(installation.expiresAt).toBeGreaterThanOrEqual(before + 3_600_000); + expect(installation.expiresAt).toBeLessThanOrEqual(after + 3_600_000); + }); + + test('leaves the stored installation and identity links untouched when another owner holds the workspace', async () => { + mockedGetConflictingLinearInstallation.mockResolvedValue({ id: 'pi_other' } as never); + + const state = createProviderState(); + const response = await callLinearCallback( + makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); + + expect(setInstallation).not.toHaveBeenCalled(); + expect(deleteInstallation).not.toHaveBeenCalled(); + expect(mockedUnlinkTeamKiloUsers).not.toHaveBeenCalled(); + expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/linear?error=workspace_already_connected'); }); - test('rolls back the Chat SDK installation when the workspace is already connected', async () => { + test('restores the installation it replaced when the workspace is claimed after the check', async () => { + const previousInstallation = { + accessToken: 'prior_tok', + botUserId: 'linear-bot-user', + expiresAt: null, + organizationId: ORGANIZATION_ID, + }; + getInstallation.mockResolvedValue(previousInstallation); mockedUpsertLinearInstallation.mockRejectedValue( new LinearWorkspaceAlreadyConnectedError('Acme') ); - const deleteInstallation = jest.fn, [string]>(async () => undefined); - const adapter = { - handleOAuthCallback: jest.fn(async () => ({ - organizationId: ORGANIZATION_ID, - installation: { - accessToken: 'tok', - botUserId: 'bot-1', - organizationId: ORGANIZATION_ID, - expiresAt: null, - }, - })), - withInstallation: jest.fn(async (_orgId: unknown, fn: () => Promise | T) => fn()), - getUser: jest.fn(async () => ({ fullName: 'Acme', userName: 'acme' })), - deleteInstallation, - }; - mockedBotGetAdapter.mockReturnValue(adapter as never); const state = createProviderState(); const response = await callLinearCallback( makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) ); + expect(setInstallation).toHaveBeenCalledTimes(2); + expect(setInstallation).toHaveBeenLastCalledWith(ORGANIZATION_ID, previousInstallation); + expect(deleteInstallation).not.toHaveBeenCalled(); + expect(mockedUnlinkTeamKiloUsers).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/linear?error=workspace_already_connected'); + }); + + test('drops only the installation it created when the workspace is claimed after the check', async () => { + mockedUpsertLinearInstallation.mockRejectedValue( + new LinearWorkspaceAlreadyConnectedError('Acme') + ); + + const state = createProviderState(); + const response = await callLinearCallback( + makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) + ); + + expect(getInstallation).toHaveBeenCalledWith(ORGANIZATION_ID); expect(deleteInstallation).toHaveBeenCalledTimes(1); expect(deleteInstallation).toHaveBeenCalledWith(ORGANIZATION_ID); - expect(mockedUnlinkTeamKiloUsers).toHaveBeenCalledTimes(1); - expect(mockedUnlinkTeamKiloUsers).toHaveBeenCalledWith( - expect.anything(), - 'linear', - ORGANIZATION_ID - ); + expect(mockedUnlinkTeamKiloUsers).not.toHaveBeenCalled(); expectRedirectLocation(response, '/integrations/linear?error=workspace_already_connected'); }); }); @@ -400,11 +448,12 @@ describe('GET /api/integrations/linear/callback bot-link flow', () => { authFailedResponse: null, } as never); - // The default install-flow adapter must NOT be invoked when the bot-link - // state matches; tests assert this. + // The install-flow adapter must NOT be invoked when the bot-link state + // matches; tests assert the install branch never persists an installation. mockedBotGetAdapter.mockReturnValue({ - handleOAuthCallback: jest.fn(async () => { - throw new Error('handleOAuthCallback should not be called for bot-link'); + getInstallation: jest.fn(async () => null), + setInstallation: jest.fn(async () => { + throw new Error('the workspace install flow must not run for a bot-link callback'); }), deleteInstallation: jest.fn(async () => undefined), } as never); @@ -609,24 +658,11 @@ describe('GET /api/integrations/linear/callback bot-link flow', () => { }); test('does not invoke the workspace install flow when a bot-link state matches', async () => { - const handleOAuthCallback = jest.fn(async () => ({ - organizationId: ORGANIZATION_ID, - installation: { - accessToken: 'tok', - botUserId: 'bot-1', - organizationId: ORGANIZATION_ID, - expiresAt: null, - }, - })); - mockedBotGetAdapter.mockReturnValue({ - handleOAuthCallback, - deleteInstallation: jest.fn(async () => undefined), - } as never); - await callLinearCallback( makeRequest('/api/integrations/linear/callback?code=abc&state=signed-bot-link') ); - expect(handleOAuthCallback).not.toHaveBeenCalled(); + expect(mockedGetConflictingLinearInstallation).not.toHaveBeenCalled(); + expect(mockedUpsertLinearInstallation).not.toHaveBeenCalled(); }); }); 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 new file mode 100644 index 0000000000..938c3ee1af --- /dev/null +++ b/apps/web/src/app/api/integrations/oauth-install-conflict.test.ts @@ -0,0 +1,327 @@ +import { afterEach, beforeAll, beforeEach, describe, expect, test } from '@jest/globals'; +import { NextRequest } from 'next/server'; +import { and, eq } from 'drizzle-orm'; +import { db } from '@kilocode/web-shared/lib/drizzle'; +import { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; +import { platform_integrations, type User } from '@kilocode/db/schema'; +import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; +import { createOAuthState } from '@/lib/integrations/oauth-state'; +import { INTEGRATION_STATUS, PLATFORM } from '@/lib/integrations/core/constants'; + +const mockInstallations = new Map(); +const mockIdentityLinks = new Map>(); +const mockGetInstallation = jest.fn(async (installationId: string) => { + return mockInstallations.get(installationId) ?? null; +}); +const mockSetInstallation = jest.fn(async (installationId: string, installation: unknown) => { + mockInstallations.set(installationId, installation); +}); +const mockDeleteInstallation = jest.fn(async (installationId: string) => { + mockInstallations.delete(installationId); +}); +const mockUnlinkTeamKiloUsers = jest.fn( + async (_state: unknown, platform: string, teamId: string) => { + mockIdentityLinks.delete(`${platform}:${teamId}`); + return 0; + } +); +const mockLinearOrganizationId = { current: 'linear-workspace' }; +const mockSlackAccess = jest.fn(); +const mockConsumeProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); + +jest.mock('@kilocode/web-shared/lib/user/server'); +jest.mock('@kilocode/web-shared/routers/organizations/utils', () => ({ + ensureOrganizationAccess: jest.fn(), +})); +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('@sentry/nextjs', () => ({ + captureException: jest.fn(), + captureMessage: jest.fn(), +})); +jest.mock('@linear/sdk', () => ({ + LinearClient: jest.fn(() => ({ + get viewer() { + return Promise.resolve({ id: 'linear-bot-user', displayName: 'Kilo' }); + }, + get organization() { + return Promise.resolve({ + id: mockLinearOrganizationId.current, + name: 'Victim Workspace', + urlKey: 'victim', + }); + }, + })), +})); +jest.mock('@slack/web-api', () => ({ + WebClient: jest.fn(() => ({ oauth: { v2: { access: mockSlackAccess } } })), +})); +jest.mock('@/lib/bot', () => ({ + bot: { + initialize: jest.fn(async () => undefined), + getAdapter: jest.fn(() => ({ + getInstallation: (installationId: string) => mockGetInstallation(installationId), + setInstallation: (installationId: string, installation: unknown) => + mockSetInstallation(installationId, installation), + deleteInstallation: (installationId: string) => mockDeleteInstallation(installationId), + })), + getState: jest.fn(() => ({ kind: 'state' })), + }, +})); +jest.mock('@/lib/bot-identity', () => ({ + linkKiloUser: jest.fn(async () => undefined), + unlinkTeamKiloUsers: (state: unknown, platform: string, teamId: string) => + mockUnlinkTeamKiloUsers(state, platform, teamId), +})); +jest.mock('@/lib/bot/linear-link-state', () => ({ + verifyLinearBotLinkState: jest.fn(() => null), +})); +jest.mock('@/lib/bot/platform-helpers', () => ({ + getPlatformIntegrationById: jest.fn(), + canKiloUserAccessPlatformIntegration: jest.fn(), +})); +jest.mock('@/lib/bot/platforms', () => ({ + botPlatforms: { require: jest.fn() }, +})); +jest.mock('@/lib/integrations/provider-oauth-attempts', () => ({ + consumeProviderOAuthAttempt: (input: unknown) => mockConsumeProviderOAuthAttempt(input), + cancelProviderOAuthAttempt: jest.fn(async () => true), +})); + +const mockedGetUserFromAuth = jest.mocked(getUserFromAuth); + +const createProviderState = (userId: string) => + createOAuthState(`user_${userId}`, userId, undefined, 'provider_install'); + +async function callCallback(platform: 'linear' | 'slack', query: string) { + const { GET } = await import('./[platform]/callback/route'); + return GET( + new NextRequest(`http://localhost:3000/api/integrations/${platform}/callback${query}`), + { + params: Promise.resolve({ platform }), + } + ); +} + +function redirectPath(response: Response) { + const location = response.headers.get('location'); + const url = new URL(location ?? ''); + return `${url.pathname}${url.search}`; +} + +async function seedIntegration(user: User, platform: 'linear' | 'slack', installationId: string) { + const [row] = await db + .insert(platform_integrations) + .values({ + owned_by_user_id: user.id, + owned_by_organization_id: null, + platform, + integration_type: 'oauth', + platform_installation_id: installationId, + platform_account_id: installationId, + platform_account_login: 'Existing Workspace', + scopes: [], + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: { bot_enabled: true }, + installed_at: '2026-01-01T00:00:00.000Z', + }) + .returning(); + return row; +} + +async function findIntegration(ownerUserId: string, platform: 'linear' | 'slack') { + const [row] = await db + .select() + .from(platform_integrations) + .where( + and( + eq(platform_integrations.owned_by_user_id, ownerUserId), + eq(platform_integrations.platform, platform) + ) + ); + return row ?? null; +} + +const existingLinearInstallation = { + accessToken: 'existing-linear-token', + botUserId: 'linear-bot-user', + expiresAt: null, + organizationId: 'linear-workspace', +}; +const existingSlackInstallation = { + botToken: 'xoxb-existing-token', + botUserId: 'U0EXISTING', + teamName: 'Existing Team', +}; + +let owner: User; +let otherOwner: User; + +beforeEach(() => { + mockInstallations.clear(); + mockIdentityLinks.clear(); + jest.clearAllMocks(); + mockConsumeProviderOAuthAttempt.mockResolvedValue(true); + mockLinearOrganizationId.current = 'linear-workspace'; + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url; + if (url === 'https://api.linear.app/oauth/token') { + return new Response(JSON.stringify({ access_token: 'lin-app-token', expires_in: 86_400 }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + } + throw new Error(`unexpected outbound request: ${url}`); + }) as typeof fetch; + + mockSlackAccess.mockResolvedValue({ + ok: true, + access_token: 'xoxb-new-token', + bot_user_id: 'U0BOT', + team: { id: 'T0TEAM', name: 'Acme Workspace' }, + is_enterprise_install: false, + }); + + mockedGetUserFromAuth.mockResolvedValue({ + user: { id: otherOwner.id }, + authFailedResponse: null, + } as never); +}); + +beforeAll(async () => { + owner = await insertTestUser({ + google_user_email: 'oauth-conflict-owner@example.com', + google_user_name: 'OAuth Conflict Owner', + }); + otherOwner = await insertTestUser({ + google_user_email: 'oauth-conflict-other@example.com', + google_user_name: 'OAuth Conflict Other', + }); +}); + +afterEach(async () => { + for (const user of [owner, otherOwner]) { + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, user.id)); + } +}); + +describe('Linear install callback against the real integration row', () => { + test('rejects a workspace held by another owner without changing its installation or links', async () => { + const victimRow = await seedIntegration(owner, PLATFORM.LINEAR, 'linear-workspace'); + mockInstallations.set('linear-workspace', existingLinearInstallation); + mockIdentityLinks.set('linear:linear-workspace', new Set(['linear-user-1'])); + + const response = await callCallback( + 'linear', + `?code=abc&state=${createProviderState(otherOwner.id)}` + ); + + expect(redirectPath(response)).toBe('/integrations/linear?error=workspace_already_connected'); + expect(mockSetInstallation).not.toHaveBeenCalled(); + expect(mockDeleteInstallation).not.toHaveBeenCalled(); + expect(mockInstallations.get('linear-workspace')).toBe(existingLinearInstallation); + expect(mockIdentityLinks.get('linear:linear-workspace')).toEqual(new Set(['linear-user-1'])); + expect(mockUnlinkTeamKiloUsers).not.toHaveBeenCalled(); + + const unchanged = await findIntegration(owner.id, PLATFORM.LINEAR); + expect(unchanged).toMatchObject({ + id: victimRow.id, + platform_installation_id: 'linear-workspace', + platform_account_login: 'Existing Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: { bot_enabled: true }, + updated_at: victimRow.updated_at, + }); + expect(await findIntegration(otherOwner.id, PLATFORM.LINEAR)).toBeNull(); + }); + + test('connects an unclaimed workspace and stores its installation and row', async () => { + mockLinearOrganizationId.current = 'linear-workspace-new'; + + const response = await callCallback( + 'linear', + `?code=abc&state=${createProviderState(otherOwner.id)}` + ); + + expect(redirectPath(response)).toBe('/integrations/linear?success=installed'); + expect(mockInstallations.get('linear-workspace-new')).toMatchObject({ + accessToken: 'lin-app-token', + botUserId: 'linear-bot-user', + organizationId: 'linear-workspace-new', + }); + + const row = await findIntegration(otherOwner.id, PLATFORM.LINEAR); + expect(row).toMatchObject({ + platform_installation_id: 'linear-workspace-new', + platform_account_login: 'Victim Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: expect.objectContaining({ bot_enabled: true, bot_user_id: 'linear-bot-user' }), + }); + }); +}); + +describe('Slack install callback against the real integration row', () => { + test('rejects a workspace held by another owner without changing its installation or links', async () => { + const victimRow = await seedIntegration(owner, PLATFORM.SLACK, 'T0TEAM'); + mockInstallations.set('T0TEAM', existingSlackInstallation); + mockIdentityLinks.set('slack:T0TEAM', new Set(['slack-user-1'])); + + const response = await callCallback( + 'slack', + `?code=abc&state=${createProviderState(otherOwner.id)}` + ); + + expect(redirectPath(response)).toBe('/integrations/slack?error=workspace_already_connected'); + expect(mockSetInstallation).not.toHaveBeenCalled(); + expect(mockDeleteInstallation).not.toHaveBeenCalled(); + expect(mockInstallations.get('T0TEAM')).toBe(existingSlackInstallation); + expect(mockIdentityLinks.get('slack:T0TEAM')).toEqual(new Set(['slack-user-1'])); + + const unchanged = await findIntegration(owner.id, PLATFORM.SLACK); + expect(unchanged).toMatchObject({ + id: victimRow.id, + platform_installation_id: 'T0TEAM', + integration_status: INTEGRATION_STATUS.ACTIVE, + updated_at: victimRow.updated_at, + }); + expect(await findIntegration(otherOwner.id, PLATFORM.SLACK)).toBeNull(); + }); + + test('connects an unclaimed workspace and stores its installation and row', async () => { + mockSlackAccess.mockResolvedValue({ + ok: true, + access_token: 'xoxb-new-token', + bot_user_id: 'U0BOT', + team: { id: 'T0NEWTEAM', name: 'New Workspace' }, + is_enterprise_install: false, + }); + + const response = await callCallback( + 'slack', + `?code=abc&state=${createProviderState(otherOwner.id)}` + ); + + expect(redirectPath(response)).toBe('/integrations/slack?success=installed'); + expect(mockInstallations.get('T0NEWTEAM')).toMatchObject({ + botToken: 'xoxb-new-token', + botUserId: 'U0BOT', + teamName: 'New Workspace', + }); + + const row = await findIntegration(otherOwner.id, PLATFORM.SLACK); + expect(row).toMatchObject({ + platform_installation_id: 'T0NEWTEAM', + platform_account_login: 'New Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: expect.objectContaining({ bot_user_id: 'U0BOT' }), + }); + }); +}); 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 new file mode 100644 index 0000000000..0feaf42e67 --- /dev/null +++ b/apps/web/src/app/api/integrations/slack/callback/route.test.ts @@ -0,0 +1,207 @@ +import { beforeEach, describe, expect, test } from '@jest/globals'; +import { NextRequest } from 'next/server'; +import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; +import { createOAuthState } from '@/lib/integrations/oauth-state'; +import { bot } from '@/lib/bot'; +import { + exchangeSlackOAuthCode, + getConflictingSlackInstallation, + SlackWorkspaceAlreadyConnectedError, + upsertSlackInstallation, +} from '@/lib/integrations/slack-service'; + +const mockConsumeProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); +const mockCancelProviderOAuthAttempt = jest.fn(async (_input: unknown) => true); + +jest.mock('@kilocode/web-shared/lib/user/server'); +jest.mock('@/lib/bot', () => ({ + bot: { + initialize: jest.fn(async () => undefined), + getAdapter: jest.fn(), + getState: jest.fn(() => ({ kind: 'state' })), + }, +})); +jest.mock('@/lib/integrations/slack-service', () => { + const actual = jest.requireActual('@/lib/integrations/slack-service'); + return { + ...actual, + upsertSlackInstallation: jest.fn(), + exchangeSlackOAuthCode: jest.fn(), + getConflictingSlackInstallation: jest.fn(), + }; +}); +jest.mock('@/lib/integrations/provider-oauth-attempts', () => ({ + consumeProviderOAuthAttempt: (input: unknown) => mockConsumeProviderOAuthAttempt(input), + cancelProviderOAuthAttempt: (input: unknown) => mockCancelProviderOAuthAttempt(input), +})); +jest.mock('@kilocode/web-shared/routers/organizations/utils', () => ({ + ensureOrganizationAccess: jest.fn(), +})); +jest.mock('@sentry/nextjs', () => ({ + captureException: jest.fn(), + captureMessage: jest.fn(), +})); + +const mockedGetUserFromAuth = jest.mocked(getUserFromAuth); +const mockedBotGetAdapter = jest.mocked(bot.getAdapter); +const mockedExchangeSlackOAuthCode = jest.mocked(exchangeSlackOAuthCode); +const mockedGetConflictingSlackInstallation = jest.mocked(getConflictingSlackInstallation); +const mockedUpsertSlackInstallation = jest.mocked(upsertSlackInstallation); + +const USER_ID = '034489e8-19e0-4479-9d69-2edad719e847'; +const TEAM_ID = 'T024BE7LD'; +const createProviderState = (returnTo?: string) => + createOAuthState(`user_${USER_ID}`, USER_ID, returnTo, 'provider_install'); + +const INSTALLATION = { + botToken: 'xoxb-new-token', + botUserId: 'U0BOT', + teamName: 'Acme Workspace', +}; + +function makeRequest(pathWithQuery: string) { + return new NextRequest(`http://localhost:3000${pathWithQuery}`); +} + +function expectRedirectLocation(response: Response, expectedPathWithQuery: string) { + const location = response.headers.get('location'); + expect(location).toBeTruthy(); + const url = new URL(location ?? ''); + expect(`${url.pathname}${url.search}`).toBe(expectedPathWithQuery); +} + +async function callSlackCallback(request: NextRequest) { + const { GET } = await import('../../[platform]/callback/route'); + return GET(request, { params: Promise.resolve({ platform: 'slack' }) }); +} + +describe('GET /api/integrations/slack/callback', () => { + let getInstallation: jest.Mock, [string]>; + let setInstallation: jest.Mock, [string, unknown]>; + let deleteInstallation: jest.Mock, [string]>; + + beforeEach(() => { + jest.clearAllMocks(); + mockConsumeProviderOAuthAttempt.mockResolvedValue(true); + mockCancelProviderOAuthAttempt.mockResolvedValue(true); + + mockedGetUserFromAuth.mockResolvedValue({ + user: { id: USER_ID }, + authFailedResponse: null, + } as never); + + mockedExchangeSlackOAuthCode.mockResolvedValue({ + installationId: TEAM_ID, + enterpriseId: undefined, + isEnterpriseInstall: false, + installation: INSTALLATION, + }); + mockedGetConflictingSlackInstallation.mockResolvedValue(null); + + getInstallation = jest.fn, [string]>(async () => null); + setInstallation = jest.fn, [string, unknown]>(async () => undefined); + deleteInstallation = jest.fn, [string]>(async () => undefined); + mockedBotGetAdapter.mockReturnValue({ + getInstallation, + setInstallation, + deleteInstallation, + } as never); + }); + + test('redirects to sign-in when the user is unauthenticated', async () => { + mockedGetUserFromAuth.mockResolvedValue({ + user: null, + authFailedResponse: new Response(null, { status: 401 }), + } as never); + + const response = await callSlackCallback(makeRequest('/api/integrations/slack/callback')); + + expect(response.headers.get('location')).toContain('/users/sign_in'); + }); + + test('persists the exchanged installation for an unclaimed workspace', async () => { + mockedUpsertSlackInstallation.mockResolvedValue({} as never); + const state = createProviderState(); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expect(mockedExchangeSlackOAuthCode).toHaveBeenCalledWith('abc'); + expect(mockedGetConflictingSlackInstallation).toHaveBeenCalledWith( + { type: 'user', id: USER_ID }, + TEAM_ID + ); + expect(setInstallation).toHaveBeenCalledTimes(1); + expect(setInstallation).toHaveBeenCalledWith(TEAM_ID, INSTALLATION); + expect(mockedUpsertSlackInstallation).toHaveBeenCalledWith({ + owner: { type: 'user', id: USER_ID }, + teamId: TEAM_ID, + installation: INSTALLATION, + }); + expectRedirectLocation(response, '/integrations/slack?success=installed'); + }); + + test('redirects successful installs to returnTo when signed state carries one', async () => { + mockedUpsertSlackInstallation.mockResolvedValue({} as never); + const state = createProviderState('/claw/new?step=slack'); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expectRedirectLocation(response, '/claw/new?step=slack&success=slack_installed'); + }); + + test('leaves the stored installation untouched when another owner holds the workspace', async () => { + mockedGetConflictingSlackInstallation.mockResolvedValue({ id: 'pi_other' } as never); + const state = createProviderState(); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expect(setInstallation).not.toHaveBeenCalled(); + expect(deleteInstallation).not.toHaveBeenCalled(); + expect(mockedUpsertSlackInstallation).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/slack?error=workspace_already_connected'); + }); + + test('restores the installation it replaced when the workspace is claimed after the check', async () => { + const previousInstallation = { + botToken: 'xoxb-prior-token', + botUserId: 'U0PRIOR', + teamName: 'Prior Workspace', + }; + getInstallation.mockResolvedValue(previousInstallation); + mockedUpsertSlackInstallation.mockRejectedValue( + new SlackWorkspaceAlreadyConnectedError('Prior Workspace') + ); + const state = createProviderState(); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expect(setInstallation).toHaveBeenCalledTimes(2); + expect(setInstallation).toHaveBeenLastCalledWith(TEAM_ID, previousInstallation); + expect(deleteInstallation).not.toHaveBeenCalled(); + expectRedirectLocation(response, '/integrations/slack?error=workspace_already_connected'); + }); + + test('drops only the installation it created when the workspace is claimed after the check', async () => { + mockedUpsertSlackInstallation.mockRejectedValue( + new SlackWorkspaceAlreadyConnectedError('Acme Workspace') + ); + const state = createProviderState(); + + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expect(getInstallation).toHaveBeenCalledWith(TEAM_ID); + expect(deleteInstallation).toHaveBeenCalledTimes(1); + expect(deleteInstallation).toHaveBeenCalledWith(TEAM_ID); + expectRedirectLocation(response, '/integrations/slack?error=workspace_already_connected'); + }); +}); diff --git a/apps/web/src/lib/integrations/linear-service.ts b/apps/web/src/lib/integrations/linear-service.ts index 568ec2e510..929fd700b9 100644 --- a/apps/web/src/lib/integrations/linear-service.ts +++ b/apps/web/src/lib/integrations/linear-service.ts @@ -160,11 +160,14 @@ export type LinearOAuthTokenResponse = { }; /** - * Exchange a Linear OAuth authorization code for a transient user-actor - * access token. This is intentionally separate from - * `linearAdapter.handleOAuthCallback` — that path persists a workspace - * installation, but the bot-link flow only needs the token long enough - * to query the viewer's identity, after which it is revoked. + * Exchange a Linear OAuth authorization code at Linear's token endpoint. + * + * Two callers share it: + * - the bot-link flow, which only needs the token long enough to query the + * viewer's identity, after which it is revoked; + * - the workspace install callback, which resolves the target workspace from + * the token before persisting a Chat SDK installation, so a workspace held + * by a different owner never has its installation replaced. */ export async function exchangeLinearOAuthCode(code: string): Promise { if (!LINEAR_CLIENT_ID || !LINEAR_CLIENT_SECRET) { @@ -273,7 +276,13 @@ function isOwnedBy(integration: PlatformIntegration, owner: Owner): boolean { : integration.owned_by_organization_id === owner.id && integration.owned_by_user_id === null; } -async function getConflictingLinearInstallation( +/** + * Find a Linear `platform_integrations` row for `organizationId` that belongs to + * a different Kilo owner. Callers use this before persisting a workspace + * installation so a workspace owned elsewhere is rejected without writing to + * the shared Chat SDK installation state. + */ +export async function getConflictingLinearInstallation( owner: Owner, organizationId: string ): Promise { @@ -318,10 +327,10 @@ type LinearUpsertOptions = { /** * Create or update a Linear `platform_integrations` row after a successful - * OAuth exchange. The OAuth access/refresh tokens are persisted inside the - * Chat SDK's state adapter by `linearAdapter.handleOAuthCallback`; this row - * stores only non-secret metadata used by the tRPC router / UI and to gate - * bot processing via `metadata.bot_enabled`. + * OAuth exchange. The OAuth access/refresh tokens live in the Chat SDK state + * adapter, written by the install callback after it has confirmed the workspace + * belongs to this owner; this row stores only non-secret metadata used by the + * tRPC router / UI and to gate bot processing via `metadata.bot_enabled`. * * When the same Kilo owner reinstalls onto a *different* Linear workspace * (`existing.platform_installation_id !== organizationId`), the optional diff --git a/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts b/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts index e13d3f7f11..b89f8e0458 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts @@ -3,11 +3,10 @@ import { NextResponse } from 'next/server'; import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; import { ensureOrganizationAccess } from '@kilocode/web-shared/routers/organizations/utils'; import { captureException, captureMessage } from '@sentry/nextjs'; -import { LinearClient } from '@linear/sdk'; import { exchangeLinearOAuthCode, fetchLinearOAuthIdentity, - LINEAR_REDIRECT_URI, + getConflictingLinearInstallation, LinearWorkspaceAlreadyConnectedError, type LinearOAuthIdentity, revokeLinearToken, @@ -49,34 +48,6 @@ async function deleteChatSdkLinearIdentityCache(organizationId: string): Promise await unlinkTeamKiloUsers(bot.getState(), PLATFORM.LINEAR, organizationId); } -/** - * Fetch the workspace name for a freshly installed Linear installation by - * querying Linear's `organization` GraphQL field with the freshly-issued - * access token. We cannot use `linearAdapter.getUser('me')` for this: with - * `actor=app` installs (which is what `getLinearOAuthUrl` uses), the - * authenticated viewer is the app actor — the configured `userName` - * (`'kilo'` / `'kilo-dev'`) — not the human installer or the workspace. - * - * Falls back to the organizationId on failure so the install still succeeds - * with a human-readable label. - */ -async function fetchLinearWorkspaceName( - accessToken: string, - organizationId: string -): Promise { - try { - const organization = await new LinearClient({ accessToken }).organization; - return organization.name || organization.urlKey || organizationId; - } catch (error) { - captureMessage('Failed to fetch Linear workspace name', { - level: 'warning', - tags: { endpoint: 'linear/callback', source: 'linear_oauth' }, - extra: { organizationId, error: error instanceof Error ? error.message : String(error) }, - }); - return organizationId; - } -} - function htmlPage(title: string, message: string, status = 200): Response { return new Response( ` @@ -334,15 +305,45 @@ export async function handleLinearOAuthCallback(request: NextRequest) { throw new Error('Linear OAuth attempt is invalid, expired, or already used'); } - // Chat SDK exchanges the code, persists the per-workspace installation in - // its state adapter, and returns the Linear organizationId + installation. + // Resolve the target Linear workspace before any Chat SDK state is + // written. The installation is stored under a workspace-global key, so a + // workspace already held by another Kilo owner has to be rejected before + // the adapter can replace that installation. await bot.initialize(); const linearAdapter = bot.getAdapter('linear'); - const { organizationId, installation } = await linearAdapter.handleOAuthCallback(request, { - redirectUri: LINEAR_REDIRECT_URI, + + const { accessToken, refreshToken, expiresIn } = await exchangeLinearOAuthCode(code); + const identity = await fetchLinearOAuthIdentity(accessToken); + const organizationId = identity.organizationId; + + const conflictRedirect = NextResponse.redirect( + new URL( + buildIntegrationOAuthRedirectPathFromOwner( + PLATFORM.LINEAR, + verifiedOwner, + 'error=workspace_already_connected', + verified?.returnTo + ), + APP_URL + ) + ); + + const conflicting = await getConflictingLinearInstallation(owner, organizationId); + if (conflicting) { + return conflictRedirect; + } + + const previousInstallation = await linearAdapter.getInstallation(organizationId); + + await linearAdapter.setInstallation(organizationId, { + accessToken, + botUserId: identity.viewerId, + expiresAt: expiresIn === null ? null : Date.now() + expiresIn * 1000, + organizationId, + ...(refreshToken ? { refreshToken } : {}), }); - const workspaceName = await fetchLinearWorkspaceName(installation.accessToken, organizationId); + const workspaceName = identity.organizationName ?? organizationId; try { await upsertLinearInstallation( @@ -350,7 +351,7 @@ export async function handleLinearOAuthCallback(request: NextRequest) { owner, organizationId, organizationName: workspaceName, - botUserId: installation.botUserId, + botUserId: identity.viewerId, }, { getChatSdkAccessToken: getChatSdkLinearAccessToken, @@ -360,25 +361,15 @@ export async function handleLinearOAuthCallback(request: NextRequest) { ); } catch (error) { if (error instanceof LinearWorkspaceAlreadyConnectedError) { - // The Chat SDK adapter already persisted the freshly-issued OAuth - // token under linear:installation:${organizationId} during - // handleOAuthCallback. Since the uniqueness check rejected this - // install, we must roll that state back — otherwise we overwrite the - // original installer's token and any future bot/webhook traffic for - // that workspace runs with mismatched credentials. - await linearAdapter.deleteInstallation(organizationId); - await unlinkTeamKiloUsers(bot.getState(), PLATFORM.LINEAR, organizationId); - return NextResponse.redirect( - new URL( - buildIntegrationOAuthRedirectPathFromOwner( - PLATFORM.LINEAR, - verifiedOwner, - 'error=workspace_already_connected', - verified?.returnTo - ), - APP_URL - ) - ); + // A competing row for this workspace appeared after the check above. + // Restore the installation this callback replaced, drop only state + // this callback created, and leave the workspace identity links alone. + if (previousInstallation) { + await linearAdapter.setInstallation(organizationId, previousInstallation); + } else { + await linearAdapter.deleteInstallation(organizationId); + } + return conflictRedirect; } throw error; } diff --git a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts index e679e699a4..f9fd6dda4e 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts @@ -4,6 +4,8 @@ import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; import { ensureOrganizationAccess } from '@kilocode/web-shared/routers/organizations/utils'; import { captureException, captureMessage } from '@sentry/nextjs'; import { + exchangeSlackOAuthCode, + getConflictingSlackInstallation, SlackWorkspaceAlreadyConnectedError, upsertSlackInstallation, } from '@/lib/integrations/slack-service'; @@ -11,7 +13,6 @@ import { isLegacyProviderOAuthState, verifyOAuthState } from '@/lib/integrations import { APP_URL } from '@kilocode/web-shared/lib/constants'; import { bot } from '@/lib/bot'; import { PLATFORM } from '@/lib/integrations/core/constants'; -import { getPlatformOAuthCallbackUrl } from '@/lib/integrations/oauth/urls'; import { appendIntegrationOAuthRedirectQuery, buildIntegrationOAuthRedirectPath, @@ -21,8 +22,6 @@ import { } from '@/lib/integrations/oauth/common'; import { consumeProviderOAuthAttempt } from '@/lib/integrations/provider-oauth-attempts'; -const SLACK_REDIRECT_URI = getPlatformOAuthCallbackUrl(PLATFORM.SLACK); - /** * Slack OAuth Callback * @@ -140,29 +139,48 @@ export async function handleSlackOAuthCallback(request: NextRequest) { throw new Error('Slack OAuth attempt is invalid, expired, or already used'); } - // 7. Let the Chat SDK exchange the code and seed its installation state + // 7. Resolve the Slack workspace before any Chat SDK state is written. The + // installation is stored under a workspace-global key, so a workspace + // already held by another Kilo owner has to be rejected before the adapter + // can replace that installation. await bot.initialize(); const slackAdapter = bot.getAdapter('slack'); - const url = new URL(request.url); - url.searchParams.set('redirect_uri', SLACK_REDIRECT_URI); - const patchedRequest = new Request(url, request); - const { teamId, installation } = await slackAdapter.handleOAuthCallback(patchedRequest); + + const { installationId, installation } = await exchangeSlackOAuthCode(code); + + const conflictRedirect = NextResponse.redirect( + new URL( + buildIntegrationOAuthRedirectPathFromState( + PLATFORM.SLACK, + state, + 'error=workspace_already_connected' + ), + APP_URL + ) + ); + + const conflicting = await getConflictingSlackInstallation(owner, installationId); + if (conflicting) { + return conflictRedirect; + } + + const previousInstallation = await slackAdapter.getInstallation(installationId); + await slackAdapter.setInstallation(installationId, installation); // 8. Store installation in database try { - await upsertSlackInstallation({ owner, teamId, installation }); + await upsertSlackInstallation({ owner, teamId: installationId, installation }); } catch (error) { if (error instanceof SlackWorkspaceAlreadyConnectedError) { - return NextResponse.redirect( - new URL( - buildIntegrationOAuthRedirectPathFromState( - PLATFORM.SLACK, - state, - 'error=workspace_already_connected' - ), - APP_URL - ) - ); + // A competing row for this workspace appeared after the check above. + // Restore the installation this callback replaced, drop only state this + // callback created, and leave the workspace identity links alone. + if (previousInstallation) { + await slackAdapter.setInstallation(installationId, previousInstallation); + } else { + await slackAdapter.deleteInstallation(installationId); + } + return conflictRedirect; } throw error; diff --git a/apps/web/src/lib/integrations/slack-service.ts b/apps/web/src/lib/integrations/slack-service.ts index d15957509f..321748417b 100644 --- a/apps/web/src/lib/integrations/slack-service.ts +++ b/apps/web/src/lib/integrations/slack-service.ts @@ -7,7 +7,7 @@ import { TRPCError } from '@trpc/server'; import type { Owner } from '@/lib/integrations/core/types'; import { INTEGRATION_STATUS, PLATFORM } from '@/lib/integrations/core/constants'; import { getPlatformOAuthCallbackUrl } from '@/lib/integrations/oauth/urls'; -import { SLACK_CLIENT_ID } from '@kilocode/web-shared/lib/config.server'; +import { SLACK_CLIENT_ID, SLACK_CLIENT_SECRET } from '@kilocode/web-shared/lib/config.server'; import { WebClient } from '@slack/web-api'; import type { SlackInstallation } from '@chat-adapter/slack'; import { getDefaultAllowedModel } from '@/lib/slack-bot/model-allow-list'; @@ -88,6 +88,59 @@ export function getSlackOAuthUrl(state: string): string { return `https://slack.com/oauth/v2/authorize?${params.toString()}`; } +export type SlackOAuthExchangeResult = { + installationId: string; + enterpriseId: string | undefined; + isEnterpriseInstall: boolean; + installation: SlackInstallation; +}; + +/** + * Exchange a Slack OAuth V2 authorization code for a workspace installation. + * + * Produces the same installation record `slackAdapter.handleOAuthCallback` + * persists, but without writing it, so the callback can resolve which workspace + * is being connected and check its ownership first. Enterprise Grid org-wide + * installs are keyed on the enterprise ID, matching the adapter. + */ +export async function exchangeSlackOAuthCode(code: string): Promise { + if (!SLACK_CLIENT_ID || !SLACK_CLIENT_SECRET) { + throw new Error('SLACK_CLIENT_ID / SLACK_CLIENT_SECRET are not configured'); + } + + const result = await new WebClient().oauth.v2.access({ + client_id: SLACK_CLIENT_ID, + client_secret: SLACK_CLIENT_SECRET, + code, + redirect_uri: SLACK_REDIRECT_URI, + }); + + const isEnterpriseInstall = Boolean(result.is_enterprise_install); + const enterpriseId = result.enterprise?.id; + const installationId = isEnterpriseInstall ? enterpriseId : result.team?.id; + + if (!result.ok || !result.access_token || !installationId) { + throw new Error( + isEnterpriseInstall + ? 'Slack OAuth failed: missing access_token or enterprise id' + : 'Slack OAuth failed: missing access_token or team id' + ); + } + + return { + installationId, + enterpriseId, + isEnterpriseInstall, + installation: { + botToken: result.access_token, + botUserId: result.bot_user_id, + teamName: result.team?.name ?? result.enterprise?.name, + ...(enterpriseId ? { enterpriseId } : {}), + ...(isEnterpriseInstall ? { isEnterpriseInstall } : {}), + }, + }; +} + /** * Revoke Slack access token */ @@ -145,7 +198,13 @@ function isOwnedBy(integration: PlatformIntegration, owner: Owner): boolean { : integration.owned_by_organization_id === owner.id && integration.owned_by_user_id === null; } -async function getConflictingSlackInstallation( +/** + * Find a Slack `platform_integrations` row for `teamId` that belongs to a + * different Kilo owner. Callers use this before persisting a workspace + * installation so a workspace owned elsewhere is rejected without writing to + * the shared Chat SDK installation state. + */ +export async function getConflictingSlackInstallation( owner: Owner, teamId: string ): Promise { From 3560b6cb0aca4f01cbf5cb9563bf605b8cff71c4 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: Tue, 20 Oct 2026 02:34:23 +0000 Subject: [PATCH 2/6] fix(integrations): resolve the Linear workspace from the viewer relation Take organizationId and the workspace label from viewer { organization }, the same lookup the Chat SDK Linear adapter uses to key an installation, so the install path resolves the workspace without a separate root organization query. --- .../integrations/oauth-install-conflict.test.ts | 15 +++++++++------ apps/web/src/lib/integrations/linear-service.ts | 14 +++++++------- 2 files changed, 16 insertions(+), 13 deletions(-) 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 938c3ee1af..a47923bdf4 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 @@ -47,13 +47,16 @@ jest.mock('@sentry/nextjs', () => ({ jest.mock('@linear/sdk', () => ({ LinearClient: jest.fn(() => ({ get viewer() { - return Promise.resolve({ id: 'linear-bot-user', displayName: 'Kilo' }); - }, - get organization() { return Promise.resolve({ - id: mockLinearOrganizationId.current, - name: 'Victim Workspace', - urlKey: 'victim', + id: 'linear-bot-user', + displayName: 'Kilo', + get organization() { + return Promise.resolve({ + id: mockLinearOrganizationId.current, + name: 'Victim Workspace', + urlKey: 'victim', + }); + }, }); }, })), diff --git a/apps/web/src/lib/integrations/linear-service.ts b/apps/web/src/lib/integrations/linear-service.ts index 929fd700b9..85a6dc5c71 100644 --- a/apps/web/src/lib/integrations/linear-service.ts +++ b/apps/web/src/lib/integrations/linear-service.ts @@ -219,19 +219,19 @@ export type LinearOAuthIdentity = { }; /** - * Query `viewer` and `organization` with a freshly-issued user-actor - * access token. The returned `viewerId` is the Linear user id we will - * link to the Kilo user; `organizationId` is checked against the signed - * state so a multi-workspace user cannot link from a different workspace - * than the one the link token was issued for. + * Query `viewer` — and the workspace the viewer belongs to — with a + * freshly-issued Linear access token. The returned `viewerId` is the Linear + * user id for `actor=user` tokens and the app user id for `actor=app` install + * tokens; `organizationId` comes from `viewer { organization { id } }`, the + * same lookup the Chat SDK Linear adapter uses to key an installation. * * `LinearClient` wraps the GraphQL request / response handling and * throws `LinearGraphQLError` for both HTTP and GraphQL-level failures, * so errors propagate to the caller with a useful message. */ export async function fetchLinearOAuthIdentity(accessToken: string): Promise { - const linear = new LinearClient({ accessToken }); - const [viewer, organization] = await Promise.all([linear.viewer, linear.organization]); + const viewer = await new LinearClient({ accessToken }).viewer; + const organization = await viewer.organization; return { viewerId: viewer.id, From 87a6de36098d32f2fa20ea711a459bf66adf3567 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 03:59:41 +0000 Subject: [PATCH 3/6] docs(integrations): describe installation persistence coordination --- docs/security-architecture-overview.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/security-architecture-overview.md b/docs/security-architecture-overview.md index 4147e7cf0d..91def9a1be 100644 --- a/docs/security-architecture-overview.md +++ b/docs/security-architecture-overview.md @@ -234,6 +234,8 @@ Kilo Cloud distinguishes among several authorization contexts: Repository code includes token-version checks in shared JWT validation paths and revocation-pepper checks in selected web and Worker authorization paths. The exact lifetime, rotation cadence, and emergency-revocation process for production credentials are operational controls managed outside this overview. +Slack and Linear installation callbacks serialize persistence by provider, owner, and workspace using primary-PostgreSQL transaction advisory locks, acquired in owner-then-workspace order. Ownership reads and integration-row writes share that transaction; installation-state updates and restoration complete while the locks are held. Organization model defaults are resolved before entering the transaction. Slack credential mirroring uses a transaction savepoint and retains its best-effort behavior. + ### 5.3 Tenant and execution scoping Application data is commonly scoped to either a user or an organization. Cloud Agent durable state is session-scoped. KiloClaw runtimes are owner-scoped rather than shared global assistant processes. Some product areas are actively evolving from user-specific instances toward broader owner-scoped multi-instance support; this document describes the stable security boundary rather than transitional routing details. From 88d87460e73b1fec8bff7839caa0221d9abf37ae 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 04:05:55 +0000 Subject: [PATCH 4/6] test(integrations): exercise organization callback persistence --- .../oauth-install-conflict.test.ts | 49 ++++++++++++++++++- .../oauth/platforms/slack-callback.test.ts | 49 ++++++------------- 2 files changed, 64 insertions(+), 34 deletions(-) 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 c9780d2365..4fcc300098 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 @@ -4,7 +4,8 @@ import { setImmediate } from 'node:timers/promises'; import { and, eq, sql } from 'drizzle-orm'; import { db } from '@kilocode/web-shared/lib/drizzle'; import { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; -import { platform_integrations, type User } from '@kilocode/db/schema'; +import { createTestOrganization } from '@kilocode/web-shared/tests/helpers/organization.helper'; +import { organizations, platform_integrations, type User } from '@kilocode/db/schema'; import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; import { createOAuthState } from '@/lib/integrations/oauth-state'; import { INTEGRATION_STATUS, PLATFORM } from '@/lib/integrations/core/constants'; @@ -34,6 +35,9 @@ jest.mock('@kilocode/web-shared/lib/user/server'); jest.mock('@kilocode/web-shared/routers/organizations/utils', () => ({ ensureOrganizationAccess: jest.fn(), })); +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', @@ -328,6 +332,49 @@ describe('Slack install callback against the real integration row', () => { metadata: expect.objectContaining({ bot_user_id: 'U0BOT' }), }); }); + + test('completes an authorized organization workspace update', async () => { + const organization = await createTestOrganization('Callback Workspace', otherOwner.id, 0); + try { + const [existing] = await db + .insert(platform_integrations) + .values({ + owned_by_organization_id: organization.id, + platform: PLATFORM.SLACK, + integration_type: 'oauth', + platform_installation_id: 'T0PREVIOUS', + platform_account_id: 'T0PREVIOUS', + platform_account_login: 'Previous Workspace', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: { model_slug: 'test/selected-model' }, + }) + .returning(); + const state = createOAuthState( + `org_${organization.id}`, + otherOwner.id, + undefined, + 'provider_install' + ); + const response = await callCallback('slack', `?code=abc&state=${state}`); + + expect(redirectPath(response)).toBe( + `/organizations/${organization.id}/integrations/slack?success=installed` + ); + const [stored] = await db + .select() + .from(platform_integrations) + .where(eq(platform_integrations.id, existing.id)); + expect(stored).toMatchObject({ + owned_by_organization_id: organization.id, + platform_installation_id: 'T0TEAM', + integration_status: INTEGRATION_STATUS.ACTIVE, + metadata: expect.objectContaining({ model_slug: 'test/selected-model' }), + }); + expect(mockInstallations.get('T0TEAM')).toMatchObject({ botUserId: 'U0BOT' }); + } finally { + await db.delete(organizations).where(eq(organizations.id, organization.id)); + } + }); }); test.each(['slack', 'linear'] as const)( diff --git a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts index 4edd5eb0b3..646cdfd262 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts @@ -3,7 +3,7 @@ process.env.NEXTAUTH_SECRET ||= 'test-nextauth-secret'; const mockEnsureOrganizationAccess = jest.fn(); const mockConsumeProviderOAuthAttempt = jest.fn(); const mockUpsertSlackInstallation = jest.fn(); -const mockHandleOAuthCallback = jest.fn(); +const mockExchangeSlackOAuthCode = jest.fn(); const mockCaptureException = jest.fn(); const mockCaptureMessage = jest.fn(); @@ -22,10 +22,15 @@ jest.mock('@/lib/bot', () => ({ bot: { initialize: jest.fn(async () => {}), getAdapter: () => ({ - handleOAuthCallback: (...args: unknown[]) => mockHandleOAuthCallback(...args), + getInstallation: jest.fn(async () => null), + setInstallation: jest.fn(async () => {}), + deleteInstallation: jest.fn(async () => {}), }), }, })); +jest.mock('@/lib/slack-bot/model-allow-list', () => ({ + getDefaultAllowedModel: jest.fn(async () => 'test/default-model'), +})); jest.mock('@/lib/integrations/provider-oauth-attempts', () => ({ beginProviderOAuthAttempt: jest.fn(), cancelProviderOAuthAttempt: jest.fn(), @@ -40,6 +45,8 @@ jest.mock('@/lib/integrations/slack-service', () => { } return { SlackWorkspaceAlreadyConnectedError, + exchangeSlackOAuthCode: (...args: unknown[]) => mockExchangeSlackOAuthCode(...args), + getConflictingSlackInstallation: jest.fn(async () => null), upsertSlackInstallation: (...args: unknown[]) => mockUpsertSlackInstallation(...args), }; }); @@ -84,7 +91,7 @@ describe('handleSlackOAuthCallback organization authority', () => { mockEnsureOrganizationAccess.mockResolvedValue('owner'); mockConsumeProviderOAuthAttempt.mockResolvedValue(true); mockUpsertSlackInstallation.mockResolvedValue({ id: 'integration-1' }); - mockHandleOAuthCallback.mockResolvedValue({ teamId: 'T123', installation }); + mockExchangeSlackOAuthCode.mockResolvedValue({ installationId: 'T123', installation }); }); it('refuses an organization completion by a plain member', async () => { @@ -102,7 +109,7 @@ describe('handleSlackOAuthCallback organization authority', () => { ORGANIZATION_MANAGE_ROLES ); expect(mockUpsertSlackInstallation).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); + expect(mockExchangeSlackOAuthCode).not.toHaveBeenCalled(); expect(mockCaptureException).not.toHaveBeenCalled(); }); @@ -118,50 +125,26 @@ describe('handleSlackOAuthCallback organization authority', () => { 'permission_required' ); expect(mockUpsertSlackInstallation).not.toHaveBeenCalled(); - expect(mockHandleOAuthCallback).not.toHaveBeenCalled(); - }); - - it('completes an organization install for an admin and authorizes a workspace change', async () => { - mockEnsureOrganizationAccess.mockResolvedValue('admin'); - - const { response } = await callCallback(`org_${ORGANIZATION_ID}`); - - expect(response.status).toBe(307); - expect(new URL(response.headers.get('location') ?? '').pathname).toBe( - '/organizations/' + ORGANIZATION_ID + '/integrations/slack' - ); - expect(mockUpsertSlackInstallation).toHaveBeenCalledWith({ - owner: { type: 'org', id: ORGANIZATION_ID }, - teamId: 'T123', - installation, - allowWorkspaceRebind: true, - }); + expect(mockExchangeSlackOAuthCode).not.toHaveBeenCalled(); }); - it('keeps a same-workspace re-install working for an organization admin', async () => { + it('completes an organization installation', async () => { mockEnsureOrganizationAccess.mockResolvedValue('owner'); - mockHandleOAuthCallback.mockResolvedValue({ teamId: 'T123', installation }); const { response } = await callCallback(`org_${ORGANIZATION_ID}`); expect(new URL(response.headers.get('location') ?? '').searchParams.get('success')).toBe( 'installed' ); - expect(mockUpsertSlackInstallation).toHaveBeenCalledWith( - expect.objectContaining({ teamId: 'T123', allowWorkspaceRebind: true }) - ); }); it('does not require organization authority for a personal completion', async () => { const { response } = await callCallback(`user_${USER_ID}`); expect(new URL(response.headers.get('location') ?? '').pathname).toBe('/integrations/slack'); + expect(new URL(response.headers.get('location') ?? '').searchParams.get('success')).toBe( + 'installed' + ); expect(mockEnsureOrganizationAccess).not.toHaveBeenCalled(); - expect(mockUpsertSlackInstallation).toHaveBeenCalledWith({ - owner: { type: 'user', id: USER_ID }, - teamId: 'T123', - installation, - allowWorkspaceRebind: false, - }); }); }); From 4a7ef184ed2100e99b403d0a0b485dae2a581cbb 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 04:22:20 +0000 Subject: [PATCH 5/6] fix(integrations): bound installation lock acquisition --- .../linear/callback/route.test.ts | 50 +++++++++++++------ .../integrations/slack/callback/route.test.ts | 42 ++++++++++++---- .../oauth/workspace-installation-lock.ts | 2 + docs/security-architecture-overview.md | 2 +- 4 files changed, 71 insertions(+), 25 deletions(-) diff --git a/apps/web/src/app/api/integrations/linear/callback/route.test.ts b/apps/web/src/app/api/integrations/linear/callback/route.test.ts index 0b8b65903d..0f7a8bde76 100644 --- a/apps/web/src/app/api/integrations/linear/callback/route.test.ts +++ b/apps/web/src/app/api/integrations/linear/callback/route.test.ts @@ -1,5 +1,9 @@ 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 { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; import { getUserFromAuth } from '@kilocode/web-shared/lib/user/server'; import { verifyOAuthState, createOAuthState } from '@/lib/integrations/oauth-state'; import { bot } from '@/lib/bot'; @@ -289,22 +293,40 @@ describe('GET /api/integrations/linear/callback', () => { expect(mockCancelProviderOAuthAttempt).not.toHaveBeenCalled(); }); - test('persists the installation and the integration row for an unclaimed workspace', async () => { - mockedUpsertLinearInstallation.mockResolvedValue({} as never); - const state = createProviderState(); - const response = await callLinearCallback( - makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) + test('persists the installation and the integration row', async () => { + await insertTestUser({ id: USER_ID }); + mockedUpsertLinearInstallation.mockImplementation( + jest.requireActual<{ upsertLinearInstallation: typeof upsertLinearInstallation }>( + '@/lib/integrations/linear-service' + ).upsertLinearInstallation ); + try { + const state = createProviderState(); + const response = await callLinearCallback( + makeRequest(`/api/integrations/linear/callback?code=abc&state=${state}`) + ); - expect(setInstallation).toHaveBeenCalledTimes(1); - expect(setInstallation).toHaveBeenCalledWith(ORGANIZATION_ID, { - accessToken: 'lin_app_tok', - botUserId: 'linear-bot-user', - expiresAt: null, - organizationId: ORGANIZATION_ID, - }); - - expectRedirectLocation(response, '/integrations/linear?success=installed'); + expectRedirectLocation(response, '/integrations/linear?success=installed'); + expect(setInstallation).toHaveBeenCalledWith(ORGANIZATION_ID, { + accessToken: 'lin_app_tok', + botUserId: 'linear-bot-user', + expiresAt: null, + organizationId: ORGANIZATION_ID, + }); + const [stored] = await db + .select() + .from(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, USER_ID)); + expect(stored).toMatchObject({ + platform: PLATFORM.LINEAR, + platform_installation_id: ORGANIZATION_ID, + platform_account_login: 'Acme Workspace', + metadata: expect.objectContaining({ bot_enabled: true, bot_user_id: 'linear-bot-user' }), + }); + } finally { + mockedUpsertLinearInstallation.mockReset(); + await db.delete(kilocode_users).where(eq(kilocode_users.id, USER_ID)); + } }); test('redirects successful installs to returnTo when signed state carries one', async () => { 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 c93c9851e1..2f9f22eb7f 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 @@ -1,5 +1,9 @@ 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 { 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'; import { bot } from '@/lib/bot'; @@ -119,17 +123,35 @@ describe('GET /api/integrations/slack/callback', () => { expect(response.headers.get('location')).toContain('/users/sign_in'); }); - test('persists the exchanged installation for an unclaimed workspace', async () => { - mockedUpsertSlackInstallation.mockResolvedValue({} as never); - const state = createProviderState(); - - const response = await callSlackCallback( - makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + test('persists the installation and the integration row', async () => { + await insertTestUser({ id: USER_ID }); + mockedUpsertSlackInstallation.mockImplementation( + jest.requireActual<{ upsertSlackInstallation: typeof upsertSlackInstallation }>( + '@/lib/integrations/slack-service' + ).upsertSlackInstallation ); - - expect(setInstallation).toHaveBeenCalledTimes(1); - expect(setInstallation).toHaveBeenCalledWith(TEAM_ID, INSTALLATION); - expectRedirectLocation(response, '/integrations/slack?success=installed'); + try { + const state = createProviderState(); + const response = await callSlackCallback( + makeRequest(`/api/integrations/slack/callback?code=abc&state=${state}`) + ); + + expectRedirectLocation(response, '/integrations/slack?success=installed'); + expect(setInstallation).toHaveBeenCalledWith(TEAM_ID, INSTALLATION); + const [stored] = await db + .select() + .from(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, USER_ID)); + expect(stored).toMatchObject({ + platform: 'slack', + platform_installation_id: TEAM_ID, + platform_account_login: 'Acme Workspace', + metadata: expect.objectContaining({ bot_user_id: 'U0BOT', access_token: 'xoxb-new-token' }), + }); + } finally { + mockedUpsertSlackInstallation.mockReset(); + await db.delete(kilocode_users).where(eq(kilocode_users.id, USER_ID)); + } }); test('redirects successful installs to returnTo when signed state carries one', async () => { diff --git a/apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts b/apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts index 2e42e83a13..e3ef07954d 100644 --- a/apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts +++ b/apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts @@ -9,6 +9,8 @@ export async function withWorkspaceInstallationLock( operation: (tx: DrizzleTransaction) => Promise ): Promise { return db.transaction(async tx => { + await tx.execute(sql`SET LOCAL lock_timeout = '5s'`); + await tx.execute(sql`SET LOCAL statement_timeout = '30s'`); const ownerKey = `integration-installation-owner:${platform}:${owner.type}:${owner.id}`; await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${ownerKey}, 0))`); const key = `integration-installation:${platform}:${installationId}`; diff --git a/docs/security-architecture-overview.md b/docs/security-architecture-overview.md index 91def9a1be..26a4d86b18 100644 --- a/docs/security-architecture-overview.md +++ b/docs/security-architecture-overview.md @@ -234,7 +234,7 @@ Kilo Cloud distinguishes among several authorization contexts: Repository code includes token-version checks in shared JWT validation paths and revocation-pepper checks in selected web and Worker authorization paths. The exact lifetime, rotation cadence, and emergency-revocation process for production credentials are operational controls managed outside this overview. -Slack and Linear installation callbacks serialize persistence by provider, owner, and workspace using primary-PostgreSQL transaction advisory locks, acquired in owner-then-workspace order. Ownership reads and integration-row writes share that transaction; installation-state updates and restoration complete while the locks are held. Organization model defaults are resolved before entering the transaction. Slack credential mirroring uses a transaction savepoint and retains its best-effort behavior. +Slack and Linear installation callbacks serialize persistence by provider, owner, and workspace using primary-PostgreSQL transaction advisory locks, acquired in owner-then-workspace order. Each transaction sets a five-second lock timeout and a thirty-second SQL statement timeout. Ownership reads and integration-row writes share that transaction; installation-state updates and restoration complete while the locks are held. Organization model defaults are resolved before entering the transaction. Slack credential mirroring uses a transaction savepoint and retains its best-effort behavior. ### 5.3 Tenant and execution scoping From b69d978fa20731c740afc0edacc8a1d82c177241 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 21:12:17 +0000 Subject: [PATCH 6/6] fix(integrations): resolve the owner model before the install transaction The install callbacks now resolve the owner's model policy before taking the workspace locks, and the services treat an explicitly supplied null as a resolved value instead of re-querying the policy inside the transaction. --- .../lib/integrations/linear-service.test.ts | 14 ++++++++++++++ .../web/src/lib/integrations/linear-service.ts | 15 ++++++++++----- .../oauth/platforms/linear-callback.ts | 9 +++++++++ .../oauth/platforms/slack-callback.ts | 9 +++++++++ .../src/lib/integrations/slack-service.test.ts | 18 ++++++++++++++++++ apps/web/src/lib/integrations/slack-service.ts | 15 ++++++++++----- 6 files changed, 70 insertions(+), 10 deletions(-) diff --git a/apps/web/src/lib/integrations/linear-service.test.ts b/apps/web/src/lib/integrations/linear-service.test.ts index 722b7c4335..ea9f85d530 100644 --- a/apps/web/src/lib/integrations/linear-service.test.ts +++ b/apps/web/src/lib/integrations/linear-service.test.ts @@ -86,6 +86,20 @@ describe('upsertLinearInstallation', () => { ); }); + test('stores no model when the caller supplies null for the model', async () => { + const result = await upsertLinearInstallation( + { + owner: { type: 'user', id: user.id }, + organizationId: 'workspace-unsupplied-model', + organizationName: 'Unsupplied Model Workspace', + botUserId: 'bot-1', + }, + { defaultModel: null } + ); + + expect(result.metadata).not.toHaveProperty('model_slug'); + }); + test('uses the supplied transaction for installation ownership and updates', async () => { const rollback = new Error('Rollback test transaction'); await expect( diff --git a/apps/web/src/lib/integrations/linear-service.ts b/apps/web/src/lib/integrations/linear-service.ts index 24f687e56e..eb18fd53c8 100644 --- a/apps/web/src/lib/integrations/linear-service.ts +++ b/apps/web/src/lib/integrations/linear-service.ts @@ -324,7 +324,11 @@ export function getOwnerFromInstallation(integration: PlatformIntegration): Owne } type LinearUpsertOptions = { - defaultModel?: string; + /** + * Model resolved by the caller before the install transaction. `null` means the + * owner's policy allows no candidate model; `undefined` resolves it here. + */ + defaultModel?: string | null; getChatSdkAccessToken?: (organizationId: string) => Promise; deleteChatSdkInstallation?: (organizationId: string) => Promise; deleteChatSdkIdentityCache?: (organizationId: string) => Promise; @@ -386,10 +390,11 @@ export async function upsertLinearInstallation( // stores no model and the settings UI keeps its model placeholder. // For user integrations, use the shared bot default model. const defaultModel = - options.defaultModel ?? - (owner.type === 'org' - ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) - : DEFAULT_BOT_MODEL); + options.defaultModel !== undefined + ? options.defaultModel + : owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; const existingMetadata = existing?.metadata && typeof existing.metadata === 'object' ? existing.metadata : {}; diff --git a/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts b/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts index 489600ecb4..8d068e66d4 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts @@ -36,6 +36,8 @@ import { } from '@/lib/integrations/oauth/common'; import { consumeProviderOAuthAttempt } from '@/lib/integrations/provider-oauth-attempts'; import { withWorkspaceInstallationLock } from '@/lib/integrations/oauth/workspace-installation-lock'; +import { getDefaultAllowedModel } from '@/lib/slack-bot/model-allow-list'; +import { DEFAULT_BOT_MODEL } from '@/lib/bot/constants'; async function getChatSdkLinearAccessToken(organizationId: string): Promise { const installation = await bot.getAdapter('linear').getInstallation(organizationId); @@ -313,6 +315,12 @@ export async function handleLinearOAuthCallback(request: NextRequest) { const { accessToken, refreshToken, expiresIn } = await exchangeLinearOAuthCode(code); const identity = await fetchLinearOAuthIdentity(accessToken); const organizationId = identity.organizationId; + // Resolve the owner's model policy before taking the workspace locks, so the + // transaction only performs the ownership check and the integration write. + const defaultModel = + owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; const conflictRedirect = NextResponse.redirect( new URL( @@ -356,6 +364,7 @@ export async function handleLinearOAuthCallback(request: NextRequest) { getChatSdkAccessToken: getChatSdkLinearAccessToken, deleteChatSdkInstallation: deleteChatSdkLinearInstallation, deleteChatSdkIdentityCache: deleteChatSdkLinearIdentityCache, + defaultModel, }, tx ); diff --git a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts index 7d9e3f6014..9601104ba3 100644 --- a/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts +++ b/apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts @@ -25,6 +25,8 @@ import { } from '@/lib/integrations/oauth/common'; import { consumeProviderOAuthAttempt } from '@/lib/integrations/provider-oauth-attempts'; import { withWorkspaceInstallationLock } from '@/lib/integrations/oauth/workspace-installation-lock'; +import { getDefaultAllowedModel } from '@/lib/slack-bot/model-allow-list'; +import { DEFAULT_BOT_MODEL } from '@/lib/bot/constants'; /** * Slack OAuth Callback @@ -149,6 +151,12 @@ export async function handleSlackOAuthCallback(request: NextRequest) { const slackAdapter = bot.getAdapter('slack'); const { installationId, installation } = await exchangeSlackOAuthCode(code); + // Resolve the owner's model policy before taking the workspace locks, so the + // transaction only performs the ownership check and the integration write. + const defaultModel = + owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; const conflictRedirect = NextResponse.redirect( new URL( @@ -177,6 +185,7 @@ export async function handleSlackOAuthCallback(request: NextRequest) { teamId: installationId, installation, allowWorkspaceRebind: owner.type === 'org', + defaultModel, }, tx ); diff --git a/apps/web/src/lib/integrations/slack-service.test.ts b/apps/web/src/lib/integrations/slack-service.test.ts index 132b1e72eb..7817849a63 100644 --- a/apps/web/src/lib/integrations/slack-service.test.ts +++ b/apps/web/src/lib/integrations/slack-service.test.ts @@ -537,6 +537,24 @@ describe('upsertSlackInstallation organization workspace binding', () => { ); }); + it('stores no model when the supplied model is null', async () => { + mockLimit.mockResolvedValueOnce([organizationIntegration('T123')]).mockResolvedValueOnce([]); + + await upsertSlackInstallation({ + owner: organizationOwner, + teamId: 'T123', + installation, + defaultModel: null, + }); + + expect(mockGetDefaultAllowedModel).not.toHaveBeenCalled(); + expect(mockUpdateSet).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.not.objectContaining({ model_slug: expect.anything() }), + }) + ); + }); + it('refuses to move an organization connection without caller authorization', async () => { mockLimit.mockResolvedValueOnce([organizationIntegration('T_OLD')]).mockResolvedValueOnce([]); diff --git a/apps/web/src/lib/integrations/slack-service.ts b/apps/web/src/lib/integrations/slack-service.ts index 85c49f8886..475ef3a716 100644 --- a/apps/web/src/lib/integrations/slack-service.ts +++ b/apps/web/src/lib/integrations/slack-service.ts @@ -325,7 +325,11 @@ export async function upsertSlackInstallation( teamId: string; installation: SlackInstallation; allowWorkspaceRebind?: boolean; - defaultModel?: string; + /** + * Model resolved by the caller before the install transaction. `null` means + * the owner's policy allows no candidate model; `undefined` resolves it here. + */ + defaultModel?: string | null; }, database: typeof db | DrizzleTransaction = db ): Promise { @@ -354,10 +358,11 @@ export async function upsertSlackInstallation( // stores no `model_slug` and the UI keeps its "Select a model" state. // For user integrations, use the shared bot default model. const defaultModel = - suppliedDefaultModel ?? - (owner.type === 'org' - ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) - : DEFAULT_BOT_MODEL); + suppliedDefaultModel !== undefined + ? suppliedDefaultModel + : owner.type === 'org' + ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) + : DEFAULT_BOT_MODEL; const existingModelSlug = existing?.metadata &&