feat: add artifical ratelimiting to v1 verify endpoint #4537
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds a deprecation rate-limiter for API v1: new UNKEY_ROOT_KEY env, dependency and types, runtime UnkeyRatelimiter creation in init middleware, post-verification limiter check in key service returning API_V1_EOL on limit exceed, plus Docker/compose and build tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Init as Init Middleware
participant KeySvc as Key Service
participant RateLim as Deprecation Ratelimiter
participant Response
Client->>Init: HTTP request
Init->>Init: if UNKEY_ROOT_KEY set\ncreate deprecationRatelimiter
Init->>KeySvc: call (services incl. limiter)
KeySvc->>KeySvc: verify key
alt key valid
KeySvc->>KeySvc: emit metrics
KeySvc->>RateLim: check namespace v1_deprecation
alt allowed
RateLim-->>KeySvc: allowed
KeySvc-->>Response: return verification result
else rejected
RateLim-->>KeySvc: rejected
KeySvc->>Response: error(API_V1_EOL, 429, migration URL)
end
else invalid
KeySvc-->>Response: error (verification failed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-07-22T09:02:12.495ZApplied to files:
📚 Learning: 2025-09-24T18:57:34.843ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (4)
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 |
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer and
ogzhanolguncu
as code owners
![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
Fix all issues with AI Agents 🤖
In @web/apps/api/src/pkg/middleware/init.ts:
- Around line 54-70: The timeout fallback on deprecationRatelimiter currently
returns remaining: 10 and success: true which bypasses rate limiting on timeout;
update the UnkeyRatelimiter timeout.fallback object used by
deprecationRatelimiter so it fails-closed (e.g., set remaining: 0, success:
false, keep limit/reset values for context) or a degraded mode (e.g., remaining:
1, limit: 10, success: true) per policy; modify the timeout.fallback inside the
deprecationRatelimiter instantiation to enforce the desired behavior.
🧹 Nitpick comments (3)
web/apps/api/Dockerfile (1)
9-9: Glob pattern**may not expand as expected in default shell.The
**glob pattern requiresglobstarto be enabled in bash. In the default shell, this may only match literal**/node_modulesor behave unexpectedly. Consider usingfindfor reliable recursive deletion:🔎 Proposed fix
-RUN rm -rf ./**/node_modules +RUN find . -name "node_modules" -type d -prune -exec rm -rf {} + 2>/dev/null || trueNote: With
node_modulesnow in.dockerignore, this cleanup step may be redundant if the ignore is working correctly.web/apps/api/src/pkg/errors/http.ts (1)
245-260: Consider handlingAPI_V1_EOLdocs URL inerrorResponsefor consistency.The
handleErrorfunction correctly uses the migration URL forAPI_V1_EOL, buterrorResponsealways uses the standard error docs URL. IferrorResponseis ever used withAPI_V1_EOL, it would return the wrong docs link.🔎 Proposed fix
export function errorResponse( c: Context, code: z.infer<typeof ErrorCode>, message: string, ) { return c.json<z.infer<typeof ErrorSchema>>( { error: { code: code, - docs: `https://unkey.dev/docs/api-reference/errors/code/${code}`, + docs: + code === "API_V1_EOL" + ? "https://www.unkey.com/docs/api-reference/v1/migration" + : `https://unkey.dev/docs/api-reference/errors/code/${code}`, message, requestId: c.get("requestId"), }, }, { status: codeToStatus(code) }, ); }web/apps/api/src/pkg/middleware/init.ts (1)
54-70: Consider adding error handling for UnkeyRatelimiter construction.The
UnkeyRatelimiterconstructor could throw if the root key is invalid or if there are initialization issues. Since this is an optional feature and the PR author noted it hasn't been tested yet (especially for root keys), consider wrapping the construction in a try-catch to gracefully degrade if initialization fails.🔎 Proposed error handling
- const deprecationRatelimiter = c.env.UNKEY_ROOT_KEY - ? new UnkeyRatelimiter({ - rootKey: c.env.UNKEY_ROOT_KEY, - namespace: "v1_deprecation", - limit: 10, - duration: 60_000, - timeout: { - ms: 5000, - fallback: { - limit: 10, - remaining: 10, - success: true, - reset: Date.now() + 60_000, - }, - }, - }) - : undefined; + let deprecationRatelimiter: UnkeyRatelimiter | undefined; + if (c.env.UNKEY_ROOT_KEY) { + try { + deprecationRatelimiter = new UnkeyRatelimiter({ + rootKey: c.env.UNKEY_ROOT_KEY, + namespace: "v1_deprecation", + limit: 10, + duration: 60_000, + timeout: { + ms: 5000, + fallback: { + limit: 10, + remaining: 0, + success: false, + reset: Date.now() + 60_000, + }, + }, + }); + } catch (error) { + // Log error but don't fail the request - deprecation limiting is optional + console.error("Failed to initialize deprecation rate limiter:", error); + } + }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
deployment/docker-compose.yamlweb/apps/api/Dockerfileweb/apps/api/Dockerfile.dockerignoreweb/apps/api/package.jsonweb/apps/api/src/pkg/env.tsweb/apps/api/src/pkg/errors/http.tsweb/apps/api/src/pkg/hono/env.tsweb/apps/api/src/pkg/keys/service.tsweb/apps/api/src/pkg/middleware/init.ts
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3810
File: go/apps/api/routes/v2_ratelimit_limit/handler.go:54-56
Timestamp: 2025-08-20T11:41:36.718Z
Learning: In the Unkey codebase, the X-Unkey-Metrics: disabled header is used by the v1 API when replaying ratelimit requests to the v2 API (go/apps/api/routes/v2_ratelimit_limit/handler.go) to prevent double billing/logging to ClickHouse. This is a legitimate internal service-to-service communication pattern for API migration scenarios, not intended for external client usage.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/api/keys/query-key-usage-timeseries/index.ts:1-1
Timestamp: 2025-08-22T12:48:58.289Z
Learning: The team at Unkey is planning to move from TRPC to calling their v2 API directly. They're comfortable keeping TRPC input schemas imported from app route folders as technical debt since this code will be replaced, rather than refactoring to move schemas to lib/schemas.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/workspace/onboarding.ts:1-1
Timestamp: 2025-08-22T12:45:07.187Z
Learning: The team at Unkey is planning to move from TRPC to calling their v2 API directly, making current TRPC schema organization temporary. They're comfortable with keeping server input schemas imported from app route folders as technical debt since this code will be replaced.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/app/(app)/[workspaceId]/apis/[apiId]/keys/[keyAuthId]/_components/components/controls/index.tsx:1-1
Timestamp: 2025-08-22T12:46:58.510Z
Learning: The team at unkeyed/unkey is moving towards calling their v2 API directly and prefers not to refactor temporary code that will be replaced in the near future.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts:1-1
Timestamp: 2025-08-22T12:49:20.668Z
Learning: The team at unkeyed/unkey is comfortable with keeping TRPC schema imports from app route folders as temporary technical debt during their migration from TRPC to calling their v2 API directly, since this code will be replaced rather than maintained long-term.
📚 Learning: 2024-10-23T12:05:31.121Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The `cloudflareRatelimiter` type definition in `apps/api/src/pkg/env.ts` should not have its interface changed; it should keep the `limit` method returning `Promise<{ success: boolean }>` without additional error properties.
Applied to files:
web/apps/api/src/pkg/hono/env.tsweb/apps/api/package.jsonweb/apps/api/src/pkg/middleware/init.tsweb/apps/api/src/pkg/errors/http.tsweb/apps/api/src/pkg/keys/service.ts
📚 Learning: 2024-10-20T07:05:55.471Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2294
File: apps/api/src/pkg/keys/service.ts:268-271
Timestamp: 2024-10-20T07:05:55.471Z
Learning: In `apps/api/src/pkg/keys/service.ts`, `ratelimitAsync` is a table relation, not a column selection. When querying, ensure that table relations are included appropriately, not as columns.
Applied to files:
web/apps/api/src/pkg/hono/env.tsweb/apps/api/src/pkg/middleware/init.tsweb/apps/api/src/pkg/keys/service.ts
📚 Learning: 2025-06-02T11:09:58.791Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3292
File: apps/dashboard/lib/trpc/routers/key/create.ts:11-14
Timestamp: 2025-06-02T11:09:58.791Z
Learning: In the unkey codebase, TypeScript and the env() function implementation already provide sufficient validation for environment variables, so additional runtime error handling for missing env vars is not needed.
Applied to files:
web/apps/api/src/pkg/hono/env.tsweb/apps/api/src/pkg/env.ts
📚 Learning: 2025-09-15T17:40:51.536Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3973
File: go/pkg/db/schema.sql:307-308
Timestamp: 2025-09-15T17:40:51.536Z
Learning: The environments table in the Unkey codebase is not being used in production yet, so schema changes to it don't require complex migration sequences to handle existing data or concurrent usage.
Applied to files:
web/apps/api/src/pkg/env.ts
📚 Learning: 2025-08-21T12:37:40.996Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3821
File: apps/dashboard/lib/trpc/routers/key/updateRootKeyPermissions.ts:74-74
Timestamp: 2025-08-21T12:37:40.996Z
Learning: Root keys in Unkey have two workspace fields: `workspaceId` (always set to env().UNKEY_WORKSPACE_ID for the Unkey workspace that owns the key) and `forWorkspaceId` (set to ctx.workspace.id for the user's workspace that the key is for). When querying root keys, the system filters by forWorkspaceId to get keys for the current user's workspace, but the returned rootKey.workspaceId is always the Unkey workspace ID.
Applied to files:
web/apps/api/package.json
📚 Learning: 2025-08-20T11:41:36.718Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3810
File: go/apps/api/routes/v2_ratelimit_limit/handler.go:54-56
Timestamp: 2025-08-20T11:41:36.718Z
Learning: In the Unkey codebase, the X-Unkey-Metrics: disabled header is used by the v1 API when replaying ratelimit requests to the v2 API (go/apps/api/routes/v2_ratelimit_limit/handler.go) to prevent double billing/logging to ClickHouse. This is a legitimate internal service-to-service communication pattern for API migration scenarios, not intended for external client usage.
Applied to files:
web/apps/api/src/pkg/middleware/init.tsweb/apps/api/src/pkg/keys/service.ts
📚 Learning: 2025-08-22T12:49:20.668Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts:1-1
Timestamp: 2025-08-22T12:49:20.668Z
Learning: The team at unkeyed/unkey is comfortable with keeping TRPC schema imports from app route folders as temporary technical debt during their migration from TRPC to calling their v2 API directly, since this code will be replaced rather than maintained long-term.
Applied to files:
web/apps/api/src/pkg/middleware/init.tsweb/apps/api/src/pkg/keys/service.ts
📚 Learning: 2025-12-16T16:52:57.276Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4508
File: apps/dashboard/lib/trpc/routers/identity/updateRatelimit.ts:141-147
Timestamp: 2025-12-16T16:52:57.276Z
Learning: In apps/dashboard/lib/trpc/routers/identity/updateRatelimit.ts, audit log resources for identity ratelimit updates intentionally use type "ratelimit" with id set to identity.id (rather than type "identity"). This design pattern shows that the ratelimit operations belong to the identity entity.
Applied to files:
web/apps/api/src/pkg/middleware/init.ts
📚 Learning: 2025-07-14T08:15:56.747Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3474
File: go/apps/api/routes/v2_keys_verify_key/200_test.go:380-380
Timestamp: 2025-07-14T08:15:56.747Z
Learning: In the Unkey codebase, there is a mechanism to set the server's time via a header for test control, which helps make ratelimit tests deterministic instead of time-dependent. This is useful for preventing flaky tests where the second request might hit a new ratelimit window.
Applied to files:
web/apps/api/src/pkg/middleware/init.ts
📚 Learning: 2025-01-03T14:22:34.611Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 2787
File: apps/dashboard/middleware.ts:86-86
Timestamp: 2025-01-03T14:22:34.611Z
Learning: The user clarified that the `apps/dashboard` and `apps/www` are separate applications hosted on different subdomains.
Applied to files:
web/apps/api/Dockerfile.dockerignore
📚 Learning: 2025-08-08T15:37:14.734Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/build.yaml:14-17
Timestamp: 2025-08-08T15:37:14.734Z
Learning: Repo: unkeyed/unkey — CI behavior: We rely on CI=true to make pnpm install behave as --frozen-lockfile. Don’t suggest adding --frozen-lockfile in .github/actions/setup-node/action.yaml or workflows like .github/workflows/build.yaml.
Applied to files:
web/apps/api/Dockerfile
📚 Learning: 2025-08-08T16:07:48.307Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:37-40
Timestamp: 2025-08-08T16:07:48.307Z
Learning: Repo unkeyed/unkey — pnpm immutable installs are enforced by setting the CI environment variable; any truthy value (e.g., "1" or "true") is acceptable. Do not require the literal string "true". Applies to .github/actions/setup-node/action.yaml and all workflows using pnpm install.
Applied to files:
web/apps/api/Dockerfile
📚 Learning: 2025-07-22T09:02:12.495Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3638
File: deployment/docker-compose.yaml:81-94
Timestamp: 2025-07-22T09:02:12.495Z
Learning: The docker-compose.yaml file in deployment/ is specifically for development environments, not production. Kafka and other service configurations in this file should be optimized for development convenience rather than production security/hardening.
Applied to files:
deployment/docker-compose.yaml
📚 Learning: 2025-09-24T18:57:34.843Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 4010
File: QUICKSTART-DEPLOY.md:17-17
Timestamp: 2025-09-24T18:57:34.843Z
Learning: In the Unkey deployment platform, API key environment variables use component-specific naming but share the same secret value: UNKEY_API_KEY for the ctrl service (validator), API_KEY for the CLI client, and CTRL_API_KEY for the dashboard client. The ctrl service acts as the source of truth for validation.
Applied to files:
deployment/docker-compose.yaml
🧬 Code graph analysis (1)
web/apps/api/src/pkg/keys/service.ts (1)
web/apps/api/src/pkg/errors/http.ts (1)
UnkeyApiError(123-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
web/apps/api/Dockerfile.dockerignore (1)
3-3: LGTM!Adding
node_modulesto the dockerignore is good practice—it reduces the build context size and aligns with the cleanup step in the Dockerfile.deployment/docker-compose.yaml (2)
226-226: LGTM!The
UNKEY_ROOT_KEYenvironment variable is correctly passed to enable the deprecation rate limiter. Since this is a development docker-compose file, having it sourced from the host environment is appropriate.
234-239: LGTM!Making the dockerfile path explicit and adding the VERSION build arg improves build configuration clarity.
web/apps/api/src/pkg/errors/http.ts (1)
24-25: LGTM!The
API_V1_EOLerror code is correctly added and mapped to HTTP 429 status, which is appropriate for rate-limiting behavior.web/apps/api/src/pkg/keys/service.ts (2)
195-212: Rate limiting is applied after metrics emission—verify this is intentional.The deprecation rate limiter is checked after
this.metrics.emit()(line 186-193), meaning successful verifications are counted/billed before potentially being rejected withAPI_V1_EOL. If you want to avoid billing users for rejected requests, consider moving this check earlier.Additionally, regarding the PR note about root key behavior: the implementation correctly uses
res.val.key?.forWorkspaceId ?? res.val.key?.workspaceId, which should handle root keys appropriately since root keys haveforWorkspaceIdset to the user's workspace.
198-202: Fail-open error handling is appropriate for deprecation warnings.Catching errors and returning
{ success: true }ensures the deprecation limiter doesn't break key verification if the rate limiting service fails. This is the right trade-off for a non-critical deprecation mechanism.web/apps/api/src/pkg/hono/env.ts (1)
3-3: LGTM!The
UnkeyRatelimitertype import and optionaldeprecationRatelimiterproperty are correctly added to the service context. Making it optional is appropriate since it's only initialized whenUNKEY_ROOT_KEYis configured.Also applies to: 24-24
web/apps/api/package.json (1)
38-38: Version2.1.3is stable and published.The dependency exists and is a stable release. However, version
2.1.4is the current latest version; consider updating if there are relevant fixes or improvements in the newer release.web/apps/api/src/pkg/middleware/init.ts (1)
11-11: LGTM!The import alias
UnkeyRatelimiterappropriately distinguishes this from the existingDurableRateLimiterused elsewhere in the file.web/apps/api/src/pkg/env.ts (1)
38-39: LGTM - Consider format validation as a future enhancement.The optional string field is appropriate for this feature. As a future enhancement, you could add format validation (e.g.,
.regex(/^unkey_/)) to catch configuration errors early, but the current implementation is acceptable for the initial rollout.
Add an artifical limiter for 10 reqs per minute to verifying keys.
It uses our @unkey/ratelimit package under the hood, so we can manually override workspace limits in our dashboard.
test:
It should make progress and eventually fail with a 429 error