From d5332773a5a2247f490b853925cbe8832e8dfa6a Mon Sep 17 00:00:00 2001 From: moe Date: Sun, 10 May 2026 21:07:50 -0400 Subject: [PATCH 1/7] Block deleted bundle reads from cache Deleted bundle objects can still have a cached response at the edge. Check the app_versions deletion state for the exact app-scoped r2_path before serving cached attachment bytes, so a soft-deleted bundle cannot be read or restored from cache. Co-authored-by: Codex --- supabase/functions/_backend/files/files.ts | 55 +++++++++++- tests/files-app-read-guard.unit.test.ts | 98 ++++++++++++++-------- 2 files changed, 113 insertions(+), 40 deletions(-) diff --git a/supabase/functions/_backend/files/files.ts b/supabase/functions/_backend/files/files.ts index 7374ecec74..eb50321171 100644 --- a/supabase/functions/_backend/files/files.ts +++ b/supabase/functions/_backend/files/files.ts @@ -317,6 +317,55 @@ 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 + ) + ) + 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 +425,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/tests/files-app-read-guard.unit.test.ts b/tests/files-app-read-guard.unit.test.ts index 50918a1650..9f8ef0dbd4 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,34 @@ 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() + }) +} From 0bed171e7e259a947753a8e0af7b5edb07ded45e Mon Sep 17 00:00:00 2001 From: moe Date: Sun, 10 May 2026 21:19:17 -0400 Subject: [PATCH 2/7] Fix local read proxy test mock Keep the local read proxy unit test aligned with the app-scoped attachment guard's database lookup by returning an empty app_versions result from the mocked pg client. Co-authored-by: Codex --- tests/files-local-read-proxy.unit.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/files-local-read-proxy.unit.test.ts b/tests/files-local-read-proxy.unit.test.ts index c84904c09e..3e15bb2546 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 }) From db281f64374cac5d243f026c43eb4ac0f642ad04 Mon Sep 17 00:00:00 2001 From: moe Date: Sun, 10 May 2026 21:28:06 -0400 Subject: [PATCH 3/7] Add deleted bundle path index Create a partial app_versions index for deleted R2 paths and cover the non-deleted cached bundle path in the app-scoped read guard tests. Co-authored-by: Codex --- ...add_app_versions_deleted_r2_path_index.sql | 3 ++ tests/files-app-read-guard.unit.test.ts | 30 +++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql 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..ecc88e2748 --- /dev/null +++ b/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql @@ -0,0 +1,3 @@ +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); diff --git a/tests/files-app-read-guard.unit.test.ts b/tests/files-app-read-guard.unit.test.ts index 9f8ef0dbd4..b667f2ec8f 100644 --- a/tests/files-app-read-guard.unit.test.ts +++ b/tests/files-app-read-guard.unit.test.ts @@ -3,7 +3,7 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' const getAppByAppIdPgMock = vi.fn() const pgClientMock = { query: vi.fn(), -}) +} const getPgClientMock = vi.fn(() => pgClientMock) const originalCaches = globalThis.caches @@ -132,4 +132,30 @@ describe('files app-scoped read guard', () => { ) 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() + }) +}) From c0165ea3a2ac220c2d7e3261413ca8837f3909b4 Mon Sep 17 00:00:00 2001 From: moe Date: Sun, 10 May 2026 21:40:04 -0400 Subject: [PATCH 4/7] Cover deleted manifest bundle reads Extend the deleted bundle guard to manifest asset paths, keep private download links from resolving deleted bundles, and add focused regression coverage for both paths. Co-authored-by: Codex --- .../_backend/private/download_link.ts | 1 + .../download-link-deleted-bundle.unit.test.ts | 87 +++++++++++++++++++ tests/files-app-read-guard.unit.test.ts | 27 ++++++ 3 files changed, 115 insertions(+) create mode 100644 tests/download-link-deleted-bundle.unit.test.ts 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/tests/download-link-deleted-bundle.unit.test.ts b/tests/download-link-deleted-bundle.unit.test.ts new file mode 100644 index 0000000000..8d977afe12 --- /dev/null +++ b/tests/download-link-deleted-bundle.unit.test.ts @@ -0,0 +1,87 @@ +import { 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', () => { + it('filters deleted bundles before creating download URLs', async () => { + vi.resetModules() + vi.clearAllMocks() + + 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 b667f2ec8f..e3715b6a26 100644 --- a/tests/files-app-read-guard.unit.test.ts +++ b/tests/files-app-read-guard.unit.test.ts @@ -132,6 +132,33 @@ describe('files app-scoped read guard', () => { ) expect(bucketPut).not.toHaveBeenCalled() }) + + it('returns 404 for soft-deleted 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(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: [] }) From e2cdda726110c17ed7138d9ec04b8bd0608b3fd9 Mon Sep 17 00:00:00 2001 From: moe Date: Sun, 10 May 2026 21:53:39 -0400 Subject: [PATCH 5/7] Avoid module reset in download link test Keep the deleted-bundle download link regression test scoped to local mocks so it does not reset modules during the wider backend suite. Co-authored-by: Codex --- tests/download-link-deleted-bundle.unit.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/download-link-deleted-bundle.unit.test.ts b/tests/download-link-deleted-bundle.unit.test.ts index 8d977afe12..003ec8af18 100644 --- a/tests/download-link-deleted-bundle.unit.test.ts +++ b/tests/download-link-deleted-bundle.unit.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from 'vitest' +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' @@ -46,10 +46,11 @@ vi.mock('../supabase/functions/_backend/utils/hono.ts', async (importOriginal) = }) describe('private download_link deleted bundle guard', () => { - it('filters deleted bundles before creating download URLs', async () => { - vi.resetModules() + beforeEach(() => { vi.clearAllMocks() + }) + it('filters deleted bundles before creating download URLs', async () => { const query = { eq: eqMock, single: singleMock, From 4bb6dbc8036e944a5f1c27a01eb6369b2e2e42f3 Mon Sep 17 00:00:00 2001 From: moe Date: Sun, 10 May 2026 22:14:53 -0400 Subject: [PATCH 6/7] Use persisted manifest rows in deleted bundle guard Also adds the manifest lookup index and updates the read-guard regression to cover persisted manifest assets.\n\nCo-authored-by: Codex --- .../20260511012637_add_app_versions_deleted_r2_path_index.sql | 3 +++ tests/files-app-read-guard.unit.test.ts | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) 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 index ecc88e2748..0bc53e77d0 100644 --- a/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql +++ b/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql @@ -1,3 +1,6 @@ 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"); diff --git a/tests/files-app-read-guard.unit.test.ts b/tests/files-app-read-guard.unit.test.ts index e3715b6a26..495efd2dd4 100644 --- a/tests/files-app-read-guard.unit.test.ts +++ b/tests/files-app-read-guard.unit.test.ts @@ -133,7 +133,7 @@ describe('files app-scoped read guard', () => { expect(bucketPut).not.toHaveBeenCalled() }) - it('returns 404 for soft-deleted manifest asset paths before serving cached content', async () => { + 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 }] }) @@ -156,6 +156,7 @@ describe('files app-scoped read guard', () => { 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() }) From f64ebc3b7af00444f71037f8b2b35fd1d4f1f591 Mon Sep 17 00:00:00 2001 From: Digz0 <172674408+digzrow-coder@users.noreply.github.com> Date: Fri, 15 May 2026 13:30:14 +0800 Subject: [PATCH 7/7] Persist deleted manifest path tombstones --- supabase/functions/_backend/files/files.ts | 6 ++ .../_backend/triggers/on_version_update.ts | 23 ++++++++ ...add_app_versions_deleted_r2_path_index.sql | 12 ++++ tests/files-app-read-guard.unit.test.ts | 26 +++++++++ tests/files-local-read-proxy.unit.test.ts | 15 +++-- tests/on-version-update-cleanup.unit.test.ts | 55 ++++++++++++++++++- 6 files changed, 130 insertions(+), 7 deletions(-) diff --git a/supabase/functions/_backend/files/files.ts b/supabase/functions/_backend/files/files.ts index eb50321171..c2bdb10133 100644 --- a/supabase/functions/_backend/files/files.ts +++ b/supabase/functions/_backend/files/files.ts @@ -351,6 +351,12 @@ async function assertReadableAppScopedAttachment(c: Context, fileId: unknown): P 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 `, 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 index 0bc53e77d0..057690eec0 100644 --- a/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql +++ b/supabase/migrations/20260511012637_add_app_versions_deleted_r2_path_index.sql @@ -4,3 +4,15 @@ 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/files-app-read-guard.unit.test.ts b/tests/files-app-read-guard.unit.test.ts index 495efd2dd4..d9f8b029c4 100644 --- a/tests/files-app-read-guard.unit.test.ts +++ b/tests/files-app-read-guard.unit.test.ts @@ -160,6 +160,32 @@ describe('files app-scoped read guard', () => { 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: [] }) diff --git a/tests/files-local-read-proxy.unit.test.ts b/tests/files-local-read-proxy.unit.test.ts index 3e15bb2546..e6ee11ec0e 100644 --- a/tests/files-local-read-proxy.unit.test.ts +++ b/tests/files-local-read-proxy.unit.test.ts @@ -57,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' }, @@ -95,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, @@ -136,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') + }) })