diff --git a/supabase/functions/_backend/files/files.ts b/supabase/functions/_backend/files/files.ts index 7374ecec74..c2bdb10133 100644 --- a/supabase/functions/_backend/files/files.ts +++ b/supabase/functions/_backend/files/files.ts @@ -317,6 +317,61 @@ async function saveBandwidthUsage(c: Context, fileSize: number | null | undefine } } +async function assertReadableAppScopedAttachment(c: Context, fileId: unknown): Promise { + const scopedPath = parseAppScopedAttachmentPath(fileId) + if (scopedPath?.kind === 'invalid_scoped') { + quickError(404, 'not_found', 'Not found') + } + if (!scopedPath) { + return + } + + // Attachment reads must use the primary to avoid replica lag serving deleted-app files. + const pgClient = getPgClient(c, false) + const drizzleClient = getDrizzleClient(pgClient) + + try { + const app = await getAppByAppIdPg(c, scopedPath.app_id, drizzleClient) + if (!app || app.owner_org !== scopedPath.owner_org) { + quickError(404, 'not_found', 'Not found') + } + + const deletedBundlePath = await pgClient.query<{ id: number }>( + ` + SELECT id + FROM public.app_versions + WHERE owner_org = $1 + AND app_id = $2 + AND deleted = true + AND ( + r2_path = $3 + OR EXISTS ( + SELECT 1 + FROM public.manifest m + WHERE m.app_version_id = public.app_versions.id + AND m.s3_path = $3 + ) + OR EXISTS ( + SELECT 1 + FROM public.deleted_app_version_manifest_paths dmp + WHERE dmp.app_version_id = public.app_versions.id + AND dmp.s3_path = $3 + ) + ) + LIMIT 1 + `, + [scopedPath.owner_org, scopedPath.app_id, fileId], + ) + + if (deletedBundlePath.rows.length > 0) { + quickError(404, 'not_found', 'Not found') + } + } + finally { + await closeClient(c, pgClient) + } +} + async function getSupabaseStorageResponse(c: Context, fileId: string): Promise { const { data: signedUrlData, error: signedUrlError } = await supabaseAdmin(c).storage.from('capgo').createSignedUrl(fileId, 60) @@ -376,9 +431,9 @@ async function getSupabaseStorageResponse(c: Context, fileId: string): Promise { const fileId = c.get('fileId') - // It is imperative that files are read without any database read to avoid bottlenecks and keep file downloads highly available, especially under heavy load. - // This was designed that way, and access to a file that is going to be deleted is not important compared to download availability. - // Do not add DB or R2 checks before serving the file; if the file is missing in R2, a 404 is expected. + await assertReadableAppScopedAttachment(c, fileId) + // Keep normal reads cache-first after the deletion guard. The guard must run + // before cache/R2 so stale deleted app-scoped attachments cannot be restored. cloudlog({ requestId: c.get('requestId'), message: 'getHandler files', fileId }) if (getRuntimeKey() !== 'workerd') { diff --git a/supabase/functions/_backend/private/download_link.ts b/supabase/functions/_backend/private/download_link.ts index 82642206f9..81ecc17720 100644 --- a/supabase/functions/_backend/private/download_link.ts +++ b/supabase/functions/_backend/private/download_link.ts @@ -44,6 +44,7 @@ app.post('/', middlewareAuth, async (c) => { .select('*, owner_org ( created_by )') .eq('app_id', body.app_id) .eq('id', body.id) + .eq('deleted', false) .single() const ownerOrg = bundle?.owner_org.created_by diff --git a/supabase/functions/_backend/triggers/on_version_update.ts b/supabase/functions/_backend/triggers/on_version_update.ts index aa6999f71e..8f9910057c 100644 --- a/supabase/functions/_backend/triggers/on_version_update.ts +++ b/supabase/functions/_backend/triggers/on_version_update.ts @@ -208,6 +208,29 @@ async function deleteManifest(c: Context, record: Database['public']['Tables'][' if (manifestEntries && manifestEntries.length > 0) { const manifestCount = manifestEntries.length + const ownerOrg = await resolveOwnerOrg(c, record) + if (ownerOrg) { + const tombstonePgClient = getPgClient(c, false) + try { + await tombstonePgClient.query( + ` + INSERT INTO public.deleted_app_version_manifest_paths + (app_version_id, owner_org, app_id, s3_path) + SELECT $1, $2, $3, m.s3_path + FROM public.manifest m + WHERE m.app_version_id = $1 + ON CONFLICT (app_version_id, s3_path) DO NOTHING + `, + [record.id, ownerOrg, record.app_id], + ) + } + catch (error) { + cloudlog({ requestId: c.get('requestId'), message: 'error storing deleted manifest path tombstones', error, id: record.id }) + } + finally { + await closeClient(c, tombstonePgClient) + } + } // Delete each file from S3 const promisesDeleteS3 = [] diff --git a/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql b/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql new file mode 100644 index 0000000000..057690eec0 --- /dev/null +++ b/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql @@ -0,0 +1,18 @@ +CREATE INDEX IF NOT EXISTS "idx_app_versions_deleted_r2_path" +ON "public"."app_versions" USING "btree" ("owner_org", "app_id", "r2_path") +WHERE ("deleted" = true); + +CREATE INDEX IF NOT EXISTS "idx_manifest_app_version_id_s3_path" +ON "public"."manifest" USING "btree" ("app_version_id", "s3_path"); + +CREATE TABLE IF NOT EXISTS "public"."deleted_app_version_manifest_paths" ( + "app_version_id" bigint NOT NULL REFERENCES "public"."app_versions" ("id") ON DELETE CASCADE, + "owner_org" uuid NOT NULL, + "app_id" character varying NOT NULL, + "s3_path" character varying NOT NULL, + "created_at" timestamp with time zone DEFAULT now() NOT NULL, + PRIMARY KEY ("app_version_id", "s3_path") +); + +CREATE INDEX IF NOT EXISTS "idx_deleted_app_version_manifest_paths_lookup" +ON "public"."deleted_app_version_manifest_paths" USING "btree" ("owner_org", "app_id", "s3_path"); diff --git a/tests/download-link-deleted-bundle.unit.test.ts b/tests/download-link-deleted-bundle.unit.test.ts new file mode 100644 index 0000000000..003ec8af18 --- /dev/null +++ b/tests/download-link-deleted-bundle.unit.test.ts @@ -0,0 +1,88 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createAllCatch, createHono } from '../supabase/functions/_backend/utils/hono.ts' +import { version } from '../supabase/functions/_backend/utils/version.ts' + +const eqMock = vi.fn() +const singleMock = vi.fn() +const getBundleUrlMock = vi.fn() + +vi.mock('../supabase/functions/_backend/utils/discord.ts', () => ({ + sendDiscordAlert500: () => Promise.resolve(), + sendDiscordAlert: () => Promise.resolve(), +})) + +vi.mock('../supabase/functions/_backend/utils/downloadUrl.ts', () => ({ + getBundleUrl: getBundleUrlMock, + getManifestUrl: vi.fn(() => []), +})) + +vi.mock('../supabase/functions/_backend/utils/rbac.ts', () => ({ + checkPermission: vi.fn(() => Promise.resolve(true)), +})) + +vi.mock('../supabase/functions/_backend/utils/supabase.ts', () => ({ + supabaseClient: vi.fn(() => ({ + from: vi.fn(() => ({ + select: vi.fn(() => ({ + eq: eqMock, + })), + })), + })), +})) + +vi.mock('../supabase/functions/_backend/utils/hono.ts', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + middlewareAuth: async (c: any, next: () => Promise) => { + c.set('authorization', 'Bearer test-token') + c.set('auth', { userId: 'test-user' }) + await next() + }, + useCors: async (_c: any, next: () => Promise) => { + await next() + }, + } +}) + +describe('private download_link deleted bundle guard', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('filters deleted bundles before creating download URLs', async () => { + const query = { + eq: eqMock, + single: singleMock, + } + eqMock.mockReturnValue(query) + singleMock.mockResolvedValue({ + data: null, + error: { message: 'no rows' }, + }) + + const { app: downloadLink } = await import('../supabase/functions/_backend/private/download_link.ts') + const appGlobal = createHono('private', version) + appGlobal.route('/download_link', downloadLink) + createAllCatch(appGlobal, 'private') + + const response = await appGlobal.fetch( + new Request('http://localhost/download_link', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + app_id: 'test-app', + id: 123, + storage_provider: 'r2', + }), + }), + ) + + const body = await response.json() as { error: string } + + expect(response.status).toBe(400) + expect(body.error).toBe('cannot_get_bundle') + expect(eqMock).toHaveBeenCalledWith('deleted', false) + expect(getBundleUrlMock).not.toHaveBeenCalled() + }) +}) diff --git a/tests/files-app-read-guard.unit.test.ts b/tests/files-app-read-guard.unit.test.ts index 50918a1650..d9f8b029c4 100644 --- a/tests/files-app-read-guard.unit.test.ts +++ b/tests/files-app-read-guard.unit.test.ts @@ -1,12 +1,11 @@ -import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' -const getAppByAppIdPgMock = vi.fn(async () => null) -const getPgClientMock = vi.fn(() => ({})) +const getAppByAppIdPgMock = vi.fn() +const pgClientMock = { + query: vi.fn(), +} +const getPgClientMock = vi.fn(() => pgClientMock) const originalCaches = globalThis.caches -const cachedBodiesByPath = new Map([ - ['/read/attachments/orgs/test-org/apps/test-app/orphan.txt', 'cached orphan bytes'], - ['/read/attachments/orgs/test-org/apps/test-app/', 'cached malformed bytes'], -]) vi.mock('hono/adapter', async (importOriginal) => { const actual = await importOriginal() @@ -44,34 +43,36 @@ async function createFilesApp() { return appGlobal } -describe('files app-scoped cached reads', () => { - beforeAll(() => { - vi.clearAllMocks() - globalThis.caches = { - default: { - match: async (request: Request) => { - const pathname = new URL(request.url).pathname - const body = cachedBodiesByPath.get(pathname) - if (body == null) - return null - - return new Response(body, { - headers: { - 'content-type': 'text/plain', - }, - }) +function installCache(body: string, contentType = 'text/plain') { + globalThis.caches = { + default: { + match: async () => new Response(body, { + headers: { + 'content-type': contentType, }, - put: async () => { }, - }, - } as any + }), + put: async () => { }, + }, + } as any +} + +describe('files app-scoped read guard', () => { + beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + pgClientMock.query.mockResolvedValue({ rows: [] }) }) afterAll(() => { globalThis.caches = originalCaches }) - it.concurrent('serves deleted app-scoped files from cache without a database lookup', async () => { + it('returns 404 for deleted app-scoped files before serving cached content', async () => { + getAppByAppIdPgMock.mockResolvedValue(null) + const bucketPut = vi.fn() + installCache('cached orphan bytes') + const appGlobal = await createFilesApp() const response = await appGlobal.fetch( @@ -82,15 +83,15 @@ describe('files app-scoped cached reads', () => { { waitUntil: () => { } } as any, ) - expect(response.status).toBe(200) - expect(await response.text()).toBe('cached orphan bytes') + expect(response.status).toBe(404) expect(bucketPut).not.toHaveBeenCalled() - expect(getPgClientMock).not.toHaveBeenCalled() - expect(getAppByAppIdPgMock).not.toHaveBeenCalled() + expect(getPgClientMock).toHaveBeenCalledWith(expect.anything(), false) }) - it.concurrent('serves malformed app-scoped paths from cache without a database lookup', async () => { + it('returns 404 for malformed app-scoped paths before serving cached content', async () => { const bucketPut = vi.fn() + installCache('cached malformed bytes') + const appGlobal = await createFilesApp() const response = await appGlobal.fetch( @@ -101,11 +102,114 @@ describe('files app-scoped cached reads', () => { { waitUntil: () => { } } as any, ) - expect(response.status).toBe(200) - expect(await response.text()).toBe('cached malformed bytes') + expect(response.status).toBe(404) expect(bucketPut).not.toHaveBeenCalled() - expect(getPgClientMock).not.toHaveBeenCalled() expect(getAppByAppIdPgMock).not.toHaveBeenCalled() - expect(getPgClientMock).not.toHaveBeenCalled() + }) + + it('returns 404 for soft-deleted bundle paths before serving cached content', async () => { + getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) + pgClientMock.query.mockResolvedValue({ rows: [{ id: 123 }] }) + + const bucketPut = vi.fn() + installCache('cached deleted bundle bytes', 'application/zip') + + const appGlobal = await createFilesApp() + + const filePath = 'orgs/test-org/apps/test-app/1.0.0.zip' + const response = await appGlobal.fetch( + new Request(`http://localhost/read/attachments/${filePath}`), + { + ATTACHMENT_BUCKET: { put: bucketPut }, + }, + { waitUntil: () => { } } as any, + ) + + expect(response.status).toBe(404) + expect(pgClientMock.query).toHaveBeenCalledWith( + expect.stringContaining('FROM public.app_versions'), + ['test-org', 'test-app', filePath], + ) + expect(bucketPut).not.toHaveBeenCalled() + }) + + it('returns 404 for soft-deleted persisted manifest asset paths before serving cached content', async () => { + getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) + pgClientMock.query.mockResolvedValue({ rows: [{ id: 123 }] }) + + const bucketPut = vi.fn() + installCache('cached deleted manifest bytes', 'text/javascript') + + const appGlobal = await createFilesApp() + + const filePath = 'orgs/test-org/apps/test-app/assets/main.js' + const response = await appGlobal.fetch( + new Request(new URL(filePath, 'http://localhost/read/attachments/')), + { + ATTACHMENT_BUCKET: { put: bucketPut }, + }, + { waitUntil: () => { } } as any, + ) + + expect(response.status).toBe(404) + expect(pgClientMock.query).toHaveBeenCalledWith( + expect.stringContaining('FROM public.manifest'), + ['test-org', 'test-app', filePath], + ) + expect(pgClientMock.query.mock.calls[0]?.[0]).not.toContain('unnest(manifest)') + expect(bucketPut).not.toHaveBeenCalled() + }) + + it('checks deleted manifest path tombstones after manifest rows are cleaned up', async () => { + getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) + pgClientMock.query.mockResolvedValue({ rows: [{ id: 123 }] }) + + const bucketPut = vi.fn() + installCache('cached deleted delta bytes', 'application/octet-stream') + + const appGlobal = await createFilesApp() + + const filePath = 'orgs/test-org/apps/test-app/delta/asset.dat' + const response = await appGlobal.fetch( + new Request(new URL(filePath, 'http://localhost/read/attachments/')), + { + ATTACHMENT_BUCKET: { put: bucketPut }, + }, + { waitUntil: () => { } } as any, + ) + + expect(response.status).toBe(404) + expect(pgClientMock.query).toHaveBeenCalledWith( + expect.stringContaining('deleted_app_version_manifest_paths'), + ['test-org', 'test-app', filePath], + ) + expect(bucketPut).not.toHaveBeenCalled() + }) + + it('serves cached content for non-deleted bundle paths', async () => { + getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) + pgClientMock.query.mockResolvedValue({ rows: [] }) + + const bucketPut = vi.fn() + installCache('cached bundle bytes', 'application/zip') + + const appGlobal = await createFilesApp() + + const filePath = 'orgs/test-org/apps/test-app/1.0.0.zip' + const response = await appGlobal.fetch( + new Request(`http://localhost/read/attachments/${filePath}`), + { + ATTACHMENT_BUCKET: { put: bucketPut }, + }, + { waitUntil: () => { } } as any, + ) + + expect(response.status).toBe(200) + expect(await response.text()).toBe('cached bundle bytes') + expect(pgClientMock.query).toHaveBeenCalledWith( + expect.stringContaining('FROM public.app_versions'), + ['test-org', 'test-app', filePath], + ) + expect(bucketPut).not.toHaveBeenCalled() }) }) diff --git a/tests/files-local-read-proxy.unit.test.ts b/tests/files-local-read-proxy.unit.test.ts index c84904c09e..e6ee11ec0e 100644 --- a/tests/files-local-read-proxy.unit.test.ts +++ b/tests/files-local-read-proxy.unit.test.ts @@ -2,7 +2,10 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' const createSignedUrlMock = vi.fn() const getAppByAppIdPgMock = vi.fn() -const getPgClientMock = vi.fn(() => ({})) +const pgClientMock = { + query: vi.fn(), +} +const getPgClientMock = vi.fn(() => pgClientMock) const storageFromMock = vi.fn(() => ({ createSignedUrl: createSignedUrlMock, })) @@ -46,6 +49,7 @@ describe('files local read proxy', () => { beforeEach(() => { vi.resetModules() vi.clearAllMocks() + pgClientMock.query.mockResolvedValue({ rows: [] }) globalThis.fetch = originalFetch }) @@ -53,7 +57,7 @@ describe('files local read proxy', () => { globalThis.fetch = originalFetch }) - it.concurrent('proxies local storage reads without checking the app in the database', async () => { + it('proxies local storage reads after checking the deleted attachment guard', async () => { getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) createSignedUrlMock.mockResolvedValue({ data: { signedUrl: 'https://storage.example/object?token=test' }, @@ -91,13 +95,18 @@ describe('files local read proxy', () => { expect(await response.text()).toBe('proxied local bytes') expect(response.headers.get('cache-control')).toBe('public, max-age=60, no-transform') expect(response.headers.get('content-disposition')).toBe('attachment; filename="orgs/test-org/apps/test-app/local.txt"') - expect(getPgClientMock).not.toHaveBeenCalled() - expect(getAppByAppIdPgMock).not.toHaveBeenCalled() + expect(getPgClientMock).toHaveBeenCalledWith(expect.anything(), false) + expect(getAppByAppIdPgMock).toHaveBeenCalledWith(expect.anything(), 'test-app', expect.anything()) + expect(pgClientMock.query).toHaveBeenCalledWith( + expect.stringContaining('FROM public.app_versions'), + ['test-org', 'test-app', 'orgs/test-org/apps/test-app/local.txt'], + ) expect(storageFromMock).toHaveBeenCalledWith('capgo') expect(createSignedUrlMock).toHaveBeenCalledWith('orgs/test-org/apps/test-app/local.txt', 60) }) it('preserves HEAD requests without downloading bytes from the signed URL proxy', async () => { + getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) createSignedUrlMock.mockResolvedValue({ data: { signedUrl: 'https://storage.example/object?token=test' }, error: null, @@ -132,7 +141,7 @@ describe('files local read proxy', () => { expect(response.status).toBe(200) expect(response.headers.get('cache-control')).toBe('public, max-age=60, no-transform') expect(response.headers.get('content-disposition')).toBe('attachment; filename="orgs/test-org/apps/test-app/local.txt"') - expect(getPgClientMock).not.toHaveBeenCalled() - expect(getAppByAppIdPgMock).not.toHaveBeenCalled() + expect(getPgClientMock).toHaveBeenCalledWith(expect.anything(), false) + expect(getAppByAppIdPgMock).toHaveBeenCalledWith(expect.anything(), 'test-app', expect.anything()) }) }) diff --git a/tests/on-version-update-cleanup.unit.test.ts b/tests/on-version-update-cleanup.unit.test.ts index 339667782d..b515466aba 100644 --- a/tests/on-version-update-cleanup.unit.test.ts +++ b/tests/on-version-update-cleanup.unit.test.ts @@ -9,12 +9,24 @@ const { deleteObject, getDrizzleClient, getPgClient, + manifestDeleteEq, + manifestEntries, + manifestMaybeSingle, + pgClient, supabaseAdmin, } = vi.hoisted(() => { const appVersionsMetaSelectEq = vi.fn() const appVersionsMetaSelect = vi.fn(() => ({ eq: appVersionsMetaSelectEq })) const appVersionsMetaUpdateEq = vi.fn() const appVersionsMetaUpdate = vi.fn(() => ({ eq: appVersionsMetaUpdateEq })) + const manifestDeleteEq = vi.fn() + const manifestDelete = vi.fn(() => ({ eq: manifestDeleteEq })) + const manifestMaybeSingle = vi.fn() + const manifestLimit = vi.fn(() => ({ maybeSingle: manifestMaybeSingle })) + const manifestSelectEqFileName = vi.fn(() => ({ limit: manifestLimit })) + const manifestSelectEqFileHash = vi.fn(() => ({ eq: manifestSelectEqFileName })) + const manifestSelect = vi.fn(() => ({ eq: manifestSelectEqFileHash })) + const manifestEntries: Array<{ id: number, file_hash: string, file_name: string, s3_path: string }> = [] const supabaseFrom = vi.fn((table: string) => { if (table === 'app_versions_meta') { return { @@ -22,8 +34,17 @@ const { update: appVersionsMetaUpdate, } } + if (table === 'manifest') { + return { + delete: manifestDelete, + select: manifestSelect, + } + } return {} }) + const pgClient = { + query: vi.fn(), + } return { appVersionsMetaSelectEq, @@ -35,11 +56,15 @@ const { getDrizzleClient: vi.fn(() => ({ select: vi.fn(() => ({ from: vi.fn(() => ({ - where: vi.fn(async () => []), + where: vi.fn(async () => manifestEntries), })), })), })), - getPgClient: vi.fn(() => ({})), + getPgClient: vi.fn(() => pgClient), + manifestDeleteEq, + manifestEntries, + manifestMaybeSingle, + pgClient, supabaseAdmin: vi.fn(() => ({ from: supabaseFrom })), supabaseFrom, } @@ -101,6 +126,10 @@ describe('on_version_update deleted version cleanup', () => { single: vi.fn(async () => ({ data: { size: 1234 }, error: null })), }) appVersionsMetaUpdateEq.mockResolvedValue({ error: null }) + manifestEntries.length = 0 + manifestDeleteEq.mockResolvedValue({ error: null }) + manifestMaybeSingle.mockResolvedValue({ data: null, error: null }) + pgClient.query.mockResolvedValue({ rows: [] }) }) it('deletes the bundle directly and clears stored size for soft-deleted versions', async () => { @@ -129,4 +158,26 @@ describe('on_version_update deleted version cleanup', () => { expect(appVersionsMetaUpdate).not.toHaveBeenCalled() expect(createStatsMeta).not.toHaveBeenCalled() }) + + it('stores deleted manifest path tombstones before removing manifest rows', async () => { + manifestEntries.push({ + file_hash: 'hash-main', + file_name: 'main.js', + id: 456, + s3_path: 'orgs/org-1/apps/com.cleanup.test/assets/main.js', + }) + + const response = await deleteIt(createContext(), createVersion({ + owner_org: 'org-1', + r2_path: null, + })) + + expect(response.status).toBe(200) + expect(pgClient.query).toHaveBeenCalledWith( + expect.stringContaining('deleted_app_version_manifest_paths'), + [123, 'org-1', 'com.cleanup.test'], + ) + expect(manifestDeleteEq).toHaveBeenCalledWith('id', 456) + expect(deleteObject).toHaveBeenCalledWith(expect.anything(), 'orgs/org-1/apps/com.cleanup.test/assets/main.js') + }) })