From 3d41b486da7dcba49c8a18b45425e356c329d986 Mon Sep 17 00:00:00 2001
From: Luca Casonato <hello@lcas.dev>
Date: Tue, 11 Jun 2024 12:39:44 +0200
Subject: [PATCH] fix(ext/node): ServerResponse header array handling (#24149)

Previously res.setHeader("foo", ["bar", "baz"]) added a single header
with a value of `bar,baz`. Really this should add two separate headers.
This is visible in `set-cookie` for example.
---
 ext/node/polyfills/http.ts   | 47 ++++++++++++++++++++++++++----------
 tests/unit_node/http_test.ts | 31 ++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/ext/node/polyfills/http.ts b/ext/node/polyfills/http.ts
index ec3fe6e0b4..32e69772d6 100644
--- a/ext/node/polyfills/http.ts
+++ b/ext/node/polyfills/http.ts
@@ -1333,7 +1333,8 @@ function onError(self, error, cb) {
 export class ServerResponse extends NodeWritable {
   statusCode = 200;
   statusMessage?: string = undefined;
-  #headers = new Headers({});
+  #headers: Record<string, string | string[]> = { __proto__: null };
+  #hasNonStringHeaders: boolean = false;
   #readable: ReadableStream;
   override writable = true;
   // used by `npm:on-finished`
@@ -1411,32 +1412,35 @@ export class ServerResponse extends NodeWritable {
     this.socket = socket;
   }
 
-  setHeader(name: string, value: string) {
-    this.#headers.set(name, value);
+  setHeader(name: string, value: string | string[]) {
+    if (Array.isArray(value)) {
+      this.#hasNonStringHeaders = true;
+    }
+    this.#headers[name] = value;
     return this;
   }
 
   getHeader(name: string) {
-    return this.#headers.get(name) ?? undefined;
+    return this.#headers[name];
   }
   removeHeader(name: string) {
-    return this.#headers.delete(name);
+    delete this.#headers[name];
   }
   getHeaderNames() {
-    return Array.from(this.#headers.keys());
+    return Object.keys(this.#headers);
   }
   getHeaders() {
-    return Object.fromEntries(this.#headers.entries());
+    return { __proto__: null, ...this.#headers };
   }
   hasHeader(name: string) {
-    return this.#headers.has(name);
+    return Object.hasOwn(this.#headers, name);
   }
 
   writeHead(status: number, headers: Record<string, string> = {}) {
     this.statusCode = status;
     for (const k in headers) {
       if (Object.hasOwn(headers, k)) {
-        this.#headers.set(k, headers[k]);
+        this.setHeader(k, headers[k]);
       }
     }
     return this;
@@ -1461,9 +1465,26 @@ export class ServerResponse extends NodeWritable {
     if (ServerResponse.#bodyShouldBeNull(this.statusCode)) {
       body = null;
     }
+    let headers: Record<string, string> | [string, string][] = this
+      .#headers as Record<string, string>;
+    if (this.#hasNonStringHeaders) {
+      headers = [];
+      // Guard is not needed as this is a null prototype object.
+      // deno-lint-ignore guard-for-in
+      for (const key in this.#headers) {
+        const entry = this.#headers[key];
+        if (Array.isArray(entry)) {
+          for (const value of entry) {
+            headers.push([key, value]);
+          }
+        } else {
+          headers.push([key, entry]);
+        }
+      }
+    }
     this.#resolve(
       new Response(body, {
-        headers: this.#headers,
+        headers,
         status: this.statusCode,
         statusText: this.statusMessage,
       }),
@@ -1473,11 +1494,11 @@ export class ServerResponse extends NodeWritable {
   // deno-lint-ignore no-explicit-any
   override end(chunk?: any, encoding?: any, cb?: any): this {
     this.finished = true;
-    if (!chunk && this.#headers.has("transfer-encoding")) {
+    if (!chunk && "transfer-encoding" in this.#headers) {
       // FIXME(bnoordhuis) Node sends a zero length chunked body instead, i.e.,
       // the trailing "0\r\n", but respondWith() just hangs when I try that.
-      this.#headers.set("content-length", "0");
-      this.#headers.delete("transfer-encoding");
+      this.#headers["content-length"] = "0";
+      delete this.#headers["transfer-encoding"];
     }
 
     // @ts-expect-error The signature for cb is stricter than the one implemented here
diff --git a/tests/unit_node/http_test.ts b/tests/unit_node/http_test.ts
index 9cb409c39b..2b26442721 100644
--- a/tests/unit_node/http_test.ts
+++ b/tests/unit_node/http_test.ts
@@ -182,6 +182,33 @@ Deno.test("[node/http] server can respond with 101, 204, 205, 304 status", async
   }
 });
 
+Deno.test("[node/http] multiple set-cookie headers", async () => {
+  const { promise, resolve } = Promise.withResolvers<void>();
+
+  const server = http.createServer((_req, res) => {
+    res.setHeader("Set-Cookie", ["foo=bar", "bar=foo"]);
+    assertEquals(res.getHeader("Set-Cookie"), ["foo=bar", "bar=foo"]);
+    res.end();
+  });
+
+  server.listen(async () => {
+    const res = await fetch(
+      // deno-lint-ignore no-explicit-any
+      `http://127.0.0.1:${(server.address() as any).port}/`,
+    );
+    assert(res.ok);
+
+    const setCookieHeaders = res.headers.getSetCookie();
+    assertEquals(setCookieHeaders, ["foo=bar", "bar=foo"]);
+
+    await res.body!.cancel();
+
+    server.close(() => resolve());
+  });
+
+  await promise;
+});
+
 Deno.test("[node/http] IncomingRequest socket has remoteAddress + remotePort", async () => {
   const { promise, resolve } = Promise.withResolvers<void>();
 
@@ -1000,8 +1027,8 @@ Deno.test("[node/http] ServerResponse getHeaders", () => {
   const res = new http.ServerResponse(req);
   res.setHeader("foo", "bar");
   res.setHeader("bar", "baz");
-  assertEquals(res.getHeaderNames(), ["bar", "foo"]);
-  assertEquals(res.getHeaders(), { "bar": "baz", "foo": "bar" });
+  assertEquals(res.getHeaderNames(), ["foo", "bar"]);
+  assertEquals(res.getHeaders(), { "foo": "bar", "bar": "baz" });
 });
 
 Deno.test("[node/http] ServerResponse default status code 200", () => {