fs: improve mode and flags validation

This fixes a bug in `fs.promises.access` as it 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.

lib/fs.js

@@ -182,7 +182,7 @@ function access(path, mode, callback) {
   path = toPathIfFileURL(path);
   validatePath(path);
 
-  mode = mode | 0;
+  mode = parseMode(mode, 'mode');
   const req = new FSReqCallback();
   req.oncomplete = makeCallback(callback);
   binding.access(pathModule.toNamespacedPath(path), mode, req);
@@ -192,10 +192,11 @@ function accessSync(path, mode) {
   path = toPathIfFileURL(path);
   validatePath(path);
 
-  if (mode === undefined)
+  if (mode === undefined) {
     mode = F_OK;
-  else
-    mode = mode | 0;
+  } else {
+    mode = parseMode(mode, 'mode');
+  }
 
   const ctx = { path };
   binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx);
@@ -304,8 +305,9 @@ function readFile(path, options, callback) {
 
   path = toPathIfFileURL(path);
   validatePath(path);
+  const flagsNumber = stringToFlags(options.flags);
   binding.open(pathModule.toNamespacedPath(path),
-               stringToFlags(options.flag || 'r'),
+               flagsNumber,
                0o666,
                req);
 }
@@ -423,11 +425,10 @@ function open(path, flags, mode, callback) {
   } else if (typeof mode === 'function') {
     callback = mode;
     mode = 0o666;
-  }
-  const flagsNumber = stringToFlags(flags);
-  if (arguments.length >= 4) {
+  } else {
     mode = parseMode(mode, 'mode', 0o666);
   }
+  const flagsNumber = stringToFlags(flags);
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -443,7 +444,7 @@ function open(path, flags, mode, callback) {
 function openSync(path, flags, mode) {
   path = toPathIfFileURL(path);
   validatePath(path);
-  const flagsNumber = stringToFlags(flags || 'r');
+  const flagsNumber = stringToFlags(flags);
   mode = parseMode(mode, 'mode', 0o666);
 
   const ctx = { path };
@@ -1757,10 +1758,10 @@ function copyFile(src, dest, flags, callback) {
 
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
-  flags = flags | 0;
+  const flagsNumber = stringToFlags(flags);
   const req = new FSReqCallback();
   req.oncomplete = makeCallback(callback);
-  binding.copyFile(src, dest, flags, req);
+  binding.copyFile(src, dest, flagsNumber, req);
 }
 
 
@@ -1774,8 +1775,8 @@ function copyFileSync(src, dest, flags) {
 
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
-  flags = flags | 0;
-  binding.copyFile(src, dest, flags, undefined, ctx);
+  const flagsNumber = stringToFlags(flags);
+  binding.copyFile(src, dest, flagsNumber, undefined, ctx);
   handleErrorFromBinding(ctx);
 }
 

lib/internal/fs/promises.js

@@ -175,7 +175,7 @@ async function access(path, mode = F_OK) {
   path = toPathIfFileURL(path);
   validatePath(path);
 
-  mode = mode | 0;
+  mode = parseMode(mode, 'mode');
   return binding.access(pathModule.toNamespacedPath(path), mode,
                         kUsePromises);
 }
@@ -185,18 +185,18 @@ async function copyFile(src, dest, flags) {
   dest = toPathIfFileURL(dest);
   validatePath(src, 'src');
   validatePath(dest, 'dest');
-  flags = flags | 0;
+  const flagsNumber = stringToFlags(flags);
   return binding.copyFile(pathModule.toNamespacedPath(src),
                           pathModule.toNamespacedPath(dest),
-                          flags, kUsePromises);
+                          flagsNumber,
+                          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 = toPathIfFileURL(path);
   validatePath(path);
-  if (arguments.length < 2) flags = 'r';
   const flagsNumber = stringToFlags(flags);
   mode = parseMode(mode, 'mode', 0o666);
   return new FileHandle(

lib/internal/fs/utils.js

@@ -322,6 +322,10 @@ function stringToFlags(flags) {
     return flags;
   }
 
+  if (flags == null) {
+    return O_RDONLY;
+  }
+
   switch (flags) {
     case 'r' : return O_RDONLY;
     case 'rs' : // Fall through.

test/parallel/test-fs-open-flags.js

@@ -84,11 +84,6 @@ common.expectsError(
   { code: 'ERR_INVALID_OPT_VALUE', type: TypeError }
 );
 
-common.expectsError(
-  () => stringToFlags(null),
-  { code: 'ERR_INVALID_OPT_VALUE', type: TypeError }
-);
-
 if (common.isLinux || common.isOSX) {
   const tmpdir = require('../common/tmpdir');
   tmpdir.refresh();

test/parallel/test-fs-promises-file-handle-sync.js

@@ -7,11 +7,12 @@ const tmpdir = require('../common/tmpdir');
 const { access, copyFile, open } = require('fs').promises;
 const path = require('path');
 
-async function validateSync() {
+async function validate() {
   tmpdir.refresh();
   const dest = path.resolve(tmpdir.path, 'baz.js');
   await copyFile(fixtures.path('baz.js'), dest);
-  await access(dest, 'r');
+  await assert.rejects(access(dest, 'r'), { code: 'ERR_INVALID_ARG_VALUE' });
+  await access(dest);
   const handle = await open(dest, 'r+');
   await handle.datasync();
   await handle.sync();
@@ -22,4 +23,4 @@ async function validateSync() {
   assert.deepStrictEqual(ret.buffer, buf);
 }
 
-validateSync();
+validate();

test/parallel/test-fs-promises.js

@@ -45,17 +45,17 @@ function nextdir() {
 assert.strictEqual(Object.keys(fs).includes('promises'), false);
 
 {
-  access(__filename, 'r')
+  access(__filename, 0)
     .then(common.mustCall());
 
-  access('this file does not exist', 'r')
-    .then(common.mustNotCall())
-    .catch(common.expectsError({
+  assert.rejects(
+    access('this file does not exist', 0),
+    {
       code: 'ENOENT',
-      type: Error,
-      message:
-        /^ENOENT: no such file or directory, access/
-    }));
+      name: 'Error',
+      message: /^ENOENT: no such file or directory, access/
+    }
+  );
 }
 
 function verifyStatObject(stat) {
@@ -66,7 +66,7 @@ function verifyStatObject(stat) {
 
 async function getHandle(dest) {
   await copyFile(fixtures.path('baz.js'), dest);
-  await access(dest, 'r');
+  await access(dest);
 
   return open(dest, 'r+');
 }