From b72ae40beb5e6070cc8ff72f386767f6204e83a0 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Tue, 7 Jul 2026 17:11:53 -0400 Subject: [PATCH] fix(integrations): don't persist unverified GitHub installation_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub App install callback returns an `installation_id`, which GitHub's own docs document as not trustworthy (it can be spoofed on the callback URL). The previous code stored it on the connection metadata, but nothing reads it anywhere, so it was unused data that a future installation-token flow could later trust unsafely. Remove the storage (and its now-unused query fields + test). If GitHub App installation-token auth is added later, resolve the installation via `GET /user/installations` with the user token — which also proves the user owns it — instead of trusting the callback value. No behavior change: the integration authenticates with the OAuth user token, which is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XyttkYNbkdYRp7DBKNuFVH --- .../controllers/oauth.controller.spec.ts | 88 ------------------- .../controllers/oauth.controller.ts | 31 ++----- 2 files changed, 7 insertions(+), 112 deletions(-) diff --git a/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts b/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts index 619cb13dc..cf3fdefc3 100644 --- a/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts +++ b/apps/api/src/integration-platform/controllers/oauth.controller.spec.ts @@ -692,94 +692,6 @@ describe('OAuthController', () => { fetchSpy.mockRestore(); }); - it('persists installation_id on the connection for the GitHub App install flow', async () => { - const futureDate = new Date(Date.now() + 600000); - mockOAuthStateRepository.findByState.mockResolvedValue({ - state: 'valid_gh_state', - providerSlug: 'github-app', - organizationId: 'org_1', - userId: 'user_1', - codeVerifier: null, - redirectUrl: null, - expiresAt: futureDate, - }); - mockedGetManifest.mockReturnValue({ - id: 'github-app', - name: 'GitHub App', - category: 'Development', - auth: { - type: 'oauth2', - config: { - authorizeUrl: - 'https://github.com/apps/{APP_SLUG}/installations/new', - tokenUrl: 'https://github.com/login/oauth/access_token', - appInstallFlow: true, - }, - }, - capabilities: [], - isActive: true, - } as never); - mockOAuthCredentialsService.getCredentials.mockResolvedValue({ - clientId: 'client_123', - clientSecret: 'secret_456', - scopes: [], - source: 'platform', - }); - mockProviderRepository.findBySlug.mockResolvedValue({ - id: 'provider_gh', - }); - mockConnectionRepository.findByProviderAndOrg.mockResolvedValue({ - id: 'conn_gh', - metadata: {}, - variables: {}, - lastSyncAt: null, - }); - mockConnectionRepository.update.mockResolvedValue({ - id: 'conn_gh', - metadata: {}, - variables: {}, - lastSyncAt: null, - }); - mockConnectionService.activateConnection.mockResolvedValue({ - id: 'conn_gh', - }); - - const fetchSpy = jest.spyOn(global, 'fetch').mockResolvedValue({ - ok: true, - status: 200, - json: () => Promise.resolve({ access_token: 'gh_user_token' }), - text: () => Promise.resolve(''), - } as unknown as Response); - - await controller.oauthCallback( - { - code: 'auth_code', - state: 'valid_gh_state', - installation_id: '987654', - setup_action: 'install', - }, - mockRequest, - mockResponse, - ); - - await new Promise((resolve) => setImmediate(resolve)); - - expect(mockConnectionRepository.update).toHaveBeenCalledWith( - 'conn_gh', - expect.objectContaining({ - metadata: expect.objectContaining({ - githubInstallationId: '987654', - githubSetupAction: 'install', - }), - }), - ); - const redirectUrl = (mockResponse.redirect as jest.Mock).mock.calls[0][0]; - expect(redirectUrl).toContain('success=true'); - expect(redirectUrl).toContain('provider=github-app'); - - fetchSpy.mockRestore(); - }); - describe('session defense-in-depth', () => { const futureDate = new Date(Date.now() + 600000); const validState = { diff --git a/apps/api/src/integration-platform/controllers/oauth.controller.ts b/apps/api/src/integration-platform/controllers/oauth.controller.ts index 4f3436dac..8007d2249 100644 --- a/apps/api/src/integration-platform/controllers/oauth.controller.ts +++ b/apps/api/src/integration-platform/controllers/oauth.controller.ts @@ -39,9 +39,6 @@ interface OAuthCallbackQuery { state: string; error?: string; error_description?: string; - // GitHub App installation flow returns these alongside `code`. - installation_id?: string; - setup_action?: string; } @Controller({ path: 'integrations/oauth', version: '1' }) @@ -425,27 +422,13 @@ export class OAuthController { }); } - // GitHub App installation flow: persist the installation id (and setup - // action) on the connection. The token stored above is a user-to-server - // token, but recording the installation id means a future server-to-server - // (installation access token) upgrade needs no re-connect. - if (query.installation_id) { - const metadata = - connection.metadata && - typeof connection.metadata === 'object' && - !Array.isArray(connection.metadata) - ? (connection.metadata as Record) - : {}; - connection = await this.connectionRepository.update(connection.id, { - metadata: { - ...metadata, - githubInstallationId: query.installation_id, - ...(query.setup_action - ? { githubSetupAction: query.setup_action } - : {}), - }, - }); - } + // NOTE: GitHub's App install flow returns an `installation_id` on this + // callback, but that value is attacker-spoofable — GitHub's own docs say + // not to rely on it — so we intentionally do NOT persist it here, and + // nothing reads it today. If/when GitHub App installation-token auth is + // added, resolve the installation via `GET /user/installations` with the + // user token (which also proves the user owns it) rather than trusting the + // value from this callback. await this.connectionService.activateConnection(connection.id);