fix: remove stale Content-Length when body is set to ReadableStream or Response#1939
fix: remove stale Content-Length when body is set to ReadableStream or Response#1939fengmk2 wants to merge 1 commit into
Conversation
…r Response
When ctx.body is replaced with a ReadableStream or Response after being
set to a string/buffer, the previously-set Content-Length header was not
removed. This could cause Content-Length mismatches, leading to body
truncation or keep-alive connection corruption.
The fix adds `this.remove('Content-Length')` to the ReadableStream and
Response handlers, consistent with the existing Stream handler behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1939 +/- ##
=======================================
Coverage 99.90% 99.90%
=======================================
Files 9 9
Lines 2094 2096 +2
=======================================
+ Hits 2092 2094 +2
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an HTTP response header correctness issue where Content-Length could become stale when replacing an already-sized body (e.g., a string) with a ReadableStream or a Response, and adds regression tests to prevent reintroduction.
Changes:
- Remove
Content-Lengthwhen switchingres.bodyto aReadableStream(when there was a previous body). - Remove
Content-Lengthwhen switchingres.bodyto aResponse(when there was a previous body). - Add tests asserting
Content-Lengthis cleared in both replacement scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/response.js |
Clears stale Content-Length when body is replaced with ReadableStream or Response. |
__tests__/response/body.test.js |
Adds regression tests ensuring Content-Length is removed on those body replacements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
guoyangzhen
left a comment
There was a problem hiding this comment.
Good catch. The Blob handler already sets this.length = val.size which implicitly overwrites Content-Length, but ReadableStream and Response handlers were silently keeping the stale value.
One thing to consider: the Blob handler at line 209 doesn't have this issue because it always sets this.length. But if you wanted to be extra consistent, you could also add if (original != null) this.remove('Content-Length') before this.length = val.size in the Blob handler — though functionally it's equivalent since the setter overwrites it anyway.
LGTM!
Problem
When
ctx.bodyis replaced with aReadableStreamorResponseafter being set to a string/buffer, the previously-setContent-Lengthheader is not removed. This causes a Content-Length mismatch between the header and the actual body, which can lead to:The existing
Streamhandler already correctly removesContent-Lengthwhen replacing a body, but theReadableStreamandResponsehandlers were missing this logic.Fix
this.remove('Content-Length')whenoriginal != null(consistent with Stream handler)this.remove('Content-Length')whenoriginal != nullbefore copying headers from the Response object. This ensures any stale Content-Length from a previous body is cleared; if the Response itself has a Content-Length, it will be set correctly when its headers are copied.Tests
Added two test cases in
__tests__/response/body.test.js:All 431 existing tests pass.