fix(api): enforce org security settings validation#1845
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced direct DB updates in the org security settings UI with a shared Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client (browser)
participant UI as Frontend UI
participant Fn as Edge Function /organization (PUT)
participant DB as Database (orgs)
User->>UI: Toggle setting (e.g., enforce_encrypted_bundles)
UI->>UI: build body, call updateOrganizationSettings(body)
UI->>Fn: PUT /organization { orgId, ...body }
Fn->>Fn: normalize/validate required_encryption_key
Fn->>DB: UPDATE orgs SET ... WHERE id=orgId
DB-->>Fn: success / validation error
Fn-->>UI: 200 OK or 400 error
UI->>UI: on success call organizationStore.fetchOrganizations()
UI-->>User: show result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/apikeys-expiration.test.ts (1)
429-467: Consider usingit.concurrent()for parallel test execution.Per coding guidelines, all tests should use
it.concurrent()instead ofit()to run tests in parallel within the same file. However, since this test modifies shared state (updateOrgId) that other sequential tests in this describe block also depend on, and the comment on line 469-470 explicitly orders tests, the sequential execution may be intentional here.The test logic itself is correct and properly validates the DB CHECK constraint by:
- Capturing the value before the direct PATCH attempt
- Verifying the REST response fails
- Confirming the database value remains unchanged
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apikeys-expiration.test.ts` around lines 429 - 467, The test "rejects invalid max expiration days via direct org update RLS path" should be converted to use it.concurrent() to follow the parallel-test guideline unless this test must run sequentially due to shared state (updateOrgId) and the explicit ordering in the describe block; either change the invocation from it(...) to it.concurrent(...) for this test, or if sequential execution is intentional, add a brief inline comment above this test referencing updateOrgId and the describe-level ordering to document why it() is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/apikeys-expiration.test.ts`:
- Around line 429-467: The test "rejects invalid max expiration days via direct
org update RLS path" should be converted to use it.concurrent() to follow the
parallel-test guideline unless this test must run sequentially due to shared
state (updateOrgId) and the explicit ordering in the describe block; either
change the invocation from it(...) to it.concurrent(...) for this test, or if
sequential execution is intentional, add a brief inline comment above this test
referencing updateOrgId and the describe-level ordering to document why it() is
required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5093541a-04ba-4c9f-a8cb-b4693ee0bbc7
📒 Files selected for processing (5)
src/pages/settings/organization/Security.vuesupabase/functions/_backend/public/organization/put.tssupabase/migrations/20260323210855_enforce_org_apikey_expiration_days_check.sqltests/apikeys-expiration.test.tstests/organization-api.test.ts
|
Pushed a small CI-unblocker commit (e309474):
Local verification in the PR worktree: |
|
1 participant
Summary (AI generated)
PUT /organizationPUT /organizationto validate encrypted bundle key updatesmax_apikey_expiration_daysat the database layer to block direct REST/RLS bypassesMotivation (AI generated)
The security advisory GHSA-964v-j58v-2q3h showed that an org admin could bypass the validated
/organizationwrite path and persist an invalidmax_apikey_expiration_daysvalue through directpublic.orgsupdates. The fix needs to block the bypass on the server side and keep the dashboard aligned with the validated endpoint.Business Impact (AI generated)
This closes an in-scope security configuration bypass, reduces advisory handling time, and keeps organization security controls consistent across API and dashboard write paths.
Test Plan (AI generated)
bun run lintbunx eslint tests/apikeys-expiration.test.tsbunx sqlfluff lint supabase/migrations/20260323210855_enforce_org_apikey_expiration_days_check.sql --dialect postgresbun run supabase:with-env -- bunx vitest run tests/apikeys-expiration.test.tsbun run supabase:with-env -- bunx vitest run tests/organization-api.test.tsbun run lint:backend(currently fails on unrelatedsupabase/functions/_backend/files/uploadHandler.tsunused import)Generated with AI
Summary by CodeRabbit
New Features
Improvements
Bug Fixes