fix(rbac): stop mapping TUS auth DB failures to 403 insufficient_permissions - #2843
Conversation
Transient Hyperdrive/Postgres errors in checkPermission(Pg) were caught and returned as false, so TUS upload auth turned infra blips into intermittent 403 insufficient_permissions. Surface them as upstream_unavailable (503) so TUS can retry. Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
📝 WalkthroughWalkthroughRBAC permission checks now classify transient infrastructure failures separately from access denials. Transient failures return HTTP 503 errors, while other query failures return ChangesRBAC error handling
Statistics test SQL access
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Invalid org UUID casts (22P02) and other query errors were incorrectly surfaced as upstream_unavailable, breaking ACL deny tests that pass non-UUID org ids. Keep those as false (deny); reserve 503 for connection timeouts and Hyperdrive failures. Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5d6fd37e-fb62-4051-a4bb-88c5a4b78a0c) |
CF shard flakes with "invalid response from the upstream server" when this test seeds/updates channels through PostgREST. Use executeSQL like createAppVersions already does under shard load. Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_acdfc2ab-343e-4e97-b0e9-601af0b2a588) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/utils/rbac.ts`:
- Around line 133-164: Confirm the intended timeout behavior for permission
checks; if statement timeouts must not produce an ACL denial, update
TRANSIENT_PG_SQLSTATES to include SQLSTATE 57014 so
isTransientPermissionCheckError recognizes query-canceled statement timeouts as
transient. Preserve the existing matching logic and behavior for other error
codes.
- Around line 182-200: Compute isTransientPermissionCheckError(error) once in
the surrounding permission-error handling flow, store the result in a local
variable, and reuse that variable for both cloudlogErr’s transient field and the
subsequent quickError branch.
In `@tests/rbac-permission-infra-errors.unit.test.ts`:
- Around line 24-31: Extend the tests using makeContext to cover the non-rbac_id
query path in checkPermission and checkPermissionPg: add a transient-failure
case with authType set to jwt or apikey.rbac_id omitted, ensuring it exercises
handlePermissionCheckError, and add a successful fallback-query case asserting
true.
In `@tests/stats.test.ts`:
- Around line 894-925: Update executeSQL and its call sites in the affected
stats tests so the generic type arguments at the channelRows and updatedRows
queries are valid: either add a generic T returning Promise<T[]> to executeSQL,
or remove those type arguments and apply local row casts. Ensure backend
typechecking no longer reports TS2558 while preserving the existing row shapes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffcad180-9c47-443a-81df-23458457ea4f
📒 Files selected for processing (3)
supabase/functions/_backend/utils/rbac.tstests/rbac-permission-infra-errors.unit.test.tstests/stats.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Treat statement timeouts (57014) as transient 503s, classify once, cover the JWT/non-rbac_id permission path, and make executeSQL generic so typed channel seed queries typecheck. Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_32b60568-4db5-4f2f-a953-44b260fd3d1c) |
There was a problem hiding this comment.
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/utils/rbac.ts (1)
194-202: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the public status-code contract.
The PR changes the TUS upload failure response from
403to503.handlePermissionCheckErrorapplies this change to every caller and has no plugin-version or client-capability input. Existing clients can use403as a terminal ACL result.
supabase/functions/_backend/utils/rbac.ts#L194-L202: move response-status selection to a version-aware public endpoint boundary, or pass a validated compatibility decision into this handler. Preserve403for existing clients.tests/rbac-permission-infra-errors.unit.test.ts#L187-L228: add legacy-client403coverage and version-enabled503coverage.As per coding guidelines, “Public API and plugin changes must remain backward compatible: do not remove or change existing fields, meanings, formats, or status codes; use plugin version detection when behavior must differ.”
🤖 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 `@supabase/functions/_backend/utils/rbac.ts` around lines 194 - 202, Preserve the existing 403 response for legacy clients by moving status selection to the version-aware public endpoint boundary or passing a validated compatibility decision into handlePermissionCheckError; only version-enabled clients may receive 503 for transient permission failures. Update supabase/functions/_backend/utils/rbac.ts:194-202 accordingly, and add legacy-client 403 plus version-enabled 503 coverage in tests/rbac-permission-infra-errors.unit.test.ts:187-228.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@supabase/functions/_backend/utils/rbac.ts`:
- Around line 194-202: Preserve the existing 403 response for legacy clients by
moving status selection to the version-aware public endpoint boundary or passing
a validated compatibility decision into handlePermissionCheckError; only
version-enabled clients may receive 503 for transient permission failures.
Update supabase/functions/_backend/utils/rbac.ts:194-202 accordingly, and add
legacy-client 403 plus version-enabled 503 coverage in
tests/rbac-permission-infra-errors.unit.test.ts:187-228.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee2ba17d-7815-4aa9-bdde-9761eea12424
📒 Files selected for processing (3)
supabase/functions/_backend/utils/rbac.tstests/rbac-permission-infra-errors.unit.test.tstests/test-utils.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 3/5
- In
supabase/functions/_backend/utils/rbac.ts, treating all PostgreSQL57014(query_canceled) errors as transient and returning 503 can misclassify non-timeout cancellations, causing incorrect auth/error behavior and noisy retry patterns for clients; narrow the mapping to true statement timeouts (or add context checks) so RBAC failures keep the right status.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/functions/_backend/utils/rbac.ts">
<violation number="1" location="supabase/functions/_backend/utils/rbac.ts:118">
P2: A permission query canceled for any reason is now classified as transient and converted to a 503. PostgreSQL's `57014` is the general `query_canceled` SQLSTATE, not a statement-timeout-only code, so client/request cancellations or other operator cancellations can be mislabeled as `upstream_unavailable`, producing misleading retry behavior and diagnostics. Restrict this code to errors whose message or other metadata confirms a timeout, and handle the distinct lock-timeout SQLSTATE separately; add a regression case for a non-timeout `57014` cancellation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| '57P03', // cannot_connect_now | ||
| '53300', // too_many_connections | ||
| '53400', // configuration_limit_exceeded | ||
| '57014', // query_canceled (statement_timeout / lock_timeout) |
There was a problem hiding this comment.
P2: A permission query canceled for any reason is now classified as transient and converted to a 503. PostgreSQL's 57014 is the general query_canceled SQLSTATE, not a statement-timeout-only code, so client/request cancellations or other operator cancellations can be mislabeled as upstream_unavailable, producing misleading retry behavior and diagnostics. Restrict this code to errors whose message or other metadata confirms a timeout, and handle the distinct lock-timeout SQLSTATE separately; add a regression case for a non-timeout 57014 cancellation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/functions/_backend/utils/rbac.ts, line 118:
<comment>A permission query canceled for any reason is now classified as transient and converted to a 503. PostgreSQL's `57014` is the general `query_canceled` SQLSTATE, not a statement-timeout-only code, so client/request cancellations or other operator cancellations can be mislabeled as `upstream_unavailable`, producing misleading retry behavior and diagnostics. Restrict this code to errors whose message or other metadata confirms a timeout, and handle the distinct lock-timeout SQLSTATE separately; add a regression case for a non-timeout `57014` cancellation.</comment>
<file context>
@@ -115,9 +115,10 @@ const TRANSIENT_PG_SQLSTATES = new Set([
'57P03', // cannot_connect_now
'53300', // too_many_connections
'53400', // configuration_limit_exceeded
+ '57014', // query_canceled (statement_timeout / lock_timeout)
])
</file context>
2 participants
Summary (AI generated)
403 insufficient_permissionsonfiles.capgo.appTUS uploads (#2842)checkPermission/checkPermissionPgnow throw503 upstream_unavailableon transient Hyperdrive/Postgres connection failures (including statement timeout57014) instead of returningfalse22P02, etc.) still returnfalse→ ACL denyfalse→403/401rbac_idpathstatsrollout-metadata test to seed/update channels via genericexecuteSQLMotivation (AI generated)
In #2842, checksum + channel compatibility succeed with the same API key, then TUS
POST /files/upload/attachments/intermittently returns:{"error":"insufficient_permissions","message":"You don't have permission to access this app"}Retry usually succeeds. That is not a stable RBAC/legacy-key mismatch: CLI already passed
app.upload_bundleon the PostgREST path before TUS starts.Root cause: TUS
checkWriteAppAccesscallscheckPermissionPg. On DB/Hyperdrive errors, that helper caught the exception and returnedfalse, which the files worker mapped to403 insufficient_permissions.tus-js-clientdoes not retry 403, only 5xx — so customers needed an outer retry loop.Business Impact (AI generated)
retryDelaysalready present in CLI)upstream_unavailableTest Plan (AI generated)
bunx vitest run tests/rbac-permission-infra-errors.unit.test.tsupstream_unavailablea2598a4a3e3a226d/a259b6da790e1566, look for historicalcheckPermissionPg errorlogsFixes #2842
Generated with AI
Summary by CodeRabbit
Bug Fixes
Tests