diff --git a/instrumentation.ts b/instrumentation.ts index c6cc6494..80d71bca 100644 --- a/instrumentation.ts +++ b/instrumentation.ts @@ -1,4 +1,8 @@ import type { Instrumentation } from 'next' +// Imported by exact path rather than through the `@/lib/observability` barrel: +// this file's module graph is loaded in every runtime Next boots, and the +// predicate is a dependency-free pure function. +import { isClientDisconnectError } from '@/lib/observability/is-client-disconnect' export async function register() { // Instrumentation hook: currently a no-op. @@ -23,8 +27,17 @@ export async function register() { * instrumentation-client.ts), which is what links a server error back to the * user's session replay. Absent that header the error is still captured, just * unattributed. + * + * A client that navigated away mid-stream is filtered out first: see + * `isClientDisconnectError`. Next reports those through this hook even though + * the response itself succeeded, and they are not exceptions. Next still + * writes its own stderr line for them, so they stay visible in Vercel's + * runtime-error table; what this drops is the false entry in Error Tracking + * and the awaited flush it would cost a healthy request. */ export const onRequestError: Instrumentation.onRequestError = async (err, request) => { + if (isClientDisconnectError(err)) return + try { const { getPostHogServer, flushAnalytics } = await import('@/lib/analytics/posthog-server') const posthog = getPostHogServer() diff --git a/lib/company/__tests__/other-account-hint.test.ts b/lib/company/__tests__/other-account-hint.test.ts index 23c9ed81..113dbd0a 100644 --- a/lib/company/__tests__/other-account-hint.test.ts +++ b/lib/company/__tests__/other-account-hint.test.ts @@ -10,20 +10,39 @@ import type { SupabaseClient } from '@supabase/supabase-js' const mockCreateServiceClient = vi.mocked(createServiceClient) +interface QueryResult { + data?: unknown + error?: unknown +} + /** - * Chainable query mock keyed by table name: every method returns the chain, - * awaiting it resolves with the configured { data, error } for that table. + * Chainable query mock keyed by table name: every method returns the chain and + * records its arguments, and awaiting it resolves with the configured + * { data, error } for that table. A table may be given an ARRAY of results, + * consumed one per from() call with the last one repeating, so the chunked + * existence probe can be answered chunk by chunk. */ -function buildClient(resultsByTable: Record) { +function buildClient(resultsByTable: Record) { + const queues = new Map( + Object.entries(resultsByTable).map(([table, value]) => [ + table, + Array.isArray(value) ? [...value] : [value], + ]), + ) + const calls: Array<{ table: string; method: string; args: unknown[] }> = [] + return { + calls, from: vi.fn((table: string) => { - const result = { - data: resultsByTable[table]?.data ?? null, - error: resultsByTable[table]?.error ?? null, - } + const queue = queues.get(table) ?? [] + const configured = (queue.length > 1 ? queue.shift() : queue[0]) ?? {} + const result = { data: configured.data ?? null, error: configured.error ?? null } const chain: Record = {} for (const m of ['select', 'eq', 'in', 'is', 'limit', 'order', 'range']) { - chain[m] = () => chain + chain[m] = (...args: unknown[]) => { + calls.push({ table, method: m, args }) + return chain + } } ;(chain as { then?: unknown }).then = (resolve: (v: unknown) => void) => resolve(result) return chain @@ -31,6 +50,11 @@ function buildClient(resultsByTable: Record) { + return client.calls.filter((c) => c.table === 'journal_entries' && c.method === 'in') +} + const OWN_COMPANY = { id: 'own-co', org_number: '5560125790' } beforeEach(() => { @@ -50,6 +74,36 @@ describe('shouldShowOtherAccountHint', () => { expect(mockCreateServiceClient).not.toHaveBeenCalled() }) + it('scopes the own-account probe to the caller\'s companies', async () => { + // Without company_id the probe falls back on the RLS qual, which under the + // authenticated role turns into a sequential scan of journal_entries on + // the blocking Hem render. + const supabase = buildClient({ + companies: { data: [OWN_COMPANY] }, + journal_entries: { data: [{ id: 'je1' }] }, + }) + + await expect(shouldShowOtherAccountHint(supabase as unknown as SupabaseClient)).resolves.toBe(false) + expect(probeFilters(supabase).map((c) => c.args)).toEqual([['company_id', ['own-co']]]) + }) + + it('chunks the probe and stops at the first company with entries', async () => { + const many = Array.from({ length: 250 }, (_, i) => ({ + id: `co-${i}`, + org_number: '5560125790', + })) + const supabase = buildClient({ + companies: { data: many }, + // First chunk finds nothing, second one does: the third must never run. + journal_entries: [{ data: [] }, { data: [{ id: 'je1' }] }, { data: [] }], + }) + + await expect(shouldShowOtherAccountHint(supabase as unknown as SupabaseClient)).resolves.toBe(false) + const chunks = probeFilters(supabase).map((c) => (c.args[1] as string[]).length) + expect(chunks).toEqual([100, 100]) + expect(mockCreateServiceClient).not.toHaveBeenCalled() + }) + it('is true when the account is empty and a same-orgnr company elsewhere has entries', async () => { const supabase = buildClient({ companies: { data: [OWN_COMPANY] }, diff --git a/lib/company/other-account-hint.ts b/lib/company/other-account-hint.ts index 77072c49..eecec1e6 100644 --- a/lib/company/other-account-hint.ts +++ b/lib/company/other-account-hint.ts @@ -2,6 +2,38 @@ import type { SupabaseClient } from '@supabase/supabase-js' import { createServiceClient } from '@/lib/supabase/server' import { fetchAllRows } from '@/lib/supabase/fetch-all' +/** + * Ids per existence probe. They travel in the PostgREST query string as + * `company_id=in.(uuid,uuid,...)`, so an unbounded list would eventually run + * past the gateway's request-header ceiling and be rejected outright. 100 + * UUIDs is roughly 3.7 KB, comfortably inside it. + */ +const PROBE_CHUNK_SIZE = 100 + +/** + * Does any of these companies have at least one journal entry? + * + * The company filter is what makes this an index probe: naming the ids lets + * Postgres use idx_journal_entries_company_id. Stops at the first chunk that + * finds a row. Throws on a query error, which the caller turns into "no hint". + */ +async function hasAnyJournalEntry( + client: SupabaseClient, + companyIds: string[], +): Promise { + for (let i = 0; i < companyIds.length; i += PROBE_CHUNK_SIZE) { + const { data, error } = await client + .from('journal_entries') + .select('id') + .in('company_id', companyIds.slice(i, i + PROBE_CHUNK_SIZE)) + .limit(1) + + if (error) throw new Error(error.message) + if ((data ?? []).length > 0) return true + } + return false +} + /** * Should the Hem page hint that the user may be signed in to the wrong * account? (#1231, the "Chillen" support case: BankID resolved to a stale @@ -15,35 +47,41 @@ import { fetchAllRows } from '@/lib/supabase/fetch-all' * NOT a member of, has at least one journal entry. * * The common case (an account with any bookkeeping at all) exits after one - * indexed existence probe. The cross-account probe runs on the service + * company-scoped existence probe. The cross-account probe runs on the service * client but the result reduces to one boolean: nothing about the other * account is revealed beyond "your bookkeeping may live elsewhere". * Fails soft to false: this is an advisory line, never worth an error. */ export async function shouldShowOtherAccountHint(supabase: SupabaseClient): Promise { try { - // RLS scopes both reads to the caller's memberships. Company lists are - // paginated with fetchAllRows (PostgREST caps at 1000 rows; a byrå user - // can belong to many companies); it throws on error, which the outer - // catch turns into false. The journal_entries read stays a bare - // limit(1): it is an existence probe, not a listing. - const [ownCompanies, { data: ownEntries, error: entriesError }] = await Promise.all([ - fetchAllRows<{ id: string; org_number: string | null }>(({ from, to }) => + // RLS scopes every read here to the caller's memberships. Company lists + // are paginated with fetchAllRows (PostgREST caps at 1000 rows; a byrå + // user can belong to many companies); it throws on error, which the outer + // catch turns into false. + const ownCompanies = await fetchAllRows<{ id: string; org_number: string | null }>( + ({ from, to }) => supabase .from('companies') .select('id, org_number') .is('archived_at', null) .order('id') .range(from, to), - ), - supabase.from('journal_entries').select('id').limit(1), - ]) + ) - if (entriesError) return false if (ownCompanies.length === 0) return false - if ((ownEntries ?? []).length > 0) return false - const ownIds = new Set(ownCompanies.map((c) => c.id)) + // The probe needs the company list, so it runs after it rather than beside + // it. Leaving company_id out and letting RLS do the scoping reads as the + // cheaper form and is the opposite: under the `authenticated` role the RLS + // qual becomes a filter on the scan, so Postgres reads all of + // journal_entries (~900 ms measured on prod, 4.2 s worst observed) on the + // blocking Hem render instead of using idx_journal_entries_company_id + // (~2 ms). Naming the ids cannot change the answer: user_company_ids(), + // which journal_entries RLS uses, excludes archived companies, and so does + // the list above. + const ownIds = ownCompanies.map((c) => c.id) + if (await hasAnyJournalEntry(supabase, ownIds)) return false + const orgNumbers = [ ...new Set(ownCompanies.map((c) => c.org_number).filter((n): n is string => Boolean(n))), ] @@ -60,17 +98,11 @@ export async function shouldShowOtherAccountHint(supabase: SupabaseClient): Prom .range(from, to), ) - const otherIds = sameOrgCompanies.map((c) => c.id).filter((id) => !ownIds.has(id)) + const ownIdSet = new Set(ownIds) + const otherIds = sameOrgCompanies.map((c) => c.id).filter((id) => !ownIdSet.has(id)) if (otherIds.length === 0) return false - const { data: otherEntries, error: otherEntriesError } = await service - .from('journal_entries') - .select('id') - .in('company_id', otherIds) - .limit(1) - - if (otherEntriesError) return false - return (otherEntries ?? []).length > 0 + return await hasAnyJournalEntry(service, otherIds) } catch { // Service key unavailable (some self-hosted setups) or transient failure. return false diff --git a/lib/observability/__tests__/is-client-disconnect.test.ts b/lib/observability/__tests__/is-client-disconnect.test.ts new file mode 100644 index 00000000..71d5883a --- /dev/null +++ b/lib/observability/__tests__/is-client-disconnect.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect } from 'vitest' +import { isClientDisconnectError } from '../is-client-disconnect' + +describe('isClientDisconnectError', () => { + it('matches React\'s cancel error for a stream the client closed', () => { + expect(isClientDisconnectError(new Error('The destination stream closed early.'))).toBe(true) + }) + + it('matches the sibling cancel error React raises on a write failure', () => { + expect( + isClientDisconnectError(new Error('The destination stream errored while writing data.')), + ).toBe(true) + }) + + it('matches Next\'s own ResponseAborted error by name', () => { + const aborted = Object.assign(new Error('The response was aborted'), { + name: 'ResponseAborted', + }) + expect(isClientDisconnectError(aborted)).toBe(true) + }) + + it('does not match an ordinary error', () => { + expect(isClientDisconnectError(new Error('boom'))).toBe(false) + expect(isClientDisconnectError(new TypeError('fetch failed'))).toBe(false) + }) + + it('does not match a real provider timeout or socket reset', () => { + // AbortSignal.timeout throws this shape all over lib/providers; treating + // it as a client disconnect would hide genuine integration failures. + expect(isClientDisconnectError(new DOMException('Aborted', 'AbortError'))).toBe(false) + expect( + isClientDisconnectError(Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' })), + ).toBe(false) + expect( + isClientDisconnectError( + Object.assign(new Error('Premature close'), { code: 'ERR_STREAM_PREMATURE_CLOSE' }), + ), + ).toBe(false) + }) + + it('matches the message exactly, so a real error cannot hide behind the phrase', () => { + expect( + isClientDisconnectError( + new Error('Supabase query failed: The destination stream closed early.'), + ), + ).toBe(false) + }) + + it('is false for anything that is not an error object', () => { + expect(isClientDisconnectError(null)).toBe(false) + expect(isClientDisconnectError(undefined)).toBe(false) + expect(isClientDisconnectError('The destination stream closed early.')).toBe(false) + expect(isClientDisconnectError(42)).toBe(false) + }) +}) diff --git a/lib/observability/is-client-disconnect.ts b/lib/observability/is-client-disconnect.ts new file mode 100644 index 00000000..3620c450 --- /dev/null +++ b/lib/observability/is-client-disconnect.ts @@ -0,0 +1,52 @@ +/** + * Is this error a client hanging up mid-response, rather than a failure? + * + * When a browser navigates away while a page or RSC payload is still + * streaming, the HTTP response closes, Next destroys the stream it handed + * React (`node_modules/next/dist/server/pipe-readable.js`), and React's + * `createCancelHandler` aborts the in-flight render with a bare + * `Error('The destination stream closed early.')`. Both renderers do it: the + * Flight server (react-server-dom-webpack-server.node.production.js:3922) and + * react-dom's SSR streamer (react-dom-server.node.production.js:7822, :8099). + * The response itself still completes: every occurrence we sampled in + * production rode on a 200. + * + * Next means to swallow these. `create-error-handler.js` early-returns on + * `isAbortError`, but that predicate knows only `name === 'AbortError'` and + * `name === 'ResponseAborted'` (pipe-readable.js:32), and React's cancel error + * is a plain `Error`, so it falls through to `onRequestError` and is reported + * as an exception that never happened. This predicate closes that gap on our + * side. It is a Next filtering gap, not a version bug: 16.3.1 and 16.3.4 are + * byte-identical here, so upgrading is not the fix. + * + * Matching is on the EXACT message. A React rewording brings the noise back, + * which is the safe direction; a substring match would let a real error hide + * behind the phrase. + * + * Deliberately NOT matched: a bare `AbortError`, `ECONNRESET`, + * `ERR_STREAM_PREMATURE_CLOSE`. Those are how this codebase's outbound calls + * report genuine failures (`AbortSignal.timeout` in `lib/http/fetch-with-timeout` + * and every provider client; `lib/providers/with-provider-call` treats them as + * retryable provider faults), and swallowing them would hide real integration + * breakage. They also buy nothing here: Next already filters `AbortError` + * before instrumentation ever sees it. + */ + +const CLIENT_DISCONNECT_MESSAGES = new Set([ + 'The destination stream closed early.', + 'The destination stream errored while writing data.', +]) + +/** + * Next's own error for a response the client aborted + * (`server/web/spec-extension/adapters/next-request.js:39`). Unlike a plain + * `AbortError`, this name is only ever produced by that one situation. + */ +const RESPONSE_ABORTED_NAME = 'ResponseAborted' + +export function isClientDisconnectError(error: unknown): boolean { + if (typeof error !== 'object' || error === null) return false + const { name, message } = error as { name?: unknown; message?: unknown } + if (name === RESPONSE_ABORTED_NAME) return true + return typeof message === 'string' && CLIENT_DISCONNECT_MESSAGES.has(message) +} diff --git a/lib/supabase/__tests__/middleware.test.ts b/lib/supabase/__tests__/middleware.test.ts index 8a952c8d..79715dac 100644 --- a/lib/supabase/__tests__/middleware.test.ts +++ b/lib/supabase/__tests__/middleware.test.ts @@ -48,18 +48,44 @@ const state = vi.hoisted(() => ({ // What the mocked isEmailOnBrandAllowlist returns (Rule 2 exemption). allowlisted: false, signOut: vi.fn(async () => ({ error: null })), + // Cookies auth-js writes through the `cookies.setAll` callback while + // getUser() runs: the ROTATED tokens after a successful refresh, and the + // maxAge-0 deletions when it removes a dead session. The middleware has to + // carry these onto whatever response it returns. + cookieWrites: [] as Array<{ + name: string + value: string + options?: Record + }>, // Row returned for user_preferences reads (the auto_logout mint lookup). userPreferences: null as null | { auto_logout: boolean }, userPreferencesError: null as unknown, })) vi.mock('@supabase/ssr', () => ({ - createServerClient: vi.fn(() => ({ + createServerClient: vi.fn(( + _url: string, + _key: string, + options: { + cookies: { + setAll: ( + cookies: Array<{ + name: string + value: string + options?: Record + }>, + ) => void + } + }, + ) => ({ auth: { - getUser: vi.fn(async () => ({ - data: { user: state.user }, - error: state.authError, - })), + getUser: vi.fn(async () => { + if (state.cookieWrites.length > 0) options.cookies.setAll(state.cookieWrites) + return { + data: { user: state.user }, + error: state.authError, + } + }), getClaims: vi.fn(async () => ({ data: { claims: state.sessionId ? { session_id: state.sessionId } : {} }, })), @@ -176,6 +202,7 @@ describe('updateSession redirect destinations', () => { state.user = null state.sessionId = 'session-1' state.authError = null + state.cookieWrites = [] state.aal = null state.factors = null state.company = { @@ -479,6 +506,30 @@ describe('updateSession redirect destinations', () => { expect(locationOf(response)).toBe(`${ORIGIN}/login`) }) + it('carries the cookies that clear a dead session', async () => { + // auth-js removes the session inside getUser() and queues the deletion + // on the response. Dropping it on the bounce made the browser replay + // the dead refresh token on /login, spending a second GoTrue 400 per + // expiry (paired 400s ~100 ms apart in production). + state.authError = { name: 'AuthApiError', code: 'refresh_token_not_found' } + state.cookieWrites = [ + { name: 'sb-test-auth-token', value: '', options: { path: '/', maxAge: 0 } }, + ] + + const response = await run('/settings/tax', { + headers: { + cookie: `sb-test-auth-token=dead; ${SESSION_TIMEOUT_COOKIE}=stale`, + }, + }) + + expect(response.status).toBe(307) + expect(new URL(locationOf(response)!).pathname).toBe('/login') + const cleared = response.cookies.get('sb-test-auth-token') + expect(cleared?.value).toBe('') + expect(cleared?.maxAge).toBe(0) + expect(response.cookies.get(SESSION_TIMEOUT_COOKIE)?.value).toBe('') + }) + it('drops a request path that normalises to a protocol-relative URL', async () => { // /..//evil.com normalises to the pathname //evil.com. Reflecting that // back as ?next= would hand the login page an off-origin destination. @@ -595,6 +646,17 @@ describe('updateSession redirect destinations', () => { expect(url.pathname).toBe('/mfa/verify') expect(url.searchParams.get('returnTo')).toBeNull() }) + + it('carries the rotated auth cookie instead of re-minting it next request', async () => { + state.cookieWrites = [ + { name: 'sb-test-auth-token', value: 'rotated', options: { path: '/' } }, + ] + + const response = await run('/settings/tax') + + expect(new URL(locationOf(response)!).pathname).toBe('/mfa/verify') + expect(response.cookies.get('sb-test-auth-token')?.value).toBe('rotated') + }) }) describe('forced enrollment bounce to /mfa/enroll', () => { diff --git a/lib/supabase/middleware.ts b/lib/supabase/middleware.ts index c639b94a..4d8aa468 100644 --- a/lib/supabase/middleware.ts +++ b/lib/supabase/middleware.ts @@ -267,7 +267,12 @@ async function updateSessionInner( supabase.auth.mfa.getAuthenticatorAssuranceLevel(), ) if (aal?.nextLevel === 'aal2' && aal?.currentLevel === 'aal1') { - return NextResponse.json({ error: 'MFA-verifiering krävs.' }, { status: 403 }) + const response = NextResponse.json( + { error: 'MFA-verifiering krävs.' }, + { status: 403 }, + ) + copyResponseCookies(supabaseResponse, response) + return response } } return supabaseResponse @@ -352,14 +357,17 @@ async function updateSessionInner( const destination = carriesDestination ? safeReturnTo(request.nextUrl.searchParams.get('next'), '/') : '/' - return NextResponse.redirect(new URL(destination, request.url)) + return redirectWithAuthCookies( + supabaseResponse, + new URL(destination, request.url), + ) } return supabaseResponse } // Protected routes - require authentication if (!user) { - return bounceToAuth(request, '/login') + return bounceToAuth(request, supabaseResponse, '/login') } // ── Home-domain affinity (WL, founder call 2026-08-05) ────────────────── @@ -380,7 +388,7 @@ async function updateSessionInner( request, ) if (homeOutcome.redirectTo) { - return NextResponse.redirect(homeOutcome.redirectTo) + return redirectWithAuthCookies(supabaseResponse, homeOutcome.redirectTo) } if (homeOutcome.cacheOk) { supabaseResponse.cookies.set( @@ -408,7 +416,8 @@ async function updateSessionInner( const mfaTarget = `/mfa/enroll${ innerReturnTo ? `?returnTo=${encodeURIComponent(innerReturnTo)}` : '' }` - return NextResponse.redirect( + return redirectWithAuthCookies( + supabaseResponse, new URL( `/account/set-password?returnTo=${encodeURIComponent(mfaTarget)}`, request.url, @@ -452,7 +461,7 @@ async function updateSessionInner( // User has MFA enrolled but hasn't verified this session → redirect to verify if (aal?.nextLevel === 'aal2' && aal?.currentLevel === 'aal1') { - return bounceToAuth(request, '/mfa/verify') + return bounceToAuth(request, supabaseResponse, '/mfa/verify') } // MFA required but user has no factor enrolled yet → force enrollment @@ -476,7 +485,7 @@ async function updateSessionInner( const hasVerifiedFactor = factors?.totp?.some(f => f.status === 'verified') if (!hasVerifiedFactor) { - return bounceToAuth(request, '/mfa/enroll') + return bounceToAuth(request, supabaseResponse, '/mfa/enroll') } } } @@ -568,7 +577,7 @@ async function updateSessionInner( if (isByraNoCompanyAllowed) { return supabaseResponse } - return NextResponse.redirect(new URL('/byra', request.url)) + return redirectWithAuthCookies(supabaseResponse, new URL('/byra', request.url)) } // Multi-user seat gate: memberships exist but every one is frozen for @@ -593,7 +602,10 @@ async function updateSessionInner( .maybeSingle() const destination = enrichmentRow ? '/select-company' : '/onboarding' - return NextResponse.redirect(new URL(destination, request.url)) + return redirectWithAuthCookies( + supabaseResponse, + new URL(destination, request.url), + ) } // Set company cookie on the response so downstream requests have it @@ -674,6 +686,28 @@ function copyResponseCookies(from: NextResponse, to: NextResponse): void { } } +/** + * Redirect that carries the auth cookies queued on `authResponse`. + * + * auth-js writes through the `setAll` callback while `getUser()` runs: a + * successful refresh puts the ROTATED tokens on the response, a dead session + * puts the cookie DELETIONS there. A bare `NextResponse.redirect()` throws + * both away, so the browser replays the old cookie on the very next request: + * a consumed refresh token on the happy path, and a dead one on the expiry + * path, where production showed the bounce and the following /login each + * spending their own GoTrue 400 about 100 ms apart. Every response that + * replaces `supabaseResponse` has to go through here (or through + * `copyResponseCookies`, for the non-redirect ones). + */ +function redirectWithAuthCookies( + authResponse: NextResponse, + url: URL | string, +): NextResponse { + const response = NextResponse.redirect(url) + copyResponseCookies(authResponse, response) + return response +} + function sessionTimeoutResponse( request: NextRequest, authResponse: NextResponse, @@ -755,9 +789,14 @@ const AUTH_DESTINATION_PARAM = { * that was going to happen anyway, on exactly the same conditions. The auth * pages navigate to the destination only after the step-up succeeds, and the * next request re-runs this same gate regardless. + * + * The bounce also has to carry the cookies auth-js queued on the response + * while `getUser()` ran, hence `authResponse`: see + * `redirectWithAuthCookies`. */ function bounceToAuth( request: NextRequest, + authResponse: NextResponse, target: keyof typeof AUTH_DESTINATION_PARAM, ) { // Absolute-path reference: replaces path AND clears query/fragment. @@ -769,7 +808,7 @@ function bounceToAuth( if (destination !== '/') { url.search = `${AUTH_DESTINATION_PARAM[target]}=${encodeURIComponent(destination)}` } - return NextResponse.redirect(url) + return redirectWithAuthCookies(authResponse, url) } /**