fix(tls): enforce rejectUnauthorized for mTLS in Bun.serve()#28003
Open
jdragulanescu wants to merge 7 commits intooven-sh:mainfrom
Open
fix(tls): enforce rejectUnauthorized for mTLS in Bun.serve()#28003jdragulanescu wants to merge 7 commits intooven-sh:mainfrom
jdragulanescu wants to merge 7 commits intooven-sh:mainfrom
Conversation
Bun.serve() with rejectUnauthorized: true was accepting rogue client certificates because us_verify_callback always returns 1. Add is_server flag to SSL context options and use a server-side verify callback that returns preverify_ok as-is, letting OpenSSL enforce cert verification natively during the handshake. Only applied to Bun.serve() — Bun.listen() and tls.createServer() handle verification post-handshake in JS. Fixes oven-sh#27985
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/27985.test.ts`:
- Around line 91-97: The test "deterministically rejects 10 rogue requests" is
too small a sample; increase the loop iterations from 10 to 30 to reduce
false-pass probability. Update the test that calls createMTLSServer(true) and
the for loop that calls connectAndRequest(server.port, { key: rogueClientKey,
cert: rogueClientCert }) so it runs 30 iterations (e.g., for (let i = 0; i < 30;
i++)) and keep the same expect(result.type).toBe("rejected") assertion and
logging.
- Around line 23-35: The test's connectAndRequest function never sets the TLS
servername so SNI code paths are not exercised; update connectAndRequest to
accept an optional servername parameter and pass it into the tls.connect options
(e.g., include servername: servername) and then update the SNI-specific test
calls to pass "localhost" as that argument so the serverName: "localhost" TLS
config and addServerNameWithOptions() changes are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79f1f6bd-7e3b-463d-b106-2bf52a64c0ec
📒 Files selected for processing (6)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/libusockets.hpackages/bun-uws/src/App.hsrc/bun.js/api/server.zigsrc/deps/uws/SocketContext.zigtest/regression/issue/27985.test.ts
- Pass servername to tls.connect() in SNI tests to actually exercise SNI - Increase deterministic rejection test from 10 to 30 iterations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/27985.test.ts`:
- Around line 139-150: The test "fetch() with rogue client cert is rejected by
mTLS server" currently only asserts err is defined which is too broad; update
the catch block to assert the error indicates a TLS/certificate rejection from
the mTLS server by inspecting err.code and/or err.message (when calling fetch
with tls: { key: rogueClientKey, cert: rogueClientCert, ca, rejectUnauthorized:
false } against createMTLSServer(true)). Concretely, replace
expect(err).toBeDefined() with assertions checking err.code equals a TLS-related
code (if available) or that err.message includes certificate/TLS keywords like
"certificate", "TLS", "handshake", or "unauthorized" to ensure the failure was
due to certificate rejection rather than an unrelated network error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b941fc4-5442-49f7-bc87-a02946203f00
📒 Files selected for processing (1)
test/regression/issue/27985.test.ts
Tighten the assertion to verify the error is actually a TLS/certificate rejection rather than an unrelated failure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/27985.test.ts`:
- Around line 23-56: connectAndRequest currently can't distinguish a socket that
completed the TLS handshake from one rejected during handshake; add a boolean
secureConnected inside connectAndRequest, set it to true on the socket's
"secureConnect" event (or when socket.authorized is true), and include that flag
in the resolved TLSResult for both accepted and rejected cases so callers/tests
can assert whether secureConnect happened; update the function
signature/returned TLSResult shape and all places that build the result in
done() (and in the "end", "error", and "close" handlers) to return { type: ...,
status?, error?, secureConnected } so rejection cases can assert secureConnected
is false.
- Around line 103-123: The SNI code path isn't exercised because the
single-entry tls array becomes the base context; update the test in
issue/27985.test.ts so Bun.serve is given two TLS entries: a permissive base
context (e.g., requestCert: false or rejectUnauthorized: false) and a separate
SNI entry with serverName: "localhost" and strict mTLS (requestCert: true,
rejectUnauthorized: true) so the SNI branch runs and addServerNameWithOptions is
used; then call connectAndRequest with the SNI servername ("localhost") to
validate acceptance/rejection, or alternatively add a control connection without
a servername to assert the different routing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1c411f9-b3d8-4456-abed-4e8c5167b2c1
📒 Files selected for processing (1)
test/regression/issue/27985.test.ts
Comment on lines
+23
to
+56
| function connectAndRequest(port: number, clientCert?: { key: string; cert: string }, servername?: string): Promise<TLSResult> { | ||
| const { promise, resolve } = Promise.withResolvers<TLSResult>(); | ||
| let resolved = false; | ||
| let data = ""; | ||
|
|
||
| const done = (result: TLSResult) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| socket.destroy(); | ||
| resolve(result); | ||
| }; | ||
|
|
||
| const socket = tls.connect({ host: "localhost", port, servername, ca, key: clientCert?.key, cert: clientCert?.cert, checkServerIdentity: () => undefined }, () => { | ||
| socket.write("GET / HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n"); | ||
| }); | ||
|
|
||
| socket.on("data", (chunk: Buffer) => { data += chunk.toString(); }); | ||
|
|
||
| socket.on("end", () => { | ||
| const status = data.match(/HTTP\/1\.1 (\d+)/)?.[1]; | ||
| done(status ? { type: "accepted", status } : { type: "rejected", error: "closed without response" }); | ||
| }); | ||
|
|
||
| socket.on("error", (err: any) => { | ||
| done({ type: "rejected", error: err.code ?? err.message }); | ||
| }); | ||
|
|
||
| socket.on("close", () => { | ||
| const status = data.match(/HTTP\/1\.1 (\d+)/)?.[1]; | ||
| done(status ? { type: "accepted", status } : { type: "rejected", error: "closed" }); | ||
| }); | ||
|
|
||
| return promise; | ||
| } |
Contributor
There was a problem hiding this comment.
Record whether secureConnect happened before treating the socket as rejected.
tls.connect(..., callback) only invokes that callback after the TLS handshake succeeds. Right now a rogue/no-cert connection that handshakes successfully and is closed later in JS looks identical to an in-handshake rejection, so these tests still pass on the weaker pre-fix behavior. Return a secureConnected flag from connectAndRequest() and assert it stays false in the rejection cases.
Minimal helper change
type TLSResult =
| { type: "accepted"; status: string }
- | { type: "rejected"; error: string };
+ | { type: "rejected"; error: string; secureConnected: boolean };
function connectAndRequest(port: number, clientCert?: { key: string; cert: string }, servername?: string): Promise<TLSResult> {
const { promise, resolve } = Promise.withResolvers<TLSResult>();
let resolved = false;
let data = "";
+ let secureConnected = false;
const done = (result: TLSResult) => {
if (resolved) return;
resolved = true;
socket.destroy();
resolve(result);
};
const socket = tls.connect({ host: "localhost", port, servername, ca, key: clientCert?.key, cert: clientCert?.cert, checkServerIdentity: () => undefined }, () => {
+ secureConnected = true;
socket.write("GET / HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n");
});
@@
socket.on("end", () => {
const status = data.match(/HTTP\/1\.1 (\d+)/)?.[1];
- done(status ? { type: "accepted", status } : { type: "rejected", error: "closed without response" });
+ done(status ? { type: "accepted", status } : { type: "rejected", error: "closed without response", secureConnected });
});
socket.on("error", (err: any) => {
- done({ type: "rejected", error: err.code ?? err.message });
+ done({ type: "rejected", error: err.code ?? err.message, secureConnected });
});
socket.on("close", () => {
const status = data.match(/HTTP\/1\.1 (\d+)/)?.[1];
- done(status ? { type: "accepted", status } : { type: "rejected", error: "closed" });
+ done(status ? { type: "accepted", status } : { type: "rejected", error: "closed", secureConnected });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/27985.test.ts` around lines 23 - 56, connectAndRequest
currently can't distinguish a socket that completed the TLS handshake from one
rejected during handshake; add a boolean secureConnected inside
connectAndRequest, set it to true on the socket's "secureConnect" event (or when
socket.authorized is true), and include that flag in the resolved TLSResult for
both accepted and rejected cases so callers/tests can assert whether
secureConnect happened; update the function signature/returned TLSResult shape
and all places that build the result in done() (and in the "end", "error", and
"close" handlers) to return { type: ..., status?, error?, secureConnected } so
rejection cases can assert secureConnected is false.
Author
There was a problem hiding this comment.
Added secureConnected tracking to the helper — it is now logged in the JSON output for diagnostics but not asserted on.
Bun fires the tls.connect callback (setting secureConnected = true) before processing the server's certificate rejection alert. Verified with openssl s_client against the patched server:
Rogue cert → tlsv1 alert unknown ca (alert 48) — proper TLS-level rejection, no HTTP response received.
Valid cert → HTTP/1.1 response — accepted and served.
So the server does reject at the TLS level with a proper alert. The secureConnected: true on rejected connections is a client-side event timing quirk in Bun's tls.connect — the callback fires before the alert tears down the connection. This is a separate minor issue, not a security concern for this fix.
Contributor
There was a problem hiding this comment.
@jdragulanescu, thanks for the thorough investigation. The openssl s_client trace confirms the fix is sound — alert 48 (unknown ca) is issued at handshake time, so the server-side enforcement is working exactly as intended. The secureConnected timing quirk is purely a client-side event ordering artifact; it has no bearing on whether the server correctly rejects the handshake.
The diagnostic logging approach is a good compromise: it preserves the observability you want without encoding a flaky assertion that would only add noise. Happy with this resolution.
Would you like me to open a follow-up issue to track the tls.connect callback timing quirk (callback firing before the rejection alert is processed), so it's not lost?
(ᐢ•ᴗ•ᐢ) ~noting this for future reviews
✏️ Learnings added
Learnt from: jdragulanescu
Repo: oven-sh/bun PR: 28003
File: test/regression/issue/27985.test.ts:26-61
Timestamp: 2026-03-11T11:45:31.057Z
Learning: In Bun's `tls.connect`, the secure-connect callback (and the `secureConnect` event) may fire before the server's TLS rejection alert (e.g., `tlsv1 alert unknown ca`, alert 48) tears down the connection. This means `secureConnected` can be `true` even for connections that are ultimately rejected at the TLS level by the server. Do not assert `secureConnected === false` in mTLS rejection tests in the Bun codebase — use the absence of an HTTP response or the presence of a TLS error/close as the reliable signal instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T02:09:00.161Z
Learning: Applies to **/*.test.ts : Always use `port: 0` in tests; do not hardcode ports or use custom random port functions
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-22T12:23:39.726Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Never use hardcoded port numbers in tests. Always use `port: 0` to get a random port.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T02:09:00.161Z
Learning: Applies to test/js/bun/**/*.test.ts : For API-specific tests (http, crypto, ffi, shell, etc.), place tests in `test/js/bun/`
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 27385
File: test/js/bun/http/tls-keepalive.test.ts:115-140
Timestamp: 2026-02-24T21:02:00.725Z
Learning: In Bun's test suites, avoid adding tests for trivial environment/fixture script validation (e.g., checking if env vars exist) within test fixtures. Focus test coverage on actual behavior being tested (e.g., TLS keepalive, memory leaks) rather than auxiliary fixture validation. If a test file is primarily for fixtures, skip or limit tests that validate simple JS behavior like if (!env) throw; prioritize meaningful end-to-end or unit behavior instead.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-22T12:23:39.726Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `await using` or `using` to ensure proper resource cleanup for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T02:09:00.161Z
Learning: Applies to **/*.test.ts : In tests, expect stdout before exit code: expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22378
File: src/sql/mysql/MySQLConnection.zig:0-0
Timestamp: 2025-09-03T22:12:26.870Z
Learning: In Bun's MySQL SSL implementation, the .require SSL mode is designed to behave the same as .prefer for postgres.js compatibility, allowing graceful fallback to non-SSL connections when the server doesn't support SSL. Only .verify_ca and .verify_full modes will fail when SSL is not available.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-22T12:23:39.726Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:25.185Z
Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.
Learnt from: robobun
Repo: oven-sh/bun PR: 27532
File: test/regression/issue/06743.test.ts:71-71
Timestamp: 2026-02-28T09:33:44.119Z
Learning: In IPC-related tests that spawn parent/child processes (e.g., test/regression/issue/06743.test.ts), set explicit test timeouts (e.g., 15000ms) because the default 5000ms timeout is often insufficient for process spawning and inter-process communication, particularly when using advanced serialization mode.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:21:42.189Z
Learning: In Bun's test runner, describe.concurrent is supported (since Bun v1.2.23). Use describe.concurrent/test.concurrent for concurrent tests. Be aware of limitations: expect.assertions() and expect.hasAssertions() are not supported; toMatchSnapshot() is not supported (toMatchInlineSnapshot() is); and beforeAll/afterAll hooks are not executed concurrently. The broader guideline to prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent remains valid and should be applied to test files such as test/cli/run/concurrency-filter.test.ts and similar test files.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:22:55.570Z
Learning: In test/cli/run/concurrency-filter.test.ts and similar test files, timing-sensitive tests that assert on wall-clock elapsed time to verify concurrency behavior (e.g., expect(elapsed).toBeGreaterThan(800)) must remain in a sequential describe block rather than describe.concurrent. Running such tests concurrently can cause CPU contention and skew timing assertions, leading to flaky results. The guideline to prefer describe.concurrent does NOT apply for timing-based correctness verification.
test/regression/issue/27985.test.ts
Outdated
Comment on lines
+103
to
+123
| await using server = Bun.serve({ | ||
| port: 0, | ||
| tls: [ | ||
| { key: serverKey, cert: serverCert, ca, requestCert: true, rejectUnauthorized: true, serverName: "localhost" }, | ||
| ], | ||
| fetch: () => new Response("OK"), | ||
| }); | ||
| const result = await connectAndRequest(server.port, { key: rogueClientKey, cert: rogueClientCert }, "localhost"); | ||
| console.log(" >>", JSON.stringify(result)); | ||
| expect(result.type).toBe("rejected"); | ||
| }); | ||
|
|
||
| test("accepts valid client cert on SNI route", async () => { | ||
| await using server = Bun.serve({ | ||
| port: 0, | ||
| tls: [ | ||
| { key: serverKey, cert: serverCert, ca, requestCert: true, rejectUnauthorized: true, serverName: "localhost" }, | ||
| ], | ||
| fetch: () => new Response("OK"), | ||
| }); | ||
| const result = await connectAndRequest(server.port, { key: validClientKey, cert: validClientCert }, "localhost"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the test file to understand its full context
fd -t f "27985.test.ts" --exec wc -l {} \;Repository: oven-sh/bun
Length of output: 96
🏁 Script executed:
# Read the entire test file
cat -n test/regression/issue/27985.test.tsRepository: oven-sh/bun
Length of output: 8399
🏁 Script executed:
# Also check the Zig server file mentioned in the review
fd -t f "server.zig" -path "*/bun.js/api/*" --exec wc -l {} \;Repository: oven-sh/bun
Length of output: 223
🏁 Script executed:
# Find the server.zig file in bun.js/api
find . -path "*/bun.js/api/server.zig" -type fRepository: oven-sh/bun
Length of output: 82
🏁 Script executed:
# Read the relevant section of server.zig around line 2787-2810
sed -n '2780,2820p' src/bun.js/api/server.zigRepository: oven-sh/bun
Length of output: 1882
🏁 Script executed:
# Also search for how TLS is initialized when only SNI entries are provided
grep -n "addServerNameWithOptions\|sni_opts.is_server\|base.*context\|default.*tls" src/bun.js/api/server.zig | head -30Repository: oven-sh/bun
Length of output: 299
🏁 Script executed:
# Search for where the base/default app context is created before SNI routes are added
sed -n '2700,2850p' src/bun.js/api/server.zigRepository: oven-sh/bun
Length of output: 6408
🏁 Script executed:
# Check if there's a default TLS context created when config.tls is null/empty
grep -n "config.tls\|default.*app\|App.create" src/bun.js/api/server.zig | grep -A5 -B5 "2700\|2750\|2800"Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Get a broader view of the TLS initialization logic
sed -n '2650,2750p' src/bun.js/api/server.zigRepository: oven-sh/bun
Length of output: 4960
🏁 Script executed:
# Check how the config distinguishes between tls (base) and sni array
grep -n "config.tls\|config.sni\|config.ssl_config" src/bun.js/api/server.zig | head -20Repository: oven-sh/bun
Length of output: 216
🏁 Script executed:
# Look for where ssl_config is populated from tls config
grep -n "ssl_config\|config\.tls" src/bun.js/api/server.zig | grep -i "sni\|base\|default" | head -30Repository: oven-sh/bun
Length of output: 304
🏁 Script executed:
# Search for how config is initialized from JS (BunServeOptions)
fd -t f ".*server.*" src/bun.js/ | grep -E "\.ts$|\.zig$" | head -10Repository: oven-sh/bun
Length of output: 344
🏁 Script executed:
# Look at the actual config type and how it's initialized
grep -B5 -A10 "struct.*config\|pub.*config:" src/bun.js/api/server.zig | head -50Repository: oven-sh/bun
Length of output: 1431
🏁 Script executed:
# Read the ServerConfig.zig to understand how tls and sni are structured
cat -n src/bun.js/api/server/ServerConfig.zig | head -200Repository: oven-sh/bun
Length of output: 7865
🏁 Script executed:
# Check if there's a default/base app created when only SNI entries are provided
grep -n "ssl_config\|tls\|sni" src/bun.js/api/server/ServerConfig.zig | head -40Repository: oven-sh/bun
Length of output: 2541
🏁 Script executed:
# Check how the config parsing works when there are multiple tls entries
sed -n '909,945p' src/bun.js/api/server/ServerConfig.zigRepository: oven-sh/bun
Length of output: 1849
🏁 Script executed:
# Look more closely at the logic for when the first tls entry is selected vs SNI
sed -n '920,940p' src/bun.js/api/server/ServerConfig.zigRepository: oven-sh/bun
Length of output: 1086
🏁 Script executed:
# Now verify: in the test, when only one tls entry with serverName is provided,
# does it become ssl_config (base) or sni?
# Let's trace through the logic: if tls is an array with 1 item
python3 << 'EOF'
# Simulating the logic from ServerConfig.zig lines 912-938:
# if (tls.jsType().isArray()) {
# for (value_iter) |item| {
# if (args.ssl_config == null) {
# args.ssl_config = ssl_config; // FIRST item goes here
# } else {
# if ((ssl_config.server_name orelse "")[0] == 0) {
# return error; // SNI items must have serverName
# }
# if (args.sni == null) {
# args.sni = BabyList.initCapacity(...);
# }
# args.sni.append(ssl_config); // SUBSEQUENT items go here
# }
# }
# }
# Test case scenario: tls array with 1 item that has serverName: "localhost"
print("Test scenario: tls array with 1 entry having serverName='localhost'")
print()
print("Processing:")
print(" Item 1: serverName='localhost'")
print(" -> args.ssl_config == null? YES")
print(" -> args.ssl_config = Item1 (becomes BASE context)")
print(" -> No more items")
print()
print("Result:")
print(" Base context (ssl_config): serverName='localhost', strict mTLS")
print(" SNI entries (sni): EMPTY")
print()
print("When client connects with servername='localhost':")
print(" -> Matches BASE context")
print(" -> No SNI routing happens")
print(" -> addServerNameWithOptions() is NOT called for the matched route")
EOFRepository: oven-sh/bun
Length of output: 554
🏁 Script executed:
# In server.zig, after the base app is created, check if SNI routing is applied
sed -n '2730,2810p' src/bun.js/api/server.zig | grep -A30 "apply SNI"Repository: oven-sh/bun
Length of output: 1292
🏁 Script executed:
# Check if there are any other SNI-related tests beyond lines 102-126
grep -n "sni\|SNI\|serverName" test/regression/issue/27985.test.tsRepository: oven-sh/bun
Length of output: 511
🏁 Script executed:
# Also verify the test file structure around the SNI tests to see if there's any base context differentiation
sed -n '100,128p' test/regression/issue/27985.test.tsRepository: oven-sh/bun
Length of output: 1212
SNI tests don't exercise the SNI code path.
The single-entry tls array at lines 106 and 119 parses as a base context, not an SNI entry. Per ServerConfig.zig:927–938, the first tls array item becomes args.ssl_config (base), leaving args.sni null. Since server.zig:2787 only applies SNI routes if (this.config.sni), the addServerNameWithOptions() call at line 2793 never executes for these tests.
Both cert rejection and acceptance are handled by the base context (line 2761–2765). The test would pass identically if the SNI code had bugs.
To validate SNI routing, either:
- Create a base context with permissive mTLS and an SNI entry with strict mTLS for
"localhost", or - Add a control test connecting without servername to confirm routing differences.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/27985.test.ts` around lines 103 - 123, The SNI code
path isn't exercised because the single-entry tls array becomes the base
context; update the test in issue/27985.test.ts so Bun.serve is given two TLS
entries: a permissive base context (e.g., requestCert: false or
rejectUnauthorized: false) and a separate SNI entry with serverName: "localhost"
and strict mTLS (requestCert: true, rejectUnauthorized: true) so the SNI branch
runs and addServerNameWithOptions is used; then call connectAndRequest with the
SNI servername ("localhost") to validate acceptance/rejection, or alternatively
add a control connection without a servername to assert the different routing
behavior.
The single-entry tls array was parsed as a base context, not an SNI entry, so the SNI code path was never exercised. Replace with a test that correctly validates mTLS via tls array configuration.
Add secureConnected tracking to connectAndRequest helper to distinguish handshake-level vs application-level rejections in test output. The flag is logged but not asserted on, as Bun's TLS implementation fires the connect callback before processing the server's certificate rejection alert.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/27985.test.ts (1)
106-131:⚠️ Potential issue | 🟠 MajorThese tests validate array syntax but don't exercise the SNI code path.
Per
ServerConfig.zig:927–938, a single-entry tls array withoutserverNamebecomes the base context (args.ssl_config), leavingargs.sninull. TheaddServerNameWithOptions()changes in this PR only execute whenconfig.sniis populated.To properly test SNI routing with the mTLS fix, provide at least two TLS entries—one base and one with
serverName:♻️ Suggested approach to test SNI code path
test("rejects rogue client cert via SNI route", async () => { await using server = Bun.serve({ port: 0, tls: [ // Base context (permissive) { key: serverKey, cert: serverCert, ca, requestCert: false }, // SNI route (strict mTLS) { key: serverKey, cert: serverCert, ca, requestCert: true, rejectUnauthorized: true, serverName: "localhost" }, ], fetch: () => new Response("OK"), }); // Connect with SNI to hit the strict route const result = await connectAndRequest(server.port, { key: rogueClientKey, cert: rogueClientCert }, "localhost"); expect(result.type).toBe("rejected"); });,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/27985.test.ts` around lines 106 - 131, The tests use a single-entry tls array so the SNI path isn't exercised; change the test cases (the Bun.serve tls arrays and the connectAndRequest calls) to include two TLS entries—a permissive base entry (requestCert: false) and a strict SNI entry with serverName set, requestCert: true and rejectUnauthorized: true—then call connectAndRequest with the SNI hostname (e.g., "localhost") so the addServerNameWithOptions/SNI routing path is hit and the mTLS behavior is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/27985.test.ts`:
- Around line 106-131: The tests use a single-entry tls array so the SNI path
isn't exercised; change the test cases (the Bun.serve tls arrays and the
connectAndRequest calls) to include two TLS entries—a permissive base entry
(requestCert: false) and a strict SNI entry with serverName set, requestCert:
true and rejectUnauthorized: true—then call connectAndRequest with the SNI
hostname (e.g., "localhost") so the addServerNameWithOptions/SNI routing path is
hit and the mTLS behavior is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 507f4dac-0439-4854-b182-70652339b921
📒 Files selected for processing (1)
test/regression/issue/27985.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #27985.
Bun.serve()withrejectUnauthorized: truewas accepting rogue client certificates (HTTP 200) instead of rejecting them during the TLS handshake.Root cause
us_verify_callbackin openssl.c always returns 1, which tells OpenSSL to continue the handshake regardless of certificate verification failures. This was designed for client-side TLS (which verifies post-handshake via thehandshakecallback in net.ts), but breaks server-side mTLS inBun.serve()where there is no post-handshake rejection mechanism.Fix
is_serverfield to the SSL context options structs (C, C++, Zig)us_verify_callback_server_rejectthat returnspreverify_okas-is, letting OpenSSL enforce cert verification natively during the handshakeis_server=1only atBun.serve()call sites (main context + SNI routes)Bun.listen()/tls.createServer()are unchanged — they handle verification post-handshake in the JavaScripthandshakecallbackHow did you verify your code works?
test/regression/issue/27985.test.ts— 11 tests covering:rejectUnauthorized: true— accepts valid cert, rejects rogue cert, rejects no cert, deterministic rejection (10x)rejectUnauthorized: false— accepts all cert typesfetch()mTLS — valid cert succeeds, rogue cert rejected by servertest/regression/issue/26125.test.ts— 1 pass (mTLS cert switching, no regression)test/js/node/tls/node-tls-cert.test.ts— 25 pass, 0 fail (no regression)