[codex] fix direct API key channel updates#1949
Conversation
📝 WalkthroughWalkthroughUpdates channel row-level security to restrict UPDATEs to Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client (hashed API key)
participant PostgREST as PostgREST / Request Context
participant RLS as channels RLS Policy
participant GetIdentity as public.get_identity_org_appid()
participant CheckRights as public.check_min_rights()
participant DB as public.channels
Client->>PostgREST: HTTP PATCH/UPDATE /channels (with headers/jwt)
PostgREST->>RLS: Evaluate UPDATE policy for row
RLS->>GetIdentity: resolve identity/org/app from session/claims
GetIdentity-->>RLS: identity (org_id, app_id, actor)
RLS->>CheckRights: verify actor has 'write' (or 'all') for target app/org
CheckRights-->>RLS: allow / deny
alt allow
RLS->>DB: permit UPDATE (operation proceeds)
DB-->>Client: 1 row affected (success)
else deny
RLS-->>Client: 0 rows affected (access denied)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![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: df4585fb8d
ℹ️ 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.
🧹 Nitpick comments (1)
tests/hashed-apikey-rls.test.ts (1)
89-128: Consider collapsing the anon/auth SQL runners into one helper.
execWithAnonCapgkey()now duplicates the same transaction, rollback, and connection-lifecycle flow fromexecWithAuthAndCapgkey(). A shared executor with role/claims parameters would reduce fixture drift the next time this session setup changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hashed-apikey-rls.test.ts` around lines 89 - 128, The test helpers execWithAnonCapgkey and execWithAuthAndCapgkey duplicate connection/transaction/rollback logic; refactor them into a single helper (e.g., execWithRoleClaims) that takes parameters for role, jwt claims object, and headers (capgkey) and performs the shared flow (pool.connect(), BEGIN, SET LOCAL ROLE, set_config for request.jwt.claims and request.headers, run query, COMMIT/ROLLBACK, release). Replace calls to execWithAnonCapgkey and execWithAuthAndCapgkey with calls to the new helper, keeping the same return shape ({ rows, rowCount }) and preserving error handling and JSON.stringify usage for set_config.
🤖 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/hashed-apikey-rls.test.ts`:
- Around line 89-128: The test helpers execWithAnonCapgkey and
execWithAuthAndCapgkey duplicate connection/transaction/rollback logic; refactor
them into a single helper (e.g., execWithRoleClaims) that takes parameters for
role, jwt claims object, and headers (capgkey) and performs the shared flow
(pool.connect(), BEGIN, SET LOCAL ROLE, set_config for request.jwt.claims and
request.headers, run query, COMMIT/ROLLBACK, release). Replace calls to
execWithAnonCapgkey and execWithAuthAndCapgkey with calls to the new helper,
keeping the same return shape ({ rows, rowCount }) and preserving error handling
and JSON.stringify usage for set_config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37541011-5684-4a27-a8f3-87a111d8bfcc
📒 Files selected for processing (2)
supabase/migrations/20260424090727_block_apikey_channel_updates.sqltests/hashed-apikey-rls.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 432a44237f
ℹ️ 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.
🧹 Nitpick comments (2)
tests/hashed-apikey-rls.test.ts (2)
620-633: Prefer explicit null checks in teardown guards.Using truthy checks on numeric ids is a bit brittle;
!== nullis safer and clearer for cleanup logic.Suggested patch
- if (channelId) { + if (channelId !== null) { await pool.query('DELETE FROM public.channels WHERE id = $1', [channelId]) } - if (versionId) { + if (versionId !== null) { await pool.query('DELETE FROM public.app_versions WHERE id = $1', [versionId]) } - if (allKey) + if (allKey !== null) await deleteApiKey(allKey.id) - if (writeKey) + if (writeKey !== null) await deleteApiKey(writeKey.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hashed-apikey-rls.test.ts` around lines 620 - 633, Teardown guards use truthy checks which can mis-handle numeric zero or other falsy values; change the conditions to explicit null checks: replace "if (channelId)" with "if (channelId !== null)", "if (versionId)" with "if (versionId !== null)", and similarly use "if (allKey !== null)" and "if (writeKey !== null)" before calling deleteApiKey or pool.query so cleanup always runs correctly for valid zero/false-y ids or objects.
745-771: Considerit.concurrent(...)for these two webhook tests if they remain independent.They look parallelizable and this would align with the test parallelism guideline.
As per coding guidelines:
tests/**/*.{ts,js}: Useit.concurrent()instead ofit()when possible to run tests in parallel within the same file, maximizing parallelism for faster CI/CD.Also applies to: 779-791
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hashed-apikey-rls.test.ts` around lines 745 - 771, Replace the sequential tests that independently query webhooks and deliveries with parallel tests by using it.concurrent(...) instead of it(...) for the tests that call execWithRoleClaims to fetch webhookRows and deliveryRows (the tests referencing webhook_deliveries and public.webhooks via execWithRoleClaims); ensure those tests do not share or mutate common state (or isolate any shared setup/fixtures) before switching to it.concurrent, and apply the same change to the related independent tests around the other occurrence noted (the tests covering the similar webhook/webhook_delivery assertions).
🤖 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/hashed-apikey-rls.test.ts`:
- Around line 620-633: Teardown guards use truthy checks which can mis-handle
numeric zero or other falsy values; change the conditions to explicit null
checks: replace "if (channelId)" with "if (channelId !== null)", "if
(versionId)" with "if (versionId !== null)", and similarly use "if (allKey !==
null)" and "if (writeKey !== null)" before calling deleteApiKey or pool.query so
cleanup always runs correctly for valid zero/false-y ids or objects.
- Around line 745-771: Replace the sequential tests that independently query
webhooks and deliveries with parallel tests by using it.concurrent(...) instead
of it(...) for the tests that call execWithRoleClaims to fetch webhookRows and
deliveryRows (the tests referencing webhook_deliveries and public.webhooks via
execWithRoleClaims); ensure those tests do not share or mutate common state (or
isolate any shared setup/fixtures) before switching to it.concurrent, and apply
the same change to the related independent tests around the other occurrence
noted (the tests covering the similar webhook/webhook_delivery assertions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c78d3ee1-4312-43d4-b3db-9241747405f0
📒 Files selected for processing (2)
supabase/migrations/20260424090727_block_apikey_channel_updates.sqltests/hashed-apikey-rls.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/migrations/20260424090727_block_apikey_channel_updates.sql
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea1214deef
ℹ️ 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".
|
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)
public.channelsupdates for write-scoped API keys/bundleAPI by moving its final channel version write onto a server-side path that keeps API-key audit attribution intact/bundleflow, and the resulting channel audit log attributionMotivation (AI generated)
A write-scoped API key could update protected
public.channelsfields directly through PostgREST because the update policy treated{write, all}keys the same, while the immutability guard skipped the NULL-auth path used by API-key requests. The RLS change closes that direct table-write path, and the/bundlefollow-up keeps the existing permission-checked deployment flow working without dropping the API-key actor from audit history.Business Impact (AI generated)
This removes a channel-configuration integrity gap for scoped API keys while preserving the supported bundle-promotion workflow and its audit trail that existing customer and CLI automation relies on.
Test Plan (AI generated)
bun run supabase:db:resetbunx eslint supabase/functions/_backend/public/bundle/set_channel.ts tests/bundle.test.ts tests/hashed-apikey-rls.test.ts tests/audit-logs.test.tsbun run supabase:with-env -- bunx vitest run tests/hashed-apikey-rls.test.ts -t \"channels rls blocks direct api-key updates|webhook and webhook_delivery rls with api-key org scope precedence\"bun run supabase:with-env -- bunx vitest run tests/bundle.test.ts -t \"should set bundle to channel successfully|should keep the supported write-scoped API key bundle promotion flow working\"bun run supabase:with-env -- bunx vitest run tests/audit-logs.test.ts -t \"app_version INSERT via API creates audit log with user_id from API key|app_version UPDATE via API creates audit log with user_id from API key|app_version soft-DELETE via API creates UPDATE audit log with user_id from API key|channel UPDATE via API key bundle promotion keeps audit user_id attribution\"Checklist (AI generated)
Screenshots (AI generated)
Not applicable. This PR only changes backend security policy, backend request handling, and automated tests.
Generated with AI