lib: mask mode_t type of arguments with 0o777

- Introduce the `validateAndMaskMode` validator that
  validates `mode_t` arguments and mask them with 0o777
  if they are 32-bit unsigned integer or octal string
  to be more consistent with POSIX APIs.
- Use the validator in fs APIs and process.umask for
  consistency.
- Add tests for 32-bit unsigned modes larger than 0o777.

PR-URL: https://github.com/nodejs/node/pull/20636
Fixes: https://github.com/nodejs/node/issues/20498
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

8 files changed, 245 insertions, 59 deletions

diff --git a/test/parallel/test-fs-chmod-mask.js b/test/parallel/test-fs-chmod-mask.js
new file mode 100644
index 0000000000..5f3a8d5ab8
--- /dev/null
+++ b/test/parallel/test-fs-chmod-mask.js
@@ -0,0 +1,95 @@
+'use strict';
+
+// This tests that mode > 0o777 will be masked off with 0o777 in fs APIs.
+
+const common = require('../common');
+const assert = require('assert');
+const path = require('path');
+const fs = require('fs');
+
+let mode;
+// On Windows chmod is only able to manipulate write permission
+if (common.isWindows) {
+  mode = 0o444;  // read-only
+} else {
+  mode = 0o777;
+}
+
+const maskToIgnore = 0o10000;
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+function test(mode, asString) {
+  const suffix = asString ? 'str' : 'num';
+  const input = asString ?
+    (mode | maskToIgnore).toString(8) : (mode | maskToIgnore);
+
+  {
+    const file = path.join(tmpdir.path, `chmod-async-${suffix}.txt`);
+    fs.writeFileSync(file, 'test', 'utf-8');
+
+    fs.chmod(file, input, common.mustCall((err) => {
+      assert.ifError(err);
+      assert.strictEqual(fs.statSync(file).mode & 0o777, mode);
+    }));
+  }
+
+  {
+    const file = path.join(tmpdir.path, `chmodSync-${suffix}.txt`);
+    fs.writeFileSync(file, 'test', 'utf-8');
+
+    fs.chmodSync(file, input);
+    assert.strictEqual(fs.statSync(file).mode & 0o777, mode);
+  }
+
+  {
+    const file = path.join(tmpdir.path, `fchmod-async-${suffix}.txt`);
+    fs.writeFileSync(file, 'test', 'utf-8');
+    fs.open(file, 'w', common.mustCall((err, fd) => {
+      assert.ifError(err);
+
+      fs.fchmod(fd, input, common.mustCall((err) => {
+        assert.ifError(err);
+        assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode);
+        fs.close(fd, assert.ifError);
+      }));
+    }));
+  }
+
+  {
+    const file = path.join(tmpdir.path, `fchmodSync-${suffix}.txt`);
+    fs.writeFileSync(file, 'test', 'utf-8');
+    const fd = fs.openSync(file, 'w');
+
+    fs.fchmodSync(fd, input);
+    assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode);
+
+    fs.close(fd, assert.ifError);
+  }
+
+  if (fs.lchmod) {
+    const link = path.join(tmpdir.path, `lchmod-src-${suffix}`);
+    const file = path.join(tmpdir.path, `lchmod-dest-${suffix}`);
+    fs.writeFileSync(file, 'test', 'utf-8');
+    fs.symlinkSync(file, link);
+
+    fs.lchmod(link, input, common.mustCall((err) => {
+      assert.ifError(err);
+      assert.strictEqual(fs.lstatSync(link).mode & 0o777, mode);
+    }));
+  }
+
+  if (fs.lchmodSync) {
+    const link = path.join(tmpdir.path, `lchmodSync-src-${suffix}`);
+    const file = path.join(tmpdir.path, `lchmodSync-dest-${suffix}`);
+    fs.writeFileSync(file, 'test', 'utf-8');
+    fs.symlinkSync(file, link);
+
+    fs.lchmodSync(link, input);
+    assert.strictEqual(fs.lstatSync(link).mode & 0o777, mode);
+  }
+}
+
+test(mode, true);
+test(mode, false);
diff --git a/test/parallel/test-fs-chmod.js b/test/parallel/test-fs-chmod.js
index 6727ec2144..ed5919ce88 100644
--- a/test/parallel/test-fs-chmod.js
+++ b/test/parallel/test-fs-chmod.js
@@ -62,7 +62,7 @@ function closeSync() {
 }
 
 
-// On Windows chmod is only able to manipulate read-only bit
+// On Windows chmod is only able to manipulate write permission
 if (common.isWindows) {
   mode_async = 0o400;   // read-only
   mode_sync = 0o600;    // read-write
@@ -112,10 +112,10 @@ fs.open(file2, 'w', common.mustCall((err, fd) => {
     common.expectsError(
       () => fs.fchmod(fd, {}),
       {
-        code: 'ERR_INVALID_ARG_TYPE',
+        code: 'ERR_INVALID_ARG_VALUE',
         type: TypeError,
-        message: 'The "mode" argument must be of type number. ' +
-                 'Received type object'
+        message: 'The argument \'mode\' must be a 32-bit unsigned integer ' +
+                 'or an octal string. Received {}'
       }
     );
 
diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js
index 63d780a57d..df7748538a 100644
--- a/test/parallel/test-fs-fchmod.js
+++ b/test/parallel/test-fs-fchmod.js
@@ -1,6 +1,7 @@
 'use strict';
-const common = require('../common');
+require('../common');
 const assert = require('assert');
+const util = require('util');
 const fs = require('fs');
 
 // This test ensures that input for fchmod is valid, testing for valid
@@ -16,7 +17,16 @@ const fs = require('fs');
   };
   assert.throws(() => fs.fchmod(input), errObj);
   assert.throws(() => fs.fchmodSync(input), errObj);
-  errObj.message = errObj.message.replace('fd', 'mode');
+});
+
+
+[false, null, undefined, {}, [], '', '123x'].forEach((input) => {
+  const errObj = {
+    code: 'ERR_INVALID_ARG_VALUE',
+    name: 'TypeError [ERR_INVALID_ARG_VALUE]',
+    message: 'The argument \'mode\' must be a 32-bit unsigned integer or an ' +
+             `octal string. Received ${util.inspect(input)}`
+  };
   assert.throws(() => fs.fchmod(1, input), errObj);
   assert.throws(() => fs.fchmodSync(1, input), errObj);
 });
@@ -62,27 +72,3 @@ const fs = require('fs');
   assert.throws(() => fs.fchmod(1, input), errObj);
   assert.throws(() => fs.fchmodSync(1, input), errObj);
 });
-
-// Check for mode values range
-const modeUpperBoundaryValue = 0o777;
-fs.fchmod(1, modeUpperBoundaryValue, common.mustCall());
-fs.fchmodSync(1, modeUpperBoundaryValue);
-
-// umask of 0o777 is equal to 775
-const modeOutsideUpperBoundValue = 776;
-assert.throws(
-  () => fs.fchmod(1, modeOutsideUpperBoundValue),
-  {
-    code: 'ERR_OUT_OF_RANGE',
-    name: 'RangeError [ERR_OUT_OF_RANGE]',
-    message: 'The value of "mode" is out of range. Received 776'
-  }
-);
-assert.throws(
-  () => fs.fchmodSync(1, modeOutsideUpperBoundValue),
-  {
-    code: 'ERR_OUT_OF_RANGE',
-    name: 'RangeError [ERR_OUT_OF_RANGE]',
-    message: 'The value of "mode" is out of range. Received 776'
-  }
-);
diff --git a/test/parallel/test-fs-mkdir-mode-mask.js b/test/parallel/test-fs-mkdir-mode-mask.js
new file mode 100644
index 0000000000..e4e8a42348
--- /dev/null
+++ b/test/parallel/test-fs-mkdir-mode-mask.js
@@ -0,0 +1,45 @@
+'use strict';
+
+// This tests that mode > 0o777 will be masked off with 0o777 in fs.mkdir().
+
+const common = require('../common');
+const assert = require('assert');
+const path = require('path');
+const fs = require('fs');
+
+let mode;
+
+if (common.isWindows) {
+  common.skip('mode is not supported in mkdir on Windows');
+  return;
+} else {
+  mode = 0o644;
+}
+
+const maskToIgnore = 0o10000;
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+function test(mode, asString) {
+  const suffix = asString ? 'str' : 'num';
+  const input = asString ?
+    (mode | maskToIgnore).toString(8) : (mode | maskToIgnore);
+
+  {
+    const dir = path.join(tmpdir.path, `mkdirSync-${suffix}`);
+    fs.mkdirSync(dir, input);
+    assert.strictEqual(fs.statSync(dir).mode & 0o777, mode);
+  }
+
+  {
+    const dir = path.join(tmpdir.path, `mkdir-${suffix}`);
+    fs.mkdir(dir, input, common.mustCall((err) => {
+      assert.ifError(err);
+      assert.strictEqual(fs.statSync(dir).mode & 0o777, mode);
+    }));
+  }
+}
+
+test(mode, true);
+test(mode, false);
diff --git a/test/parallel/test-fs-open-mode-mask.js b/test/parallel/test-fs-open-mode-mask.js
new file mode 100644
index 0000000000..4db41864af
--- /dev/null
+++ b/test/parallel/test-fs-open-mode-mask.js
@@ -0,0 +1,48 @@
+'use strict';
+
+// This tests that mode > 0o777 will be masked off with 0o777 in fs.open().
+
+const common = require('../common');
+const assert = require('assert');
+const path = require('path');
+const fs = require('fs');
+
+let mode;
+
+if (common.isWindows) {
+  mode = 0o444;
+} else {
+  mode = 0o644;
+}
+
+const maskToIgnore = 0o10000;
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+function test(mode, asString) {
+  const suffix = asString ? 'str' : 'num';
+  const input = asString ?
+    (mode | maskToIgnore).toString(8) : (mode | maskToIgnore);
+
+  {
+    const file = path.join(tmpdir.path, `openSync-${suffix}.txt`);
+    const fd = fs.openSync(file, 'w+', input);
+    assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode);
+    fs.closeSync(fd);
+    assert.strictEqual(fs.statSync(file).mode & 0o777, mode);
+  }
+
+  {
+    const file = path.join(tmpdir.path, `open-${suffix}.txt`);
+    fs.open(file, 'w+', input, common.mustCall((err, fd) => {
+      assert.ifError(err);
+      assert.strictEqual(fs.fstatSync(fd).mode & 0o777, mode);
+      fs.closeSync(fd);
+      assert.strictEqual(fs.statSync(file).mode & 0o777, mode);
+    }));
+  }
+}
+
+test(mode, true);
+test(mode, false);
diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js
index 992f30afec..ea779e499e 100644
--- a/test/parallel/test-fs-promises.js
+++ b/test/parallel/test-fs-promises.js
@@ -113,28 +113,10 @@ function verifyStatObject(stat) {
     await fchmod(handle, 0o666);
     await handle.chmod(0o666);
 
-    // `mode` can't be > 0o777
-    assert.rejects(
-      async () => chmod(handle, (0o777 + 1)),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        name: 'TypeError [ERR_INVALID_ARG_TYPE]'
-      }
-    );
-    assert.rejects(
-      async () => fchmod(handle, (0o777 + 1)),
-      {
-        code: 'ERR_OUT_OF_RANGE',
-        name: 'RangeError [ERR_OUT_OF_RANGE]'
-      }
-    );
-    assert.rejects(
-      async () => handle.chmod(handle, (0o777 + 1)),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        name: 'TypeError [ERR_INVALID_ARG_TYPE]'
-      }
-    );
+    // Mode larger than 0o777 should be masked off.
+    await chmod(dest, (0o777 + 1));
+    await fchmod(handle, 0o777 + 1);
+    await handle.chmod(0o777 + 1);
 
     await utimes(dest, new Date(), new Date());
 
diff --git a/test/parallel/test-process-umask-mask.js b/test/parallel/test-process-umask-mask.js
new file mode 100644
index 0000000000..c312c061d2
--- /dev/null
+++ b/test/parallel/test-process-umask-mask.js
@@ -0,0 +1,29 @@
+'use strict';
+
+// This tests that mask > 0o777 will be masked off with 0o777 in
+// process.umask()
+
+const common = require('../common');
+const assert = require('assert');
+
+let mask;
+
+if (common.isWindows) {
+  mask = 0o600;
+} else {
+  mask = 0o664;
+}
+
+const maskToIgnore = 0o10000;
+
+const old = process.umask();
+
+function test(input, output) {
+  process.umask(input);
+  assert.strictEqual(process.umask(), output);
+
+  process.umask(old);
+}
+
+test(mask | maskToIgnore, mask);
+test((mask | maskToIgnore).toString(8), mask);
diff --git a/test/parallel/test-process-umask.js b/test/parallel/test-process-umask.js
index 869619f191..1d496f05ef 100644
--- a/test/parallel/test-process-umask.js
+++ b/test/parallel/test-process-umask.js
@@ -33,20 +33,20 @@ if (common.isWindows) {
 
 const old = process.umask(mask);
 
-assert.strictEqual(parseInt(mask, 8), process.umask(old));
+assert.strictEqual(process.umask(old), parseInt(mask, 8));
 
 // confirm reading the umask does not modify it.
 // 1. If the test fails, this call will succeed, but the mask will be set to 0
-assert.strictEqual(old, process.umask());
+assert.strictEqual(process.umask(), old);
 // 2. If the test fails, process.umask() will return 0
-assert.strictEqual(old, process.umask());
+assert.strictEqual(process.umask(), old);
 
 assert.throws(() => {
   process.umask({});
 }, {
-  code: 'ERR_INVALID_ARG_TYPE',
-  message: 'The "mask" argument must be one of type number, string, or ' +
-    'undefined. Received type object'
+  code: 'ERR_INVALID_ARG_VALUE',
+  message: 'The argument \'mask\' must be a 32-bit unsigned integer ' +
+           'or an octal string. Received {}'
 });
 
 ['123x', 'abc', '999'].forEach((value) => {
@@ -54,6 +54,7 @@ assert.throws(() => {
     process.umask(value);
   }, {
     code: 'ERR_INVALID_ARG_VALUE',
-    message: `The argument 'mask' must be an octal string. Received '${value}'`
+    message: 'The argument \'mask\' must be a 32-bit unsigned integer ' +
+             `or an octal string. Received '${value}'`
   });
 });