Harden get_identity_apikey_only RPC permissions#1765
Conversation
📝 WalkthroughWalkthroughThis PR adds database security hardening to the Changes
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.0.4)supabase/migrations/20260308120000_hardening_get_identity_apikey_only_rpcs.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: 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: a8740dd3bd
ℹ️ 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".
| import { describe, expect, it } from 'vitest' | ||
| import { APIKEY_TEST_ALL } from './test-utils' | ||
|
|
||
| const SUPABASE_URL = env.SUPABASE_URL as string |
There was a problem hiding this comment.
This test uses env.SUPABASE_URL directly, but the suite already documents and works around a real CI issue where localhost is resolved to ::1 while Supabase is bound to 127.0.0.1 (see the normalization helpers in tests/test-utils.ts). In environments where SUPABASE_URL is http://localhost:..., these RPC calls can fail with connection errors unrelated to permissions, making this new regression test flaky or consistently failing.
Useful? React with 👍 / 👎.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/get-identity-apikey-only-rpc.test.ts`:
- Around line 2-4: Reorder the top imports so built-in modules come first: move
"import { env } from 'node:process'" before the external import "import {
createClient } from '@supabase/supabase-js'"; leave the test imports ("import {
describe, expect, it } from 'vitest'") after the external import so the
file-level import order satisfies the ESLint builtin→external→test pattern.
- Around line 13-47: Add a new concurrent test that asserts the authenticated
role is denied RPC access: create a Supabase client that represents a logged-in
user (use createClient and set an authenticated JWT/session so the client acts
as the authenticated role), include the same global header { capgkey:
APIKEY_TEST_ALL }, call rpc('get_identity_apikey_only', { keymode: keyModes })
and assert data is null and error is not null with error.message containing
'permission denied' (mirroring the anon test structure but using an
authenticated session instead of SUPABASE_ANON_KEY or SUPABASE_SERVICE_KEY).
- Line 11: The test fails because keyModes is declared as a readonly tuple with
"as const" and cannot be passed into the RPC parameter typed as
Database["public"]["Enums"]["key_mode"][]; remove the "as const" so keyModes is
a mutable array whose element type matches
Database["public"]["Enums"]["key_mode"], or explicitly type it as
Database["public"]["Enums"]["key_mode"][] (update the const declaration of
keyModes used by the RPC calls in get-identity-apikey-only-rpc.test.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c36e1c99-4967-4a6c-98b3-b162b0b924a3
📒 Files selected for processing (2)
supabase/migrations/20260308120000_hardening_get_identity_apikey_only_rpcs.sqltests/get-identity-apikey-only-rpc.test.ts
| import { createClient } from '@supabase/supabase-js' | ||
| import { env } from 'node:process' | ||
| import { describe, expect, it } from 'vitest' |
There was a problem hiding this comment.
Fix the import order to satisfy the current lint rule.
ESLint is already flagging this hunk because the builtin node:process import needs to come before @supabase/supabase-js.
Proposed fix
-import { createClient } from '@supabase/supabase-js'
import { env } from 'node:process'
+import { createClient } from '@supabase/supabase-js'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { createClient } from '@supabase/supabase-js' | |
| import { env } from 'node:process' | |
| import { describe, expect, it } from 'vitest' | |
| import { env } from 'node:process' | |
| import { createClient } from '@supabase/supabase-js' | |
| import { describe, expect, it } from 'vitest' |
🧰 Tools
🪛 ESLint
[error] 3-3: Expected "node:process" (builtin) to come before "@supabase/supabase-js" (external).
(perfectionist/sort-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/get-identity-apikey-only-rpc.test.ts` around lines 2 - 4, Reorder the
top imports so built-in modules come first: move "import { env } from
'node:process'" before the external import "import { createClient } from
'@supabase/supabase-js'"; leave the test imports ("import { describe, expect, it
} from 'vitest'") after the external import so the file-level import order
satisfies the ESLint builtin→external→test pattern.
| const SUPABASE_ANON_KEY = env.SUPABASE_ANON_KEY as string | ||
| const SUPABASE_SERVICE_KEY = (env.SUPABASE_SERVICE_KEY || env.SUPABASE_SERVICE_ROLE_KEY || env.SERVICE_ROLE_KEY) as string | ||
|
|
||
| const keyModes = ['all', 'read', 'write'] as const |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/get-identity-apikey-only-rpc.test.ts | head -60Repository: Cap-go/capgo
Length of output: 2316
🏁 Script executed:
# Find the RPC type definitions to understand what the function expects
rg -A 5 "get_identity_apikey_only" --type ts --type tsx | head -50Repository: Cap-go/capgo
Length of output: 83
🏁 Script executed:
# Check if there's a types file for Supabase
fd -e ts -e tsx -p "*supabase*" | grep -i type | head -20Repository: Cap-go/capgo
Length of output: 345
🏁 Script executed:
# Look for Database type definitions
rg "Database\['public'\]\['Enums'\]\['key_mode'\]" --type ts --type tsxRepository: Cap-go/capgo
Length of output: 83
🏁 Script executed:
# Find the Supabase types file
fd -t f "supabase.types" | head -5Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Search for get_identity_apikey_only RPC definition
rg "get_identity_apikey_only" -A 10 -B 2 --type tsRepository: Cap-go/capgo
Length of output: 5273
🏁 Script executed:
# Look for keymode parameter expectations in the types
rg "keymode" --type ts -B 5 -A 5Repository: Cap-go/capgo
Length of output: 2818
🏁 Script executed:
# Check the actual Supabase types definition to understand RPC parameters
find . -name "supabase.types.ts" -exec cat {} \; 2>/dev/null | head -200Repository: Cap-go/capgo
Length of output: 5566
🏁 Script executed:
# Get more of the supabase types file to find RPC function definitions
cat src/types/supabase.types.ts | grep -A 20 "get_identity_apikey_only" | head -40Repository: Cap-go/capgo
Length of output: 638
🏁 Script executed:
# Search for the key_mode enum definition
rg "key_mode" src/types/supabase.types.ts -A 3 -B 3Repository: Cap-go/capgo
Length of output: 4304
🏁 Script executed:
# Look for Enums in supabase types to see all key_mode values
rg "Enums.*:" src/types/supabase.types.ts -A 50 | head -100Repository: Cap-go/capgo
Length of output: 2956
🏁 Script executed:
# Check if there are other places where key_mode values are used with "upload"
rg "upload.*key_mode\|key_mode.*upload" --type ts -B 2 -A 2Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Check the hashed-apikey-rls.test.ts to see how they construct the key_mode array
rg "key_mode\|keymode" tests/hashed-apikey-rls.test.ts -B 2 -A 2Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Let's check if the test is actually compiling or if there are TypeScript errors
cd tests && ls -la get-identity-apikey-only-rpc.test.tsRepository: Cap-go/capgo
Length of output: 141
🏁 Script executed:
# Search for any TypeScript configuration or tsconfig
fd tsconfig.jsonRepository: Cap-go/capgo
Length of output: 69
🏁 Script executed:
# Check if there's a way to see if the tests are currently passing or failing
cat package.json | grep -A 5 "test"Repository: Cap-go/capgo
Length of output: 2713
🏁 Script executed:
# Check the exact enum values for key_mode
rg "key_mode:.*\[" src/types/supabase.types.ts -A 1Repository: Cap-go/capgo
Length of output: 158
🏁 Script executed:
# Let's verify the complete context - is "upload" used elsewhere in RPC tests
rg "upload" tests/*.test.ts -B 2 -A 2Repository: Cap-go/capgo
Length of output: 45566
🏁 Script executed:
# Check if there are any notes about what key modes should be tested
git log --oneline tests/get-identity-apikey-only-rpc.test.ts | head -10Repository: Cap-go/capgo
Length of output: 131
🏁 Script executed:
# Let me look at what values are actually needed - check if there's a reason to exclude "upload"
rg "key_mode.*upload\|upload.*key_mode" tests/ -B 3 -A 3 | head -60Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Check if the test might be intentionally testing subset of modes
git log -1 --format="%B" tests/get-identity-apikey-only-rpc.test.tsRepository: Cap-go/capgo
Length of output: 123
🏁 Script executed:
# Check hashed-apikey-rls.test.ts more closely to see if they test all modes
rg "get_identity_apikey_only.*all.*write.*read.*upload" tests/hashed-apikey-rls.test.ts -B 2 -A 2Repository: Cap-go/capgo
Length of output: 486
🏁 Script executed:
# Let me check if there are actual compilation errors by looking at recent test runs or CI config
fd -t f ".github" | head -5Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Check if there's any evidence the tests are currently failing
git log --oneline -n 20 tests/get-identity-apikey-only-rpc.test.tsRepository: Cap-go/capgo
Length of output: 131
🏁 Script executed:
# Let's verify the type mismatch more explicitly - check what TypeScript would infer
cat -n tests/get-identity-apikey-only-rpc.test.ts | sed -n '1,25p'Repository: Cap-go/capgo
Length of output: 1252
🏁 Script executed:
# Check if there's a reason the test uses only 3 modes (all, read, write) and not upload
rg "get_identity_apikey_only" supabase/functions -A 10 -B 2 | head -40Repository: Cap-go/capgo
Length of output: 1084
🏁 Script executed:
# Check database function definition to understand if all 4 modes should be tested
fd -t f "*.sql" | xargs grep -l "get_identity_apikey_only" 2>/dev/null | head -3Repository: Cap-go/capgo
Length of output: 340
🏁 Script executed:
# Find SQL files with proper syntax
fd -e sql | xargs grep -l "get_identity_apikey_only" 2>/dev/null | head -3Repository: Cap-go/capgo
Length of output: 228
🏁 Script executed:
# Check if there's a reason to test with only 3 modes vs all 4
rg "get_identity_apikey_only" supabase/ -B 5 -A 15 | grep -A 10 "function\|FUNCTION" | head -50Repository: Cap-go/capgo
Length of output: 3820
🏁 Script executed:
# Check the base migration to understand the function signature
grep -A 30 "CREATE OR REPLACE FUNCTION get_identity_apikey_only" supabase/migrations/20250530233128_base.sql | head -35Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Search more broadly for the function definition
rg "get_identity_apikey_only" supabase/migrations/ -B 2 -A 20 | head -60Repository: Cap-go/capgo
Length of output: 5973
🏁 Script executed:
# Let's also verify the exact error - check if TypeScript would actually reject this
python3 << 'EOF'
# Verify the type incompatibility explanation
# In TypeScript:
# 'as const' creates: readonly ["all", "read", "write"]
# The parameter expects: ("all" | "read" | "write" | "upload")[]
# A readonly tuple is NOT assignable to a mutable array in strict mode
print("Type Analysis:")
print("================")
print("Line 11: const keyModes = ['all', 'read', 'write'] as const")
print("")
print("TypeScript inference:")
print(" typeof keyModes = readonly ['all', 'read', 'write']")
print("")
print("RPC parameter type:")
print(" keymode: ('all' | 'read' | 'write' | 'upload')[]")
print("")
print("Type incompatibility:")
print(" - readonly ['all', 'read', 'write'] cannot be assigned to ('all' | 'read' | 'write' | 'upload')[]")
print(" - Error: TS4104 - readonly tuple is not assignable to mutable array")
print("")
print("Proposed fix removes 'as const' and uses explicit type:")
print(" const keyModes: ('all' | 'read' | 'write' | 'upload')[] = ['all', 'read', 'write']")
print("")
print("This creates a mutable array that matches the parameter type")
EOFRepository: Cap-go/capgo
Length of output: 693
Remove as const to fix TS4104 type incompatibility with the RPC parameter.
The readonly tuple created by as const cannot be assigned to the mutable array parameter keymode: Database["public"]["Enums"]["key_mode"][]. This causes compilation failures at lines 19, 31, and 43.
Proposed fix
-const keyModes = ['all', 'read', 'write'] as const
+const keyModes: Database['public']['Enums']['key_mode'][] = ['all', 'read', 'write']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const keyModes = ['all', 'read', 'write'] as const | |
| const keyModes: Database['public']['Enums']['key_mode'][] = ['all', 'read', 'write'] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/get-identity-apikey-only-rpc.test.ts` at line 11, The test fails
because keyModes is declared as a readonly tuple with "as const" and cannot be
passed into the RPC parameter typed as
Database["public"]["Enums"]["key_mode"][]; remove the "as const" so keyModes is
a mutable array whose element type matches
Database["public"]["Enums"]["key_mode"], or explicitly type it as
Database["public"]["Enums"]["key_mode"][] (update the const declaration of
keyModes used by the RPC calls in get-identity-apikey-only-rpc.test.ts).
| describe('get_identity_apikey_only RPC permissions', () => { | ||
| it.concurrent('denies anonymous RPC access', async () => { | ||
| const supabaseAnon = createClient<Database>(SUPABASE_URL, SUPABASE_ANON_KEY, { | ||
| global: { headers: { capgkey: APIKEY_TEST_ALL } }, | ||
| }) | ||
|
|
||
| const { data, error } = await supabaseAnon.rpc('get_identity_apikey_only', { keymode: keyModes }) | ||
|
|
||
| expect(data).toBeNull() | ||
| expect(error).not.toBeNull() | ||
| expect(error?.message).toContain('permission denied') | ||
| }) | ||
|
|
||
| it.concurrent('allows service role RPC access', async () => { | ||
| const supabaseServiceRole = createClient<Database>(SUPABASE_URL, SUPABASE_SERVICE_KEY, { | ||
| global: { headers: { capgkey: APIKEY_TEST_ALL } }, | ||
| }) | ||
|
|
||
| const { data, error } = await supabaseServiceRole.rpc('get_identity_apikey_only', { keymode: keyModes }) | ||
|
|
||
| expect(error).toBeNull() | ||
| expect(typeof data).toBe('string') | ||
| expect((data as string)).toMatch(/^[0-9a-fA-F-]{36}$/) | ||
| }) | ||
|
|
||
| it.concurrent('returns null for invalid API key on service role', async () => { | ||
| const supabaseServiceRole = createClient<Database>(SUPABASE_URL, SUPABASE_SERVICE_KEY, { | ||
| global: { headers: { capgkey: '00000000-0000-0000-0000-000000000000' } }, | ||
| }) | ||
|
|
||
| const { data, error } = await supabaseServiceRole.rpc('get_identity_apikey_only', { keymode: keyModes }) | ||
|
|
||
| expect(error).toBeNull() | ||
| expect(data).toBeNull() | ||
| }) |
There was a problem hiding this comment.
Add a denial test for the authenticated role.
This suite proves anon is blocked, but the migration also revokes EXECUTE from authenticated. Without a logged-in-session case here, a future re-grant to authenticated users would slip past the regression coverage.
Based on learnings: in this repository, API keys authenticate through the authenticated role via JWT, not the anon role.
🧰 Tools
🪛 ESLint
[error] 35-35: The character class(es) '[0-9a-fA-F-]' can be simplified using the i flag.
(regexp/use-ignore-case)
🪛 GitHub Actions: Run tests
[error] 19-19: TypeScript error TS4104: The type 'readonly ["all", "read", "write"]' is 'readonly' and cannot be assigned to the mutable type '("all" | "read" | "write" | "upload")[]'.
[error] 31-31: TypeScript error TS4104: The type 'readonly ["all", "read", "write"]' is 'readonly' and cannot be assigned to the mutable type '("all" | "read" | "write" | "upload")[]'.
[error] 43-43: TypeScript error TS4104: The type 'readonly ["all", "read", "write"]' is 'readonly' and cannot be assigned to the mutable type '("all" | "read" | "write" | "upload")[]'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/get-identity-apikey-only-rpc.test.ts` around lines 13 - 47, Add a new
concurrent test that asserts the authenticated role is denied RPC access: create
a Supabase client that represents a logged-in user (use createClient and set an
authenticated JWT/session so the client acts as the authenticated role), include
the same global header { capgkey: APIKEY_TEST_ALL }, call
rpc('get_identity_apikey_only', { keymode: keyModes }) and assert data is null
and error is not null with error.message containing 'permission denied'
(mirroring the anon test structure but using an authenticated session instead of
SUPABASE_ANON_KEY or SUPABASE_SERVICE_KEY).
1 participant
Summary
Test plan
Screenshots
Checklist
bun run lint:backend && bun run lint.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests