From 3c70124f4acc4e3bc29cc8662162c0065d9dbd0c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 4 Aug 2026 07:52:30 +0000 Subject: [PATCH] node:https: fail closed when createServer has no key or cert https.createServer delegated to http.Server, which only enables TLS when it sees key/cert/ca/pfx in the options. With none of those present the options were handed to Bun.serve({tls}) where a cert-less tls object is treated as no TLS, starting a plaintext HTTP listener on what the user intended as an https endpoint. A typo'd option name, an undefined from a failed file read, or a conditionally-assembled config would silently turn an https server into a working cleartext http server. Node's https.Server extends tls.Server, so it is always a TLS listener: with no cert every handshake fails and no byte is ever served in the clear. Give node:https its own Server class that forces the TLS path on (and carries the ALPN-defaulting that was previously only on createServer, so it also applies to `new https.Server`). Hoist http.Server's TLS-option parsing out of the options-object branch so the forced flag is honored regardless of how options was passed. http.createServer keeps its cleartext default. --- src/js/node/_http_server.ts | 139 ++++++++++--------- src/js/node/https.ts | 65 ++++----- test/js/node/http/node-https-server.test.ts | 141 ++++++++++++++++++++ 3 files changed, 244 insertions(+), 101 deletions(-) create mode 100644 test/js/node/http/node-https-server.test.ts diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 038a68813fae..96adf125a5fe 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -319,85 +319,84 @@ function Server(options, callback): void { } else { validateObject(options, "options"); options = { ...options }; + } - // Node's https.Server accepts PKCS#12 bundles (pfx [+ passphrase]); fold - // them into plain key/cert/ca so the native TLS config sees PEM material. - let tlsOptions = options; - if (options.pfx) { - tlsOptions = processPfxOptions(options); - this[isTlsSymbol] = true; - } + // Node's https.Server accepts PKCS#12 bundles (pfx [+ passphrase]); fold + // them into plain key/cert/ca so the native TLS config sees PEM material. + let tlsOptions = options; + if (options.pfx) { + tlsOptions = processPfxOptions(options); + this[isTlsSymbol] = true; + } - let cert = tlsOptions.cert; - if (cert) { - throwOnInvalidTLSArray("options.cert", cert); - this[isTlsSymbol] = true; - } + let cert = tlsOptions.cert; + if (cert) { + throwOnInvalidTLSArray("options.cert", cert); + this[isTlsSymbol] = true; + } - let key = tlsOptions.key; - if (key) { - throwOnInvalidTLSArray("options.key", key); - this[isTlsSymbol] = true; - } + let key = tlsOptions.key; + if (key) { + throwOnInvalidTLSArray("options.key", key); + this[isTlsSymbol] = true; + } - let ca = tlsOptions.ca; - // PKCS#12-embedded CAs extend the trust set; the server path hands raw - // {key, cert, ca} to the native config and has no addCACert hook, so fold - // them into `ca` (mirrors tls.Server.setSecureContext). - const pfxExtraCAs = tlsOptions._pfxExtraCACerts; - if (pfxExtraCAs?.length) { - ca = ca == null ? pfxExtraCAs : $isArray(ca) ? [...ca, ...pfxExtraCAs] : [ca, ...pfxExtraCAs]; - } - if (ca) { - throwOnInvalidTLSArray("options.ca", ca); - this[isTlsSymbol] = true; - } + let ca = tlsOptions.ca; + // PKCS#12-embedded CAs extend the trust set; the server path hands raw + // {key, cert, ca} to the native config and has no addCACert hook, so fold + // them into `ca` (mirrors tls.Server.setSecureContext). + const pfxExtraCAs = tlsOptions._pfxExtraCACerts; + if (pfxExtraCAs?.length) { + ca = ca == null ? pfxExtraCAs : $isArray(ca) ? [...ca, ...pfxExtraCAs] : [ca, ...pfxExtraCAs]; + } + if (ca) { + throwOnInvalidTLSArray("options.ca", ca); + this[isTlsSymbol] = true; + } - let passphrase = options.passphrase; - if (passphrase && typeof passphrase !== "string") { - throw $ERR_INVALID_ARG_TYPE("options.passphrase", "string", passphrase); - } + let passphrase = options.passphrase; + if (passphrase && typeof passphrase !== "string") { + throw $ERR_INVALID_ARG_TYPE("options.passphrase", "string", passphrase); + } - let serverName = options.servername; - if (serverName && typeof serverName !== "string") { - throw $ERR_INVALID_ARG_TYPE("options.servername", "string", serverName); - } + let serverName = options.servername; + if (serverName && typeof serverName !== "string") { + throw $ERR_INVALID_ARG_TYPE("options.servername", "string", serverName); + } - let secureOptions = options.secureOptions || 0; - if (secureOptions && typeof secureOptions !== "number") { - throw $ERR_INVALID_ARG_TYPE("options.secureOptions", "number", secureOptions); - } + let secureOptions = options.secureOptions || 0; + if (secureOptions && typeof secureOptions !== "number") { + throw $ERR_INVALID_ARG_TYPE("options.secureOptions", "number", secureOptions); + } - if (this[isTlsSymbol]) { - // Translate minVersion/maxVersion/secureProtocol into the integer - // protocol range the native layer applies (secureProtocol wins, like - // Node's SecureContext::Init); 0 keeps the native defaults. - validateSecureProtocol(options.secureProtocol); - let minVersion, maxVersion; - const range = secureProtocolToVersionRange(options.secureProtocol); - if (range) { - minVersion = range[0]; - maxVersion = range[1]; - } else { - minVersion = tlsStringToProtocolVersion(options.minVersion); - maxVersion = tlsStringToProtocolVersion(options.maxVersion); - } - this[tlsSymbol] = normalizeServerTls({ - serverName, - key, - cert, - ca, - passphrase, - secureOptions, - minVersion, - maxVersion, - ciphers: typeof options.ciphers === "string" && options.ciphers ? options.ciphers : undefined, - requestCert: options.requestCert, - rejectUnauthorized: options.rejectUnauthorized, - }); + // node:https pre-sets `isTlsSymbol` so its Server never falls back to plaintext. + if (this[isTlsSymbol]) { + // Translate minVersion/maxVersion/secureProtocol into the integer + // protocol range the native layer applies (secureProtocol wins, like + // Node's SecureContext::Init); 0 keeps the native defaults. + validateSecureProtocol(options.secureProtocol); + let minVersion, maxVersion; + const range = secureProtocolToVersionRange(options.secureProtocol); + if (range) { + minVersion = range[0]; + maxVersion = range[1]; } else { - this[tlsSymbol] = null; - } + minVersion = tlsStringToProtocolVersion(options.minVersion); + maxVersion = tlsStringToProtocolVersion(options.maxVersion); + } + this[tlsSymbol] = normalizeServerTls({ + serverName, + key, + cert, + ca, + passphrase, + secureOptions, + minVersion, + maxVersion, + ciphers: typeof options.ciphers === "string" && options.ciphers ? options.ciphers : undefined, + requestCert: options.requestCert, + rejectUnauthorized: options.rejectUnauthorized, + }); } this[optionsSymbol] = options; diff --git a/src/js/node/https.ts b/src/js/node/https.ts index 9d0ba688ee63..5ddf219a10c6 100644 --- a/src/js/node/https.ts +++ b/src/js/node/https.ts @@ -8,7 +8,7 @@ const net = require("node:net"); const { urlToHttpOptions } = require("internal/url"); const { kEmptyObject, once } = require("internal/shared"); const { validateObject } = require("internal/validators"); -const { kProxyConfig, checkShouldUseProxy, kWaitForProxyTunnel } = require("internal/http"); +const { kProxyConfig, checkShouldUseProxy, kWaitForProxyTunnel, isTlsSymbol } = require("internal/http"); const { validateHeaderValue } = require("node:_http_common"); const ArrayPrototypeShift = Array.prototype.shift; @@ -16,6 +16,38 @@ const ObjectAssign = Object.assign; const ArrayPrototypeUnshift = Array.prototype.unshift; const JSONStringify = JSON.stringify; +// Force TLS on so a missing key/cert fails handshakes instead of serving plaintext. +// https://github.com/nodejs/node/blob/v26.3.0/lib/https.js#L82-L97 +function Server(options, requestListener): void { + if (!(this instanceof Server)) return new Server(options, requestListener); + if (typeof options === "function") { + requestListener = options; + options = {}; + } else if (options == null) { + options = {}; + } else { + validateObject(options, "options"); + options = { ...options }; + } + if (!options.ALPNProtocols && !options.ALPNCallback) { + // http/1.0 is not in IANA's ALPN ID registry; Node defaults this to http/1.1. + options.ALPNProtocols = ["http/1.1"]; + } + this[isTlsSymbol] = true; + http.Server.$call(this, options, requestListener); + const optionsALPNProtocols = options.ALPNProtocols; + if (optionsALPNProtocols) { + tls.convertALPNProtocols(optionsALPNProtocols, this); + } + this.ALPNCallback = options.ALPNCallback; + return this; +} +$toClass(Server, "Server", http.Server); + +function createServer(options, requestListener) { + return new Server(options, requestListener); +} + function request(...args) { let options = {}; @@ -496,35 +528,6 @@ Agent.prototype._evictSession = function _evictSession(key) { const { shouldUseEnvProxy } = require("node:_http_agent"); -// Like Node's https.Server constructor: default ALPNProtocols to ['http/1.1'] -// when neither ALPNProtocols nor ALPNCallback was given, and store the -// normalized protocol list / callback on the server instance the way -// tls.Server does (test-https-argument-of-creating.js). -// https://github.com/nodejs/node/blob/v26.3.0/lib/https.js#L82-L97 -function createServer(options, requestListener) { - if (typeof options === "function") { - requestListener = options; - options = {}; - } else if (options == null) { - options = {}; - } else { - validateObject(options, "options"); - options = { ...options }; - } - if (!options.ALPNProtocols && !options.ALPNCallback) { - // http/1.0 is not defined as a Protocol ID in the IANA registry, so - // ALPN requests are always answered with http/1.1. - options.ALPNProtocols = ["http/1.1"]; - } - const server = http.createServer(options, requestListener); - const optionsALPNProtocols = options.ALPNProtocols; - if (optionsALPNProtocols) { - tls.convertALPNProtocols(optionsALPNProtocols, server); - } - server.ALPNCallback = options.ALPNCallback; - return server; -} - var https = { Agent, globalAgent: new Agent({ @@ -533,7 +536,7 @@ var https = { timeout: 5000, proxyEnv: shouldUseEnvProxy() ? process.env : undefined, }), - Server: http.Server, + Server, createServer, get, request, diff --git a/test/js/node/http/node-https-server.test.ts b/test/js/node/http/node-https-server.test.ts new file mode 100644 index 000000000000..2002482317b7 --- /dev/null +++ b/test/js/node/http/node-https-server.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, test } from "bun:test"; +import { tls as validCert } from "harness"; +import http from "node:http"; +import https from "node:https"; +import type { AddressInfo } from "node:net"; +import net from "node:net"; +import tls from "node:tls"; + +function listen(server: http.Server): Promise { + const { promise, resolve, reject } = Promise.withResolvers(); + server.once("error", reject); + server.listen(0, "127.0.0.1", () => resolve((server.address() as AddressInfo).port)); + return promise; +} + +// Speaks bare HTTP/1.1 at `port` and resolves with everything the server wrote +// back before the connection ended. +function plaintextRequest(port: number): Promise { + const { promise, resolve } = Promise.withResolvers(); + let received = ""; + const socket = net.connect(port, "127.0.0.1", () => { + socket.write("GET /secret HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n"); + }); + socket.on("data", chunk => (received += chunk)); + // A TLS listener answers cleartext bytes with an alert and hangs up, so an + // 'error' (ECONNRESET) is as valid an outcome here as a clean 'close'. + socket.on("error", () => resolve(received)); + socket.on("close", () => resolve(received)); + return promise; +} + +function tlsHandshake(port: number): Promise<{ connected: boolean; code?: string }> { + const { promise, resolve } = Promise.withResolvers<{ connected: boolean; code?: string }>(); + const socket = tls.connect({ port, host: "127.0.0.1", rejectUnauthorized: false }, () => { + socket.destroy(); + resolve({ connected: true }); + }); + socket.on("error", (err: NodeJS.ErrnoException) => resolve({ connected: false, code: err.code })); + return promise; +} + +// An https.Server with no key/cert must still be a TLS listener, never a cleartext one. +describe("https.Server with no key and no cert", () => { + const shapes: Array<[string, (handler: http.RequestListener) => http.Server]> = [ + ["createServer(requestListener)", handler => https.createServer(handler)], + ["createServer({}, requestListener)", handler => https.createServer({}, handler)], + ["createServer(undefined, requestListener)", handler => https.createServer(undefined, handler)], + ["new https.Server({}, requestListener)", handler => new https.Server({}, handler)], + ]; + + describe.each(shapes)("%s", (_label, createServer) => { + test("never answers a cleartext HTTP request", async () => { + let requestHandlerRan = false; + await using server = createServer((_req, res) => { + requestHandlerRan = true; + res.end("SECRET"); + }); + const port = await listen(server); + + expect(await plaintextRequest(port)).toBe(""); + expect(requestHandlerRan).toBe(false); + }); + + test("is a TLS listener, so every handshake fails", async () => { + await using server = createServer((_req, res) => res.end("SECRET")); + const port = await listen(server); + + const result = await tlsHandshake(port); + expect(result.connected).toBe(false); + // A cleartext listener would yield ERR_SSL_WRONG_VERSION_NUMBER here. + expect(result.code).toContain("_ALERT_"); + }); + }); +}); + +test("https.createServer() with a key and cert still serves over TLS", async () => { + await using server = https.createServer({ ...validCert }, (_req, res) => res.end("ok")); + const port = await listen(server); + + const response = await fetch(`https://127.0.0.1:${port}/`, { tls: { rejectUnauthorized: false } }); + expect(await response.text()).toBe("ok"); + expect(response.status).toBe(200); +}); + +test("https.createServer() with a key and cert does not answer cleartext", async () => { + await using server = https.createServer({ ...validCert }, (_req, res) => res.end("SECRET")); + const port = await listen(server); + + expect(await plaintextRequest(port)).toBe(""); +}); + +// Only node:https forces the TLS path on. http.createServer() without cert +// material stays a plain HTTP listener. +test("http.createServer({}) still serves cleartext", async () => { + await using server = http.createServer({}, (_req, res) => res.end("PLAINTEXT")); + const port = await listen(server); + + expect(await plaintextRequest(port)).toContain("PLAINTEXT"); +}); + +test("https.Server is its own class, not http.Server", () => { + expect(https.Server).not.toBe(http.Server); + expect(https.createServer({})).toBeInstanceOf(https.Server); + expect(new https.Server({})).toBeInstanceOf(https.Server); + expect(new https.Server({})).toBeInstanceOf(http.Server); +}); + +describe("https.Server ALPN defaults apply to new Server() as well as createServer()", () => { + const defaultALPN = {} as { ALPNProtocols: Buffer }; + tls.convertALPNProtocols(["http/1.1"], defaultALPN); + + test.each([ + ["https.createServer()", () => https.createServer()], + ["new https.Server()", () => new https.Server()], + ])("%s defaults ALPNProtocols to http/1.1", (_label, create) => { + const server = create(); + expect(Buffer.isBuffer(server.ALPNProtocols)).toBe(true); + expect(server.ALPNProtocols.equals(defaultALPN.ALPNProtocols)).toBe(true); + expect(server.ALPNCallback).toBeUndefined(); + }); + + test("new https.Server() keeps an explicit ALPNProtocols", () => { + const explicit = {} as { ALPNProtocols: Buffer }; + tls.convertALPNProtocols(["h2", "http/1.1"], explicit); + const server = new https.Server({ ALPNProtocols: ["h2", "http/1.1"] }); + expect(server.ALPNProtocols.equals(explicit.ALPNProtocols)).toBe(true); + }); + + test("new https.Server() stores ALPNCallback and skips the default protocol list", () => { + const ALPNCallback = () => "http/1.1"; + const server = new https.Server({ ALPNCallback }); + expect(server.ALPNCallback).toBe(ALPNCallback); + expect(server.ALPNProtocols).toBeUndefined(); + }); + + test("new https.Server(requestListener) registers the listener", () => { + const requestListener = () => {}; + const server = new https.Server(requestListener); + expect(server.listeners("request")).toEqual([requestListener]); + }); +});