fix: prevent information disclosure via error messages in controllers (#162) - #171
Conversation
|
@Yerimahjr Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughControllers across the application now pass unhandled exceptions to Express error middleware via ChangesController error propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/event-ticket.controller.test.ts (1)
152-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen all error-propagation assertions.
These tests currently accept any
Errorand only forbidres.status; they would pass if the controller replaced the original error or responded directly viares.json.
tests/event-ticket.controller.test.ts#L152-L166: capture the service rejection, assertnextis called once with that exact error, and verify bothres.statusandres.jsonare unused.tests/message-center.controller.test.ts#L157-L170: apply the same exact-error and no-response assertions tocreateMessage.tests/message-center.controller.test.ts#L240-L256: apply the same assertions togetScheduledMessages.tests/news.controller.test.ts#L99-L111: apply the same assertions toincrementReadCount.tests/news.controller.test.ts#L171-L181: apply the same assertions togetSingleNewsBySlug.🤖 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 `@tests/event-ticket.controller.test.ts` around lines 152 - 166, Strengthen the unexpected-error tests by storing each mocked service rejection in a shared error instance, asserting the controller calls next exactly once with that same instance, and verifying neither res.status nor res.json is called. Apply this to tests/event-ticket.controller.test.ts:152-166 for searchEventTickets, tests/message-center.controller.test.ts:157-170 for createMessage and :240-256 for getScheduledMessages, and tests/news.controller.test.ts:99-111 for incrementReadCount and :171-181 for getSingleNewsBySlug.
🤖 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 `@src/controllers/ticket-order.controller.ts`:
- Around line 79-80: Restore the corrupted comments in the verify-payment flow,
including the comment at the payment verification section and the related
comments near the ticket issuance and completion sections. Replace the mojibake
text with the intended readable Unicode punctuation and wording, without
changing executable code.
---
Nitpick comments:
In `@tests/event-ticket.controller.test.ts`:
- Around line 152-166: Strengthen the unexpected-error tests by storing each
mocked service rejection in a shared error instance, asserting the controller
calls next exactly once with that same instance, and verifying neither
res.status nor res.json is called. Apply this to
tests/event-ticket.controller.test.ts:152-166 for searchEventTickets,
tests/message-center.controller.test.ts:157-170 for createMessage and :240-256
for getScheduledMessages, and tests/news.controller.test.ts:99-111 for
incrementReadCount and :171-181 for getSingleNewsBySlug.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5b33485-f52e-4010-9728-ebaad99f2037
📒 Files selected for processing (17)
src/controllers/event-ticket.controller.tssrc/controllers/login.controller.tssrc/controllers/media.controller.tssrc/controllers/message-center.controller.tssrc/controllers/news.controller.tssrc/controllers/organizer-balance.controller.tssrc/controllers/otp.controller.tssrc/controllers/queue-monitor.controller.tssrc/controllers/resendotp.controller.tssrc/controllers/ticket-order.controller.tssrc/controllers/verify-attend.controller.tssrc/controllers/verify.controller.tssrc/controllers/waitlist.controller.tssrc/controllers/zkemail.controller.tstests/event-ticket.controller.test.tstests/message-center.controller.test.tstests/news.controller.test.ts
DioChuks
left a comment
DioChuks
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Great Work!
Thanks for your contribution!
2 participants
Summary
Closes #162. Controllers across the app were leaking raw internal
error.messagetext directly to clients on 500 responses (and in some cases even on non-500 responses), exposing exception details, stack-adjacent info, and other internals that shouldn't reach an end user.What was already there
This repo already had solid infrastructure for this: a centralized
globalErrorHandler(handles ZodError formatting, AppError subclasses safely, and masks generic 500s) and a fullAppErrorclass taxonomy (NotFoundError,ValidationError,ConflictError, etc.). The actual gap was that controllers weren't using it — they were catching errors and responding directly instead of callingnext(error).What changed
14 controllers fixed, in two passes:
login,otp,resendotp,zkemail,verify,organizer-balance,queue-monitor,verify-attend) — straightforward: replacedres.status(500).json({ message: error.message })withnext(error).event-ticket,media,message-center,news,ticket-order,waitlist) — more care taken here: legitimate, intentional status-code branches (e.g. mapping a known "News article not found" message to 404, or a Zod validation failure to 400) were left untouched, since those aren't the vulnerability. Only the generic catch-all 500 fallback in each was replaced withnext(error).Also fixed a related correctness bug found along the way in
news.controller.ts'supdateNews: it had no fallback branch at all, so an unexpected error would leave the request hanging with no response.Testing
Updated 3 test files (
event-ticket,message-center,news.controller.test.ts) that were asserting the old raw-500 behavior directly — they now assertnextwas called with anErrorinstead.Full suite: 276/276 tests passing, 36/36 suites, zero regressions.
Summary by CodeRabbit
Bug Fixes
Tests