From 1c16fde60fe9e67c3ea1f62bfdec41e43d3ed199 Mon Sep 17 00:00:00 2001 From: Daniel Osvaldo R Date: Tue, 8 Jul 2025 17:30:45 +0700 Subject: [PATCH] fix(ext/node): don't throw error on invalid path input on `exists` and `existsSync` (#29971) Currently `fs.exists` and `fs.existsSync` throws error when invalid path is given to the input. The expected behavior is to: - Call the callback with false on `fs.exists`. - Return false on `fs.existsSync`. Towards #29972, #24236 --- ext/node/polyfills/_fs/_fs_common.ts | 4 +-- ext/node/polyfills/_fs/_fs_exists.ts | 51 ++++++++++++++++++---------- ext/node/polyfills/internal/util.mjs | 4 ++- tests/node_compat/config.toml | 2 ++ 4 files changed, 40 insertions(+), 21 deletions(-) diff --git a/ext/node/polyfills/_fs/_fs_common.ts b/ext/node/polyfills/_fs/_fs_common.ts index e8605ec60e..660ccecea2 100644 --- a/ext/node/polyfills/_fs/_fs_common.ts +++ b/ext/node/polyfills/_fs/_fs_common.ts @@ -253,9 +253,9 @@ export function maybeCallback(cb: unknown) { // Ensure that callbacks run in the global context. Only use this function // for callbacks that are passed to the binding layer, callbacks that are // invoked from JS already run in the proper scope. -export function makeCallback( +export function makeCallback( this: unknown, - cb?: (err: Error | null, result?: unknown) => void, + cb?: (arg: T) => void, ) { validateFunction(cb, "cb"); diff --git a/ext/node/polyfills/_fs/_fs_exists.ts b/ext/node/polyfills/_fs/_fs_exists.ts index 6a285f5e60..18591d84fe 100644 --- a/ext/node/polyfills/_fs/_fs_exists.ts +++ b/ext/node/polyfills/_fs/_fs_exists.ts @@ -1,41 +1,56 @@ // Copyright 2018-2025 the Deno authors. MIT license. -// TODO(petamoriken): enable prefer-primordials for node polyfills -// deno-lint-ignore-file prefer-primordials - import { op_node_fs_exists, op_node_fs_exists_sync } from "ext:core/ops"; +import { getValidatedPathToString } from "ext:deno_node/internal/fs/utils.mjs"; +import { primordials } from "ext:core/mod.js"; +import { makeCallback } from "ext:deno_node/_fs/_fs_common.ts"; +import type { Buffer } from "node:buffer"; +import * as pathModule from "node:path"; +import { kCustomPromisifiedSymbol } from "ext:deno_node/internal/util.mjs"; -import { pathFromURL } from "ext:deno_web/00_infra.js"; +const { ObjectDefineProperty, Promise, PromisePrototypeThen } = primordials; type ExistsCallback = (exists: boolean) => void; /** - * TODO: Also accept 'path' parameter as a Node polyfill Buffer type once these - * are implemented. See https://github.com/denoland/deno/issues/3403 * Deprecated in node api */ -export function exists(path: string | URL, callback: ExistsCallback) { - path = path instanceof URL ? pathFromURL(path) : path; - op_node_fs_exists(path).then(callback); +export function exists(path: string | Buffer | URL, callback: ExistsCallback) { + callback = makeCallback(callback); + + try { + path = getValidatedPathToString(path); + } catch { + callback(false); + return; + } + + PromisePrototypeThen( + op_node_fs_exists(pathModule.toNamespacedPath(path)), + callback, + ); } // The callback of fs.exists doesn't have standard callback signature. // We need to provide special implementation for promisify. // See https://github.com/nodejs/node/pull/13316 -const kCustomPromisifiedSymbol = Symbol.for("nodejs.util.promisify.custom"); -Object.defineProperty(exists, kCustomPromisifiedSymbol, { +ObjectDefineProperty(exists, kCustomPromisifiedSymbol, { + __proto__: null, value: (path: string | URL) => { return new Promise((resolve) => { exists(path, (exists) => resolve(exists)); }); }, + enumerable: false, + writable: false, + configurable: true, }); -/** - * TODO: Also accept 'path' parameter as a Node polyfill Buffer or URL type once these - * are implemented. See https://github.com/denoland/deno/issues/3403 - */ -export function existsSync(path: string | URL): boolean { - path = path instanceof URL ? pathFromURL(path) : path; - return op_node_fs_exists_sync(path); +export function existsSync(path: string | Buffer | URL): boolean { + try { + path = getValidatedPathToString(path); + } catch { + return false; + } + return op_node_fs_exists_sync(pathModule.toNamespacedPath(path)); } diff --git a/ext/node/polyfills/internal/util.mjs b/ext/node/polyfills/internal/util.mjs index e670a7c36c..e104d36a71 100644 --- a/ext/node/polyfills/internal/util.mjs +++ b/ext/node/polyfills/internal/util.mjs @@ -46,7 +46,9 @@ export function once(callback) { // In addition to being accessible through util.promisify.custom, // this symbol is registered globally and can be accessed in any environment as // Symbol.for('nodejs.util.promisify.custom'). -const kCustomPromisifiedSymbol = SymbolFor("nodejs.util.promisify.custom"); +export const kCustomPromisifiedSymbol = SymbolFor( + "nodejs.util.promisify.custom", +); // This is an internal Node symbol used by functions returning multiple // arguments, e.g. ['bytesRead', 'buffer'] for fs.read(). const kCustomPromisifyArgsSymbol = SymbolFor( diff --git a/tests/node_compat/config.toml b/tests/node_compat/config.toml index ad0796593e..a0566a8f71 100644 --- a/tests/node_compat/config.toml +++ b/tests/node_compat/config.toml @@ -424,6 +424,8 @@ "parallel/test-fs-close.js" = {} "parallel/test-fs-constants.js" = {} "parallel/test-fs-empty-readStream.js" = {} +"parallel/test-fs-exists.js" = {} +"parallel/test-fs-existssync-false.js" = {} "parallel/test-fs-fchown.js" = {} "parallel/test-fs-long-path.js" = {} "parallel/test-fs-promises-exists.js" = {}