Close the anonymous write path into client_error_log

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}) => {

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<string> {
+  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<string, unknown>;
+    try {
+      payload = JSON.parse(raw) as Record<string, unknown>;
+    } 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();
+  }
+});

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;