Check http.request options as Node does (#2606)

packages/compiler/src/frontend/lowering/lower-server.ts

@@ -6499,6 +6499,33 @@ function lowerHttp2ModuleCall(
   );
 }
 
+/** The scalar request options the literal walk lowers, with the type it
+ * lowers each one at. */
+const LITERAL_REQUEST_OPTION_KINDS: ReadonlyMap<string, "string" | "f64"> = new Map([
+  ["hostname", "string"],
+  ["host", "string"],
+  ["port", "f64"],
+  ["path", "string"],
+  ["method", "string"],
+  ["timeout", "f64"],
+  ["maxHeaderSize", "f64"],
+]);
+
+/** Whether a literal's scalar option holds a value of another type than the
+ * literal walk lowers it at, which sends the literal to the runtime walk. */
+function runtimeCheckedOption(
+  lowerer: Lowerer,
+  prop: ts.PropertyAssignment | ts.ShorthandPropertyAssignment,
+): boolean {
+  const want = LITERAL_REQUEST_OPTION_KINDS.get(
+    (prop.name as ts.Identifier | ts.StringLiteral).text,
+  );
+  if (want === undefined) return false;
+  const value = ts.isPropertyAssignment(prop) ? prop.initializer : prop.name;
+  const t = lowerer.mapTypeOf(lowerer.typeOf(value));
+  return t !== null && t.kind !== want;
+}
+
 /** The checked-dynamic options value of a request whose options the
  * runtime reads: a non-literal options value (a RequestOptions variable —
  * checked-dynamic storage — or a record that converts to one), or a
@@ -6528,7 +6555,11 @@ function runtimeRequestOptions(
         (ts.isIdentifier(prop.name) || ts.isStringLiteral(prop.name)) &&
         (prop.name.text === "socketPath" ||
           // a client certificate: the runtime request walk reads cert and key
-          (secureish && (prop.name.text === "cert" || prop.name.text === "key")))
+          (secureish && (prop.name.text === "cert" || prop.name.text === "key")) ||
+          // a scalar option whose value is not the walk's type (an untyped
+          // or union value, a string port): the runtime walk runs Node's
+          // checks on it (ERR_INVALID_ARG_TYPE with the option's name)
+          runtimeCheckedOption(lowerer, prop))
       );
     });
     if (!needsRuntime) return null;

packages/compiler/test/ts7/baselines/order-parity/tests/corpus/http-client-request-path-check.ts.json

@@ -0,0 +1,6 @@
+{
+ "order": [
+  "<repo>/tests/corpus/http-client-request-path-check.ts"
+ ],
+ "diags": []
+}

packages/compiler/test/ts7/baselines/order-parity/tests/corpus/http-request-checks-before-socket.ts.json

@@ -0,0 +1,6 @@
+{
+ "order": [
+  "<repo>/tests/corpus/http-request-checks-before-socket.ts"
+ ],
+ "diags": []
+}

packages/compiler/test/ts7/baselines/order-parity/tests/corpus/http-request-option-checks.cjs.json

@@ -0,0 +1,6 @@
+{
+ "order": [
+  "<repo>/tests/corpus/http-request-option-checks.cjs"
+ ],
+ "diags": []
+}

packages/runtime/src/scr_http.c

@@ -5158,18 +5158,31 @@ static void scr_http_client_conn_free(ScrHttpConn *conn) {
 
 /* ── http.request / http.get ─────────────────────────────────────────── */
 
-static ScrHttpClientReq *scr_http_request_impl(ScrStr *host /*borrowed*/, double port,
-                                                ScrStr *path /*borrowed*/, ScrStr *method /*borrowed*/,
-                                                double timeout_ms, ScrArr *header_pairs /*borrowed*/,
-                                                bool auto_end, ScrClosure *cb /*moves, nullable*/,
-                                                ScrHttpRespFn fn, int default_port,
-                                                void (*wrap)(ScrNetSocket *, void *), void *wrap_ctx,
-                                                ScrNetSocket *presock /*moves, nullable*/) {
-  /* Node's ClientRequest constructor validates the method token
-   * SYNCHRONOUSLY (RFC 9110 tchar, at least one): the catchable
-   * ERR_INVALID_HTTP_TOKEN TypeError with Node's exact message, the raw
-   * method text inside the quotes. Moved-in arguments release — the
-   * request never exists. */
+/* ClientRequest's path check: characters outside U+0021..U+00FF throw
+ * ERR_UNESCAPED_CHARACTERS. In UTF-8, U+0080..U+00FF are the C2/C3 lead
+ * bytes. */
+static bool scr_http_request_path_valid(const ScrStr *path /*borrowed*/) {
+  for (size_t i = 0; i < path->len; i++) {
+    unsigned char ch = (unsigned char)path->data[i];
+    if (ch >= 0x21 && ch < 0x80) continue;
+    if (ch == 0xc2 || ch == 0xc3) {
+      i++;
+      continue;
+    }
+    static const char msg[] = "Request path contains unescaped characters";
+    scr_throw_error_msg_code(SCR_ERR_TYPE, msg, sizeof msg - 1, "ERR_UNESCAPED_CHARACTERS");
+    return false;
+  }
+  return true;
+}
+
+/* The ClientRequest constructor's checks that come before any socket
+ * exists, in Node's order: the path (ERR_UNESCAPED_CHARACTERS), then the
+ * method token (RFC 9110 tchar, at least one; ERR_INVALID_HTTP_TOKEN with
+ * the raw method text inside the quotes). False with the exception
+ * pending. */
+static bool scr_http_request_checks(const ScrStr *path /*borrowed*/, const ScrStr *method /*borrowed*/) {
+  if (!scr_http_request_path_valid(path)) return false;
   bool method_ok = method->len > 0;
   for (size_t i = 0; i < method->len && method_ok; i++) {
     unsigned char ch = (unsigned char)method->data[i];
@@ -5177,17 +5190,30 @@ static ScrHttpClientReq *scr_http_request_impl(ScrStr *host /*borrowed*/, double
                 (ch >= '0' && ch <= '9') ||
                 (ch != 0 && strchr("!#$%&'*+-.^_`|~", ch) != NULL);
   }
-  if (!method_ok) {
+  if (method_ok) return true;
+  ScrJsonBuf b;
+  scr_jb_init(&b);
+  scr_jb_puts(&b, "Method must be a valid HTTP token [\"");
+  scr_jb_write(&b, method->data, method->len);
+  scr_jb_puts(&b, "\"]");
+  ScrStr *msg = scr_jb_finish(&b);
+  scr_throw_error_msg_code(SCR_ERR_TYPE, msg->data, msg->len, "ERR_INVALID_HTTP_TOKEN");
+  scr_str_release(msg);
+  return false;
+}
+
+static ScrHttpClientReq *scr_http_request_impl(ScrStr *host /*borrowed*/, double port,
+                                                ScrStr *path /*borrowed*/, ScrStr *method /*borrowed*/,
+                                                double timeout_ms, ScrArr *header_pairs /*borrowed*/,
+                                                bool auto_end, ScrClosure *cb /*moves, nullable*/,
+                                                ScrHttpRespFn fn, int default_port,
+                                                void (*wrap)(ScrNetSocket *, void *), void *wrap_ctx,
+                                                ScrNetSocket *presock /*moves, nullable*/) {
+  /* Node's ClientRequest constructor checks; moved-in arguments release
+   * when they fail — the request never exists. */
+  if (!scr_http_request_checks(path, method)) {
     if (cb) scr_closure_release(cb);
     if (presock) scr_net_sock_release(presock);
-    ScrJsonBuf b;
-    scr_jb_init(&b);
-    scr_jb_puts(&b, "Method must be a valid HTTP token [\"");
-    scr_jb_write(&b, method->data, method->len);
-    scr_jb_puts(&b, "\"]");
-    ScrStr *msg = scr_jb_finish(&b);
-    scr_throw_error_msg_code(SCR_ERR_TYPE, msg->data, msg->len, "ERR_INVALID_HTTP_TOKEN");
-    scr_str_release(msg);
     return NULL;
   }
   scr_http_install();
@@ -5288,6 +5314,12 @@ ScrHttpClientReq *scr_http_request_conn(ScrClosure *conn_cb /*moves*/, ScrStr *p
                                          ScrStr *method /*borrowed*/, double timeout_ms,
                                          ScrArr *header_pairs /*borrowed*/, bool auto_end,
                                          ScrClosure *cb /*moves, nullable*/, ScrHttpRespFn fn) {
+  /* Node checks the path and method before it calls createConnection */
+  if (!scr_http_request_checks(path, method)) {
+    scr_closure_release(conn_cb);
+    if (cb != NULL) scr_closure_release(cb);
+    return NULL;
+  }
   ScrNetSocket *sock = ((ScrNetSocket *(*)(ScrClosure *))conn_cb->fn)(conn_cb); /* +1 result */
   scr_closure_release(conn_cb);
   if (scr_exc_pending() || sock == NULL) {
@@ -5809,6 +5841,11 @@ static ScrHttpClientReq *scr_http_request_pooled(ScrHttpAgent *ag, ScrStr *host
                                                  void (*wrap)(ScrNetSocket *, void *),
                                                  void *wrap_ctx, ScrStr *pool_key /*borrowed*/,
                                                  ScrStr *socket_path /*borrowed*/) {
+  /* the constructor's checks come before the agent takes or dials a socket */
+  if (!scr_http_request_checks(path, method)) {
+    if (cb) scr_closure_release(cb);
+    return NULL;
+  }
   double p = port >= 0 ? port
            : ag->default_port > 0 ? ag->default_port
            : (double)default_port;
@@ -5869,6 +5906,11 @@ ScrHttpClientReq *scr_http_request_agent_ex(ScrStr *host /*borrowed*/, double po
   /* agent: false — Node's one-shot Agent: exactly this client's model,
    * plus its Connection: close request header (unless the caller set one) */
   if (agent->kind == SCR_DYN_BOOL && !agent->v.b) {
+    /* the constructor's checks come before a Unix socket is dialed */
+    if (!scr_http_request_checks(path, method)) {
+      if (cb) scr_closure_release(cb);
+      return NULL;
+    }
     double p = port >= 0 ? port : (double)default_port;
     bool has_conn = false;
     size_t npairs = (size_t)scr_arr_len(header_pairs);
@@ -7435,19 +7477,7 @@ bool scr_http_req_opts_read(const ScrDyn *opts, bool secure, const char *api, Sc
       scr_http_req_opts_free(o);
       return false;
     }
-    bool bad = false;
-    for (size_t i = 0; i < o->path->len && !bad; i++) {
-      unsigned char ch = (unsigned char)o->path->data[i];
-      if (ch < 0x21) bad = true;
-      else if (ch >= 0x80) {
-        /* U+0080..U+00FF (lead byte C2/C3) pass; anything above is unescaped */
-        if (ch != 0xc2 && ch != 0xc3) bad = true;
-        else i++;
-      }
-    }
-    if (bad) {
-      static const char msg[] = "Request path contains unescaped characters";
-      scr_throw_error_msg_code(SCR_ERR_TYPE, msg, sizeof msg - 1, "ERR_UNESCAPED_CHARACTERS");
+    if (!scr_http_request_path_valid(o->path)) {
       scr_http_req_opts_free(o);
       return false;
     }

tests/corpus/http-client-request-path-check.ts

@@ -0,0 +1,30 @@
+// http.request checks the path before it sends anything: characters outside
+// U+0021..U+00FF throw ERR_UNESCAPED_CHARACTERS and no request is made.
+// Latin-1 characters and DEL pass.
+import * as http from "node:http";
+
+const paths = ["/plain", "/latinéÿ", "/del\u007f", "/space here", "/tab\there", "/wideĀ", "/emoji\u{1f600}"];
+
+const server = http.createServer((req, res) => {
+  console.log("served", JSON.stringify(req.url));
+  res.end();
+});
+
+server.listen(0, "127.0.0.1", () => {
+  const port = (server.address() as { port: number }).port;
+  for (const path of paths) {
+    try {
+      const req = http.request({ host: "127.0.0.1", port, path });
+      req.on("error", () => {});
+      req.destroy();
+      console.log("accepted", JSON.stringify(path));
+    } catch (error) {
+      const e = error as NodeJS.ErrnoException;
+      console.log("threw", JSON.stringify(path), e.name, e.code, e.message);
+    }
+  }
+  http.get({ host: "127.0.0.1", port, path: "/after" }, (res) => {
+    res.resume();
+    res.on("end", () => server.close());
+  });
+});

tests/corpus/http-request-checks-before-socket.ts

@@ -0,0 +1,59 @@
+// http.request checks the path and the method before it opens a socket, as
+// Node's ClientRequest constructor does: createConnection is not called and
+// a socketPath server sees no connection when either check throws.
+import * as http from "node:http";
+import * as net from "node:net";
+import * as os from "node:os";
+import * as path from "node:path";
+
+function attempt(label: string, fn: () => void): void {
+  try {
+    fn();
+    console.log(label, "no throw");
+  } catch (error) {
+    const e = error as NodeJS.ErrnoException;
+    console.log(label, e.name, e.code);
+  }
+}
+
+let connections = 0;
+const socketPath = path.join(os.tmpdir(), `scriptc-checks-${process.pid}.sock`);
+const server = net.createServer((socket) => {
+  connections++;
+  socket.destroy();
+});
+
+server.listen(socketPath, () => {
+  attempt("createConnection path", () => {
+    http.request({
+      path: "/a b",
+      createConnection: () => {
+        console.log("createConnection called");
+        return net.connect(socketPath);
+      },
+    });
+  });
+  attempt("createConnection method", () => {
+    http.request({
+      path: "/",
+      method: "BAD METHOD",
+      createConnection: () => {
+        console.log("createConnection called");
+        return net.connect(socketPath);
+      },
+    });
+  });
+  attempt("socketPath path", () => {
+    http.request({ socketPath, path: "/a\tb" });
+  });
+  attempt("socketPath agent false path", () => {
+    http.request({ socketPath, path: "/a b", agent: false });
+  });
+  attempt("socketPath method", () => {
+    http.request({ socketPath, path: "/", method: "A(B" });
+  });
+  setTimeout(() => {
+    console.log("connections", connections);
+    server.close(() => console.log("closed"));
+  }, 200);
+});

tests/corpus/http-request-option-checks.cjs

@@ -0,0 +1,34 @@
+// http.request runs Node's option checks on untyped values in an options
+// literal: a method, hostname or host that is not a string throws
+// ERR_INVALID_ARG_TYPE naming the option, undefined and null take the
+// defaults, and a port string that is not a port throws ERR_SOCKET_BAD_PORT.
+'use strict';
+const http = require('http');
+
+function attempt(label, fn) {
+  try {
+    fn();
+    console.log(label, 'ok');
+  } catch (e) {
+    console.log(label, e.name, e.code, e.message);
+  }
+}
+
+for (const method of [-1, {}, true, []]) {
+  attempt('method', () => http.request({ method, path: '/' }, () => {}));
+}
+for (const hostname of [1, [], new Date(), false]) {
+  attempt('hostname', () => http.request({ hostname }));
+}
+for (const host of [NaN, {}]) {
+  attempt('host', () => http.request({ host }));
+}
+[undefined, null, 'GET', 'post'].forEach((method) => {
+  attempt(`method ${method}`, () => {
+    const req = http.request({ method, port: 1, agent: false });
+    req.on('error', () => {});
+    req.destroy();
+  });
+});
+const port = 'x';
+attempt('port', () => http.request({ port }));