From ece1f104a765c194d9e1065348b1525bba22cb19 Mon Sep 17 00:00:00 2001 From: biosxxx Date: Sun, 16 Aug 2026 13:12:01 +0300 Subject: [PATCH] Route client error reports through an Edge Function with a real quota MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit client_error_log granted INSERT to anon so the browser could post its own crashes. The caps that made that look bounded — ten reports per page load, dedupe by message — live in src/lib/errorReporting.ts, on the reporting side of the network. The anon key ships in the JS bundle, so anything could POST straight to PostgREST and write unbounded rows of nearly 8 KB, and those caps would never see the traffic. Reports now go through the report-client-error Edge Function, which runs under the service role and calls record_client_error(). anon and authenticated lose INSERT entirely. The quota lives in the database, not the function: the slot is claimed by the same statement that increments it, so concurrent reports cannot race past it, and it survives a redeploy. Two limits — 30 per address per hour, and 2000 rows per hour across the table. The second is not symmetry. The address header can be forged, which spreads a flood across buckets; the global ceiling does not depend on that value and bounds the worst case regardless. The quota table is separate from the log on purpose. client_error_log stores no identity and that stays true: the quota holds only a salted SHA-256 of the address, there is no shared key between the tables, and rows are pruned once their window is spent. The user agent now comes from the request headers rather than the body, and the function answers identically whether a report was stored, throttled or malformed — a caller who could tell those apart could calibrate against the limit. Verified against a throwaway Postgres 17: the migration is idempotent, anon retains neither table nor column INSERT privileges, 35 reports from one address store exactly 30, a second address is unaffected, an expired window resets the counter, the 2001st row in an hour is refused even to a fresh address, and fields truncate to the column limits. typecheck, lint and a six-locale build pass. Deploy order matters: function first, then the site, then the migration. Applying the migration ahead of the others silently drops reports in between. Co-Authored-By: Claude Opus 5 --- src/lib/errorReporting.ts | 27 ++- .../functions/report-client-error/index.ts | 160 ++++++++++++++++++ ...20260816000000_harden_client_error_log.sql | 148 ++++++++++++++++ 3 files changed, 327 insertions(+), 8 deletions(-) create mode 100644 supabase/functions/report-client-error/index.ts create mode 100644 supabase/migrations/20260816000000_harden_client_error_log.sql 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;