worker: reject file:// URLs with non-localhost host#28862
Conversation
|
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 synchronous validation of the host component 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: 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 `@src/bun.js/bindings/webcore/Worker.cpp`:
- Around line 158-169: Extract the duplicated host-validation block into a
single helper and call it from both locations: create a local static helper
(e.g., validateFileURLHost or isAllowedFileURLHost) that takes the URL object
(urlObject) and returns an Exception or bool, keeping the existing platform
guards (`#if` !OS(WINDOWS) and `#if` OS(DARWIN)/else) inside that helper, then
replace the two identical if-blocks with calls to that helper in Worker.cpp so
both the original check and the preload modules loop use the same validation
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58e1499c-c0c6-477a-99f1-7ef63e9e5b47
📒 Files selected for processing (2)
src/bun.js/bindings/webcore/Worker.cpptest/regression/issue/28861.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — prior feedback addressed: error now carries and preload path has test coverage.
Extended reasoning...
Overview
The PR fixes a real bug in Worker::create where file:// URLs with non-empty, non-localhost hosts were silently accepted (host was dropped by fileSystemPath()). Two files changed: Worker.cpp adds a static validateFileURLHost helper called for both the main URL and preload modules, and a new regression test covers both paths.
Prior Feedback Resolution
Both issues raised in my previous review have been resolved. The error thrown now uses Bun::ERR::INVALID_FILE_URL_HOST so the JavaScript error object carries .code = 'ERR_INVALID_FILE_URL_HOST', matching Node.js behavior and Bun.fileURLToPath. A third test was added to cover the preload option validation path, which was previously untested.
Security Risks
No security risks. This is a hardening change that rejects malformed file:// URLs earlier (synchronously) rather than letting them silently resolve to unintended paths.
Level of Scrutiny
Low — focused bug fix with a clear root cause, correct use of existing Bun error infrastructure, and good test coverage including positive and negative cases for both the main URL and preload paths.
Other Factors
The only outstanding item is a nit reported by the bug-hunting system: the four tests run sequentially instead of concurrently (violating CLAUDE.md convention for independent subprocess-spawning tests). This is a performance nit with no functional impact and has been flagged as an inline comment.
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/28861.test.ts`:
- Around line 5-7: Remove the multi-line bug context comment in the test file
and keep only the single-line GitHub issue URL comment on the preceding line;
specifically delete the explanatory comment block that starts after the issue
URL in test/regression/issue/28861.test.ts so the file follows the repo
convention of a single-line issue reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 072cbb21-760b-4678-a048-f141a42930f9
📒 Files selected for processing (1)
test/regression/issue/28861.test.ts
There was a problem hiding this comment.
LGTM — all three previously raised issues (missing property, preload test coverage, and sequential tests) have been addressed in follow-up commits.
Extended reasoning...
Overview
The PR adds a validateFileURLHost static helper in Worker.cpp that rejects file:// URLs with non-empty, non-localhost hosts on POSIX systems. It is called for both the main Worker URL and each preload module URL. A regression test with 4 concurrent tests covers the fix.
Security risks
None — this is a defensive input validation fix that narrows what URLs the Worker constructor accepts, matching Node.js behavior.
Level of scrutiny
Low. The change is a focused input validation guard touching only the Worker constructor's URL parsing path. The helper correctly delegates to the existing Bun::ERR::INVALID_FILE_URL_HOST error path used by Bun.fileURLToPath, ensuring consistent .code properties. Windows is correctly excluded via the OS(WINDOWS) guard.
Other factors
All three concerns raised in prior reviews were addressed: the error now carries code: "ERR_INVALID_FILE_URL_HOST" (fb444b5), preload module validation has test coverage (c4c7436), and the test suite runs concurrently (28a03bd). No outstanding reviewer comments remain.
There was a problem hiding this comment.
LGTM — all prior review feedback addressed.
Extended reasoning...
Overview
This PR adds synchronous host validation for file:// URLs in Worker::create on non-Windows platforms, throwing ERR_INVALID_FILE_URL_HOST when the host is neither empty nor "localhost". Two call sites are covered: the main Worker URL and each preload module entry. A regression test with 4 concurrent sub-process-spawning cases validates the new behavior.
Security risks
None. The change is purely input validation, rejecting malformed file:// URLs before they reach the file-system path resolver. No auth, crypto, or permission-sensitive paths are touched.
Level of scrutiny
Low-to-medium. The logic is small and self-contained, directly mirrors the existing Bun.fileURLToPath validation, and is backed by tests that assert the correct error name, message, and code. The ExceptionOr<void> / ExistingExceptionError plumbing follows an established pattern in the file.
Other factors
All substantive feedback from the prior review round has been incorporated: the Bun::ERR::INVALID_FILE_URL_HOST helper is now used (giving the error a .code property), duplication was eliminated by extracting validateFileURLHost, tests run concurrently, and a dedicated test covers the preload path. The one open nit (stderr not drained in tests) is flagged as an inline comment and does not affect correctness.
There was a problem hiding this comment.
LGTM — all previous concerns (missing .code, no preload test, sequential tests, unread stderr) have been addressed.
Extended reasoning...
Overview
PR adds synchronous file:// host validation to Worker::create in Worker.cpp, and a new concurrent regression test for issue #28861. Two files changed: the C++ Worker implementation and a new TypeScript test.
Security risks
None. This is an input validation fix that makes Bun more strict (rejecting malformed file:// URLs), not less. No auth, crypto, or permission-sensitive paths touched.
Level of scrutiny
Moderate. The C++ change is a targeted guard at the URL-parsing stage, correctly factored into a static ExceptionOr<void> validateFileURLHost helper. It uses the established Bun::ERR::INVALID_FILE_URL_HOST error path (same as Bun.fileURLToPath) for Node compatibility. The Windows UNUSED_PARAM guard is correct. Risk is low.
Other factors
All feedback from earlier review rounds was addressed: the error now carries .code = "ERR_INVALID_FILE_URL_HOST", preload URL validation is covered by test 3, tests run concurrently, and proc.stderr.text() is included in the Promise.all. The remaining inline nit (stderr drained but not asserted in tests 1–3) is a test quality gap, not a functional issue — stderr is correctly consumed to prevent pipe deadlock.
There was a problem hiding this comment.
LGTM — all prior feedback has been addressed.
Extended reasoning...
Overview
The PR adds a validateFileURLHost static helper in Worker.cpp that is called for both the main Worker URL and each preload module URL. On non-Windows platforms it rejects file:// URLs whose host is neither empty nor localhost, throwing via Bun::ERR::INVALID_FILE_URL_HOST so the error carries .code = "ERR_INVALID_FILE_URL_HOST" matching Node.js and Bun.fileURLToPath.
Security risks
None. This is a purely additive validation that rejects malformed URLs earlier and more loudly than before.
Level of scrutiny
Low-to-medium. The C++ change is a self-contained guard in Worker::create with no logic after the early return, and the helper is stateless. The test file covers the main URL path, the preload path, and the acceptance case.
Other factors
All feedback from prior review rounds was incorporated: the error code is attached, tests are concurrent, stderr is drained and asserted clean in tests 1–3, and the acceptance test (test 4) intentionally skips the stderr assertion because the Workers fail asynchronously. No bugs were found by the automated bug-hunting system.
robobun
force-pushed
the
farm/c6b3b6a3/worker-reject-invalid-file-url-host
branch
from
1199564 to
aeb65a1
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the remaining case-sensitivity note is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR adds synchronous host validation for file:// URLs in Worker::create (src/bun.js/bindings/webcore/Worker.cpp), rejecting non-empty/non-localhost hosts on POSIX with Node's ERR_INVALID_FILE_URL_HOST. The validation is factored into a static validateFileURLHost helper, reuses the existing Bun::ERR::INVALID_FILE_URL_HOST error builder (so .code is set), and is applied to both the main worker URL and each preload entry. The ExistingExceptionError return pattern is correct: createDOMException short-circuits on it (JSDOMExceptionHandling.cpp:153) and the caller in JSWorker.cpp:341 has a RETURN_IF_EXCEPTION immediately after Worker::create, so the JS-thrown coded TypeError propagates intact. A new regression test (test/regression/issue/28861.test.ts) covers the . host, example.com host, the preload path, and a non-regression check for empty/localhost hosts.
Security risks
None. The change only tightens input validation — previously-accepted malformed file:// URLs are now rejected synchronously instead of silently resolving to a wrong filesystem path. Windows behavior (UNC handling via fileSystemPath()) is intentionally unchanged, matching Bun.fileURLToPath. No new file access, no network surface, no auth/crypto.
Level of scrutiny
Low-to-moderate. ~30 lines of C++ that mirror an existing validated pattern (Bun.fileURLToPath in BunObject.cpp), plus a self-contained regression test. No CODEOWNER covers either modified file. The PR has gone through five rounds of review feedback (error .code property, helper extraction, preload coverage, concurrent tests, stderr drain + assertion), all of which are now resolved in the current diff.
Other factors
The one outstanding inline comment flags that the surrounding url.startsWith("file://"_s) guard is case-sensitive, so FILE:// bypasses both the new validation and the pre-existing fileSystemPath() conversion. That guard predates this PR and is the established pattern across ~10 sites in src/bun.js/bindings/; changing it here would be scope creep on a codebase-wide convention, and the practical impact is negligible (the bypassed path falls through to a resolver error rather than reproducing the original silent-host-drop bug). It's a reasonable follow-up but not a blocker.
| if (url.startsWith("file://"_s)) { | ||
| WTF::URL urlObject = WTF::URL(url); | ||
| if (urlObject.isValid()) { | ||
| auto hostCheck = validateFileURLHost(globalObject, urlObject); | ||
| if (hostCheck.hasException()) | ||
| return hostCheck.releaseException(); |
There was a problem hiding this comment.
🟣 Minor: the url.startsWith("file://"_s) guard is case-sensitive, so new Worker("FILE://./worker.mjs") or File://example.com/x.mjs skips both the new validateFileURLHost call and the existing fileSystemPath() conversion (same for the preload-modules loop below). This is a pre-existing gating condition used elsewhere in the codebase, not something this PR introduced — but since this PR's purpose is to validate file:// hosts here, it might be worth switching to url.startsWithIgnoringASCIICase("file://"_s) to close the gap while you're touching this block.
Extended reasoning...
What the gap is
Worker::create gates the new host validation on url.startsWith("file://"_s) (Worker.cpp:135). WTF::String::startsWith is case-sensitive — WTF provides a separate startsWithIgnoringASCIICase for case-insensitive matching — and JSWorker.cpp passes the raw user-supplied string into Worker::create without any scheme normalization. So a string-argument call like new Worker("FILE://./worker.mjs") or new Worker("File://example.com/x.mjs") evaluates the prefix check to false and skips the entire if block: both the new validateFileURLHost call AND the pre-existing urlObject.fileSystemPath() conversion. The same case-sensitive guard appears in the preloadModuleStrings loop a few lines down (Worker.cpp:153), so { preload: ["FILE://example.com/x.mjs"] } bypasses validation there too.
Why these are valid file URLs
WHATWG URL parsing normalizes the scheme to lowercase, so new URL("FILE://./x.mjs").protocol === "file:" in any conforming implementation. Node.js parses these inputs as file URLs and rejects them with ERR_INVALID_FILE_URL_HOST. Bun's own WTF::URL parser would likewise lowercase the scheme — but this code never constructs a WTF::URL for uppercase-scheme inputs because the raw-string prefix check short-circuits first.
Why this is pre-existing, not a regression
The case-sensitive startsWith("file://"_s) line predates this PR — the PR only inserted the validateFileURLHost call inside the existing branch. The same case-sensitive pattern is used in ~10 other places across src/bun.js/bindings/ (ZigGlobalObject.cpp, ImportMetaObject.cpp, BunProcess.cpp, etc.), so it reflects an established codebase convention rather than a defect introduced here. Behavior for FILE:// inputs is unchanged from before this PR.
Practical impact
Low. Users almost never write uppercase URL schemes, and this validation is not a security boundary — it exists to give a clear synchronous error rather than a confusing async resolution failure. When the guard is bypassed, fileSystemPath() is also skipped, so the raw FILE://... string falls through to the Zig resolveEntryPointSpecifier and produces a resolution error — it does not reproduce the original #28861 silent-host-drop symptom (which required fileSystemPath() to run and discard the host).
How to close it
Replace url.startsWith("file://"_s) with url.startsWithIgnoringASCIICase("file://"_s) (and likewise for str.startsWith(...) in the preload loop), or parse with WTF::URL(url) first and check urlObject.protocolIs("file"). Either is a one-line change per site.
Step-by-step proof
- User calls
new Worker("File://example.com/x.mjs", { type: "module" }). JSWorker.cppreadsargument0.value().getString()→ the literal"File://example.com/x.mjs"and passes it toWorker::createasurlInit.- Worker.cpp:135 evaluates
"File://example.com/x.mjs".startsWith("file://")→ false (case mismatch onF). - The body — including
validateFileURLHost(globalObject, urlObject)— is never executed;urlremains the raw input string. - Control proceeds to
WebWorker__createwithurlStr = "File://example.com/x.mjs", which the Zig resolver fails to resolve (different error thanERR_INVALID_FILE_URL_HOST). - In Node.js, the same call throws
TypeError [ERR_INVALID_FILE_URL_HOST]synchronously, because Node normalizes the scheme vianew URL()before checking the host.
new Worker("file://./worker.mjs") used to silently drop the host
portion (`.`) and try to load "/worker.mjs" because WTF::URL's
fileSystemPath() ignores the host. Validate the host after parsing so
Worker now behaves like node:url.fileURLToPath and matches Node's
ERR_INVALID_FILE_URL_HOST message.
Fixes #28861
Route the host validation through Bun::ERR::INVALID_FILE_URL_HOST so the thrown error has the same .code = 'ERR_INVALID_FILE_URL_HOST' that Node and Bun.fileURLToPath already set. Extract the duplicated check into a static helper used for both the Worker URL and preload modules.
Jarred-Sumner
force-pushed
the
farm/c6b3b6a3/worker-reject-invalid-file-url-host
branch
from
aeb65a1 to
e813bba
Compare
There was a problem hiding this comment.
LGTM — narrow Node-compat fix mirroring Bun.fileURLToPath's host check, with regression coverage; the case-sensitivity note I left earlier is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR adds file:// host validation to Worker::create in src/jsc/bindings/webcore/Worker.cpp so that new Worker("file://./x.mjs") and similar non-empty/non-localhost hosts throw a synchronous TypeError with code: 'ERR_INVALID_FILE_URL_HOST' on posix, matching Node.js and Bun.fileURLToPath. The new validateFileURLHost helper is called for both the main URL and each preload entry. A new regression test (test/regression/issue/28861.test.ts) covers the reject path for the main URL, the reject path for preload, and the accept path for empty/localhost hosts.
Security risks
None. This adds an input-validation rejection path; it does not relax any check, open any new resource, or change behavior for valid inputs. The host check only fires on posix and only when the URL already started with file:// and parsed successfully — it strictly narrows the set of accepted inputs.
Level of scrutiny
Low-to-moderate. Worker::create is a core code path, but the diff is small (~30 LOC of C++), purely additive validation, and copies the exact Bun::ERR::INVALID_FILE_URL_HOST invocation already used in BunObject.cpp:908-911. The ExceptionOr<void> + ExistingExceptionError pattern correctly propagates the JS exception thrown via ThrowScope back through the WebCore exception machinery. Windows is intentionally excluded (UNC paths are valid hosts there), matching Bun.fileURLToPath.
Other factors
This PR went through several review iterations and every substantive piece of feedback was addressed: switched to the Bun::ERR:: helper so the error carries .code, extracted the duplicated check into a helper, added preload coverage, made the tests concurrent, and now drains/asserts stderr. The only remaining inline note is my own optional observation about the pre-existing case-sensitive startsWith("file://") guard, which is a codebase-wide convention and not introduced by this PR. CI failures in the Buildkite comment (s3-storage-class, no-orphans, dev-and-prod, 26225) are unrelated flakes. No CODEOWNERS cover this path.
1 participant
Fixes #28861
Repro
Bun silently accepted the URL and the program exited normally. Node and
Deno both throw synchronously.
Cause
Worker::createinsrc/bun.js/bindings/webcore/Worker.cppparsesthe URL with
WTF::URL, then callsurlObject.fileSystemPath(). Forfile://./worker.mjs, WTF parses host=.+ path=/worker.mjsandfileSystemPath()drops the host entirely — so Bun went on to load/worker.mjs, which didn't exist. The resulting async resolver errorfired an
errorevent with no listener and was swallowed.Bun.fileURLToPathalready validates the host inBunObject.cpp;Worker::createjust wasn't calling into that code path.Fix
Validate the host after parsing the URL in
Worker::create. On posix,reject any
file://URL whose host isn't empty or"localhost"with a
TypeErrorwhose message matches Node'sERR_INVALID_FILE_URL_HOST(File URL host must be "localhost" or empty on linux/darwin). Windows is unaffected —fileSystemPath()handles UNC paths there, same as
Bun.fileURLToPathdoes.The same validation is applied to
file://preload modules in theWorker's options.
Verification
The passing case with system bun is the "accepts empty/localhost host"
test, which validates we haven't regressed valid
file:///URLs.