fix(sandbox): repair the silently-failing nightly sandbox cleanup and lock down its RPCs (#1449)

* fix(sandbox): repair the silently-failing nightly sandbox cleanup and lock down its RPCs The daily cleanup cron has deleted nothing for months: cleanup_sandbox_user died on the journal-line immutability trigger for every user (the seed posts vouchers since spring), and cleanup_expired_sandbox_users swallowed each failure as a WARNING while reporting success. 658 expired sandbox users plus 21 orphaned anonymous users had accumulated in prod auth.users. - cleanup_sandbox_user sets the sanctioned gnubok.allow_delete flag plus a new transaction-local gnubok.sandbox_cleanup flag, only after verifying is_sandbox; write_audit_log, audit_log_immutable (DELETE only, per-row sandbox re-check), enforce_dimension_registry_guards (DELETE only) and enforce_pending_operations_no_delete (DELETE only) respect it - clears salary_runs voucher-link FKs and purges the sandbox company's audit rows before the auth.users cascade - cleanup_expired_sandbox_users returns {cleaned, failed, orphans_removed}, additionally sweeps expired anonymous users that never got a company_settings row, and takes an optional p_limit for bounded batches; the cron route logs failures at error level and accepts both return shapes - both RPCs lose their default PUBLIC EXECUTE grant (anon and authenticated could call them via PostgREST) and are now service_role-only - validated by replaying the full delete chain against prod inside aborted transactions (21 users sampled across all seed eras, zero failures) and a committed staging run; pg-real suite + cron route unit tests added Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sandbox): per-row sandbox re-verification in teardown guards, is_anonymous column guard Resolution pass for PR #1449 review findings and the pg-real CI failure: - Swedish accounting review: enforce_dimension_registry_guards and enforce_pending_operations_no_delete now re-verify per row that OLD.company_id belongs to a sandbox company (same pattern as audit_log_immutable) instead of trusting the gnubok.sandbox_cleanup flag alone. Because that re-check needs company_settings to still exist, cleanup_sandbox_user deletes pending_operations and dimensions explicitly before the auth.users cascade. - pg-real CI: auth.users.is_anonymous does not exist in the CI supabase/postgres image (or on older self-hosted stacks); the orphan sweep in cleanup_expired_sandbox_users is now guarded on the column's existence, and the pg test skips the orphan assertions on such stacks. Re-validated on staging end-to-end: {cleaned: 5, failed: 0, orphans_removed: 1}, fresh users and non-sandbox rows untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sandbox): make company_settings.is_sandbox write-once, prove orphan sweep fails loudly Round-2 review findings (Swedish accounting review on PR #1449): - Every teardown bypass trusts company_settings.is_sandbox, and RLS lets an owner update their own settings row via PostgREST, so a real company that flipped the flag would become eligible for full deletion by the nightly cron. New trigger makes the flag write-once (no application path updates it; a future sandbox-to-real conversion would ship its own migration). - New pg test pins the reviewer's remaining concern: an anonymous user who somehow has bookkeeping but no company_settings row is NOT silently deleted by the orphan sweep; the unbypasseed immutability triggers make the deletion fail loudly into the summary's failed count. Validated on staging: flip blocked in both directions, unrelated company_settings updates unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sandbox): guard is_sandbox provenance at INSERT, make orphan sweep exclusions explicit Round-3 review hardening, approved by Emil: - is_sandbox = true can now only be created by an anonymous-user JWT (the sandbox seed's actor), service_role, or a direct database session. A regular authenticated user could previously insert their settings row pre-flagged and have the nightly cron destroy their real books, which BFL 7 kap. forbids even self-inflicted. Claims are read from the request.jwt.* GUCs directly so the check behaves identically on hosted, self-hosted, and the CI auth shim. - The orphan sweep now explicitly excludes anonymous users attached to any companies or company_members row, instead of relying on downstream immutability triggers throwing (emergent safety) to protect half-seeded users. - pg tests updated accordingly: blocked/allowed provenance paths, and the half-seeded user is proven unreachable rather than merely failing loudly. Validated on staging: authed insert blocked, anonymous-claim insert allowed, half-seeded user untouched, sweep summary failed=0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sandbox): all-rows sandbox check, cleared bypass flags, tighter insert guard CodeRabbit review pass on PR #1449 (its first non-rate-limited run): - cleanup_sandbox_user now requires EVERY company_settings row of the user to be sandbox-flagged, not an arbitrary single row: a hypothetical mixed-company user would otherwise have their real company's rows reached by the user-scoped deletes. - Both bypass flags are cleared before the RPC returns, so later work in the same transaction (the expired loop's next iterations, the orphan sweep) never runs with them still armed. - The is_sandbox insert guard now treats ANY PostgREST claims context (claims json without a role claim included) as guarded, instead of falling open when the role claim is absent. - The flag-leak pg test now runs inside an explicit transaction (the old version could not observe transaction-local GUCs at all), and a new test covers the mixed sandbox/real user refusal. Declined: replacing the em dashes inside the two replicated Swedish exception messages; they are byte-identical copies of the strings already deployed by migration 20260702084500 and changing them would alter live user-facing errors out of scope. Validated on staging: mixed user refused, flags cleared post-teardown, role-less claims blocked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-07 13:51:01 +02:00
parent 6458180bf5
commit f7f3a31f8e
5 changed files with 1052 additions and 3 deletions
@@ -0,0 +1,111 @@
+/**
+ * Tests for the sandbox cleanup cron route: the RPC's jsonb summary
+ * ({cleaned, failed, orphans_removed}, migration 20260807130000) is passed
+ * through, the legacy bare-integer shape is still accepted (deploy/migration
+ * ordering), and per-user failures are logged at error level: the failure
+ * mode this fixes was months of silently swallowed cleanup errors.
+ */
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+
+const h = vi.hoisted(() => ({
+  logInfo: vi.fn(),
+  logError: vi.fn(),
+  rpc: vi.fn(),
+}))
+
+vi.mock('@/lib/api/with-cron-context', () => ({
+  withCronContext:
+    (_name: string, handler: (req: Request, ctx: unknown) => Promise<Response>) =>
+    (req: Request) =>
+      handler(req, {
+        log: { info: h.logInfo, error: h.logError, warn: vi.fn() },
+        requestId: 'req_test',
+      }),
+}))
+
+vi.mock('@supabase/supabase-js', () => ({
+  createClient: vi.fn(() => ({ rpc: h.rpc })),
+}))
+
+import { GET } from '../route'
+
+function cronRequest(): Request {
+  return new Request('http://localhost:3000/api/sandbox/cleanup/cron')
+}
+
+describe('GET /api/sandbox/cleanup/cron', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    process.env.NEXT_PUBLIC_SUPABASE_URL = 'https://example.supabase.co'
+    process.env.SUPABASE_SERVICE_ROLE_KEY = 'service-role-key'
+  })
+
+  it('passes the jsonb summary through and logs at info level when nothing failed', async () => {
+    h.rpc.mockResolvedValue({
+      data: { cleaned: 3, failed: 0, orphans_removed: 2 },
+      error: null,
+    })
+
+    const res = await GET(cronRequest())
+    const body = await res.json()
+
+    expect(h.rpc).toHaveBeenCalledWith('cleanup_expired_sandbox_users', {
+      p_max_age_hours: 24,
+    })
+    expect(res.status).toBe(200)
+    expect(body).toEqual({ success: true, cleaned: 3, failed: 0, orphans_removed: 2 })
+    expect(h.logInfo).toHaveBeenCalled()
+    expect(h.logError).not.toHaveBeenCalled()
+  })
+
+  it('logs at error level when the summary reports failures', async () => {
+    h.rpc.mockResolvedValue({
+      data: { cleaned: 1, failed: 4, orphans_removed: 0 },
+      error: null,
+    })
+
+    const res = await GET(cronRequest())
+    const body = await res.json()
+
+    expect(res.status).toBe(200)
+    expect(body.failed).toBe(4)
+    expect(h.logError).toHaveBeenCalledWith(
+      'sandbox cleanup completed with failures',
+      expect.objectContaining({ failed: 4 }),
+    )
+  })
+
+  it('still accepts the legacy bare-integer return shape', async () => {
+    h.rpc.mockResolvedValue({ data: 5, error: null })
+
+    const res = await GET(cronRequest())
+    const body = await res.json()
+
+    expect(res.status).toBe(200)
+    expect(body).toEqual({ success: true, cleaned: 5, failed: 0, orphans_removed: 0 })
+  })
+
+  it('returns an error envelope when the RPC fails', async () => {
+    h.rpc.mockResolvedValue({
+      data: null,
+      error: { message: 'boom', code: 'XX000' },
+    })
+
+    const res = await GET(cronRequest())
+    const body = await res.json()
+
+    expect(res.status).toBeGreaterThanOrEqual(400)
+    expect(body.error).toBeDefined()
+    expect(h.logError).toHaveBeenCalled()
+  })
+
+  it('returns an error when Supabase configuration is missing', async () => {
+    delete process.env.SUPABASE_SERVICE_ROLE_KEY
+
+    const res = await GET(cronRequest())
+    const body = await res.json()
+
+    expect(res.status).toBeGreaterThanOrEqual(500)
+    expect(body.error).toBeDefined()
+  })
+})
@@ -29,8 +29,25 @@ export const GET = withCronContext('cron.sandbox_cleanup', async (_request, ctx)
    return errorResponse(error, ctx.log, { requestId: ctx.requestId })
  }

-  const cleaned = data ?? 0
-  ctx.log.info('sandbox cleanup summary', { cleaned })
+  // Migration 20260807130000 changed the RPC's return from a bare integer to
+  // a {cleaned, failed, orphans_removed} summary; accept both shapes so
+  // deploy/migration ordering cannot break the cron.
+  const summary =
+    typeof data === 'number'
+      ? { cleaned: data, failed: 0, orphans_removed: 0 }
+      : {
+          cleaned: Number(data?.cleaned ?? 0),
+          failed: Number(data?.failed ?? 0),
+          orphans_removed: Number(data?.orphans_removed ?? 0),
+        }

-  return NextResponse.json({ success: true, cleaned })
+  // Per-user failures used to be swallowed as Postgres WARNINGs, which is how
+  // the cleanup sat broken for months; surface them at error level instead.
+  if (summary.failed > 0) {
+    ctx.log.error('sandbox cleanup completed with failures', summary)
+  } else {
+    ctx.log.info('sandbox cleanup summary', summary)
+  }
+
+  return NextResponse.json({ success: true, ...summary })
 })