From f202322ea46e58baf1b3d064d075b65a591a94e9 Mon Sep 17 00:00:00 2001 From: Livia Medeiros Date: Mon, 6 May 2024 22:18:42 +0900 Subject: [PATCH] fs: adjust typecheck for `type` in `fs.symlink()` Throws `TypeError` instead of `Error` Enables autodetection on Windows if `type === undefined` Explicitly disallows unknown strings and non-string values PR-URL: https://github.com/nodejs/node/pull/49741 Reviewed-By: Antoine du Hamel Reviewed-By: Luigi Pinca Reviewed-By: Rafael Gonzaga Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- doc/api/errors.md | 18 +++++++++++------- doc/api/fs.md | 4 ++-- lib/fs.js | 21 +++++++++++++-------- lib/internal/errors.js | 3 --- lib/internal/fs/promises.js | 7 ++++--- lib/internal/fs/utils.js | 25 +++++++++---------------- test/parallel/test-fs-symlink.js | 21 +++++++++++++++++---- 7 files changed, 56 insertions(+), 43 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 48dc1f1e392c93..53963080da39bb 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1377,13 +1377,6 @@ Path is a directory. An attempt has been made to read a file whose size is larger than the maximum allowed size for a `Buffer`. - - -### `ERR_FS_INVALID_SYMLINK_TYPE` - -An invalid symlink type was passed to the [`fs.symlink()`][] or -[`fs.symlinkSync()`][] methods. - ### `ERR_HTTP_HEADERS_SENT` @@ -3276,6 +3269,17 @@ The UTF-16 encoding was used with [`hash.digest()`][]. While the causing the method to return a string rather than a `Buffer`, the UTF-16 encoding (e.g. `ucs` or `utf16le`) is not supported. + + +### `ERR_FS_INVALID_SYMLINK_TYPE` + + + +An invalid symlink type was passed to the [`fs.symlink()`][] or +[`fs.symlinkSync()`][] methods. + ### `ERR_HTTP2_FRAME_ERROR` diff --git a/doc/api/fs.md b/doc/api/fs.md index 18138be8a77ee9..9dc7f2c8cf940e 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1693,7 +1693,7 @@ changes: Creates a symbolic link. The `type` argument is only used on Windows platforms and can be one of `'dir'`, -`'file'`, or `'junction'`. If the `type` argument is not a string, Node.js will +`'file'`, or `'junction'`. If the `type` argument is `null`, Node.js will autodetect `target` type and use `'file'` or `'dir'`. If the `target` does not exist, `'file'` will be used. Windows junction points require the destination path to be absolute. When using `'junction'`, the `target` argument will @@ -4444,7 +4444,7 @@ See the POSIX symlink(2) documentation for more details. The `type` argument is only available on Windows and ignored on other platforms. It can be set to `'dir'`, `'file'`, or `'junction'`. If the `type` argument is -not a string, Node.js will autodetect `target` type and use `'file'` or `'dir'`. +`null`, Node.js will autodetect `target` type and use `'file'` or `'dir'`. If the `target` does not exist, `'file'` will be used. Windows junction points require the destination path to be absolute. When using `'junction'`, the `target` argument will automatically be normalized to absolute path. Junction diff --git a/lib/fs.js b/lib/fs.js index a16fcf426a641d..cc27b1c93a4850 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -142,6 +142,7 @@ const { validateFunction, validateInteger, validateObject, + validateOneOf, validateString, kValidateObjectAllowNullable, } = require('internal/validators'); @@ -1715,13 +1716,17 @@ function readlinkSync(path, options) { * Creates the link called `path` pointing to `target`. * @param {string | Buffer | URL} target * @param {string | Buffer | URL} path - * @param {string | null} [type_] - * @param {(err?: Error) => any} callback_ + * @param {string | null} [type] + * @param {(err?: Error) => any} callback * @returns {void} */ -function symlink(target, path, type_, callback_) { - const type = (typeof type_ === 'string' ? type_ : null); - const callback = makeCallback(arguments[arguments.length - 1]); +function symlink(target, path, type, callback) { + if (callback === undefined) { + callback = makeCallback(type); + type = undefined; + } else { + validateOneOf(type, 'type', ['dir', 'file', 'junction', null, undefined]); + } if (permission.isEnabled()) { // The permission model's security guarantees fall apart in the presence of @@ -1740,7 +1745,7 @@ function symlink(target, path, type_, callback_) { target = getValidatedPath(target, 'target'); path = getValidatedPath(path); - if (isWindows && type === null) { + if (isWindows && type == null) { let absoluteTarget; try { // Symlinks targets can be relative to the newly created path. @@ -1786,8 +1791,8 @@ function symlink(target, path, type_, callback_) { * @returns {void} */ function symlinkSync(target, path, type) { - type = (typeof type === 'string' ? type : null); - if (isWindows && type === null) { + validateOneOf(type, 'type', ['dir', 'file', 'junction', null, undefined]); + if (isWindows && type == null) { const absoluteTarget = pathModule.resolve(`${path}`, '..', `${target}`); if (statSync(absoluteTarget, { throwIfNoEntry: false })?.isDirectory()) { type = 'dir'; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index e29b7eb58638e6..530afc07f7bb8a 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1217,9 +1217,6 @@ E('ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY', E('ERR_FS_CP_UNKNOWN', 'Cannot copy an unknown file type', SystemError); E('ERR_FS_EISDIR', 'Path is a directory', SystemError, HideStackFramesError); E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than 2 GiB', RangeError); -E('ERR_FS_INVALID_SYMLINK_TYPE', - 'Symlink type must be one of "dir", "file", or "junction". Received "%s"', - Error); // Switch to TypeError. The current implementation does not seem right E('ERR_HTTP2_ALTSVC_INVALID_ORIGIN', 'HTTP/2 ALTSVC frames require a valid origin', TypeError); E('ERR_HTTP2_ALTSVC_LENGTH', diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 3b6bc934810ccd..c1842fdb4640cd 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -85,6 +85,7 @@ const { validateEncoding, validateInteger, validateObject, + validateOneOf, validateString, kValidateObjectAllowNullable, } = require('internal/validators'); @@ -973,9 +974,9 @@ async function readlink(path, options) { ); } -async function symlink(target, path, type_) { - let type = (typeof type_ === 'string' ? type_ : null); - if (isWindows && type === null) { +async function symlink(target, path, type) { + validateOneOf(type, 'type', ['dir', 'file', 'junction', null, undefined]); + if (isWindows && type == null) { try { const absoluteTarget = pathModule.resolve(`${path}`, '..', `${target}`); type = (await stat(absoluteTarget)).isDirectory() ? 'dir' : 'file'; diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index a5f1b499e3b9c3..f1d2d28dbfd681 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -31,7 +31,6 @@ const { UVException, codes: { ERR_FS_EISDIR, - ERR_FS_INVALID_SYMLINK_TYPE, ERR_INCOMPATIBLE_OPTION_PAIR, ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, @@ -647,22 +646,16 @@ function stringToFlags(flags, name = 'flags') { } const stringToSymlinkType = hideStackFrames((type) => { - let flags = 0; - if (typeof type === 'string') { - switch (type) { - case 'dir': - flags |= UV_FS_SYMLINK_DIR; - break; - case 'junction': - flags |= UV_FS_SYMLINK_JUNCTION; - break; - case 'file': - break; - default: - throw new ERR_FS_INVALID_SYMLINK_TYPE(type); - } + switch (type) { + case undefined: + case null: + case 'file': + return 0; + case 'dir': + return UV_FS_SYMLINK_DIR; + case 'junction': + return UV_FS_SYMLINK_JUNCTION; } - return flags; }); // converts Date or number to a fractional UNIX timestamp diff --git a/test/parallel/test-fs-symlink.js b/test/parallel/test-fs-symlink.js index ad4d6d1410d694..de122020f0da6f 100644 --- a/test/parallel/test-fs-symlink.js +++ b/test/parallel/test-fs-symlink.js @@ -76,14 +76,27 @@ fs.symlink(linkData, linkPath, common.mustSucceed(() => { }); const errObj = { - code: 'ERR_FS_INVALID_SYMLINK_TYPE', - name: 'Error', - message: - 'Symlink type must be one of "dir", "file", or "junction". Received "🍏"' + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', }; assert.throws(() => fs.symlink('', '', '🍏', common.mustNotCall()), errObj); assert.throws(() => fs.symlinkSync('', '', '🍏'), errObj); +assert.throws(() => fs.symlink('', '', 'nonExistentType', common.mustNotCall()), errObj); +assert.throws(() => fs.symlinkSync('', '', 'nonExistentType'), errObj); +assert.rejects(() => fs.promises.symlink('', '', 'nonExistentType'), errObj) + .then(common.mustCall()); + +assert.throws(() => fs.symlink('', '', false, common.mustNotCall()), errObj); +assert.throws(() => fs.symlinkSync('', '', false), errObj); +assert.rejects(() => fs.promises.symlink('', '', false), errObj) + .then(common.mustCall()); + +assert.throws(() => fs.symlink('', '', {}, common.mustNotCall()), errObj); +assert.throws(() => fs.symlinkSync('', '', {}), errObj); +assert.rejects(() => fs.promises.symlink('', '', {}), errObj) + .then(common.mustCall()); + process.on('exit', () => { assert.notStrictEqual(linkTime, fileTime); });