Repository navigation
fix: address code-review findings across node, express and sirv - #347
Conversation
Resolves the verified findings from the xhigh code review: - express: the synthetic request socket is a real EventEmitter, so a body parser draining an oversized body (413) no longer crashes on `socket.on`. - node: `X-Forwarded-*` is authoritative and read first-value (Express `trust proxy` semantics); RFC 7239 `Forwarded` only fills params the legacy header omits, so a passed-through client `Forwarded` cannot override the proxy. `isClientGone` also covers `ERR_STREAM_DESTROYED`/`ABORT_ERR`, and the HEAD check guards a possibly-absent `req`. - sirv: a backwards range serves 200 instead of 416 (416 reserved for past-EOF); `Accept-Encoding` honors `*` and a valueless `q`; a missing dir in dev mode no longer throws at construction. - node: documented that `responseAdapter` honors only the `TRUST_PROXY` env var, not `createRequestAdapter`'s `trustProxy` option.
🦋 Changeset detectedLatest commit: ee7188c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 46 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR updates three areas: Sirv now ignores backwards ranges, improves Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🧹 Nitpick comments (2)
packages/sirv/tests/ranges.spec.ts (1)
155-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the response body, not only its metadata.
A response with the expected
Content-Lengthcould still contain incorrect or truncated bytes. Validate the full body to prove the backwards range is ignored.Suggested test assertion
assert.equal(res.headers["content-range"], undefined); + await utils.matches(res, 200, "bundle.67329.js", "utf8");🤖 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/sirv/tests/ranges.spec.ts` around lines 155 - 166, Extend the “should ignore a backwards range and serve the full file” test to assert that the response body exactly matches the complete file contents loaded by utils.lookup, in addition to the existing status and header checks. Use the existing file value and response body representation used by nearby tests.packages/node/src/request.ts (1)
48-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
for/IP-address clause from this JSDoctrustProxyonly affectsprotoandhosthere, so mentioningforimplies client-IP handling that this package doesn’t implement.🤖 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/node/src/request.ts` around lines 48 - 54, Update the JSDoc for the trustProxy option to remove the clause stating that the Forwarded header’s “for” value determines the IP address. Keep the documentation describing only proto and host handling, including the existing precedence and first-entry behavior.
🤖 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/sirv/src/middleware.ts`:
- Around line 292-295: Update the coding-acceptance loop in the middleware so it
evaluates every explicit coding match instead of returning immediately on an
unacceptable match. Track whether any explicit coding matched, return true if
any matched coding is acceptable, and use the wildcard fallback only when no
explicit coding matched.
- Around line 302-309: Update the fs.realpathSync error handling in the
middleware initialization catch to defer only errors whose code is ENOENT when
opts.dev is enabled; re-throw permission, broken-symlink, invalid-path, and all
other errors, while preserving the existing realDir fallback for missing
directories.
---
Nitpick comments:
In `@packages/node/src/request.ts`:
- Around line 48-54: Update the JSDoc for the trustProxy option to remove the
clause stating that the Forwarded header’s “for” value determines the IP
address. Keep the documentation describing only proto and host handling,
including the existing precedence and first-entry behavior.
In `@packages/sirv/tests/ranges.spec.ts`:
- Around line 155-166: Extend the “should ignore a backwards range and serve the
full file” test to assert that the response body exactly matches the complete
file contents loaded by utils.lookup, in addition to the existing status and
header checks. Use the existing file value and response body representation used
by nearby tests.
🪄 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
Run ID: 796a1236-616d-4632-826b-db67d29477b5
📒 Files selected for processing (13)
.changeset/lemon-carts-melt.md.changeset/soft-rings-flow.md.changeset/witty-otters-hunt.mdpackages/adapter-express/src/utils.tspackages/adapter-express/tests/connect-to-web.spec.tspackages/adapter-express/tests/node-forwarded.spec.tspackages/node/src/forwarded.tspackages/node/src/request.tspackages/node/src/response.tspackages/sirv/src/middleware.tspackages/sirv/tests/encoding.spec.tspackages/sirv/tests/ranges.spec.tspackages/sirv/tests/sirv.spec.ts
`acceptsEncoding` returned on the first matching coding, so `gzip;q=0, x-gzip` reported gzip as refused even though the client accepts it under its alias. It now accepts if any aliased spelling is acceptable, with an explicit coding still overriding the `*` wildcard. The dev-mode `realpathSync` fallback now defers only a genuinely missing directory (`ENOENT`); permission errors, non-directory path components, and other resolution failures are real misconfigurations and are surfaced instead of being degraded into request-time 404s.
socket.on.X-Forwarded-*is authoritative and read first-value (Expresstrust proxysemantics); RFC 7239Forwardedonly fills params the legacy header omits, so a passed-through clientForwardedcannot override the proxy.isClientGonealso coversERR_STREAM_DESTROYED/ABORT_ERR, and the HEAD check guards a possibly-absentreq.Accept-Encodinghonors*and a valuelessq; a missing dir in dev mode no longer throws at construction.responseAdapterhonors only theTRUST_PROXYenv var, notcreateRequestAdapter'strustProxyoption.Summary by CodeRabbit
Accept-Encodingwildcard and quality-value negotiation.HEADrequests and redirect behavior.