From 1cdeb9f956a9f54dde7c6b1a792b97584acbfb8e Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sun, 10 Mar 2019 23:06:33 +0100
Subject: [PATCH] fs: improve mode validation

Do not return a default mode in case invalid mode values are provided.
This strictens and simplifies the function by reusing existing
functionality.

PR-URL: https://github.com/nodejs/node/pull/26575
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
---
 lib/internal/validators.js      | 13 +++-------
 test/parallel/test-fs-fchmod.js |  4 +--
 test/parallel/test-fs-lchmod.js | 12 +++------
 test/parallel/test-fs-open.js   | 43 ++++++++++++++++++++++++---------
 4 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/lib/internal/validators.js b/lib/internal/validators.js
index 0ecf286266678a..6de46349c00fa8 100644
--- a/lib/internal/validators.js
+++ b/lib/internal/validators.js
@@ -35,24 +35,17 @@ function validateMode(value, name, def) {
   }
 
   if (typeof value === 'number') {
-    if (!Number.isInteger(value)) {
-      throw new ERR_OUT_OF_RANGE(name, 'an integer', value);
-    } else {
-      // 2 ** 32 === 4294967296
-      throw new ERR_OUT_OF_RANGE(name, '>= 0 && < 4294967296', value);
-    }
+    validateInt32(value, name, 0, 2 ** 32 - 1);
   }
 
   if (typeof value === 'string') {
     if (!octalReg.test(value)) {
       throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
     }
-    const parsed = parseInt(value, 8);
-    return parsed;
+    return parseInt(value, 8);
   }
 
-  // TODO(BridgeAR): Only return `def` in case `value == null`
-  if (def !== undefined) {
+  if (def !== undefined && value == null) {
     return def;
   }
 
diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js
index 4f6350e63c241a..06f4c0b272c551 100644
--- a/test/parallel/test-fs-fchmod.js
+++ b/test/parallel/test-fs-fchmod.js
@@ -46,8 +46,8 @@ const fs = require('fs');
   const errObj = {
     code: 'ERR_OUT_OF_RANGE',
     name: 'RangeError [ERR_OUT_OF_RANGE]',
-    message: 'The value of "mode" is out of range. It must be >= 0 && < ' +
-             `4294967296. Received ${input}`
+    message: 'The value of "mode" is out of range. It must be >= 0 && <= ' +
+             `4294967295. Received ${input}`
   };
 
   assert.throws(() => fs.fchmod(1, input), errObj);
diff --git a/test/parallel/test-fs-lchmod.js b/test/parallel/test-fs-lchmod.js
index 3238790152b70e..e84fa08aacc799 100644
--- a/test/parallel/test-fs-lchmod.js
+++ b/test/parallel/test-fs-lchmod.js
@@ -46,9 +46,7 @@ assert.throws(() => fs.lchmod(f, {}), { code: 'ERR_INVALID_CALLBACK' });
              `octal string. Received ${util.inspect(input)}`
   };
 
-  promises.lchmod(f, input, () => {})
-    .then(common.mustNotCall())
-    .catch(common.expectsError(errObj));
+  assert.rejects(promises.lchmod(f, input, () => {}), errObj);
   assert.throws(() => fs.lchmodSync(f, input), errObj);
 });
 
@@ -56,12 +54,10 @@ assert.throws(() => fs.lchmod(f, {}), { code: 'ERR_INVALID_CALLBACK' });
   const errObj = {
     code: 'ERR_OUT_OF_RANGE',
     name: 'RangeError [ERR_OUT_OF_RANGE]',
-    message: 'The value of "mode" is out of range. It must be >= 0 && < ' +
-             `4294967296. Received ${input}`
+    message: 'The value of "mode" is out of range. It must be >= 0 && <= ' +
+             `4294967295. Received ${input}`
   };
 
-  promises.lchmod(f, input, () => {})
-    .then(common.mustNotCall())
-    .catch(common.expectsError(errObj));
+  assert.rejects(promises.lchmod(f, input, () => {}), errObj);
   assert.throws(() => fs.lchmodSync(f, input), errObj);
 });
diff --git a/test/parallel/test-fs-open.js b/test/parallel/test-fs-open.js
index 9a1ae623fea302..79b78ff2ef14d0 100644
--- a/test/parallel/test-fs-open.js
+++ b/test/parallel/test-fs-open.js
@@ -98,15 +98,36 @@ for (const extra of [[], ['r'], ['r', 0], ['r', 0, 'bad callback']]) {
       type: TypeError
     }
   );
-  fs.promises.open(i, 'r')
-    .then(common.mustNotCall())
-    .catch(common.mustCall((err) => {
-      common.expectsError(
-        () => { throw err; },
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError
-        }
-      );
-    }));
+  assert.rejects(
+    fs.promises.open(i, 'r'),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError [ERR_INVALID_ARG_TYPE]'
+    }
+  );
+});
+
+// Check invalid modes.
+[false, [], {}].forEach((mode) => {
+  assert.throws(
+    () => fs.open(__filename, 'r', mode, common.mustNotCall()),
+    {
+      message: /'mode' must be a 32-bit/,
+      code: 'ERR_INVALID_ARG_VALUE'
+    }
+  );
+  assert.throws(
+    () => fs.openSync(__filename, 'r', mode, common.mustNotCall()),
+    {
+      message: /'mode' must be a 32-bit/,
+      code: 'ERR_INVALID_ARG_VALUE'
+    }
+  );
+  assert.rejects(
+    fs.promises.open(__filename, 'r', mode),
+    {
+      message: /'mode' must be a 32-bit/,
+      code: 'ERR_INVALID_ARG_VALUE'
+    }
+  );
 });