chore: wrap samlResponse in try/catch #22984
Conversation
WalkthroughThe SAML callback handler in the authentication API was updated to improve error handling and logging. A unique trace identifier (uid) is now included in the logger prefix for each request. The main logic processing the SAML response is enclosed in a try-catch block. Depending on the outcome, the handler either redirects the user, logs and returns a 400 error with details, or, in case of exceptions, logs the error and returns a 500 error. The fallback response remains a 400 error if no redirect URL is provided. No changes were made to the handler's exported signature. Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 consumer team as reviewer" took an action on this PR • (08/08/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/08/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/auth/saml/callback/route.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/app/api/auth/saml/callback/route.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/app/api/auth/saml/callback/route.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.352Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Learnt from: hariombalhara
PR: calcom/cal.com#22968
File: packages/features/auth/lib/next-auth-options.ts:327-327
Timestamp: 2025-08-08T09:12:08.269Z
Learning: In packages/features/auth/lib/next-auth-options.ts, do not log credentials in authorize() handlers (e.g., the "saml-idp" CredentialsProvider). Remove accidental console.log statements and avoid including credential contents in logs; prefer either no logging or structured logs without sensitive data.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:50-60
Timestamp: 2025-07-18T17:54:04.613Z
Learning: For Microsoft Graph webhook handlers, when dealing with internal configuration errors (like missing MICROSOFT_WEBHOOK_TOKEN), it's better to return 200 OK with errors tracked in the response body rather than throwing 5xx errors. This prevents retry storms from Microsoft Graph and maintains webhook subscription health, while still providing visibility through error logs and structured error responses.
📚 Learning: 2025-08-08T09:12:08.269Z
Learnt from: hariombalhara
PR: calcom/cal.com#22968
File: packages/features/auth/lib/next-auth-options.ts:327-327
Timestamp: 2025-08-08T09:12:08.269Z
Learning: In packages/features/auth/lib/next-auth-options.ts, do not log credentials in authorize() handlers (e.g., the "saml-idp" CredentialsProvider). Remove accidental console.log statements and avoid including credential contents in logs; prefer either no logging or structured logs without sensitive data.
Applied to files:
apps/web/app/api/auth/saml/callback/route.ts
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
apps/web/app/api/auth/saml/callback/route.ts
📚 Learning: 2025-08-08T10:26:13.352Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.352Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
apps/web/app/api/auth/saml/callback/route.ts
⏰ 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: Install dependencies / Yarn install & cache
🔇 Additional comments (3)
apps/web/app/api/auth/saml/callback/route.ts (3)
12-13: Good addition: per-request trace ID and sub-logger.This improves observability and correlates logs to responses.
21-24: Validate redirect_url to prevent open-redirects.If not already guaranteed by oauthController.samlResponse, ensure redirect_url is validated against an allowlist and is absolute/safe.
Can you confirm that samlResponse sanitizes/validates redirect_url internally? If not, I can provide a small helper to enforce an allowlist.
30-33: Ensure the error object is captured with stack by the logger.Depending on the logger API, prefer structured logging (e.g., log.error(error, '...') or log.error({ err: error }, '...')) so stacks aren’t lost.
Also consider including the trace ID in all JSON responses for consistency (e.g., the final 400 fallback). For example:
return NextResponse.json({ message: `No redirect URL provided. trace: ${uid}` }, { status: 400 });
| try { | ||
| const { redirect_url, error } = await oauthController.samlResponse(requestData); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also wrap parseRequestData in the try/catch.
parseRequestData(req) can throw (malformed body, unexpected content-type). Move it inside the try to return a consistent 500 with the same trace ID.
Example rewrite:
try {
const requestData = (await parseRequestData(req)) as SAMLResponsePayload;
const { redirect_url, error } = await oauthController.samlResponse(requestData);
// ...
} catch (error) {
log.error(`Error processing SAML response`, error);
return NextResponse.json({ message: `Error processing SAML response. trace: ${uid}` }, { status: 500 });
}🤖 Prompt for AI Agents
In apps/web/app/api/auth/saml/callback/route.ts around lines 18 to 20, the call
to parseRequestData is outside the try/catch block, but it can throw errors on
malformed input. Move the parseRequestData call inside the existing try block so
that any errors it throws are caught and handled consistently with the rest of
the code, returning a 500 response with the trace ID.
| if (error) { | ||
| const uid = uuid(); | ||
| log.error(`Error authenticating user with error ${error} for relayState ${requestData?.RelayState}`); | ||
| return NextResponse.json({ message: `Error authorizing user. trace: ${uid}` }, { status: 400 }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t generate a second trace ID; reuse the request’s uid.
Redeclaring const uid here shadows the request-scoped uid and breaks log/response correlation.
Apply this diff:
- const uid = uuid();
- log.error(`Error authenticating user with error ${error} for relayState ${requestData?.RelayState}`);
+ // reuse request-scoped uid from the logger prefix
+ log.error(`Error authenticating user with error ${error}`);
return NextResponse.json({ message: `Error authorizing user. trace: ${uid}` }, { status: 400 });Optional: avoid logging raw RelayState as it may contain user-provided context; log a boolean or a redacted snippet instead.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the SAML callback route file and display its top 200 lines with line numbers for context
FILE=$(find apps/web -type f -path "*/api/auth/saml/callback/route.ts")
if [[ -z "$FILE" ]]; then
echo "Error: route.ts not found under apps/web/app/api/auth/saml/callback"
exit 1
fi
echo "Inspecting file: $FILE"
nl -ba "$FILE" | sed -n '1,200p'Length of output: 480
🏁 Script executed:
#!/bin/bash
set -e
# Display lines 1-60 with line numbers for context
awk '{printf("%4d %s\n", NR, $0)}' apps/web/app/api/auth/saml/callback/route.ts | sed -n '1,60p'Length of output: 1794
Reuse the request-scoped trace ID for error logging and response
The const uid = uuid() on line 26 shadows the request’s original uid (from line 12), breaking log/response correlation. Instead, remove the shadowed declaration and use the existing uid. You may also choose to redact or omit raw RelayState in the log.
• File: apps/web/app/api/auth/saml/callback/route.ts
• Lines needing update: 25–29
Suggested diff:
@@ apps/web/app/api/auth/saml/callback/route.ts:25-29
- const uid = uuid();
- log.error(`Error authenticating user with error ${error} for relayState ${requestData?.RelayState}`);
+ // reuse request-scoped uid from the logger prefix
+ log.error(`Error authenticating user with error ${error}`);
return NextResponse.json({ message: `Error authorizing user. trace: ${uid}` }, { status: 400 });Optional: replace logging of requestData.RelayState with a boolean or redacted snippet (e.g. { hasRelayState: !!requestData.RelayState }) to avoid logging raw user data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error) { | |
| const uid = uuid(); | |
| log.error(`Error authenticating user with error ${error} for relayState ${requestData?.RelayState}`); | |
| return NextResponse.json({ message: `Error authorizing user. trace: ${uid}` }, { status: 400 }); | |
| } | |
| if (error) { | |
| // reuse request-scoped uid from the logger prefix | |
| log.error(`Error authenticating user with error ${error}`); | |
| return NextResponse.json({ message: `Error authorizing user. trace: ${uid}` }, { status: 400 }); | |
| } |
🤖 Prompt for AI Agents
In apps/web/app/api/auth/saml/callback/route.ts lines 25 to 29, remove the
redeclaration of uid with uuid() to avoid shadowing the original request-scoped
uid declared earlier. Use the existing uid variable for both logging and the
response message to maintain trace correlation. Additionally, modify the log
statement to avoid logging the raw RelayState value by replacing it with a
boolean indicating its presence or a redacted snippet.
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