From 3538521bf61ac4928a92a8f6aad3d038b14b0e8b Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 23 Feb 2023 10:54:45 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/46601 Reviewed-By: Matteo Collina Reviewed-By: Paolo Insogna Reviewed-By: Yagiz Nizipli --- lib/_http_outgoing.js | 82 +++++++++---------- .../test-http-content-length-mismatch.js | 2 +- 2 files changed, 39 insertions(+), 45 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index eae242995f6bf6..75de2321aa32b6 100644 --- 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 index fc294db16407bc..540acbe759d84a 100644 --- 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' });