Skip to content

Commit fb6df3b

Browse files
BridgeARBridgeAR
committed
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]>
1 parent 2d8febc commit fb6df3b

12 files changed

Lines changed: 148 additions & 150 deletions

‎doc/api/fs.md‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3897,6 +3897,10 @@ This happens when:
38973897
<!-- YAML
38983898
added: v0.0.2
38993899
changes:
3900+
- version: REPLACEME
3901+
pr-url: https://github.com/nodejs/node/pull/31030
3902+
description: The `buffer` parameter won't coerce unsupported input to
3903+
strings anymore.
39003904
- version: v10.10.0
39013905
pr-url: https://github.com/nodejs/node/pull/22150
39023906
description: The `buffer` parameter can now be any `TypedArray` or a
@@ -3954,6 +3958,10 @@ the end of the file.
39543958
<!-- YAML
39553959
added: v0.11.5
39563960
changes:
3961+
- version: REPLACEME
3962+
pr-url: https://github.com/nodejs/node/pull/31030
3963+
description: The `string` parameter won't coerce unsupported input to
3964+
strings anymore.
39573965
- version: v10.0.0
39583966
pr-url: https://github.com/nodejs/node/pull/12562
39593967
description: The `callback` parameter is no longer optional. Not passing
@@ -4009,6 +4017,10 @@ details.
40094017
<!-- YAML
40104018
added: v0.1.29
40114019
changes:
4020+
- version: REPLACEME
4021+
pr-url: https://github.com/nodejs/node/pull/31030
4022+
description: The `data` parameter won't coerce unsupported input to
4023+
strings anymore.
40124024
- version: v10.10.0
40134025
pr-url: https://github.com/nodejs/node/pull/22150
40144026
description: The `data` parameter can now be any `TypedArray` or a
@@ -4095,6 +4107,10 @@ to contain only `', World'`.
40954107
<!-- YAML
40964108
added: v0.1.29
40974109
changes:
4110+
- version: REPLACEME
4111+
pr-url: https://github.com/nodejs/node/pull/31030
4112+
description: The `data` parameter won't coerce unsupported input to
4113+
strings anymore.
40984114
- version: v10.10.0
40994115
pr-url: https://github.com/nodejs/node/pull/22150
41004116
description: The `data` parameter can now be any `TypedArray` or a
@@ -4123,6 +4139,10 @@ this API: [`fs.writeFile()`][].
41234139
<!-- YAML
41244140
added: v0.1.21
41254141
changes:
4142+
- version: REPLACEME
4143+
pr-url: https://github.com/nodejs/node/pull/31030
4144+
description: The `buffer` parameter won't coerce unsupported input to
4145+
strings anymore.
41264146
- version: v10.10.0
41274147
pr-url: https://github.com/nodejs/node/pull/22150
41284148
description: The `buffer` parameter can now be any `TypedArray` or a
@@ -4149,6 +4169,10 @@ this API: [`fs.write(fd, buffer...)`][].
41494169
<!-- YAML
41504170
added: v0.11.5
41514171
changes:
4172+
- version: REPLACEME
4173+
pr-url: https://github.com/nodejs/node/pull/31030
4174+
description: The `string` parameter won't coerce unsupported input to
4175+
strings anymore.
41524176
- version: v7.2.0
41534177
pr-url: https://github.com/nodejs/node/pull/7856
41544178
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
44864510
#### `filehandle.write(buffer[, offset[, length[, position]]])`
44874511
<!-- YAML
44884512
added: v10.0.0
4513+
changes:
4514+
- version: REPLACEME
4515+
pr-url: https://github.com/nodejs/node/pull/31030
4516+
description: The `buffer` parameter won't coerce unsupported input to
4517+
buffers anymore.
44894518
-->
44904519

44914520
* `buffer` {Buffer|Uint8Array}
@@ -4518,6 +4547,11 @@ the end of the file.
45184547
#### `filehandle.write(string[, position[, encoding]])`
45194548
<!-- YAML
45204549
added: v10.0.0
4550+
changes:
4551+
- version: REPLACEME
4552+
pr-url: https://github.com/nodejs/node/pull/31030
4553+
description: The `string` parameter won't coerce unsupported input to
4554+
strings anymore.
45214555
-->
45224556

45234557
* `string` {string}
@@ -4526,7 +4560,7 @@ added: v10.0.0
45264560
* Returns: {Promise}
45274561

45284562
Write `string` to the file. If `string` is not a string, then
4529-
the value will be coerced to one.
4563+
an exception will be thrown.
45304564

45314565
The `Promise` is resolved with an object containing a `bytesWritten` property
45324566
identifying the number of bytes written, and a `buffer` property containing
@@ -4549,6 +4583,11 @@ the end of the file.
45494583
#### `filehandle.writeFile(data, options)`
45504584
<!-- YAML
45514585
added: v10.0.0
4586+
changes:
4587+
- version: REPLACEME
4588+
pr-url: https://github.com/nodejs/node/pull/31030
4589+
description: The `data` parameter won't coerce unsupported input to
4590+
strings anymore.
45524591
-->
45534592

45544593
* `data` {string|Buffer|Uint8Array}
@@ -5137,6 +5176,11 @@ The `atime` and `mtime` arguments follow these rules:
51375176
### `fsPromises.writeFile(file, data[, options])`
51385177
<!-- YAML
51395178
added: v10.0.0
5179+
changes:
5180+
- version: REPLACEME
5181+
pr-url: https://github.com/nodejs/node/pull/31030
5182+
description: The `data` parameter won't coerce unsupported input to
5183+
strings anymore.
51405184
-->
51415185

51425186
* `file` {string|Buffer|URL|FileHandle} filename or `FileHandle`

‎lib/fs.js‎

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ const {
9292
validateOffsetLengthWrite,
9393
validatePath,
9494
validateRmdirOptions,
95+
validateStringAfterArrayBufferView,
9596
warnOnNonPortableTemplate
9697
} = require('internal/fs/utils');
9798
const {
@@ -548,9 +549,6 @@ function write(fd, buffer, offset, length, position, callback) {
548549

549550
validateInt32(fd, 'fd', 0);
550551

551-
const req = new FSReqCallback();
552-
req.oncomplete = wrapper;
553-
554552
if (isArrayBufferView(buffer)) {
555553
callback = maybeCallback(callback || position || length || offset);
556554
if (offset == null || typeof offset === 'function') {
@@ -563,11 +561,14 @@ function write(fd, buffer, offset, length, position, callback) {
563561
if (typeof position !== 'number')
564562
position = null;
565563
validateOffsetLengthWrite(offset, length, buffer.byteLength);
564+
565+
const req = new FSReqCallback();
566+
req.oncomplete = wrapper;
566567
return binding.writeBuffer(fd, buffer, offset, length, position, req);
567568
}
568569

569-
if (typeof buffer !== 'string')
570-
buffer += '';
570+
validateStringAfterArrayBufferView(buffer, 'buffer');
571+
571572
if (typeof position !== 'function') {
572573
if (typeof offset === 'function') {
573574
position = offset;
@@ -578,6 +579,9 @@ function write(fd, buffer, offset, length, position, callback) {
578579
length = 'utf8';
579580
}
580581
callback = maybeCallback(position);
582+
583+
const req = new FSReqCallback();
584+
req.oncomplete = wrapper;
581585
return binding.writeString(fd, buffer, offset, length, req);
582586
}
583587

@@ -606,8 +610,8 @@ function writeSync(fd, buffer, offset, length, position) {
606610
result = binding.writeBuffer(fd, buffer, offset, length, position,
607611
undefined, ctx);
608612
} else {
609-
if (typeof buffer !== 'string')
610-
buffer += '';
613+
validateStringAfterArrayBufferView(buffer, 'buffer');
614+
611615
if (offset === undefined)
612616
offset = null;
613617
result = binding.writeString(fd, buffer, offset, length,
@@ -1277,38 +1281,41 @@ function writeFile(path, data, options, callback) {
12771281
options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
12781282
const flag = options.flag || 'w';
12791283

1284+
if (!isArrayBufferView(data)) {
1285+
validateStringAfterArrayBufferView(data, 'data');
1286+
data = Buffer.from(data, options.encoding || 'utf8');
1287+
}
1288+
12801289
if (isFd(path)) {
1281-
writeFd(path, true);
1290+
const isUserFd = true;
1291+
writeAll(path, isUserFd, data, 0, data.byteLength, null, callback);
12821292
return;
12831293
}
12841294

12851295
fs.open(path, flag, options.mode, (openErr, fd) => {
12861296
if (openErr) {
12871297
callback(openErr);
12881298
} else {
1289-
writeFd(fd, false);
1299+
const isUserFd = false;
1300+
const position = /a/.test(flag) ? null : 0;
1301+
writeAll(fd, isUserFd, data, 0, data.byteLength, position, callback);
12901302
}
12911303
});
1292-
1293-
function writeFd(fd, isUserFd) {
1294-
const buffer = isArrayBufferView(data) ?
1295-
data : Buffer.from('' + data, options.encoding || 'utf8');
1296-
const position = (/a/.test(flag) || isUserFd) ? null : 0;
1297-
1298-
writeAll(fd, isUserFd, buffer, 0, buffer.byteLength, position, callback);
1299-
}
13001304
}
13011305

13021306
function writeFileSync(path, data, options) {
13031307
options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
1308+
1309+
if (!isArrayBufferView(data)) {
1310+
validateStringAfterArrayBufferView(data, 'data');
1311+
data = Buffer.from(data, options.encoding || 'utf8');
1312+
}
1313+
13041314
const flag = options.flag || 'w';
13051315

13061316
const isUserFd = isFd(path); // File descriptor ownership
13071317
const fd = isUserFd ? path : fs.openSync(path, flag, options.mode);
13081318

1309-
if (!isArrayBufferView(data)) {
1310-
data = Buffer.from('' + data, options.encoding || 'utf8');
1311-
}
13121319
let offset = 0;
13131320
let length = data.byteLength;
13141321
let position = (/a/.test(flag) || isUserFd) ? null : 0;

‎lib/internal/fs/promises.js‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const {
2626
ERR_INVALID_ARG_VALUE,
2727
ERR_METHOD_NOT_IMPLEMENTED
2828
} = require('internal/errors').codes;
29-
const { isUint8Array } = require('internal/util/types');
29+
const { isArrayBufferView } = require('internal/util/types');
3030
const { rimrafPromises } = require('internal/fs/rimraf');
3131
const {
3232
copyObject,
@@ -44,6 +44,7 @@ const {
4444
validateOffsetLengthRead,
4545
validateOffsetLengthWrite,
4646
validateRmdirOptions,
47+
validateStringAfterArrayBufferView,
4748
warnOnNonPortableTemplate
4849
} = require('internal/fs/utils');
4950
const { opendir } = require('internal/fs/dir');
@@ -140,16 +141,18 @@ function validateFileHandle(handle) {
140141
}
141142

142143
async function writeFileHandle(filehandle, data, options) {
143-
let buffer = isUint8Array(data) ?
144-
data : Buffer.from('' + data, options.encoding || 'utf8');
145-
let remaining = buffer.length;
144+
if (!isArrayBufferView(data)) {
145+
validateStringAfterArrayBufferView(data, 'data');
146+
data = Buffer.from(data, options.encoding || 'utf8');
147+
}
148+
let remaining = data.length;
146149
if (remaining === 0) return;
147150
do {
148151
const { bytesWritten } =
149-
await write(filehandle, buffer, 0,
150-
MathMin(16384, buffer.length));
152+
await write(filehandle, data, 0,
153+
MathMin(16384, data.length));
151154
remaining -= bytesWritten;
152-
buffer = buffer.slice(bytesWritten);
155+
data = data.slice(bytesWritten);
153156
} while (remaining > 0);
154157
}
155158

@@ -260,7 +263,7 @@ async function write(handle, buffer, offset, length, position) {
260263
if (buffer.length === 0)
261264
return { bytesWritten: 0, buffer };
262265

263-
if (isUint8Array(buffer)) {
266+
if (isArrayBufferView(buffer)) {
264267
if (offset == null) {
265268
offset = 0;
266269
} else {
@@ -277,8 +280,7 @@ async function write(handle, buffer, offset, length, position) {
277280
return { bytesWritten, buffer };
278281
}
279282

280-
if (typeof buffer !== 'string')
281-
buffer += '';
283+
validateStringAfterArrayBufferView(buffer, 'buffer');
282284
const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
283285
length, kUsePromises)) || 0;
284286
return { bytesWritten, buffer };

‎lib/internal/fs/utils.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -651,6 +651,16 @@ const getValidMode = hideStackFrames((mode, type) => {
651651
'mode', `an integer >= ${min} && <= ${max}`, mode);
652652
});
653653

654+
const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
655+
if (typeof buffer !== 'string') {
656+
throw new ERR_INVALID_ARG_TYPE(
657+
name,
658+
['string', 'Buffer', 'TypedArray', 'DataView'],
659+
buffer
660+
);
661+
}
662+
});
663+
654664
module.exports = {
655665
assertEncoding,
656666
BigIntStats, // for testing
@@ -675,5 +685,6 @@ module.exports = {
675685
validateOffsetLengthWrite,
676686
validatePath,
677687
validateRmdirOptions,
688+
validateStringAfterArrayBufferView,
678689
warnOnNonPortableTemplate
679690
};

‎test/parallel/test-fs-append-file-sync.js‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,18 @@ const fileData3 = fs.readFileSync(filename3);
7070

7171
assert.strictEqual(buf.length + currentFileData.length, fileData3.length);
7272

73-
// Test that appendFile accepts numbers.
7473
const filename4 = join(tmpdir.path, 'append-sync4.txt');
7574
fs.writeFileSync(filename4, currentFileData, { mode: m });
7675

77-
fs.appendFileSync(filename4, num, { mode: m });
76+
[
77+
true, false, 0, 1, Infinity, () => {}, {}, [], undefined, null
78+
].forEach((value) => {
79+
assert.throws(
80+
() => fs.appendFileSync(filename4, value, { mode: m }),
81+
{ message: /data/, code: 'ERR_INVALID_ARG_TYPE' }
82+
);
83+
});
84+
fs.appendFileSync(filename4, `${num}`, { mode: m });
7885

7986
// Windows permissions aren't Unix.
8087
if (!common.isWindows) {

0 commit comments

Comments
 (0)