fs: validate copyFile paths in the existing C++ binding by anonrig · Pull Request #65376 · nodejs/node

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

anonrig · 2026-08-18T11:41:18Z

Move getValidatedPath, file URL conversion, and NUL checks into the existing C++ binding.copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all pass the original arguments through.

There is no second copyFileSync binding. JavaScript only does VFS dispatch (still before validation). The copy itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE / FICLONE_FORCE, mode/timestamp preservation, and UV error shapes (syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper: ERR_INVALID_FILE_URL_HOST used a format string without %s, which aborted on file URLs with a hostname. That path is now reachable from copyFile / copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath + binding.copyFile path vs the unified C++-validated binding.copyFile (n=10000, 9 interleaved repeats, Release):

case	old	C++	change
valid copy	60.8 µs/op	61.1 µs/op	~same
missing src (ENOENT)	5.52 µs/op	5.39 µs/op	~2% faster
invalid type	8.04 µs/op	3.92 µs/op	~51% faster
file URL	60.0 µs/op	60.2 µs/op	~same

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by uv_fs_copyfile I/O on valid (~16k ops/sec) and by ENOENT on invalid (~260k ops/sec). Those cases stay within a few percent of the old path. The win is validation-bound input (wrong type). Because validation now lives in binding.copyFile, the callback and promises entry points get the same path — not only copyFileSync.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Tests

test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host; callback copyFile also throws ERR_INVALID_URL_SCHEME)
test/parallel/test-fs-copyfile-respect-permissions.js
test/parallel/test-fs-null-bytes.js
test/parallel/test-fs-error-messages.js
test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
VFS copyFile / copyFileSync tests

nodejs-github-bot · 2026-08-18T11:41:23Z

Review requested:

@nodejs/url

Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>

codecov · 2026-08-18T14:57:50Z

Codecov Report

❌ Patch coverage is 71.42857% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (0fcda9b).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.52%	10 Missing and 18 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65376 +/- ##
==========================================
+ Coverage 90.11% 90.12% +0.01% 
==========================================
Files 752 752 Lines 251861 252394 +533 Branches 47365 47457 +92 ==========================================
+ Hits 226955 227478 +523 + Misses 16238 16217 -21 - Partials 8668 8699 +31

Files with missing lines	Coverage Δ
lib/fs.js	`98.39% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`92.50% <100.00%> (-0.50%)`	⬇️
src/node_url.cc	`80.70% <100.00%> (+2.98%)`	⬆️
src/node_file.cc	`73.96% <70.52%> (-0.27%)`	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina · 2026-08-21T01:06:41Z

I don't understand this. copyFile seems already implemented in C++.

anonrig · 2026-08-21T01:08:09Z

I don't understand this. copyFile seems already implemented in C++.

Except getValidatedPath @mcollina

mcollina · 2026-08-21T01:13:13Z

Then why keep both methods? Can't we just fix copyFile and update all usage of that?

Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>

anonrig · 2026-08-21T01:26:28Z

@mcollina you're right — no need for a second binding.

getValidatedPath, file URL conversion, and NUL checks now live in the existing C++ copyFile. fs.copyFile, fs.copyFileSync, and fs.promises.copyFile all call that one method with the original arguments. The extra copyFileSync binding is gone.

VFS dispatch is still in JS and still runs first.

mcollina

lgtm

Please update the PR description

nodejs-github-bot · 2026-08-21T10:37:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76111/

jasnell · 2026-08-21T14:37:35Z

+ }
+
+ Local<String> path_string;
+ if (!String::NewFromUtf8(isolate,


ToV8Value?

nodejs-github-bot · 2026-08-21T16:55:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76168/

mcollina · 2026-08-21T19:26:11Z

The first commit needs some changes, as it does not reflect what this PR does anymore.
The second commit has metadata issues.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 18, 2026

cursorBotforce-pushed the cursor/copyfilesync-cpp-upstream-317c branch from e13ed43 to 8170ff5Compare August 18, 2026 12:47

cursorBotforce-pushed the cursor/copyfilesync-cpp-upstream-317c branch from 8170ff5 to e4dc3a1Compare August 18, 2026 13:11

anonrig mentioned this pull request Aug 21, 2026
fs: validate copyFile paths in the existing C++ binding anonrig/node#28
Draft

mcollina approved these changes Aug 21, 2026
View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2026

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2026

anonrig changed the title ~~fs: implement copyFileSync in C++~~fs: validate copyFile paths in the existing C++ bindingAug 21, 2026

sxa mentioned this pull request Aug 21, 2026
All nodes of label ‘macos15-arm64’ are offline nodejs/build#4432
Closed

jasnell reviewed Aug 21, 2026
View reviewed changes

Comment threadsrc/node_file.cc
}

Local<String> path_string;
if (!String::NewFromUtf8(isolate,
jasnellAug 21, 2026
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToV8Value?

mcollina added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Aug 21, 2026