fix(apikey): server-side key generation + hashed keys#1586
Conversation
📝 WalkthroughWalkthroughThis PR migrates API key generation from client-side to server-side operations, introducing secure hashed key storage. It adds database functions for key creation and regeneration, refactors frontend components to use a new service layer, updates edge function handlers, and expands test coverage and localization across 15+ languages. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Vue Component
participant Service as apikeys.ts
participant EdgeFn as Edge Function<br/>(POST /apikey)
participant RPC as PostgreSQL<br/>RPC Function
participant DB as PostgreSQL<br/>Database
User->>Frontend: Trigger API key creation
Frontend->>Service: createDefaultApiKey(supabase, name)
Service->>EdgeFn: POST /apikey {name, mode: 'all'}
EdgeFn->>EdgeFn: Validate input & auth
alt Hashed Key Path
EdgeFn->>RPC: Call create_hashed_apikey(...)
RPC->>RPC: Generate UUID key
RPC->>RPC: Hash key (SHA-256)
RPC->>DB: Insert with hashed key
DB-->>RPC: Return apikey record
RPC-->>EdgeFn: Return record + plaintext
else Plaintext Key Path
EdgeFn->>DB: Insert with plaintext key
DB-->>EdgeFn: Return record
end
EdgeFn-->>Service: Return apikey data
Service-->>Frontend: Return result
Frontend->>Frontend: Display key (one-time)
Frontend->>Frontend: Store hashed/reference
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.0)supabase/migrations/20260206120000_apikey_server_generation.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: 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements server-side API key generation with support for hashed (secure) keys that are only displayed once to users. The changes improve security by moving key generation logic from the client to the database layer via triggers and stored functions. Additionally, the PR hardens Stripe integration checks to handle CI/test environments more gracefully and improves the app creation trigger to handle partial webhook payloads.
Changes:
- Server-side API key generation using database triggers and stored functions
- Support for hashed API keys (SHA-256) with one-time plaintext display
- API key regeneration endpoint with support for both hashed and plain-text keys
- Improved Stripe configuration validation using regex pattern matching
- Enhanced app creation trigger to handle missing
owner_orgin webhook payloads
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/migrations/20260205120000_apikey_server_generation.sql | Adds triggers and functions for server-side key generation, hashing, and regeneration |
| supabase/functions/_backend/public/apikey/post.ts | Refactored to use server-side key generation for both hashed and plain-text keys |
| supabase/functions/_backend/public/apikey/put.ts | Added regenerate parameter support with dual endpoint routes (with/without :id) |
| supabase/functions/_backend/utils/utils.ts | Added isStripeConfigured() with regex validation for Stripe keys |
| supabase/functions/_backend/utils/stripe.ts | Replaced existInEnv checks with isStripeConfigured() throughout |
| supabase/functions/_backend/triggers/on_app_create.ts | Added defensive logic to fetch owner_org from database when missing from webhook |
| src/pages/ApiKeys.vue | Updated to use backend API for key creation and regeneration |
| src/services/apikeys.ts | New service function for creating default API keys |
| src/components/dashboard/*.vue | Updated onboarding flows to use new API key service |
| tests/apikeys.test.ts | Updated test to expect correct error code (400 vs 500) |
| supabase/functions/_backend/utils/supabase.types.ts | Added TypeScript types for new database functions |
| src/types/supabase.types.ts | Added TypeScript types for new database functions |
| } | ||
|
|
||
| const { data: updatedData, error: updateError } = await supabase | ||
| .from('apikeys') |
There was a problem hiding this comment.
The magic string 'regenerate' used here is confusing and unnecessary. The database trigger apikeys_force_server_key() will regenerate the key whenever the key column value changes (when it's not NULL and different from the old value). Any non-null value would work here.
Consider either:
- Adding a comment explaining this implicit behavior relies on the database trigger, or
- Using a more explicit approach like passing a flag to indicate regeneration intent, though the current trigger-based approach does work correctly.
| .from('apikeys') | |
| .from('apikeys') | |
| // Any non-null value different from the current key will trigger the | |
| // `apikeys_force_server_key()` database trigger to regenerate the key. | |
| // We use the literal string 'regenerate' here purely as a placeholder; | |
| // the final key returned below is the value generated by the trigger. |
| END IF; | ||
|
|
||
| IF TG_OP = 'UPDATE' THEN | ||
| IF NEW.key IS NOT DISTINCT FROM OLD.key AND NEW.key_hash IS NOT DISTINCT FROM OLD.key_hash THEN |
There was a problem hiding this comment.
The trigger logic at lines 44-46 has a subtle issue. When both key and key_hash haven't changed, it returns early. However, this means that if a user tries to regenerate a key but accidentally sends the same key value in the UPDATE, the regeneration will be silently skipped.
While this is unlikely to happen in practice (since the frontend doesn't send the old key value), it could cause confusion during manual database operations or when using the API directly. Consider whether this early return should check if this is an explicit regeneration request rather than just comparing old vs new values.
| IF NEW.key IS NOT DISTINCT FROM OLD.key AND NEW.key_hash IS NOT DISTINCT FROM OLD.key_hash THEN | |
| IF current_setting('capgo.force_regenerate_apikey', true) IS DISTINCT FROM 'true' | |
| AND NEW.key IS NOT DISTINCT FROM OLD.key | |
| AND NEW.key_hash IS NOT DISTINCT FROM OLD.key_hash THEN |
| limited_to_apps?: string[] | ||
| limited_to_orgs?: string[] | ||
| expires_at?: string | null | ||
| regenerate?: boolean |
There was a problem hiding this comment.
The new regenerate parameter for API key regeneration lacks test coverage. This is a critical security feature that should have comprehensive tests covering:
- Regenerating a plain-text API key (verifying the key changes)
- Regenerating a hashed API key (verifying the key changes and remains hashed)
- Combining regenerate with other update operations (e.g., regenerate + name change)
- Verifying the old key no longer works after regeneration
- Edge cases like regenerating a non-existent key
According to the coding guidelines, test coverage should be added for new behavior when similar functions in the same file have tests. The existing tests extensively cover PUT /apikey operations, so regeneration should also be tested.
| // Extract the plaintext key for one-time display before clearing it | ||
| const plainKeyForDisplay = wasHashed ? data.key as string | undefined : undefined |
There was a problem hiding this comment.
When regenerating a plain-text (non-hashed) API key, the new key is generated but not displayed to the user. The plainKeyForDisplay is only set for hashed keys (line 553), but plain-text keys should also show the newly generated key to the user since:
- The old key is no longer valid after regeneration
- The new key is in
data.keyand needs to be displayed/copied - Without displaying it, users have no way to know what their new plain-text key is
Consider either:
- Showing the new key for both hashed and non-hashed keys, or
- Adding UI indication that the key has been updated and is visible in the table for non-hashed keys
| // Extract the plaintext key for one-time display before clearing it | |
| const plainKeyForDisplay = wasHashed ? data.key as string | undefined : undefined | |
| // Extract the plaintext key for one-time display before optionally clearing it | |
| const plainKeyForDisplay = data.key as string | undefined |
riderx
force-pushed
the
riderx/ci-fix-triggers
branch
from
d45c7e0 to
71b541e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/functions/_backend/public/apikey/put.ts (1)
54-128:⚠️ Potential issue | 🟡 MinorHonor explicit empty org lists during policy validation.
You now treat empty arrays as meaningful, butlimited_to_orgs?.lengthtreats[]as false. When a caller clears orgs while settingexpires_at, validation runs against the old org list.🛠️ Proposed fix
- const orgsToValidate = limited_to_orgs?.length ? limited_to_orgs : (existingApikey.limited_to_orgs || []) + const orgsToValidate = limited_to_orgs !== undefined ? limited_to_orgs : (existingApikey.limited_to_orgs || []) @@ - if (expires_at !== undefined || limited_to_orgs?.length) { + if (expires_at !== undefined || limited_to_orgs !== undefined) {src/pages/ApiKeys.vue (1)
435-463:⚠️ Potential issue | 🟡 MinorAdd null guards when accessing
supabase.functions.invoke()responses in bothcreateApiKeyandregenerateKeyfunctions.After checking for errors, verify that
datais not null before dereferencingdata.key. If the edge function returns an empty response body,datacan be null and will cause a runtime error when accessing properties. Additionally, add a type check for thekeyproperty before assigning toplainKeyForDisplay.💡 Proposed fix for createApiKey
if (error) { + if (!data) { + console.error('Error creating API key: empty response') + toast.error('Failed to create API key') + return false + } } const createdKey = data as Database['public']['Tables']['apikeys']['Row'] if (isHashed && typeof createdKey.key === 'string') plainKeyForDisplay = createdKey.keyApply the same defensive pattern in the
regenerateKeyfunction (lines 538-569) wheredata.keyis accessed.
🤖 Fix all issues with AI agents
In `@src/pages/ApiKeys.vue`:
- Around line 538-569: The current PUT invoke handling uses data without
guarding against a null/undefined response; update the regenerate API key flow
(the block that calls supabase.functions.invoke('apikey') and uses variables
data, error, apikey, wasHashed, keys, and showOneTimeKeyModal) to defensively
check for missing data after the invoke (e.g., treat as failure when error is
truthy or data is falsy), log/notify and return in that case, and only proceed
to read data.key, clear data.key, update keys.value[idx], and call
showOneTimeKeyModal when data is confirmed non-null so subsequent usage is safe.
In `@supabase/functions/_backend/public/apikey/post.ts`:
- Around line 55-64: The code assumes limitedToOrgs and limitedToApps are arrays
before calling splice, which can throw TypeError for malformed inputs; before
the splice calls in the handler (where limitedToOrgs.splice(...) and
limitedToApps.splice(...) are used), validate each with Array.isArray(...) and
if not an array return a client error (e.g., quickError(400, 'invalid_payload',
...)) indicating the field must be an array; apply this check both for the org
branch (before limitedToOrgs.splice and setting allOrgIds) and the app branch
(before limitedToApps.splice) so malformed payloads return 400 instead of
causing a 500.
🧹 Nitpick comments (3)
supabase/functions/_backend/triggers/on_app_create.ts (1)
48-52: Consider a retry/alert path when owner_org can’t be resolved.
Skipping onboarding/default versions may leave the app without baseline rows; a retry or metric would prevent silent drift.supabase/migrations/20260205120000_apikey_server_generation.sql (1)
109-189: Consider derivinguser_idinside the RPCs for defense in depth.
Both RPCs acceptp_user_idfrom the caller. If any RLS rule becomes permissive, callers could pass another user’s ID. Usingauth.uid()inside the functions avoids that risk (call sites and GRANT signatures would need updating).🔒 Suggested hardening
-CREATE OR REPLACE FUNCTION public.create_hashed_apikey( - p_user_id uuid, - p_mode public.key_mode, +CREATE OR REPLACE FUNCTION public.create_hashed_apikey( + p_mode public.key_mode, p_name text, p_limited_to_orgs uuid[], p_limited_to_apps text[], p_expires_at timestamptz ) @@ - p_user_id, + auth.uid(), @@ -CREATE OR REPLACE FUNCTION public.regenerate_hashed_apikey( - p_apikey_id bigint, - p_user_id uuid +CREATE OR REPLACE FUNCTION public.regenerate_hashed_apikey( + p_apikey_id bigint ) @@ - WHERE id = p_apikey_id - AND user_id = p_user_id + WHERE id = p_apikey_id + AND user_id = auth.uid()supabase/functions/_backend/public/apikey/put.ts (1)
29-41: Type the handler context withContext<MiddlewareKeyVariables>.
This keeps request context types consistent across backend handlers and makesrequestId/auth/apikeyaccess explicit.As per coding guidelines, all Hono endpoint handlers must accept Context and use c.get('requestId'), c.get('apikey'), and c.get('auth') for request context.♻️ Proposed fix
-import type { Context } from 'hono' +import type { Context } from 'hono' +import type { MiddlewareKeyVariables } from '../../utils/hono_middleware.ts' @@ -async function handlePut(c: Context, idParam?: string) { +async function handlePut(c: Context<MiddlewareKeyVariables>, idParam?: string) {
riderx
force-pushed
the
riderx/ci-fix-triggers
branch
from
f2bedba to
2ac1812
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/pages/ApiKeys.vue`:
- Around line 448-456: The creation response lacks a null check for data, so
after the existing error check add a guard that verifies data exists before
accessing properties: if (!data) { console.error('Create API key returned empty
data'); toast.error('Failed to create API key'); return false } then proceed to
set createdKey = data and only read data.key into plainKeyForDisplay when
isHashed and data.key is present; reference variables/identifiers: data,
createdKey, plainKeyForDisplay, isHashed in ApiKeys.vue to locate and apply the
fix.
In `@supabase/migrations/20260205120000_apikey_server_generation.sql`:
- Around line 188-189: The SQL grants are overly permissive: remove GRANT
EXECUTE to anon for the SECURITY INVOKER functions
public.create_hashed_apikey(uuid, public.key_mode, text, uuid[], text[],
timestamptz) and public.regenerate_hashed_apikey(bigint, uuid) and ensure only
authenticated retains EXECUTE; additionally verify that any inner functions that
accept a user_id parameter are not granted to anon or authenticated (only the
no-argument wrapper functions should be public) so least-privilege and RLS
protections are preserved.
🧹 Nitpick comments (2)
supabase/functions/_backend/public/apikey/put.ts (2)
54-99: Move input validation before payload construction.Validation (Lines 76-95) runs after fields are already placed into
updateData(Lines 57-72). While the subsequentthrowprevents the bad payload from reaching the DB, placing validation first is clearer and less fragile if future changes add early-use ofupdateData.♻️ Suggested reordering
+ // --- Validate inputs first --- + if (name !== undefined && typeof name !== 'string') { + throw simpleError('name_must_be_a_string', 'Name must be a string') + } + + const validModes = Constants.public.Enums.key_mode + if (mode !== undefined && (typeof mode !== 'string' || !validModes.includes(mode as any))) { + throw simpleError('invalid_mode', `Invalid mode. Must be one of: ${validModes.join(', ')}`) + } + + if (limited_to_apps !== undefined && (!Array.isArray(limited_to_apps) || !limited_to_apps.every(item => typeof item === 'string'))) { + throw simpleError('limited_to_apps_must_be_an_array_of_strings', 'limited_to_apps must be an array of strings') + } + + if (limited_to_orgs !== undefined && (!Array.isArray(limited_to_orgs) || !limited_to_orgs.every(item => typeof item === 'string'))) { + throw simpleError('limited_to_orgs_must_be_an_array_of_strings', 'limited_to_orgs must be an array of strings') + } + + if (regenerate !== undefined && typeof regenerate !== 'boolean') { + throw simpleError('regenerate_must_be_boolean', 'regenerate must be a boolean') + } + - // Build update data from only explicitly-provided fields. - // Note: empty arrays are meaningful and should clear the list. + // --- Build update data from only explicitly-provided fields --- const updateData: Partial<Database['public']['Tables']['apikeys']['Update']> = {} ... - - if (name !== undefined && typeof name !== 'string') { ... } - const validModes = ... - ...
105-110: String-interpolated.or()filter is safe here thanks toisValidIdFormat, but consider the parameterized alternative.The PostgREST
.or()at Line 108 injectsresolvedIddirectly into the filter string. TheisValidIdFormatguard (UUID/numeric regex) prevents injection, but if the validation ever loosens, this becomes a vector. The Supabase JS client doesn't expose parameterized.or()easily, so this is acceptable for now — just keep the tight coupling in mind.
| GRANT EXECUTE ON FUNCTION public.create_hashed_apikey(uuid, public.key_mode, text, uuid[], text[], timestamptz) TO anon, authenticated; | ||
| GRANT EXECUTE ON FUNCTION public.regenerate_hashed_apikey(bigint, uuid) TO anon, authenticated; |
There was a problem hiding this comment.
Granting EXECUTE to anon is unnecessarily permissive.
Both functions are SECURITY INVOKER, so RLS and table-level grants will block unauthorized operations. However, granting to anon violates least-privilege — unauthenticated callers have no business creating or regenerating API keys. Consider granting only to authenticated.
🔒 Suggested fix
-GRANT EXECUTE ON FUNCTION public.create_hashed_apikey(uuid, public.key_mode, text, uuid[], text[], timestamptz) TO anon, authenticated;
-GRANT EXECUTE ON FUNCTION public.regenerate_hashed_apikey(bigint, uuid) TO anon, authenticated;
+GRANT EXECUTE ON FUNCTION public.create_hashed_apikey(uuid, public.key_mode, text, uuid[], text[], timestamptz) TO authenticated;
+GRANT EXECUTE ON FUNCTION public.regenerate_hashed_apikey(bigint, uuid) TO authenticated;Based on learnings: "The inner functions with user_id parameter should NOT be granted to anon/authenticated roles as this allows any user to query other users' organizations; only the no-argument wrapper functions should be public."
🤖 Prompt for AI Agents
In `@supabase/migrations/20260205120000_apikey_server_generation.sql` around lines
188 - 189, The SQL grants are overly permissive: remove GRANT EXECUTE to anon
for the SECURITY INVOKER functions public.create_hashed_apikey(uuid,
public.key_mode, text, uuid[], text[], timestamptz) and
public.regenerate_hashed_apikey(bigint, uuid) and ensure only authenticated
retains EXECUTE; additionally verify that any inner functions that accept a
user_id parameter are not granted to anon or authenticated (only the no-argument
wrapper functions should be public) so least-privilege and RLS protections are
preserved.
2ac1812 to
99c772c
Compare
riderx
force-pushed
the
riderx/ci-fix-triggers
branch
from
1af184a to
9ef70bd
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/functions/_backend/utils/plans.ts (1)
439-465:⚠️ Potential issue | 🟠 MajorGuard against null
customer_idnow that Stripe gating is removed.
checkPlanStatusOnlynow runs without Stripe config, but downstream calls assumeorg.customer_idis non-null (org.customer_id!). In on-prem/local scenarios this can be null, which risks updating allstripe_inforows withcustomer_id IS NULLor failing unexpectedly. Add a defensive guard (or update by org ID) before trial/plan updates.🛡️ Suggested guard
export async function checkPlanStatusOnly(c: Context, orgId: string, drizzleClient: ReturnType<typeof getDrizzleClient>): Promise<void> { // This cron task updates plan usage + exceeded flags based on DB state. // It must run even when Stripe is not configured (e.g. local tests / on-prem), // as it does not require Stripe API calls. const org = await getOrgWithCustomerInfo(c, orgId) + if (!org?.customer_id) { + cloudlog({ requestId: c.get('requestId'), message: 'checkPlanStatusOnly missing customer_id', orgId }) + return + }supabase/functions/_backend/triggers/on_app_create.ts (1)
101-129:⚠️ Potential issue | 🟠 MajorUse Drizzle ORM for app_versions and orgs queries.
Lines 101-129 still use
supabaseAdminfor the app_versions upsert and orgs lookup. Per the D1 migration guidelines, backend DB operations must usegetPgClient()/getDrizzleClient()fromutils/pg.ts. The file already demonstrates the correct pattern earlier (lines 38-49); apply the same approach here: create a new pg client, execute queries using the Drizzle schema frompostgres_schema.ts, and close the client in a finally block.
🤖 Fix all issues with AI agents
In `@messages/pt-br.json`:
- Around line 1500-1502: The two message keys "failed-to-create-api-key" and
"failed-to-regenerate-api-key" are still in English; translate them into
Portuguese and replace their values in messages/pt-br.json so the locale is
consistent—e.g. set "failed-to-create-api-key" to a Portuguese phrase like
"Falha ao criar a chave de API" and "failed-to-regenerate-api-key" to "Falha ao
regenerar a chave de API".
In `@supabase/functions/_backend/triggers/on_app_create.ts`:
- Around line 36-72: The drizzle query failure currently leaves appExists false
and makes the handler treat the app as deleted; update the catch block around
the drizzleClient.select call (where schema.apps is queried) to either rethrow
the error after logging so the trigger can retry (i.e., cloudlog(...); throw
error) or, if you prefer to continue, fall back to the webhook payload by
setting ownerOrg = record.owner_org and appExists = true before closing the
client; ensure closeClient(c, pg) still runs in finally and keep cloudlog of the
original error.
🧹 Nitpick comments (7)
messages/vi.json (1)
1501-1502: New keys break the file's alphabetical ordering convention.The rest of this file maintains strict alphabetical key order (other
failed-*keys like"failed-to-fetch-release-status"sit around lines 766–769). These two new keys should be placed there for consistency and to make future duplicate detection easier.Suggested placement
Move the two new entries between the existing
"failed-to-update-policy"and"fast-backward"keys (around line 769–770), keeping alphabetical order:"failed-to-update-policy": "Cập nhật chính sách thất bại", + "failed-to-create-api-key": "Không thể tạo khóa API", + "failed-to-regenerate-api-key": "Không thể tạo lại khóa API", "fast-backward": "Tua nhanh",And revert
"zip-bundle"to being the last entry (no trailing comma needed if nothing follows):- "zip-bundle": "Gói ứng dụng Zip", - "failed-to-create-api-key": "Không thể tạo khóa API", - "failed-to-regenerate-api-key": "Không thể tạo lại khóa API" + "zip-bundle": "Gói ứng dụng Zip"src/components/dashboard/StepsApp.vue (1)
218-229: This optimization is unnecessary for the current implementation; the plaintext key is correctly persisted in the database.The edge function does return the plaintext key in its response, but for non-hashed API keys (which is what the frontend creates), the key is also stored in the database. The subsequent
getKey()query successfully retrieves it, so the CLI command populates correctly. Capturing the response would eliminate a redundant database query, but it's not required for correctness.tests/apikeys.test.ts (2)
17-28: Consider usingit.concurrent()for parallelizable tests.All tests in this file use
it()instead ofit.concurrent(). The self-contained tests (those that create and clean up their own keys) are good candidates for concurrent execution. Tests relying on shared seeded IDs (10–13) would need dedicated seed data with unique prefixes to be safely parallelized.As per coding guidelines: "Design tests for parallel execution; use
it.concurrent()instead ofit()to run tests in parallel within the same file."
6-7: Consider prefixing test resource names with the test file name for traceability.The key names (e.g.,
'temp-plain-key-regenerate') and app name don't follow the naming convention guideline. While therandomUUID()suffix ensures uniqueness, prefixing withtest_apikeys_would aid debugging when inspecting shared resources.As per coding guidelines: "use unique naming conventions prefixed with the test file name (e.g.,
test_my_feature_user@capgo.app)."supabase/functions/_backend/public/apikey/put.ts (2)
55-100: Move input validation before buildingupdateData.The type/value checks at lines 77-96 run after
updateDatais already constructed at lines 55-73. While the thrown errors prevent any DB operation, readers encountering this code may assume the payload was already validated when they reachupdateData. Swapping the order would make the flow clearer.
141-183: Dual update + regenerate path runs as two separate transactions.When both
hasUpdatesandregenerateare true, lines 141-153 commit the field update in one PostgREST call, and lines 156-183 commit the key regeneration in a second call. If the regeneration call fails, the field update remains committed silently while the client receives an error.In practice the frontend only sends
{ id, regenerate: true }(no field updates), so this is unlikely to surface. However, adding a brief comment documenting the two-transaction semantics would help future maintainers if the API is ever used differently.supabase/migrations/20260206120000_apikey_server_generation.sql (1)
201-254: Inconsistent indentation in public wrapper functions.Lines 213-221 use tab indentation while lines 223-230 switch to spaces. Same issue appears in the
regenerate_hashed_apikeywrapper at lines 242-252.Also, the
COALESCEcalls at lines 227-228 duplicate the ones already present insidecreate_hashed_apikey_for_userat lines 154-155. Not harmful, but the redundancy could be removed.
| "zip-bundle": "Pacote de aplicativo ZIP", | ||
| "failed-to-create-api-key": "Failed to create API key", | ||
| "failed-to-regenerate-api-key": "Failed to regenerate API key" |
There was a problem hiding this comment.
Localize new pt-BR strings (currently English).
Please translate failed-to-create-api-key and failed-to-regenerate-api-key into Portuguese to keep the file consistent with the rest of the locale.
🤖 Prompt for AI Agents
In `@messages/pt-br.json` around lines 1500 - 1502, The two message keys
"failed-to-create-api-key" and "failed-to-regenerate-api-key" are still in
English; translate them into Portuguese and replace their values in
messages/pt-br.json so the locale is consistent—e.g. set
"failed-to-create-api-key" to a Portuguese phrase like "Falha ao criar a chave
de API" and "failed-to-regenerate-api-key" to "Falha ao regenerar a chave de
API".
| const { data, error } = await supabase.rpc('create_hashed_apikey', { | ||
| p_mode: mode, | ||
| p_name: name, | ||
| p_limited_to_orgs: limitedToOrgs, | ||
| p_limited_to_apps: limitedToApps, | ||
| p_expires_at: expiresAt, | ||
| }) | ||
| apikeyData = data | ||
| apikeyError = error | ||
| } | ||
| else { | ||
| const { data, error } = await supabase | ||
| .from('apikeys') | ||
| .insert({ | ||
| user_id: auth.userId, | ||
| key: null, | ||
| key_hash: null, | ||
| mode, | ||
| name, | ||
| limited_to_apps: limitedToApps, | ||
| limited_to_orgs: limitedToOrgs, | ||
| expires_at: expiresAt, | ||
| }) | ||
| .select() | ||
| .single() | ||
| apikeyData = data |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full file structure and context
find supabase/functions/_backend -name "post.ts" -o -name "apikey" -type d | head -20Repository: Cap-go/capgo
Length of output: 467
🏁 Script executed:
# Get the full post.ts file to understand the context
wc -l supabase/functions/_backend/public/apikey/post.tsRepository: Cap-go/capgo
Length of output: 109
🏁 Script executed:
# Read the relevant sections of the file
cat -n supabase/functions/_backend/public/apikey/post.ts | head -120Repository: Cap-go/capgo
Length of output: 5349
🏁 Script executed:
# Check utils/pg.ts to understand available functions
cat -n supabase/functions/_backend/utils/pg.tsRepository: Cap-go/capgo
Length of output: 50368
🏁 Script executed:
# Look for examples of getDrizzleClient/getPgClient usage in similar files
rg "getDrizzleClient|getPgClient" supabase/functions/_backend/ -A 5 -B 2 | head -80Repository: Cap-go/capgo
Length of output: 5872
🏁 Script executed:
# Check if postgress_schema.ts has apikeys table definition
cat -n supabase/functions/_backend/utils/postgress_schema.ts | grep -A 20 "apikeys"Repository: Cap-go/capgo
Length of output: 141
🏁 Script executed:
# Search for postgres schema file with correct spelling
find supabase/functions/_backend -name "*schema*" -type fRepository: Cap-go/capgo
Length of output: 108
🏁 Script executed:
# Check what supabaseWithAuth returns
rg "supabaseWithAuth" supabase/functions/_backend/utils/ -A 10 | head -50Repository: Cap-go/capgo
Length of output: 2987
🏁 Script executed:
# Look for similar POST endpoints to see patterns
fd "post.ts" supabase/functions/_backend/public/ | head -5Repository: Cap-go/capgo
Length of output: 317
🏁 Script executed:
# Check if any other similar endpoints have migrated to pg/drizzle
rg "getDrizzleClient\|getPgClient" supabase/functions/_backend/public/ -B 5 -A 10 | head -80Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Check the postgres_schema.ts file for apikeys table definition
cat -n supabase/functions/_backend/utils/postgres_schema.ts | grep -A 20 "apikeys"Repository: Cap-go/capgo
Length of output: 1215
🏁 Script executed:
# Check if there are any comments or TODOs about migration in post.ts
cat -n supabase/functions/_backend/public/apikey/post.ts | grep -i "TODO\|FIXME\|migration\|drizzle\|pg"Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Look at other POST endpoints to see if they use pg/drizzle
cat -n supabase/functions/_backend/public/app/post.ts | head -100Repository: Cap-go/capgo
Length of output: 2629
🏁 Script executed:
# Check if supabaseWithAuth is documented as a pattern or if it's deprecated
rg "supabaseWithAuth" supabase/functions/_backend/public/ --filesRepository: Cap-go/capgo
Length of output: 2795
🏁 Script executed:
# Check if the postgres_schema.ts has all required definitions and imports
head -50 supabase/functions/_backend/utils/postgres_schema.tsRepository: Cap-go/capgo
Length of output: 2128
🏁 Script executed:
# Look at how other recent endpoints have been converted to pg/drizzle pattern
rg "insert.*values\|from.*schema" supabase/functions/_backend/public/ -B 5 -A 3 | head -60Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Check the RPC function create_hashed_apikey to understand the pattern
rg "create_hashed_apikey" supabase/functions/_backend/ -B 3 -A 5Repository: Cap-go/capgo
Length of output: 1526
Migrate API key creation to use pg/drizzle client for D1 migration compliance.
Lines 85-110 use supabase.rpc() and .from('apikeys').insert(). During the active migration to Cloudflare D1, all database operations must use getPgClient() or getDrizzleClient() from utils/pg.ts with Drizzle ORM patterns from postgres_schema.ts. Migrate both the hashed and non-hashed creation paths to the pg/drizzle client and close the connection using closeClient().
| try { | ||
| const rows = await drizzleClient | ||
| .select({ owner_org: schema.apps.owner_org }) | ||
| .from(schema.apps) | ||
| .where(or(eq(schema.apps.id, record.id), eq(schema.apps.app_id, record.app_id))) | ||
| .limit(1) | ||
| appExists = rows.length > 0 | ||
| ownerOrg = rows[0]?.owner_org ?? undefined | ||
| } | ||
| catch (error) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'Error fetching app owner_org', error, appId: record.id, app_id: record.app_id }) | ||
| } | ||
| finally { | ||
| closeClient(c, pg) | ||
| } | ||
|
|
||
| // If the app no longer exists (deleted between INSERT and async trigger processing), skip | ||
| // all side effects. Still validate the org exists to keep the "error cases" contract. | ||
| if (!appExists) { | ||
| ownerOrg = ownerOrg ?? (record.owner_org ?? undefined) | ||
| if (!ownerOrg) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'App missing and no owner_org in webhook payload, skipping', record }) | ||
| return c.json(BRES) | ||
| } | ||
|
|
||
| const { data, error } = await supabase | ||
| .from('orgs') | ||
| .select('*') | ||
| .eq('id', ownerOrg) | ||
| .single() | ||
| if (error || !data) { | ||
| throw simpleError('error_fetching_organization', 'Error fetching organization', { error }) | ||
| } | ||
|
|
||
| cloudlog({ requestId: c.get('requestId'), message: 'App missing, skipping onboarding and default versions', record }) | ||
| return c.json(BRES) | ||
| } |
There was a problem hiding this comment.
Don’t treat owner_org lookup failures as app deletion.
If the drizzle query throws (Line 45), appExists stays false and the handler skips onboarding/default versions even when the app still exists. Consider surfacing the error (so the trigger can retry) or falling back to record.owner_org and continuing.
Proposed fix
catch (error) {
cloudlog({ requestId: c.get('requestId'), message: 'Error fetching app owner_org', error, appId: record.id, app_id: record.app_id })
+ throw simpleError('error_fetching_app_owner_org', 'Error fetching app owner_org', { error })
}📝 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.
| try { | |
| const rows = await drizzleClient | |
| .select({ owner_org: schema.apps.owner_org }) | |
| .from(schema.apps) | |
| .where(or(eq(schema.apps.id, record.id), eq(schema.apps.app_id, record.app_id))) | |
| .limit(1) | |
| appExists = rows.length > 0 | |
| ownerOrg = rows[0]?.owner_org ?? undefined | |
| } | |
| catch (error) { | |
| cloudlog({ requestId: c.get('requestId'), message: 'Error fetching app owner_org', error, appId: record.id, app_id: record.app_id }) | |
| } | |
| finally { | |
| closeClient(c, pg) | |
| } | |
| // If the app no longer exists (deleted between INSERT and async trigger processing), skip | |
| // all side effects. Still validate the org exists to keep the "error cases" contract. | |
| if (!appExists) { | |
| ownerOrg = ownerOrg ?? (record.owner_org ?? undefined) | |
| if (!ownerOrg) { | |
| cloudlog({ requestId: c.get('requestId'), message: 'App missing and no owner_org in webhook payload, skipping', record }) | |
| return c.json(BRES) | |
| } | |
| const { data, error } = await supabase | |
| .from('orgs') | |
| .select('*') | |
| .eq('id', ownerOrg) | |
| .single() | |
| if (error || !data) { | |
| throw simpleError('error_fetching_organization', 'Error fetching organization', { error }) | |
| } | |
| cloudlog({ requestId: c.get('requestId'), message: 'App missing, skipping onboarding and default versions', record }) | |
| return c.json(BRES) | |
| } | |
| try { | |
| const rows = await drizzleClient | |
| .select({ owner_org: schema.apps.owner_org }) | |
| .from(schema.apps) | |
| .where(or(eq(schema.apps.id, record.id), eq(schema.apps.app_id, record.app_id))) | |
| .limit(1) | |
| appExists = rows.length > 0 | |
| ownerOrg = rows[0]?.owner_org ?? undefined | |
| } | |
| catch (error) { | |
| cloudlog({ requestId: c.get('requestId'), message: 'Error fetching app owner_org', error, appId: record.id, app_id: record.app_id }) | |
| throw simpleError('error_fetching_app_owner_org', 'Error fetching app owner_org', { error }) | |
| } | |
| finally { | |
| closeClient(c, pg) | |
| } | |
| // If the app no longer exists (deleted between INSERT and async trigger processing), skip | |
| // all side effects. Still validate the org exists to keep the "error cases" contract. | |
| if (!appExists) { | |
| ownerOrg = ownerOrg ?? (record.owner_org ?? undefined) | |
| if (!ownerOrg) { | |
| cloudlog({ requestId: c.get('requestId'), message: 'App missing and no owner_org in webhook payload, skipping', record }) | |
| return c.json(BRES) | |
| } | |
| const { data, error } = await supabase | |
| .from('orgs') | |
| .select('*') | |
| .eq('id', ownerOrg) | |
| .single() | |
| if (error || !data) { | |
| throw simpleError('error_fetching_organization', 'Error fetching organization', { error }) | |
| } | |
| cloudlog({ requestId: c.get('requestId'), message: 'App missing, skipping onboarding and default versions', record }) | |
| return c.json(BRES) | |
| } |
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/triggers/on_app_create.ts` around lines 36 - 72,
The drizzle query failure currently leaves appExists false and makes the handler
treat the app as deleted; update the catch block around the drizzleClient.select
call (where schema.apps is queried) to either rethrow the error after logging so
the trigger can retry (i.e., cloudlog(...); throw error) or, if you prefer to
continue, fall back to the webhook payload by setting ownerOrg =
record.owner_org and appExists = true before closing the client; ensure
closeClient(c, pg) still runs in finally and keep cloudlog of the original
error.
Summary (AI generated)
Test plan (AI generated)
bun run lint:backendbun run lintScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
New Features
Bug Fixes
Documentation