diff --git a/extensions/general/skatteverket/__tests__/oauth-callback.test.ts b/extensions/general/skatteverket/__tests__/oauth-callback.test.ts index 6c073380..19479778 100644 --- a/extensions/general/skatteverket/__tests__/oauth-callback.test.ts +++ b/extensions/general/skatteverket/__tests__/oauth-callback.test.ts @@ -55,7 +55,10 @@ const STATE = 'state-1' * whichever key the chain last filtered on, and the post-exchange cleanup * delete resolves via .in(). */ -function makeServiceSupabase(overrides: Record = {}) { +function makeServiceSupabase( + overrides: Record = {}, + options: { isMember?: boolean } = {}, +) { const values: Record = { oauth_state: STATE, oauth_user_id: 'user-1', @@ -64,8 +67,10 @@ function makeServiceSupabase(overrides: Record = {}) { oauth_return_to: '/settings/tax', ...overrides, } + const isMember = options.isMember ?? true const gte = vi.fn() - const from = vi.fn(() => { + const inCalls: string[][] = [] + const from = vi.fn((table: string) => { let key: string | null = null const chain: any = { select: vi.fn(() => chain), @@ -75,10 +80,18 @@ function makeServiceSupabase(overrides: Record = {}) { return chain }), gte: gte.mockImplementation(() => chain), - in: vi.fn(() => Promise.resolve({ error: null })), - maybeSingle: vi.fn(async () => ({ - data: key !== null && values[key] != null ? { value: values[key] } : null, - })), + in: vi.fn((_col: string, keys: string[]) => { + inCalls.push(keys) + return Promise.resolve({ error: null }) + }), + maybeSingle: vi.fn(async () => { + if (table === 'company_members') { + return { data: isMember ? { user_id: values.oauth_user_id ?? 'user-1' } : null } + } + return { + data: key !== null && values[key] != null ? { value: values[key] } : null, + } + }), then: (resolve: any, reject: any) => { const rows = values.oauth_state != null @@ -89,7 +102,7 @@ function makeServiceSupabase(overrides: Record = {}) { } return chain }) - return { from, gte } + return { from, gte, inCalls } } /** Cookie-bound client: only consulted by the legacy session fallback. */ @@ -250,4 +263,38 @@ describe('skatteverket OAuth callback', () => { expect(html).toContain('skatteverket-oauth-error') expect(mockRefresh).not.toHaveBeenCalled() }) + + it('cleans up the ephemeral state rows (incl. oauth_user_id) when the exchange fails', async () => { + const service = makeServiceSupabase() + mockCreateServiceClient.mockReturnValue(service as any) + mockExchange.mockRejectedValueOnce(new Error('exchange boom')) + + await callbackRoute().handler(callbackRequest(`code=abc&state=${STATE}`)) + + // The failure path must delete the same ephemeral keys the success + // path does: oauth_user_id holds a user identity and must not be + // retained past the flow. (#1090) + expect(service.inCalls).toHaveLength(1) + expect(service.inCalls[0]).toEqual( + expect.arrayContaining(['oauth_state', 'oauth_user_id', 'oauth_code_verifier']), + ) + }) + + it('rejects the flow when the stored user is no longer a member of the company', async () => { + mockCreateServiceClient.mockReturnValue( + makeServiceSupabase({}, { isMember: false }) as any, + ) + + const response = await callbackRoute().handler( + callbackRequest(`code=abc&state=${STATE}`), + ) + + expect(response.status).toBe(200) + const html = await response.text() + expect(html).toContain('skatteverket-oauth-error') + // Rejected before the exchange so the one-shot code is not burned, + // and no token write is attempted. (#1091) + expect(mockExchange).not.toHaveBeenCalled() + expect(mockStoreTokens).not.toHaveBeenCalled() + }) }) diff --git a/extensions/general/skatteverket/index.ts b/extensions/general/skatteverket/index.ts index 760e36a8..2fa7b03e 100644 --- a/extensions/general/skatteverket/index.ts +++ b/extensions/general/skatteverket/index.ts @@ -423,6 +423,25 @@ export const skatteverketExtension: Extension = { ) } + // Defense in depth for the service-role write below: the stored + // user must still be a member of the company that initiated the + // flow (membership can be revoked between /authorize and this + // callback, and RLS no longer backstops the write). Checked before + // the exchange so a rejected flow does not burn the one-shot + // authorization code. (#1091) + const { data: membership } = await db + .from('company_members') + .select('user_id') + .eq('company_id', companyId) + .eq('user_id', userId) + .maybeSingle() + if (!membership) { + return respondWithError( + 'Behörighet saknas för företaget', + `/reports?tab=vat-declaration&skv_error=${encodeURIComponent('Behörighet saknas för företaget')}`, + ) + } + const redirectUri = (await readSetting('oauth_redirect_uri')) || `${getSkvOauthBaseUrl()}/api/extensions/ext/skatteverket/callback` @@ -481,6 +500,20 @@ export const skatteverketExtension: Extension = { return respondWithSuccess(successPath) } catch (err) { console.error('[skatteverket] Token exchange failed:', err) + // The ephemeral flow rows must not outlive the flow: oauth_user_id + // in particular holds a user identity and serves no purpose once + // the exchange has failed (#1090). Best-effort: a cleanup failure + // must not mask the exchange error shown to the user. + try { + await db + .from('extension_data') + .delete() + .eq('company_id', companyId) + .eq('extension_id', 'skatteverket') + .in('key', ['oauth_state', 'oauth_user_id', 'oauth_return_to', 'oauth_code_verifier']) + } catch (cleanupErr) { + log.error('oauth state cleanup after failed exchange failed', cleanupErr, { companyId }) + } // BankID auth codes expire after 5 minutes. Surface timeouts distinctly // so the user retries quickly instead of exhausting the code window. const message = err instanceof TimeoutError diff --git a/lib/domains/__tests__/legacy-redirect.test.ts b/lib/domains/__tests__/legacy-redirect.test.ts new file mode 100644 index 00000000..cbe30477 --- /dev/null +++ b/lib/domains/__tests__/legacy-redirect.test.ts @@ -0,0 +1,30 @@ +import { describe, it, expect } from 'vitest' +import { isRedirectedFromLegacyHost } from '../legacy-redirect' + +describe('legacy-host redirect exclusions', () => { + it('forwards ordinary pages to the new domain', () => { + expect(isRedirectedFromLegacyHost('/')).toBe(true) + expect(isRedirectedFromLegacyHost('/dashboard')).toBe(true) + expect(isRedirectedFromLegacyHost('/reports')).toBe(true) + // login and MFA pages MUST redirect: a usable login page on the + // legacy host would establish sessions there and loop users between + // domains. + expect(isRedirectedFromLegacyHost('/login')).toBe(true) + expect(isRedirectedFromLegacyHost('/mfa')).toBe(true) + }) + + it('keeps machine surfaces on the legacy host', () => { + expect(isRedirectedFromLegacyHost('/api/v1/companies')).toBe(false) + expect(isRedirectedFromLegacyHost('/api/extensions/ext/skatteverket/callback')).toBe(false) + expect(isRedirectedFromLegacyHost('/.well-known/oauth-authorization-server')).toBe(false) + expect(isRedirectedFromLegacyHost('/_next/static/chunk.js')).toBe(false) + }) + + it('keeps PKCE-cookie-bound auth flows on the legacy host (#1092)', () => { + // Password reset and signup confirmation links sent before the + // cutover carry a PKCE code whose verifier cookie lives on the + // legacy host; forwarding them would break the exchange. + expect(isRedirectedFromLegacyHost('/auth/callback')).toBe(false) + expect(isRedirectedFromLegacyHost('/reset-password')).toBe(false) + }) +}) diff --git a/lib/domains/legacy-redirect.ts b/lib/domains/legacy-redirect.ts new file mode 100644 index 00000000..f19a068b --- /dev/null +++ b/lib/domains/legacy-redirect.ts @@ -0,0 +1,25 @@ +/** + * Path exclusions for the legacy-host (app.gnubok.se) page redirect in + * next.config.ts. + * + * Machine surfaces (api/, .well-known/), assets (_next/) and the + * PKCE-cookie-bound auth flows (auth/, reset-password) must keep answering + * on the legacy host after the app.accounted.se cutover; everything else + * forwards to the new domain. auth/ and reset-password stay because email + * links sent before the cutover carry a PKCE code or recovery session + * whose cookies live on the legacy host (#1092); the login and MFA pages + * are deliberately NOT excluded, since a usable login page on the legacy + * host would establish sessions there and bounce users in a redirect loop. + */ +export const LEGACY_HOST_REDIRECT_EXCLUSIONS = + '(?!api/|\\.well-known/|_next/|auth/|reset-password)' + +/** + * Mirror of how the path-to-regexp source `/:path((?!...).*)` decides + * whether a legacy-host request is forwarded. Used by tests to pin the + * exclusion behavior without booting Next's router. + */ +export function isRedirectedFromLegacyHost(pathname: string): boolean { + const relative = pathname.replace(/^\//, '') + return new RegExp(`^${LEGACY_HOST_REDIRECT_EXCLUSIONS}.*$`).test(relative) +} diff --git a/next.config.ts b/next.config.ts index db17959c..28c796c4 100644 --- a/next.config.ts +++ b/next.config.ts @@ -2,6 +2,7 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import type { NextConfig } from "next"; import createNextIntlPlugin from "next-intl/plugin"; +import { LEGACY_HOST_REDIRECT_EXCLUSIONS } from "./lib/domains/legacy-redirect"; const withNextIntl = createNextIntlPlugin("./i18n/request.ts"); @@ -104,12 +105,20 @@ const nextConfig: NextConfig = { // points somewhere other than the legacy host, so merging this is // inert and the actual cutover is the env flip + redeploy. Kept // non-permanent until the cutover has soaked. + // + // auth/ and reset-password are excluded so email links that carry a + // PKCE code (password reset, signup confirmation) sent before the + // cutover still complete on the legacy host, where their code + // verifier / recovery-session cookies live (#1092). login and MFA + // pages are deliberately NOT excluded: serving a usable login page + // on the legacy host would establish sessions there and bounce + // users in a redirect loop. ...(appUrlForRedirect && appUrlForRedirect.startsWith('https://') && !appUrlForRedirect.includes('app.gnubok.se') ? [ { - source: '/:path((?!api/|\\.well-known/|_next/).*)', + source: `/:path(${LEGACY_HOST_REDIRECT_EXCLUSIONS}.*)`, has: [{ type: 'host' as const, value: 'app.gnubok.se' }], destination: `${appUrlForRedirect}/:path`, permanent: false,