fix(files): block orphaned attachment access#1964
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughConsolidates preview-subdomain logic into a shared module, tightens app-scoped attachment read/write guards (RBAC and plan checks), adds S3 cleanup and helper to delete objects by prefix, and adds unit and E2E tests for attachment security, read-proxy, and preview label limits. Changes
Sequence DiagramssequenceDiagram
participant Client
participant FilesAPI as Files API Handler
participant DB as Primary Database
participant Storage as Supabase Storage / SignedURL
participant Workerd as Workerd Runtime
Client->>FilesAPI: GET /read/attachments/.../orgs/{owner_org}/apps/{app_id}/...
FilesAPI->>FilesAPI: Parse scoped path (owner_org, app_id)
FilesAPI->>DB: getAppByAppId(app_id)
alt App missing or owner mismatch
FilesAPI->>Client: 404 Not Found (JSON)
else App exists and org matches
FilesAPI->>FilesAPI: Determine runtime (workerd vs proxy)
alt workerd
FilesAPI->>Workerd: serve via direct runtime streaming
Workerd->>FilesAPI: Stream response
else proxy
FilesAPI->>Storage: createSignedUrl(path, ttl)
FilesAPI->>Storage: fetch(signedUrl) with Range passthrough
Storage->>FilesAPI: Upstream response (or error)
end
FilesAPI->>FilesAPI: Normalize headers (cache-control: no-transform, content-disposition)
FilesAPI->>Client: 200 OK with attachment (track bandwidth)
end
sequenceDiagram
participant Client
participant DeleteAPI as Delete App Endpoint
participant DB as Database
participant Trigger as Deletion Trigger
participant S3
Client->>DeleteAPI: DELETE /app/{appId}
DeleteAPI->>DB: Delete app record
DeleteAPI->>Trigger: Invoke deletion trigger / post-delete hook
Trigger->>DB: Read record.owner_org
alt owner_org exists
Trigger->>S3: deleteObjectsWithPrefix("orgs/{owner_org}/apps/{app_id}/")
S3->>Trigger: Return deletedCount (or throw)
Trigger->>Trigger: Log success with count / log failure (non-blocking)
else no owner_org
Trigger->>Trigger: Skip S3 cleanup
end
DeleteAPI->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1bf6a4d6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/files/files.ts (1)
356-372:⚠️ Potential issue | 🔴 CriticalThe Supabase runtime still exposes a public bypass.
The new guard runs before
/files/read/..., but the non-workerdbranch still redirects tostorage.from('capgo').getPublicUrl(fileId). Because app deletion only does best-effort storage cleanup, any leftover object remains directly readable through that public storage URL, so orphaned access is still open on the Supabase path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/files/files.ts` around lines 356 - 372, The non-workerd branch currently redirects to the public storage URL (storage.from('capgo').getPublicUrl(fileId)) leaving orphaned objects publicly accessible; instead, replace the redirect with a guarded proxy fetch of the object via the Supabase Admin SDK (e.g. use supabaseAdmin(c).storage.from('capgo').download(fileId) or the SDK equivalent) inside the getRuntimeKey() !== 'workerd' branch and stream/return the file bytes through the existing handler (preserving content-type and status) so callers never get the raw public URL; keep the existing assertReadableAppScopedAttachment(c, fileId) check and use the same requestId/logging (cloudlog) and c.redirect should be removed for this branch.
🧹 Nitpick comments (1)
supabase/functions/_backend/files/files.ts (1)
341-346: Use the shared error helper for the new guard failures.These new 404/429 branches hand-roll
HTTPException/c.json(...)responses instead of going throughquickError(). Keeping them on the shared helper path preserves the backend’s normal error envelope and middleware behavior.Possible fix
- throw new HTTPException(404, { - res: c.json({ - error: 'not_found', - message: 'Not found', - }, 404), - }) + throw quickError(404, 'not_found', 'Not found')- return c.json({ - error: 'on_premise_app', - message: 'On-premise app detected', - }, 429) + throw quickError(429, 'on_premise_app', 'On-premise app detected', { app_id })As per coding guidelines, "
supabase/functions/**/*.ts: All API endpoints must return success responses usingc.json(data),c.json(data, 200), orc.json(BRES)whereBRES = { status: 'ok' }, and errors usingsimpleError()orquickError(status, ...)fromutils/hono.ts".Also applies to: 869-874
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/files/files.ts` around lines 341 - 346, The manual construction of error responses using new HTTPException and c.json must be replaced with the shared helper quickError/simpleError so the middleware and envelope remain consistent; replace the throw new HTTPException(404, { res: c.json({...}, 404) }) pattern (and the similar 429 branch) with the shared helper call quickError(404, 'not_found', 'Not found') or simpleError(...) imported from utils/hono.ts (remove the c.json wrapper and the HTTPException construction), ensuring the code calls the helper (quickError/simpleError) wherever those guard failures occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/files/files.ts`:
- Around line 315-327: The parser parseAppScopedAttachmentPath currently accepts
paths ending with an empty suffix (e.g. "orgs/x/apps/y/") because split produces
a trailing empty segment; modify the validation to require a non-empty suffix
after app_id — e.g. after deconstructing parts into [orgs, owner_org, apps,
app_id, ...rest] ensure orgs === 'orgs' && apps === 'apps' && owner_org &&
app_id && rest.length > 0 && rest.join('/') !== '' (or simply check parts[4] is
truthy) and return null otherwise so prefix-only keys are rejected.
In `@supabase/functions/_backend/utils/s3.ts`:
- Around line 128-131: The current for-await loop over client.listObjects stops
on the first delete failure because await client.deleteObject(object.key) can
throw; modify the loop in the function using client.listObjects /
client.deleteObject so each deletion is wrapped in a try/catch (catch and log
the error with context including object.key), increment deletedCount only on
success, and continue to the next object so remaining objects get best-effort
cleanup; optionally collect or return aggregated error info but do not let one
failure abort the loop.
In `@supabase/functions/shared/preview-subdomain.ts`:
- Around line 52-54: buildPreviewSubdomain can return labels >63 bytes because
encodePreviewAppId expands characters; modify buildPreviewSubdomain to construct
the preview label using versionId, PREVIEW_VERSION_SEPARATOR and
encodePreviewAppId(appId), then validate the resulting label length against the
63-character DNS limit and reject (throw a clear error) if it exceeds the limit;
reference the functions/symbols buildPreviewSubdomain, encodePreviewAppId and
PREVIEW_VERSION_SEPARATOR and ensure the error message includes the computed
label and its length for easier debugging.
---
Outside diff comments:
In `@supabase/functions/_backend/files/files.ts`:
- Around line 356-372: The non-workerd branch currently redirects to the public
storage URL (storage.from('capgo').getPublicUrl(fileId)) leaving orphaned
objects publicly accessible; instead, replace the redirect with a guarded proxy
fetch of the object via the Supabase Admin SDK (e.g. use
supabaseAdmin(c).storage.from('capgo').download(fileId) or the SDK equivalent)
inside the getRuntimeKey() !== 'workerd' branch and stream/return the file bytes
through the existing handler (preserving content-type and status) so callers
never get the raw public URL; keep the existing
assertReadableAppScopedAttachment(c, fileId) check and use the same
requestId/logging (cloudlog) and c.redirect should be removed for this branch.
---
Nitpick comments:
In `@supabase/functions/_backend/files/files.ts`:
- Around line 341-346: The manual construction of error responses using new
HTTPException and c.json must be replaced with the shared helper
quickError/simpleError so the middleware and envelope remain consistent; replace
the throw new HTTPException(404, { res: c.json({...}, 404) }) pattern (and the
similar 429 branch) with the shared helper call quickError(404, 'not_found',
'Not found') or simpleError(...) imported from utils/hono.ts (remove the c.json
wrapper and the HTTPException construction), ensuring the code calls the helper
(quickError/simpleError) wherever those guard failures occur.
🪄 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: 4bfc1fd4-a04d-4453-aa2b-3aabdff53c6a
📒 Files selected for processing (9)
shared/preview-subdomain.tssupabase/functions/_backend/files/files.tssupabase/functions/_backend/files/preview.tssupabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/triggers/on_app_delete.tssupabase/functions/_backend/utils/s3.tssupabase/functions/shared/preview-subdomain.tstests/files-app-read-guard.unit.test.tstests/files-security.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22dc84b871
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/files/files.ts`:
- Around line 915-927: The app plan check is using a replica-backed client
(drizzleClient) so replica lag can let blocked/deleted apps bypass the write
gate; change the call that fetches the plan so it reads from the primary instead
of a replica—i.e., ensure getAppByIdPg is invoked with a primary DB client
(create or obtain a drizzle client from getPgClient(c, false) or pass the flag
that disables replicas) when calling getAppByIdPg(c, app_id, drizzleClient,
ATTACHMENT_PLAN_LIMIT) so the subsequent check of appPlan.plan_valid is
authoritative.
- Around line 383-415: The signed-URL proxy always issues a GET which makes HEAD
requests download bytes and be billed; change the fetch call to use the incoming
request method (use c.req.method as the fetch option method when calling
fetch(signedUrlData.signedUrl, ...)) and preserve headers (e.g., continue
passing requestHeaders when rangeHeader exists). Also only call
saveBandwidthUsage/getTransferredBytesFromResponse for non-HEAD requests (e.g.,
if (c.req.method !== 'HEAD') await saveBandwidthUsage(c,
getTransferredBytesFromResponse(response))). Update references:
signedUrlData.signedUrl, requestHeaders/rangeHeader, fetch(...),
saveBandwidthUsage, getTransferredBytesFromResponse,
withAttachmentResponseHeaders.
In `@supabase/functions/shared/preview-subdomain.ts`:
- Around line 53-57: Validate that versionId is a non-negative safe integer
before using it in label generation: in buildPreviewSubdomain (and the other
label-emitting function around lines 106-112), replace the current unconstrained
usage of versionId with a guard like if (!Number.isSafeInteger(versionId) ||
versionId < 0) throw new Error(`Invalid versionId: must be a non-negative safe
integer`); this prevents decimals, negatives, and > Number.MAX_SAFE_INTEGER
values from being encoded into labels that cannot be parsed back. Ensure the
same check is applied in both functions that emit versionId into labels.
🪄 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: 2b7dbead-5a9d-4841-aad0-5be22b172327
📒 Files selected for processing (6)
supabase/functions/_backend/files/files.tssupabase/functions/_backend/utils/s3.tssupabase/functions/shared/preview-subdomain.tstests/files-app-read-guard.unit.test.tstests/files-local-read-proxy.unit.test.tstests/preview-subdomain.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/files-app-read-guard.unit.test.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/functions/_backend/files/files.ts (1)
922-934: Consider collapsing the second app lookup.This middleware now reads
appsonce for existence/org and again for plan state. If the row changes between those calls, the error shape can flip from a deterministic 404/403 into the 503 fallback here. A single helper that returnsowner_orgplusplan_validwould remove the extra primary round trip and close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/files/files.ts` around lines 922 - 934, Duplicate reads of the app row cause a race where existence/ownership checks and plan state can diverge; replace the two separate lookups with a single helper (e.g., extend or create a function like getAppOwnerAndPlanPg or augment getAppByIdPg to return owner_org and plan_valid in one query, using the same ATTACHMENT_PLAN_LIMIT), then use that single result for both existence/org checks and the plan_valid branch so you remove the second call that produces appPlan and the 503 fallback race.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/files/files.ts`:
- Around line 274-285: withAttachmentResponseHeaders currently only sets
Content-Disposition when absent, causing inconsistent behavior vs the Worker
path; change it to always set/overwrite the 'content-disposition' header to
`attachment; filename="${fileId}"` so both Edge/Supabase and Worker flows force
download consistently. Locate the withAttachmentResponseHeaders function and
replace the conditional headers.set('content-disposition', ...) with an
unconditional set, preserving the existing cache-control handling and returning
the new Response with the updated headers, status and statusText.
---
Nitpick comments:
In `@supabase/functions/_backend/files/files.ts`:
- Around line 922-934: Duplicate reads of the app row cause a race where
existence/ownership checks and plan state can diverge; replace the two separate
lookups with a single helper (e.g., extend or create a function like
getAppOwnerAndPlanPg or augment getAppByIdPg to return owner_org and plan_valid
in one query, using the same ATTACHMENT_PLAN_LIMIT), then use that single result
for both existence/org checks and the plan_valid branch so you remove the second
call that produces appPlan and the 503 fallback race.
🪄 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: 44d80873-d88a-44c6-b0cd-8a4e6210197c
📒 Files selected for processing (6)
src/components/BundlePreviewFrame.vuesupabase/functions/_backend/files/files.tssupabase/functions/_backend/files/preview.tssupabase/functions/shared/preview-subdomain.tstests/files-local-read-proxy.unit.test.tstests/preview-subdomain.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- supabase/functions/_backend/files/preview.ts
- tests/files-local-read-proxy.unit.test.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
1 participant
Summary (AI generated)
app.upload_bundlefor attachment uploads and validate theorgs/{owner_org}/apps/{app_id}/...path shape404for app-scoped attachment reads once the app/org mapping no longer existsMotivation (AI generated)
GitHub advisory
GHSA-q52j-ggvx-cr4vreported that plan-blocked apps could still create publicly readable orphaned attachment objects that survived app deletion. The files path needed the same plan enforcement and app existence checks that already protect the hotter plugin endpoints.Business Impact (AI generated)
This closes a public file exposure path for canceled or on-premise apps and reduces leftover storage after app deletion. It lowers security risk without changing the expected behavior for valid cloud apps.
Test Plan (AI generated)
bun lintbun run supabase:with-env -- bunx vitest run tests/files-security.test.ts tests/files-app-read-guard.unit.test.ts tests/files-r2-error.test.ts tests/tus-upload.test.ts tests/preview-subdomain.unit.test.ts tests/plugin-credits-flag.test.tsGenerated with AI
Summary by CodeRabbit
Security Improvements
Bug Fixes / Maintenance
New Features
Tests