fs: validate the input data to be of expected types

The input was not validated so far and that caused unwanted side effects. E.g., `undefined` became the string `'undefined'`. It was expected to fail or to end up as empty string. Now all input is validated to be either some type of array buffer view or a string. That way it's always clear what the user intents. PR-URL: #31030 Fixes: #31025 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
nodejs · Feb 5, 2020 · fb6df3b · fb6df3b
1 parent 2d8febc
commit fb6df3b
Show file tree

Hide file tree

Showing 12 changed files with 148 additions and 150 deletions.
diff --git a/doc/api/fs.md b/doc/api/fs.md
@@ -3897,6 +3897,10 @@ This happens when:
 <!-- YAML
 added: v0.0.2
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `buffer` parameter won't coerce unsupported input to
+                 strings anymore.
   - version: v10.10.0
     pr-url: https://github.com/nodejs/node/pull/22150
     description: The `buffer` parameter can now be any `TypedArray` or a
@@ -3954,6 +3958,10 @@ the end of the file.
 <!-- YAML
 added: v0.11.5
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `string` parameter won't coerce unsupported input to
+                 strings anymore.
   - version: v10.0.0
     pr-url: https://github.com/nodejs/node/pull/12562
     description: The `callback` parameter is no longer optional. Not passing
@@ -4009,6 +4017,10 @@ details.
 <!-- YAML
 added: v0.1.29
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `data` parameter won't coerce unsupported input to
+                 strings anymore.
   - version: v10.10.0
     pr-url: https://github.com/nodejs/node/pull/22150
     description: The `data` parameter can now be any `TypedArray` or a
@@ -4095,6 +4107,10 @@ to contain only `', World'`.
 <!-- YAML
 added: v0.1.29
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `data` parameter won't coerce unsupported input to
+                 strings anymore.
   - version: v10.10.0
     pr-url: https://github.com/nodejs/node/pull/22150
     description: The `data` parameter can now be any `TypedArray` or a
@@ -4123,6 +4139,10 @@ this API: [`fs.writeFile()`][].
 <!-- YAML
 added: v0.1.21
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `buffer` parameter won't coerce unsupported input to
+                 strings anymore.
   - version: v10.10.0
     pr-url: https://github.com/nodejs/node/pull/22150
     description: The `buffer` parameter can now be any `TypedArray` or a
@@ -4149,6 +4169,10 @@ this API: [`fs.write(fd, buffer...)`][].
 <!-- YAML
 added: v0.11.5
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `string` parameter won't coerce unsupported input to
+                 strings anymore.
   - version: v7.2.0
     pr-url: https://github.com/nodejs/node/pull/7856
     description: The `position` parameter is optional now.
@@ -4486,6 +4510,11 @@ This function does not work on AIX versions before 7.1, it will resolve the
 #### `filehandle.write(buffer[, offset[, length[, position]]])`
 <!-- YAML
 added: v10.0.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `buffer` parameter won't coerce unsupported input to
+                 buffers anymore.
 -->
 
 * `buffer` {Buffer|Uint8Array}
@@ -4518,6 +4547,11 @@ the end of the file.
 #### `filehandle.write(string[, position[, encoding]])`
 <!-- YAML
 added: v10.0.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `string` parameter won't coerce unsupported input to
+                 strings anymore.
 -->
 
 * `string` {string}
@@ -4526,7 +4560,7 @@ added: v10.0.0
 * Returns: {Promise}
 
 Write `string` to the file. If `string` is not a string, then
-the value will be coerced to one.
+an exception will be thrown.
 
 The `Promise` is resolved with an object containing a `bytesWritten` property
 identifying the number of bytes written, and a `buffer` property containing
@@ -4549,6 +4583,11 @@ the end of the file.
 #### `filehandle.writeFile(data, options)`
 <!-- YAML
 added: v10.0.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `data` parameter won't coerce unsupported input to
+                 strings anymore.
 -->
 
 * `data` {string|Buffer|Uint8Array}
@@ -5137,6 +5176,11 @@ The `atime` and `mtime` arguments follow these rules:
 ### `fsPromises.writeFile(file, data[, options])`
 <!-- YAML
 added: v10.0.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31030
+    description: The `data` parameter won't coerce unsupported input to
+                 strings anymore.
 -->
 
 * `file` {string|Buffer|URL|FileHandle} filename or `FileHandle`

diff --git a/lib/fs.js b/lib/fs.js
@@ -92,6 +92,7 @@ const {
   validateOffsetLengthWrite,
   validatePath,
   validateRmdirOptions,
+  validateStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 } = require('internal/fs/utils');
 const {
@@ -548,9 +549,6 @@ function write(fd, buffer, offset, length, position, callback) {
 
   validateInt32(fd, 'fd', 0);
 
-  const req = new FSReqCallback();
-  req.oncomplete = wrapper;
-
   if (isArrayBufferView(buffer)) {
     callback = maybeCallback(callback || position || length || offset);
     if (offset == null || typeof offset === 'function') {
@@ -563,11 +561,14 @@ function write(fd, buffer, offset, length, position, callback) {
     if (typeof position !== 'number')
       position = null;
     validateOffsetLengthWrite(offset, length, buffer.byteLength);
+
+    const req = new FSReqCallback();
+    req.oncomplete = wrapper;
     return binding.writeBuffer(fd, buffer, offset, length, position, req);
   }
 
-  if (typeof buffer !== 'string')
-    buffer += '';
+  validateStringAfterArrayBufferView(buffer, 'buffer');
+
   if (typeof position !== 'function') {
     if (typeof offset === 'function') {
       position = offset;
@@ -578,6 +579,9 @@ function write(fd, buffer, offset, length, position, callback) {
     length = 'utf8';
   }
   callback = maybeCallback(position);
+
+  const req = new FSReqCallback();
+  req.oncomplete = wrapper;
   return binding.writeString(fd, buffer, offset, length, req);
 }
 
@@ -606,8 +610,8 @@ function writeSync(fd, buffer, offset, length, position) {
     result = binding.writeBuffer(fd, buffer, offset, length, position,
                                  undefined, ctx);
   } else {
-    if (typeof buffer !== 'string')
-      buffer += '';
+    validateStringAfterArrayBufferView(buffer, 'buffer');
+
     if (offset === undefined)
       offset = null;
     result = binding.writeString(fd, buffer, offset, length,
@@ -1277,38 +1281,41 @@ function writeFile(path, data, options, callback) {
   options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
   const flag = options.flag || 'w';
 
+  if (!isArrayBufferView(data)) {
+    validateStringAfterArrayBufferView(data, 'data');
+    data = Buffer.from(data, options.encoding || 'utf8');
+  }
+
   if (isFd(path)) {
-    writeFd(path, true);
+    const isUserFd = true;
+    writeAll(path, isUserFd, data, 0, data.byteLength, null, callback);
     return;
   }
 
   fs.open(path, flag, options.mode, (openErr, fd) => {
     if (openErr) {
       callback(openErr);
     } else {
-      writeFd(fd, false);
+      const isUserFd = false;
+      const position = /a/.test(flag) ? null : 0;
+      writeAll(fd, isUserFd, data, 0, data.byteLength, position, callback);
     }
   });
-
-  function writeFd(fd, isUserFd) {
-    const buffer = isArrayBufferView(data) ?
-      data : Buffer.from('' + data, options.encoding || 'utf8');
-    const position = (/a/.test(flag) || isUserFd) ? null : 0;
-
-    writeAll(fd, isUserFd, buffer, 0, buffer.byteLength, position, callback);
-  }
 }
 
 function writeFileSync(path, data, options) {
   options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
+
+  if (!isArrayBufferView(data)) {
+    validateStringAfterArrayBufferView(data, 'data');
+    data = Buffer.from(data, options.encoding || 'utf8');
+  }
+
   const flag = options.flag || 'w';
 
   const isUserFd = isFd(path); // File descriptor ownership
   const fd = isUserFd ? path : fs.openSync(path, flag, options.mode);
 
-  if (!isArrayBufferView(data)) {
-    data = Buffer.from('' + data, options.encoding || 'utf8');
-  }
   let offset = 0;
   let length = data.byteLength;
   let position = (/a/.test(flag) || isUserFd) ? null : 0;

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -26,7 +26,7 @@ const {
   ERR_INVALID_ARG_VALUE,
   ERR_METHOD_NOT_IMPLEMENTED
 } = require('internal/errors').codes;
-const { isUint8Array } = require('internal/util/types');
+const { isArrayBufferView } = require('internal/util/types');
 const { rimrafPromises } = require('internal/fs/rimraf');
 const {
   copyObject,
@@ -44,6 +44,7 @@ const {
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
   validateRmdirOptions,
+  validateStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 } = require('internal/fs/utils');
 const { opendir } = require('internal/fs/dir');
@@ -140,16 +141,18 @@ function validateFileHandle(handle) {
 }
 
 async function writeFileHandle(filehandle, data, options) {
-  let buffer = isUint8Array(data) ?
-    data : Buffer.from('' + data, options.encoding || 'utf8');
-  let remaining = buffer.length;
+  if (!isArrayBufferView(data)) {
+    validateStringAfterArrayBufferView(data, 'data');
+    data = Buffer.from(data, options.encoding || 'utf8');
+  }
+  let remaining = data.length;
   if (remaining === 0) return;
   do {
     const { bytesWritten } =
-      await write(filehandle, buffer, 0,
-                  MathMin(16384, buffer.length));
+      await write(filehandle, data, 0,
+                  MathMin(16384, data.length));
     remaining -= bytesWritten;
-    buffer = buffer.slice(bytesWritten);
+    data = data.slice(bytesWritten);
   } while (remaining > 0);
 }
 
@@ -260,7 +263,7 @@ async function write(handle, buffer, offset, length, position) {
   if (buffer.length === 0)
     return { bytesWritten: 0, buffer };
 
-  if (isUint8Array(buffer)) {
+  if (isArrayBufferView(buffer)) {
     if (offset == null) {
       offset = 0;
     } else {
@@ -277,8 +280,7 @@ async function write(handle, buffer, offset, length, position) {
     return { bytesWritten, buffer };
   }
 
-  if (typeof buffer !== 'string')
-    buffer += '';
+  validateStringAfterArrayBufferView(buffer, 'buffer');
   const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
                                                   length, kUsePromises)) || 0;
   return { bytesWritten, buffer };

diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
@@ -651,6 +651,16 @@ const getValidMode = hideStackFrames((mode, type) => {
     'mode', `an integer >= ${min} && <= ${max}`, mode);
 });
 
+const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
+  if (typeof buffer !== 'string') {
+    throw new ERR_INVALID_ARG_TYPE(
+      name,
+      ['string', 'Buffer', 'TypedArray', 'DataView'],
+      buffer
+    );
+  }
+});
+
 module.exports = {
   assertEncoding,
   BigIntStats,  // for testing
@@ -675,5 +685,6 @@ module.exports = {
   validateOffsetLengthWrite,
   validatePath,
   validateRmdirOptions,
+  validateStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 };
diff --git a/test/parallel/test-fs-append-file-sync.js b/test/parallel/test-fs-append-file-sync.js
@@ -70,11 +70,18 @@ const fileData3 = fs.readFileSync(filename3);
 
 assert.strictEqual(buf.length + currentFileData.length, fileData3.length);
 
-// Test that appendFile accepts numbers.
 const filename4 = join(tmpdir.path, 'append-sync4.txt');
 fs.writeFileSync(filename4, currentFileData, { mode: m });
 
-fs.appendFileSync(filename4, num, { mode: m });
+[
+  true, false, 0, 1, Infinity, () => {}, {}, [], undefined, null
+].forEach((value) => {
+  assert.throws(
+    () => fs.appendFileSync(filename4, value, { mode: m }),
+    { message: /data/, code: 'ERR_INVALID_ARG_TYPE' }
+  );
+});
+fs.appendFileSync(filename4, `${num}`, { mode: m });
 
 // Windows permissions aren't Unix.
 if (!common.isWindows) {