[codex] fix hashed API key RLS auth bypass#1948
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:
📝 WalkthroughWalkthroughAdds DB functions to locate API keys and enforce org-level "hashed only" policy during RLS identity lookups, adds guarded app-owner transfer logic with permission checks and cooldown, and adds/updates tests exercising hashed-api-key RLS behavior and a test auth-header fix. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RLS as RLS Identity Path
participant Finder as find_apikey_by_value
participant Enforcer as check_apikey_hashed_key_enforcement
participant DB as Database
Client->>RLS: request with API key
RLS->>Finder: find_apikey_by_value(key)
Finder->>DB: SELECT FROM public.apikeys WHERE key=key OR key_hash=sha256_hex(key)
DB-->>Finder: candidate apikey row(s)
Finder->>Enforcer: check_apikey_hashed_key_enforcement(apikey_row)
Enforcer->>DB: evaluate org membership/role paths for enforce_hashed_api_keys
alt enforcement triggered (plaintext present for enforced org)
DB-->>Enforcer: enforcement=true
Enforcer->>DB: pg_log denial (deny: ORG_REQUIRES_HASHED_API_KEY, apikey_id, user_id)
Enforcer-->>Finder: false (reject row)
Finder-->>RLS: no matching apikey
RLS-->>Client: auth rejected
else enforcement not triggered or hashed key used
Enforcer-->>Finder: true (allow row)
Finder-->>RLS: apikey row returned
RLS-->>Client: auth accepted
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 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: 2e0a02a15e
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93577dc063
ℹ️ 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".
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql (1)
50-67:⚠️ Potential issue | 🟠 MajorLock down
find_apikey_by_value()explicitly.This helper stays
SECURITY DEFINER, but this migration only resets the owner. Please also make the EXECUTE ACL explicit here instead of relying on whatever grants happened to exist before:REVOKE ALL ... FROM PUBLIC, then grant only the roles that should be able to call it.As per coding guidelines,
**/*.sql: For PostgreSQL RPC and helper functions, apply minimum privileges explicitly: start from deny-by-default, set OWNER explicitly, use REVOKE ALL ... FROM PUBLIC, and grant only required roles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql` around lines 50 - 67, The migration updates owner but doesn't explicitly set the function's execution ACL; update the SQL for function "public"."find_apikey_by_value"(key_value text) to revoke all privileges from PUBLIC and then grant EXECUTE only to the specific roles that should call it (e.g., the service role and any app role), i.e. add REVOKE ALL ON FUNCTION "public"."find_apikey_by_value"(text) FROM PUBLIC; followed by GRANT EXECUTE ON FUNCTION "public"."find_apikey_by_value"(text) TO <allowed_role(s)> so the function remains SECURITY DEFINER with a deny-by-default ACL and only the minimum required roles can invoke find_apikey_by_value.
🧹 Nitpick comments (1)
tests/hashed-apikey-rls.test.ts (1)
365-396: Avoid togglingORG_ID_RLSin-place for this suite.This block mutates a shared seeded org in
beforeAlland again inside the pending-invite test. If the suite fails before cleanup, later tests inherit the wrongenforce_hashed_api_keysstate. Prefer a dedicated org/member seed for this suite and toggle that instead.As per coding guidelines,
tests/**/*.{ts,js}: When creating tests that interact with shared resources, create dedicated seed data when your test modifies resources; reuse existing seed data only when you read without modification and create your own child resources.🤖 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 365 - 396, The test suite mutates the shared seeded org (ORG_ID_RLS) in beforeAll/afterAll via setOrgHashedApiKeyEnforcement which can leave global state dirty if the suite fails; instead create and use a dedicated test org/member for this suite (e.g., seed a new org before tests and remove it after) and toggle enforcement on that org. Update beforeAll to call a helper like createTestOrg/seedOrgForTests, use its returned id in createHashedApiKey/createPlainApiKey and in setOrgHashedApiKeyEnforcement, and in afterAll delete the test org and keys via deleteApiKey and a corresponding deleteTestOrg/cleanup helper to avoid touching ORG_ID_RLS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql`:
- Around line 50-67: The migration updates owner but doesn't explicitly set the
function's execution ACL; update the SQL for function
"public"."find_apikey_by_value"(key_value text) to revoke all privileges from
PUBLIC and then grant EXECUTE only to the specific roles that should call it
(e.g., the service role and any app role), i.e. add REVOKE ALL ON FUNCTION
"public"."find_apikey_by_value"(text) FROM PUBLIC; followed by GRANT EXECUTE ON
FUNCTION "public"."find_apikey_by_value"(text) TO <allowed_role(s)> so the
function remains SECURITY DEFINER with a deny-by-default ACL and only the
minimum required roles can invoke find_apikey_by_value.
---
Nitpick comments:
In `@tests/hashed-apikey-rls.test.ts`:
- Around line 365-396: The test suite mutates the shared seeded org (ORG_ID_RLS)
in beforeAll/afterAll via setOrgHashedApiKeyEnforcement which can leave global
state dirty if the suite fails; instead create and use a dedicated test
org/member for this suite (e.g., seed a new org before tests and remove it
after) and toggle enforcement on that org. Update beforeAll to call a helper
like createTestOrg/seedOrgForTests, use its returned id in
createHashedApiKey/createPlainApiKey and in setOrgHashedApiKeyEnforcement, and
in afterAll delete the test org and keys via deleteApiKey and a corresponding
deleteTestOrg/cleanup helper to avoid touching ORG_ID_RLS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5281ffe4-0edf-42c5-95ef-10a3ae7d6213
📒 Files selected for processing (3)
supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sqltests/hashed-apikey-rls.test.tstests/organization-api.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql`:
- Around line 139-155: The new SECURITY DEFINER helper
find_apikey_by_value(text) must be locked down: after creating/altering the
function, explicitly REVOKE ALL ON FUNCTION public.find_apikey_by_value(text)
FROM PUBLIC and then GRANT EXECUTE ON FUNCTION public.find_apikey_by_value(text)
only to the minimal role(s) that need it (e.g., the DB owner/service role such
as postgres or your application service role); keep the OWNER as set and ensure
the revoke/grant appear in the same migration so the function is not publicly
executable by default.
In `@tests/hashed-apikey-rls.test.ts`:
- Around line 418-444: The test is mutating the shared ORG_ID_RLS fixture;
instead create and use a dedicated org fixture for this suite: replace uses of
ORG_ID_RLS in the beforeAll/afterAll block and calls to
setOrgHashedApiKeyEnforcement with a new org created via the project’s org
creation helper (e.g. createOrg or createTestOrg), store its id in a local
variable (e.g. testOrgId) and tear it down in afterAll; ensure
createHashedApiKey/createPlainApiKey and deleteApiKey are called against that
org’s context if required, and restore/remove the org instead of toggling the
shared org’s enforce_hashed_api_keys flag.
🪄 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: f109669d-eaa3-4d0e-ab7a-e392540632d8
📒 Files selected for processing (2)
supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.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: c1e607d85c
ℹ️ 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: 1
🤖 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/migrations/20260427092702_fix_transfer_app_guard_allowlist.sql`:
- Around line 131-160: The call to PERFORM
set_config('capgo.allow_owner_org_transfer','true',true) in transfer_app() leaks
to the surrounding transaction and can let guard_owner_org_reassignment() be
bypassed; fix by scoping the allowlist change inside a nested PL/pgSQL block so
you always reset it before leaving the block: move the PERFORM
set_config('capgo.allow_owner_org_transfer','true',true) into a BEGIN ... END
block and add an EXCEPTION handler that PERFORMs
set_config('capgo.allow_owner_org_transfer','false',true) and re-raises, and
also PERFORM the 'false' reset after the block on the normal path so the flag is
always cleared even on error. Ensure you reference the existing PERFORM
set_config call and transfer_app()/guard_owner_org_reassignment() when making
the change.
🪄 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: 7f4f93d1-84c0-40bc-98c0-ecc76eda2b71
📒 Files selected for processing (1)
supabase/migrations/20260427092702_fix_transfer_app_guard_allowlist.sql
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3618b6a0d2
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 941b047a48
ℹ️ 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: 1
🧹 Nitpick comments (1)
supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql (1)
42-42: Fully qualify built-ins under the emptysearch_path.This migration still calls
now(),jsonb_build_object(), andencode()unqualified. WithSET search_path TO '', please make thesepg_catalog.now(),pg_catalog.jsonb_build_object(), andpg_catalog.encode()to match the repository rule.As per coding guidelines:
**/*.sql: Set an empty search path (search_path = '') in every PostgreSQL function and use fully qualified names for all referencesAlso applies to: 59-59, 69-69, 79-79, 97-97, 108-108, 119-119, 138-138, 150-150, 159-159, 182-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql` at line 42, The migration uses unqualified built-ins under an empty search_path: replace calls to now(), jsonb_build_object(), and encode() with fully-qualified pg_catalog.now(), pg_catalog.jsonb_build_object(), and pg_catalog.encode() respectively (e.g., inside the RLS/identity SQL snippets and any function bodies referenced by names in the diff) so all built-in references are fully qualified; apply the same replacements for every other occurrence of these built-ins in the file (the other instances noted in the review) to comply with the repository rule requiring search_path = ''.
🤖 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/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql`:
- Around line 188-190: The migration currently revokes public access and grants
"find_apikey_by_value(text)" only to service_role, which will break direct
RPC/SQL calls from Edge Functions (supabase/functions/_backend/utils/supabase.ts
uses .rpc('find_apikey_by_value', ...) and
supabase/functions/_backend/utils/hono_middleware.ts runs SELECT * FROM
find_apikey_by_value(...)); fix by either granting the function to the
authenticated/anon roles in the migration (add GRANT EXECUTE ON FUNCTION
"public"."find_apikey_by_value"(text) TO authenticated, anon) or convert the
function to a SECURITY DEFINER wrapper owned by service_role and keep
restrictive grants so callers invoke the wrapper or RPC without permission
errors (ensure wrapper name and callers are updated accordingly).
---
Nitpick comments:
In
`@supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql`:
- Line 42: The migration uses unqualified built-ins under an empty search_path:
replace calls to now(), jsonb_build_object(), and encode() with fully-qualified
pg_catalog.now(), pg_catalog.jsonb_build_object(), and pg_catalog.encode()
respectively (e.g., inside the RLS/identity SQL snippets and any function bodies
referenced by names in the diff) so all built-in references are fully qualified;
apply the same replacements for every other occurrence of these built-ins in the
file (the other instances noted in the review) to comply with the repository
rule requiring search_path = ''.
🪄 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: d429f874-b557-4959-a050-aff197a6f893
📒 Files selected for processing (3)
supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sqlsupabase/migrations/20260427092702_fix_transfer_app_guard_allowlist.sqltests/hashed-apikey-rls.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/hashed-apikey-rls.test.ts
- supabase/migrations/20260427092702_fix_transfer_app_guard_allowlist.sql
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)
tests/organization-api.test.ts (1)
48-60:⚠️ Potential issue | 🟠 MajorKeep
ORG_IDaccessible to the default test user to avoid cross-suite regressionsAt Line 48 and Line 59, the seed now grants
ORG_IDownership/membership only toUSER_ID_2, but many existing tests in this file still operate onORG_IDusingheaders/USER_IDassumptions. This can turn unrelated tests into auth failures.Suggested fix
const { error: orgUserError } = await getSupabaseClient().from('org_users').insert({ - org_id: ORG_ID, - user_id: USER_ID_2, - user_right: 'super_admin', - }) + org_id: ORG_ID, + user_id: USER_ID_2, + user_right: 'super_admin', + }) + if (orgUserError) + throw orgUserError + + // Keep legacy organization API tests (USER_ID/headers) valid on this shared org seed + const { error: orgUserErrorUser1 } = await getSupabaseClient().from('org_users').insert({ + org_id: ORG_ID, + user_id: USER_ID, + user_right: 'super_admin', + }) + if (orgUserErrorUser1) + throw orgUserErrorUser1 - if (orgUserError) - throw orgUserError🤖 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 48 - 60, The seed grants ORG_ID only to USER_ID_2 which breaks tests that use the default test user (USER_ID); update the seeding so the default test user retains access: in the org creation/seed block (the code that sets created_by and inserts into org_users via getSupabaseClient().from('org_users').insert) also insert or upsert an org_users row for USER_ID with appropriate user_right (e.g., 'super_admin' or same right as USER_ID_2) or set created_by to USER_ID; ensure ORG_ID, USER_ID, and USER_ID_2 remain referenced so existing tests using headers/USER_ID still authenticate against ORG_ID.
🧹 Nitpick comments (1)
tests/updates.test.ts (1)
85-87: Make transient-429 detection schema-based instead of string-matchThe retry guard is currently brittle to JSON formatting differences. Parse once and check
error === 'on_premise_app'to avoid false negatives.Suggested patch
- const responseText = await response.text() - if (!responseText.includes('"error":"on_premise_app"')) { + const responseText = await response.text() + let errorCode: string | undefined + try { + errorCode = (JSON.parse(responseText) as { error?: string }).error + } + catch { + errorCode = undefined + } + if (errorCode !== 'on_premise_app') { throw new Error(`Unexpected update failure after channel mutation (${response.status}): ${responseText}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/updates.test.ts` around lines 85 - 87, Replace the brittle string-match check against responseText with a schema-based check: JSON.parse the responseText (or use await response.json() if available) and assert that the parsed object's error property === 'on_premise_app'; if parsing fails, fall back to the existing behavior that throws the Error with response.status and responseText. Update the assertion in tests/updates.test.ts (the block referencing responseText and response.status) to perform the parsed-object check so transient-429 detection doesn’t depend on JSON formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/organization-api.test.ts`:
- Around line 48-60: The seed grants ORG_ID only to USER_ID_2 which breaks tests
that use the default test user (USER_ID); update the seeding so the default test
user retains access: in the org creation/seed block (the code that sets
created_by and inserts into org_users via
getSupabaseClient().from('org_users').insert) also insert or upsert an org_users
row for USER_ID with appropriate user_right (e.g., 'super_admin' or same right
as USER_ID_2) or set created_by to USER_ID; ensure ORG_ID, USER_ID, and
USER_ID_2 remain referenced so existing tests using headers/USER_ID still
authenticate against ORG_ID.
---
Nitpick comments:
In `@tests/updates.test.ts`:
- Around line 85-87: Replace the brittle string-match check against responseText
with a schema-based check: JSON.parse the responseText (or use await
response.json() if available) and assert that the parsed object's error property
=== 'on_premise_app'; if parsing fails, fall back to the existing behavior that
throws the Error with response.status and responseText. Update the assertion in
tests/updates.test.ts (the block referencing responseText and response.status)
to perform the parsed-object check so transient-429 detection doesn’t depend on
JSON formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9183f82e-80d1-4174-8a85-c891a802ef46
📒 Files selected for processing (4)
supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sqltests/hashed-apikey-rls.test.tstests/organization-api.test.tstests/updates.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/hashed-apikey-rls.test.ts
- supabase/migrations/20260424091645_enforce_hashed_api_keys_on_rls_identity_path.sql
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e488e01ab7
ℹ️ 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".
|
1 participant
Summary (AI generated)
find_apikey_by_value()auth path used by PostgREST/RLScapgkeyrejection on the anon/RLS planeenforce_hashed_api_keys=trueMotivation (AI generated)
Repository security advisory
GHSA-6g74-8cpq-g2c8showed that hashed API key enforcement only applied on one auth plane. Direct PostgREST/RLS requests could still authenticate with a legacy plaintext API key through the shared lookup path, which bypassed the intended org security control.Business Impact (AI generated)
This closes a high-severity authentication bypass on the direct Supabase/PostgREST plane. Organizations that enable hashed API key enforcement now get consistent behavior across both backend and RLS access paths, reducing the chance of silent policy bypass in production.
Test Plan (AI generated)
bun lintbunx eslint tests/hashed-apikey-rls.test.tsbun run supabase:db:resetbun run supabase:with-env -- bunx vitest run tests/hashed-apikey-rls.test.ts -t "enforce_hashed_api_keys blocks plaintext capgkey auth on the RLS plane" --reporter verbose --testTimeout 30000Generated with AI
Summary by CodeRabbit
New Features
Tests