Error if message is bigger than Content-Length #39133

betochf · 2021-06-24T05:14:58Z

Pr for issue #39041

The test for the changes is very simple, it just contains a 200 request with a header specifying the "Content-Lenght", in this case with a size of 2 bytes, with a response of 4 so the error could be triggered.

tests:

If we change the value of the Content-Lenght for the actual one or a bigger one, we have no error.
Finally, we do a request with no headers just to prove that the code still running, even with the new #changes.

ronag · 2021-06-24T06:41:17Z

lib/_http_outgoing.js

@@ -66,7 +66,8 @@ const {
    ERR_STREAM_ALREADY_FINISHED,
    ERR_STREAM_WRITE_AFTER_END,
    ERR_STREAM_NULL_VALUES,
-    ERR_STREAM_DESTROYED
+    ERR_STREAM_DESTROYED,
+    ERR_CONTENT_LENGTH_HEADER


Suggested change

ERR_CONTENT_LENGTH_HEADER

ERR_CONTENT_LENGTH_MISMATCH,

nit: Could this list also be sorted in alphabetical order?

Ok, thanks for the feedback, would make the changes

ronag

Would also be nice if you check content length in .end() to make sure not to few bytes have been written as well.

lib/_http_outgoing.js

test/sequential/test-http-OutgoingMessage-ContentLength.js

VoltrexKeyva · 2021-06-24T09:20:37Z

Unrelated documentation changes in doc/api/errors.md. (Line 9 to Line 805 | Line 821 to Line 3241)

betochf · 2021-06-24T22:23:39Z

Hey @ronag for .end(), how many bytes would be a good amount so the error is not triggered?
I was thinking that it should be at least half of the size of the Content-Length

ronag · 2021-06-25T07:17:47Z

Hey @ronag for .end(), how many bytes would be a good amount so the error is not triggered?
I was thinking that it should be at least half of the size of the Content-Length

If it doesn’t match content length then error

ronag · 2021-06-25T22:42:08Z

lib/_http_outgoing.js

@@ -514,6 +517,9 @@ function processHeader(self, state, key, value, validate) {
 function storeHeader(self, state, key, value, validate) {
  if (validate)
    validateHeaderValue(key, value);
+  if (key === 'Content-Length') {


Suggested change

if (key === 'Content-Length') {

if (key.toLowerCase() === 'content-length') {

https://nodejs.org/es/docs/guides/anatomy-of-an-http-transaction/

It's important to note here that all headers are represented in lower-case only, regardless of how the client actually sent them. This simplifies the task of parsing headers for whatever purpose.

ronag · 2021-06-25T22:42:55Z

lib/_http_outgoing.js

@@ -575,6 +581,10 @@ OutgoingMessage.prototype.setHeader = function setHeader(name, value) {
  validateHeaderName(name);
  validateHeaderValue(name, value);

+  if (name === 'content-length') {
+    this._contentLength = NumberParseInt(value);
+  }


Why? Isn’t store header enough?

Oh sorry, shouldn't be there, my mistake

I checked the code again and store header is not enough, the Content-length is not set when setHeader is used... I think it happens because it never goes throw processHeader

Putting inside the validation function is wrong though.

It seemed right sorry, I'll create a new function for the validating the content length

lib/_http_outgoing.js

ronag

A lot of unrelated doc changes...

lib/_http_outgoing.js

RaisinTen · 2021-07-04T05:55:16Z

@betochf Could you please rebase and resolve the merge conflicts?

betochf · 2021-07-05T16:05:10Z

@RaisinTen done, it was an error in the Doc

ronag · 2021-07-07T15:10:03Z

lib/_http_outgoing.js

  let headers = this[kOutHeaders];
  if (headers === null)
    this[kOutHeaders] = headers = ObjectCreate(null);
-


Unrelated change

+1. By the way, are changes in doc/api/errors.md also required? It seems like most of them are unrelated to an issue as well.

ronag · 2021-07-07T15:10:24Z

lib/_http_outgoing.js

@@ -756,6 +772,12 @@ function write_(msg, chunk, encoding, callback, fromEnd) {
    return false;
  }

+  if (msg._contentLength) {
+    if (msg[kBytesWritten] > msg._contentLength) {


You can combine these two if statements

ronag · 2021-07-07T15:11:06Z

lib/_http_outgoing.js

+    if (msg[kBytesWritten] > msg._contentLength) {
+      throw new ERR_CONTENT_LENGTH_MISMATCH(msg._contentLength, msg[kBytesWritten]);
+    }
+  }


Why is this check needed if you already have it below?

ronag

Needs more tests.

betochf · 2021-07-09T22:14:31Z

@ronag I did the changes... which other tests should I try?

Ayase-252 · 2021-08-12T09:50:08Z

I think 3 tests should be added at least:

when actual length is mismatched (more or less) with Content-Length, it should throw;
when Content-Length is negative, it should throw;

jasnell · 2021-08-12T15:28:12Z

lib/_http_outgoing.js

@@ -81,7 +84,7 @@ const HIGH_WATER_MARK = getDefaultHighWaterMark();
 const { CRLF } = common;

 const kCorked = Symbol('corked');
-
+const kBytesWritten = Symbol ('bytes');


Suggested change

const kBytesWritten = Symbol ('bytes');

const kBytesWritten = Symbol('bytes');

jasnell · 2021-08-12T15:30:46Z

lib/_http_outgoing.js

@@ -849,6 +871,11 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
      this.socket.cork();
    }

+    const byteCount = (this[kBytesWritten] + chunk.length);
+    if (this._contentLength && (byteCount) !== this._contentLength) {


Suggested change

if (this._contentLength && (byteCount) !== this._contentLength) {

if (this._contentLength !== undefined && (byteCount) !== this._contentLength) {

jasnell · 2021-08-12T15:31:56Z

lib/internal/errors.js

@@ -1191,6 +1199,10 @@ E('ERR_INVALID_CHAR',
    }
    return msg;
  }, TypeError);
+E('ERR_INVALID_CONTENT_LENGTH', function(size) {


ERR_INVALID_ARG_VALUE could be used instead.

jasnell · 2021-08-12T15:36:52Z

lib/_http_outgoing.js

@@ -849,6 +871,11 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
      this.socket.cork();
    }

+    const byteCount = (this[kBytesWritten] + chunk.length);


This may not be entirely accurate in every case due to content encoding. For instance, if I pass in w.end('afef', 'hex'), I'm only writing 2 bytes. The calculation here needs to take into consideration the encoding and the actual byte length.

Mifrill · 2023-10-31T22:14:23Z

lib/internal/errors.js

+  let msg;
+  if (byteCount < contentLength) {
+    msg = 'less';
+  } else msg = 'more';


Suggested change

let msg;

if (byteCount < contentLength) {

msg = 'less';

} else msg = 'more';

const msg = byteCount < contentLength ? 'less' : 'more';

github-actions bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jun 24, 2021

ronag reviewed Jun 24, 2021

View reviewed changes

ronag requested changes Jun 24, 2021

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

lib/_http_outgoing.js Outdated Show resolved Hide resolved

test/sequential/test-http-OutgoingMessage-ContentLength.js Show resolved Hide resolved

betochf added 8 commits June 25, 2021 16:53

Content-Length error created

569c463

Content-Length error doc added

5e1db58

Content-Length changes

cec834c

test for Content-Length

278b097

lib: OutgoingMessage changes

94e742b

lib/internal: content_lenght_mismatch error improved

a5c3e00

test/sequential: Test improved

9acc0d6

lib: .end() error fixed

16944cf

betochf force-pushed the issue/39041 branch from 9766cb3 to 16944cf Compare June 25, 2021 22:02

ronag reviewed Jun 25, 2021

View reviewed changes