fs: validate the input data to be of expected types

doc/api/fs.md

@@ -3891,6 +3891,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
@@ -3948,6 +3952,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
@@ -3971,7 +3979,7 @@ changes:
   * `string` {string}
 
 Write `string` to the file specified by `fd`. If `string` is not a string, then
-the value will be coerced to one.
+an exception will be thrown.
 
 `position` refers to the offset from the beginning of the file where this data
 should be written. If `typeof position !== 'number'` the data will be written at
@@ -4003,6 +4011,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
@@ -4033,7 +4045,7 @@ changes:
   * `err` {Error}
 
 When `file` is a filename, asynchronously writes data to the file, replacing the
-file if it already exists.  `data` can be a string or a buffer.
+file if it already exists. `data` can be a string or a buffer.
 
 When `file` is a file descriptor, the behavior is similar to calling
 `fs.write()` directly (which is recommended). See the notes below on using
@@ -4089,6 +4101,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
@@ -4117,6 +4133,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
@@ -4143,6 +4163,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.
@@ -4480,6 +4504,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}
@@ -4512,6 +4541,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}
@@ -4520,7 +4554,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
@@ -4543,6 +4577,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}
@@ -5131,6 +5170,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`

lib/fs.js

@@ -87,6 +87,7 @@ const {
   validateOffsetLengthWrite,
   validatePath,
   validateRmdirOptions,
+  validateStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 } = require('internal/fs/utils');
 const {
@@ -542,9 +543,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') {
@@ -557,11 +555,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;
@@ -572,6 +573,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);
 }
 
@@ -600,8 +604,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,
@@ -1268,38 +1272,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;

lib/internal/fs/promises.js

@@ -22,7 +22,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,
@@ -40,6 +40,7 @@ const {
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
   validateRmdirOptions,
+  validateStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 } = require('internal/fs/utils');
 const { opendir } = require('internal/fs/dir');
@@ -133,16 +134,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);
 }
 
@@ -253,7 +256,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 {
@@ -270,8 +273,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 };

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
 };

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) {