Skip to content
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/http/ssl_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,22 @@ impl SSLConfig {
Self::default()
}

/// Whether this config carries a key+cert pair a TLS server can present.
#[inline]
pub fn has_identity_material(&self) -> bool {
self.has_any_cert() && self.has_any_key()
}

#[inline]
pub fn has_any_cert(&self) -> bool {
self.cert.is_some() || !self.cert_file_name.is_null()
}

#[inline]
pub fn has_any_key(&self) -> bool {
self.key.is_some() || !self.key_file_name.is_null()
}
Comment thread
robobun marked this conversation as resolved.
Resolved

/// Borrow `server_name` as a `&CStr` (None if null). Convenience accessor
/// for callers that previously pattern-matched `Option<CString>`.
#[inline]
Expand Down
5 changes: 5 additions & 0 deletions src/js/node/_http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,11 @@ function Server(options, callback): void {
} else {
validateObject(options, "options");
options = { ...options };
if (options[isTlsSymbol]) {
// Set by `node:https`; an https server is TLS even without key/cert.
this[isTlsSymbol] = true;
delete options[isTlsSymbol];
}

// 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.
Expand Down
21 changes: 14 additions & 7 deletions src/js/node/https.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -501,7 +501,8 @@ const { shouldUseEnvProxy } = require("node:_http_agent");
// 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) {
function Server(options, requestListener) {
if (!(this instanceof Server)) return new Server(options, requestListener);
if (typeof options === "function") {
requestListener = options;
options = {};
Expand All @@ -516,13 +517,19 @@ function createServer(options, requestListener) {
// ALPN requests are always answered with http/1.1.
options.ALPNProtocols = ["http/1.1"];
}
const server = http.createServer(options, requestListener);
// https is always TLS even without key/cert (handshakes fail closed, like Node).
options[isTlsSymbol] = true;
http.Server.$call(this, options, requestListener);
const optionsALPNProtocols = options.ALPNProtocols;
if (optionsALPNProtocols) {
tls.convertALPNProtocols(optionsALPNProtocols, server);
tls.convertALPNProtocols(optionsALPNProtocols, this);
}
server.ALPNCallback = options.ALPNCallback;
return server;
this.ALPNCallback = options.ALPNCallback;
}
$toClass(Server, "Server", http.Server);

function createServer(options, requestListener) {
return new Server(options, requestListener);
}

var https = {
Expand All @@ -533,7 +540,7 @@ var https = {
timeout: 5000,
proxyEnv: shouldUseEnvProxy() ? process.env : undefined,
}),
Server: http.Server,
Server,
createServer,
get,
request,
Expand Down
40 changes: 30 additions & 10 deletions src/runtime/server/ServerConfig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1369,25 +1369,36 @@
}
}

// node:https keeps Node's cert-less fail-closed listener; direct Bun.serve throws.
let is_node_http = !args.on_node_http_request.is_empty();
let require_identity = |global: &JSGlobalObject, cfg: Option<&SSLConfig>| -> JsResult<()> {
if is_node_http || cfg.is_some_and(SSLConfig::has_identity_material) {
return Ok(());
}
Err(global.throw_invalid_arguments(format_args!(
"tls option requires both \"key\" and \"cert\" (or \"keyFile\" and \"certFile\")",
)))
};

let mut saw_tls_key = false;
if let Some(tls) = arg.get_truthy(global, "tls")? {
if tls.is_falsey() {
args.ssl_config = None;
} else if tls.js_type().is_array() {
if tls.js_type().is_array() {

Check failure on line 1385 in src/runtime/server/ServerConfig.rs

View check run for this annotation

Claude / Claude Code Review

tls: false regression — removed is_falsey() branch was not dead

Commit e746f9dd removed `if tls.is_falsey() { args.ssl_config = None }` as "dead", but `get_truthy()` only filters `undefined`/`null`/empty-string — it passes `false`/`0`/`NaN` through — so the branch was live. `Bun.serve({ tls: false, fetch })` (and the common `tls: process.env.USE_TLS && config` idiom) now falls into the single-object arm, where `SSLConfig::from_js` on a boolean primitive throws `"tls" must be an object` instead of serving plaintext HTTP as before. Restore the falsey guard (le
Comment thread
robobun marked this conversation as resolved.
Resolved
let mut value_iter = tls.array_iterator(global)?;
if value_iter.len == 0 {
// Empty TLS array means no TLS - this is valid
// `[]` = zero SNI configs (plain HTTP, #21792), unlike `{}` which throws.
} else {
saw_tls_key = true;
while let Some(item) = value_iter.next()? {
let ssl_config = match SSLConfig::from_js(vm, global, item)? {
Some(c) => c,
None => {
if global.has_exception() {
return Err(JsError::Thrown);
}
// Backwards-compatibility; we ignored empty tls objects.
continue;
}
};
require_identity(global, Some(&ssl_config))?;
Comment thread
robobun marked this conversation as resolved.
Resolved

if args.ssl_config.is_none() {
args.ssl_config = Some(ssl_config);
Expand All @@ -1405,14 +1416,19 @@
args.sni.as_mut().unwrap().push(ssl_config);
}
}
if args.ssl_config.is_none() {
require_identity(global, None)?;
}
Comment thread
robobun marked this conversation as resolved.
Resolved
}
} else {
if let Some(ssl_config) = SSLConfig::from_js(vm, global, tls)? {
args.ssl_config = Some(ssl_config);
}
saw_tls_key = true;
let parsed = SSLConfig::from_js(vm, global, tls)?;
if global.has_exception() {
return Err(JsError::Thrown);
}
require_identity(global, parsed.as_ref())?;
// Only node:https reaches here without identity; arm cert-less.
args.ssl_config = Some(parsed.unwrap_or_else(SSLConfig::zero));
}
}
if global.has_exception() {
Expand All @@ -1421,9 +1437,13 @@

// @compatibility Bun v0.x - v0.2.1
// this used to be top-level, now it's "tls" object
if args.ssl_config.is_none() {
if args.ssl_config.is_none() && !saw_tls_key {
Comment thread
robobun marked this conversation as resolved.
Resolved
if let Some(ssl_config) = SSLConfig::from_js(vm, global, arg)? {
args.ssl_config = Some(ssl_config);
if ssl_config.has_identity_material() {
args.ssl_config = Some(ssl_config);
} else if ssl_config.has_any_key() || ssl_config.has_any_cert() {
require_identity(global, Some(&ssl_config))?;
}
}
Comment thread
robobun marked this conversation as resolved.
Resolved
if global.has_exception() {
return Err(JsError::Thrown);
Expand Down
6 changes: 2 additions & 4 deletions test/integration/bun-types/fixture/serve-types.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,10 +293,8 @@ test(
tls: {},
},
{
overrideExpectBehavior: server => {
expect(server.hostname).toBeUndefined();
expect(server.port).toBeUndefined();
expect(server.url.toString()).toStartWith("unix://");
onConstructorFailure: error => {
expect(error.message).toContain('tls option requires both "key" and "cert"');
},
},
);
Expand Down
150 changes: 150 additions & 0 deletions test/js/bun/http/bun-serve-ssl.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { describe, expect, test } from "bun:test";
import { tls as tlsCert } from "harness";
import { once } from "node:events";
import { Server as HttpServer } from "node:http";
import { createServer as createHttpsServer, Server as HttpsServer } from "node:https";
import { connect as tlsConnect } from "node:tls";
import privateKey from "../../third_party/jsonwebtoken/priv.pem" with { type: "text" };
import publicKey from "../../third_party/jsonwebtoken/pub.pem" with { type: "text" };

Expand Down Expand Up @@ -149,3 +154,148 @@ describe("Bun.serve SSL validations", () => {
}
}
});

// `tls: {}` used to fall back to plaintext and tuning-only `tls` stood up a
// cert-less https:// listener; both must now throw synchronously.
describe("Bun.serve tls must carry key+cert", () => {
const withoutIdentity = [
["empty object", {}],
["rejectUnauthorized only", { rejectUnauthorized: false }],
["requestCert only", { requestCert: true }],
["lowMemoryMode only", { lowMemoryMode: true }],
["ciphers only", { ciphers: "DEFAULT" }],
["key without cert", { key: tlsCert.key }],
["cert without key", { cert: tlsCert.cert }],
["array entry without cert", [{ key: tlsCert.key, rejectUnauthorized: false }]],
["array with only an empty entry", [{}]],
["array with only empty entries", [{}, {}]],
] as const;
for (const [label, tls] of withoutIdentity) {
test(label, () => {
expect(() =>
Bun.serve({
port: 0,
tls: tls as Bun.TLSOptions,
fetch: () => new Response("ok"),
}),
).toThrow('tls option requires both "key" and "cert"');
});
}

test("key+cert is accepted and serves over TLS", async () => {
await using server = Bun.serve({
port: 0,
tls: { key: tlsCert.key, cert: tlsCert.cert },
fetch: () => new Response("ok"),
});
expect(server.url.protocol).toBe("https:");
const res = await fetch(server.url, { tls: { rejectUnauthorized: false } });
expect(await res.text()).toBe("ok");
});

test("top-level tuning field does not arm TLS", async () => {
await using server = Bun.serve({
port: 0,
lowMemoryMode: true,
requestCert: true,
fetch: () => new Response("ok"),
} as any);
expect(server.url.protocol).toBe("http:");
const res = await fetch(server.url);
expect(await res.text()).toBe("ok");
});

for (const [label, opts] of [
["key without cert", { key: tlsCert.key }],
["cert without key", { cert: tlsCert.cert }],
] as const) {
test(`top-level ${label} throws (legacy path)`, () => {
expect(() => Bun.serve({ port: 0, ...opts, fetch: () => new Response("ok") } as any)).toThrow(
'tls option requires both "key" and "cert"',
);
});
}

test("top-level key+cert still arms TLS (legacy path)", async () => {
await using server = Bun.serve({
port: 0,
key: tlsCert.key,
cert: tlsCert.cert,
fetch: () => new Response("ok"),
} as any);
expect(server.url.protocol).toBe("https:");
});

test("tls: [] with top-level key+cert still arms TLS (legacy path)", async () => {
await using server = Bun.serve({
port: 0,
tls: [],
key: tlsCert.key,
cert: tlsCert.cert,
fetch: () => new Response("ok"),
} as any);
expect(server.url.protocol).toBe("https:");
});
});

describe("node:https without key/cert is TLS fail-closed", () => {
const matrix = [
["no options", undefined],
["empty options", {}],
["requestCert only", { requestCert: true }],
["rejectUnauthorized only", { rejectUnauthorized: false }],
] as const;
const makers = [
["createServer", (o: any, h: any) => createHttpsServer(o, h)],
["new Server", (o: any, h: any) => new HttpsServer(o, h)],
] as const;
for (const [makeLabel, make] of makers) {
for (const [label, options] of matrix) {
test(`${makeLabel}: ${label}`, async () => {
const server = make(options, (_req, res) => res.end("plaintext"));
await once(server.listen(0), "listening");
const { port } = server.address() as import("node:net").AddressInfo;
try {
// A plaintext HTTP client must not get a response body.
await expect(
fetch(`http://127.0.0.1:${port}/`, { signal: AbortSignal.timeout(2000) }).then(r => r.text()),
).rejects.toThrow();

// A TLS client must see the handshake fail (not succeed).
const handshake = new Promise<void>((resolve, reject) => {
const sock = tlsConnect({ port, host: "127.0.0.1", rejectUnauthorized: false });
sock.once("secureConnect", () => {
sock.destroy();
reject(new Error("handshake unexpectedly succeeded"));
});
sock.once("error", () => {
sock.destroy();
resolve();
});
});
await expect(handshake).resolves.toBeUndefined();
} finally {
await new Promise<void>(r => server.close(() => r()));
}
});
}
}

test("https.Server instanceof http.Server", () => {
const server = new HttpsServer({});
expect(server instanceof HttpsServer).toBe(true);
expect(server instanceof HttpServer).toBe(true);
});

test("with key+cert serves over TLS", async () => {
const server = createHttpsServer({ key: tlsCert.key, cert: tlsCert.cert }, (_req, res) => res.end("ok"));
await once(server.listen(0), "listening");
const { port } = server.address() as import("node:net").AddressInfo;
try {
const res = await fetch(`https://127.0.0.1:${port}/`, { tls: { rejectUnauthorized: false } });
expect(await res.text()).toBe("ok");
} finally {
await new Promise<void>(r => server.close(() => r()));
}
});
});
2 changes: 2 additions & 0 deletions test/js/bun/http/serve.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2353,9 +2353,11 @@ describe.concurrent("should error with invalid options", async () => {
tls: [
{
key: "lkwejflkwjeflkj",
cert: "lkwejflkwjeflkj",
},
{
key: "lkwjefhwlkejfklwj",
cert: "lkwjefhwlkejfklwj",
},
],
});
Expand Down
Loading