From 1cd6d3c788470a6112fcd54e8589cfc45506d20d Mon Sep 17 00:00:00 2001 From: Mao Nakamoto Date: Sun, 6 Sep 2026 04:05:49 +0000 Subject: [PATCH] fix(api): stop a failed ownership check from masquerading as 404 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ownsProject/ownsFigure discarded the Supabase error from their select() calls and returned false whenever data came back null — which is also what happens on a real query failure (RLS misconfig, DB outage). Every route's authorization check then reported "Not found" instead of surfacing a server error, same bug class as #40 but one layer down in the shared ownership helper, affecting every POST/PATCH/DELETE (and several GETs) across surfaces, compositions, and figures. ownsProject/ownsFigure now return { owns, error } so callers can tell a real failure (500) apart from "caller doesn't own this" (404). Co-Authored-By: Claude Sonnet 5 --- app/src/app/api/compositions/route.ts | 10 ++++-- app/src/app/api/figures/[id]/route.ts | 10 ++++-- app/src/app/api/figures/route.ts | 10 ++++-- app/src/app/api/surfaces/route.ts | 10 ++++-- app/src/lib/api/ownership.test.ts | 52 ++++++++++++++++++++++----- app/src/lib/api/ownership.ts | 16 +++++---- 6 files changed, 86 insertions(+), 22 deletions(-) diff --git a/app/src/app/api/compositions/route.ts b/app/src/app/api/compositions/route.ts index 3688b99..83411d3 100644 --- a/app/src/app/api/compositions/route.ts +++ b/app/src/app/api/compositions/route.ts @@ -9,7 +9,10 @@ export async function GET(request: NextRequest) { const projectId = request.nextUrl.searchParams.get('project_id'); if (!projectId) return NextResponse.json({ success: false, error: 'project_id required' }, { status: 400 }); - if (!(await ownsProject(supabase, projectId, userId))) { + const ownership = await ownsProject(supabase, projectId, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } @@ -37,7 +40,10 @@ export async function POST(request: NextRequest) { ); } - if (!(await ownsProject(supabase, parsed.data.project_id, userId))) { + const ownership = await ownsProject(supabase, parsed.data.project_id, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } diff --git a/app/src/app/api/figures/[id]/route.ts b/app/src/app/api/figures/[id]/route.ts index 9b48e23..3a30d30 100644 --- a/app/src/app/api/figures/[id]/route.ts +++ b/app/src/app/api/figures/[id]/route.ts @@ -17,7 +17,10 @@ export async function PATCH(request: NextRequest, context: RouteContext) { { status: 400 }, ); } - if (!(await ownsFigure(supabase, id, userId))) { + const ownership = await ownsFigure(supabase, id, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } @@ -36,7 +39,10 @@ export async function DELETE(_request: NextRequest, context: RouteContext) { const { id } = await context.params; const { supabase, userId } = await getApiClient(); - if (!(await ownsFigure(supabase, id, userId))) { + const ownership = await ownsFigure(supabase, id, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } diff --git a/app/src/app/api/figures/route.ts b/app/src/app/api/figures/route.ts index 7678c8e..a8bc449 100644 --- a/app/src/app/api/figures/route.ts +++ b/app/src/app/api/figures/route.ts @@ -9,7 +9,10 @@ export async function GET(request: NextRequest) { const projectId = request.nextUrl.searchParams.get('project_id'); if (!projectId) return NextResponse.json({ success: false, error: 'project_id required' }, { status: 400 }); - if (!(await ownsProject(supabase, projectId, userId))) { + const ownership = await ownsProject(supabase, projectId, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } @@ -34,7 +37,10 @@ export async function POST(request: NextRequest) { { status: 400 }, ); } - if (!(await ownsProject(supabase, parsed.data.project_id, userId))) { + const ownership = await ownsProject(supabase, parsed.data.project_id, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } diff --git a/app/src/app/api/surfaces/route.ts b/app/src/app/api/surfaces/route.ts index 1168ba5..df00243 100644 --- a/app/src/app/api/surfaces/route.ts +++ b/app/src/app/api/surfaces/route.ts @@ -9,7 +9,10 @@ export async function GET(request: NextRequest) { const projectId = request.nextUrl.searchParams.get('project_id'); if (!projectId) return NextResponse.json({ success: false, error: 'project_id required' }, { status: 400 }); - if (!(await ownsProject(supabase, projectId, userId))) { + const ownership = await ownsProject(supabase, projectId, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } @@ -35,7 +38,10 @@ export async function POST(request: NextRequest) { ); } - if (!(await ownsProject(supabase, parsed.data.project_id, userId))) { + const ownership = await ownsProject(supabase, parsed.data.project_id, userId); + if (ownership.error) + return NextResponse.json({ success: false, error: ownership.error }, { status: 500 }); + if (!ownership.owns) { return NextResponse.json({ success: false, error: 'Not found' }, { status: 404 }); } diff --git a/app/src/lib/api/ownership.test.ts b/app/src/lib/api/ownership.test.ts index bd99bc6..1aed7a0 100644 --- a/app/src/lib/api/ownership.test.ts +++ b/app/src/lib/api/ownership.test.ts @@ -12,7 +12,7 @@ import { ownsProject, ownsFigure } from './ownership'; type Row = Record; type SupabaseArg = Parameters[0]; -function fakeSupabase(tables: Record): SupabaseArg { +function fakeSupabase(tables: Record, failTable?: string): SupabaseArg { const client = { from(table: string) { const filters: Row = {}; @@ -23,6 +23,9 @@ function fakeSupabase(tables: Record): SupabaseArg { return builder; }, maybeSingle: async () => { + if (table === failTable) { + return { data: null, error: { message: `${table} query failed` } }; + } const rows = tables[table] ?? []; const match = rows.find((row) => Object.entries(filters).every(([column, value]) => row[column] === value), @@ -46,34 +49,67 @@ const db = { describe('ownsProject', () => { it('accepts the owner', async () => { - expect(await ownsProject(fakeSupabase(db), 'project-a', OWNER)).toBe(true); + expect(await ownsProject(fakeSupabase(db), 'project-a', OWNER)).toEqual({ + owns: true, + error: null, + }); }); it('refuses someone else holding a real project id', async () => { - expect(await ownsProject(fakeSupabase(db), 'project-a', STRANGER)).toBe(false); + expect(await ownsProject(fakeSupabase(db), 'project-a', STRANGER)).toEqual({ + owns: false, + error: null, + }); }); it('refuses an id that does not exist', async () => { - expect(await ownsProject(fakeSupabase(db), 'project-nope', OWNER)).toBe(false); + expect(await ownsProject(fakeSupabase(db), 'project-nope', OWNER)).toEqual({ + owns: false, + error: null, + }); + }); + + it('surfaces a query failure instead of silently reporting not-owned', async () => { + const result = await ownsProject(fakeSupabase(db, 'projects'), 'project-a', OWNER); + expect(result.owns).toBe(false); + expect(result.error).not.toBeNull(); }); }); describe('ownsFigure', () => { it("accepts a figure inside the caller's own project", async () => { - expect(await ownsFigure(fakeSupabase(db), 'figure-a', OWNER)).toBe(true); + expect(await ownsFigure(fakeSupabase(db), 'figure-a', OWNER)).toEqual({ + owns: true, + error: null, + }); }); it("refuses a figure inside somebody else's project", async () => { - expect(await ownsFigure(fakeSupabase(db), 'figure-a', STRANGER)).toBe(false); + expect(await ownsFigure(fakeSupabase(db), 'figure-a', STRANGER)).toEqual({ + owns: false, + error: null, + }); }); it('refuses a figure that does not exist', async () => { - expect(await ownsFigure(fakeSupabase(db), 'figure-nope', OWNER)).toBe(false); + expect(await ownsFigure(fakeSupabase(db), 'figure-nope', OWNER)).toEqual({ + owns: false, + error: null, + }); }); it('refuses an orphaned figure rather than defaulting to allow', async () => { const orphan = { projects: db.projects, figures: [{ id: 'figure-orphan' }] }; - expect(await ownsFigure(fakeSupabase(orphan), 'figure-orphan', OWNER)).toBe(false); + expect(await ownsFigure(fakeSupabase(orphan), 'figure-orphan', OWNER)).toEqual({ + owns: false, + error: null, + }); + }); + + it('surfaces a figure-query failure instead of silently reporting not-owned', async () => { + const result = await ownsFigure(fakeSupabase(db, 'figures'), 'figure-a', OWNER); + expect(result.owns).toBe(false); + expect(result.error).not.toBeNull(); }); }); diff --git a/app/src/lib/api/ownership.ts b/app/src/lib/api/ownership.ts index eb76a04..855ac8d 100644 --- a/app/src/lib/api/ownership.ts +++ b/app/src/lib/api/ownership.ts @@ -15,32 +15,36 @@ import type { getApiClient } from '@/lib/supabase/api-client'; type ApiSupabase = Awaited>['supabase']; +export type OwnershipResult = { owns: boolean; error: string | null }; + export async function ownsProject( supabase: ApiSupabase, projectId: string, userId: string, -): Promise { - const { data } = await supabase +): Promise { + const { data, error } = await supabase .from('projects') .select('id') .eq('id', projectId) .eq('user_id', userId) .maybeSingle(); - return Boolean(data); + if (error) return { owns: false, error: error.message }; + return { owns: Boolean(data), error: null }; } export async function ownsFigure( supabase: ApiSupabase, figureId: string, userId: string, -): Promise { - const { data } = await supabase +): Promise { + const { data, error } = await supabase .from('figures') .select('project_id') .eq('id', figureId) .maybeSingle(); - if (!data?.project_id) return false; + if (error) return { owns: false, error: error.message }; + if (!data?.project_id) return { owns: false, error: null }; return ownsProject(supabase, data.project_id, userId); }