From 7ab244bab41135ad4e160263aca56b9c25de0285 Mon Sep 17 00:00:00 2001 From: sacha <23283108+sacha-l@users.noreply.github.com> Date: Wed, 17 Jun 2026 00:02:33 +0200 Subject: [PATCH] security: enable RLS on public tables + fix cross-program IDOR in app status update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1) RLS: the Supabase Data API exposed every public table to the anon key (shipped in the client bundle) for read AND write — verified anon could SELECT/DELETE/UPDATE projects, program_admins, multisig_*. Enable RLS (deny-all) on all public tables; server uses service_role (BYPASSRLS) and the client only uses Supabase auth, so zero functional impact. Already applied to prod; committing so repo history matches. 2) IDOR: updateApplicationStatus updated by applicationId without verifying it belongs to the program in the slug, letting a program-A admin mutate program-B applications. Now fetch + assert programId match before any write. 429 server tests pass. --- .../__tests__/program-application.test.js | 39 ++++++++++++------- server/api/controllers/program.controller.js | 15 ++++--- ...0260616000000_enable_rls_public_tables.sql | 28 +++++++++++++ 3 files changed, 61 insertions(+), 21 deletions(-) create mode 100644 supabase/migrations/20260616000000_enable_rls_public_tables.sql diff --git a/server/api/controllers/__tests__/program-application.test.js b/server/api/controllers/__tests__/program-application.test.js index f71ad31..ae723ac 100644 --- a/server/api/controllers/__tests__/program-application.test.js +++ b/server/api/controllers/__tests__/program-application.test.js @@ -221,7 +221,7 @@ describe('ProgramController.updateApplicationStatus', () => { it('updates on valid payload', async () => { programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding' }); - applicationService.getById.mockResolvedValue({ id: 'a1', status: 'submitted' }); + applicationService.getById.mockResolvedValue({ id: 'a1', programId: 'p1', status: 'submitted' }); applicationService.updateStatus.mockResolvedValue({ id: 'a1', status: 'accepted', projectId: 'proj-1' }); notificationService.notifyProjectTeam.mockResolvedValue([]); const req = { @@ -242,7 +242,7 @@ describe('ProgramController.updateApplicationStatus', () => { it('maps PostgREST no-rows error to 404', async () => { programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding' }); - applicationService.getById.mockResolvedValue({ id: 'missing', status: 'submitted' }); + applicationService.getById.mockResolvedValue({ id: 'missing', programId: 'p1', status: 'submitted' }); const err = Object.assign(new Error('no rows'), { code: 'PGRST116' }); applicationService.updateStatus.mockRejectedValue(err); const req = { @@ -257,7 +257,7 @@ describe('ProgramController.updateApplicationStatus', () => { it('calls notifyProjectTeam with application_accepted when submitted → accepted', async () => { programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding 2026' }); - applicationService.getById.mockResolvedValue({ id: 'app-1', status: 'submitted' }); + applicationService.getById.mockResolvedValue({ id: 'app-1', programId: 'p1', status: 'submitted' }); applicationService.updateStatus.mockResolvedValue({ id: 'app-1', status: 'accepted', projectId: 'proj-1' }); notificationService.notifyProjectTeam.mockResolvedValue([]); const req = { @@ -278,7 +278,7 @@ describe('ProgramController.updateApplicationStatus', () => { it('calls notifyProjectTeam with application_rejected when submitted → rejected', async () => { programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding 2026' }); - applicationService.getById.mockResolvedValue({ id: 'app-2', status: 'submitted' }); + applicationService.getById.mockResolvedValue({ id: 'app-2', programId: 'p1', status: 'submitted' }); applicationService.updateStatus.mockResolvedValue({ id: 'app-2', status: 'rejected', projectId: 'proj-2' }); notificationService.notifyProjectTeam.mockResolvedValue([]); const req = { @@ -299,7 +299,7 @@ describe('ProgramController.updateApplicationStatus', () => { it('does not call notifyProjectTeam when new status is submitted (no real transition)', async () => { programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding 2026' }); - applicationService.getById.mockResolvedValue({ id: 'app-3', status: 'submitted' }); + applicationService.getById.mockResolvedValue({ id: 'app-3', programId: 'p1', status: 'submitted' }); applicationService.updateStatus.mockResolvedValue({ id: 'app-3', status: 'submitted', projectId: 'proj-3' }); const req = { params: { slug: 'dogfooding-2026', applicationId: 'app-3' }, @@ -314,7 +314,7 @@ describe('ProgramController.updateApplicationStatus', () => { it('still returns 200 even when notifyProjectTeam rejects', async () => { programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding 2026' }); - applicationService.getById.mockResolvedValue({ id: 'app-4', status: 'submitted' }); + applicationService.getById.mockResolvedValue({ id: 'app-4', programId: 'p1', status: 'submitted' }); applicationService.updateStatus.mockResolvedValue({ id: 'app-4', status: 'accepted', projectId: 'proj-4' }); notificationService.notifyProjectTeam.mockRejectedValue(new Error('boom')); const req = { @@ -327,20 +327,33 @@ describe('ProgramController.updateApplicationStatus', () => { expect(res.status).toHaveBeenCalledWith(200); }); - it('still updates and returns 200 when the prior-status lookup (getById) fails', async () => { + it('404s (no write) when the application belongs to a DIFFERENT program (IDOR)', async () => { programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding 2026' }); - applicationService.getById.mockRejectedValue(new Error('supabase transport error')); - applicationService.updateStatus.mockResolvedValue({ id: 'app-5', status: 'accepted', projectId: 'proj-5' }); + // Application exists but is scoped to another program. + applicationService.getById.mockResolvedValue({ id: 'other', programId: 'p2', status: 'submitted' }); const req = { - params: { slug: 'dogfooding-2026', applicationId: 'app-5' }, + params: { slug: 'dogfooding-2026', applicationId: 'other' }, body: { status: 'accepted' }, user: { address: 'admin' }, }; const res = mockRes(); await programController.updateApplicationStatus(req, res); - // The status update still succeeds; getById failure only skips the notify gate. - expect(applicationService.updateStatus).toHaveBeenCalled(); - expect(res.status).toHaveBeenCalledWith(200); + expect(res.status).toHaveBeenCalledWith(404); + expect(applicationService.updateStatus).not.toHaveBeenCalled(); expect(notificationService.notifyProjectTeam).not.toHaveBeenCalled(); }); + + it('404s (no write) when the application does not exist', async () => { + programService.findBySlug.mockResolvedValue({ id: 'p1', name: 'Dogfooding 2026' }); + applicationService.getById.mockResolvedValue(null); + const req = { + params: { slug: 'dogfooding-2026', applicationId: 'ghost' }, + body: { status: 'accepted' }, + user: { address: 'admin' }, + }; + const res = mockRes(); + await programController.updateApplicationStatus(req, res); + expect(res.status).toHaveBeenCalledWith(404); + expect(applicationService.updateStatus).not.toHaveBeenCalled(); + }); }); diff --git a/server/api/controllers/program.controller.js b/server/api/controllers/program.controller.js index d0cecaa..7921e07 100644 --- a/server/api/controllers/program.controller.js +++ b/server/api/controllers/program.controller.js @@ -120,15 +120,14 @@ class ProgramController { return res.status(404).json({ status: 'error', message: 'Program not found' }); } - // Prior-status lookup feeds only the notification gate below — a failure - // here must not break the status update or change the HTTP response. - let prevStatus; - try { - const existing = await programApplicationService.getById(applicationId); - prevStatus = existing?.status; - } catch (err) { - logger.error('Failed to read application prior status for notification gating:', err); + // Cross-check program scoping: an application from program B must not be + // updatable via program A's slug (IDOR). Authoritative — reject before any + // write. The fetched row also feeds the prior-status notification gate. + const existing = await programApplicationService.getById(applicationId); + if (!existing || existing.programId !== program.id) { + return res.status(404).json({ status: 'error', message: 'Application not found' }); } + const prevStatus = existing.status; const reviewedBy = req.user?.address || req.auth?.address || 'unknown'; const updated = await programApplicationService.updateStatus({ diff --git a/supabase/migrations/20260616000000_enable_rls_public_tables.sql b/supabase/migrations/20260616000000_enable_rls_public_tables.sql new file mode 100644 index 0000000..793cd77 --- /dev/null +++ b/supabase/migrations/20260616000000_enable_rls_public_tables.sql @@ -0,0 +1,28 @@ +-- Enable Row Level Security on every public table (deny-by-default). +-- +-- WHY: the Supabase Data API (PostgREST) exposes the `public` schema to the +-- `anon` / `authenticated` roles via the public anon key, which ships in the +-- client bundle. With RLS disabled, that key can read AND write every public +-- table, bypassing the Express API and its SIWS/admin auth. Verified on prod: +-- anon could SELECT and DELETE/UPDATE projects, program_admins, multisig_*. +-- +-- WHY THIS IS SAFE HERE: the browser never queries these tables — the client +-- uses Supabase only for auth (supabase.auth.*), and ALL data access goes +-- through the Express server using the SERVICE_ROLE key, which has BYPASSRLS. +-- So enabling RLS with NO policies denies anon/authenticated while the server +-- is unaffected. If a table ever needs direct anon/authenticated access, add an +-- explicit policy for it. +-- +-- Idempotent: ENABLE on an already-RLS-enabled table is a no-op. + +DO $$ +DECLARE r RECORD; +BEGIN + FOR r IN + SELECT tablename + FROM pg_tables + WHERE schemaname = 'public' + LOOP + EXECUTE format('ALTER TABLE public.%I ENABLE ROW LEVEL SECURITY;', r.tablename); + END LOOP; +END $$;