fix(domains,skatteverket): #1087 follow-ups: auth-path redirect exclusions, SKV callback hardening (#1094)

- Exclude auth/ and reset-password from the legacy-host redirect (#1092):
  email links sent before the cutover carry a PKCE code or recovery
  session whose cookies live on app.gnubok.se; forwarding them to the
  new domain breaks password resets and signup confirmations clicked
  after the flip. login/MFA stay redirected on purpose: a usable login
  page on the legacy host would establish sessions there and loop.
  Exclusion pattern extracted to lib/domains/legacy-redirect.ts with a
  unit test pinning the behavior.
- Clean up ephemeral oauth state rows (incl. oauth_user_id) when the
  SKV token exchange fails (#1090): identity data must not outlive the
  flow; best-effort so cleanup failure never masks the user-facing error.
- Assert the stored user is still a member of the company before the
  service-role storeTokens write (#1091): membership can be revoked
  between /authorize and the callback, and RLS no longer backstops the
  write. Checked before the exchange so the one-shot code is not burned.

Fixes #1090, fixes #1091, fixes #1092.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

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<string, string | null> = {}) {
+function makeServiceSupabase(
+  overrides: Record<string, string | null> = {},
+  options: { isMember?: boolean } = {},
+) {
   const values: Record<string, string | null> = {
     oauth_state: STATE,
     oauth_user_id: 'user-1',
@@ -64,8 +67,10 @@ function makeServiceSupabase(overrides: Record<string, string | null> = {}) {
     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<string, string | null> = {}) {
         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<string, string | null> = {}) {
     }
     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()
+  })
 })

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

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)
+  })
+})

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)
+}

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,