fix: route API key writes through backend#2197
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR centralizes API key CRUD into new service helpers ( ChangesAPI Key Operations Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/apikeys.test.ts`:
- Around line 42-57: Replace direct uses of BASE_URL for the new API calls with
the test helper getEndpointUrl(path): call fetch(getEndpointUrl('/apikey'), ...)
when creating the key and fetch(getEndpointUrl(`/apikey/${createData.key}`),
...) when retrieving it; ensure getEndpointUrl is imported/available in
tests/apikeys.test.ts and apply the same replacement to the other occurrences
noted (around the other block at lines 549-552) so requests route correctly
between Supabase and Cloudflare Workers.
🪄 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: 938c7137-6028-4db0-afff-471c55ee831b
📒 Files selected for processing (7)
src/components/organization/ApiKeyRbacManager.vuesrc/pages/ApiKeys.vuesrc/pages/settings/organization/ApiKeys.[id].vuesrc/services/apikeys.tssupabase/functions/_backend/public/apikey/delete.tssupabase/functions/_backend/public/apikey/get.tstests/apikeys.test.ts
jihadMo
force-pushed
the
codex/route-apikey-ui-writes
branch
2 times, most recently
from
925e3b9 to
6d5fe12
Compare
Flob365
left a comment
Flob365
left a comment
There was a problem hiding this comment.
Thanks for moving the API key mutations behind the backend route. One blocker I found from the latest CI run: bun typecheck fails in supabase/functions/_backend/public/apikey/delete.ts after the new getApiKeyLookupFilter() helper is used.
The failing job reports:
supabase/functions/_backend/public/apikey/delete.ts(...): error TS2339: Property 'id' does not exist on type 'GenericStringError'.
This looks like a Supabase type inference issue caused by passing the dynamic lookup.column union into .eq(...); the query builder can no longer prove that the selected row is an apikeys row, so data becomes GenericStringError and apikey.id is rejected.
A small fix would be to keep the runtime behavior but branch explicitly, similar to put.ts:
const baseQuery = supabase
.from('apikeys')
.select('id')
.eq('user_id', auth.userId)
const apikeyQuery = /^\d+$/.test(id)
? baseQuery.eq('id', Number(id))
: baseQuery.eq('key', id)
const { data: apikey, error: apikeyError } = await apikeyQuery.single()That should preserve the cast-error avoidance while restoring the typed row shape. The same pattern may also be worth using in get.ts if the typecheck reaches that file after delete.ts is fixed.
jihadMo
force-pushed
the
codex/route-apikey-ui-writes
branch
from
6d5fe12 to
49bccd0
Compare
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
One security tradeoff to call out: the UI changes now pass numeric IDs, which is good, but the backend/tests still deliberately support using a plaintext API key value as the :id path segment for GET/DELETE. That preserves compatibility with the old .or(key.eq...,id.eq...) behavior, but it also means raw API key material can appear in URLs, reverse-proxy logs, request traces, browser history, and test output.
If that compatibility path is required, I would document it as legacy/deprecated and keep the new UI/helper examples anchored on numeric id only. The new regression tests currently exercise /apikey/${createData.key} directly; a short comment there (or an additional test asserting the UI path uses id) would make it clearer that secret-in-path is only for backward compatibility rather than the preferred contract going forward.
|
Good point. I agree the numeric id path should be the preferred contract, and the key-as-path-segment behavior should be treated as legacy compatibility only. The UI helpers added here already call the route with numeric ids, so documenting/deprecating the plaintext-key route and making the regression tests explicitly label it as legacy would make the intent clearer. |
|
Thanks, good point. I pushed c04472b to make the intent explicit: the frontend API-key helpers now accept numeric IDs only, the backend lookup helper labels plaintext-key path usage as legacy compatibility, and the regression tests are renamed/commented as legacy coverage while using getEndpointUrl(). |
|
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
Verified the follow-up at c04472b. The new frontend service helpers now take a numeric ApiKeyId and build routes from that id, the UI callers pass apiKey.id, and the remaining key-as-URL tests/helper path are explicitly marked as legacy compatibility. I also re-ran the focused API-key regression suite successfully:
bun x vitest run tests/apikeys.test.ts → 36 passed.
The remaining full vue-tsc run is still blocked locally by unrelated CLI test/module errors (@capgo/cli/sdk resolution and an implicit-any in tests/cli-channel.test.ts), not by the touched API-key route files. From the reviewed scope, the legacy-path intent is now clear.
|
Closing as AI-generated spam. Part of a 50+ PR wave of duplicate |
4 participants
/claim #1667
Summary
PUT /apikey/:idandDELETE /apikey/:idbackend routesapikeystable mutationsGET /apikey/:idandDELETE /apikey/:idlookup behavior withPUT /apikey/:idso numeric ids and UUID key values are queried separatelyWhy
The backend API-key routes already centralize ownership checks, limited-key guards, validation, and policy enforcement. The UI should use that path for API-key mutations instead of writing the table directly.
The GET/DELETE lookup change also avoids mixing UUID key values into the numeric
idfilter, matching the safer lookup pattern already used by the PUT handler.Related: #1667
Testing
git diff --checknpx.cmd --yes vitest run tests/apikeys.test.ts(blocked locally: this checkout has no installed dependencies; Vitest cannot resolvevitefromvitest.config.ts)npx.cmd --yes eslint src/services/apikeys.ts src/pages/ApiKeys.vue src/components/organization/ApiKeyRbacManager.vue src/pages/settings/organization/ApiKeys.[id].vue supabase/functions/_backend/public/apikey/get.ts supabase/functions/_backend/public/apikey/delete.ts tests/apikeys.test.ts(blocked locally: ESLint cannot resolve@antfu/eslint-configwithout installed dependencies)Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores