fix: return content length mismatch error #742

simone-sanfratello · 2021-04-15T07:03:44Z

@ronag @mcollina this fix the issue but I'm not very happy with the solution, looks a little bit "patchy" to me, please have a look, then I'll go on on the "@todo"

I also expect to send the same error if the body received is smaller then the "content-length", now it send "UND_ERR_INFO"

Also, I think we should/could refactor client.js code to improve readability, for example moving the Parser class to its own file and rename properties and methods where it is not clear they are referring to request or response phase - in another PR of course.

mcollina · 2021-04-15T07:12:53Z

Why are you not happy with the solution?

ronag · 2021-04-15T07:15:59Z

lib/client.js

+    this[kClient][kBodySize] !== this[kClient][kResponseContentLength]) {
+    util.destroy(this, new ResponseContentLengthMismatchError('end'))
+    return
+  }


This check should be in onBody and onMessageComplete

Should probably also check for the strictcontentlength flag

That flag refers to the sending headers, I think we should rename it, because in the current situation there are 2 cases and both return an error:

the server content-length is greater than the actual body => connection is closed by the server, client returns "UND_ERR_SOCKET" >> fix sending "UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH"

the server content-length is less than the actual body => connection is closed by the client, client returns "UND_ERR_INFO" >> ? fix sending "UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH"

We can add another flag to suppress these errors, because I think those flags have different meanings: as client we can control what we send, so if we see an error/warning on sending headers we can fix it, but we can't control what the server is sending.

The only place the check works is in onSocketEnd, doing in onMessage and/or onBody don't work, and also break other tests.

I think we can't use the strictContentLength option, or another one, because the connection always ends with an error, so we can only send the right one instead of the generic

Doing it in onSocketEnd is unfortunately just plain wrong. It should basically only be call if the server unexpectedly closes the connection.

With keep-alive the server can give a body that doesn't match content-length without closing the connection.

I've seen body can't be larger than content-length because of https://github.com/nodejs/undici/blob/e2a9ddc07d9e0ff6be25d243523b53d1d71a3e9e/deps/llhttp/include/llhttp.h - unless we want to add this option

I think you were intending to link to a specific line?

yes, sorry

undici/deps/llhttp/include/llhttp.h

Line 472 in e2a9ddc

/* Enables/disables lenient handling of `Connection: close` and HTTP/1.0

With keep-alive the server can give a body that doesn't match content-length without closing the connection.

ok, I'll work on that

@ronag to me it's working fine also with keep-alive option

https://github.com/nodejs/undici/pull/742/files#diff-3058feaff6f3b2902370ee08a7528340aa7a98e5b76063f0176cb61972fadbb4R347

simone-sanfratello · 2021-04-15T07:27:29Z

Why are you not happy with the solution?

because I added this[kResponseContentLength] = 0 to client class, is there a better way to do that?

mcollina · 2021-04-15T07:32:07Z

Why are you not happy with the solution?

because I added this[kResponseContentLength] = 0 to client class, is there a better way to do that?

I don't think there is any different way to do that.

lib/client.js

simone-sanfratello · 2021-04-15T07:35:03Z

thanks @mcollina @ronag for the instant review 😃

I'll go on

lib/client.js

dnlup · 2021-04-15T10:17:29Z

+1 to move the parser into its own file

ronag · 2021-04-15T18:23:46Z

@simone-sanfratello What's your timeline on this? Should we aim to land it before v4?

simone-sanfratello · 2021-04-16T04:19:35Z

I'll probably close it in the weekend

codecov-commenter · 2021-04-18T07:31:18Z

Codecov Report

Merging #742 (ffc97e8) into main (9f6bf86) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #742   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1904      1918   +14     
=========================================
+ Hits          1904      1918   +14

Impacted Files	Coverage Δ
lib/client.js	`100.00% <100.00%> (ø)`
lib/core/errors.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f6bf86...ffc97e8. Read the comment docs.

simone-sanfratello · 2021-04-18T07:40:05Z

docs/api/Errors.md

+
+\* The `UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH` is returned when the received body is less than the value in the received header `Content-Length`.  
+If the server try to send a body larger than `Content-Length`, Undici closes the connection when the length is received and return an `UND_ERR_INFO`, to prevent a cache poisoning attack. 


Please note that at https://github.com/nodejs/undici/blob/e2a9ddc07d9e0ff6be25d243523b53d1d71a3e9e/deps/llhttp/include/llhttp.h

If we want to allow body larger than content-length, we probably should expose this option

lib/core/errors.js

docs/api/Errors.md

types/errors.d.ts

ronag · 2021-04-19T07:35:32Z

lib/core/util.js

+    keepAlive: null,
+    trailers: null,
+    contentLength: 0
+  }


I'm a little worried this extra object has perf implications?

I don't see much difference between instancing 3 vars instead of 1 object, btw the destructuration of the result is not necessary, so I removed

I'm ok to move back without this function

It would be better if we could avoid it.

ronag · 2021-04-19T07:37:23Z

test/util.js

+        Buffer.from('timeout=5'),
+        Buffer.from('Content-Length'),
+        Buffer.from('2')
+      ],


Maybe easier with a map(x => Buffer.from(x))?

ronag

I don't think keep-alive test is working here as intended. Unless lhttp errors you need to check bodySize in onBody or onMessageComplete.

ronag · 2021-04-24T15:53:12Z

lib/client.js

@@ -450,6 +451,8 @@ class Parser {
    this.headersSize = 0
    this.headersMaxSize = client[kMaxHeadersSize]
    this.shouldKeepAlive = false
+    this.bodySize = 0
+    this.contentLength = 0


Suggested change

this.contentLength = 0

this.contentLength = null

ronag · 2021-04-24T15:53:22Z

lib/client.js

      } else if (!trailers && key.length === 7 && key.toString().toLowerCase() === 'trailer') {
        trailers = val
-        looking = !keepAlive
+        looking++
+      } else if (!this.contentLength && key.length === 14 && key.toString().toLowerCase() === 'content-length') {


Suggested change

} else if (!this.contentLength && key.length === 14 && key.toString().toLowerCase() === 'content-length') {

} else if (this.contentLength == null && key.length === 14 && key.toString().toLowerCase() === 'content-length') {

ronag · 2021-04-24T16:27:59Z

test/content-length.js

+  t.teardown(server.close.bind(server))
+  server.listen(0, () => {
+    const client = new Client(`http://localhost:${server.address().port}`, {
+      pipelining: 0


you need to use pipelining > 0 or keep-alive won't work. This doesn't actually test keep-alive as it is now.

ronag · 2021-04-24T16:28:28Z

test/content-length.js

+  const server = createServer((req, res) => {
+    res.writeHead(200, {
+      'content-length': 10,
+      'keep-alive': 'timeout=2s',


Node already does this so you don't need to set it.

simone-sanfratello · 2021-04-25T09:33:21Z

I don't think keep-alive test is working here as intended. Unless lhttp errors you need to check bodySize in onBody or onMessageComplete.

Already tried, is not working as expected. I'm closing the PR

Make request related errors explicit in name in case we want to add errors on response side later. Refs: #742

Make request related errors explicit in name in case we want to add errors on response side later. Refs: nodejs#742

simone-sanfratello marked this pull request as draft April 15, 2021 07:03

ronag reviewed Apr 15, 2021

View reviewed changes

mcollina requested changes Apr 15, 2021

View reviewed changes

lib/client.js Outdated Show resolved Hide resolved

ronag reviewed Apr 15, 2021

View reviewed changes

lib/client.js Outdated Show resolved Hide resolved

simone-sanfratello force-pushed the bug/content-length-429 branch from ef06f53 to 4559b28 Compare April 18, 2021 07:29

simone-sanfratello marked this pull request as ready for review April 18, 2021 07:35

simone-sanfratello requested review from ronag and mcollina April 18, 2021 07:35

simone-sanfratello commented Apr 18, 2021

View reviewed changes

ronag reviewed Apr 18, 2021

View reviewed changes

lib/core/errors.js Outdated Show resolved Hide resolved

ronag reviewed Apr 19, 2021

View reviewed changes

docs/api/Errors.md Outdated Show resolved Hide resolved

ronag reviewed Apr 19, 2021

View reviewed changes

types/errors.d.ts Show resolved Hide resolved

simone-sanfratello force-pushed the bug/content-length-429 branch from 2121e93 to 9d17b67 Compare April 19, 2021 07:33

ronag reviewed Apr 19, 2021

View reviewed changes

simone-sanfratello force-pushed the bug/content-length-429 branch 2 times, most recently from 6e92b64 to 218a28e Compare April 24, 2021 06:16

fix: return content length mismatch error

ffc97e8

simone-sanfratello force-pushed the bug/content-length-429 branch from 218a28e to ffc97e8 Compare April 24, 2021 08:25

simone-sanfratello requested a review from ronag April 24, 2021 08:26

ronag requested changes Apr 24, 2021

View reviewed changes

ronag reviewed Apr 24, 2021

View reviewed changes

ronag added this to the 4.0 milestone Apr 24, 2021

simone-sanfratello closed this Apr 25, 2021

ronag added a commit that referenced this pull request Apr 26, 2021

refactor: make request errors explicit

bb76d01

Make request related errors explicit in name in case we want to add errors on response side later. Refs: #742

ronag added a commit that referenced this pull request Apr 26, 2021

refactor: make request errors explicit

1c55561

Make request related errors explicit in name in case we want to add errors on response side later. Refs: #742

ronag added a commit that referenced this pull request Apr 26, 2021

refactor: make request errors explicit (#777)

ca309d8

Make request related errors explicit in name in case we want to add errors on response side later. Refs: #742

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

refactor: make request errors explicit (nodejs#777)

17ae174

Make request related errors explicit in name in case we want to add errors on response side later. Refs: nodejs#742

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: return content length mismatch error #742

fix: return content length mismatch error #742

simone-sanfratello commented Apr 15, 2021 •

edited

Loading

mcollina commented Apr 15, 2021

ronag Apr 15, 2021

ronag Apr 15, 2021

simone-sanfratello Apr 16, 2021 •

edited

Loading

simone-sanfratello Apr 18, 2021

ronag Apr 18, 2021

ronag Apr 19, 2021 •

edited

Loading

ronag Apr 19, 2021

simone-sanfratello Apr 19, 2021

simone-sanfratello Apr 19, 2021

simone-sanfratello Apr 24, 2021

simone-sanfratello commented Apr 15, 2021 •

edited

Loading

mcollina commented Apr 15, 2021

simone-sanfratello commented Apr 15, 2021

dnlup commented Apr 15, 2021

ronag commented Apr 15, 2021

simone-sanfratello commented Apr 16, 2021

codecov-commenter commented Apr 18, 2021 •

edited

Loading

simone-sanfratello Apr 18, 2021

ronag Apr 19, 2021 •

edited

Loading

simone-sanfratello Apr 24, 2021 •

edited

Loading

simone-sanfratello Apr 24, 2021

mcollina Apr 24, 2021

simone-sanfratello Apr 24, 2021

ronag Apr 19, 2021

simone-sanfratello Apr 24, 2021

ronag left a comment •

edited

Loading

ronag Apr 24, 2021

ronag Apr 24, 2021

ronag Apr 24, 2021 •

edited

Loading

ronag Apr 24, 2021

simone-sanfratello commented Apr 25, 2021


		\* The `UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH` is returned when the received body is less than the value in the received header `Content-Length`.
		If the server try to send a body larger than `Content-Length`, Undici closes the connection when the length is received and return an `UND_ERR_INFO`, to prevent a cache poisoning attack.

	} else if (!this.contentLength && key.length === 14 && key.toString().toLowerCase() === 'content-length') {
	} else if (this.contentLength == null && key.length === 14 && key.toString().toLowerCase() === 'content-length') {

fix: return content length mismatch error #742

fix: return content length mismatch error #742

Conversation

simone-sanfratello commented Apr 15, 2021 • edited Loading

mcollina commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-sanfratello Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-sanfratello commented Apr 15, 2021 • edited Loading

mcollina commented Apr 15, 2021

simone-sanfratello commented Apr 15, 2021

dnlup commented Apr 15, 2021

ronag commented Apr 15, 2021

simone-sanfratello commented Apr 16, 2021

codecov-commenter commented Apr 18, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ronag Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

simone-sanfratello Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-sanfratello commented Apr 25, 2021

simone-sanfratello commented Apr 15, 2021 •

edited

Loading

simone-sanfratello Apr 16, 2021 •

edited

Loading

ronag Apr 19, 2021 •

edited

Loading

simone-sanfratello commented Apr 15, 2021 •

edited

Loading

codecov-commenter commented Apr 18, 2021 •

edited

Loading

ronag Apr 19, 2021 •

edited

Loading

simone-sanfratello Apr 24, 2021 •

edited

Loading

ronag left a comment •

edited

Loading

ronag Apr 24, 2021 •

edited

Loading