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: nodejs#49741 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
bmeck · Jun 22, 2024 · 82d9e91 · 82d9e91
1 parent acd50ee
commit 82d9e91
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 43 deletions.
diff --git 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`.
 
-<a id="ERR_FS_INVALID_SYMLINK_TYPE"></a>
-
-### `ERR_FS_INVALID_SYMLINK_TYPE`
-
-An invalid symlink type was passed to the [`fs.symlink()`][] or
-[`fs.symlinkSync()`][] methods.
-
 <a id="ERR_HTTP_HEADERS_SENT"></a>
 
 ### `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.
 
+<a id="ERR_FS_INVALID_SYMLINK_TYPE"></a>
+
+### `ERR_FS_INVALID_SYMLINK_TYPE`
+
+<!-- YAML
+removed: REPLACEME
+-->
+
+An invalid symlink type was passed to the [`fs.symlink()`][] or
+[`fs.symlinkSync()`][] methods.
+
 <a id="ERR_HTTP2_FRAME_ERROR"></a>
 
 ### `ERR_HTTP2_FRAME_ERROR`

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
 });