Repository navigation
Conversation
…t over TCP
fetch(url, { unix }) only used the option when it was a non-empty JS
string. A Buffer, URL, number, or object was skipped, so the request
went to the URL's host over TCP with no error. A truthy non-string now
rejects with TypeError ERR_INVALID_ARG_TYPE, like Bun.connect and
Bun.listen. Falsy values still mean no unix socket.
|
Status Reproduced on 1.4.3-canary ( Fail-before: CI (build 111927): the diff is green on every platform, including the new tests on Windows and macOS. The two red jobs do not touch it: one darwin aarch64 shard timed out downloading the build artifact before any test ran ( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. Walkthrough
ChangesUnix fetch option validation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Fetch now rejects truthy non-string unix options rather than silently falling back to TCP, while falsy values retain TCP routing. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — tightens a silent-misroute into a loud ERR_INVALID_ARG_TYPE, no change to previously-valid inputs.
What was reviewed:
- The new branch in
fetch.rs:to_boolean()is the non-throwing JS ToBoolean (safe on Symbol),determine_specific_typeandto_bun_stringpropagate with?, and thethrow_invalid_argumentsshape matches the neighboringprotocolblock. - Falsy handling:
""/null/undefined/false/0all fall through to "no unix socket" as before, matchingBun.connect'sString.nullable.loose; only truthy non-strings now reject. - Tests: added to the existing
fetch.unix.test.ts,port: 0+usingcleanup, full type matrix, exact class/code/message asserted, andrequests === 0proves no TCP dial happened.
Extended reasoning...
Overview
This PR changes option validation for the Bun-specific unix extension to fetch(). In src/runtime/webcore/fetch.rs, the extract_unix_socket_path block previously accepted only is_string() && length > 0 and silently fell through for anything else, causing a request intended for a unix socket to be dialed over TCP to the URL's host. The new code keeps the string path unchanged, but any truthy non-string now returns Err(throw_invalid_arguments(...)), which reject_on_exception turns into a rejected promise. Falsy values remain "absent". Two tests in test/js/web/fetch/fetch.unix.test.ts cover the reject matrix (Buffer, Uint8Array, URL, {toString}, number, boolean, Symbol) with a zero-request assertion, and the falsy passthrough.
Security risks
The change is security-positive: it closes a silent transport misroute where a caller passing Buffer.from(path) would unknowingly send the request over TCP to the URL host. No new input is accepted, no new coercion runs on user values (the added calls are to_boolean() which never enters user JS, and determine_specific_type for the error message). No auth, crypto, or permission surface is touched.
Level of scrutiny
Low-to-moderate. The native diff is ~12 lines inside an existing labeled block, structurally identical to the protocol validation immediately below it (same throw_invalid_arguments(format_args!(...)) pattern). Exception discipline is correct: get/get_length/to_bun_string/determine_specific_type all carry ?, and to_boolean() is confirmed non-fallible in src/jsc/JSValue.rs. No CODEOWNERS entry covers these paths.
Other factors
Tests follow repo conventions closely: existing module test file, port: 0, using for server lifetime, single .toEqual on a results object, exact error class + .code + message asserted, negative contract via request counter, no sleeps or external network. The hunt exited on dry_streak with no findings and no ruled-out candidates. The PR timeline shows only the initial commit — no prior reviews or outstanding objections.
|
Updated 9:41 PM PT - Sep 6th, 2026
❌ @robobun, your commit 2460474 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41786That installs a local version of the PR into your bun-41786 --bun |
Problem
fetch(url, { unix })ignoresunixwhen it is not a JS string. ABuffer,Uint8Array,URL, number,true,Symbol, or an object withtoStringall send the request to the URL's host over TCP. There is no error and no coercion.unixextraction infetch_impl(src/runtime/webcore/fetch.rs:690). It takes the option only whenis_string() && length > 0and falls through for everything else.Fix
unixnow rejects withTypeError(ERR_INVALID_ARG_TYPE):fetch: 'unix' must be a string. Received an instance of Buffer. No request is sent."",null,undefined,false,0) still mean "no unix socket". This matchesBun.connectandBun.listen, whoseunixmember isString.nullable.loose: falsy is absent, a truthy non-string throwsSocketOptions.unix must be a string.fetch_implerrors become rejected promises throughreject_on_exception, sofetch()still never throws synchronously.test/js/web/fetch/fetch.unix.test.ts(two new tests, stock bun fails the first). Alsotest/js/web/fetch/fetch-args.test.ts.Background
unixis a Bun extension tofetchinit. It dials a unix domain socket and sends the request forurlover it. The docs (docs/runtime/networking/fetch.mdx) give its type asunix: string.fetch_implreads each Bun extension from fetch's init object and, when the first argument is a plain object, from that object. Both paths go through the same extraction block, so both get the new check.unixoption onfetch. Its closest analogue,net.connect({ path: Buffer }), throwsERR_INVALID_ARG_TYPE.Notes
Repro on 1.4.3-canary (
f42e98025), every non-string row answered by the TCP server:Before:
string -> via UNIX, every other row-> via TCP. After:buffer,url,number,toStringObjreject withERR_INVALID_ARG_TYPE.emptyandnulstill go to TCP.Accepting
Buffer/URLlikenode:fspath arguments was considered.Bun.connect,Bun.listen, and the docs are string-only, so this change keepsfetchconsistent with them instead of adding a new accepted shape in one place.No built-in JS module passes
unixto nativefetch(node:httpuses raw sockets forsocketPath), so nothing internal depends on the old fall-through.Out of scope here:
new Request(url, { unix })does not keep the option, sofetch(request)goes over TCP. That is a separate design question (store Bun extensions onRequest, or throw in the constructor).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch.unix.test.ts