Move API key creation to server-side RPC#1581
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughMigrated API key generation from client-side to server-side by replacing local UUID generation and direct database inserts across frontend components with Supabase Edge Function invocations. Added new Postgres RPC functions ( Changes
Sequence DiagramsequenceDiagram
participant Vue as Vue Component
participant EF as Edge Function
participant RPC as Postgres RPC
participant DB as Database
rect rgba(100, 150, 255, 0.5)
Note over Vue,DB: Old Flow (Client-Side Generation)
Vue->>Vue: Generate UUID locally
Vue->>DB: Upsert to apikeys table
DB-->>Vue: Confirm insert
end
rect rgba(100, 200, 100, 0.5)
Note over Vue,DB: New Flow (Server-Side Generation)
Vue->>EF: invoke('apikey', {name, mode})
EF->>RPC: create_apikey_v2(name, mode, ...)
RPC->>DB: Generate key + insert
DB-->>RPC: Return created apikey
RPC-->>EF: Return apikey data
EF-->>Vue: Return apikey + plaintext key
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: cf9bf0c50e
ℹ️ 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".
| ) RETURNS public.apikeys | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = '' | ||
| AS $$ |
There was a problem hiding this comment.
The new create_apikey_v2 function runs as SECURITY DEFINER, which bypasses RLS policies such as Prevent non 2FA access on public.apikeys (see supabase/migrations/20250530233128_base.sql around the policy definition). Previously, API key creation went through normal inserts and was blocked when public.verify_mfa() failed for authenticated users. With the RPC, there is no explicit MFA check, so users without verified MFA can create keys, weakening the 2FA gate that existed before. Consider explicitly enforcing public.verify_mfa() (or equivalent) inside the function or reintroducing RLS enforcement for this path.
Useful? React with 👍 / 👎.
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = '' | ||
| AS $$ |
There was a problem hiding this comment.
The regenerate_apikey RPC is also SECURITY DEFINER, so it bypasses the same public.apikeys RLS policies that require public.verify_mfa() for authenticated users. Before this change, regeneration updated the row via standard updates and would have been blocked by the 2FA restrictive policy; now any authenticated user with access can regenerate a key without passing MFA. Add an explicit MFA check (or re-enable RLS) in the function to preserve the prior 2FA enforcement.
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
🤖 Fix all issues with AI agents
In `@src/pages/ApiKeys.vue`:
- Around line 538-544: The current supabase.functions.invoke call is using an
invalid path option; update the invocation of
supabase.functions.invoke('apikey', ...) to remove the path property and instead
include the apikey id in the request body (e.g., body: { regenerate: true, id:
apikey.id }) so the edge function receives the id via payload, and then update
the backend handler (put.ts) to read the id from the request body when URL
params are absent (or create a dedicated apikey/regenerate endpoint) so
regeneration works without path-based routing.
In `@supabase/functions/_backend/public/apikey/put.ts`:
- Around line 140-148: The TypeScript types for the Supabase RPC
"regenerate_apikey" were not updated, causing type mismatches in the put
handler; regenerate your Supabase types (the RPC definitions) the same way you
did for post.ts so the call in the put handler (the .rpc('regenerate_apikey', {
p_apikey_id: existingApikey.id }) and the destructured
regeneratedApikey/regenerateError) has correct typings; run your Supabase type
generation command (e.g., supabase gen types or your project codegen script),
commit the updated types, and ensure the regenerated types are imported/used by
this file so the RPC return type and error are correctly typed.
- Around line 56-57: The code fails when limited_to_apps or limited_to_orgs is
undefined; change the conditional assignments to use optional chaining when
checking length so they only access .length if defined — e.g., set
limited_to_apps: limited_to_apps?.length > 0 ? limited_to_apps : undefined and
limited_to_orgs: limited_to_orgs?.length > 0 ? limited_to_orgs : undefined (also
ensure the parameters/variables are typed as array | undefined so the compiler
recognizes the optional case).
🧹 Nitpick comments (1)
supabase/functions/_backend/public/apikey/post.ts (1)
55-63: Consider using reassignment instead of splice for clarity.Using
splice(0, arr.length, newValue)to replace array contents is non-obvious. Since these are local variables, simple reassignment would be clearer.♻️ Suggested refactor for clarity
if (orgId) { const { data: org, error } = await supabase.from('orgs').select('*').eq('id', orgId).single() if (!org || error) { throw quickError(404, 'org_not_found', 'Org not found', { supabaseError: error }) } - limitedToOrgs.splice(0, limitedToOrgs.length, org.id) + limitedToOrgs.length = 0 + limitedToOrgs.push(org.id) allOrgIds = [org.id] } if (appId) { const { data: app, error } = await supabase.from('apps').select('*').eq('id', appId).single() if (!app || error) { throw quickError(404, 'app_not_found', 'App not found', { supabaseError: error }) } - limitedToApps.splice(0, limitedToApps.length, app.app_id) + limitedToApps.length = 0 + limitedToApps.push(app.app_id) }Or simply reassign since these are passed by value from the body:
- limitedToOrgs.splice(0, limitedToOrgs.length, org.id) + body.limited_to_orgs = [org.id]
| if (regenerate) { | ||
| const { data: regeneratedApikey, error: regenerateError } = await supabase | ||
| .rpc('regenerate_apikey', { p_apikey_id: existingApikey.id }) | ||
| .single() | ||
| if (regenerateError || !regeneratedApikey) { | ||
| throw quickError(500, 'failed_to_regenerate_apikey', 'Failed to regenerate API key', { supabaseError: regenerateError }) | ||
| } | ||
| return c.json(regeneratedApikey) | ||
| } |
There was a problem hiding this comment.
TypeScript types need regeneration for regenerate_apikey RPC.
Same issue as post.ts - the RPC function types need to be regenerated after applying the migration.
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 142-142: TypeScript error: Argument of type '"regenerate_apikey"' is not assignable to parameter of the union type.
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/public/apikey/put.ts` around lines 140 - 148, The
TypeScript types for the Supabase RPC "regenerate_apikey" were not updated,
causing type mismatches in the put handler; regenerate your Supabase types (the
RPC definitions) the same way you did for post.ts so the call in the put handler
(the .rpc('regenerate_apikey', { p_apikey_id: existingApikey.id }) and the
destructured regeneratedApikey/regenerateError) has correct typings; run your
Supabase type generation command (e.g., supabase gen types or your project
codegen script), commit the updated types, and ensure the regenerated types are
imported/used by this file so the RPC return type and error are correctly typed.
There was a problem hiding this comment.
Pull request overview
This PR enhances API key security by moving key generation from client-side to server-side RPC functions. Previously, API keys were generated in the frontend using crypto.randomUUID() and sent to the database, which posed a security risk. Now, keys are generated server-side in PostgreSQL functions with proper access controls.
Changes:
- Server-side RPC functions (
create_apikey_v2andregenerate_apikey) now handle all API key generation - Column-level permissions prevent direct manipulation of key/key_hash fields by clients
- Frontend and backend updated to use new RPC endpoints
- Database constraint ensures at least one of key or key_hash is present
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/migrations/20260205120000_apikey_server_generation.sql | Adds RPC functions for secure server-side key generation, revokes direct client permissions on key columns, and adds constraint |
| supabase/functions/_backend/public/apikey/put.ts | Adds regenerate parameter to PUT endpoint that calls regenerate_apikey RPC |
| supabase/functions/_backend/public/apikey/post.ts | Refactored to call create_apikey_v2 RPC instead of direct database insert |
| src/pages/ApiKeys.vue | Updated to use backend API for key creation/regeneration instead of client-side generation |
| src/components/dashboard/StepsBundle.vue | Updated to use backend API endpoint for key creation |
| src/components/dashboard/StepsBuild.vue | Updated to use backend API endpoint for key creation |
| src/components/dashboard/StepsApp.vue | Updated to use backend API endpoint for key creation |
Comments suppressed due to low confidence (2)
supabase/functions/_backend/public/apikey/put.ts:148
- The regenerate functionality is not covered by tests. Consider adding test cases to verify that the regenerate parameter correctly regenerates both hashed and plain-text API keys, returns the new key value, and maintains the correct key/key_hash state in the database.
}
updatedApikey = updatedData
}
return c.json(updatedApikey)
})
export default app
supabase/migrations/20260205120000_apikey_server_generation.sql:161
- The GRANT EXECUTE permissions grant access to 'anon' role, which allows unauthenticated users to call these functions. However, both functions use get_identity which requires authentication, so unauthenticated calls will fail with 'not_authenticated'. Consider removing 'anon' from the GRANT statements since it serves no functional purpose and may be confusing.
| if (!hasUpdates && !regenerate) { | ||
| throw simpleError('no_valid_fields_provided_for_update', 'No valid fields provided for update. Provide name, mode, limited_to_apps, limited_to_orgs, or expires_at.') | ||
| } |
There was a problem hiding this comment.
The test case "update api key with no valid fields" expects a 500 status with error 'failed_to_update_apikey', but the new code will return a 400 status with error 'no_valid_fields_provided_for_update'. This test will fail with the new implementation and needs to be updated to match the new validation behavior.
| ALTER TABLE public.apikeys | ||
| ALTER COLUMN key DROP DEFAULT; |
There was a problem hiding this comment.
Setting a DEFAULT value on the key column will only affect future INSERT operations. Existing rows with NULL keys will remain NULL. Consider whether this is the intended behavior, especially if there are any existing rows where both key and key_hash are NULL (which would now violate the new constraint). The constraint check at lines 4-16 will prevent adding the constraint if it already exists, but it does not verify that existing data satisfies the new constraint.
riderx
force-pushed
the
riderx/api-key-auth
branch
from
02731e2 to
c4015e4
Compare
riderx
force-pushed
the
riderx/api-key-auth
branch
from
bc8b608 to
2d629d5
Compare
|
Summary (AI generated)
Test plan (AI generated)
bun lintbun lint:backendScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.accordingly.
my tests
Generated with AI
Summary by CodeRabbit
New Features
Improvements