Refactor error response handling in proxy plugin#1258
Conversation
Updated error handling to avoid leaking any information of the use of proxy to the client, and preserve more detailed log server-side
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates the error response plugin to enhance server-side error observability while simplifying client-facing error messages. The plugin now logs complete proxy error details (including request host and URL) to the server console when a response is available, and returns a generic constant body ChangesError Response Plugin Update
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/default/error-response-plugin.ts (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import.
The
sanitizeimport is no longer used after switching to the generic constant error message.🧹 Proposed fix
-import { sanitize } from '../../utils/sanitize.js';🤖 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 `@src/plugins/default/error-response-plugin.ts` at line 6, Remove the unused import "sanitize" from the top-level imports in error-response-plugin.ts: locate the import statement that imports sanitize (import { sanitize } from '../../utils/sanitize.js';) and delete it so there are no unused imports remaining after switching to the generic constant error message.
🤖 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/plugins/default/error-response-plugin.ts`:
- Around line 23-24: The log call in error-response-plugin.ts uses
req.headers.host and req.url directly which can be undefined; update the
console.error invocation to use safe fallbacks (e.g., (req.headers?.host ??
'<no-host>') and (req.url ?? '<no-url>')) so the message never contains
"undefined" and still includes useful context; preserve the err argument so full
error details are logged.
---
Outside diff comments:
In `@src/plugins/default/error-response-plugin.ts`:
- Line 6: Remove the unused import "sanitize" from the top-level imports in
error-response-plugin.ts: locate the import statement that imports sanitize
(import { sanitize } from '../../utils/sanitize.js';) and delete it so there are
no unused imports remaining after switching to the generic constant error
message.
🪄 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
Run ID: 1e564c64-ac21-48e6-a207-ef577e31906c
📒 Files selected for processing (1)
src/plugins/default/error-response-plugin.ts
fallback values for undefined
1 participant
Updated error handling
avoid leaking any information of the use of proxy to the client, and preserve more detailed log server-side
Motivation and Context
The details of the backend using proxy for services are not for the client, this sanitizes that information
How has this been tested?
The error could not be trapped in my backend code and the client persistently saw an error message that did not leak much information but did include "proxy" which is not for the client to see.
Types of changes
Checklist:
Summary by CodeRabbit