Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: loosen validation to allow objects with an own toString function #34993

Merged
merged 1 commit into from
Sep 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 61 additions & 19 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -4163,6 +4163,10 @@ This happens when:
<!-- YAML
added: v0.0.2
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `buffer` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `buffer` parameter won't coerce unsupported input to
Expand All @@ -4188,7 +4192,7 @@ changes:
-->

* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `buffer` {Buffer|TypedArray|DataView|string|Object}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
Expand All @@ -4197,7 +4201,8 @@ changes:
* `bytesWritten` {integer}
* `buffer` {Buffer|TypedArray|DataView}

Write `buffer` to the file specified by `fd`.
Write `buffer` to the file specified by `fd`. If `buffer` is a normal object, it
must have an own `toString` function property.

`offset` determines the part of the buffer to be written, and `length` is
an integer specifying the number of bytes to write.
Expand All @@ -4224,6 +4229,10 @@ the end of the file.
<!-- YAML
added: v0.11.5
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `string` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `string` parameter won't coerce unsupported input to
Expand All @@ -4242,16 +4251,16 @@ changes:
-->

* `fd` {integer}
* `string` {string}
* `string` {string|Object}
* `position` {integer}
* `encoding` {string} **Default:** `'utf8'`
* `callback` {Function}
* `err` {Error}
* `written` {integer}
* `string` {string}

Write `string` to the file specified by `fd`. If `string` is not a string, then
an exception will be thrown.
Write `string` to the file specified by `fd`. If `string` is not a string, or an
object with an own `toString` function property, then an exception is 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
Expand Down Expand Up @@ -4283,6 +4292,10 @@ details.
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `data` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `data` parameter won't coerce unsupported input to
Expand All @@ -4308,7 +4321,7 @@ changes:
-->

* `file` {string|Buffer|URL|integer} filename or file descriptor
* `data` {string|Buffer|TypedArray|DataView}
* `data` {string|Buffer|TypedArray|DataView|Object}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand All @@ -4324,6 +4337,7 @@ When `file` is a file descriptor, the behavior is similar to calling
a file descriptor.

The `encoding` option is ignored if `data` is a buffer.
If `data` is a normal object, it must have an own `toString` function property.

```js
const data = new Uint8Array(Buffer.from('Hello Node.js'));
Expand Down Expand Up @@ -4373,6 +4387,10 @@ to contain only `', World'`.
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `data` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `data` parameter won't coerce unsupported input to
Expand All @@ -4390,7 +4408,7 @@ changes:
-->

* `file` {string|Buffer|URL|integer} filename or file descriptor
* `data` {string|Buffer|TypedArray|DataView}
* `data` {string|Buffer|TypedArray|DataView|Object}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand All @@ -4405,6 +4423,10 @@ this API: [`fs.writeFile()`][].
<!-- YAML
added: v0.1.21
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `buffer` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `buffer` parameter won't coerce unsupported input to
Expand All @@ -4422,7 +4444,7 @@ changes:
-->

* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `buffer` {Buffer|TypedArray|DataView|string|Object}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
Expand All @@ -4435,6 +4457,10 @@ this API: [`fs.write(fd, buffer...)`][].
<!-- YAML
added: v0.11.5
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `string` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `string` parameter won't coerce unsupported input to
Expand All @@ -4445,7 +4471,7 @@ changes:
-->

* `fd` {integer}
* `string` {string}
* `string` {string|Object}
* `position` {integer}
* `encoding` {string}
* Returns: {number} The number of bytes written.
Expand Down Expand Up @@ -4812,13 +4838,17 @@ This function does not work on AIX versions before 7.1, it will resolve the
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `buffer` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `buffer` parameter won't coerce unsupported input to
buffers anymore.
-->

* `buffer` {Buffer|Uint8Array}
* `buffer` {Buffer|Uint8Array|string|Object}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
Expand Down Expand Up @@ -4849,19 +4879,23 @@ the end of the file.
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `string` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
pr-url: https://github.com/nodejs/node/pull/31030
description: The `string` parameter won't coerce unsupported input to
strings anymore.
-->

* `string` {string}
* `string` {string|Object}
* `position` {integer}
* `encoding` {string} **Default:** `'utf8'`
* Returns: {Promise}

Write `string` to the file. If `string` is not a string, then
an exception will be thrown.
Write `string` to the file. If `string` is not a string, or an
object with an own `toString` function property, then an exception is thrown.

The `Promise` is resolved with an object containing a `bytesWritten` property
identifying the number of bytes written, and a `buffer` property containing
Expand All @@ -4885,20 +4919,24 @@ the end of the file.
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `data` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
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}
* `data` {string|Buffer|Uint8Array|Object}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* Returns: {Promise}

Asynchronously writes data to a file, replacing the file if it already exists.
`data` can be a string or a buffer. The `Promise` will be resolved with no
arguments upon success.
`data` can be a string, a buffer, or an object with an own `toString` function
property. The `Promise` is resolved with no arguments upon success.

The `encoding` option is ignored if `data` is a buffer.

Expand Down Expand Up @@ -5516,23 +5554,27 @@ The `atime` and `mtime` arguments follow these rules:
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34993
description: The `data` parameter will stringify an object with an
explicit `toString` function.
- version: v14.0.0
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`
* `data` {string|Buffer|Uint8Array}
* `data` {string|Buffer|Uint8Array|Object}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
* `flag` {string} See [support of file system `flags`][]. **Default:** `'w'`.
* Returns: {Promise}

Asynchronously writes data to a file, replacing the file if it already exists.
`data` can be a string or a buffer. The `Promise` will be resolved with no
arguments upon success.
`data` can be a string, a buffer, or an object with an own `toString` function
property. The `Promise` is resolved with no arguments upon success.

The `encoding` option is ignored if `data` is a buffer.

Expand Down
7 changes: 4 additions & 3 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const {
ObjectDefineProperties,
ObjectDefineProperty,
Promise,
String,
} = primordials;

const { fs: constants } = internalBinding('constants');
Expand Down Expand Up @@ -663,7 +664,7 @@ function write(fd, buffer, offset, length, position, callback) {

const req = new FSReqCallback();
req.oncomplete = wrapper;
return binding.writeString(fd, buffer, offset, length, req);
return binding.writeString(fd, String(buffer), offset, length, req);
}

ObjectDefineProperty(write, internalUtil.customPromisifyArgs,
Expand Down Expand Up @@ -1383,7 +1384,7 @@ function writeFile(path, data, options, callback) {

if (!isArrayBufferView(data)) {
validateStringAfterArrayBufferView(data, 'data');
data = Buffer.from(data, options.encoding || 'utf8');
data = Buffer.from(String(data), options.encoding || 'utf8');
}

if (isFd(path)) {
Expand All @@ -1407,7 +1408,7 @@ function writeFileSync(path, data, options) {

if (!isArrayBufferView(data)) {
validateStringAfterArrayBufferView(data, 'data');
data = Buffer.from(data, options.encoding || 'utf8');
data = Buffer.from(String(data), options.encoding || 'utf8');
}

const flag = options.flag || 'w';
Expand Down
24 changes: 18 additions & 6 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const {
BigInt,
DateNow,
Error,
ObjectPrototypeHasOwnProperty,
Number,
NumberIsFinite,
MathMin,
Expand Down Expand Up @@ -697,13 +698,24 @@ const getValidMode = hideStackFrames((mode, type) => {
});

const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
if (typeof buffer !== 'string') {
throw new ERR_INVALID_ARG_TYPE(
name,
['string', 'Buffer', 'TypedArray', 'DataView'],
buffer
);
if (typeof buffer === 'string') {
return;
}

if (
typeof buffer === 'object' &&
buffer !== null &&
typeof buffer.toString === 'function' &&
ObjectPrototypeHasOwnProperty(buffer, 'toString')
) {
ljharb marked this conversation as resolved.
Show resolved Hide resolved
return;
}

throw new ERR_INVALID_ARG_TYPE(
name,
['string', 'Buffer', 'TypedArray', 'DataView'],
buffer
);
});

module.exports = {
Expand Down
14 changes: 14 additions & 0 deletions test/parallel/test-fs-write-file-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,17 @@ tmpdir.refresh();
const content = fs.readFileSync(file, { encoding: 'utf8' });
assert.strictEqual(content, 'hello world!');
}

// Test writeFileSync with an object with an own toString function
{
const file = path.join(tmpdir.path, 'testWriteFileSyncStringify.txt');
const data = {
toString() {
return 'hello world!';
}
};

fs.writeFileSync(file, data, { encoding: 'utf8', flag: 'a' });
const content = fs.readFileSync(file, { encoding: 'utf8' });
assert.strictEqual(content, String(data));
}
16 changes: 16 additions & 0 deletions test/parallel/test-fs-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ tmpdir.refresh();
const fn = path.join(tmpdir.path, 'write.txt');
const fn2 = path.join(tmpdir.path, 'write2.txt');
const fn3 = path.join(tmpdir.path, 'write3.txt');
const fn4 = path.join(tmpdir.path, 'write4.txt');
const expected = 'ümlaut.';
const constants = fs.constants;

Expand Down Expand Up @@ -134,6 +135,21 @@ fs.open(fn3, 'w', 0o644, common.mustCall((err, fd) => {
fs.write(fd, expected, done);
}));

fs.open(fn4, 'w', 0o644, common.mustCall((err, fd) => {
assert.ifError(err);

const done = common.mustCall((err, written) => {
assert.ifError(err);
assert.strictEqual(written, Buffer.byteLength(expected));
fs.closeSync(fd);
});

const data = {
toString() { return expected; }
};
fs.write(fd, data, done);
}));

[false, 'test', {}, [], null, undefined].forEach((i) => {
assert.throws(
() => fs.write(i, common.mustNotCall()),
Expand Down