Repository navigation
Conversation
…es.cp The node cp port (#31830) runs node:path checks on the validated src and dest before the native copy. getValidatedFsPath passes a Buffer through unchanged, so path.dirname threw ERR_INVALID_ARG_TYPE. bun 1.3.14 took Buffer paths in all three functions. Decode the bytes to a string once, after validation, in a helper shared by the three entry points.
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. Walkthrough
ChangesBuffer-aware filesystem copying
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change preserves Buffer paths through copy operations while keeping string-path behavior unchanged, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed problem statement, implementation summary, verification results, background, and testing context. It does not use the exact template headings, but it covers the required information for what the PR does and how the code was verified. Comment |
|
Status: reworked per review, ready for another look. Reproduced on bun 1.4.1 and on a debug build of main with The fix keeps a Buffer path as bytes through the node checks, the walker, and the native copy. The path arithmetic runs on a latin1 view of the bytes and goes back to bytes. The 7 new tests in |
There was a problem hiding this comment.
LGTM — small, well-scoped regression fix restoring Buffer/Uint8Array path support in the cp family.
What was reviewed:
getValidatedCpPathcorrectly wrapsgetValidatedFsPathand decodes non-string results viaBufferPrototypeToString.$call(p), matching the file's existing primordial pattern and the.$callconvention.- All three entry points (
cpSync, callbackcp,promises.cp) are switched consistently; the droppedrequire("internal/validators")at each site is now dead since the helper moved intocp-sync. - Tests cover Buffer src, Buffer dest, plain Uint8Array, recursive directories, filter-receives-strings, and the callback form; they're in the existing
cp.test.tsusingtempDir/await usingper harness conventions.
Extended reasoning...
Overview
This PR fixes a regression in fs.cp, fs.cpSync, and fs.promises.cp where Buffer/Uint8Array path arguments stopped working after the Node cp port landed. The fix adds a single helper getValidatedCpPath in src/js/internal/fs/cp-sync.ts that runs the existing getValidatedFsPath validator (which passes Uint8Array through unchanged) and then decodes any non-string result to a UTF-8 string so the JS-side node:path operations (dirname, resolve, join) can consume it. The three dispatchers in src/js/node/fs.ts and src/js/node/fs.promises.ts are switched to the new helper, dropping their now-redundant separate require("internal/validators") import. Five new tests are added to the existing test/js/node/fs/cp.test.ts.
Security risks
None. The change decodes user-supplied path bytes to a string before handing them to the same validation and copy machinery that already handles string paths. Null-byte rejection happens in getValidatedFsPath before the decode, on the raw bytes. No new privilege boundaries, no new parsing of untrusted structured data.
Level of scrutiny
Low-to-moderate. This is a ~10-line runtime change plus test additions. The helper follows the file's established primordial-capture pattern exactly (const BufferPrototypeToString = Buffer.prototype.toString; alongside the existing ArrayPrototypeEvery, StringPrototypeSplit) and uses .$call per the builtins tamper-proofing convention. Buffer.prototype.toString called on a plain Uint8Array works correctly since Buffer extends Uint8Array. The comment on the helper is concise and explains the "why" (node's cpSync C++ takes raw bytes; the JS walker uses node:path which needs strings). The PR description transparently notes the UTF-8-decode limitation for non-UTF-8 byte paths as an explicit scope decision, which is reasonable — Bun 1.3.14 passed bytes straight to native, but the ported walker already decodes child names as UTF-8, so this doesn't newly break anything.
Other factors
Test coverage is thorough for the size of the fix: the parameterized impls loop exercises both cpSync and promises.cp with Buffer src, Buffer dest, plain Uint8Array, and a recursive directory copy; a separate test covers the callback form; the filter test asserts the documented Node contract that filter receives string paths and sorts its captured pairs to avoid ordering flakiness. Tests use tempDir with await using, live in the existing module test file, and would fail on the pre-fix build (the path.dirname call would throw ERR_INVALID_ARG_TYPE). No CODEOWNERS cover the touched paths. Exit reason was dry_streak with no candidate findings.
|
The review requests no changes, so the branch stays as pushed (15603cd). Buildkite build #109045 is still running. The red |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Keep it as a buffer, don't decode UTF-8
|
Understood. I will rework this so the Buffer stays a Buffer: the node checks and the walker will run their path arithmetic on a latin1 view of the bytes (one byte per code unit, so it round-trips) and pass the bytes themselves to every syscall. I will update the PR when it is pushed. |
|
Updated 8:42 PM PT - Sep 1st, 2026
✅ @robobun, your commit 234f1f4aaaf8958d56c2056614dea1ac6da8fe5d passed in 🧪 To try this PR locally: bunx bun-pr 41112That installs a local version of the PR into your bun-41112 --bun |
fe3a4c8 to
8aa45e6
Compare
The node cp port (#31830) runs node:path checks and a JS walker on the validated src and dest before the native copy. getValidatedFsPath passes a Buffer through unchanged, so path.dirname threw ERR_INVALID_ARG_TYPE. bun 1.3.14 took Buffer paths in all three functions. A Buffer path now stays bytes all the way to the syscall. The checks and the walker run their dirname/join/resolve on a latin1 view of the bytes (one byte per code unit, and node:path inspects only ASCII) and turn the result back into bytes. Entry names and link targets under a Buffer path are read as bytes too. The filter and the error objects get the Buffers they were given.
8aa45e6 to
234f1f4
Compare
|
Reworked as asked, pushed as 234f1f4: the Buffer stays a Buffer.
Tests: 7 new cases in |
Problem
fs.cpSync(Buffer.from(src), dest)throwsTypeError [ERR_INVALID_ARG_TYPE]: The "path" property must be of type string, got object.fs.cp,fs.promises.cp, andUint8Arraypaths fail the same way. Bun 1.3.14 copied the file in all three.checkParentPathsSync(src/js/internal/fs/cp-sync.ts:228) callspath.dirnameon theUint8ArraythatgetValidatedFsPathpasses through, andnode:pathtakes only strings.Fix
node:patharithmetic on a latin1 view of the bytes, then turn the result back into a Buffer.{ encoding: "buffer" }, so names that are not UTF-8 copy byte for byte.filterand error objects get the Buffers they were given. String paths are unchanged.test/js/node/fs/cp.test.ts(7 new tests, all fail on 1.4.1, one is Linux only). Alsocp-symlink-target.test.tsand the 77 vendored nodetest-fs-cp-*tests.readdirwithwithFileTypesand encoding"buffer"returns undefined for Dirent.name #27914 at theopendirfork).Background
fs.cpSyncvalidates the arguments, runs node's checks in JS, then calls the native binding or the ported walker (internal/fs/cp-sync.ts, async ininternal/fs/cp.ts).latin1Slicemaps each byte to one code unit andBuffer.from(view, "latin1")maps it back, andnode:pathinspects only ASCII.Notes
resolve()falls back toprocess.cwd(), a UTF-16 string, so the helpers pass a latin1 view of the cwd in explicitly when they resolve a Buffer path.cpSyncaccepts a Buffer path for a file or for a directory withoutfilter(its checks and directory copy are C++). Withfilter, and infs.cpandfs.promises.cp, node throwsERR_INVALID_ARG_TYPEfrompath.joinorpath.dirname, because its JS walker has the same shape as the one ported here. Node documentssrcanddestasstring | URL. This PR accepts Buffers in all three, as bun 1.3.14 did, and in the walker too.readdirSync(dir, { withFileTypes: true, encoding: "buffer" })andopendir(dir, { encoding: "buffer" })return entries without usable names in bun today (fs: copy directory entries with non-UTF-8 names byte-exact in cp/cpSync #36064 covers the first). The walker only needs names, so it lists a Buffer directory withreaddir(dir, { encoding: "buffer" }). The macOS clonefile pre-scan still useswithFileTypes, so a directory name that is not UTF-8 makes the scan bail to the walker, which is the conservative outcome.onLinkcomments that fs throws anyway). Andfs.openAsBlob(Buffer)trips a JSC debug assertion (JSCell::classInfoduringMutatorState::Sweeping) under GC pressure: the Blob store drops its pinned buffer inside the finalizer, andJSC__JSValue__unpinArrayBufferinspects the cell there. String paths do not. Neither involves cp.fs.cpSync(src, dest)with no options went straight to the native binding (if (!options) return fs.cpSync(src, dest)), which is why 1.3.14 worked.bun 1.4.1:
TypeError: The "path" property must be of type string, got objectatcheckParentPathsSync (internal:fs/cp-sync:147:34). bun 1.3.14 and node v26.3.0:hello.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/node/fs/cp.test.ts