fix(sql): don't treat URL pathname as Unix socket path#27714
fix(sql): don't treat URL pathname as Unix socket path#27714
Conversation
When parsing a Postgres/MySQL connection URL like `postgres://user:pass@host:5432/mydb`, the URL pathname `/mydb` (the database name) was incorrectly assigned to the `path` variable used for Unix domain socket connections. This caused the native code to attempt a Unix socket connection to `/mydb` instead of TCP, resulting in FailedToOpenSocket. Only use `url.pathname` as the socket path when the protocol is `unix://`. For all other protocols (postgres://, mysql://, etc.), the pathname is the database name and should not be used as a socket path. Fixes #27713 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
WalkthroughSelects Unix socket path only for Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/27713.test.ts`:
- Around line 122-131: The test "unix:// protocol should still use pathname as
socket path" uses a fixed socket path ("/tmp/bun-test-27713.sock") which can
collide across runs; change the setup in the test around the Bun.listen call and
the sock variable to generate a unique per-run socket path (e.g., use
os.tmpdir() + a unique suffix via process.pid/timestamp or crypto.randomUUID)
and pass that generated path to Bun.listen and to new SQL(`unix://${sock.unix}`)
(references: test name, variable sock, Bun.listen, and SQL constructor) and
ensure the test cleans up the socket after completion to avoid stale files.
- Around line 34-51: The afterAll cleanup in the test currently iterates over
and mutates all environment variables (using process.env, Bun.env,
import.meta.env and originalEnv), causing cross-test coupling; restrict
restoration/cleanup to only the SQL-related keys by using the SQL_ENV_VARS list:
in the afterAll handler, remove the loops that delete/restore every key and
instead iterate only over SQL_ENV_VARS to delete those keys from process.env,
Bun.env, and import.meta.env, and restore only keys present in originalEnv for
those same SQL_ENV_VARS (referencing afterAll, SQL_ENV_VARS, originalEnv,
process.env, Bun.env, import.meta.env to locate the changes).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/js/internal/sql/shared.tstest/regression/issue/27713.test.ts
- Restrict afterAll env cleanup to only SQL_ENV_VARS instead of wiping all environment variables, preventing cross-test coupling - Use unique per-run socket path with process.pid suffix to avoid collisions across parallel test runs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward fix correctly scoping url.pathname as a socket path to only unix:// protocol URLs, with solid regression test coverage.
Extended reasoning...
Overview
This PR fixes a bug where url.pathname (the database name in standard connection URLs like postgres://user:pass@host:5432/mydb) was being incorrectly used as a Unix domain socket path. The fix is a single conditional in src/js/internal/sql/shared.ts (lines 620-627) that only assigns url.pathname to path when url.protocol === "unix:". A new regression test file with 7 test cases covers postgres URLs, mysql URLs, DATABASE_URL env var, and the unix:// protocol.
Security risks
No security risks. The change is purely about correct connection routing — it prevents TCP connections from being misrouted as Unix socket connections, which was causing connection failures rather than any data exposure.
Level of scrutiny
This is production-critical code (SQL connection parsing), but the change is minimal and well-isolated. The before/after behavior is clear: previously path ||= options.path || url.pathname for all protocols; now url.pathname only feeds into path for unix: protocol. The logic is straightforward and the test coverage validates all the key scenarios.
Other factors
The one bug report filed is pre-existing — the database name fallback using url.pathname for unix:// URLs was not introduced by this PR and has limited practical impact since unix:// users typically specify database explicitly. The PR description is thorough, existing test suites pass, and the fix aligns with how other SQL client libraries handle URL parsing.
| // Only use url.pathname as the Unix socket path for unix:// URLs. | ||
| // For postgres://, mysql://, etc., the pathname is the database name (e.g. "/mydb"), | ||
| // not a socket path. Using it as the socket path causes FailedToOpenSocket (#27713). | ||
| if (url.protocol === "unix:") { | ||
| path ||= options.path || url.pathname; | ||
| } else { | ||
| path ||= options.path; | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing issue: For unix:// URLs, the database resolution fallback at line 732 still uses decodeIfValid((url?.pathname ?? "").slice(1)) without a unix: protocol guard. This means new SQL("unix:///var/run/postgresql", { adapter: "postgres" }) without an explicit database option would incorrectly set the database name to "var/run/postgresql". Consider adding the same url.protocol === "unix:" guard used for path (line 623) to the database fallback.
Extended reasoning...
What the bug is
The PR correctly fixes the path variable so that url.pathname is only used as a Unix socket path for unix:// protocol URLs (line 623). However, further down in the function, the database name resolution for postgres (line 732), mysql (line 743), and mariadb still uses decodeIfValid((url?.pathname ?? "").slice(1)) as a fallback for all protocols, including unix:.
Concrete example
Consider: new SQL("unix:///var/run/postgresql", { adapter: "postgres" })
- The URL is parsed:
url.protocol = "unix:",url.pathname = "/var/run/postgresql" - The PR correctly sets
path = "/var/run/postgresql"(the socket path) becauseurl.protocol === "unix:" - When resolving the database name at line 727-733, the fallback chain is evaluated:
options.databaseis undefinedoptions.dbis undefinedenv.PG_DATABASEis undefinedenv.PGDATABASEis undefineddecodeIfValid("/var/run/postgresql".slice(1))evaluates to"var/run/postgresql"— this is the bug
- The database name is set to
"var/run/postgresql", which is the socket path minus the leading slash, not a valid database name.
Why existing code does not prevent it
The url.protocol === "unix:" guard was only added to the path resolution (line 623), not to the database resolution (lines 725-745). The database fallback chain unconditionally reads url.pathname regardless of protocol.
Impact
In practice, most users of unix:// URLs will explicitly specify a database option, which short-circuits the fallback chain. However, if they omit it, they will get a confusing connection error about a non-existent database named after their socket path. The practical impact is limited but the inconsistency with the path fix is worth addressing.
How to fix
Add a similar url.protocol === "unix:" guard to the database resolution. For example, replace decodeIfValid((url?.pathname ?? "").slice(1)) with (url?.protocol !== "unix:" ? decodeIfValid((url?.pathname ?? "").slice(1)) : null) in all three adapter cases (postgres at line 732, mysql at line 743, mariadb).
Pre-existing status
This is a pre-existing issue. The database resolution code at lines 725-745 was not modified by this PR. The url.pathname fallback for database names has always applied to all protocols. However, since this PR makes unix:// URLs more functional by fixing the socket path issue, it creates a natural place to also address this related problem. As one verifier noted, the typical unix:// usage pattern involves explicitly specifying the database option, so the practical impact is limited.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — clean, targeted fix with good test coverage.
Extended reasoning...
Overview
This PR fixes a bug where url.pathname (the database name portion of a connection URL like postgres://user:pass@host:5432/mydb) was being incorrectly assigned as a Unix socket path. The fix is a single conditional change in src/js/internal/sql/shared.ts that only uses url.pathname as the socket path when url.protocol === "unix:". A new regression test file with 7 test cases validates the fix.
Security risks
No security risks. The change actually fixes a connectivity issue — it prevents the SQL driver from attempting Unix domain socket connections when TCP was intended. No auth, crypto, or permissions code is touched.
Level of scrutiny
This is production-critical code (SQL connection handling), but the change itself is minimal and mechanical — a single if/else guard around an existing assignment. The logic is straightforward: for unix: protocol, use url.pathname as socket path; for everything else, don't. The risk of regression is low and mitigated by the comprehensive test suite.
Other factors
- The CodeRabbit bot's nit comments (env cleanup scoping, unique socket path) were addressed in commit 3cd29a9
- My previous pre-existing issue comment about the database fallback for
unix:URLs is orthogonal to this fix and doesn't block it - Existing test suites (adapter-env-var-precedence, sqlite-url-parsing) reportedly pass
- The bug hunting system found no issues
1 participant
Summary
FailedToOpenSocketfor any standard Postgres/MySQL connection URLurl.pathnameas the socket path forunix://protocol URLs; forpostgres://,mysql://, etc., the pathname is the database name, not a socket pathRoot cause
In
src/js/internal/sql/shared.ts, the line:was extracting
url.pathname(e.g./mydbfrompostgres://user:pass@host:5432/mydb) and setting it as the Unix socket path. The native Zig code sees a non-emptypathand usesconnectUnixAnon()instead of TCP, resulting inFailedToOpenSocketorECONNREFUSEDwhen the database name happens to match an existing filesystem path (common in Docker/Kubernetes environments).Fix
Only use
url.pathnameas the socket path whenurl.protocol === "unix:". For all other protocols, the pathname is the database name.Test plan
test/regression/issue/27713.test.tswith 7 test casespathis not set from URL pathname for postgres/mysql URLsunix://protocol still correctly uses pathname as socket pathUSE_SYSTEM_BUN=1(system Bun 1.3.10) and passes with debug buildadapter-env-var-precedence.test.ts(38 tests) all passsqlite-url-parsing.test.ts(176 tests) all passCloses #27713
🤖 Generated with Claude Code