fix(node/http): correctly send Content-length header instead of Transfer-Encoding: chunked (#20127)

Fix #20063.

cli/tests/node_compat/config.jsonc

@@ -72,6 +72,7 @@
       "test-fs-rmdir-recursive.js",
       "test-fs-write-file.js",
       "test-fs-write.js",
+      "test-http-content-length.js",
       "test-net-better-error-messages-path.js",
       "test-net-connect-buffer.js",
       "test-net-connect-buffer2.js",
@@ -363,6 +364,7 @@
       // TODO(lev): ClientRequest.socket is not polyfilled so this test keeps
       //            failing
       //"test-http-client-set-timeout.js",
+      "test-http-content-length.js",
       "test-http-localaddress.js",
       // TODO(bartlomieju): temporarily disabled while we iterate on the HTTP client
       // "test-http-outgoing-buffer.js",

cli/tests/node_compat/test/parallel/test-http-content-length.js

@@ -0,0 +1,111 @@
+// deno-fmt-ignore-file
+// deno-lint-ignore-file
+
+// Copyright Joyent and Node contributors. All rights reserved. MIT license.
+// Taken from Node 18.12.1
+// This file is automatically generated by `tools/node_compat/setup.ts`. Do not modify this file manually.
+
+'use strict';
+require('../common');
+const assert = require('assert');
+const http = require('http');
+const Countdown = require('../common/countdown');
+
+const expectedHeadersMultipleWrites = {
+  'connection': 'close',
+  'transfer-encoding': 'chunked',
+};
+
+const expectedHeadersEndWithData = {
+  'connection': 'close',
+  'content-length': String('hello world'.length)
+};
+
+const expectedHeadersEndNoData = {
+  'connection': 'close',
+  'content-length': '0',
+};
+
+let error;
+const countdown = new Countdown(3, () => server.close());
+
+const server = http.createServer(function(req, res) {
+  res.removeHeader('Date');
+
+  try {
+    switch (req.url.substr(1)) {
+      case 'multiple-writes':
+        // assert.deepStrictEqual(req.headers, expectedHeadersMultipleWrites);
+        assert.equal(req.headers['transfer-encoding'], expectedHeadersMultipleWrites['transfer-encoding']);
+        assert.equal(req.headers['content-length'], expectedHeadersMultipleWrites['content-length']);
+        res.write('hello');
+        res.end('world');
+        break;
+      case 'end-with-data':
+        // assert.deepStrictEqual(req.headers, expectedHeadersEndWithData);
+        assert.equal(req.headers['transfer-encoding'], expectedHeadersEndWithData['transfer-encoding']);
+        assert.equal(req.headers['content-length'], expectedHeadersEndWithData['content-length']);
+        res.end('hello world');
+        break;
+      case 'empty':
+        // assert.deepStrictEqual(req.headers, expectedHeadersEndNoData);
+        assert.equal(req.headers['transfer-encoding'], expectedHeadersEndNoData['transfer-encoding']);
+        assert.equal(req.headers['content-length'], expectedHeadersEndNoData['content-length']);
+        res.end();
+        break;
+      default:
+        throw new Error('Unreachable');
+    }
+    countdown.dec();
+  }
+  catch (e) {
+    error = e;
+    server.close();
+  }
+});
+
+server.on('close', () => {
+  if (error) throw error
+})
+
+server.listen(0, function() {
+  let req;
+
+  req = http.request({
+    port: this.address().port,
+    method: 'POST',
+    path: '/multiple-writes'
+  });
+  req.removeHeader('Date');
+  req.removeHeader('Host');
+  req.write('hello ');
+  req.end('world');
+  req.on('response', function(res) {
+    // assert.deepStrictEqual(res.headers, expectedHeadersMultipleWrites);
+  });
+
+  req = http.request({
+    port: this.address().port,
+    method: 'POST',
+    path: '/end-with-data'
+  });
+  req.removeHeader('Date');
+  req.removeHeader('Host');
+  req.end('hello world');
+  req.on('response', function(res) {
+    // assert.deepStrictEqual(res.headers, expectedHeadersEndWithData);
+  });
+
+  req = http.request({
+    port: this.address().port,
+    method: 'POST',
+    path: '/empty'
+  });
+  req.removeHeader('Date');
+  req.removeHeader('Host');
+  req.end();
+  req.on('response', function(res) {
+    // assert.deepStrictEqual(res.headers, expectedHeadersEndNoData);
+  });
+
+});

ext/node/polyfills/_http_outgoing.ts

@@ -14,14 +14,14 @@ import type { Socket } from "node:net";
 import {
   kNeedDrain,
   kOutHeaders,
-  // utcDate,
+  utcDate,
 } from "ext:deno_node/internal/http.ts";
 import { notImplemented } from "ext:deno_node/_utils.ts";
 import { Buffer } from "node:buffer";
 import {
   _checkInvalidHeaderChar as checkInvalidHeaderChar,
   _checkIsHttpToken as checkIsHttpToken,
-  // chunkExpression as RE_TE_CHUNKED,
+  chunkExpression as RE_TE_CHUNKED,
 } from "ext:deno_node/_http_common.ts";
 import {
   defaultTriggerAsyncIdScope,
@@ -32,8 +32,8 @@ const { async_id_symbol } = symbols;
 import {
   ERR_HTTP_HEADERS_SENT,
   ERR_HTTP_INVALID_HEADER_VALUE,
-  // ERR_HTTP_TRAILER_INVALID,
-  // ERR_INVALID_ARG_TYPE,
+  ERR_HTTP_TRAILER_INVALID,
+  ERR_INVALID_ARG_TYPE,
   // ERR_INVALID_ARG_VALUE,
   ERR_INVALID_CHAR,
   ERR_INVALID_HTTP_TOKEN,
@@ -41,12 +41,12 @@ import {
   // ERR_STREAM_ALREADY_FINISHED,
   ERR_STREAM_CANNOT_PIPE,
   // ERR_STREAM_DESTROYED,
-  // ERR_STREAM_NULL_VALUES,
+  ERR_STREAM_NULL_VALUES,
   // ERR_STREAM_WRITE_AFTER_END,
   hideStackFrames,
 } from "ext:deno_node/internal/errors.ts";
 import { validateString } from "ext:deno_node/internal/validators.mjs";
-// import { isUint8Array } from "ext:deno_node/internal/util/types.ts";
+import { isUint8Array } from "ext:deno_node/internal/util/types.ts";
 // import { kStreamBaseField } from "ext:deno_node/internal_binding/stream_wrap.ts";
 
 import { debuglog } from "ext:deno_node/internal/util/debuglog.ts";
@@ -60,6 +60,8 @@ const kCorked = Symbol("corked");
 
 const nop = () => {};
 
+const RE_CONN_CLOSE = /(?:^|\W)close(?:$|\W)/i;
+
 export class OutgoingMessage extends Stream {
   // deno-lint-ignore no-explicit-any
   outputData: any[];
@@ -90,8 +92,7 @@ export class OutgoingMessage extends Stream {
 
   // TODO(crowlKats): use it
   socket: null;
-  // TODO(crowlKats): use it
-  _header: null;
+  _header: string | null;
   [kOutHeaders]: null | Record<string, [string, string]>;
 
   _keepAliveTimeout: number;
@@ -377,27 +378,46 @@ export class OutgoingMessage extends Stream {
     encoding: string | null,
     callback: () => void,
   ): boolean {
-    if (
-      (typeof chunk === "string" && chunk.length > 0) ||
-      ((chunk instanceof Buffer || chunk instanceof Uint8Array) &&
-        chunk.buffer.byteLength > 0)
-    ) {
-      if (typeof chunk === "string") {
-        chunk = Buffer.from(chunk, encoding);
+    if (typeof encoding === "function") {
+      callback = encoding;
+      encoding = null;
     }
-      if (chunk instanceof Buffer) {
-        chunk = new Uint8Array(chunk.buffer);
+    return this.write_(chunk, encoding, callback, false);
   }
 
-      core.writeAll(this._bodyWriteRid, chunk).then(() => {
-        callback?.();
-        this.emit("drain");
-      }).catch((e) => {
-        this._requestSendError = e;
-      });
+  write_(
+    chunk: string | Uint8Array | Buffer,
+    encoding: string | null,
+    callback: () => void,
+    fromEnd: boolean,
+  ): boolean {
+    // Ignore lint to keep the code as similar to Nodejs as possible
+    // deno-lint-ignore no-this-alias
+    const msg = this;
+
+    if (chunk === null) {
+      throw new ERR_STREAM_NULL_VALUES();
+    } else if (typeof chunk !== "string" && !isUint8Array(chunk)) {
+      throw new ERR_INVALID_ARG_TYPE(
+        "chunk",
+        ["string", "Buffer", "Uint8Array"],
+        chunk,
+      );
     }
 
-    return false;
+    let len: number;
+
+    if (!msg._header) {
+      if (fromEnd) {
+        len ??= typeof chunk === "string"
+          ? Buffer.byteLength(chunk, encoding)
+          : chunk.byteLength;
+        msg._contentLength = len;
+      }
+      msg._implicitHeader();
+    }
+
+    return msg._send(chunk, encoding, callback);
   }
 
   // deno-lint-ignore no-explicit-any
@@ -499,70 +519,42 @@ export class OutgoingMessage extends Stream {
     return ret;
   }
 
-  // This abstract either writing directly to the socket or buffering it.
   // deno-lint-ignore no-explicit-any
   _send(data: any, encoding?: string | null, callback?: () => void) {
-    // 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.
     if (!this._headerSent && this._header !== null) {
-      // `this._header` can be null if OutgoingMessage is used without a proper Socket
-      // See: /test/parallel/test-http-outgoing-message-inheritance.js
-      if (
-        typeof data === "string" &&
-        (encoding === "utf8" || encoding === "latin1" || !encoding)
-      ) {
-        data = this._header + data;
-      } else {
-        const header = this._header;
-        this.outputData.unshift({
-          data: header,
-          encoding: "latin1",
-          callback: null,
-        });
-        this.outputSize += header.length;
-        this._onPendingData(header.length);
-      }
+      this._writeHeader();
       this._headerSent = true;
     }
     return this._writeRaw(data, encoding, callback);
   }
 
+  _writeHeader() {
+    throw new ERR_METHOD_NOT_IMPLEMENTED("_writeHeader()");
+  }
+
   _writeRaw(
-    // deno-lint-ignore no-explicit-any
-    this: any,
     // deno-lint-ignore no-explicit-any
     data: any,
     encoding?: string | null,
     callback?: () => void,
   ) {
-    const conn = this.socket;
-    if (conn && conn.destroyed) {
-      // The socket was destroyed. If we're still trying to write to it,
-      // then we haven't gotten the 'close' event yet.
+    if (typeof data === "string") {
+      data = Buffer.from(data, encoding);
+    }
+    if (data instanceof Buffer) {
+      data = new Uint8Array(data.buffer);
+    }
+    if (data.buffer.byteLength > 0) {
+      core.writeAll(this._bodyWriteRid, data).then(() => {
+        callback?.();
+        this.emit("drain");
+      }).catch((e) => {
+        this._requestSendError = e;
+      });
+    }
     return false;
   }
 
-    if (typeof encoding === "function") {
-      callback = encoding;
-      encoding = null;
-    }
-
-    if (conn && conn._httpMessage === this && conn.writable) {
-      // There might be pending data in the this.output buffer.
-      if (this.outputData.length) {
-        this._flushOutput(conn);
-      }
-      // Directly write to socket.
-      return conn.write(data, encoding, callback);
-    }
-    // Buffer, as long as we're not destroyed.
-    this.outputData.push({ data, encoding, callback });
-    this.outputSize += data.length;
-    this._onPendingData(data.length);
-    return this.outputSize < HIGH_WATER_MARK;
-  }
-
   _renderHeaders() {
     if (this._header) {
       throw new ERR_HTTP_HEADERS_SENT("render");
@@ -584,6 +576,170 @@ export class OutgoingMessage extends Stream {
     return headers;
   }
 
+  _storeHeader(firstLine: string, _headers: never) {
+    // firstLine in the case of request is: 'GET /index.html HTTP/1.1\r\n'
+    // in the case of response it is: 'HTTP/1.1 200 OK\r\n'
+    const state = {
+      connection: false,
+      contLen: false,
+      te: false,
+      date: false,
+      expect: false,
+      trailer: false,
+      header: firstLine,
+    };
+
+    const headers = this[kOutHeaders];
+    if (headers) {
+      // headers is null-prototype object, so ignore the guard lint
+      // deno-lint-ignore guard-for-in
+      for (const key in headers) {
+        const entry = headers[key];
+        this._matchHeader(state, entry[0], entry[1]);
+      }
+    }
+
+    // Date header
+    if (this.sendDate && !state.date) {
+      this.setHeader("Date", utcDate());
+    }
+
+    // Force the connection to close when the response is a 204 No Content or
+    // a 304 Not Modified and the user has set a "Transfer-Encoding: chunked"
+    // header.
+    //
+    // RFC 2616 mandates that 204 and 304 responses MUST NOT have a body but
+    // node.js used to send out a zero chunk anyway to accommodate clients
+    // that don't have special handling for those responses.
+    //
+    // It was pointed out that this might confuse reverse proxies to the point
+    // of creating security liabilities, so suppress the zero chunk and force
+    // the connection to close.
+    if (
+      this.chunkedEncoding && (this.statusCode === 204 ||
+        this.statusCode === 304)
+    ) {
+      debug(
+        this.statusCode + " response should not use chunked encoding," +
+          " closing connection.",
+      );
+      this.chunkedEncoding = false;
+      this.shouldKeepAlive = false;
+    }
+
+    // TODO(osddeitf): this depends on agent and underlying socket
+    // keep-alive logic
+    // if (this._removedConnection) {
+    //   this._last = true;
+    //   this.shouldKeepAlive = false;
+    // } else if (!state.connection) {
+    //   const shouldSendKeepAlive = this.shouldKeepAlive &&
+    //       (state.contLen || this.useChunkedEncodingByDefault || this.agent);
+    //   if (shouldSendKeepAlive && this.maxRequestsOnConnectionReached) {
+    //     this.setHeader('Connection', 'close');
+    //   } else if (shouldSendKeepAlive) {
+    //     this.setHeader('Connection', 'keep-alive');
+    //     if (this._keepAliveTimeout && this._defaultKeepAlive) {
+    //       const timeoutSeconds = Math.floor(this._keepAliveTimeout / 1000);
+    //       let max = '';
+    //       if (~~this._maxRequestsPerSocket > 0) {
+    //         max = `, max=${this._maxRequestsPerSocket}`;
+    //       }
+    //       this.setHeader('Keep-Alive', `timeout=${timeoutSeconds}${max}`);
+    //     }
+    //   } else {
+    //     this._last = true;
+    //     this.setHeader('Connection', 'close');
+    //   }
+    // }
+
+    if (!state.contLen && !state.te) {
+      if (!this._hasBody) {
+        // Make sure we don't end the 0\r\n\r\n at the end of the message.
+        this.chunkedEncoding = false;
+      } else if (!this.useChunkedEncodingByDefault) {
+        this._last = true;
+      } else if (
+        !state.trailer &&
+        !this._removedContLen &&
+        typeof this._contentLength === "number"
+      ) {
+        this.setHeader("Content-Length", this._contentLength);
+      } else if (!this._removedTE) {
+        this.setHeader("Transfer-Encoding", "chunked");
+        this.chunkedEncoding = true;
+      } else {
+        // We should only be able to get here if both Content-Length and
+        // Transfer-Encoding are removed by the user.
+        // See: test/parallel/test-http-remove-header-stays-removed.js
+        debug("Both Content-Length and Transfer-Encoding are removed");
+      }
+    }
+
+    // Test non-chunked message does not have trailer header set,
+    // message will be terminated by the first empty line after the
+    // header fields, regardless of the header fields present in the
+    // message, and thus cannot contain a message body or 'trailers'.
+    if (this.chunkedEncoding !== true && state.trailer) {
+      throw new ERR_HTTP_TRAILER_INVALID();
+    }
+
+    const { header } = state;
+    this._header = header + "\r\n";
+    this._headerSent = false;
+
+    // Wait until the first body chunk, or close(), is sent to flush,
+    // UNLESS we're sending Expect: 100-continue.
+    if (state.expect) this._send("");
+  }
+
+  _matchHeader(
+    // deno-lint-ignore no-explicit-any
+    state: any,
+    field: string,
+    // deno-lint-ignore no-explicit-any
+    value: any,
+  ) {
+    // Ignore lint to keep the code as similar to Nodejs as possible
+    // deno-lint-ignore no-this-alias
+    const self = this;
+    if (field.length < 4 || field.length > 17) {
+      return;
+    }
+    field = field.toLowerCase();
+    switch (field) {
+      case "connection":
+        state.connection = true;
+        self._removedConnection = false;
+        if (RE_CONN_CLOSE.exec(value) !== null) {
+          self._last = true;
+        } else {
+          self.shouldKeepAlive = true;
+        }
+        break;
+      case "transfer-encoding":
+        state.te = true;
+        self._removedTE = false;
+        if (RE_TE_CHUNKED.exec(value) !== null) {
+          self.chunkedEncoding = true;
+        }
+        break;
+      case "content-length":
+        state.contLen = true;
+        self._contentLength = value;
+        self._removedContLen = false;
+        break;
+      case "date":
+      case "expect":
+      case "trailer":
+        state[field] = true;
+        break;
+      case "keep-alive":
+        self._defaultKeepAlive = false;
+        break;
+    }
+  }
+
   // deno-lint-ignore no-explicit-any
   [EE.captureRejectionSymbol](err: any, _event: any) {
     this.destroy(err);

ext/node/polyfills/http.ts

@@ -566,7 +566,9 @@ class ClientRequest extends OutgoingMessage {
       }
     }*/
     this.onSocket(new FakeSocket({ encrypted: this._encrypted }));
+  }
 
+  _writeHeader() {
     const url = this._createUrlStrFromOptions();
 
     const headers = [];
@@ -585,12 +587,22 @@ class ClientRequest extends OutgoingMessage {
       url,
       headers,
       client.rid,
-      this.method === "POST" || this.method === "PATCH" ||
-        this.method === "PUT",
+      (this.method === "POST" || this.method === "PATCH" ||
+        this.method === "PUT") && this._contentLength !== 0,
     );
     this._bodyWriteRid = this._req.requestBodyRid;
   }
 
+  _implicitHeader() {
+    if (this._header) {
+      throw new ERR_HTTP_HEADERS_SENT("render");
+    }
+    this._storeHeader(
+      this.method + " " + this.path + " HTTP/1.1\r\n",
+      this[kOutHeaders],
+    );
+  }
+
   _getClient(): Deno.HttpClient | undefined {
     return undefined;
   }
@@ -615,8 +627,12 @@ class ClientRequest extends OutgoingMessage {
     }
 
     this.finished = true;
-    if (chunk !== undefined && chunk !== null) {
-      this.write(chunk, encoding);
+    if (chunk) {
+      this.write_(chunk, encoding, null, true);
+    } else if (!this._headerSent) {
+      this._contentLength = 0;
+      this._implicitHeader();
+      this._send("", "latin1");
     }
 
     (async () => {

tools/node_compat/TODO.md

@@ -3,7 +3,7 @@
 
 NOTE: This file should not be manually edited. Please edit `cli/tests/node_compat/config.json` and run `deno task setup` in `tools/node_compat` dir instead.
 
-Total: 2934
+Total: 2933
 
 - [abort/test-abort-backtrace.js](https://github.com/nodejs/node/tree/v18.12.1/test/abort/test-abort-backtrace.js)
 - [abort/test-abort-fatal-error.js](https://github.com/nodejs/node/tree/v18.12.1/test/abort/test-abort-fatal-error.js)
@@ -1002,7 +1002,6 @@ Total: 2934
 - [parallel/test-http-connect-req-res.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-http-connect-req-res.js)
 - [parallel/test-http-connect.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-http-connect.js)
 - [parallel/test-http-content-length-mismatch.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-http-content-length-mismatch.js)
-- [parallel/test-http-content-length.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-http-content-length.js)
 - [parallel/test-http-contentLength0.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-http-contentLength0.js)
 - [parallel/test-http-correct-hostname.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-http-correct-hostname.js)
 - [parallel/test-http-createConnection.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-http-createConnection.js)