Fix/critical issues (#351)
* fix: add 15s timeout to accounting provider HTTP clients Node's built-in fetch has no default timeout, so a stalled provider could hold a serverless worker open for many minutes — worse with withRetry (6x on Fortnox, 3x on others) and getPaginated stacking across pages. Wrap each fetch() in the Fortnox, Visma, Bokio, Briox, and Björn Lundén clients with signal: AbortSignal.timeout(15_000), and treat TimeoutError/AbortError as retryable so a single stalled attempt retries cleanly instead of hanging the request. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: add timeouts to OAuth token endpoints Wrap every OAuth2 token exchange, refresh, and revoke POST in an AbortController via a new fetchWithTimeout helper. Without this, a hung provider endpoint holds the request thread indefinitely — worst case being Skatteverket, where refreshAccessToken sits on the hot path of every bookkeeping action and exchangeCodeForTokens races the 5-minute BankID auth-code TTL. On timeout, the Skatteverket OAuth callback now redirects to /reports?tab=vat-declaration with a Swedish retry message instead of leaving the user stranded on the callback URL. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: close RLS escalation on membership and settings tables Any authenticated user who was a member (including viewer) could issue a direct PostgREST PATCH against company_members and promote themselves to owner, bypassing the app-layer requireWritePermission guard entirely. Reproduced on prod, then verified the fix on staging. Tighten INSERT/UPDATE/DELETE policies on company_members, team_members, api_keys, company_invitations, team_invitations, companies, teams, and company_settings to require the caller to hold role IN ('owner','admin') in the target company/team. Role check is wrapped in SECURITY DEFINER helpers (user_is_company_admin, user_is_team_admin, user_role_in_company) to avoid RLS recursion when a policy on company_members references company_members in its subquery. Add a BEFORE UPDATE trigger on company_members that rejects any role change unless the caller already holds role='owner', so admins cannot mint further owners even though they can otherwise write. Legitimate write paths are unaffected: company creation goes through the create_company_with_owner SECURITY DEFINER RPC, invite acceptance uses the service role, and team->company membership syncs via SECURITY DEFINER triggers. All bypass RLS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(migrations): resolve duplicate schema_migrations version 20260421160000 Two migration files shared timestamp 20260421160000 on main (booking_template_usage.sql and opening_balances_rpc.sql), causing supabase_migrations.schema_migrations PK collisions on any fresh CI run: duplicate key value violates unique constraint "schema_migrations_pkey" Key (version)=(20260421160000) already exists. Bump opening_balances_rpc.sql to 20260421160500. booking_template_usage keeps 20260421160000 because its table already exists on prod; the renamed file has an idempotent CREATE OR REPLACE FUNCTION body and has not yet been deployed to prod, so moving its version is free. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(migrations): make booking_template_usage migration idempotent The table already exists on prod (applied out-of-band) but prod's schema_migrations does not track version 20260421160000, so the next PR-driven deploy would re-run this migration and fail on `CREATE TABLE public.booking_template_usage` with a duplicate-relation error. Add IF NOT EXISTS to CREATE TABLE and CREATE INDEX, and DROP POLICY IF EXISTS before each CREATE POLICY. No functional change on fresh databases; prod just silently no-ops the table/index creates and re-declares policies without dropping-then-missing them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: implement isTimeoutError utility and enforce role restrictions on company_members insert * fix: implement fallback for user_id in commit_journal_entry function when auth.uid() is NULL --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
e13a450e21
commit
02f94ef631
@@ -0,0 +1,124 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
|
||||
import { fetchWithTimeout, TimeoutError } from '../fetch-with-timeout'
|
||||
|
||||
describe('fetchWithTimeout', () => {
|
||||
const originalFetch = globalThis.fetch
|
||||
|
||||
beforeEach(() => {
|
||||
vi.resetAllMocks()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
globalThis.fetch = originalFetch
|
||||
})
|
||||
|
||||
it('returns the response when fetch resolves before the deadline', async () => {
|
||||
const mockResponse = new Response('ok', { status: 200 })
|
||||
globalThis.fetch = vi.fn().mockResolvedValue(mockResponse)
|
||||
|
||||
const result = await fetchWithTimeout(
|
||||
'https://example.test/',
|
||||
{ method: 'POST' },
|
||||
{ timeoutMs: 1000, description: 'test fetch' },
|
||||
)
|
||||
|
||||
expect(result).toBe(mockResponse)
|
||||
expect(globalThis.fetch).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('throws TimeoutError when fetch hangs past the timeout', async () => {
|
||||
globalThis.fetch = vi.fn().mockImplementation((_url, init?: RequestInit) => {
|
||||
return new Promise((_resolve, reject) => {
|
||||
const signal = init?.signal
|
||||
if (!signal) return
|
||||
signal.addEventListener('abort', () => {
|
||||
const reason = (signal as AbortSignal & { reason?: unknown }).reason
|
||||
const err = new Error('aborted')
|
||||
err.name =
|
||||
reason instanceof DOMException ? reason.name : 'AbortError'
|
||||
reject(err)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
const start = Date.now()
|
||||
await expect(
|
||||
fetchWithTimeout(
|
||||
'https://example.test/',
|
||||
{ method: 'POST' },
|
||||
{ timeoutMs: 50, description: 'slow fetch' },
|
||||
),
|
||||
).rejects.toBeInstanceOf(TimeoutError)
|
||||
const elapsed = Date.now() - start
|
||||
expect(elapsed).toBeGreaterThanOrEqual(40)
|
||||
expect(elapsed).toBeLessThan(1000)
|
||||
})
|
||||
|
||||
it('includes the description and timeout ms in the error message', async () => {
|
||||
globalThis.fetch = vi.fn().mockImplementation((_url, init?: RequestInit) => {
|
||||
return new Promise((_resolve, reject) => {
|
||||
const signal = init?.signal
|
||||
signal?.addEventListener('abort', () => {
|
||||
const err = new Error('aborted')
|
||||
err.name = 'TimeoutError'
|
||||
reject(err)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
try {
|
||||
await fetchWithTimeout(
|
||||
'https://example.test/',
|
||||
{ method: 'POST' },
|
||||
{ timeoutMs: 50, description: 'Fortnox token exchange' },
|
||||
)
|
||||
expect.fail('expected TimeoutError to be thrown')
|
||||
} catch (err) {
|
||||
expect(err).toBeInstanceOf(TimeoutError)
|
||||
expect((err as Error).name).toBe('TimeoutError')
|
||||
expect((err as Error).message).toContain('Fortnox token exchange')
|
||||
expect((err as Error).message).toContain('50')
|
||||
}
|
||||
})
|
||||
|
||||
it('propagates non-timeout errors unchanged', async () => {
|
||||
const networkError = new TypeError('fetch failed')
|
||||
globalThis.fetch = vi.fn().mockRejectedValue(networkError)
|
||||
|
||||
await expect(
|
||||
fetchWithTimeout(
|
||||
'https://example.test/',
|
||||
{ method: 'POST' },
|
||||
{ timeoutMs: 1000, description: 'test fetch' },
|
||||
),
|
||||
).rejects.toBe(networkError)
|
||||
})
|
||||
|
||||
it('honours an external AbortSignal without labelling it a timeout', async () => {
|
||||
const externalController = new AbortController()
|
||||
globalThis.fetch = vi.fn().mockImplementation((_url, init?: RequestInit) => {
|
||||
return new Promise((_resolve, reject) => {
|
||||
init?.signal?.addEventListener('abort', () => {
|
||||
const err = new Error('aborted by caller')
|
||||
err.name = 'AbortError'
|
||||
reject(err)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
const promise = fetchWithTimeout(
|
||||
'https://example.test/',
|
||||
{ method: 'POST', signal: externalController.signal },
|
||||
{ timeoutMs: 10_000, description: 'caller-cancelable fetch' },
|
||||
)
|
||||
|
||||
externalController.abort()
|
||||
|
||||
await expect(promise).rejects.toSatisfy(
|
||||
(err: unknown) =>
|
||||
err instanceof Error &&
|
||||
!(err instanceof TimeoutError) &&
|
||||
err.name === 'AbortError',
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,54 @@
|
||||
/**
|
||||
* Thin `fetch` wrapper that aborts after a deadline.
|
||||
*
|
||||
* Used by OAuth token endpoints where a hung provider would otherwise hold
|
||||
* the request thread indefinitely. The Skatteverket callback handler also
|
||||
* relies on `TimeoutError` to distinguish a hung token exchange (where the
|
||||
* 5-minute BankID auth code may expire mid-call) from other failures.
|
||||
*/
|
||||
|
||||
export class TimeoutError extends Error {
|
||||
readonly name = 'TimeoutError'
|
||||
}
|
||||
|
||||
export function isTimeoutError(error: unknown): boolean {
|
||||
return (
|
||||
error instanceof Error &&
|
||||
(error.name === 'TimeoutError' || error.name === 'AbortError')
|
||||
)
|
||||
}
|
||||
|
||||
export const OAUTH_TIMEOUT_MS = 10_000
|
||||
export const OAUTH_REVOKE_TIMEOUT_MS = 5_000
|
||||
export const SKATTEVERKET_EXCHANGE_TIMEOUT_MS = 8_000
|
||||
|
||||
interface FetchWithTimeoutOptions {
|
||||
timeoutMs: number
|
||||
description: string
|
||||
}
|
||||
|
||||
export async function fetchWithTimeout(
|
||||
input: RequestInfo | URL,
|
||||
init: RequestInit,
|
||||
options: FetchWithTimeoutOptions,
|
||||
): Promise<Response> {
|
||||
const { timeoutMs, description } = options
|
||||
|
||||
const timeoutSignal = AbortSignal.timeout(timeoutMs)
|
||||
const signal = init.signal
|
||||
? AbortSignal.any([init.signal, timeoutSignal])
|
||||
: timeoutSignal
|
||||
|
||||
try {
|
||||
return await fetch(input, { ...init, signal })
|
||||
} catch (err) {
|
||||
if (
|
||||
timeoutSignal.aborted &&
|
||||
err instanceof Error &&
|
||||
(err.name === 'TimeoutError' || err.name === 'AbortError')
|
||||
) {
|
||||
throw new TimeoutError(`${description} timed out after ${timeoutMs}ms`)
|
||||
}
|
||||
throw err
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user