E54A fix: prevent sending raw errors in responses by dhairyashiil · Pull Request #24282 · calcom/cal.com · GitHub
[go: up one dir, main page]
Skip to content

fix: prevent sending raw errors in responses #24282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: prevent sending raw errors in responses #24282

wants to merge 2 commits into from
+54 −65

Conversation

dhairyashiil
Copy link
Member

What does this PR do?

This PR addresses issues like #20915, I was not able to reproduce that error again as mentioned by Rajiv, it might have occurred due to a failed migration. So, I don’t think this is still an issue.

As per the discussion with Pallav and Rajiv, a try-catch block was necessary. After reviewing the codebase, I noticed that most places already have a catch block, but in many of them, error.message is being sent directly in the response. I also found a utility function getServerErrorFromUnknown, which seems to be designed for handling such cases. I’m not sure which file the issue mentioned above originated from, but I’ve tried to update as many files as possible where error.message was being sent directly in the response.

Discussion link: https://calendso.slack.com/archives/C08B8SYUY02/p1759573378526789?thread_ts=1759562047.239579&cid=C08B8SYUY02

@dhairyashiil dhairyashiil requested a review from a team as a code owner October 5, 2025 22:08
@vercel Vercel
Copy link
vercel bot commented Oct 5, 2025

@dhairyashiil is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Oct 5, 2025
@graphite-app graphite-app bot requested a review from a team October 5, 2025 22:08
@keithwillcode keithwillcode added the community-interns The team responsible for reviewing, testing and shipping low/medium community PRs label Oct 5, 2025
@coderabbitai coderabbitai
Copy link
Contributor
coderabbitai bot commented Oct 5, 2025 •
edited
Loading

Walkthrough

The change centralizes server-side error handling across multiple API handlers and one booking utility by importing and using getServerErrorFromUnknown to derive httpError.statusCode and httpError.message inside catch blocks. It replaces prior instanceof/Error or ad-hoc 500 branches with a unified error-mapping path across many app-store APIs (alby, giphy, googlevideo, hitpay, jitsivideo, paypal, sylapsvideo, wipemycalother, zapier), the integrations API route, and features/bookings loadUsers. Minor local tweaks: teamId/userId normalization in some admin-access calls and a small truthiness simplification in loadDynamicUsers. No exported/public signatures changed.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely and accurately summarizes the primary change of the pull request by stating that raw error messages will no longer be sent in responses. It clearly reflects the updates to centralize error handling through the getServerErrorFromUnknown utility. It follows conventional commit style and will be understandable to teammates scanning the history.
Description Check ✅ Passed The description provides clear context about why the change was made by referencing issue #20915 and Slack discussions with stakeholders, and it explains that the PR updates multiple catch blocks to avoid sending error.message directly by leveraging getServerErrorFromUnknown. It directly relates to the changes in the pull request and outlines the scope of affected files without introducing unrelated information.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b272b95 and 243402a.

📒 Files selected for processing (1)
  • packages/app-store/hitpay/api/add.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/app-store/hitpay/api/add.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:

  • packages/app-store/hitpay/api/add.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/app-store/hitpay/api/add.ts
🧬 Code graph analysis (1)
packages/app-store/hitpay/api/add.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Install dependencies / Yarn install & cache
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
packages/app-store/hitpay/api/add.ts (2)

33-33: Error message fix applied.

The error message now correctly references "hitpay" instead of "Alby", addressing the previous review comment.

3-3: LGTM: Centralized error handling implemented correctly.

The implementation follows the PR objective by using getServerErrorFromUnknown to standardize error responses. This prevents raw error messages from being exposed while maintaining appropriate HTTP status codes and user-friendly messages.

Also applies to: 35-38

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot dosubot bot added api area: API, enterprise API, access token, OAuth 🐛 bug Something isn't working labels Oct 5, 2025
coderabbitai[bot]
coderabbitai bot reviewed Oct 5, 2025
View reviewed changes
Copy link
Contributor
@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a4be822 and b272b95.

📒 Files selected for processing (14)
  • apps/web/pages/api/integrations/[...args].ts (2 hunks)
  • packages/app-store/alby/api/add.ts (2 hunks)
  • packages/app-store/giphy/api/add.ts (2 hunks)
  • packages/app-store/giphy/api/get.ts (2 hunks)
  • packages/app-store/giphy/api/search.ts (2 hunks)
  • packages/app-store/googlevideo/api/_getAdd.ts (2 hunks)
  • packages/app-store/hitpay/api/add.ts (2 hunks)
  • packages/app-store/jitsivideo/api/add.ts (3 hunks)
  • packages/app-store/paypal/api/add.ts (3 hunks)
  • packages/app-store/sylapsvideo/api/add.ts (2 hunks)
  • packages/app-store/wipemycalother/api/add.ts (2 hunks)
  • packages/app-store/wipemycalother/api/wipe.ts (2 hunks)
  • packages/app-store/zapier/api/add.ts (2 hunks)
  • packages/features/bookings/lib/handleNewBooking/loadUsers.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/app-store/giphy/api/get.ts
  • packages/app-store/hitpay/api/add.ts
  • packages/app-store/googlevideo/api/_getAdd.ts
  • packages/app-store/jitsivideo/api/add.ts
  • packages/app-store/alby/api/add.ts
  • apps/web/pages/api/integrations/[...args].ts
  • packages/app-store/giphy/api/add.ts
  • packages/app-store/zapier/api/add.ts
  • packages/app-store/giphy/api/search.ts
  • packages/app-store/sylapsvideo/api/add.ts
  • packages/app-store/wipemycalother/api/add.ts
  • packages/features/bookings/lib/handleNewBooking/loadUsers.ts
  • packages/app-store/wipemycalother/api/wipe.ts
  • packages/app-store/paypal/api/add.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:

  • packages/app-store/giphy/api/get.ts
  • packages/app-store/hitpay/api/add.ts
  • packages/app-store/googlevideo/api/_getAdd.ts
  • packages/app-store/jitsivideo/api/add.ts
  • packages/app-store/alby/api/add.ts
  • apps/web/pages/api/integrations/[...args].ts
  • packages/app-store/giphy/api/add.ts
  • packages/app-store/zapier/api/add.ts
  • packages/app-store/giphy/api/search.ts
  • packages/app-store/sylapsvideo/api/add.ts
  • packages/app-store/wipemycalother/api/add.ts
  • packages/features/bookings/lib/handleNewBooking/loadUsers.ts
  • packages/app-store/wipemycalother/api/wipe.ts
  • packages/app-store/paypal/api/add.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/app-store/giphy/api/get.ts
  • packages/app-store/hitpay/api/add.ts
  • packages/app-store/googlevideo/api/_getAdd.ts
  • packages/app-store/jitsivideo/api/add.ts
  • packages/app-store/alby/api/add.ts
  • apps/web/pages/api/integrations/[...args].ts
  • packages/app-store/giphy/api/add.ts
  • packages/app-store/zapier/api/add.ts
  • packages/app-store/giphy/api/search.ts
  • packages/app-store/sylapsvideo/api/add.ts
  • packages/app-store/wipemycalother/api/add.ts
  • packages/features/bookings/lib/handleNewBooking/loadUsers.ts
  • packages/app-store/wipemycalother/api/wipe.ts
  • packages/app-store/paypal/api/add.ts
🧬 Code graph analysis (14)
packages/app-store/giphy/api/get.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/hitpay/api/add.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/googlevideo/api/_getAdd.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/jitsivideo/api/add.ts (2)
packages/app-store/_utils/throwIfNotHaveAdminAccessToTeam.ts (1)
  • throwIfNotHaveAdminAccessToTeam (5-23)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/alby/api/add.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
apps/web/pages/api/integrations/[...args].ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/giphy/api/add.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/zapier/api/add.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/giphy/api/search.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/sylapsvideo/api/add.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/wipemycalother/api/add.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/features/bookings/lib/handleNewBooking/loadUsers.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/wipemycalother/api/wipe.ts (1)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
packages/app-store/paypal/api/add.ts (2)
packages/app-store/_utils/throwIfNotHaveAdminAccessToTeam.ts (1)
  • throwIfNotHaveAdminAccessToTeam (5-23)
packages/lib/server/getServerErrorFromUnknown.ts (1)
  • getServerErrorFromUnknown (39-98)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Install dependencies / Yarn install & cache
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (25)
apps/web/pages/api/integrations/[...args].ts (2)

8-8: LGTM!

The import for getServerErrorFromUnknown is correctly added to support centralized error handling.

79-80: LGTM!

The error handling is correctly standardized to use getServerErrorFromUnknown, which prevents leaking raw error details to clients while providing appropriate HTTP status codes and safe error messages.

packages/app-store/sylapsvideo/api/add.ts (2)

3-3: LGTM!

The import for getServerErrorFromUnknown is correctly added.

42-43: LGTM!

The error handling is correctly standardized using getServerErrorFromUnknown.

packages/app-store/wipemycalother/api/wipe.ts (2)

5-5: LGTM!

The import for getServerErrorFromUnknown is correctly added.

59-60: LGTM!

The error handling is correctly standardized using getServerErrorFromUnknown.

packages/app-store/zapier/api/add.ts (2)

3-3: LGTM!

The import for getServerErrorFromUnknown is correctly added.

33-34: LGTM!

The error handling is correctly standardized using getServerErrorFromUnknown.

packages/app-store/wipemycalother/api/add.ts (2)

3-3: LGTM!

The import for getServerErrorFromUnknown is correctly added.

40-41: LGTM!

The error handling is correctly standardized using getServerErrorFromUnknown.

packages/app-store/giphy/api/add.ts (2)

3-3: LGTM!

The import for getServerErrorFromUnknown is correctly added.

48-49: LGTM!

The error handling is correctly standardized using getServerErrorFromUnknown.

packages/app-store/giphy/api/search.ts (2)

4-4: LGTM!

The import for getServerErrorFromUnknown is correctly added.

43-44: LGTM!

The error handling is correctly standardized using getServerErrorFromUnknown.

packages/app-store/hitpay/api/add.ts (2)

3-3: LGTM!

The import for getServerErrorFromUnknown is correctly added.

36-37: LGTM!

The error handling is correctly standardized using getServerErrorFromUnknown.

packages/app-store/googlevideo/api/_getAdd.ts (1)

3-3: LGTM! Improved error handling security.

The import and usage of getServerErrorFromUnknown centralizes error handling and prevents raw error messages from being exposed in HTTP responses. The error mapper handles various error types (TRPCError, ZodError, PrismaError, HttpError) appropriately and redacts sensitive information.

Also applies to: 36-37

packages/app-store/alby/api/add.ts (1)

3-3: LGTM! Consistent error handling improvement.

The centralized error handling using getServerErrorFromUnknown aligns with the PR objective to prevent raw error messages in responses and follows the same secure pattern applied across other API handlers.

Also applies to: 36-37

packages/app-store/giphy/api/get.ts (1)

4-5: LGTM! Centralized error handling implemented.

The use of getServerErrorFromUnknown improves error response security by preventing raw error messages from being exposed and ensuring consistent status code mapping.

Also applies to: 40-41

packages/app-store/jitsivideo/api/add.ts (2)

4-4: LGTM! Centralized error handling implemented.

The use of getServerErrorFromUnknown ensures consistent and secure error responses by preventing raw error messages from being exposed.

Also applies to: 49-50

20-23: LGTM! Clear parameter formatting.

The explicit parameter formatting with teamId: teamId ? Number(teamId) : null and userId: req.session.user.id improves readability while maintaining correct behavior per the function signature.

packages/app-store/paypal/api/add.ts (2)

4-4: LGTM! Consistent error handling improvement.

The centralized error handling using getServerErrorFromUnknown aligns with the PR objective and provides secure, consistent error responses across API handlers.

Also applies to: 47-48

16-19: LGTM! Clear parameter formatting.

The explicit parameter formatting improves readability and maintains correct behavior per the throwIfNotHaveAdminAccessToTeam function signature.

packages/features/bookings/lib/handleNewBooking/loadUsers.ts (2)

9-9: LGTM! Centralized error handling in utility function.

The use of getServerErrorFromUnknown to transform and throw the error is appropriate for this utility function, allowing proper error propagation to callers with consistent error structure and redacted sensitive information.

Also applies to: 72-73

101-101: LGTM! Cleaner truthiness check.

Simplifying from !!currentOrgDomain ? currentOrgDomain : null to currentOrgDomain ? currentOrgDomain : null is correct and more concise, as both expressions are equivalent for the string | null type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
api area: API, enterprise API, access token, OAuth 🐛 bug Something isn't working community Created by Linear-GitHub Sync community-interns The team responsible for reviewing, testing and shipping low/medium community PRs size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhairyashiil @keithwillcode
0