fix: security and quality improvements across tRPC routers#29497
Open
Dnyanesh182 wants to merge 3 commits into
Open
fix: security and quality improvements across tRPC routers#29497Dnyanesh182 wants to merge 3 commits into
Dnyanesh182 wants to merge 3 commits into
Conversation
Contributor
|
Welcome to Cal.diy, @Dnyanesh182! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
Dnyanesh182
force-pushed
the
fix/security-quality-improvements
branch
from
35c79ff to
90dbf30
Compare
Contributor
📝 WalkthroughWalkthroughThis PR narrows Prisma query projections in multiple viewer handlers (bookings, event types, duplicate flow), replaces console.log with a shared structured logger in the schedule handler, and adds an explicit TypeScript type for a reduce accumulator. These edits change fetched object shapes and a context type while preserving existing control flow and runtime behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/availability/schedule/getScheduleByEventTypeSlug.handler.ts (1)
60-61: 💤 Low valueConsider wrapping the error for structured logging consistency.
The change to
logger.erroris a clear improvement. For consistency with the pattern shown in other handlers (seepackages/trpc/server/routers/viewer/admin/watchlist/delete.handler.ts), consider wrapping the error in an object with a named field.📊 Optional: match structured logging pattern
} catch (e) { - logger.error("Failed to retrieve schedule by event type slug", e); + logger.error("Failed to retrieve schedule by event type slug", { error: e }); return {🤖 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 `@packages/trpc/server/routers/viewer/availability/schedule/getScheduleByEventTypeSlug.handler.ts` around lines 60 - 61, The catch block in getScheduleByEventTypeSlug.handler currently calls logger.error with the message and raw error; change it to use structured logging like logger.error({ err: e, eventTypeSlug }, "Failed to retrieve schedule by event type slug") so the error is wrapped in an object (key name `err` or `error` to match existing handlers such as delete.handler) and include relevant context (e.g., eventTypeSlug) for consistency with other handlers.packages/trpc/server/routers/viewer/bookings/util.ts (1)
39-55: 💤 Low valueConsider refactoring to use
selectinstead ofincludefor the user object.The coding guideline recommends using
selectinstead ofincludein Prisma queries for performance and security. Currently, theuserobject usesincludeto fetch relations. Converting to an explicitselectwould align with the guideline and provide better control over which fields are fetched.This is a broader refactor and not blocking the current security fix, which correctly prioritizes preventing credential.key exposure.
As per coding guidelines: "Use
selectinstead ofincludein Prisma queries for performance and security."🤖 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 `@packages/trpc/server/routers/viewer/bookings/util.ts` around lines 39 - 55, Change the Prisma query for the user relation to use select instead of include; replace the user: { include: { destinationCalendar, credentials: { select: { id, type, appId } }, profiles: { select: { organizationId } } } } with an explicit user: { select: { destinationCalendar: true, credentials: { select: { id: true, type: true, appId: true } }, profiles: { select: { organizationId: true } } } } so only the intended relations/fields (destinationCalendar, credentials.{id,type,appId}, profiles.{organizationId}) are returned and credential.key is never selected. Ensure the surrounding query (wherever this user selection is used) still composes correctly with the rest of the Prisma call.
🤖 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 `@packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts`:
- Around line 41-49: The hosts selection in duplicate.handler.ts currently only
selects userId, isFixed, priority, weight, and eventTypeId which causes
scheduleId, groupId, and location to be dropped when you later do hosts.map(({
eventTypeId: _, ...rest }) => rest); update the Prisma select inside the hosts
block to also include scheduleId, groupId, and location so those optional Host
fields are fetched, and keep the existing map that strips eventTypeId
(hosts.map(({ eventTypeId: _, ...rest }) => rest)) so the duplicated hosts
preserve scheduleId/groupId/location while removing the old eventTypeId.
---
Nitpick comments:
In
`@packages/trpc/server/routers/viewer/availability/schedule/getScheduleByEventTypeSlug.handler.ts`:
- Around line 60-61: The catch block in getScheduleByEventTypeSlug.handler
currently calls logger.error with the message and raw error; change it to use
structured logging like logger.error({ err: e, eventTypeSlug }, "Failed to
retrieve schedule by event type slug") so the error is wrapped in an object (key
name `err` or `error` to match existing handlers such as delete.handler) and
include relevant context (e.g., eventTypeSlug) for consistency with other
handlers.
In `@packages/trpc/server/routers/viewer/bookings/util.ts`:
- Around line 39-55: Change the Prisma query for the user relation to use select
instead of include; replace the user: { include: { destinationCalendar,
credentials: { select: { id, type, appId } }, profiles: { select: {
organizationId } } } } with an explicit user: { select: { destinationCalendar:
true, credentials: { select: { id: true, type: true, appId: true } }, profiles:
{ select: { organizationId: true } } } } so only the intended relations/fields
(destinationCalendar, credentials.{id,type,appId}, profiles.{organizationId})
are returned and credential.key is never selected. Ensure the surrounding query
(wherever this user selection is used) still composes correctly with the rest of
the Prisma call.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7dae27b-9c8d-4c62-9dab-f1138d0e6668
📒 Files selected for processing (5)
packages/trpc/server/routers/viewer/availability/schedule/getScheduleByEventTypeSlug.handler.tspackages/trpc/server/routers/viewer/bookings/util.tspackages/trpc/server/routers/viewer/eventTypes/getEventTypesFromGroup.handler.tspackages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.tspackages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
Comment on lines
+41
to
+49
| hosts: { | ||
| select: { | ||
| userId: true, | ||
| isFixed: true, | ||
| priority: true, | ||
| weight: true, | ||
| eventTypeId: true, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if hosts with scheduleId, groupId, or location exist that would be affected by this change
ast-grep --pattern 'scheduleId: $$$' | head -20
rg -nP --type=ts -C3 'hosts.*scheduleId|hosts.*groupId|hosts.*location' -g '!node_modules' -g '!*.test.*' -g '!*.spec.*'Repository: calcom/cal.diy
Length of output: 4966
🏁 Script executed:
#!/bin/bash
# Find the Host model definition in Prisma schema
rg -nP -A 20 'model Host \{' --glob='*.prisma'Repository: calcom/cal.diy
Length of output: 1720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts"
echo "== file =="
wc -l "$FILE"
echo
echo "== select (around lines 35-70) =="
nl -ba "$FILE" | sed -n '35,90p'
echo
echo "== duplication/createMany mapping (search + context) =="
rg -n "createMany|hosts\.map|duplicate|copy" "$FILE" -S
echo
# Print a wider window around the createMany if present
LINE_CREATE=$(rg -n "createMany" "$FILE" -S | head -n 1 | cut -d: -f1 || true)
if [ -n "${LINE_CREATE:-}" ]; then
START=$((LINE_CREATE-60))
END=$((LINE_CREATE+60))
echo
echo "== around createMany at $LINE_CREATE (lines $START-$END) =="
nl -ba "$FILE" | sed -n "${START},${END}p"
fi
echo
echo "== around any hosts.map mapping =="
LINE_HOSTSMAP=$(rg -n "hosts\.map" "$FILE" -S | head -n 1 | cut -d: -f1 || true)
if [ -n "${LINE_HOSTSMAP:-}" ]; then
START=$((LINE_HOSTSMAP-40))
END=$((LINE_HOSTSMAP+60))
echo
echo "== around hosts.map at $LINE_HOSTSMAP (lines $START-$END) =="
nl -ba "$FILE" | sed -n "${START},${END}p"
fiRepository: calcom/cal.diy
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts"
echo "== file =="
wc -l "$FILE"
echo
show_range () {
local start="$1" end="$2"
echo "== $start-$end =="
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d %s\n", NR, $0}' "$FILE"
echo
}
echo "== select/copy mapping context (35-90) =="
show_range 35 90
echo "== find createMany/hosts.map lines (grep context) =="
rg -n "createMany|hosts\.map|duplicate|copy" "$FILE" -S
LINE_CREATE=$(rg -n "createMany" "$FILE" -S | head -n 1 | cut -d: -f1 || true)
if [ -n "${LINE_CREATE:-}" ]; then
show_range $((LINE_CREATE-80)) $((LINE_CREATE+60))
fi
LINE_HOSTSMAP=$(rg -n "hosts\.map" "$FILE" -S | head -n 1 | cut -d: -f1 || true)
if [ -n "${LINE_HOSTSMAP:-}" ]; then
show_range $((LINE_HOSTSMAP-60)) $((LINE_HOSTSMAP+80))
fiRepository: calcom/cal.diy
Length of output: 13138
Fix Host field preservation in event-type duplication (scheduleId/groupId/location are dropped).
packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts narrows hosts to only userId, isFixed, priority, weight, and eventTypeId and then duplicates via hosts.map(({ eventTypeId: _, ...rest }) => rest), so Host.scheduleId, Host.groupId, and Host.location (optional fields on model Host in packages/prisma/schema.prisma) are not copied to the duplicated event type’s hosts.
🤖 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 `@packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts`
around lines 41 - 49, The hosts selection in duplicate.handler.ts currently only
selects userId, isFixed, priority, weight, and eventTypeId which causes
scheduleId, groupId, and location to be dropped when you later do hosts.map(({
eventTypeId: _, ...rest }) => rest); update the Prisma select inside the hosts
block to also include scheduleId, groupId, and location so those optional Host
fields are fetched, and keep the existing map that strips eventTypeId
(hosts.map(({ eventTypeId: _, ...rest }) => rest)) so the duplicated hosts
preserve scheduleId/groupId/location while removing the old eventTypeId.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts (1)
41-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the optional
Hostfields when duplicating hosts.This
selectstill dropsscheduleId,groupId, andlocation, so the laterhosts.map(({ eventTypeId: _, ...rest }) => rest)silently strips those values from duplicated hosts.Suggested fix
hosts: { select: { userId: true, isFixed: true, priority: true, weight: true, + scheduleId: true, + groupId: true, + location: true, eventTypeId: true, }, },🤖 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 `@packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts` around lines 41 - 48, The host duplication is losing optional Host fields because the Prisma select in duplicate.handler.ts for "hosts" omits scheduleId, groupId, and location; update the select inside the query that populates hosts to include scheduleId: true, groupId: true, and location: true so those optional fields are fetched, and keep the existing hosts.map(({ eventTypeId: _, ...rest }) => rest) to only drop eventTypeId while preserving the newly-selected optional fields.
🤖 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.
Duplicate comments:
In `@packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts`:
- Around line 41-48: The host duplication is losing optional Host fields because
the Prisma select in duplicate.handler.ts for "hosts" omits scheduleId, groupId,
and location; update the select inside the query that populates hosts to include
scheduleId: true, groupId: true, and location: true so those optional fields are
fetched, and keep the existing hosts.map(({ eventTypeId: _, ...rest }) => rest)
to only drop eventTypeId while preserving the newly-selected optional fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86459d91-9022-4beb-9044-cfb4e95213a5
📒 Files selected for processing (5)
packages/trpc/server/routers/viewer/availability/schedule/getScheduleByEventTypeSlug.handler.tspackages/trpc/server/routers/viewer/bookings/util.tspackages/trpc/server/routers/viewer/eventTypes/getEventTypesFromGroup.handler.tspackages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.tspackages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
✅ Files skipped from review due to trivial changes (1)
- packages/trpc/server/routers/viewer/availability/schedule/getScheduleByEventTypeSlug.handler.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/trpc/server/routers/viewer/eventTypes/getEventTypesFromGroup.handler.ts
- packages/trpc/server/routers/viewer/bookings/util.ts
- packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
- fix(bookings): replace credentials: true with select to prevent credential.key exposure in booking middleware - fix(eventTypes): remove console.log statements that leaked private hashed link data to stdout - fix(schedule): replace console.log with structured logger.error in getScheduleByEventTypeSlug - refactor(eventTypes): replace Prisma include with select in getEventTypesFromGroup and duplicate handlers - fix(eventTypes): add missing scheduleId/groupId/memberId to hosts select in duplicate handler - fix(eventTypes): remove @ts-expect-error by typing reduce accumulator as Record<string, unknown>
Dnyanesh182
force-pushed
the
fix/security-quality-improvements
branch
from
ecfc991 to
ffe2bea
Compare
Member
|
Please attach it to an issue (create one if does not exist). and please attach visual proofs of your changes. Making it draft. |
Author
What does this PR do?Fixes #ISSUE_NUMBER Addresses multiple security, data-leak, and code-quality issues in tRPC viewer routers. Changes
Visual Proof of ChangesFix 1: Credential key exposure preventedBefore ( // packages/trpc/server/routers/viewer/bookings/util.ts (line 42)
user: {
include: {
destinationCalendar: true,
credentials: true, // ❌ Fetches credential.key (OAuth tokens, API secrets)After: Only non-sensitive fields selected: user: {
include: {
destinationCalendar: true,
credentials: { // ✅ Only safe fields
select: {
id: true,
type: true,
appId: true,
},
},Fix 2: Debug console.log removed (private data leak)Before ( // packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts
console.log("multiplePrivateLinks", multiplePrivateLinks); // ❌ Line 611
// ...
console.log("connectedLinks", connectedLinks); // ❌ Line 615After: Both lines removed. Fix 3: Structured loggingBefore ( // packages/trpc/server/routers/viewer/availability/schedule/getScheduleByEventTypeSlug.handler.ts
} catch (e) {
console.log(e); // ❌ Raw error dumpAfter: import logger from "@calcom/lib/logger";
// ...
} catch (e) {
logger.error("Failed to retrieve schedule by event type slug", e); // ✅ StructuredFix 4: Prisma
|
Author
|
Hi @bandhan-majumder , I've created issue #29500 and updated the PR description with visual proof (before/after code comparisons and grep verification). Could you please add the run-ci label when you get a chance? Thanks! |
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
2 participants
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.
What does this PR do?
Fixes #29500
Addresses multiple security, data-leak, and code-quality issues in tRPC viewer routers.
Changes
credentials: trueexposescredential.key(OAuth tokens)bookings/util.tsconsole.logdumps private hashed links to stdouteventTypes/heavy/update.handler.tsconsole.log(e)instead of structuredlogger.errorschedule/getScheduleByEventTypeSlug.handler.tsincludefetches all columns instead ofselectgetEventTypesFromGroup.handler.ts,duplicate.handler.ts@ts-expect-errorreplaced with properRecord<string, unknown>typingeventTypes/heavy/update.handler.tsVisual Proof of Changes
Fix 1: Credential key exposure prevented
Before (
main):credentials: truefetches ALL fields including sensitivekey:After: Only non-sensitive fields selected:
Fix 2: Debug console.log removed (private data leak)
Before (
main): Private hashed link data dumped to stdout:After: Both lines removed.
Fix 3: Structured logging
Before (
main):After:
Fix 4: Prisma
include→select(performance)Before (
main): Fetches all columns from hosts, team, webhooks:After: Only needed fields:
Fix 5: Type safety improvement
Before (
main):After:
Verification
Post-fix grep scan — zero remaining violations in viewer routers:
Type of change
How should this be tested?
credential.keyis no longer returned by the bookings procedure middlewareconsole.logremains in the changed filesscheduleId,groupId,memberIdyarn type-check:ci --forceto confirm no type errors introduced