diff --git a/app/api/salary/employees/[id]/absence/__tests__/route.test.ts b/app/api/salary/employees/[id]/absence/__tests__/route.test.ts index 82e8532c..c6765e0e 100644 --- a/app/api/salary/employees/[id]/absence/__tests__/route.test.ts +++ b/app/api/salary/employees/[id]/absence/__tests__/route.test.ts @@ -82,4 +82,43 @@ describe('POST /api/salary/employees/[id]/absence', () => { const response = await POST(post({ absence_date: '2026-07-01', absence_type: 'sick', hours: 8 }), params) expect(response.status).toBe(404) }) + + it('does not leak raw PG text for DB failures (42501)', async () => { + enqueue({ data: { id: 'emp-1' } }) // loadEmployee + enqueue({ + data: null, + error: { + code: '42501', + message: + 'new row violates row-level security policy for table "salary_absence_franvaro_audit"', + }, + }) // upsert denied + + const response = await POST(post({ absence_date: '2026-07-01', absence_type: 'parental', hours: 8 }), params) + const { status, body } = await parseJsonResponse<{ error: string; code: string }>(response) + + expect(status).toBe(500) + expect(body.code).toBe('DB_PERMISSION_DENIED') + expect(body.error).not.toMatch(/row-level security/) + // The registry's Swedish message is shown instead. + expect(body.error).toContain('behörighetsfel') + }) + + it('still passes the 24h-cap trigger detail through (Swedish, user-facing)', async () => { + enqueue({ data: { id: 'emp-1' } }) // loadEmployee + enqueue({ + data: null, + error: { + code: '23514', + message: 'Total tid (arbete + frånvaro) för 2026-07-01 får inte överstiga 24 timmar', + }, + }) // 24h cap trips + + const response = await POST(post({ absence_date: '2026-07-01', absence_type: 'sick', hours: 20 }), params) + const { status, body } = await parseJsonResponse<{ error: string; code: string }>(response) + + expect(status).toBe(409) + expect(body.code).toBe('ABSENCE_HOURS_CONFLICT') + expect(body.error).toContain('Total tid') + }) }) diff --git a/app/api/salary/employees/[id]/absence/route.ts b/app/api/salary/employees/[id]/absence/route.ts index 98a0c62c..af9a9ae5 100644 --- a/app/api/salary/employees/[id]/absence/route.ts +++ b/app/api/salary/employees/[id]/absence/route.ts @@ -21,8 +21,13 @@ ensureInitialized() function errorResponse(code: string, details?: Record): NextResponse { const entry = getErrorEntry(code) + // Only the 24h-cap trigger's Swedish text is user-facing detail; for every + // other code details.message is raw Postgres text and must not reach the + // client (the registry message is shown instead). const message = - (details?.message as string | undefined) ?? entry?.message_sv ?? 'Något gick fel' + (code === 'ABSENCE_HOURS_CONFLICT' ? (details?.message as string | undefined) : undefined) ?? + entry?.message_sv ?? + 'Något gick fel' return NextResponse.json({ error: message, code }, { status: entry?.httpStatus ?? 500 }) } diff --git a/lib/errors/structured-errors.ts b/lib/errors/structured-errors.ts index 61c82c66..d306cf51 100644 --- a/lib/errors/structured-errors.ts +++ b/lib/errors/structured-errors.ts @@ -81,6 +81,16 @@ const GENERIC: Record = { message_sv: 'Du har inte behörighet att utföra denna åtgärd.', message_en: 'Insufficient permissions.', }, + // A Postgres privilege/RLS denial (42501) on a write the application + // expected to succeed: a server-side configuration bug (e.g. a SECURITY + // INVOKER trigger writing to a policy-less RLS table), not a user-permission + // problem. Kept distinct from FORBIDDEN (which blames the user) and from + // INTERNAL_ERROR (which hides the failure mode from diagnostics). + DB_PERMISSION_DENIED: { + httpStatus: 500, + message_sv: 'Ett behörighetsfel i databasen stoppade åtgärden. Kontakta supporten om felet kvarstår.', + message_en: 'A database permission (RLS) denial blocked the write. This indicates a server-side misconfiguration.', + }, NOT_FOUND: { httpStatus: 404, message_sv: 'Resursen kunde inte hittas.', diff --git a/lib/pending-operations/__tests__/payroll-executors.test.ts b/lib/pending-operations/__tests__/payroll-executors.test.ts index e37454cd..f771a6b8 100644 --- a/lib/pending-operations/__tests__/payroll-executors.test.ts +++ b/lib/pending-operations/__tests__/payroll-executors.test.ts @@ -184,6 +184,49 @@ describe('commitPendingOperation: register_absence', () => { expect(result.status).toBe('rejected') expect(result.http_status).toBe(409) }) + + it('logs the PG details and persists a sanitized error_code when the upsert fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const { supabase, enqueue, findCalls } = createQueuedMockSupabase() + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: { id: 'emp-1' } }) // assertEmployee + enqueue({ + data: null, + error: { + code: '42501', + message: + 'new row violates row-level security policy for table "salary_absence_franvaro_audit"', + }, + }) // upsert denied (the franvaro audit-trigger bug) + enqueue({ data: null, error: null }) // finalize (failed) + + const op = makePendingOp({ + operation_type: 'register_absence', + params: { employee_id: 'emp-1', from: '2026-03-02', to: '2026-03-02', absence_type: 'parental' }, + }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + expect(result.status).toBe('failed') + expect(result.http_status).toBe(500) + expect(result.code).toBe('DB_PERMISSION_DENIED') + // The op row keeps the structured code so the failure mode is + // diagnosable later, but never the raw PG text. + const updates = findCalls('pending_operations', 'update') + const finalize = updates[updates.length - 1]![0] as { result_data?: Record } + expect(finalize.result_data).toMatchObject({ + error_code: 'DB_PERMISSION_DENIED', + http_status: 500, + }) + expect(JSON.stringify(finalize.result_data)).not.toContain('row-level security') + // The raw PG message goes to the log: it is persisted nowhere else. + const logged = consoleError.mock.calls.map((c) => c.join(' ')).join('\n') + expect(logged).toContain('register_absence commit failed') + expect(logged).toContain('row-level security') + } finally { + consoleError.mockRestore() + } + }) }) describe('commitPendingOperation: book_salary_run', () => { @@ -279,6 +322,34 @@ describe('commitPendingOperation: delete_absence', () => { expect(result.status).not.toBe('committed') expect(result.error).toBeDefined() }) + + it('logs the PG details and persists a sanitized error_code when the delete fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + try { + const { supabase, enqueue, findCalls } = createQueuedMockSupabase() + enqueue({ data: { id: 'op-1' }, error: null }) // CAS claim + enqueue({ data: { id: 'emp-1' } }) // assertEmployee + enqueue({ data: null, error: { code: '57014', message: 'canceling statement due to statement timeout' } }) // delete fails + enqueue({ data: null, error: null }) // finalize (failed) + + const op = makePendingOp({ + operation_type: 'delete_absence', + params: { employee_id: 'emp-1', from: '2026-03-02', to: '2026-03-06' }, + }) + const result = await commitPendingOperation(supabase as never, 'user-1', 'company-1', op) + + expect(result.status).toBe('failed') + expect(result.code).toBe('INTERNAL_ERROR') + const updates = findCalls('pending_operations', 'update') + const finalize = updates[updates.length - 1]![0] as { result_data?: Record } + expect(finalize.result_data).toMatchObject({ error_code: 'INTERNAL_ERROR' }) + const logged = consoleError.mock.calls.map((c) => c.join(' ')).join('\n') + expect(logged).toContain('delete_absence commit failed') + expect(logged).toContain('statement timeout') + } finally { + consoleError.mockRestore() + } + }) }) describe('commitPendingOperation: create_employee', () => { diff --git a/lib/pending-operations/commit.ts b/lib/pending-operations/commit.ts index 93ead551..d13ad656 100644 --- a/lib/pending-operations/commit.ts +++ b/lib/pending-operations/commit.ts @@ -4982,9 +4982,23 @@ async function commitRegisterAbsence( includeWeekends: (params.include_weekends as boolean | undefined) ?? false, }) if (!result.ok) { + // The user-facing message is generic Swedish; without this log the + // underlying PG error (e.g. the franvaro audit-trigger RLS denial that + // caused five untraceable 500s, feedback 2026-08-13) leaves no trace. + // pgDetails, not details: `details` is the logger record's own field + // for non-object args and would be swallowed by the pretty emitter. + createLogger('commit/register_absence').error('register_absence commit failed', { + code: result.code, + pgDetails: result.details, + employeeId, + absenceType, + from, + to, + }) const entry = getErrorEntry(result.code) return { error: entry?.message_sv ?? `Kunde inte registrera frånvaron: ${result.code}`, + errorCode: result.code, status: entry?.httpStatus ?? 500, } } @@ -5081,9 +5095,19 @@ async function commitDeleteAbsence( absenceType: (params.absence_type as string | undefined) || undefined, }) if (!result.ok) { + // Same diagnosability treatment as commitRegisterAbsence: keep the PG + // error in the logs and the registry code on the op row. + createLogger('commit/delete_absence').error('delete_absence commit failed', { + code: result.code, + pgDetails: result.details, + employeeId, + from, + to, + }) const entry = getErrorEntry(result.code) return { error: entry?.message_sv ?? `Kunde inte ta bort frånvaron: ${result.code}`, + errorCode: result.code, status: entry?.httpStatus ?? 500, } } diff --git a/lib/salary/__tests__/absence.test.ts b/lib/salary/__tests__/absence.test.ts index 8c5b3f78..4d4e4849 100644 --- a/lib/salary/__tests__/absence.test.ts +++ b/lib/salary/__tests__/absence.test.ts @@ -160,6 +160,72 @@ describe('upsertAbsenceRange', () => { expect(result.ok).toBe(false) if (!result.ok) expect(result.code).toBe('ABSENCE_HOURS_CONFLICT') }) + + it('maps a non-24h CHECK violation (23514) to VALIDATION_ERROR, not ABSENCE_HOURS_CONFLICT', async () => { + mock.enqueue({ data: { id: EMPLOYEE_ID } }) + mock.enqueue({ + data: null, + error: { + code: '23514', + message: + 'new row for relation "salary_absence_days" violates check constraint "salary_absence_days_hours_check"', + }, + }) + + const result = await upsertAbsenceRange(supabase, { + companyId: COMPANY_ID, + employeeId: EMPLOYEE_ID, + from: '2026-03-02', + to: '2026-03-02', + absenceType: 'sick', + hoursPerDay: 30, + }) + + expect(result.ok).toBe(false) + if (!result.ok) expect(result.code).toBe('VALIDATION_ERROR') + }) + + it('maps an RLS/privilege denial (42501) to DB_PERMISSION_DENIED with the PG message in details', async () => { + mock.enqueue({ data: { id: EMPLOYEE_ID } }) + mock.enqueue({ + data: null, + error: { + code: '42501', + message: + 'new row violates row-level security policy for table "salary_absence_franvaro_audit"', + }, + }) + + const result = await upsertAbsenceRange(supabase, { + companyId: COMPANY_ID, + employeeId: EMPLOYEE_ID, + from: '2026-03-02', + to: '2026-03-02', + absenceType: 'parental', + }) + + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.code).toBe('DB_PERMISSION_DENIED') + expect(result.details?.message).toMatch(/row-level security/) + } + }) + + it('keeps unrecognized DB errors as INTERNAL_ERROR', async () => { + mock.enqueue({ data: { id: EMPLOYEE_ID } }) + mock.enqueue({ data: null, error: { code: '57014', message: 'canceling statement due to statement timeout' } }) + + const result = await upsertAbsenceRange(supabase, { + companyId: COMPANY_ID, + employeeId: EMPLOYEE_ID, + from: '2026-03-02', + to: '2026-03-02', + absenceType: 'sick', + }) + + expect(result.ok).toBe(false) + if (!result.ok) expect(result.code).toBe('INTERNAL_ERROR') + }) }) describe('upsertAbsenceDay', () => { diff --git a/lib/salary/__tests__/franvaro-specnummer.pg.test.ts b/lib/salary/__tests__/franvaro-specnummer.pg.test.ts new file mode 100644 index 00000000..bfac0363 --- /dev/null +++ b/lib/salary/__tests__/franvaro-specnummer.pg.test.ts @@ -0,0 +1,202 @@ +import { randomUUID } from 'crypto' +import { describe, expect, it } from 'vitest' +import type { PoolClient } from 'pg' +import { seedCompany } from '@/tests/pg/fixtures' +import { getPool, withUserContext } from '@/tests/pg/setup' + +/** + * Regression lock for the franvaro-specifikationsnummer trigger under the + * `authenticated` role (feedback 2026-08-13: register_absence 500 for + * foraldraledighet/VAB). + * + * Migration 20260517135000 made the BEFORE INSERT trigger on + * salary_absence_days write an audit row into salary_absence_franvaro_audit, + * a table with RLS enabled and zero policies, from a SECURITY INVOKER + * function. Every vab/parental INSERT from a non-BYPASSRLS role then failed + * with 42501 while 'sick' (which skips the trigger) kept working. Migration + * 20260813120000 makes both trigger functions SECURITY DEFINER with a pinned + * search_path; these tests fail with /row-level security|permission denied/ + * without it. + * + * The audit table deliberately has no policies (trigger/service-only + * writes), so audit assertions run on the superuser pool connection after + * RESET ROLE inside the same transaction (withUserContext always rolls back, + * so nothing persists across tests). + */ + +async function insertEmployee(params: { + userId: string + companyId: string +}): Promise { + const id = randomUUID() + // personnummer must be 12 digits; last4 mirrors the last four chars. + const pnr = '199001011234' + await getPool().query( + `INSERT INTO public.employees + (id, user_id, company_id, first_name, last_name, personnummer, + personnummer_last4, employment_start, monthly_salary, tax_table_number) + VALUES ($1, $2, $3, 'Test', 'Person', $4, '1234', '2026-01-01', 30000, 32)`, + [id, params.userId, params.companyId, pnr], + ) + return id +} + +async function insertAbsenceDayAs( + client: PoolClient, + params: { + companyId: string + employeeId: string + date: string + type: string + }, +): Promise<{ id: string; specnummer: number | null }> { + const res = await client.query<{ id: string; franvaro_specifikationsnummer: number | null }>( + `INSERT INTO public.salary_absence_days + (company_id, employee_id, absence_date, absence_type, hours) + VALUES ($1, $2, $3, $4, 8) + RETURNING id, franvaro_specifikationsnummer`, + [params.companyId, params.employeeId, params.date, params.type], + ) + return { + id: res.rows[0]!.id, + specnummer: res.rows[0]!.franvaro_specifikationsnummer, + } +} + +interface AuditRow { + absence_day_id: string + year_month: string + new_specifikationsnummer: number + trigger_op: string +} + +/** Read the audit table as superuser (no policies exist by design). */ +async function readAudit(client: PoolClient, employeeId: string): Promise { + await client.query('RESET ROLE') + const res = await client.query( + `SELECT absence_day_id, year_month, new_specifikationsnummer, trigger_op + FROM public.salary_absence_franvaro_audit + WHERE employee_id = $1 + ORDER BY assigned_at, new_specifikationsnummer`, + [employeeId], + ) + return res.rows +} + +describe('franvaro-specnummer.pg: authenticated-role vab/parental inserts', () => { + it('parental INSERT succeeds under role authenticated and mints the shared per-month sequence + audit rows', async () => { + const a = await seedCompany() + const emp = await insertEmployee({ userId: a.userId, companyId: a.companyId }) + + await withUserContext(a.userId, async (client) => { + const day1 = await insertAbsenceDayAs(client, { + companyId: a.companyId, + employeeId: emp, + date: '2026-03-02', + type: 'parental', + }) + expect(day1.specnummer).toBe(1) + + const day2 = await insertAbsenceDayAs(client, { + companyId: a.companyId, + employeeId: emp, + date: '2026-03-03', + type: 'parental', + }) + expect(day2.specnummer).toBe(2) + + // vab shares the same per-(employee, year-month) sequence. + const vabDay = await insertAbsenceDayAs(client, { + companyId: a.companyId, + employeeId: emp, + date: '2026-03-04', + type: 'vab', + }) + expect(vabDay.specnummer).toBe(3) + + // A different month restarts the sequence. + const aprilDay = await insertAbsenceDayAs(client, { + companyId: a.companyId, + employeeId: emp, + date: '2026-04-01', + type: 'parental', + }) + expect(aprilDay.specnummer).toBe(1) + + const audit = await readAudit(client, emp) + expect(audit).toHaveLength(4) + expect(audit.every((r) => r.trigger_op === 'insert')).toBe(true) + const march = audit.filter((r) => r.year_month === '2026-03') + expect(march.map((r) => r.new_specifikationsnummer)).toEqual([1, 2, 3]) + expect(march.map((r) => r.absence_day_id)).toEqual([day1.id, day2.id, vabDay.id]) + const april = audit.filter((r) => r.year_month === '2026-04') + expect(april.map((r) => r.new_specifikationsnummer)).toEqual([1]) + }) + }) + + it('sick days skip the trigger: no specnummer, no audit row', async () => { + const a = await seedCompany() + const emp = await insertEmployee({ userId: a.userId, companyId: a.companyId }) + + await withUserContext(a.userId, async (client) => { + const sickDay = await insertAbsenceDayAs(client, { + companyId: a.companyId, + employeeId: emp, + date: '2026-03-02', + type: 'sick', + }) + expect(sickDay.specnummer).toBeNull() + + const audit = await readAudit(client, emp) + expect(audit).toHaveLength(0) + }) + }) + + it('upsert retry (ON CONFLICT DO UPDATE) is idempotent: no error, specnummer unchanged', async () => { + const a = await seedCompany() + const emp = await insertEmployee({ userId: a.userId, companyId: a.companyId }) + + await withUserContext(a.userId, async (client) => { + const day1 = await insertAbsenceDayAs(client, { + companyId: a.companyId, + employeeId: emp, + date: '2026-03-02', + type: 'parental', + }) + expect(day1.specnummer).toBe(1) + + // Mirror the PostgREST upsert lib/salary/absence.ts sends: the payload + // columns land in SET, franvaro_specifikationsnummer is never touched. + const retry = await client.query<{ franvaro_specifikationsnummer: number | null }>( + `INSERT INTO public.salary_absence_days + (company_id, employee_id, absence_date, absence_type, hours) + VALUES ($1, $2, '2026-03-02', 'parental', 4) + ON CONFLICT (employee_id, absence_date, absence_type) + DO UPDATE SET hours = EXCLUDED.hours + RETURNING franvaro_specifikationsnummer`, + [a.companyId, emp], + ) + expect(retry.rows[0]!.franvaro_specifikationsnummer).toBe(1) + }) + }) + + it('both trigger functions are SECURITY DEFINER with a pinned search_path', async () => { + const res = await getPool().query<{ + proname: string + prosecdef: boolean + proconfig: string[] | null + }>( + `SELECT proname, prosecdef, proconfig + FROM pg_proc + WHERE proname IN ( + 'assign_franvaro_specifikationsnummer', + 'assign_franvaro_specifikationsnummer_on_update' + )`, + ) + expect(res.rows).toHaveLength(2) + for (const row of res.rows) { + expect(row.prosecdef).toBe(true) + expect(row.proconfig ?? []).toContain('search_path=public, pg_temp') + } + }) +}) diff --git a/lib/salary/absence.ts b/lib/salary/absence.ts index 07753692..0e6839d0 100644 --- a/lib/salary/absence.ts +++ b/lib/salary/absence.ts @@ -91,11 +91,23 @@ function mapInsertError(error: { code?: string; message?: string }): { code: string details?: Record } { - // The 24h cap trigger raises check_violation when worked + absence > 24h - // for the same date. - if (error.code === '23514' || error.message?.includes('Total tid')) { + // Privilege/RLS denial (42501). Seen when a DB trigger writes to a + // protected table as SECURITY INVOKER (the franvaro audit-table bug fixed + // in migration 20260813120000): a server-side configuration error, kept + // distinct from INTERNAL_ERROR so the failure mode is diagnosable. + if (error.code === '42501') { + return { code: 'DB_PERMISSION_DENIED', details: { message: error.message } } + } + // The 24h cap trigger raises check_violation with 'Total tid' text when + // worked + absence hours exceed 24h for the same date. + if (error.message?.includes('Total tid')) { return { code: 'ABSENCE_HOURS_CONFLICT', details: { message: error.message } } } + // Any other CHECK violation (hours range, absence_type enum) is invalid + // input, not an hours conflict. + if (error.code === '23514') { + return { code: 'VALIDATION_ERROR', details: { message: error.message } } + } return { code: 'INTERNAL_ERROR', details: { message: error.message } } } diff --git a/supabase/migrations/20260813120000_fix_franvaro_audit_trigger_definer.sql b/supabase/migrations/20260813120000_fix_franvaro_audit_trigger_definer.sql new file mode 100644 index 00000000..594d306d --- /dev/null +++ b/supabase/migrations/20260813120000_fix_franvaro_audit_trigger_definer.sql @@ -0,0 +1,33 @@ +-- Fix: register_absence 500 for foraldraledighet/VAB from user-scoped surfaces. +-- +-- Migration 20260517135000_skatteverket_audit_franvaro_lock.sql created +-- salary_absence_franvaro_audit with ENABLE ROW LEVEL SECURITY and ZERO +-- policies, and rewrote the specifikationsnummer trigger functions to INSERT +-- an audit row into it. That migration's comment claims the trigger "runs +-- SECURITY DEFINER (implicit in plpgsql functions that own the table)"; the +-- claim is false: plpgsql functions default to SECURITY INVOKER, so the audit +-- INSERT executes as the calling role. Under any non-BYPASSRLS role (role +-- authenticated: the dashboard absence POST, the web /pending approval, the +-- in-app Assistenten chat committing staged operations) the INSERT is denied +-- with SQLSTATE 42501, which aborts the whole salary_absence_days write. The +-- trigger fires only for absence_type IN ('vab', 'parental'), which is why +-- exactly those registrations failed with a generic 500 while 'sick' and +-- every other type kept working. Service-role paths (BYPASSRLS) were never +-- affected. +-- +-- Fix: make both trigger functions SECURITY DEFINER so the audit INSERT runs +-- as the function owner (the migration runner, which also owns the audit +-- table; RLS is not FORCEd, so the owner is exempt). search_path is pinned +-- because SECURITY DEFINER without it is a privilege-escalation footgun. +-- +-- Deliberately NO RLS policy is added on salary_absence_franvaro_audit: the +-- design intent is trigger/service-only writes, and an INSERT policy for +-- authenticated would let clients forge audit rows. + +ALTER FUNCTION public.assign_franvaro_specifikationsnummer() + SECURITY DEFINER + SET search_path = public, pg_temp; + +ALTER FUNCTION public.assign_franvaro_specifikationsnummer_on_update() + SECURITY DEFINER + SET search_path = public, pg_temp;