diff --git a/src/lib/errorReporting.ts b/src/lib/errorReporting.ts index 4931b78..e0f693e 100644 --- a/src/lib/errorReporting.ts +++ b/src/lib/errorReporting.ts @@ -7,7 +7,15 @@ import {logger} from './logger'; * production failures become visible. Privacy: no user id, no email — only the * error, the route, an optional tool context, user agent, and locale. * - * Guards against noise and abuse: + * Reports go through the `report-client-error` Edge Function rather than + * inserting directly. The table used to grant INSERT to anon, which made the + * caps below the only thing standing between the public anon key and unbounded + * writes — and they stand on the wrong side of the network. The real limit is + * now a per-address and per-hour quota enforced in the database. + * + * The caps here stay because they are still worth having on this side: they + * keep a render loop from firing a hundred requests that the server would only + * throw away. * - production only (dev errors stay in the console); * - per-page-load cap of MAX_REPORTS; * - dedupe by message so a render loop reports once; @@ -45,13 +53,16 @@ export function reportClientError({message, stack, context}: ClientErrorReport): void import('./supabaseClient') .then(({supabase}) => - supabase.from('client_error_log').insert({ - message: truncate(message, 1000) ?? 'Unknown error', - stack: truncate(stack, 6000), - source: truncate(window.location.pathname, 300) ?? '/', - context: truncate(context, 200), - user_agent: truncate(navigator.userAgent, 400), - locale: truncate(document.documentElement.lang, 10), + // The user agent is not sent: the Edge Function reads it from the request + // headers, where it cannot be dressed up by the caller. + supabase.functions.invoke('report-client-error', { + body: { + message: truncate(message, 1000) ?? 'Unknown error', + stack: truncate(stack, 6000), + source: truncate(window.location.pathname, 300) ?? '/', + context: truncate(context, 200), + locale: truncate(document.documentElement.lang, 10), + }, }), ) .then(({error}) => { diff --git a/supabase/functions/report-client-error/index.ts b/supabase/functions/report-client-error/index.ts new file mode 100644 index 0000000..6dd3593 --- /dev/null +++ b/supabase/functions/report-client-error/index.ts @@ -0,0 +1,160 @@ +// Client-side error intake (follow-up to issue #98). +// +// The browser used to INSERT into `client_error_log` directly with the anon +// key. That key is public — it ships in the JS bundle — so the per-page-load +// cap and dedupe in src/lib/errorReporting.ts bounded only well-behaved +// clients; anything else could POST unbounded ~8 KB rows to PostgREST. The +// table no longer grants INSERT to anon, and reports come through here instead. +// +// This function runs under the service role and calls record_client_error(), +// which enforces the quota in the same transaction as the insert. The quota is +// keyed on a salted hash of the caller's address: enough to count against, +// never enough to identify anyone. The error log itself still stores no +// identity of any kind. +// +// The response is always the same, whether the report was stored, throttled or +// malformed — a caller who could tell throttling apart from acceptance could +// calibrate against the limit, and the browser has nothing useful to do with +// the difference either way. +import {createClient} from 'jsr:@supabase/supabase-js@2'; + +const corsHeaders = { + 'Access-Control-Allow-Origin': '*', + 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', + 'Access-Control-Allow-Methods': 'POST, OPTIONS', +}; + +/** Mirrors the check constraints on client_error_log. */ +const LIMITS = { + message: 1000, + stack: 6000, + source: 300, + context: 200, + userAgent: 400, + locale: 10, +} as const; + +/** + * Bodies larger than this are rejected before parsing. The longest legitimate + * report is a little under 8 KB of column data; the rest is JSON overhead. + */ +const MAX_BODY_BYTES = 16_000; + +function clean(value: unknown, max: number): string | null { + if (typeof value !== 'string') return null; + const trimmed = value.trim(); + if (!trimmed) return null; + return trimmed.length > max ? trimmed.slice(0, max) : trimmed; +} + +/** + * Best-effort client address. + * + * `cf-connecting-ip` is written by the edge network and cannot be set by the + * caller, so it is preferred. The leftmost `x-forwarded-for` entry is the + * fallback and *is* forgeable, which only costs an attacker a wider spread + * across quota buckets — the global per-window ceiling in record_client_error() + * is what actually bounds the damage, and it does not depend on this value. + */ +function clientAddress(req: Request): string { + const direct = req.headers.get('cf-connecting-ip'); + if (direct) return direct.trim(); + const forwarded = req.headers.get('x-forwarded-for'); + if (forwarded) { + const first = forwarded.split(',')[0]?.trim(); + if (first) return first; + } + return 'unknown'; +} + +async function hashAddress(address: string, salt: string): Promise { + const digest = await crypto.subtle.digest( + 'SHA-256', + new TextEncoder().encode(`${salt}:${address}`), + ); + return Array.from(new Uint8Array(digest)) + .map((byte) => byte.toString(16).padStart(2, '0')) + .join(''); +} + +Deno.serve(async (req: Request) => { + if (req.method === 'OPTIONS') { + return new Response('ok', {headers: corsHeaders}); + } + + const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, + headers: {...corsHeaders, 'Content-Type': 'application/json'}, + }); + + // Same answer for every outcome below — see the note at the top. + const genericOk = () => json({ok: true}); + + if (req.method !== 'POST') { + return json({ok: false, error: 'method-not-allowed'}, 405); + } + + try { + const declaredLength = Number(req.headers.get('content-length') ?? '0'); + if (declaredLength > MAX_BODY_BYTES) { + return genericOk(); + } + + const raw = await req.text(); + if (raw.length > MAX_BODY_BYTES) { + return genericOk(); + } + + let payload: Record; + try { + payload = JSON.parse(raw) as Record; + } catch { + return genericOk(); + } + + const message = clean(payload.message, LIMITS.message); + if (!message) { + return genericOk(); + } + + const serviceRoleKey = Deno.env.get('SUPABASE_SERVICE_ROLE_KEY') ?? ''; + const supabaseUrl = Deno.env.get('SUPABASE_URL') ?? ''; + // A dedicated salt is preferred, but falling back to the service role key + // keeps the hash unguessable without adding a deploy step that, if + // forgotten, would silently make the digests reversible by dictionary. + const salt = Deno.env.get('ERROR_LOG_IP_SALT') || serviceRoleKey; + if (!salt || !serviceRoleKey || !supabaseUrl) { + console.error('[report-client-error] missing environment configuration'); + return genericOk(); + } + + const ipHash = await hashAddress(clientAddress(req), salt); + + const admin = createClient(supabaseUrl, serviceRoleKey, { + auth: {persistSession: false, autoRefreshToken: false}, + }); + + // The user agent comes from the request headers rather than the body: it is + // the same value the browser would have sent, and taking it here is one + // less caller-controlled string to size-check. + const {error} = await admin.rpc('record_client_error', { + p_ip_hash: ipHash, + p_message: message, + p_stack: clean(payload.stack, LIMITS.stack), + p_source: clean(payload.source, LIMITS.source), + p_context: clean(payload.context, LIMITS.context), + p_user_agent: clean(req.headers.get('user-agent'), LIMITS.userAgent), + p_locale: clean(payload.locale, LIMITS.locale), + }); + + if (error) { + console.error('[report-client-error] record failed', error.message); + } + + return genericOk(); + } catch (err) { + console.error('[report-client-error] unexpected error', err); + return genericOk(); + } +}); diff --git a/supabase/migrations/20260816000000_harden_client_error_log.sql b/supabase/migrations/20260816000000_harden_client_error_log.sql new file mode 100644 index 0000000..64a2687 --- /dev/null +++ b/supabase/migrations/20260816000000_harden_client_error_log.sql @@ -0,0 +1,148 @@ +-- Close the anonymous write path into client_error_log (follow-up to issue #98). +-- +-- The table accepted INSERTs from anon so the browser could post its own +-- crashes. What made that look bounded -- a per-page-load cap and dedupe by +-- message -- lives in src/lib/errorReporting.ts, i.e. entirely on the reporting +-- side. The anon key ships inside the JS bundle, so anything could POST +-- straight to PostgREST and write unbounded rows of nearly 8 KB each, and the +-- client-side caps would never see the traffic. +-- +-- Reports now arrive through the report-client-error Edge Function, which runs +-- under the service role and calls record_client_error() below. The quota lives +-- in the database rather than the function so that concurrent reports cannot +-- race past it, and so it survives the function being redeployed. + +-- --------------------------------------------------------------------------- +-- 1. Remove the direct write path. +-- --------------------------------------------------------------------------- +drop policy if exists "Anyone can report errors" on public.client_error_log; + +-- The original grant was column-level; revoke it in both forms so the outcome +-- does not depend on how the server folds column privileges into table ones. +revoke insert (message, stack, source, context, user_agent, locale) + on public.client_error_log from anon, authenticated; +revoke insert on public.client_error_log from anon, authenticated; + +-- Admin read access (the "Admins can read error log" policy) is unchanged. + +-- --------------------------------------------------------------------------- +-- 2. Rate-limit bookkeeping. +-- --------------------------------------------------------------------------- +-- Deliberately a separate table from the error log: client_error_log stores no +-- identity at all, and that stays true. Nothing here is joinable back to a +-- report -- there is no shared key and no timestamp precise enough to correlate +-- one, and rows are pruned once their window is spent. +create table if not exists public.client_error_report_quota ( + -- SHA-256 of (secret salt || client address). Never the address itself: this + -- exists to bound abuse, not to recognise anyone, so a one-way digest is all + -- the function needs to count against. + ip_hash text primary key check (ip_hash ~ '^[0-9a-f]{64}$'), + window_started_at timestamptz not null default now(), + report_count integer not null default 0 +); + +alter table public.client_error_report_quota enable row level security; + +-- No policies and no grants: only the service role reaches this table, and it +-- bypasses RLS. Enabling RLS anyway means a future accidental grant still +-- denies by default. +revoke all on public.client_error_report_quota from public, anon, authenticated; + +-- --------------------------------------------------------------------------- +-- 3. Quota-checked insert. +-- --------------------------------------------------------------------------- +create or replace function public.record_client_error( + p_ip_hash text, + p_message text, + p_stack text, + p_source text, + p_context text, + p_user_agent text, + p_locale text +) +returns boolean +language plpgsql +security definer +set search_path to 'public' +as $$ +declare + window_length constant interval := interval '1 hour'; + -- One address may report this many times per window. A browser that is + -- genuinely broken sends at most MAX_REPORTS (10) per page load, so this + -- leaves room for a few reloads before anything is dropped. + max_per_ip constant integer := 30; + -- ...and this is the ceiling for the whole table per window, so a flood + -- spread over many addresses -- or one that forges its address header -- is + -- bounded regardless of how well the per-address count holds up. + max_per_window constant integer := 2000; + v_count integer; + v_window_total integer; +begin + if p_ip_hash is null or p_ip_hash !~ '^[0-9a-f]{64}$' then + return false; + end if; + + if p_message is null or char_length(btrim(p_message)) = 0 then + return false; + end if; + + -- Claim a slot. An expired window is reset in place rather than deleted, so + -- this stays one statement and two simultaneous reports cannot both read a + -- stale count before either writes. + insert into public.client_error_report_quota (ip_hash, window_started_at, report_count) + values (p_ip_hash, now(), 1) + on conflict (ip_hash) do update + set report_count = case + when public.client_error_report_quota.window_started_at < now() - window_length then 1 + else public.client_error_report_quota.report_count + 1 + end, + window_started_at = case + when public.client_error_report_quota.window_started_at < now() - window_length then now() + else public.client_error_report_quota.window_started_at + end + returning report_count into v_count; + + if v_count > max_per_ip then + return false; + end if; + + select count(*) into v_window_total + from public.client_error_log + where created_at > now() - window_length; + + if v_window_total >= max_per_window then + return false; + end if; + + -- Truncate rather than reject: a report that is 20 bytes over the column + -- limit is still worth having, and the caller cannot fix it. Empty strings + -- become NULL so "no stack" reads the same however the caller spelled it. + insert into public.client_error_log (message, stack, source, context, user_agent, locale) + values ( + left(p_message, 1000), + nullif(left(p_stack, 6000), ''), + coalesce(nullif(left(p_source, 300), ''), '/'), + nullif(left(p_context, 200), ''), + nullif(left(p_user_agent, 400), ''), + nullif(left(p_locale, 10), '') + ); + + -- Opportunistic prune, so the quota table does not accumulate one row per + -- address seen forever. Two windows of slack keeps it clear of the reset + -- logic above. + if random() < 0.02 then + delete from public.client_error_report_quota + where window_started_at < now() - (window_length * 2); + end if; + + return true; +end; +$$; + +comment on function public.record_client_error(text, text, text, text, text, text, text) is + 'Quota-checked insert into client_error_log. Called only by the report-client-error Edge Function under the service role; anon lost its direct INSERT in this migration (issue #98 follow-up).'; + +revoke execute on function public.record_client_error(text, text, text, text, text, text, text) + from public, anon, authenticated; +grant execute on function public.record_client_error(text, text, text, text, text, text, text) + to service_role;