http: correctly calculate strict content length

Fixes some logical errors related to strict content length. Also, previously Buffer.byteLength (which is slow) was run regardless of whether or not the len was required. PR-URL: #46601 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
nodejs · Apr 5, 2023 · 3538521 · 3538521
1 parent 9582c8e
commit 3538521
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 45 deletions.
diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
@@ -25,7 +25,6 @@ const {
   Array,
   ArrayIsArray,
   ArrayPrototypeJoin,
-  MathAbs,
   MathFloor,
   NumberPrototypeToString,
   ObjectCreate,
@@ -87,7 +86,6 @@ const HIGH_WATER_MARK = getDefaultHighWaterMark();
 const kCorked = Symbol('corked');
 const kUniqueHeaders = Symbol('kUniqueHeaders');
 const kBytesWritten = Symbol('kBytesWritten');
-const kEndCalled = Symbol('kEndCalled');
 const kErrored = Symbol('errored');
 
 const nop = () => {};
@@ -134,7 +132,6 @@ function OutgoingMessage() {
 
   this.strictContentLength = false;
   this[kBytesWritten] = 0;
-  this[kEndCalled] = false;
   this._contentLength = null;
   this._hasBody = true;
   this._trailer = '';
@@ -356,7 +353,7 @@ OutgoingMessage.prototype.destroy = function destroy(error) {
 
 
 // This abstract either writing directly to the socket or buffering it.
-OutgoingMessage.prototype._send = function _send(data, encoding, callback) {
+OutgoingMessage.prototype._send = function _send(data, encoding, callback, byteLength) {
   // This is a shameful hack to get the headers and first body chunk onto
   // the same packet. Future versions of Node are going to take care of
   // this at a lower level and in a more general way.
@@ -378,20 +375,11 @@ OutgoingMessage.prototype._send = function _send(data, encoding, callback) {
     }
     this._headerSent = true;
   }
-  return this._writeRaw(data, encoding, callback);
+  return this._writeRaw(data, encoding, callback, byteLength);
 };
 
-function _getMessageBodySize(chunk, headers, encoding) {
-  if (Buffer.isBuffer(chunk)) return chunk.length;
-  const chunkLength = chunk ? Buffer.byteLength(chunk, encoding) : 0;
-  const headerLength = headers ? headers.length : 0;
-  if (headerLength === chunkLength) return 0;
-  if (headerLength < chunkLength) return MathAbs(chunkLength - headerLength);
-  return chunkLength;
-}
-
 OutgoingMessage.prototype._writeRaw = _writeRaw;
-function _writeRaw(data, encoding, callback) {
+function _writeRaw(data, encoding, callback, size) {
   const conn = this.socket;
   if (conn && conn.destroyed) {
     // The socket was destroyed. If we're still trying to write to it,
@@ -404,25 +392,6 @@ function _writeRaw(data, encoding, callback) {
     encoding = null;
   }
 
-  // TODO(sidwebworks): flip the `strictContentLength` default to `true` in a future PR
-  if (this.strictContentLength && conn && conn.writable && !this._removedContLen && this._hasBody) {
-    const skip = conn._httpMessage.statusCode === 304 || (this.hasHeader('transfer-encoding') || this.chunkedEncoding);
-
-    if (typeof this._contentLength === 'number' && !skip) {
-      const size = _getMessageBodySize(data, conn._httpMessage._header, encoding);
-
-      if ((size + this[kBytesWritten]) > this._contentLength) {
-        throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(size + this[kBytesWritten], this._contentLength);
-      }
-
-      if (this[kEndCalled] && (size + this[kBytesWritten]) !== this._contentLength) {
-        throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(size + this[kBytesWritten], this._contentLength);
-      }
-
-      this[kBytesWritten] += size;
-    }
-  }
-
   if (conn && conn._httpMessage === this && conn.writable) {
     // There might be pending data in the this.output buffer.
     if (this.outputData.length) {
@@ -882,18 +851,24 @@ function emitErrorNt(msg, err, callback) {
   }
 }
 
+function strictContentLength(msg) {
+  return (
+    msg.strictContentLength &&
+    msg._contentLength != null &&
+    msg._hasBody &&
+    !msg._removedContLen &&
+    !msg.chunkedEncoding &&
+    !msg.hasHeader('transfer-encoding')
+  );
+}
+
 function write_(msg, chunk, encoding, callback, fromEnd) {
   if (typeof callback !== 'function')
     callback = nop;
 
-  let len;
   if (chunk === null) {
     throw new ERR_STREAM_NULL_VALUES();
-  } else if (typeof chunk === 'string') {
-    len = Buffer.byteLength(chunk, encoding);
-  } else if (isUint8Array(chunk)) {
-    len = chunk.length;
-  } else {
+  } else if (typeof chunk !== 'string' && !isUint8Array(chunk)) {
     throw new ERR_INVALID_ARG_TYPE(
       'chunk', ['string', 'Buffer', 'Uint8Array'], chunk);
   }
@@ -914,8 +889,24 @@ function write_(msg, chunk, encoding, callback, fromEnd) {
     return false;
   }
 
+  let len;
+
+  if (msg.strictContentLength) {
+    len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength;
+
+    if (
+      strictContentLength(msg) &&
+      (fromEnd ? msg[kBytesWritten] + len !== msg._contentLength : msg[kBytesWritten] + len > msg._contentLength)
+    ) {
+      throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(len + msg[kBytesWritten], msg._contentLength);
+    }
+
+    msg[kBytesWritten] += len;
+  }
+
   if (!msg._header) {
     if (fromEnd) {
+      len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength;
       msg._contentLength = len;
     }
     msg._implicitHeader();
@@ -935,12 +926,13 @@ function write_(msg, chunk, encoding, callback, fromEnd) {
 
   let ret;
   if (msg.chunkedEncoding && chunk.length !== 0) {
+    len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength;
     msg._send(NumberPrototypeToString(len, 16), 'latin1', null);
     msg._send(crlf_buf, null, null);
-    msg._send(chunk, encoding, null);
+    msg._send(chunk, encoding, null, len);
     ret = msg._send(crlf_buf, null, callback);
   } else {
-    ret = msg._send(chunk, encoding, callback);
+    ret = msg._send(chunk, encoding, callback, len);
   }
 
   debug('write ret = ' + ret);
@@ -1012,8 +1004,6 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
     encoding = null;
   }
 
-  this[kEndCalled] = true;
-
   if (chunk) {
     if (this.finished) {
       onError(this,
@@ -1048,6 +1038,10 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
   if (typeof callback === 'function')
     this.once('finish', callback);
 
+  if (strictContentLength(this) && this[kBytesWritten] !== this._contentLength) {
+    throw new ERR_HTTP_CONTENT_LENGTH_MISMATCH(this[kBytesWritten], this._contentLength);
+  }
+
   const finish = onFinish.bind(undefined, this);
 
   if (this._hasBody && this.chunkedEncoding) {

diff --git a/test/parallel/test-http-content-length-mismatch.js b/test/parallel/test-http-content-length-mismatch.js
@@ -57,7 +57,7 @@ function shouldThrowOnFewerBytes() {
     res.write('a');
     res.statusCode = 200;
     assert.throws(() => {
-      res.end();
+      res.end('aaa');
     }, {
       code: 'ERR_HTTP_CONTENT_LENGTH_MISMATCH'
     });