Repository navigation
Conversation
…esent Content-Length and Transfer-Encoding can't be present in the response headers together, so Content-Length should be added only if there is no Transfer-Encoding header. The body length still needs to be calculated on every path because the automatic ETag depends on it, so only the header write is guarded. This imports the same invariant master already carries from expressjs#4893 and expressjs#7459 onto the 4.x branch.
kgeminicdev
left a comment
kgeminicdev
left a comment
There was a problem hiding this comment.
Thanks for the backport. I checked it against master and it matches: the guard in res.send is identical to master after #4893 and #7459, and the length is still computed for the ETag.
I also verified it locally on 4.x (899b524): with lib/response.js reverted, the new test fails with Parse Error: Content-Length can't be present with Transfer-Encoding, and with this change it passes, along with the rest of test/res.send.js (71 passing).
One small suggestion inline: porting #4893's tests too.
Not for this PR, just noting it: res.redirect still sets Content-Length unconditionally (on both 4.x and master), so it could hit the same conflict if Transfer-Encoding was set earlier. That might be worth a separate follow-up.
| }) | ||
| }) | ||
|
|
||
| describe('when Transfer-Encoding is set', function () { |
There was a problem hiding this comment.
Could this also carry over the tests from #4893? They cover an empty body (send('')) and the other Transfer-Encoding values (compress, deflate, gzip), which this test doesn't:
describe('when Transfer-Encoding header is present', function(){
['chunked', 'compress', 'deflate', 'gzip'].forEach(function(encoding){
it('should not add Content-Length header if Transfer-Encoding header is equal to ' + encoding, function(done){
var app = express();
app.use(function(_, res){
res.status(200).set('Transfer-Encoding', encoding).send('');
});
request(app)
.get('/')
.expect(utils.shouldNotHaveHeader('Content-Length'))
.expect(utils.shouldHaveHeader('Transfer-Encoding'))
.expect(200, '', done);
})
});
})I ran them against this branch unchanged and all 4 pass, so it's just for coverage parity with master.
There was a problem hiding this comment.
good idea @kgeminicdev, i ported the four encoding cases from #4893 so chunked, compress, deflate and gzip are all covered now, and i reverted lib/response.js on the branch to be sure they bite, all five fail with the same Content-Length and Transfer-Encoding parse error without the fix, then pass with it, full test/res.send.js run is 76 passing and the whole test folder is 1222 passing, pushed as a separate commit a308170 so the sha you reviewed is still the one you tested
…js#4893 master pinned chunked, compress, deflate and gzip plus the empty body in expressjs#4893, this 4.x backport only pinned chunked, so this adds the missing cases to keep the coverage the same as master
|
thanks for checking this against master and testing it locally on 4.x @kgeminicdev, really appreciate it, i ported the four encoding cases from #4893 as a separate commit a308170 so the sha you reviewed is untouched, on the res.redirect note you are right it can hit the same conflict, i will not mix it into this backport so this stays a straight port |
3 participants
What is the current behavior?
when res.send() runs with a Transfer-Encoding header already set it still adds Content-Length next to it. RFC 9110 section 8.6 says a server must not send Content-Length in a response with Transfer-Encoding, and node's own http client refuses to parse such a response
raw response from untouched 4.22.3 (origin/4.x 899b524), Transfer-Encoding set by the app, body and etag untouched:
a node client on the same wire fails before the body is even read:
so any node client hitting an express 4.x app that sets Transfer-Encoding (a streaming proxy handler, a gateway mixin, anything forwarding the hop-by-hop header) gets a connection level error on every response
Steps to reproduce
What is the new behavior?
the body length is still calculated on every path because the automatic ETag depends on it, that is the exact regression #7459 fixed on master, so only the header write is guarded:
same wire probe on the fixed branch:
204, 205 and 304 paths are untouched (they manage their own headers), HEAD responses keep skipping the body, and the full test run passes with the one new test proving the guard
Does this PR introduce a breaking change?