Block deleted bundle reads from cache#2124
Conversation
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 <noreply@openai.com>
📝 WalkthroughWalkthroughAdds a DB-backed soft-deletion guard for app-scoped attachments (checks app_versions/manifest and returns 404), tightens download_link to exclude deleted app_versions, adds indexes, and updates/introduces unit tests plus a shared pgClient mock. ChangesSoft-Deleted Bundle Read Prevention
Sequence Diagram(s)sequenceDiagram
participant Client
participant assertReadableAppScopedAttachment
participant PostgreSQL
participant AttachmentCache
Client->>assertReadableAppScopedAttachment: GET app-scoped attachment (fileId)
assertReadableAppScopedAttachment->>PostgreSQL: SELECT FROM public.app_versions / EXISTS(public.manifest) with (owner_org, app_id, fileId)
alt deleted match found
PostgreSQL-->>assertReadableAppScopedAttachment: deleted row found
assertReadableAppScopedAttachment-->>Client: 404 not_found
else no deleted match
PostgreSQL-->>assertReadableAppScopedAttachment: no deleted row
assertReadableAppScopedAttachment->>AttachmentCache: request cached bytes or fallback read
AttachmentCache-->>Client: 200 + bytes
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/files-app-read-guard.unit.test.ts (1)
111-150: ⚡ Quick winConsider adding a positive test case for non-deleted bundles.
The new test correctly validates that soft-deleted bundles return 404 before serving cached content. However, the test suite lacks coverage for the positive path:
- App exists ✅ (mocked)
- Path is valid ✅
- Bundle is NOT deleted
⚠️ (not tested)- Content should be served from cache/R2
⚠️ (not tested)This positive case may be covered in integration tests, but adding it here would strengthen confidence that the deletion check doesn't break normal reads.
📝 Suggested test case for non-deleted bundle
it('serves cached content for non-deleted app-scoped bundle paths', async () => { getAppByAppIdPgMock.mockResolvedValue({ app_id: 'test-app', owner_org: 'test-org' }) pgClientMock.query.mockResolvedValue({ rows: [] }) // No deleted bundle found const cachedContent = 'valid bundle content' globalThis.caches = { default: { match: async () => new Response(cachedContent, { headers: { 'content-type': 'application/zip', }, }), put: async () => { }, }, } as any const { app: files } = await import('../supabase/functions/_backend/files/files.ts') const { createAllCatch, createHono } = await import('../supabase/functions/_backend/utils/hono.ts') const { version } = await import('../supabase/functions/_backend/utils/version.ts') const appGlobal = createHono('files', version) appGlobal.route('/', files) createAllCatch(appGlobal, 'files') 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: { get: async () => null, // Simulate R2 miss, rely on cache }, }, { waitUntil: () => { } } as any, ) expect(response.status).toBe(200) expect(await response.text()).toBe(cachedContent) expect(pgClientMock.query).toHaveBeenCalledWith( expect.stringContaining('FROM public.app_versions'), ['test-org', 'test-app', filePath], ) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/files-app-read-guard.unit.test.ts` around lines 111 - 150, Add a positive test that mirrors the soft-delete test but mocks pgClientMock.query to return no rows (bundle not deleted), mocks globalThis.caches.default.match to return a Response with ZIP content, and provides ATTACHMENT_BUCKET with a get that returns null (simulate R2 miss) so the cached response is served; in that test (e.g., it('serves cached content for non-deleted app-scoped bundle paths'...)) import files, createHono, createAllCatch and version as in the existing test, call appGlobal.fetch for the same filePath, then assert response.status === 200, response.text() matches the cached content, and pgClientMock.query was called with expect.stringContaining('FROM public.app_versions') and ['test-org', 'test-app', filePath] to mirror the deletion-check verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/functions/_backend/files/files.ts`:
- Around line 360-375: The SELECT in the app-scoped attachment read (the
pgClient.query that populates deletedBundlePath in files.ts) is slow because
there is no composite index on public.app_versions for (owner_org, app_id,
r2_path) and no coverage for the deleted flag; add a DB migration that creates
either a full composite index on (owner_org, app_id, r2_path, deleted) or a
partial index like (owner_org, app_id, r2_path) WHERE deleted = true to fully
cover the WHERE clause, then run migrations and remove/adjust any fallback query
changes—locate the query by the deletedBundlePath variable/pgClient.query in
files.ts and create the migration altering public.app_versions accordingly.
---
Nitpick comments:
In `@tests/files-app-read-guard.unit.test.ts`:
- Around line 111-150: Add a positive test that mirrors the soft-delete test but
mocks pgClientMock.query to return no rows (bundle not deleted), mocks
globalThis.caches.default.match to return a Response with ZIP content, and
provides ATTACHMENT_BUCKET with a get that returns null (simulate R2 miss) so
the cached response is served; in that test (e.g., it('serves cached content for
non-deleted app-scoped bundle paths'...)) import files, createHono,
createAllCatch and version as in the existing test, call appGlobal.fetch for the
same filePath, then assert response.status === 200, response.text() matches the
cached content, and pgClientMock.query was called with
expect.stringContaining('FROM public.app_versions') and ['test-org', 'test-app',
filePath] to mirror the deletion-check verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecb1ddfe-4732-4476-9ed4-e9a145d19de5
📒 Files selected for processing (2)
supabase/functions/_backend/files/files.tstests/files-app-read-guard.unit.test.ts
There was a problem hiding this comment.
Pull request overview
This PR closes a security gap in the files worker where cached bundle bytes could still be served (and potentially restored to R2) after the corresponding bundle path was soft-deleted in public.app_versions.
Changes:
- Add a Postgres guard that treats app-scoped attachment reads as 404 when the requested path matches a soft-deleted
app_versions.r2_path. - Ensure this guard runs before any cached attachment content is returned.
- Add a regression unit test that simulates a cache hit for a deleted bundle path and verifies a 404 is returned.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
supabase/functions/_backend/files/files.ts |
Adds a DB check in assertReadableAppScopedAttachment to block reads of cached bundle content when the path corresponds to a soft-deleted app_versions.r2_path. |
tests/files-app-read-guard.unit.test.ts |
Updates PG client mocking and adds a regression test ensuring deleted bundle paths return 404 even when cache contains bytes. |
lyndon050516
left a comment
lyndon050516
left a comment
There was a problem hiding this comment.
I would not merge this without a matching migration for the new app_versions lookup.
I checked the current schema and migrations: app_versions has separate indexes for owner_org, app_id, and deleted, plus deleted = false partial indexes for other paths, but I do not see any index involving r2_path or a deleted = true partial index. Since this guard runs in assertReadableAppScopedAttachment before serving cached content, every app-scoped attachment read now pays for this query before it can use the cache.
A narrow partial index would keep the security guard cheap and scoped to the rows it cares about, for example:
CREATE INDEX IF NOT EXISTS idx_app_versions_deleted_r2_path
ON public.app_versions (owner_org, app_id, r2_path)
WHERE deleted = true;If the migration runner supports concurrent index creation, CONCURRENTLY would also be worth considering. The key point is covering (owner_org, app_id, r2_path) for deleted rows so cache hits do not become repeated table scans as app_versions grows.
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 <noreply@openai.com>
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 <noreply@openai.com>
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 <noreply@openai.com>
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 <noreply@openai.com>
|
Addressed the index and coverage feedback. Current head
CI is green on the current head. |
SpeedyArt
left a comment
SpeedyArt
left a comment
There was a problem hiding this comment.
I think the manifest-asset branch of this guard can miss the normal persisted state.
on_version_update copies record.manifest into rows in public.manifest, updates manifest_count, and then clears app_versions.manifest with .update({ manifest: null }). Runtime manifest download paths are also built from public.manifest (download_link.ts -> .from('manifest'), plus the update queries join schema.manifest). So after the trigger has run, the deleted version's asset paths usually live in public.manifest, not in the app_versions.manifest array this query unnests.
That means a cached direct read for a deleted manifest asset can still get rows.length === 0 here and be served from cache, even though the PR now blocks r2_path bundle reads and private download-link lookup. I would change the EXISTS branch to check the persisted table, for example EXISTS (SELECT 1 FROM public.manifest m WHERE m.app_version_id = app_versions.id AND m.s3_path = $3), and add a regression where app_versions.manifest is null but public.manifest contains the deleted asset path. Since this now runs before cache hits, it probably also wants an index covering the manifest lookup such as (app_version_id, s3_path) or (s3_path, app_version_id).
Also adds the manifest lookup index and updates the read-guard regression to cover persisted manifest assets.\n\nCo-authored-by: Codex <noreply@openai.com>
|
Pushed Changes:
Validation:
|
|
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
Reviewed current head 0374e6c with focus on the deleted-bundle cache-read guard and the two earlier review concerns. The files worker now checks the primary DB before any cached/R2 response is served, and the guard covers both app_versions.r2_path and persisted public.manifest.s3_path rows for deleted versions. The private /download_link path also filters deleted = false, so it no longer mints bundle/manifest links for soft-deleted rows.
I also checked the performance feedback path: the migration adds the deleted app_versions(owner_org, app_id, r2_path) partial index and the manifest(app_version_id, s3_path) index used by the EXISTS branch.
Local verification passed on the touched scope:
bun x eslint supabase/functions/_backend/files/files.ts supabase/functions/_backend/private/download_link.ts tests/download-link-deleted-bundle.unit.test.ts tests/files-app-read-guard.unit.test.ts tests/files-local-read-proxy.unit.test.tsbun x vitest run tests/files-app-read-guard.unit.test.ts tests/files-local-read-proxy.unit.test.ts tests/download-link-deleted-bundle.unit.test.ts→ 3 files / 8 tests passedgit diff --check 3d184f8..HEAD
No remaining touched-file blocker from me.
|
The manifest-file cache guard still has a hole after the normal deleted-version cleanup runs. The new tests cover the direct bundle |
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked latest head (0374e6c). The new cache guard still depends on a live public.manifest row to recognize deleted manifest assets:
EXISTS (SELECT 1 FROM public.manifest m WHERE m.app_version_id = public.app_versions.id AND m.s3_path = $3)But the normal delete flow removes manifest rows during deleteManifest()/version cleanup before or while R2 objects are deleted. Once that row is gone, a cached delta asset request no longer matches the guard, so /files/read/attachments/.../delta/... can still serve stale cached bytes. The added regression only covers the persisted-manifest-row case, not the row-removed-after-soft-delete case.
To close this, persist a durable tombstone/path index that survives manifest cleanup, or preserve enough path metadata until cache/R2 deletion is guaranteed. A regression should seed/cache a manifest asset, run the delete flow so the manifest row is removed, then assert the cached attachment read remains blocked. The PR is currently merge-conflicted (DIRTY); keep blocked until this path is covered.
|
The DB-side guard work here looks solid -- the manifest persistence concern @digzrow-coder and @KCDaemon raised is the biggest miss to address before merging. Two additional gaps worth considering since they sit upstream of this guard and aren't covered by the new tests. 1. Edge / CDN caching can serve deleted bundle bytes despite the DB guard
For SaaS billing/customer-data bundles this is the exact "deleted but still accessible" scenario the PR is trying to close. Suggested follow-ups:
2. Signed download links already issued continue to resolve after deletion
If TTLs are short (minutes) the exposure window is small; if TTLs are hour/day-scale (common for big bundle downloads) the deletion is essentially advisory until expiry. Two options, low to high effort:
A test that issues a signed URL, soft-deletes the version, and asserts the URL returns 404 would make either fix verifiable. Neither blocks the direct-fetch fix that's the meat of this PR, but both undercut the security claim if left out. Happy to send a follow-up PR with the cache-invalidate path + a regression test if there's interest. |
|
I could not push to the original
This addresses the remaining blocker from @KCDaemon: the guard no longer depends only on a live Validation on the rebased branch:
I also tried opening a replacement PR from |
Summary
app_versions.r2_pathor a persisted manifest asset path./claim #1667
Test plan
0374e6c.bunx vitest run tests/files-app-read-guard.unit.test.tsbunx vitest run tests/files-app-read-guard.unit.test.ts tests/files-local-read-proxy.unit.test.ts tests/download-link-deleted-bundle.unit.test.tsbunx eslint supabase/functions/_backend/files/files.ts supabase/functions/_backend/private/download_link.ts tests/files-app-read-guard.unit.test.ts tests/files-local-read-proxy.unit.test.ts tests/download-link-deleted-bundle.unit.test.tsgit diff --checkScreenshots
Not applicable; backend-only change.
Checklist