From e4dc3a1f635f205a761625277c4f14e2e28aba87 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 18 Aug 2026 13:11:50 +0000 Subject: [PATCH 1/2] fs: implement copyFileSync in C++ 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 --- lib/fs.js | 6 +- src/node_file.cc | 182 ++++++++++++++++++++++++++++++ src/node_url.cc | 2 +- test/parallel/test-fs-copyfile.js | 54 +++++++++ typings/internalBinding/fs.d.ts | 2 + 5 files changed, 240 insertions(+), 6 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 81c4d2b9c884..10fa76d51001 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -3682,11 +3682,7 @@ function copyFileSync(src, dest, mode) { const result = h.copyFileSync(src, dest, mode); if (result !== undefined) return; } - binding.copyFile( - getValidatedPath(src, 'src'), - getValidatedPath(dest, 'dest'), - mode, - ); + binding.copyFileSync(src, dest, mode); } /** diff --git a/src/node_file.cc b/src/node_file.cc index ae0d9f34f8e1..a3c0dbff32ee 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -46,6 +46,7 @@ #include #include #include +#include #include #if defined(__MINGW32__) || defined(_MSC_VER) @@ -77,6 +78,7 @@ using v8::Local; using v8::LocalVector; using v8::Maybe; using v8::MaybeLocal; +using v8::NewStringType; using v8::Nothing; using v8::Number; using v8::Object; @@ -2416,6 +2418,184 @@ static void OpenFileHandle(const FunctionCallbackInfo& args) { } } +// Matches lib/internal/url.js isURL(): duck-type WHATWG URL objects vs +// legacy url.parse() results (which have `auth` and `path` properties). +static bool IsUrlLike(Environment* env, Local value) { + if (!value->IsObject() || value->IsUint8Array()) { + return false; + } + + Isolate* isolate = env->isolate(); + Local context = env->context(); + Local obj = value.As(); + + Local href; + if (!obj->Get(context, env->href_string()).ToLocal(&href)) { + return false; + } + if (!href->BooleanValue(isolate)) { + return false; + } + + Local protocol; + if (!obj->Get(context, env->protocol_string()).ToLocal(&protocol)) { + return false; + } + if (!protocol->BooleanValue(isolate)) { + return false; + } + + Local auth; + if (!obj->Get(context, FIXED_ONE_BYTE_STRING(isolate, "auth")) + .ToLocal(&auth)) { + return false; + } + + Local path; + if (!obj->Get(context, env->path_string()).ToLocal(&path)) { + return false; + } + + return auth->IsUndefined() && path->IsUndefined(); +} + +static bool ContainsNul(const BufferValue& path) { + return path.length() > 0 && + std::memchr(path.out(), '\0', path.length()) != nullptr; +} + +// C++ equivalent of getValidatedPath(value, propName): accepts string, +// Uint8Array/Buffer, or WHATWG URL, rejects embedded NUL bytes, and converts +// file: URLs to paths. Returns an empty MaybeLocal and throws on failure. +static MaybeLocal GetValidatedPath(Environment* env, + Local input, + const char* prop_name) { + Isolate* isolate = env->isolate(); + + if (input->IsString() || input->IsUint8Array()) { + return input; + } + + if (IsUrlLike(env, input)) { + Local context = env->context(); + Local href; + if (!input.As()->Get(context, env->href_string()).ToLocal(&href)) { + return MaybeLocal(); + } + + Utf8Value href_utf8(isolate, href); + auto parsed = ada::parse(href_utf8.ToStringView()); + if (!parsed) { + url::ThrowInvalidURL(env, href_utf8.ToStringView(), std::nullopt); + return MaybeLocal(); + } + + // Match lib/internal/url.js fileURLToPath(): non-file schemes throw + // ERR_INVALID_URL_SCHEME with the same message as JS (`file`, not `file:`). + if (parsed->type != ada::scheme::FILE) { + THROW_ERR_INVALID_URL_SCHEME(isolate, "The URL must be of scheme file"); + return MaybeLocal(); + } + + std::optional file_path = url::FileURLToPath(env, *parsed); + if (!file_path.has_value()) { + return MaybeLocal(); + } + + if (file_path->find('\0') != std::string::npos) { + THROW_ERR_INVALID_ARG_VALUE( + env, + "The argument '%s' must be a string, Uint8Array, or URL " + "without null bytes. Received %s", + prop_name, + DetermineSpecificErrorType(env, input)); + return MaybeLocal(); + } + + Local path_string; + if (!String::NewFromUtf8(isolate, + file_path->data(), + NewStringType::kNormal, + static_cast(file_path->size())) + .ToLocal(&path_string)) { + return MaybeLocal(); + } + return path_string; + } + + if (isolate->HasPendingException()) { + return MaybeLocal(); + } + + THROW_ERR_INVALID_ARG_TYPE( + env, + "The \"%s\" argument must be of type string or an instance of " + "Buffer or URL. Received %s", + prop_name, + DetermineSpecificErrorType(env, input)); + return MaybeLocal(); +} + +// Full C++ implementation of fs.copyFileSync(): path validation, mode +// validation, permission checks, and the copy itself. +static void CopyFileSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + + CHECK_GE(args.Length(), 2); // src, dest[, mode] + + Local src_val; + if (!GetValidatedPath(env, args[0], "src").ToLocal(&src_val)) { + return; + } + Local dest_val; + if (!GetValidatedPath(env, args[1], "dest").ToLocal(&dest_val)) { + return; + } + + BufferValue src(isolate, src_val); + CHECK_NOT_NULL(*src); + if (ContainsNul(src)) { + THROW_ERR_INVALID_ARG_VALUE( + env, + "The argument 'src' must be a string, Uint8Array, or URL " + "without null bytes. Received %s", + DetermineSpecificErrorType(env, args[0])); + return; + } + + BufferValue dest(isolate, dest_val); + CHECK_NOT_NULL(*dest); + if (ContainsNul(dest)) { + THROW_ERR_INVALID_ARG_VALUE( + env, + "The argument 'dest' must be a string, Uint8Array, or URL " + "without null bytes. Received %s", + DetermineSpecificErrorType(env, args[1])); + return; + } + + Local mode = args.Length() > 2 ? args[2] : Undefined(isolate); + int flags; + if (!GetValidFileMode(env, mode, UV_FS_COPYFILE).To(&flags)) { + return; + } + + ToNamespacedPath(env, &src); + ToNamespacedPath(env, &dest); + + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemRead, src.ToStringView()); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView()); + + FSReqWrapSync req_wrap_sync("copyfile", *src, *dest); + FS_SYNC_TRACE_BEGIN(copyfile); + SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_copyfile, *src, *dest, flags); + FS_SYNC_TRACE_END(copyfile); +} + static void CopyFile(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -4229,6 +4409,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "writeFileUtf8", WriteFileUtf8); SetMethod(isolate, target, "realpath", RealPath); SetMethod(isolate, target, "copyFile", CopyFile); + SetMethod(isolate, target, "copyFileSync", CopyFileSync); SetMethod(isolate, target, "chmod", Chmod); SetMethod(isolate, target, "fchmod", FChmod); @@ -4357,6 +4538,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(WriteFileUtf8); registry->Register(RealPath); registry->Register(CopyFile); + registry->Register(CopyFileSync); registry->Register(CpSyncCheckPaths); registry->Register(CpSyncOverrideFile); diff --git a/src/node_url.cc b/src/node_url.cc index 38aa7fc48eb1..03f66d68c476 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -688,7 +688,7 @@ std::optional FileURLToPath(Environment* env, if (hostname.size() > 0) { THROW_ERR_INVALID_FILE_URL_HOST( env->isolate(), - "File URL host must be \"localhost\" or empty on ", + "File URL host must be \"localhost\" or empty on %s", std::string(per_process::metadata.platform)); return std::nullopt; } diff --git a/test/parallel/test-fs-copyfile.js b/test/parallel/test-fs-copyfile.js index 51d7153de025..edae50fb6d84 100644 --- a/test/parallel/test-fs-copyfile.js +++ b/test/parallel/test-fs-copyfile.js @@ -10,6 +10,7 @@ const { UV_ENOENT, UV_EEXIST } = internalBinding('uv'); +const { pathToFileURL } = require('url'); const src = fixtures.path('a.js'); const dest = tmpdir.resolve('copyfile.out'); const { @@ -55,6 +56,22 @@ verify(src, dest); fs.copyFileSync(src, dest, 0); verify(src, dest); +// Verify Buffer and file: URL paths. +{ + const destBuf = tmpdir.resolve('copyfile.buffer'); + fs.copyFileSync(Buffer.from(src), Buffer.from(destBuf)); + verify(src, destBuf); + + const destUrl = tmpdir.resolve('copyfile.url'); + fs.copyFileSync(pathToFileURL(src), pathToFileURL(destUrl)); + verify(src, destUrl); + + const destU8 = tmpdir.resolve('copyfile.uint8'); + fs.copyFileSync(new Uint8Array(Buffer.from(src)), + new Uint8Array(Buffer.from(destU8))); + verify(src, destU8); +} + // Verify that UV_FS_COPYFILE_FICLONE can be used. fs.unlinkSync(dest); fs.copyFileSync(src, dest, UV_FS_COPYFILE_FICLONE); @@ -142,6 +159,43 @@ assert.throws(() => { ); }); +assert.throws(() => { + fs.copyFileSync(new URL('http://example.com/a'), dest); +}, { + code: 'ERR_INVALID_URL_SCHEME', + name: 'TypeError', + message: 'The URL must be of scheme file', +}); + +if (common.isWindows) { + ['%2f', '%2F', '%5c', '%5C'].forEach((i) => { + assert.throws( + () => fs.copyFileSync(new URL(`file:///c:/tmp/${i}`), dest), + { + code: 'ERR_INVALID_FILE_URL_PATH', + name: 'TypeError', + } + ); + }); +} else { + ['%2f', '%2F'].forEach((i) => { + assert.throws( + () => fs.copyFileSync(new URL(`file:///c:/tmp/${i}`), dest), + { + code: 'ERR_INVALID_FILE_URL_PATH', + name: 'TypeError', + } + ); + }); + assert.throws( + () => fs.copyFileSync(new URL('file://hostname/a/b/c'), dest), + { + code: 'ERR_INVALID_FILE_URL_HOST', + name: 'TypeError', + } + ); +} + assert.throws(() => { fs.copyFileSync(src, dest, 'r'); }, { diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index 6e1702996dfc..907a48c07bc4 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -75,6 +75,7 @@ declare namespace InternalFSBinding { function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: FSReqCallback): void; function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: undefined, ctx: FSSyncContext): void; function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, usePromises: typeof kUsePromises): Promise; + function copyFileSync(src: unknown, dest: unknown, mode?: unknown): void; function cpSyncCheckPaths(src: StringOrBuffer, dest: StringOrBuffer, dereference: boolean, recursive: boolean): void; function cpSyncOverrideFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, preserveTimestamps: boolean): void; @@ -261,6 +262,7 @@ export interface FsBinding { chown: typeof InternalFSBinding.chown; close: typeof InternalFSBinding.close; copyFile: typeof InternalFSBinding.copyFile; + copyFileSync: typeof InternalFSBinding.copyFileSync; cpSyncCheckPaths: typeof InternalFSBinding.cpSyncCheckPaths; cpSyncOverrideFile: typeof InternalFSBinding.cpSyncOverrideFile; cpSyncCopyDir: typeof InternalFSBinding.cpSyncCopyDir; From 0fcda9bbf1b17470cc650f059c67515505f3f2e7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 21 Aug 2026 01:26:16 +0000 Subject: [PATCH 2/2] fs: validate copyFile paths in the existing C++ binding 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 Co-authored-by: Yagiz Nizipli --- lib/fs.js | 4 +-- lib/internal/fs/promises.js | 7 +---- src/node_file.cc | 52 ++++++------------------------- test/parallel/test-fs-copyfile.js | 8 +++++ typings/internalBinding/fs.d.ts | 9 +++--- 5 files changed, 24 insertions(+), 56 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 10fa76d51001..d993c467e12d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -3659,8 +3659,6 @@ function copyFile(src, dest, mode, callback) { const h = vfsState.handlers; if (h !== null && vfsVoid(h.copyFile(src, dest, mode), callback)) return; - src = getValidatedPath(src, 'src'); - dest = getValidatedPath(dest, 'dest'); callback = makeCallback(callback); const req = new FSReqCallback(); @@ -3682,7 +3680,7 @@ function copyFileSync(src, dest, mode) { const result = h.copyFileSync(src, dest, mode); if (result !== undefined) return; } - binding.copyFileSync(src, dest, mode); + binding.copyFile(src, dest, mode); } /** diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 3e336024a15a..6e2252abd683 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -1335,12 +1335,7 @@ async function copyFile(src, dest, mode) { if (promise !== undefined) { await promise; return; } } return await PromisePrototypeThen( - binding.copyFile( - getValidatedPath(src, 'src'), - getValidatedPath(dest, 'dest'), - mode, - kUsePromises, - ), + binding.copyFile(src, dest, mode, kUsePromises), undefined, handleErrorFromBinding, ); diff --git a/src/node_file.cc b/src/node_file.cc index a3c0dbff32ee..15eaa9afad4f 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2464,9 +2464,10 @@ static bool ContainsNul(const BufferValue& path) { std::memchr(path.out(), '\0', path.length()) != nullptr; } -// C++ equivalent of getValidatedPath(value, propName): accepts string, -// Uint8Array/Buffer, or WHATWG URL, rejects embedded NUL bytes, and converts -// file: URLs to paths. Returns an empty MaybeLocal and throws on failure. +// C++ equivalent of getValidatedPath(value, propName), used by CopyFile for +// the sync, callback, and promises paths. Accepts string, Uint8Array/Buffer, +// or WHATWG URL, rejects embedded NUL bytes, and converts file: URLs to +// paths. Returns an empty MaybeLocal and throws on failure. static MaybeLocal GetValidatedPath(Environment* env, Local input, const char* prop_name) { @@ -2536,13 +2537,15 @@ static MaybeLocal GetValidatedPath(Environment* env, return MaybeLocal(); } -// Full C++ implementation of fs.copyFileSync(): path validation, mode -// validation, permission checks, and the copy itself. -static void CopyFileSync(const FunctionCallbackInfo& args) { +// Shared by the sync, callback, and promises copyFile paths. Path validation +// (string / Uint8Array / file: URL, NUL checks) happens here so JS callers +// can pass the original arguments through. +static void CopyFile(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); - CHECK_GE(args.Length(), 2); // src, dest[, mode] + const int argc = args.Length(); + CHECK_GE(argc, 3); // src, dest, flags[, req] Local src_val; if (!GetValidatedPath(env, args[0], "src").ToLocal(&src_val)) { @@ -2575,45 +2578,12 @@ static void CopyFileSync(const FunctionCallbackInfo& args) { return; } - Local mode = args.Length() > 2 ? args[2] : Undefined(isolate); - int flags; - if (!GetValidFileMode(env, mode, UV_FS_COPYFILE).To(&flags)) { - return; - } - - ToNamespacedPath(env, &src); - ToNamespacedPath(env, &dest); - - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, src.ToStringView()); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView()); - - FSReqWrapSync req_wrap_sync("copyfile", *src, *dest); - FS_SYNC_TRACE_BEGIN(copyfile); - SyncCallAndThrowOnError( - env, &req_wrap_sync, uv_fs_copyfile, *src, *dest, flags); - FS_SYNC_TRACE_END(copyfile); -} - -static void CopyFile(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - Isolate* isolate = env->isolate(); - - const int argc = args.Length(); - CHECK_GE(argc, 3); // src, dest, flags - int flags; if (!GetValidFileMode(env, args[2], UV_FS_COPYFILE).To(&flags)) { return; } - BufferValue src(isolate, args[0]); - CHECK_NOT_NULL(*src); ToNamespacedPath(env, &src); - - BufferValue dest(isolate, args[1]); - CHECK_NOT_NULL(*dest); ToNamespacedPath(env, &dest); if (argc > 3) { // copyFile(src, dest, flags, req) @@ -4409,7 +4379,6 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "writeFileUtf8", WriteFileUtf8); SetMethod(isolate, target, "realpath", RealPath); SetMethod(isolate, target, "copyFile", CopyFile); - SetMethod(isolate, target, "copyFileSync", CopyFileSync); SetMethod(isolate, target, "chmod", Chmod); SetMethod(isolate, target, "fchmod", FChmod); @@ -4538,7 +4507,6 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(WriteFileUtf8); registry->Register(RealPath); registry->Register(CopyFile); - registry->Register(CopyFileSync); registry->Register(CpSyncCheckPaths); registry->Register(CpSyncOverrideFile); diff --git a/test/parallel/test-fs-copyfile.js b/test/parallel/test-fs-copyfile.js index edae50fb6d84..b08772ac66f9 100644 --- a/test/parallel/test-fs-copyfile.js +++ b/test/parallel/test-fs-copyfile.js @@ -167,6 +167,14 @@ assert.throws(() => { message: 'The URL must be of scheme file', }); +assert.throws(() => { + fs.copyFile(new URL('http://example.com/a'), dest, common.mustNotCall()); +}, { + code: 'ERR_INVALID_URL_SCHEME', + name: 'TypeError', + message: 'The URL must be of scheme file', +}); + if (common.isWindows) { ['%2f', '%2F', '%5c', '%5C'].forEach((i) => { assert.throws( diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index 907a48c07bc4..2c1cfa8ec721 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -72,10 +72,10 @@ declare namespace InternalFSBinding { function close(fd: number, req: FSReqCallback): void; function close(fd: number): void; - function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: FSReqCallback): void; - function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: undefined, ctx: FSSyncContext): void; - function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, usePromises: typeof kUsePromises): Promise; - function copyFileSync(src: unknown, dest: unknown, mode?: unknown): void; + function copyFile(src: unknown, dest: unknown, mode?: unknown): void; + function copyFile(src: unknown, dest: unknown, mode: unknown, req: FSReqCallback): void; + function copyFile(src: unknown, dest: unknown, mode: unknown, req: undefined, ctx: FSSyncContext): void; + function copyFile(src: unknown, dest: unknown, mode: unknown, usePromises: typeof kUsePromises): Promise; function cpSyncCheckPaths(src: StringOrBuffer, dest: StringOrBuffer, dereference: boolean, recursive: boolean): void; function cpSyncOverrideFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, preserveTimestamps: boolean): void; @@ -262,7 +262,6 @@ export interface FsBinding { chown: typeof InternalFSBinding.chown; close: typeof InternalFSBinding.close; copyFile: typeof InternalFSBinding.copyFile; - copyFileSync: typeof InternalFSBinding.copyFileSync; cpSyncCheckPaths: typeof InternalFSBinding.cpSyncCheckPaths; cpSyncOverrideFile: typeof InternalFSBinding.cpSyncOverrideFile; cpSyncCopyDir: typeof InternalFSBinding.cpSyncCopyDir;