http: return `Content-Length` header for `HEAD`s #56681

qwerzl · 2025-01-21T16:08:25Z

In the current http library, all responses without body will not return the Content-Length header.

Fixes: #56680

nodejs-github-bot · 2025-01-21T16:08:31Z

Review requested:

@nodejs/http
@nodejs/net

geeksilva97 · 2025-01-21T18:05:35Z

Can you add a test that covers this change?

codecov · 2025-01-21T18:08:57Z

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (01dd7c4) to head (0f4470f).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/_http_outgoing.js	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56681   +/-   ##
=======================================
  Coverage   89.21%   89.22%           
=======================================
  Files         662      662           
  Lines      191934   191934           
  Branches    36953    36951    -2     
=======================================
+ Hits       171242   171250    +8     
- Misses      13536    13541    +5     
+ Partials     7156     7143   -13

Files with missing lines	Coverage Δ
lib/_http_outgoing.js	`95.15% <50.00%> (-0.08%)`	⬇️

... and 37 files with indirect coverage changes

In the current http library, all responses without body will not return the Content-Length header. Fixes: nodejs#56680

qwerzl · 2025-01-22T02:37:06Z

Can you add a test that covers this change?

@geeksilva97 Sure! I've modified the test file for HEADs to make sure that Content-Length is received. Also I've changed the message of the first commit to adhere to the guideline.

LiviaMedeiros

For the record, the specs do not require us to include this header.

8.6. Content-Length says:

A server MAY send a Content-Length header field in a response to a HEAD request (Section 9.3.2); a server MUST NOT send Content-Length in such a response unless its field value equals the decimal number of octets that would have been sent in the content of a response if the same request had used the GET method.

9.3.2. HEAD says:

The server SHOULD send the same header fields in response to a HEAD request as it would have sent if the request method had been GET. However, a server MAY omit header fields for which a value is determined only while generating the content. For example, some servers buffer a dynamic response to GET until a minimum amount of data is generated so that they can more efficiently delimit small responses or make late decisions with regard to content selection. Such a response to GET might contain Content-Length and Vary fields, for example, that are not generated within a HEAD response. These minor inconsistencies are considered preferable to generating and discarding the content for a HEAD request, since HEAD is usually requested for the sake of efficiency.

This PR aligns with the recommendations, but we might need to revisit this once HEAD will not touch the body and therefore will not know much about it.

The code LGTM with fixing linter error.

Also it would be great to have more specific test for this (for example, HEAD -> HEAD -> GET -> GET -> HEAD sequence of requests for different response bodies, making sure the value of Content-Length stays correct.

test/parallel/test-http-head-response-has-no-body-end-implicit-headers.js

Co-authored-by: Livia Medeiros <[email protected]>

test/parallel/test-http-head-response-has-no-body-end-implicit-headers.js

ronag

I'm a little worried this change might add some incorrect extra stuff at the end of HEAD replies which clients might interpret as body.

ronag · 2025-01-22T12:18:41Z

lib/_http_outgoing.js

@@ -547,7 +547,7 @@ function _storeHeader(firstLine, headers) {
  }

  if (!state.contLen && !state.te) {
-    if (!this._hasBody) {
+    if (!this._hasBody && this.req.method !== 'HEAD') {


If I'm understanding the logic correctly, the first if condition applies to those that should return neither of the Transfer-Encoding and Content-Length headers. While it's true for other requests that don't have a body, HEADs should match the 3rd condition (i.e. the !state.trailer && !this._removedContLen && typeof this._contentLength === 'number' one).

ronag

See comments above. Can we add a test that check that the RAW http message is correct, i.e. no extra tralining line delimiters etc...

mcollina · 2025-01-22T13:51:51Z

We should do a quick check if we are breaking anything in the ecosystem.

mcollina

I'm not really convinced this is something should be the responsibility of Node.js to do. All frameworks are handling this on top.

qwerzl · 2025-01-22T16:35:16Z

See comments above. Can we add a test that check that the RAW http message is correct, i.e. no extra tralining line delimiters etc...

@ronag I've added the test in 6458459.

qwerzl · 2025-01-22T17:16:09Z

I'm not really convinced this is something should be the responsibility of Node.js to do. All frameworks are handling this on top.

@mcollina For me, we use HEAD mostly to obtain the content length, and enabling it only requires changing a few if conditions. Just my two cents, after all the specs does not require that a server must return that.

pimterry

Thanks for contributing @qwerzl!

I think the correct logic for this is actually a bit more tricky that the current implementation, so this needs a few tweaks and some more testing to fix the edge cases I've listed here if it's going forward.

That said, I'm also sympathetic to @mcollina's view that this is a framework responsibility, not something for Node. I could be persuaded either way I think, but right now I'm gently against this addition I'm afraid.

Imo, Node should aim to provide a good clear base for low-level MUST requirements of HTTP, but without many 'clever' features (because there are many people doing low-level HTTP development on top of Node's APIs, such as building HTTP frameworks and tools, where these extra behaviours can be quite confusing/inconvenient). Frameworks on top of Node should where the optional conveniences come in, so users can opt into the right framework or raw-HTTP-no-frills behaviour that fits each use case, and Node can focus on core HTTP and stay simpler & relatively unopinionated about optional extras.

pimterry · 2025-01-22T17:05:52Z

lib/_http_outgoing.js

@@ -547,7 +547,7 @@ function _storeHeader(firstLine, headers) {
  }

  if (!state.contLen && !state.te) {
-    if (!this._hasBody) {
+    if (!this._hasBody && this.req.method !== 'HEAD') {


If you send a HEAD response with no body at all (e.g. because your code actually looks at the method, and skips it - not unusual I think) then what does this do?

It looks like it skips the correct case (this first one), skips the next 2 (useChunkedEncodingByDefault is true by default, _contentLength will be null) and instead sets a transfer-encoding: chunked header and potentially sends a 0\r\n\r\n end-of-chunk response in the body (as this comment suggests) even though HEAD shouldn't have a body.

I haven't actually tested this, but that's certainly how it looks. Either way we'll need a test for that case as it's a bit tricky - we definitely shouldn't accidentally send content-length: 0 which is probably wrong. Imo, if you don't write a body on HEAD, we should still hit this first case - sending no CL headers or chunking at all.

Oh, also, if you call removeHeader for TE & CL for a HEAD request (this is what you generally do to disable all Node's default header handling) this change will now break keep-alive for HEAD requests, where previously it didn't (and it doesn't need to). It will hit the _last = true case at the end, and close the connection after this response. That's correct for GET/etc because without those headers you don't know how long the body is. It's not correct for HEAD, because you do always know how long the body is: 0 bytes.

In both these cases, I think the issue is trying to squeeze this logic into the existing cases. This logic you want here is doable, but I think it's a bit more complicated than currently implemented here.

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jan 21, 2025

qwerzl changed the title ~~fix(http): return Content-Length header for HEADs~~ http: return Content-Length header for HEADs Jan 21, 2025

http: return Content-Length header for HEADs

8c4c5fc

In the current http library, all responses without body will not return the Content-Length header. Fixes: nodejs#56680

qwerzl force-pushed the fix/head-content-length branch from 4183136 to 8c4c5fc Compare January 22, 2025 02:28

test: make sure content-length headers are sent for HEADs

f129867

LiviaMedeiros mentioned this pull request Jan 22, 2025

http: HEAD request consumes response body #56697

Open

LiviaMedeiros reviewed Jan 22, 2025

View reviewed changes

test/parallel/test-http-head-response-has-no-body-end-implicit-headers.js Outdated Show resolved Hide resolved

test: fix assertion style issues

ece7877

Co-authored-by: Livia Medeiros <[email protected]>

ronag reviewed Jan 22, 2025

View reviewed changes

test/parallel/test-http-head-response-has-no-body-end-implicit-headers.js Outdated Show resolved Hide resolved

ronag approved these changes Jan 22, 2025

View reviewed changes

ronag reviewed Jan 22, 2025

View reviewed changes

ronag requested changes Jan 22, 2025

View reviewed changes

mcollina added the needs-citgm PRs that need a CITGM CI run. label Jan 22, 2025

Merge branch 'nodejs:main' into fix/head-content-length

5051a38

mcollina requested changes Jan 22, 2025

View reviewed changes

qwerzl added 2 commits January 23, 2025 00:31

test: test for raw HEAD responses

6458459

test: test for actual content-length instead of undefined

0f4470f

pimterry requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: return `Content-Length` header for `HEAD`s #56681

http: return `Content-Length` header for `HEAD`s #56681

qwerzl commented Jan 21, 2025

nodejs-github-bot commented Jan 21, 2025

geeksilva97 commented Jan 21, 2025

codecov bot commented Jan 21, 2025 •

edited

Loading

qwerzl commented Jan 22, 2025

LiviaMedeiros left a comment

ronag left a comment

ronag Jan 22, 2025

qwerzl Jan 22, 2025

ronag left a comment •

edited

Loading

mcollina commented Jan 22, 2025

mcollina left a comment

qwerzl commented Jan 22, 2025

qwerzl commented Jan 22, 2025

pimterry left a comment

pimterry Jan 22, 2025

pimterry Jan 22, 2025

http: return Content-Length header for HEADs #56681

Are you sure you want to change the base?

http: return Content-Length header for HEADs #56681

Conversation

qwerzl commented Jan 21, 2025

nodejs-github-bot commented Jan 21, 2025

geeksilva97 commented Jan 21, 2025

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

qwerzl commented Jan 22, 2025

LiviaMedeiros left a comment

Choose a reason for hiding this comment

ronag left a comment

Choose a reason for hiding this comment

ronag Jan 22, 2025

Choose a reason for hiding this comment

qwerzl Jan 22, 2025

Choose a reason for hiding this comment

ronag left a comment • edited Loading

Choose a reason for hiding this comment

mcollina commented Jan 22, 2025

mcollina left a comment

Choose a reason for hiding this comment

qwerzl commented Jan 22, 2025

qwerzl commented Jan 22, 2025

pimterry left a comment

Choose a reason for hiding this comment

pimterry Jan 22, 2025

Choose a reason for hiding this comment

pimterry Jan 22, 2025

Choose a reason for hiding this comment

http: return `Content-Length` header for `HEAD`s #56681

http: return `Content-Length` header for `HEAD`s #56681

codecov bot commented Jan 21, 2025 •

edited

Loading

ronag left a comment •

edited

Loading