Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdded an onError fallback to multiple Ratelimit instances so rate-limiter failures log and return a permissive response, and bumped Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRPC as TRPC handler
participant RL as Ratelimit
participant Service as Downstream service
Client->>TRPC: request (resendAuthCode / trpc call)
TRPC->>RL: rl.limit(key)
RL-->>TRPC: success OR (error -> onError fallback)
Note right of TRPC#lightblue: onError logs and returns permissive response
TRPC->>Service: continue processing (validation, resend)
Service-->>TRPC: result
TRPC-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
chronark
requested review from
Flo4604,
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
Would it make sense to start using the onError callback in our uses as well, its not used anywhere atm. |
|
yes |
|
I thought we did, let me fix |
![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 (3)
apps/dashboard/app/auth/actions.ts (1)
103-131: Validate input before rate-limiting and centralize PII redactionPlease apply the following changes:
Early email validation
Move the empty-email check to before you instantiate the rate limiter, so you don’t consume quota or produce unnecessary errors:- const rl = new Ratelimit({ + if (!email.trim()) { + return { + success: false, + code: AuthErrorCode.INVALID_EMAIL, + message: "Email address is required.", + }; + } + + const rl = new Ratelimit({ namespace: "resend_code", duration: "5m", limit: 5, rootKey: unkeyRootKey,Enhance the
onErrorhook
Redact any PII (email addresses or other identifiers) before logging, and return a failure rather than silently swallowing errors:onError: (err: Error, identifier: string) => { - console.error(`Error occurred while rate limiting ${identifier}: ${err.message}`); - return { success: true, limit: 0, remaining: 1, reset: 1 }; + const redacted = + identifier && identifier.includes("@") + ? identifier.replace(/(^.).*(@.*$)/, "$1***$2") + : `${identifier?.slice(0, 2) ?? ""}***`; + console.error("[ratelimit] resend_code failure for %s", redacted, err); + return { success: false, limit: 0, remaining: 0, reset: 1 }; },Audit and refactor existing
console/loggercalls
Our scan flagged multiple locations where raw user identifiers or full user objects (with emails) are logged. These should all be replaced with a centralized, PII-aware logging helper. Examples include:
- In
apps/dashboard/app/auth/actions.tsat lines 99–102:onError: (err, identifier) => { console.error(`${identifier} - ${err.message}`) // raw identifier return fallback(identifier) }- Anywhere a full
userobject is logged (e.g. lines 164–167):await cache.user.set("userId", { id: "userId", email: "user@email.com" }); const user = await cache.user.get("userId"); console.log(user); // logs email in plaintext- Multiple other
console.log(userId)orconsole.error("…", error)calls across the codebase.Recommendation: introduce a logging utility (e.g.
logRedacted(identifier, message, error?)) or wrap your existing logger to automatically mask emails and IDs. Then replace all directconsole.*orlogger.*calls that surface PII with that helper.This will both prevent wasted rate-limit calls on invalid input and ensure no sensitive identifiers ever slip into your logs unredacted.
apps/dashboard/lib/trpc/trpc.ts (2)
148-156: Avoid fail-open on LLM ratelimit (cost exposure).If the ratelimiter goes down, LLM requests could spike costs. Prefer failing closed or a tight local fallback.
? new Ratelimit({ rootKey: env().UNKEY_ROOT_KEY ?? "", namespace: "trpc_llm", limit: LLM_LIMITS.RATE_LIMIT, duration: LLM_LIMITS.RATE_DURATION, - onError, })
89-121: Avoid repeated env() calls; retrieve once for consistency and micro-optimizations.env() parses process.env each call. Read once and reuse.
+const rootKey = env().UNKEY_ROOT_KEY ?? ""; -export const ratelimit = env().UNKEY_ROOT_KEY +export const ratelimit = rootKey ? { create: new Ratelimit({ - rootKey: env().UNKEY_ROOT_KEY ?? "", + rootKey, namespace: "trpc_create", limit: 25, duration: "3s", }), read: new Ratelimit({ - rootKey: env().UNKEY_ROOT_KEY ?? "", + rootKey, namespace: "trpc_read", limit: 100, duration: "10s", onError, }), update: new Ratelimit({ - rootKey: env().UNKEY_ROOT_KEY ?? "", + rootKey, namespace: "trpc_update", limit: 25, duration: "5s", }), delete: new Ratelimit({ - rootKey: env().UNKEY_ROOT_KEY ?? "", + rootKey, namespace: "trpc_delete", limit: 25, duration: "5s", }), } : {}; -const llmRatelimit = env().UNKEY_ROOT_KEY +const llmRatelimit = rootKey ? new Ratelimit({ - rootKey: env().UNKEY_ROOT_KEY ?? "", + rootKey, namespace: "trpc_llm", limit: LLM_LIMITS.RATE_LIMIT, duration: LLM_LIMITS.RATE_DURATION, }) : null;Also applies to: 148-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/dashboard/app/auth/actions.ts(1 hunks)apps/dashboard/lib/trpc/trpc.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Use Biome for formatting and linting in TypeScript/JavaScript projects
Prefer named exports over default exports in TypeScript/JavaScript, except for Next.js pages
Files:
apps/dashboard/app/auth/actions.tsapps/dashboard/lib/trpc/trpc.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Follow strict TypeScript configuration
Use Zod for runtime validation in TypeScript projects
Files:
apps/dashboard/app/auth/actions.tsapps/dashboard/lib/trpc/trpc.ts
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
apps/dashboard/app/auth/actions.tsapps/dashboard/lib/trpc/trpc.ts
🧠 Learnings (1)
📚 Learning: 2024-10-23T12:05:31.121Z
Learnt from: chronark
PR: unkeyed/unkey#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:
apps/dashboard/app/auth/actions.tsapps/dashboard/lib/trpc/trpc.ts
🧬 Code graph analysis (1)
apps/dashboard/lib/trpc/trpc.ts (1)
apps/dashboard/lib/env.ts (1)
env(3-49)
🔇 Additional comments (1)
apps/dashboard/lib/trpc/trpc.ts (1)
84-87: The TypeScript compiler isn’t available in this environment, so thetscstep failed. Please run a local typecheck (for example:npx tsc -p apps/dashboard/tsconfig.json --noEmit), then verify that the fallbackonErrorhandler’s return value:{ success: true, limit: 0, remaining: 1, reset: 1 }matches the type expected by
@unkey/ratelimit@2.1.3(in particular, ensure that:
resetis meant to be anumber(not aDate),- there aren’t any additional required properties on the response object).
Once that’s confirmed, we can consider this concern addressed.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/21/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
|
the point of the fallback is to not crash our application, so it should return success: true |
3 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Chores
Bug Fixes
Impact