fix: define conditions when content-length should be sent #2305

pxue · 2023-10-03T19:14:19Z

Fixes: #2046

…tLength=0

codecov-commenter · 2023-10-03T19:26:37Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (741b811) 85.84%.
Report is 58 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2305      +/-   ##
==========================================
+ Coverage   85.54%   85.84%   +0.29%     
==========================================
  Files          76       76              
  Lines        6858     6872      +14     
==========================================
+ Hits         5867     5899      +32     
+ Misses        991      973      -18

Files	Coverage Δ
index-fetch.js	`100.00% <100.00%> (ø)`
lib/core/connect.js	`80.00% <100.00%> (-1.25%)`	⬇️
lib/fetch/body.js	`97.67% <100.00%> (-0.02%)`	⬇️
lib/fetch/global.js	`100.00% <ø> (ø)`
lib/pool.js	`100.00% <100.00%> (ø)`
lib/websocket/connection.js	`89.15% <100.00%> (+0.13%)`	⬆️
lib/websocket/frame.js	`100.00% <100.00%> (ø)`
lib/client.js	`93.19% <80.00%> (+0.07%)`	⬆️
lib/core/util.js	`94.73% <85.71%> (-1.06%)`	⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/client.js

…-length is added to the header

metcoder95

Tests seem failing.
Can we also port this to writeH2?

lib/client.js

mcollina

lgtm

… body

metcoder95

Thanks!

acavalli-lunalabs · 2023-10-10T10:05:37Z

lib/client.js

+  if (contentLength > 0) {
+    if (method === 'POST' || method === 'PUT' || method === 'PATCH') {
+      return true
+    }
+    return false
+  }
+  return (method === 'POST' || method === 'PUT' || method === 'PATCH')


what?
If this code is correct, its logic must be simplified to just one line:

function shouldSendContentLength(method) { return (method === 'POST' || method === 'PUT' || method === 'PATCH'); }

You're right!

Is this what we want? We're basically saying all non Post/put/patch methods have no defined semantics for a body.

Therefore we should not send content-length

What if someone still puts a content body in the request? Does it make sense to still put the content-length in that case?

Still, the code must be changed to this:

function shouldSendContentLength(contentLength, method) { return contentLength > 0 || method === 'POST' || method === 'PUT' || method === 'PATCH'; }

or this:

function shouldSendContentLength(method) { return method === 'POST' || method === 'PUT' || method === 'PATCH'; }

depending on the logic that you want to obtain.

metcoder95 · 2023-10-10T19:44:26Z

lib/client.js

-  if (request.contentLength !== null && request.contentLength !== contentLength) {
+  // https://github.com/nodejs/undici/issues/2046
+  // A user agent may send a Content-Length header with 0 value, this should be allowed.
+  if (shouldSendContentLength(contentLength, method) && request.contentLength !== null && request.contentLength !== contentLength) {


Following previous comment, it is better if we offload the contentLength to the if statement instead. In that sense if contentLength is greater than 0, and the method has explicit semantics to handle the request's body; we attach it. Otherwise, we just ignore it.

metcoder95 · 2023-10-10T19:46:09Z

lib/client.js

@@ -1495,6 +1495,16 @@ function _resume (client, sync) {
  }
 }

+function shouldSendContentLength (contentLength, method) {
+  if (contentLength > 0) {
+    if (method === 'POST' || method === 'PUT' || method === 'PATCH') {


Thinking twice is better if we exclude those who don't explicitly support the request's body semantics, so methods like DELETE, whose bodies are up to the server to decide what to do with them, can still have a content-length header.

Made the changes, glad we're getting closer.

test/content-length.js

pxue · 2023-10-20T22:25:05Z

@metcoder95
anything else needed to get this PR across the finish line? I'll have some time this weekend.

metcoder95

Small nit but LGTM 👍

lib/client.js

fix: bypass DELETE method content-length mismatch when request.conten…

670f31f

…tLength=0

metcoder95 reviewed Oct 3, 2023

View reviewed changes

lib/client.js Outdated Show resolved Hide resolved

add shouldSendContentLength to better define conditions where content…

2461dbc

…-length is added to the header

pxue changed the title ~~fix: bypass DELETE method content-length mismatch when 0~~ fix: define conditions when content-length should be sent Oct 6, 2023

metcoder95 reviewed Oct 6, 2023

View reviewed changes

lib/client.js Outdated Show resolved Hide resolved

mcollina approved these changes Oct 6, 2023

View reviewed changes

refactor to ignore content-length on methods that do not anticipate a…

75aefbd

… body

KhafraDev approved these changes Oct 9, 2023

View reviewed changes

metcoder95 approved these changes Oct 9, 2023

View reviewed changes

acavalli-lunalabs suggested changes Oct 10, 2023

View reviewed changes

metcoder95 reviewed Oct 10, 2023

View reviewed changes

fix up content-length to handle all cases with no defined semantics

c8bf1f6

metcoder95 reviewed Oct 13, 2023

View reviewed changes

test/content-length.js Show resolved Hide resolved

test/content-length.js Show resolved Hide resolved

pxue added 3 commits October 13, 2023 19:34

fix up tests

4587f37

remove prettierignore

194a60c

test content-length in request is undefined or 0

741b811

pxue force-pushed the main branch from e428cf8 to 741b811 Compare October 18, 2023 16:09

pxue mentioned this pull request Oct 18, 2023

Breaking change: deprecate body on http methods with no defined body semantics #2357

Closed

metcoder95 approved these changes Oct 22, 2023

View reviewed changes

lib/client.js Show resolved Hide resolved

metcoder95 merged commit fc29aa0 into nodejs:main Oct 26, 2023
31 of 35 checks passed

brc-dd mentioned this pull request Oct 31, 2023

No content length on DELETE and OPTIONS nodejs/node#27880

Open

viceice mentioned this pull request Dec 5, 2023

[HttpError]: Request body length does not match content-length header semantic-release/github#746

Closed

pi0 mentioned this pull request Jan 4, 2024

fix(proxy): workaround undici issue with DELETE and content-length of 0 unjs/h3#605

Closed

8 tasks

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

fix: define conditions when content-length should be sent (nodejs#2305)

540a9f8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: define conditions when content-length should be sent #2305

fix: define conditions when content-length should be sent #2305

pxue commented Oct 3, 2023

codecov-commenter commented Oct 3, 2023 •

edited

Loading

metcoder95 left a comment

mcollina left a comment

metcoder95 left a comment

acavalli-lunalabs Oct 10, 2023

pxue Oct 10, 2023

acavalli-lunalabs Oct 10, 2023

metcoder95 Oct 10, 2023

metcoder95 Oct 10, 2023

pxue Oct 13, 2023

pxue commented Oct 20, 2023

metcoder95 left a comment

fix: define conditions when content-length should be sent #2305

fix: define conditions when content-length should be sent #2305

Conversation

pxue commented Oct 3, 2023

codecov-commenter commented Oct 3, 2023 • edited Loading

Codecov Report

metcoder95 left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

metcoder95 left a comment

Choose a reason for hiding this comment

acavalli-lunalabs Oct 10, 2023

Choose a reason for hiding this comment

pxue Oct 10, 2023

Choose a reason for hiding this comment

acavalli-lunalabs Oct 10, 2023

Choose a reason for hiding this comment

metcoder95 Oct 10, 2023

Choose a reason for hiding this comment

metcoder95 Oct 10, 2023

Choose a reason for hiding this comment

pxue Oct 13, 2023

Choose a reason for hiding this comment

pxue commented Oct 20, 2023

metcoder95 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 3, 2023 •

edited

Loading