feat(api-key): add scopes (read/write/update/delete) and token-type r…#383
Merged
Conversation
…estriction Scopes are selected per key on creation and editable afterwards. Default scope per HTTP method (GET=read, POST=write, PATCH/PUT=update, DELETE=delete) with per-route override via decorator config. Session-token requests bypass scope checks; only api_key tokens are enforced. Token-type restriction added so security-critical routes can opt out of api-key access entirely. Hidden routes (schema.hide=true) are now session- only by default rule, and api-key management routes are explicitly session- only — no key can mint, list, update, or revoke other keys. Frontend: checkbox group on create + edit dialogs; scope badges in list view (single "Full access" badge when all four scopes are granted, including legacy keys with empty scopes); i18n in all 9 locales. Tests: middleware unit tests, default-method-mapping unit test, integration tests for scope rejection, token-type rejection, and update flow on real Clerk-issued keys, plus scope-coverage matrix that introspects every registered route and asserts correct scope and token-type configuration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
WalkthroughThis PR introduces API key scope-based access control and expands API key management capabilities. It adds new infrastructure for enforcing API-key scopes and token types at the HTTP layer, extends the shared API-key DTOs to support scope assignment, creates an update use case for API keys, and wires scope/token-type validation into route middleware. The frontend gains UI components to select and edit API key scopes, and comprehensive test coverage validates the new enforcement behavior across the backend. ChangesAPI Key Scope & Token-Type Access Control
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastifyRoute
participant AuthMiddleware
participant TokenTypeMiddleware
participant ScopeMiddleware
participant Handler
Client->>FastifyRoute: PATCH /api-key/:id<br/>(Bearer: api_key)
FastifyRoute->>AuthMiddleware: extract auth, resolve user
AuthMiddleware->>FastifyRoute: request.user = {id, tokenType: 'api_key', scopes: ['read']}
FastifyRoute->>TokenTypeMiddleware: enforceTokenType(['session_token'])
Note over TokenTypeMiddleware: user.tokenType = 'api_key'<br/>not in allowed list
TokenTypeMiddleware->>FastifyRoute: throw TokenTypeNotAllowedError
FastifyRoute->>Client: 403 {errorCode, providedTokenType, allowedTokenTypes}
Client->>FastifyRoute: POST /api/v1/tag<br/>(Bearer: api_key with scopes: ['read'])
FastifyRoute->>AuthMiddleware: extract auth, resolve user
AuthMiddleware->>FastifyRoute: request.user = {id, tokenType: 'api_key', scopes: ['read']}
FastifyRoute->>TokenTypeMiddleware: enforceTokenType(['api_key'])
TokenTypeMiddleware->>FastifyRoute: ok (api_key in allowed)
FastifyRoute->>ScopeMiddleware: enforceScope('write')
Note over ScopeMiddleware: required = 'write',<br/>granted = ['read']
ScopeMiddleware->>FastifyRoute: throw InsufficientScopeError
FastifyRoute->>Client: 403 {errorCode, requiredScope, grantedScopes}
Client->>FastifyRoute: GET /api/v1/qr-codes<br/>(Bearer: api_key with scopes: ['read'])
FastifyRoute->>AuthMiddleware: extract auth, resolve user
AuthMiddleware->>FastifyRoute: request.user = {id, tokenType: 'api_key', scopes: ['read']}
FastifyRoute->>TokenTypeMiddleware: enforceTokenType(['api_key'])
TokenTypeMiddleware->>FastifyRoute: ok
FastifyRoute->>ScopeMiddleware: enforceScope('read')
Note over ScopeMiddleware: 'read' in ['read']
ScopeMiddleware->>FastifyRoute: ok
FastifyRoute->>Handler: invoke handler
Handler->>Client: 200 {data}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Function-level JSDoc kept short on enforce-scope, enforce-token-type, filterKnownScopes, resolveScopeForMethod. Property comments on the route decorator config collapsed to one line each. Inline narrative comments in helpers and tests reduced to one line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/src/modules/api-key/useCase/list-api-keys.use-case.ts (1)
12-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
limit: 100includes revoked keys, so active-key count can be silently under-reported.The
limit: 100cap is applied before the client-side.filter((key) => !key.revoked). If a user has accumulated, say, 90 revoked keys and 20 active ones, Clerk returns the first 100 (which may be 90 revoked + 10 active), and after filtering only 10 active keys are returned — with no indication to the caller that 10 more exist.At minimum, log a warning when the raw page fills the limit, so the issue is observable:
⚠️ Suggested defensive logconst { data } = await this.clerkApiKeys.apiKeys.list({ subject: userId, limit: 100 }); + +if (data.length === 100) { + this.logger.warn('api-key.list.limit_reached', { + userId, + message: 'Clerk returned 100 API keys; some keys may not be listed.', + }); +}A more robust fix would be to check whether Clerk supports a
revoked: falsefilter parameter on the list call, so the limit applies only to the active-key population.🤖 Prompt for 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. In `@apps/backend/src/modules/api-key/useCase/list-api-keys.use-case.ts` around lines 12 - 27, The current call in list-api-keys.use-case.ts uses this.clerkApiKeys.apiKeys.list({ subject: userId, limit: 100 }) and then filters out revoked keys client-side, which can under-report active keys when the raw page is full; update the code in the function that calls this.clerkApiKeys.apiKeys.list to (1) prefer using Clerk's server-side filter if available (e.g., add revoked: false or equivalent to the list call) so the limit applies only to active keys, and (2) if a server-side filter is not supported, detect when the returned data.length equals the requested limit (100) and emit a warning via the module logger (e.g., processLogger or the existing logger in this use case) indicating the page was full and results may be truncated before applying .filter((key) => !key.revoked), so callers/operators can investigate.apps/backend/src/modules/custom-domain/http/controller/custom-domain.controller.ts (1)
283-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
403toclear-defaultresponse schema now that scope enforcement is enabled.With
config.scope = 'update'(Line 296), this endpoint can return forbidden for API keys without the required scope, but403is not declared inresponseSchema.Patch
`@Post`('/clear-default', { responseSchema: { + 403: DEFAULT_ERROR_RESPONSES[403], },🤖 Prompt for 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. In `@apps/backend/src/modules/custom-domain/http/controller/custom-domain.controller.ts` around lines 283 - 297, The response schema for the clear-default endpoint is missing 403 despite scope enforcement; update the `@Post`('/clear-default', ...) decorator's responseSchema to include a 403 entry (e.g. add 403: DEFAULT_ERROR_RESPONSES[403]) so the clear-default route (operationId 'custom-domain/clear-default') properly declares forbidden responses when config: { scope: 'update' } blocks the caller.
🧹 Nitpick comments (7)
apps/backend/src/modules/api-key/useCase/update-api-key.use-case.ts (1)
9-13: ⚡ Quick winExtract
isClerkClientErrorto a shared utility to avoid duplication.The function is defined identically in both
update-api-key.use-case.tsandrevoke-api-key.use-case.ts. Move it toapps/backend/src/modules/api-key/util/clerk-error.tsand import it in both use cases to maintain a single source of truth.🤖 Prompt for 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. In `@apps/backend/src/modules/api-key/useCase/update-api-key.use-case.ts` around lines 9 - 13, Extract the duplicated isClerkClientError function into a shared util: create a new module clerk-error.ts under apps/backend/src/modules/api-key/util that exports isClerkClientError, move the current implementation there, then update both update-api-key.use-case.ts and revoke-api-key.use-case.ts to import { isClerkClientError } from that util instead of defining the function locally; ensure the exported function signature and behavior remain identical so callers in updateApiKey (or any function/class referencing isClerkClientError) and revokeApiKey keep working without changes.apps/backend/src/modules/api-key/http/__tests__/utils.ts (1)
3-9: ⚡ Quick win
ALL_API_KEY_SCOPESshould be derived from the shared package rather than hardcoded.
API_KEY_SCOPESis already exported from@shared/schemas(used in the frontend). Hardcoding the values here means tests won't fail if a scope is later added to the shared definition.♻️ Proposed fix
-import { type ApiKeyScope } from '@shared/schemas'; +import { API_KEY_SCOPES, type ApiKeyScope } from '@shared/schemas'; -export const ALL_API_KEY_SCOPES: ApiKeyScope[] = ['read', 'write', 'update', 'delete']; +export const ALL_API_KEY_SCOPES: ApiKeyScope[] = [...API_KEY_SCOPES];🤖 Prompt for 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. In `@apps/backend/src/modules/api-key/http/__tests__/utils.ts` around lines 3 - 9, ALL_API_KEY_SCOPES is hardcoded but should mirror the canonical list exported from `@shared/schemas`; replace the manual array with the shared export (API_KEY_SCOPES) by importing API_KEY_SCOPES from '@shared/schemas' and use it (or a shallow copy like [...API_KEY_SCOPES] cast to ApiKeyScope[] if mutability is a concern) so tests automatically reflect any scope changes in the shared package; update the reference to ALL_API_KEY_SCOPES to derive from API_KEY_SCOPES instead of the literal ['read','write','update','delete'].apps/frontend/src/components/dashboard/api-keys/CreateApiKeyDialog.tsx (1)
221-254: ⚡ Quick winFragile i18n key derivation via
as-cast string interpolation — and duplicated inApiKeyListItem.tsx.
scope${cap}is cast to a fixed union (line 224) rather than looked up. If a new entry is added toAPI_KEY_SCOPES, the cast silently allows a key that doesn't exist in the translation namespace, producing a runtime translation miss. The same capitalisation+cast pattern is repeated verbatim inApiKeyListItem.tsxlines 61-65.A typed lookup map eliminates both issues:
♻️ Extract a shared lookup (e.g. in a co-located `utils.ts` or `ApiKeyScopes.tsx`)
+// apps/frontend/src/components/dashboard/api-keys/scopeI18nKeys.ts +import type { ApiKeyScope } from '@shared/schemas'; + +export const SCOPE_I18N_KEYS = { + read: { label: 'scopeRead', hint: 'scopeReadHint' }, + write: { label: 'scopeWrite', hint: 'scopeWriteHint' }, + update: { label: 'scopeUpdate', hint: 'scopeUpdateHint' }, + delete: { label: 'scopeDelete', hint: 'scopeDeleteHint' }, +} as const satisfies Record<ApiKeyScope, { label: string; hint: string }>;Then in
CreateApiKeyDialog.tsx:-const cap = `${scope.charAt(0).toUpperCase()}${scope.slice(1)}`; -const labelKey = `scope${cap}` as - | 'scopeRead' - | 'scopeWrite' - | 'scopeUpdate' - | 'scopeDelete'; -const hintKey: - | 'scopeReadHint' - | 'scopeWriteHint' - | 'scopeUpdateHint' - | 'scopeDeleteHint' = `${labelKey}Hint`; +const { label: labelKey, hint: hintKey } = SCOPE_I18N_KEYS[scope];And in
ApiKeyListItem.tsx:-const labelKey = `scope${s.charAt(0).toUpperCase()}${s.slice(1)}` as - | 'scopeRead' - | 'scopeWrite' - | 'scopeUpdate' - | 'scopeDelete'; +const { label: labelKey } = SCOPE_I18N_KEYS[s];🤖 Prompt for 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. In `@apps/frontend/src/components/dashboard/api-keys/CreateApiKeyDialog.tsx` around lines 221 - 254, The code derives i18n keys via string interpolation and an unsafe `as` cast (e.g., the `labelKey = \`scope${cap}\` as 'scopeRead'|'scopeWrite'|'scopeUpdate'|'scopeDelete'` and `hintKey = \`${labelKey}Hint\`` in CreateApiKeyDialog) which is fragile and duplicated in ApiKeyListItem; create a typed lookup object (e.g., API_KEY_SCOPE_LABELS and API_KEY_SCOPE_HINTS or a single map mapping each entry of API_KEY_SCOPES to its label and hint keys) in a co-located utils module and replace the interpolation code in both CreateApiKeyDialog (the Checkbox/label rendering block) and ApiKeyListItem (the same capitalization+cast block) to read keys from that map so additions to API_KEY_SCOPES are type-checked and won’t produce runtime missing-translation keys.apps/backend/src/core/http/middleware/enforce-scope.middleware.ts (1)
14-27: LGTM, with a note on the grandfather clause.The grandfather behavior at Lines 21–22 (legacy keys with empty/missing scopes pass all checks) is clearly documented and matches the corresponding test cases in
enforce-scope.middleware.test.ts. Just make sure there's a tracked plan/migration to backfillscopeson legacy keys so this bypass can eventually be removed — otherwise it remains a permanent privilege-escalation surface for any pre-feature key.🤖 Prompt for 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. In `@apps/backend/src/core/http/middleware/enforce-scope.middleware.ts` around lines 14 - 27, The enforceScope middleware currently lets legacy API keys with empty/missing scopes bypass checks (the "grandfather clause" in enforceScope); create and reference a concrete backfill plan: add a DB migration or one-off script to populate a default/appropriate scopes array for existing api keys, open a tracked ticket/PR that schedules running that migration, and add a TODO comment in enforce-scope.middleware.ts (near enforceScope) to remove the empty-scopes bypass once the backfill is complete; after backfill, update enforceScope to treat empty/missing scopes as denying access (i.e., remove the early return that allows empty scopes) and update enforce-scope.middleware.test.ts accordingly.apps/backend/src/libs/fastify/helpers.ts (1)
293-313: 💤 Low valueDefault scope enforcement is method-derived for every authed route — confirm intent.
Lines 308–309 apply a default scope (derived from HTTP method) to every route that doesn't opt out of auth, even when the route author didn't explicitly think about API-key scopes. That's fine for the API-key model (scopes are only enforced for
api_keytokens, so session calls are unaffected), but it does mean any new authed route silently inherits a scope requirement based on its verb. Two things worth verifying:
- Routes that intentionally allow any-scope read/write access can no longer do so without going through
config.scope(orallowedTokenTypesto exclude API keys). There's no "no scope required" escape hatch besides the legacy empty-scope grandfather.- The hidden-route default at Line 303 only restricts token type, not scope. Hidden +
allowedTokenTypesoverride +scopeinterplay is non-obvious; consider adding a brief comment near the block summarizing the precedence (config.allowedTokenTypes> hidden-default > none, andconfig.scope> method-derived).🤖 Prompt for 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. In `@apps/backend/src/libs/fastify/helpers.ts` around lines 293 - 313, The method-derived default scope (via resolveScopeForMethod) is being applied to every authed route when routeMeta.options.config?.scope is unset, which can surprise authors and interacts non-obviously with the hidden-route token-type default; update the block around enforceTokenType/enforceScope (referencing enforceTokenType, enforceScope, resolveScopeForMethod, routeMeta.options.config?.allowedTokenTypes and routeMeta.options.config?.scope) to add a concise comment that documents the precedence and behavior: 1) allowedTokenTypes (explicit) overrides hidden-route default token restriction, 2) hidden-route default sets token types to ['session_token'] if not explicit, 3) config.scope (explicit) overrides method-derived scope, and 4) method-derived scope is used only when config.scope is absent (and affects API-key checks only); ensure the comment also mentions there is no other “no-scope” escape hatch besides leaving config.scope undefined or excluding API keys via allowedTokenTypes.apps/backend/src/core/http/middleware/add-user-to-request.middleware.ts (1)
40-53: 💤 Low valueUse the existing
filterKnownScopesutility to validate scopes.The type assertion at lines 42–46 casts Clerk's
getAuthreturn toscopes?: string[]. The codebase already providesfilterKnownScopes(inmodules/api-key/util/filter-known-scopes.ts), which filters rawstring[]scopes down to validApiKeyScopevalues—exactly matching its docstring purpose. Apply it here:const scopes = tokenType === 'api_key' ? filterKnownScopes(auth.scopes) : undefined;This ensures only known scopes are stored in
request.user.scopesand prevents unknown upstream changes in Clerk's payload from silently propagating into the request object.🤖 Prompt for 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. In `@apps/backend/src/core/http/middleware/add-user-to-request.middleware.ts` around lines 40 - 53, The code currently trusts Clerk's raw auth.scopes when building request.user (in the block using getAuth and resolveUserPlan); update the scopes assignment so that when tokenType === 'api_key' you pass auth.scopes through the existing filterKnownScopes utility (from modules/api-key/util/filter-known-scopes.ts) before assigning to request.user.scopes, and add the corresponding import for filterKnownScopes; keep tokenType and plan logic the same so only validated ApiKeyScope values are stored on request.user.apps/backend/src/modules/api-key/http/controller/api-key.controller.ts (1)
25-25: ⚡ Quick winUse path alias for the new use-case import.
Please replace the new relative import with the project alias style to keep backend imports consistent.
Suggested change
-import { UpdateApiKeyUseCase } from '../../useCase/update-api-key.use-case'; +import { UpdateApiKeyUseCase } from '@/modules/api-key/useCase/update-api-key.use-case';As per coding guidelines:
**/*.{ts,tsx}: Use path aliases@/* for local imports and@shared/schemasfor shared package imports.🤖 Prompt for 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. In `@apps/backend/src/modules/api-key/http/controller/api-key.controller.ts` at line 25, Replace the relative import of UpdateApiKeyUseCase with the project path-alias style: change the import of UpdateApiKeyUseCase (from '../../useCase/update-api-key.use-case') to the alias-based import using the @ prefix (e.g. import { UpdateApiKeyUseCase } from '@/modules/api-key/useCase/update-api-key.use-case') so backend imports follow the `@/`* convention.
🤖 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 `@apps/backend/src/modules/api-key/useCase/update-api-key.use-case.ts`:
- Around line 37-47: The update call to this.clerkApiKeys.apiKeys.update in
update-api-key.use-case.ts lacks error handling and can surface raw Clerk errors
as 500s; wrap the update invocation in a try-catch similar to the existing get()
handling, inspect the caught Clerk error (status/code) and convert 4xx Clerk
responses into the appropriate custom error (e.g., ApiKeyNotFoundError or
another domain-specific error used elsewhere), rethrow non-4xx errors, and
ensure the thrown custom error includes the expected errorCode so the Fastify
handler returns the correct 4xx response.
In `@apps/frontend/src/components/dashboard/api-keys/EditApiKeyDialog.tsx`:
- Around line 77-79: Replace the raw error object sent to PostHog in the error
handler inside EditApiKeyDialog: keep Sentry.captureException(err) but change
the posthog.capture call (the call with event 'error:api-key-update') to send a
sanitized payload built from err.message, err.name and any optional err.code or
status fields plus apiKey.id; ensure you do not pass the whole err object (and
avoid non-serializable fields) so PostHog receives only primitive, serializable
values.
In `@packages/shared/src/dtos/api-key/UpdateApiKeyDto.ts`:
- Around line 9-17: Create a dedicated schema module for the non-trivial DTO:
move the z.object(...) that defines description, scopes, expiresInDays and its
.refine(...) into a new exported schema (e.g., ApiKeyUpdateSchema) that
references API_KEY_DESCRIPTION_MAX_LENGTH and ApiKeyScopeSchema; keep the same
validation including the at-least-one-field .refine. Then update UpdateApiKeyDto
to be derived from that schema (e.g., export const UpdateApiKeyDto =
ApiKeyUpdateSchema or derive via .pick()/ .partial() from ApiKeyUpdateSchema) so
the DTO file only re-exports the schema-derived DTO and the complex validation
logic lives in the dedicated schema module.
---
Outside diff comments:
In `@apps/backend/src/modules/api-key/useCase/list-api-keys.use-case.ts`:
- Around line 12-27: The current call in list-api-keys.use-case.ts uses
this.clerkApiKeys.apiKeys.list({ subject: userId, limit: 100 }) and then filters
out revoked keys client-side, which can under-report active keys when the raw
page is full; update the code in the function that calls
this.clerkApiKeys.apiKeys.list to (1) prefer using Clerk's server-side filter if
available (e.g., add revoked: false or equivalent to the list call) so the limit
applies only to active keys, and (2) if a server-side filter is not supported,
detect when the returned data.length equals the requested limit (100) and emit a
warning via the module logger (e.g., processLogger or the existing logger in
this use case) indicating the page was full and results may be truncated before
applying .filter((key) => !key.revoked), so callers/operators can investigate.
In
`@apps/backend/src/modules/custom-domain/http/controller/custom-domain.controller.ts`:
- Around line 283-297: The response schema for the clear-default endpoint is
missing 403 despite scope enforcement; update the `@Post`('/clear-default', ...)
decorator's responseSchema to include a 403 entry (e.g. add 403:
DEFAULT_ERROR_RESPONSES[403]) so the clear-default route (operationId
'custom-domain/clear-default') properly declares forbidden responses when
config: { scope: 'update' } blocks the caller.
---
Nitpick comments:
In `@apps/backend/src/core/http/middleware/add-user-to-request.middleware.ts`:
- Around line 40-53: The code currently trusts Clerk's raw auth.scopes when
building request.user (in the block using getAuth and resolveUserPlan); update
the scopes assignment so that when tokenType === 'api_key' you pass auth.scopes
through the existing filterKnownScopes utility (from
modules/api-key/util/filter-known-scopes.ts) before assigning to
request.user.scopes, and add the corresponding import for filterKnownScopes;
keep tokenType and plan logic the same so only validated ApiKeyScope values are
stored on request.user.
In `@apps/backend/src/core/http/middleware/enforce-scope.middleware.ts`:
- Around line 14-27: The enforceScope middleware currently lets legacy API keys
with empty/missing scopes bypass checks (the "grandfather clause" in
enforceScope); create and reference a concrete backfill plan: add a DB migration
or one-off script to populate a default/appropriate scopes array for existing
api keys, open a tracked ticket/PR that schedules running that migration, and
add a TODO comment in enforce-scope.middleware.ts (near enforceScope) to remove
the empty-scopes bypass once the backfill is complete; after backfill, update
enforceScope to treat empty/missing scopes as denying access (i.e., remove the
early return that allows empty scopes) and update
enforce-scope.middleware.test.ts accordingly.
In `@apps/backend/src/libs/fastify/helpers.ts`:
- Around line 293-313: The method-derived default scope (via
resolveScopeForMethod) is being applied to every authed route when
routeMeta.options.config?.scope is unset, which can surprise authors and
interacts non-obviously with the hidden-route token-type default; update the
block around enforceTokenType/enforceScope (referencing enforceTokenType,
enforceScope, resolveScopeForMethod, routeMeta.options.config?.allowedTokenTypes
and routeMeta.options.config?.scope) to add a concise comment that documents the
precedence and behavior: 1) allowedTokenTypes (explicit) overrides hidden-route
default token restriction, 2) hidden-route default sets token types to
['session_token'] if not explicit, 3) config.scope (explicit) overrides
method-derived scope, and 4) method-derived scope is used only when config.scope
is absent (and affects API-key checks only); ensure the comment also mentions
there is no other “no-scope” escape hatch besides leaving config.scope undefined
or excluding API keys via allowedTokenTypes.
In `@apps/backend/src/modules/api-key/http/__tests__/utils.ts`:
- Around line 3-9: ALL_API_KEY_SCOPES is hardcoded but should mirror the
canonical list exported from `@shared/schemas`; replace the manual array with the
shared export (API_KEY_SCOPES) by importing API_KEY_SCOPES from
'@shared/schemas' and use it (or a shallow copy like [...API_KEY_SCOPES] cast to
ApiKeyScope[] if mutability is a concern) so tests automatically reflect any
scope changes in the shared package; update the reference to ALL_API_KEY_SCOPES
to derive from API_KEY_SCOPES instead of the literal
['read','write','update','delete'].
In `@apps/backend/src/modules/api-key/http/controller/api-key.controller.ts`:
- Line 25: Replace the relative import of UpdateApiKeyUseCase with the project
path-alias style: change the import of UpdateApiKeyUseCase (from
'../../useCase/update-api-key.use-case') to the alias-based import using the @
prefix (e.g. import { UpdateApiKeyUseCase } from
'@/modules/api-key/useCase/update-api-key.use-case') so backend imports follow
the `@/`* convention.
In `@apps/backend/src/modules/api-key/useCase/update-api-key.use-case.ts`:
- Around line 9-13: Extract the duplicated isClerkClientError function into a
shared util: create a new module clerk-error.ts under
apps/backend/src/modules/api-key/util that exports isClerkClientError, move the
current implementation there, then update both update-api-key.use-case.ts and
revoke-api-key.use-case.ts to import { isClerkClientError } from that util
instead of defining the function locally; ensure the exported function signature
and behavior remain identical so callers in updateApiKey (or any function/class
referencing isClerkClientError) and revokeApiKey keep working without changes.
In `@apps/frontend/src/components/dashboard/api-keys/CreateApiKeyDialog.tsx`:
- Around line 221-254: The code derives i18n keys via string interpolation and
an unsafe `as` cast (e.g., the `labelKey = \`scope${cap}\` as
'scopeRead'|'scopeWrite'|'scopeUpdate'|'scopeDelete'` and `hintKey =
\`${labelKey}Hint\`` in CreateApiKeyDialog) which is fragile and duplicated in
ApiKeyListItem; create a typed lookup object (e.g., API_KEY_SCOPE_LABELS and
API_KEY_SCOPE_HINTS or a single map mapping each entry of API_KEY_SCOPES to its
label and hint keys) in a co-located utils module and replace the interpolation
code in both CreateApiKeyDialog (the Checkbox/label rendering block) and
ApiKeyListItem (the same capitalization+cast block) to read keys from that map
so additions to API_KEY_SCOPES are type-checked and won’t produce runtime
missing-translation keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d749f544-4594-4ad3-b147-4810ff23a3a5
⛔ Files ignored due to path filters (1)
apps/backend/src/__tests__/__snapshots__/scope-coverage-matrix.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (44)
apps/backend/src/__tests__/scope-coverage-matrix.test.tsapps/backend/src/core/decorators/route.tsapps/backend/src/core/domain/schema/UserSchema.tsapps/backend/src/core/error/http/index.tsapps/backend/src/core/error/http/insufficient-scope.error.tsapps/backend/src/core/error/http/token-type-not-allowed.error.tsapps/backend/src/core/http/middleware/__tests__/enforce-scope.middleware.test.tsapps/backend/src/core/http/middleware/__tests__/enforce-token-type.middleware.test.tsapps/backend/src/core/http/middleware/add-user-to-request.middleware.tsapps/backend/src/core/http/middleware/enforce-scope.middleware.tsapps/backend/src/core/http/middleware/enforce-token-type.middleware.tsapps/backend/src/libs/fastify/__tests__/resolve-scope-for-method.test.tsapps/backend/src/libs/fastify/helpers.tsapps/backend/src/modules/analytics-integration/http/controller/analytics-integration.controller.tsapps/backend/src/modules/api-key/http/__tests__/api-key.test.tsapps/backend/src/modules/api-key/http/__tests__/utils.tsapps/backend/src/modules/api-key/http/controller/api-key.controller.tsapps/backend/src/modules/api-key/useCase/create-api-key.use-case.tsapps/backend/src/modules/api-key/useCase/list-api-keys.use-case.tsapps/backend/src/modules/api-key/useCase/update-api-key.use-case.tsapps/backend/src/modules/api-key/util/filter-known-scopes.tsapps/backend/src/modules/custom-domain/http/controller/custom-domain.controller.tsapps/backend/src/modules/qr-code/http/__tests__/render-qr-code.test.tsapps/backend/src/modules/qr-code/http/controller/qr-code.controller.tsapps/frontend/src/components/dashboard/api-keys/ApiKeyList.tsxapps/frontend/src/components/dashboard/api-keys/ApiKeyListItem.tsxapps/frontend/src/components/dashboard/api-keys/ApiKeyListItemActions.tsxapps/frontend/src/components/dashboard/api-keys/CreateApiKeyDialog.tsxapps/frontend/src/components/dashboard/api-keys/EditApiKeyDialog.tsxapps/frontend/src/dictionaries/de.jsonapps/frontend/src/dictionaries/en.jsonapps/frontend/src/dictionaries/es.jsonapps/frontend/src/dictionaries/fr.jsonapps/frontend/src/dictionaries/it.jsonapps/frontend/src/dictionaries/nl.jsonapps/frontend/src/dictionaries/pl.jsonapps/frontend/src/dictionaries/pt.jsonapps/frontend/src/dictionaries/ru.jsonapps/frontend/src/lib/api/api-key.tspackages/shared/src/dtos/api-key/ApiKeyResponseDto.tspackages/shared/src/dtos/api-key/ApiKeyScope.tspackages/shared/src/dtos/api-key/CreateApiKeyDto.tspackages/shared/src/dtos/api-key/UpdateApiKeyDto.tspackages/shared/src/index.ts
CodeRabbit feedback on PR #383: - Wrap apiKeys.update() in the same try-catch as apiKeys.get(), so 4xx Clerk responses surface as ApiKeyNotFoundError instead of opaque 500. - Strip raw error objects from posthog.capture() calls in create/edit/ revoke flows; send only sanitized name + message + ids. Skipped: CodeRabbit's suggestion to extract UpdateApiKeyDto into a dedicated schema module — local convention in dtos/api-key/ already keeps small DTOs flat. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements