Reject interior null bytes in Bun.mmap, Glob.scan, and FileSystemRouter paths - #33181
Reject interior null bytes in Bun.mmap, Glob.scan, and FileSystemRouter paths#33181robobun wants to merge 2 commits into
Conversation
|
Updated 6:18 PM PT - Jul 6th, 2026
❌ @robobun, your commit 3b69257 has some failures in 🧪 To try this PR locally: bunx bun-pr 33181That installs a local version of the PR into your bun-33181 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR adds null-byte validation for path and pattern inputs across Bun's filesystem APIs. A new reusable ChangesNull-byte validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward application of the existing Valid::path_null_bytes pattern to three more path-taking entry points, with thorough tests.
Extended reasoning...
Overview
This PR closes a null-byte path-truncation gap in three Bun APIs (Bun.mmap, Glob.scan/scanSync, Bun.FileSystemRouter) by adding the same interior-NUL check that Bun.file() and node:fs already perform. The change generalizes Valid::path_null_bytes into Valid::no_null_bytes(slice, name, accepts, global) (the old function becomes a thin wrapper with identical output), then adds four one-line calls at the JS→native boundaries. Tests are added to each API's existing test file, plus a drive-by speedup of an unrelated slow test that keeps its guarding assertion.
Security risks
The change is security-hardening: it adds fail-closed validation, never removes any. The refactor of path_null_bytes is verifiably behavior-preserving — same ERR_INVALID_ARG_VALUE code, byte-identical message for existing callers. The only observable behavior change is that previously-truncated paths (which are never legitimate) now throw. There is no new attack surface.
Level of scrutiny
Low-to-moderate. The production diff is ~30 lines: one helper refactor plus four validation calls placed before any allocation or side effect on their respective paths, so the new early-return error paths don't leak (ZigStringSlice/OwnedString locals drop normally). This is the mechanical extension of an established, already-reviewed pattern to sibling call sites — exactly the "fix the whole class" shape the repo guidelines ask for.
Other factors
Test coverage is thorough: sync/async, string-form and object-form cwd, the glob pattern itself, and a negative test confirming Glob.match() (pure string matcher, no FS access) intentionally still treats NUL as a plain character. The hash-collision test speedup replaces a 600k-iteration birthday search with a precomputed pair but retains the hash32(a) === hash32(b) assertion so it can't go vacuous if wyhash changes. No prior reviewer comments; bug hunter found nothing.
There was a problem hiding this comment.
LGTM — straightforward input validation using the existing Valid helper, with tests at every entry point.
Extended reasoning...
Overview
This PR adds interior-NUL-byte rejection to three Bun-native path-taking APIs (Bun.mmap, Glob.scan/scanSync, FileSystemRouter constructor) that previously passed user strings to C syscalls unchecked. The existing Valid::path_null_bytes helper is generalized into Valid::no_null_bytes(slice, name, accepts, global) so callers can name their argument in the error message, and the original wrapper delegates to it with the exact same message text — so Bun.file / node:fs behavior is byte-for-byte unchanged. Four one-line validation calls are inserted at the JS→native boundaries, each with a corresponding test that asserts both the error code and message substring. A tangential change makes the wyhash-collision router test deterministic (precomputed pair + explicit hash32(a) === hash32(b) guard) to fix a debug+ASAN timeout.
Security risks
None introduced; this is security hardening. The change is purely additive fail-closed validation that closes a path-truncation confused-deputy vector (JS-level checks see "safe\0/../secret", the kernel sees "safe"). No security check is removed or weakened. Error paths return early via ? before any allocation or side effect that would need unwinding — in __scan the pending-activity counter is only incremented after make_glob_walker succeeds, and in filesystem_router the check runs before the arena/log setup.
Level of scrutiny
Medium — it touches path validation, which is security-adjacent, but the change shape is mechanical: reuse the same centralized helper the rest of node:fs already routes through, at the same layer (JS argument parsing, before any syscall). No new parsing logic, no control-flow changes to happy paths, no cross-platform branching. The refactored helper's fast path (index_of_char(slice, 0).is_none()) is identical to before.
Other factors
Tests are placed in the existing per-API files, cover both the object and string forms of cwd, both sync/async scan, assert the specific error code, and include a negative test confirming Glob.match (pure string matcher) still accepts NULs. The PR description confirms all seven tests fail on the unfixed build. The hash-collision test rewrite keeps an explicit collision assertion so it can't go vacuous. No outstanding reviewer comments; bug-hunting system found nothing.
|
Rebased onto current main and force-pushed ( CI status across the runs on this PR. Every run passes the full matrix except for jobs that fail on infrastructure or documented cross-PR flakes, none of which touch this change:
The three test files this PR adds to ( The change itself is four validation calls routed through one shared helper. It is ready for a maintainer. |
…in path arguments
Bun.mmap(path), Glob.scan/scanSync (both the pattern and the cwd
option), and new FileSystemRouter({ dir }) passed their path arguments
through to C-string syscalls without checking for interior NUL bytes.
C paths stop at the first NUL, so a string that passed JS-level
validation reached the OS as a different, shorter path (CWE-158). For
example:
new Bun.Glob("*.ts").scanSync({ cwd: safe + "\0/../secret" })
scans `safe/`, while every JS-level check (and path.resolve) saw a path
resolving to `secret/`. Bun.file() and node:fs already reject these
inputs with ERR_INVALID_ARG_VALUE.
Generalize the existing Valid::path_null_bytes helper (the one
Bun.file uses) to take the argument name, and call it from the three
entry points. Glob's match() and constructor are intentionally
unchanged: match() never touches the filesystem, so the pattern is only
validated when scan/scanSync turn it into a path.
Also make the pre-existing FileSystemRouter 32-bit-hash-collision test
deterministic; its 600k-iteration birthday search alone exceeded the
default per-test timeout on debug+ASAN builds.
620a625 to
3b69257
Compare
There was a problem hiding this comment.
LGTM — small, well-tested tightening that routes four path entry points through the existing NUL-byte validator.
Extended reasoning...
Overview
This PR adds interior-NUL-byte rejection to three Bun-native path-taking APIs (Bun.mmap, Glob.scan/scanSync cwd + pattern, new Bun.FileSystemRouter({ dir })) by generalizing the existing Valid::path_null_bytes helper into a parameterized Valid::no_null_bytes(slice, name, accepts, global) and calling it at four JS→native entry points. The refactor of path_null_bytes is a pure delegation with identical error code and message. Seven new tests are added to the existing per-API test files, plus a drive-by determinism fix to the wyhash-collision test in filesystem_router.test.ts (precomputed collision pair with an explicit hash32(a) === hash32(b) guard, replacing a 600k-iteration birthday search that timed out under debug+ASAN).
Security risks
This is a security-hardening change that tightens validation (fail-closed): previously, a NUL-embedded path could reach the syscall truncated, so JS-level validation and the actual filesystem operation disagreed on which path was used. The fix aligns these APIs with the existing behavior of Bun.file() and node:fs, using the same ERR_INVALID_ARG_VALUE error shape. There is no loosening of any check, no new attack surface, and no change to any TLS/auth/crypto path. The only behavioral change is that inputs which were previously silently truncated now throw — which is the desired outcome.
Level of scrutiny
Low-to-medium. The native diff is four one-line validator calls plus a small helper refactor that is trivially behavior-preserving for the existing path_null_bytes callers (same INVALID_ARG_VALUE code, same message text). Each call site is placed after the string is materialized and before it reaches path resolution, and the ? early-return relies on ordinary Rust Drop for the just-created slice locals — no manual cleanup is bypassed. Glob.match() is intentionally left alone (pure string matcher, never touches the FS), and a test pins that contract.
Other factors
- No CODEOWNERS coverage on the touched files.
- Bug-hunting system found nothing.
- CI: the three modified test files passed on every lane across two full-matrix runs; the four failing jobs are documented cross-PR flakes (#33044, terminal.test.ts macOS timeout, MySQL docker readiness) unrelated to this change.
- The wyhash test rewrite is a strict improvement: it keeps the collision precondition asserted, so it cannot go vacuous if the hash changes, and drops a 60s per-test timeout.
- Tests cover both the object and string forms of
cwd, bothscanandscanSync, and assert both the errorcodeand the message substring.
1 participant
Problem
Bun.mmap(path),Glob.prototype.scan/scanSync(the pattern and thecwdoption, in both object and string form), andnew Bun.FileSystemRouter({ dir })hand their path arguments to C-string syscalls without checking for interior NUL bytes. A C path stops at the first NUL, so the OS operates on a different, shorter path than the one JS-level code saw and validated.Bun.file()andnode:fsalready reject these inputs withERR_INVALID_ARG_VALUE; these three APIs missed the check. Debug builds trip theZStr::as_cstr: interior NUL would truncate the C viewassertion instead of truncating.Reproduction on Bun 1.4.0:
Fix
Generalize the existing
Valid::path_null_byteshelper (the oneBun.fileandnode:fsuse) to take the argument name, and call it at the three JS-to-native entry points:Bun.mmap: thepathargumentGlob.scan/Glob.scanSync: thecwdoption and the patternnew Bun.FileSystemRouter: thediroptionEach now throws the same error shape as
Bun.file:Intentionally unchanged:
Glob.prototype.matchand theGlobconstructor.match()is a pure string matcher that never touches the filesystem, so the pattern is only validated oncescan/scanSyncturn it into a path.Tests
Added to each API's existing test file (
mmap.test.js,glob/scan.test.ts,filesystem_router.test.ts). All seven fail on the unfixed build; the glob pattern one fails by listing the truncated directory's contents:Also made the pre-existing
filesystem_router.test.tshash-collision test deterministic: its 600k-iteration birthday search alone exceeded the 5s default per-test timeout under debug+ASAN. The precomputed pair keeps an explicithash32(a) === hash32(b)assertion so the test still fails, rather than going vacuous, if wyhash ever changes.