fix(lib): prevent data leak and improve HTTP error handling robustness - #29866
Open
adwaitm1301 wants to merge 1 commit into
Open
fix(lib): prevent data leak and improve HTTP error handling robustness#29866adwaitm1301 wants to merge 1 commit into
adwaitm1301 wants to merge 1 commit into
Conversation
Contributor
|
Welcome to Cal.diy, @adwaitm1301! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughUpdated shared response handlers to parse error bodies safely, handle empty and gzip-compressed responses, and report JSON parsing failures. Updated the HTTP wrapper to preserve parsed error messages for failed requests and provide fallbacks for non-JSON bodies. Added Vitest coverage for response handling, unknown error conversion, HTTP methods, successful JSON responses, HTTP errors, and network failures. 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/lib/fetch-wrapper.test.ts (1)
28-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert each helper’s request method and body.
The PUT, PATCH, and DELETE tests pass even if the wrapper sends the wrong method or omits the serialized body. Add
mockFetchrequest assertions for every helper.🤖 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/lib/fetch-wrapper.test.ts` around lines 28 - 65, Expand the assertions in the PUT, PATCH, and DELETE tests alongside the existing POST assertion to verify mockFetch receives the correct HTTP method and serialized request body for each helper. Use the request payload already supplied in each test and preserve the existing response and result assertions.
🤖 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/lib/errors.ts`:
- Around line 83-85: Replace generic Error instances in packages/lib/errors.ts
at lines 83-85, 90-91, and 99-101 with ErrorWithCode, assigning appropriate
structured codes for non-OK JSON responses, successful-response JSON parse
failures, and non-OK raw responses respectively. Preserve the existing response
status, body, and parse-error context in each error message.
- Around line 61-67: Update the JSON parsing logic in the error-message helper
to safely inspect the parsed value before accessing message. Return
parsed.message only when it is a non-empty string; otherwise consistently return
the raw body, including when the parsed value is null or another incompatible
shape.
- Around line 74-76: Update the response handling in the surrounding
error-parsing function to validate response.ok before entering the gzip branch,
so gzip-encoded 4xx/5xx responses follow the existing error path instead of
being parsed as Type. Preserve the successful gzip parsing behavior and add a
regression test covering a gzip error response.
In `@packages/lib/fetch-wrapper.ts`:
- Around line 3-9: Update getErrorBody to read the response body as text once,
then conditionally parse valid JSON while preserving non-JSON content. Ensure
parsed JSON is validated as a non-null object before returning it, and otherwise
return a message using body || statusText || String(status) so HttpError
creation remains safe.
---
Nitpick comments:
In `@packages/lib/fetch-wrapper.test.ts`:
- Around line 28-65: Expand the assertions in the PUT, PATCH, and DELETE tests
alongside the existing POST assertion to verify mockFetch receives the correct
HTTP method and serialized request body for each helper. Use the request payload
already supplied in each test and preserve the existing response and result
assertions.
🪄 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 Plus
Run ID: a5918fd6-c67d-4dd5-a690-9ddf541ebffc
📒 Files selected for processing (4)
packages/lib/errors.test.tspackages/lib/errors.tspackages/lib/fetch-wrapper.test.tspackages/lib/fetch-wrapper.ts
- Remove console.log(data) leak from handleErrorsJson that exposed raw error bodies
- Remove console.error({ response }) and response.text().then(console.log) from handleErrorsRaw
- Add getResponseBody() helper: safely extracts body text, tries JSON.parse for .message
- Fix getErrorBody() fallback when response.statusText is empty (Node.js HTTP/2)
- Add comprehensive unit tests (21 tests) for errors.ts and fetch-wrapper.ts
|
|
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.
This PR fixes two issues in packages/lib/errors.ts and packages/lib/fetch-wrapper.ts.
Changes: