Conversation
Contributor
WalkthroughDefault SSL mode changed to "prefer". Option parsing now maps Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
Member
|
Thank you, this seems correct. I've unblocked the CI. |
S4N-T0S
marked this pull request as draft
January 28, 2026 09:17
S4N-T0S
marked this pull request as ready for review
January 28, 2026 10:29
![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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/sql/mysql/js/JSMySQLConnection.zig (1)
336-360: Allownull/undefinedTLS objects to use default empty TLS config.When a user sets
ssl_modebut omits thetlsoption, the JS layer passesnull(via thetls ?? nullcoalescing at postgres.ts:516, mysql.ts equivalent). The comments in postgres.ts:338 and mysql.ts:112 explicitly document that "boolean false or null => nothing" (i.e., use default TLS config), yet the current Zig code throws "tls must be a boolean or an object" on line 354.The fix should check
tls_object.isEmptyOrUndefinedOrNull()first and treat it the same as an explicit boolean true, consistent with the documented behavior:Suggested fix
if (ssl_mode != .disable) { tls_config = if (tls_object.isEmptyOrUndefinedOrNull() or (tls_object.isBoolean() and tls_object.toBoolean())) .{} else if (tls_object.isObject()) (jsc.API.ServerConfig.SSLConfig.fromJS(vm, globalObject, tls_object) catch return .zero) orelse .{} else { return globalObject.throwInvalidArguments("tls must be a boolean or an object", .{}); };src/sql/postgres/PostgresSQLConnection.zig (1)
594-618: Allow null/undefinedtlswhenssl_modeis enabled to support graceful SSL fallback.Lines 613–615 throw an error if
tlsis not a boolean or object. However, whentlsis omitted butssl_modeis set (e.g.,.requireor.prefer), the JavaScript side may passundefinedornullforarguments[6], causing an unnecessary error. Per the MySQL connector behavior,.requireand.prefershould gracefully degrade when SSL is unavailable; only.verify_caand.verify_fullshould reject non-SSL connections.Update the condition to accept null/undefined as a default (empty) TLS config:
🔧 Suggested fix
- tls_config = if (tls_object.isBoolean() and tls_object.toBoolean()) + tls_config = if (tls_object.isEmptyOrUndefinedOrNull()) + .{} + else if (tls_object.isBoolean() and tls_object.toBoolean()) .{} else if (tls_object.isObject()) (jsc.API.ServerConfig.SSLConfig.fromJS(vm, globalObject, tls_object) catch return .zero) orelse .{} else { return globalObject.throwInvalidArguments("tls must be a boolean or an object", .{}); };Apply the same fix to the MySQL connector at
src/sql/mysql/js/JSMySQLConnection.ziglines 354–360.
S4N-T0S
changed the title
S4N-T0S
marked this pull request as draft
January 28, 2026 17:48
…lt was disabled instead of prefer
Author
|
I apologise for the mess here, I have edited the main PR to reflect what has been changed. I was at first expecting this to be a straightforward fix but then I just found more and more inconsistencies and issues as I dug deeper. |
S4N-T0S
changed the title
S4N-T0S
marked this pull request as ready for review
January 28, 2026 23:21
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/sql/mysql/js/JSMySQLConnection.zig`:
- Around line 236-275: In onHandshake_ for JSMySQLConnection, when handling
.verify_full mode and calling bun.BoringSSL.c.SSL_get_servername, add a failure
path if SSL_get_servername returns null so hostname verification does not get
silently skipped; if servername is unavailable, call this.failWithJSValue with
an appropriate JS error (e.g., create an error like ssl_error.toJS or a new
error indicating missing server name) and return. Update the same logic in
MySQLConnection.zig's onHandshake_ as well (referencing .verify_full,
SSL_get_servername, checkServerIdentity, and failWithJSValue) so verify_full
consistently rejects connections when server name cannot be obtained.
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 424-447: In PostgresSQLConnection.zig update the .verify_full
branch to explicitly fail when the servername is missing and when
BoringSSL.checkServerIdentity returns false instead of skipping or reporting
ssl_error: detect when BoringSSL.c.SSL_get_servername returns null and call
this.failWithJSValue with a clear hostname-verification JS error (not
ssl_error), and when checkServerIdentity returns false similarly construct and
pass a descriptive JS error indicating hostname mismatch; use the existing
this.failWithJSValue and this.globalObject helpers to produce these dedicated
errors so verify_full never silently downgrades or emits the generic ssl_error.
cirospaciari
requested changes
Jan 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/internal/sql/shared.ts`:
- Around line 671-679: The current enforcement of rejectUnauthorized for sslMode
values SSLMode.verify_ca and SSLMode.verify_full runs before the code that may
convert a boolean tls into an object when injecting serverName, which can drop
rejectUnauthorized; update the logic in the module that handles tls/serverName
(references: sslMode, SSLMode.verify_ca, SSLMode.verify_full, tls, and the
serverName injection code) so that rejectUnauthorized = true is applied after
any conversion from boolean-to-object (or re-applied when you create the new tls
object), ensuring the final tls object always contains rejectUnauthorized: true.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/sql/mysql/js/JSMySQLConnection.zig`:
- Around line 242-278: The TLS verification in JSMySQLConnection.onHandshake_
duplicates logic already present in MySQLConnection.doHandshake; pick one place
to own verification to avoid redundancy — remove the duplicate verification
block from MySQLConnection.doHandshake and keep the full verification in
JSMySQLConnection.onHandshake_ (or vice versa if you prefer the core to own it),
but before removing confirm there are no other callers of
MySQLConnection.doHandshake that expect verification; update/remove any
now-unused helper functions/branches in MySQLConnection.doHandshake and run
related tests to ensure behavior remains the same.
In `@src/sql/mysql/MySQLConnection.zig`:
- Around line 237-238: The comment above the switch on this.#ssl_mode
incorrectly references the postgres library; update it to point to the correct
MySQL/TLS documentation (or a generic TLS mode reference) to avoid confusion.
Find the ssl mode handling in MySQLConnection (the switch on this.#ssl_mode) and
replace the postgres link with a link to MySQL's TLS/SSL docs (or an
RFC/OWASP/TLS-mode spec) and adjust the comment text to describe that the switch
implements MySQL client TLS modes.
…n.zig as it is in MySQLConnection.zig
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/sql/mysql/js/JSMySQLConnection.zig`:
- Around line 236-266: When onHandshake_ sees doHandshake return false with
ssl_error.error_no == 0 in verify_full mode and checkServerIdentity returns
true, add an explicit fallback error before falling through to failWithJSValue
to avoid an ambiguous message; locate the onHandshake_ function and after the
checkServerIdentity success path (the block using
bun.BoringSSL.checkServerIdentity) return a clear failFmt or fail call (e.g.,
using error.SslConnectionError) describing the inconsistent state (doHandshake
failed despite successful server identity verification) so that failWithJSValue
is not relied on to report an unclear zero-error SSL object.
…that it executes before the Enforce rejectUnauthorized block. Add fallthrough for edgecase in MySQL tls/ssl connection.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/js/sql/ssl-postgres-handshake.test.ts`:
- Around line 9-21: Replace the hardcoded/random PORT usage with an OS-assigned
port by setting Bun.listen's port to 0 in the test setup (the beforeAll block
that initializes server), then read the actual assigned port from the server
(e.g., server.port or server.address().port) and use that value wherever PORT is
referenced to build the connection URL; update any other occurrences (including
the later block around the other test) to stop using the PORT constant and
instead use the runtime-assigned port variable so tests are not flaky in CI.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/js/sql/ssl-postgres-handshake.test.ts`:
- Around line 21-102: The data(socket, data) handler is assuming each chunk is a
full frame, causing flaky parsing; fix it by implementing per-socket buffering
inside data(socket, data): append incoming chunk to a buffer, then loop parsing
as long as the buffer contains a full message; for messages that start with 0
(Startup/SSL) use the 4-byte length at offset 0 to know total size and read the
code at offset 4 (refer to SSL_REQUEST_CODE and PROTOCOL_V3_CODE handling in
data), for normal typed messages (e.g. 'P' (80), 'Q' (81), 'C', 'Z', 'X' etc.)
require at least 5 bytes to read the 4-byte length at offset 1 and wait until
buffer has that many bytes before consuming and responding, and only then slice
consumed bytes from the buffer and continue the loop; ensure the same response
construction logic (parseComplete, bindComplete, cmdComplete, ready, etc.) is
used when a full message is available.
…tai suggestion on handshake test.
…ed on reject_unauthorized)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/js/sql/ssl-postgres-behavior-verification.test.ts`:
- Line 86: Fix the typo in the test comment: replace "TSL" with "TLS" in the
comment that reads "// Depending on where the error is caught (TSL layer or
Postgres layer)," in ssl-postgres-behavior-verification.test.ts so it correctly
references the TLS layer.
- Around line 129-145: Add two tests to
ssl-postgres-behavior-verification.test.ts that cover the missing scenarios: one
that constructs a new SQL({...getBaseOptions(), ssl: "disable"}) and runs a
simple query to assert it connects without TLS (expecting SELECT 1 to return 1),
and another that constructs new SQL({...getBaseOptions(), tls: false}) and runs
the same simple query to assert TLS is explicitly disabled; place them alongside
the existing tls/ssl tests and use the same container.ready and using sql
patterns so they exercise the same connection lifecycle and prevent regressions
for the TypeError and prefer-default behavior fixes.
- Around line 92-127: The tests "ssl: 'verify-ca' throws without CA provided"
and "ssl: 'verify-full' throws on host mismatch/untrusted cert" only assert that
an error exists; update each test (the SQL client created with new SQL({...
getBaseOptions(), ssl: "verify-ca" }) and ssl: "verify-full") to assert the
error is a TLS verification error rather than a generic failure by checking
error.message or error.code for TLS-related indicators (e.g., message contains
"certificate", "self signed", "host", or codes like DEPTH_ZERO_SELF_SIGNED_CERT
or CERT_HAS_EXPIRED), so replace the loose expect(error).toBeDefined() with a
targeted assertion (e.g., expect(error.message).toMatch(/certificate|self
signed|host/i) or expect(error.code).toBeDefined() with the expected TLS code)
while keeping a fallback loose assertion if platform messages differ.
Contributor
✅ Actions performedFull review triggered. |
S4N-T0S
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/internal/sql/shared.ts`:
- Around line 660-668: The code currently overrides an explicit options.tls ===
false when sslMode is derived from URL/env; change the logic so that an explicit
options.tls === false is honored (or surfaces a conflict) before the defaulting
that sets tls = true: detect options.tls === false and either (a) set tls =
false and skip the later sslMode !== SSLMode.disable && !tls block, or (b) if
sslMode requires TLS (e.g., SSLMode.require), throw a clear error about the
conflicting settings; update the branches around sslMode, options.tls, and tls
(the variables named sslMode, options.tls, and tls) so explicit user TLS choice
wins or surfaces a conflict immediately.
…es from URL/env", corrected docs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-types/sql.d.ts (1)
326-336:⚠️ Potential issue | 🟡 MinorJSDoc defaults for
tls/sslare now misleading.Runtime defaults now favor
sslMode: "prefer"with TLS enabled when unspecified; the@default falseand “alias for tls” wording no longer matches behavior or the new mode strings.📝 Suggested doc update
- /** - * Whether to use TLS/SSL for the connection - * `@default` false - */ + /** + * TLS options or boolean toggle. If omitted, TLS is enabled by default + * when sslMode defaults to "prefer". + */ tls?: Bun.BunFile | TLSOptions | boolean | undefined; - /** - * Whether to use TLS/SSL for the connection (alias for tls) - * `@deprecated` Prefer {`@link` tls} - * `@default` false - */ + /** + * SSL mode string or TLS options. Supports "disable" | "prefer" | "require" | "verify-ca" | "verify-full". + * `@deprecated` Prefer {`@link` tls} for TLS options; use {`@link` ssl} for mode selection. + * `@default` "prefer" + */ ssl?: Bun.BunFile | TLSOptions | boolean | "disable" | "prefer" | "require" | "verify-ca" | "verify-full" | undefined;
🤖 Fix all issues with AI agents
In `@src/js/internal/sql/shared.ts`:
- Around line 640-646: The guard `if (options.ssl)` ignores a boolean false and
so doesn't apply the override; change the presence check to test for undefined
(e.g. `if (options.ssl !== undefined)` or `if ("ssl" in options)`) so
`options.ssl === false` is handled, then keep the existing branches that call
`normalizeSSLMode(options.ssl)` for string and set `sslMode = options.ssl ?
SSLMode.prefer : SSLMode.disable` for boolean to ensure false disables TLS;
update the block around the `options.ssl` check that surrounds normalizeSSLMode,
SSLMode.prefer and SSLMode.disable accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/internal/sql/shared.ts`:
- Around line 660-668: When options.tls === false, don't unconditionally set
sslMode = SSLMode.disable; instead detect if an explicit ssl mode was provided
(i.e., options.ssl / sslMode was set to a non-default value like "require",
"verify-ca", "verify-full") and throw an argument conflict error referencing
"tls" vs "ssl"/sslMode; only downgrade sslMode to disable and set tls = false
when sslMode is still the default (e.g., SSLMode.prefer or undefined). Update
the logic around options.tls, options.ssl, and the sslMode variable to enforce
this conflict handling.
…, throw error. Update comments, docs and test. +Ran the auto-formatter.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/js/internal/sql/shared.ts`:
- Around line 682-690: The hostname-to-SNI injection currently skips IP literals
(require("node:net").isIP(hostname)), which breaks verify_full because later
verification (SSL_get_servername/verify_full) expects a serverName; either allow
injection when sslMode === SSLMode.verify_full or change the verification path
to fall back to the original hostname when SSL_get_servername is missing. Update
the injection block around sslMode/SSLMode.disable and tls to set tls.serverName
= hostname even for IPs when sslMode === SSLMode.verify_full (or alternatively
modify the verification routine to use the provided hostname when
SSL_get_servername returns null), referencing the sslMode/SSLMode.disable check,
tls variable, hostname, require("node:net").isIP, and the
verify_full/SSL_get_servername flow.
In `@src/sql/mysql/js/JSMySQLConnection.zig`:
- Around line 294-295: The onData handler (JSMySQLConnection.onData) currently
drops data when this.#connection.isProcessingData() is true; instead, append
incoming chunks to an internal buffer/queue and defer processing until current
work completes: add a per-connection buffer or queue (e.g., this.#pending_reads
or reuse this.#read_buffer) to store data in onData whenever isProcessingData()
is true, and when readAndProcessData() or the processing routine finishes, drain
that queue into readAndProcessData() (or invoke the normal processing path) so
no bytes are lost; reference JSMySQLConnection.onData,
JSMySQLConnection.readAndProcessData(), this.#read_buffer, and
this.#connection.isProcessingData() when implementing the buffering and drain
logic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/internal/sql/shared.ts`:
- Around line 682-693: There's an extra closing brace causing a syntax error in
the SSL SNI/hostname injection block that uses sslMode, SSLMode, tls and
hostname; remove the stray `}` following that if-block so the braces balance
(ensure the conditional starting with `if (sslMode !== SSLMode.disable &&
!tls?.serverName && hostname) {` is closed exactly once and no additional `}`
remains).
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/sql/mysql/MySQLConnection.zig`:
- Around line 239-242: The current defer unconditionally resets
this.#flags.is_processing_data and can clear it when doHandshake runs nested;
modify the handshake entry to save the previous value (e.g., const prev =
this.#flags.is_processing_data), set this.#flags.is_processing_data = true only
if not already true, and in the defer restore it to prev (or only clear it if
prev was false). Apply this change around doHandshake and the surrounding write
block so re-entrant onData remains disabled unless the outer scope originally
left it enabled.
In `@test/js/sql/ssl-postgres-handshake.test.ts`:
- Around line 207-212: The test "Default (No Options) -> Prefer (SSLRequest ->
Fallback -> Startup)" contains a debug console.error(error) that pollutes CI
output; remove that call (or gate it behind a debug flag) inside the test so the
connect({}) result is still asserted but errors are not unconditionally logged.
Locate the test function and the connect invocation and either delete the
console.error(error) line or wrap it in a conditional debug check (e.g., if
(process.env.DEBUG) ...) around the console.error call.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 413-435: The code redundantly resets flags.is_processing_data and
re-checks read_buffer.remaining() after calling startTLS and start, which
duplicates the outer defer logic; remove the explicit resets and onData calls
that follow this.startTLS(socket) and this.start() so the single defer (which
clears is_processing_data and invokes onData when needed) handles cleanup and
buffered-data processing; ensure you keep the early return after
this.startTLS(socket) but do not reset flags.is_processing_data or call
this.onData("") there—let the defer perform those actions for consistency with
the initial re-entrancy protection.
In `@test/js/sql/ssl-postgres-handshake.test.ts`:
- Around line 244-248: The test "TLS: true -> Acts as Prefer (Defaults)" lacks
an inline comment explaining why tls: true is expected to behave like SSL mode
"prefer"; add a short clarifying comment above the test or inside it stating
that when tls is set to true without an explicit sslMode the client defaults to
"prefer" behavior, so connect({ tls: true }) should emit ["SSLRequest",
"StartupMessage", "Query"] — reference the test block name, the connect function
call (connect({ tls: true })), and the events assertion
(expect(events).toEqual([...])) so readers understand the expected handshake
sequence.
S4N-T0S
commented
Feb 1, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/js/internal/sql/shared.ts`:
- Around line 681-690: The serverName injection currently runs before hostname
defaults are applied, so when hostname falls back to a default (e.g.,
"localhost") serverName can remain unset and SSLMode.verify_full fails; move the
block that sets tls.serverName (the logic referencing sslMode,
SSLMode.verify_full, tls, and hostname) to after the code that resolves/defaults
hostname (or call the same injection logic again immediately after hostname is
resolved) so serverName is always populated when sslMode !== SSLMode.disable and
tls lacks serverName.
In `@src/sql/mysql/js/JSMySQLConnection.zig`:
- Around line 246-270: The code path handling a failed handshake in
JSMySQLConnection checks handshakeWasSuccessful and ultimately calls
this.failWithJSValue(ssl_error.toJS(...)) but does not return, allowing
subsequent processing on a closed/failed socket; update the logic in the
handshake failure block (the branch using ssl_error and calling
this.failWithJSValue) to return immediately after the this.failWithJSValue(...)
call to stop further post-failure processing and avoid using a failed
connection.
Contributor
✅ Actions performedFull review triggered. |
Author
|
@alii @cirospaciari Please let me know what to do with this pull request, Bun without this pull request or other changes uses plaintext by default on communications with databases which is unheard of. |
This branch has not been deployed
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
3 participants
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?
This PR fixes several inconsistencies and bugs regarding SSL/TLS configuration in
Bun.SQL. It changes the default SSL mode fromdisabletoprefer(aligning with standard Postgres clients and existing code comments), ensures thatverify-caandverify-fullare correctly implemented in the native code for both Postgres and MySQL, and fixes option parsing bugs inshared.ts.Specifically, it:
sslmode toprefer(tries SSL, falls back to plaintext).TypeErrorthat occurred when settingssl: 'disable'without atlsobject.tls: falsebehavior:sslmode is provided,tls: falseoverrides the defaultprefertodisable.sslmode is provided (e.g.require), passingtls: falsethrows a validation error to prevent accidental security downgrades.PostgresSQLConnection.zigandJSMySQLConnection.zigto distinguish betweenverify-ca(cert chain check only) andverify-full(cert chain + hostname check).What is the issue?
There were multiple issues with the previous SSL implementation:
preferwas the default, the code actually defaulted to plaintext (disable).{ ssl: 'disable' }caused aTypeError: tls must be a boolean or an objectbecause the string value was incorrectly being assigned to thetlsconfiguration.tls: falsedid not consistently disable SSL if other defaults were at play.verify-caandverify-fulllogic to ensure strict verification modes behaved as expected (e.g.,verify-cashouldn't enforce hostname validation, butverify-fullmust).How did you verify your code works?
I included test scripts and tested all connection modes for Postgresql.