chore: when using pii for rate limit (email, ip) hash #22922
Conversation
WalkthroughThis change set introduces a new utility module for hashing personally identifiable information (PII), specifically email addresses, IP addresses, and phone numbers, using an MD5-based hashing scheme with a configurable salt. The new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Graphite Automations"Add foundation team as reviewer" took an action on this PR • (08/06/25)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/organizations/verifyCode.handler.ts (1)
41-43: Consider consistency in email handling for secret generation.The secret generation still uses the raw email while rate limiting now uses the hashed email. For consistency with the PII protection goals, consider whether the secret generation should also use hashed email.
const secret = createHash("md5") - .update(email + process.env.CALENDSO_ENCRYPTION_KEY) + .update(hashEmail(email) + process.env.CALENDSO_ENCRYPTION_KEY) .digest("hex");Note: This would be a breaking change that affects existing tokens, so verify if this aligns with your migration strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/web/app/api/auth/forgot-password/route.ts(2 hunks)apps/web/pages/api/book/event.ts(2 hunks)apps/web/pages/api/book/instant-event.ts(1 hunks)apps/web/pages/api/book/recurring-event.ts(2 hunks)packages/features/auth/lib/next-auth-options.ts(2 hunks)packages/features/auth/lib/verifyEmail.ts(3 hunks)packages/lib/server/PiiHasher.test.ts(1 hunks)packages/lib/server/PiiHasher.ts(1 hunks)packages/sms/sms-manager.ts(2 hunks)packages/trpc/server/routers/loggedInViewer/addSecondaryEmail.handler.ts(1 hunks)packages/trpc/server/routers/viewer/auth/resendVerifyEmail.handler.ts(1 hunks)packages/trpc/server/routers/viewer/auth/sendVerifyEmailCode.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/verifyCode.handler.ts(2 hunks)packages/trpc/server/routers/viewer/teams/removeMember.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/pages/api/book/event.tspackages/trpc/server/routers/viewer/organizations/verifyCode.handler.tspackages/trpc/server/routers/loggedInViewer/addSecondaryEmail.handler.tspackages/trpc/server/routers/viewer/auth/resendVerifyEmail.handler.tsapps/web/pages/api/book/recurring-event.tsapps/web/pages/api/book/instant-event.tspackages/features/auth/lib/verifyEmail.tspackages/sms/sms-manager.tsapps/web/app/api/auth/forgot-password/route.tspackages/features/auth/lib/next-auth-options.tspackages/trpc/server/routers/viewer/teams/removeMember.handler.tspackages/trpc/server/routers/viewer/auth/sendVerifyEmailCode.handler.tspackages/lib/server/PiiHasher.test.tspackages/lib/server/PiiHasher.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/pages/api/book/event.tspackages/trpc/server/routers/viewer/organizations/verifyCode.handler.tspackages/trpc/server/routers/loggedInViewer/addSecondaryEmail.handler.tspackages/trpc/server/routers/viewer/auth/resendVerifyEmail.handler.tsapps/web/pages/api/book/recurring-event.tsapps/web/pages/api/book/instant-event.tspackages/features/auth/lib/verifyEmail.tspackages/sms/sms-manager.tsapps/web/app/api/auth/forgot-password/route.tspackages/features/auth/lib/next-auth-options.tspackages/trpc/server/routers/viewer/teams/removeMember.handler.tspackages/trpc/server/routers/viewer/auth/sendVerifyEmailCode.handler.tspackages/lib/server/PiiHasher.test.tspackages/lib/server/PiiHasher.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: For large pull requests (>500 lines changed or >10 files touched), advise splitting into smaller, focused PRs by feature, layer, dependency chain, or file/module
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.891Z
Learning: In PR #22304 for Cal.com private link expiration features, the `maxUsageCount` field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
📚 Learning: in packages/app-store/office365calendar/lib/calendarservice.ts, the fetcher method in office365calen...
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
apps/web/pages/api/book/event.tsapps/web/pages/api/book/recurring-event.ts
📚 Learning: the office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is spec...
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/sms/sms-manager.ts
📚 Learning: in cal.com's getusereventgroups handler refactor (pr #22618), the membershipcount field for team eve...
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Applied to files:
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
🧬 Code Graph Analysis (9)
apps/web/pages/api/book/event.ts (1)
packages/lib/server/PiiHasher.ts (1)
piiHasher(14-14)
packages/trpc/server/routers/viewer/organizations/verifyCode.handler.ts (1)
packages/lib/server/PiiHasher.ts (1)
hashEmail(16-20)
apps/web/pages/api/book/recurring-event.ts (1)
packages/lib/server/PiiHasher.ts (1)
piiHasher(14-14)
apps/web/pages/api/book/instant-event.ts (1)
packages/lib/server/PiiHasher.ts (1)
piiHasher(14-14)
packages/sms/sms-manager.ts (1)
packages/lib/server/PiiHasher.ts (1)
piiHasher(14-14)
apps/web/app/api/auth/forgot-password/route.ts (1)
packages/lib/server/PiiHasher.ts (1)
piiHasher(14-14)
packages/features/auth/lib/next-auth-options.ts (1)
packages/lib/server/PiiHasher.ts (1)
hashEmail(16-20)
packages/trpc/server/routers/viewer/auth/sendVerifyEmailCode.handler.ts (1)
packages/lib/server/PiiHasher.ts (2)
piiHasher(14-14)hashEmail(16-20)
packages/lib/server/PiiHasher.test.ts (1)
packages/lib/server/PiiHasher.ts (2)
Md5PiiHasher(7-12)hashEmail(16-20)
🔇 Additional comments (27)
apps/web/pages/api/book/event.ts (2)
10-10: LGTM: Proper import of PiiHasher utility.The import statement correctly brings in the piiHasher instance for use in this file.
24-24: LGTM: Consistent IP hashing for rate limiting.The change correctly replaces the raw IP address with its hashed version for rate limiting, aligning with the PR objective to anonymize PII data.
packages/trpc/server/routers/loggedInViewer/addSecondaryEmail.handler.ts (1)
25-25: LGTM: Switched from email to user ID for rate limiting.Using
user.idinstead ofuser.emaileliminates PII from the rate limiting identifier while maintaining effective rate limiting. This is a cleaner approach than hashing the email.packages/trpc/server/routers/viewer/organizations/verifyCode.handler.ts (2)
11-11: LGTM: Proper import of hashEmail utility.The import correctly brings in the hashEmail function for email hashing.
38-38: LGTM: Consistent email hashing for rate limiting.The change correctly replaces the raw email with its hashed version for rate limiting, maintaining privacy while preserving functionality.
apps/web/pages/api/book/recurring-event.ts (2)
10-10: LGTM: Proper import of PiiHasher utility.The import statement correctly brings in the piiHasher instance, consistent with the pattern in other booking API files.
41-41: LGTM: Consistent IP hashing for rate limiting.The change correctly replaces the raw IP address with its hashed version for rate limiting, maintaining consistency with the single event booking API and the overall PR objective to anonymize PII data.
apps/web/pages/api/book/instant-event.ts (2)
9-9: LGTM: Proper import of PII hasher utility.The import of
piiHasheris correctly added to support hashing of IP addresses for rate limiting.
16-16: LGTM: IP address hashing enhances privacy.The change from raw IP address to hashed IP (
piiHasher.hash(userIp)) properly anonymizes the rate limiting identifier while maintaining functionality. This aligns with the PR's privacy objectives.packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (1)
20-20: LGTM: User ID-based rate limiting is more appropriate for authenticated operations.Switching from IP-based (
ctx.sourceIp) to user ID-based (ctx.user.id) rate limiting is a good improvement for authenticated operations, as it prevents users from circumventing limits by changing IP addresses and provides more accurate per-user throttling.packages/trpc/server/routers/viewer/auth/resendVerifyEmail.handler.ts (1)
27-27: LGTM: User ID-based rate limiting improves security.Changing from email-based to user ID-based rate limiting (
ctx.user.id) is appropriate for authenticated operations and prevents users from circumventing rate limits by using different email addresses.packages/sms/sms-manager.ts (2)
7-7: LGTM: Proper import of PII hasher for phone number anonymization.The import of
piiHasheris correctly added to support hashing of phone numbers in the fallback rate limiting scenario.
36-36: LGTM: Phone number hashing in fallback scenario enhances privacy.The change to hash the phone number (
piiHasher.hash(reminderPhone)) in the fallback case is appropriate. The hierarchical approach (team ID > user ID > hashed phone) maintains specificity while protecting PII when more specific identifiers aren't available.apps/web/app/api/auth/forgot-password/route.ts (2)
10-10: LGTM: Proper import of PII hasher utility.The import of
piiHasheris correctly added to support anonymization of IP addresses and email fallback values for rate limiting.
32-32: LGTM: IP address hashing protects user privacy.The change to hash the IP address (
piiHasher.hash(ip)) properly anonymizes the rate limiting identifier. Sinceipcan fall back to email when not available, hashing protects both IP addresses and email addresses from being stored in raw form for rate limiting purposes.packages/features/auth/lib/next-auth-options.ts (2)
51-51: LGTM - Clean import of PII hashing utility.The import of
hashEmailfrom the PII hasher module is correctly placed and follows the established import organization.
135-135: LGTM - Proper anonymization of email for rate limiting.The change correctly replaces the raw email with a hashed version when used as a rate limiting identifier. This enhances privacy by preventing exposure of email addresses in rate limiting logs while maintaining the rate limiting functionality.
packages/features/auth/lib/verifyEmail.ts (3)
16-16: LGTM - Clean import of PII hashing utility.The import statement is properly placed and follows the existing import structure.
58-58: LGTM - Proper email hashing for rate limiting.The change correctly hashes the email address before using it as a rate limiting identifier, enhancing privacy while preserving functionality. The plain email continues to be used appropriately for database operations and email sending.
146-146: LGTM - Consistent email hashing for rate limiting.The change maintains consistency by hashing
user.emailFromfor rate limiting purposes while preserving the plain email for database operations and email delivery.packages/trpc/server/routers/viewer/auth/sendVerifyEmailCode.handler.ts (3)
9-9: LGTM - Appropriate imports for PII hashing.The import correctly brings in both
hashEmailfor email hashing andpiiHasherfor IP address hashing, which are both used in this handler.
17-17: LGTM - Smart dual-approach for rate limiting identifiers.The logic appropriately prioritizes IP-based rate limiting when a request is available, falling back to email-based rate limiting otherwise. Both identifiers are properly hashed to protect PII while maintaining effective rate limiting.
21-21: LGTM - Proper use of hashed identifier for rate limiting.The prefixed identifier correctly uses the hashed value determined in the previous line, maintaining consistency with the rate limiting pattern while protecting PII.
packages/lib/server/PiiHasher.test.ts (4)
1-2: LGTM - Clean test setup with appropriate imports.The imports correctly bring in the Vitest testing utilities and the PII hashing functionality to be tested.
8-12: LGTM - Comprehensive email hashing test.The test correctly verifies that
hashEmailhashes the local part of the email while preserving the domain. The expected hash value aligns with the plain PII test, confirming consistent hashing behavior.
14-18: LGTM - Validates core hashing functionality.The test properly verifies that the MD5 hasher produces the expected hash for a given input and salt combination.
20-25: LGTM - Important salt sensitivity validation.This test is crucial for security as it ensures that different salts produce different hash outputs for the same input, preventing rainbow table attacks and ensuring proper hash uniqueness.
E2E results are ready! |
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist