fs: improve mode and flags validation

This fixes a few bugs in `fs`. E.g., `fs.promises.access` accepted strings as mode. It should have only accepted numbers. It will now always validate the flags and the mode argument in an consistent way.
nodejs · BridgeAR · Sep 25, 2019 · Apr 1, 2019 · Sep 26, 2019 · Jan 11, 2020
commit 047fbc8f135d85a7007432797f33f2438e5f6185
diff --git a/doc/api/fs.md b/doc/api/fs.md
@@ -5493,8 +5493,8 @@ On Linux, positional writes don't work when the file is opened in append mode.
 The kernel ignores the position argument and always appends the data to
 the end of the file.
 
-Modifying a file rather than replacing it may require a flags mode of `'r+'`
-rather than the default mode `'w'`.
+Modifying a file rather than replacing it may require the `flag` option to be
+set to `'r+'` rather than the default `'w'`.
 
 The behavior of some flags are platform-specific. As such, opening a directory
 on macOS and Linux with the `'a+'` flag, as in the example below, will return an

diff --git a/lib/fs.js b/lib/fs.js
@@ -72,6 +72,7 @@ const {
   getDirents,
   getOptions,
   getValidatedPath,
+  getValidMode,
   handleErrorFromBinding,
   nullCheck,
   preprocessSymlinkDestination,
@@ -99,7 +100,7 @@ const {
 } = require('internal/constants');
 const {
   isUint32,
-  parseMode,
+  parseFileMode,
   validateBuffer,
   validateInteger,
   validateInt32,
@@ -183,20 +184,15 @@ function access(path, mode, callback) {
   }
 
   path = getValidatedPath(path);
-
-  mode = mode | 0;
+  mode = getValidMode(mode, 'access');
   const req = new FSReqCallback();
   req.oncomplete = makeCallback(callback);
   binding.access(pathModule.toNamespacedPath(path), mode, req);
 }
 
 function accessSync(path, mode) {
   path = getValidatedPath(path);
-
-  if (mode === undefined)
-    mode = F_OK;
-  else
-    mode = mode | 0;
+  mode = getValidMode(mode, 'access');
 
   const ctx = { path };
   binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx);
@@ -310,8 +306,9 @@ function readFile(path, options, callback) {
   }
 
   path = getValidatedPath(path);
+  const flagsNumber = stringToFlags(options.flags);
   binding.open(pathModule.toNamespacedPath(path),
-               stringToFlags(options.flag || 'r'),
+               flagsNumber,
                0o666,
                req);
 }
@@ -428,11 +425,10 @@ function open(path, flags, mode, callback) {
   } else if (typeof mode === 'function') {
     callback = mode;
     mode = 0o666;
+  } else {
+    mode = parseFileMode(mode, 'mode', 0o666);
   }
   const flagsNumber = stringToFlags(flags);
-  if (arguments.length >= 4) {
-    mode = parseMode(mode, 'mode', 0o666);
-  }
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -447,8 +443,8 @@ function open(path, flags, mode, callback) {
 
 function openSync(path, flags, mode) {
   path = getValidatedPath(path);
-  const flagsNumber = stringToFlags(flags || 'r');
-  mode = parseMode(mode, 'mode', 0o666);
+  const flagsNumber = stringToFlags(flags);
+  mode = parseFileMode(mode, 'mode', 0o666);
 
   const ctx = { path };
   const result = binding.open(pathModule.toNamespacedPath(path),
@@ -814,16 +810,18 @@ function fsyncSync(fd) {
 }
 
 function mkdir(path, options, callback) {
+  let mode = 0o777;
+  let recursive = false;
   if (typeof options === 'function') {
     callback = options;
-    options = {};
   } else if (typeof options === 'number' || typeof options === 'string') {
-    options = { mode: options };
+    mode = options;
+  } else if (options) {
+    if (options.recursive !== undefined)
+      recursive = options.recursive;
+    if (options.mode !== undefined)
+      mode = options.mode;
   }
-  const {
-    recursive = false,
-    mode = 0o777
-  } = options || {};
   callback = makeCallback(callback);
   path = getValidatedPath(path);
 
@@ -833,25 +831,27 @@ function mkdir(path, options, callback) {
   const req = new FSReqCallback();
   req.oncomplete = callback;
   binding.mkdir(pathModule.toNamespacedPath(path),
-                parseMode(mode, 'mode', 0o777), recursive, req);
+                parseFileMode(mode, 'mode'), recursive, req);
 }
 
 function mkdirSync(path, options) {
+  let mode = 0o777;
+  let recursive = false;
   if (typeof options === 'number' || typeof options === 'string') {
-    options = { mode: options };
+    mode = options;
+  } else if (options) {
+    if (options.recursive !== undefined)
+      recursive = options.recursive;
+    if (options.mode !== undefined)
+      mode = options.mode;
   }
-  const {
-    recursive = false,
-    mode = 0o777
-  } = options || {};
-
   path = getValidatedPath(path);
   if (typeof recursive !== 'boolean')
     throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive);
 
   const ctx = { path };
   binding.mkdir(pathModule.toNamespacedPath(path),
-                parseMode(mode, 'mode', 0o777), recursive, undefined,
+                parseFileMode(mode, 'mode'), recursive, undefined,
                 ctx);
   handleErrorFromBinding(ctx);
 }
@@ -1070,7 +1070,7 @@ function unlinkSync(path) {
 
 function fchmod(fd, mode, callback) {
   validateInt32(fd, 'fd', 0);
-  mode = parseMode(mode, 'mode');
+  mode = parseFileMode(mode, 'mode');
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -1080,7 +1080,7 @@ function fchmod(fd, mode, callback) {
 
 function fchmodSync(fd, mode) {
   validateInt32(fd, 'fd', 0);
-  mode = parseMode(mode, 'mode');
+  mode = parseFileMode(mode, 'mode');
   const ctx = {};
   binding.fchmod(fd, mode, undefined, ctx);
   handleErrorFromBinding(ctx);
@@ -1120,7 +1120,7 @@ function lchmodSync(path, mode) {
 
 function chmod(path, mode, callback) {
   path = getValidatedPath(path);
-  mode = parseMode(mode, 'mode');
+  mode = parseFileMode(mode, 'mode');
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -1130,7 +1130,7 @@ function chmod(path, mode, callback) {
 
 function chmodSync(path, mode) {
   path = getValidatedPath(path);
-  mode = parseMode(mode, 'mode');
+  mode = parseFileMode(mode, 'mode');
 
   const ctx = { path };
   binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx);
@@ -1791,10 +1791,10 @@ function mkdtempSync(prefix, options) {
 }
 
 
-function copyFile(src, dest, flags, callback) {
-  if (typeof flags === 'function') {
-    callback = flags;
-    flags = 0;
+function copyFile(src, dest, mode, callback) {
+  if (typeof mode === 'function') {
+    callback = mode;
+    mode = 0;
   } else if (typeof callback !== 'function') {
     throw new ERR_INVALID_CALLBACK(callback);
   }
@@ -1804,23 +1804,23 @@ function copyFile(src, dest, flags, callback) {
 
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
-  flags = flags | 0;
+  mode = getValidMode(mode, 'copyFile');
   const req = new FSReqCallback();
   req.oncomplete = makeCallback(callback);
-  binding.copyFile(src, dest, flags, req);
+  binding.copyFile(src, dest, mode, req);
 }
 
 
-function copyFileSync(src, dest, flags) {
+function copyFileSync(src, dest, mode) {
   src = getValidatedPath(src, 'src');
   dest = getValidatedPath(dest, 'dest');
 
   const ctx = { path: src, dest };  // non-prefixed
 
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
-  flags = flags | 0;
-  binding.copyFile(src, dest, flags, undefined, ctx);
+  mode = getValidMode(mode, 'copyFile');
+  binding.copyFile(src, dest, mode, undefined, ctx);
   handleErrorFromBinding(ctx);
 }
 

diff --git a/lib/internal/bootstrap/switches/does_own_process_state.js b/lib/internal/bootstrap/switches/does_own_process_state.js
@@ -23,7 +23,7 @@ if (credentials.implementsPosixCredentials) {
 // ----              compare the setups side-by-side                    -----
 
 const {
-  parseMode,
+  parseFileMode,
   validateString
 } = require('internal/validators');
 
@@ -119,7 +119,7 @@ function wrappedChdir(directory) {
 
 function wrappedUmask(mask) {
   if (mask !== undefined) {
-    mask = parseMode(mask, 'mask');
+    mask = parseFileMode(mask, 'mask');
   }
   return rawMethods.umask(mask);
 }

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -30,6 +30,7 @@ const {
   getOptions,
   getStatsFromBinding,
   getValidatedPath,
+  getValidMode,
   nullCheck,
   preprocessSymlinkDestination,
   stringToFlags,
@@ -43,7 +44,7 @@ const {
 } = require('internal/fs/utils');
 const { opendir } = require('internal/fs/dir');
 const {
-  parseMode,
+  parseFileMode,
   validateBuffer,
   validateInteger,
   validateUint32
@@ -189,27 +190,27 @@ async function readFileHandle(filehandle, options) {
 async function access(path, mode = F_OK) {
   path = getValidatedPath(path);
 
-  mode = mode | 0;
+  mode = getValidMode(mode, 'access');
   return binding.access(pathModule.toNamespacedPath(path), mode,
                         kUsePromises);
 }
 
-async function copyFile(src, dest, flags) {
+async function copyFile(src, dest, mode) {
   src = getValidatedPath(src, 'src');
   dest = getValidatedPath(dest, 'dest');
-  flags = flags | 0;
+  mode = getValidMode(mode, 'copyFile');
   return binding.copyFile(pathModule.toNamespacedPath(src),
                           pathModule.toNamespacedPath(dest),
-                          flags, kUsePromises);
+                          mode,
+                          kUsePromises);
 }
 
 // Note that unlike fs.open() which uses numeric file descriptors,
 // fsPromises.open() uses the fs.FileHandle class.
 async function open(path, flags, mode) {
   path = getValidatedPath(path);
-  if (arguments.length < 2) flags = 'r';
   const flagsNumber = stringToFlags(flags);
-  mode = parseMode(mode, 'mode', 0o666);
+  mode = parseFileMode(mode, 'mode', 0o666);
   return new FileHandle(
     await binding.openFileHandle(pathModule.toNamespacedPath(path),
                                  flagsNumber, mode, kUsePromises));
@@ -342,7 +343,7 @@ async function mkdir(path, options) {
     throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive);
 
   return binding.mkdir(pathModule.toNamespacedPath(path),
-                       parseMode(mode, 'mode', 0o777), recursive,
+                       parseFileMode(mode, 'mode', 0o777), recursive,
                        kUsePromises);
 }
 
@@ -410,13 +411,13 @@ async function unlink(path) {
 
 async function fchmod(handle, mode) {
   validateFileHandle(handle);
-  mode = parseMode(mode, 'mode');
+  mode = parseFileMode(mode, 'mode');
   return binding.fchmod(handle.fd, mode, kUsePromises);
 }
 
 async function chmod(path, mode) {
   path = getValidatedPath(path);
-  mode = parseMode(mode, 'mode');
+  mode = parseFileMode(mode, 'mode');
   return binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises);
 }