fs: validate the input data to be of expected types #31030

BridgeAR · 2019-12-19T18:11:30Z

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.

Fixes: #31025

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-12-19T18:12:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27812/

BridgeAR · 2019-12-19T18:12:45Z

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2142/

BridgeAR · 2019-12-19T23:32:59Z

The only CITGM errors that came up here are tests that just need a file and not the content. I opened a PR to fix their tests: ember-cli/ember-cli#8975

Trott · 2019-12-19T23:53:28Z

Will docs need to be updated to mention that the functions can throw in these situations? (I imagine "yes" but I haven't looked to see what they say right now.)

BridgeAR · 2019-12-20T00:02:31Z

@Trott I can do that but I think we never documented that in fs. I just checked other functions that also throw sync and the ones I looked at also do not have any documentation about that.

Should we maybe just outline in a generic fs part that input validation is synchronous?

Or should I only add an entry to the changes part?

Update: I just added the changes entries.

lib/fs.js

Trott · 2019-12-20T19:25:10Z

@Trott I can do that but I think we never documented that in fs. I just checked other functions that also throw sync and the ones I looked at also do not have any documentation about that.

In that case, I guess it's OK to omit. Someone can always go through and add the information at a later date. (Aside: Might not be a bad thing to add an entry to everything that throws indicating what it might throw, but that would be a pretty big project. @nodejs/documentation)

nodejs-github-bot · 2019-12-24T12:35:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27894/

BridgeAR · 2019-12-24T16:40:17Z

@nodejs/tsc PTAL. This needs one more LG to be ready.

nodejs-github-bot · 2019-12-24T16:42:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27910/

nodejs-github-bot · 2019-12-24T21:39:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27922/

BridgeAR · 2019-12-31T14:18:31Z

@nodejs/tsc this needs another review. PTAL

nodejs-github-bot · 2019-12-31T14:19:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28049/

Trott · 2020-01-04T03:25:24Z

Awaiting ember-cli/ember-cli#8975.

BridgeAR · 2020-01-07T12:14:22Z

@Trott do you want to wait until the test fix is released? It is merged but the next release is up in 6-12 weeks. I would rather land this before that.

Trott · 2020-01-11T03:37:31Z

@Trott do you want to wait until the test fix is released? It is merged but the next release is up in 6-12 weeks. I would rather land this before that.

There is a release coming the week of January 19. I know you're eager to get this in, but I think a 2-week delay should be tolerable. Since this PR is a semver-major change, it won't end up in a Node.js release until April. So, at least as I see it, there's no reason not to wait two weeks to land this.

An alternative is to land something in CITGM's lookup.json to grab the master branch of ember-cli rather than the last release, but I think waiting for the release is preferable.

Trott · 2020-01-12T03:43:34Z

@nodejs/tsc This has enough reviews to land, but could probably stand a little more attention/scrutiny/awareness.

mcollina

lgtm

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. Fixes: nodejs#31025

nodejs-github-bot · 2020-01-13T08:05:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28426/

BridgeAR · 2020-02-05T12:55:07Z

Landed in fb6df3b, afe3530 🎉

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 <rtrott@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: #31030 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: #31731 Refs: #31030 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

- Use Node.js 14 in actions/setup-node - Add Node.js 14.x to the test matrix - Remove CI: true from the environment The variable is already set by GitHub - Fix bug in test fixture Discovered with v14.x because of nodejs/node#31030.

ljharb · 2020-08-31T03:50:25Z

fwiw i believe this breaks uglify-js v2 (i can't upgrade to v3 because it lacks IE 8 support). The specific value being passed into fs.writeFileSync is an explicit object with a toString function.

Could this validation perhaps be loosened, to still stringify objects that have a toString function? That would still throw on the nullish cases referred to in the OP.

ljharb · 2020-08-31T04:24:55Z

Went ahead and submitted #34993, on the off chance the loosening would be accepted.

A Node.js 14.x added strict type checking when writing files to disk, preventing methods with their own `.toString()` method from being written to disk and generating a `ERR_INVALID_ARG_TYPE` error in the process. This affected using this plugin in combination with `--source-map`. The behavioral change was introduced in nodejs/node#31030 and recently fixed in nodejs/node#34993. That fix was not comprehensive, and did not resolve the issue for the plugin. To avoid this issue for all versions of Node, we no longer assume there will be an implicit call to `SourceMapGenerator.toString()`. Instead, it's now explicitly called when setting the data to write for the source map, fixing source map generation. This was tested on the latest releases of Node 12 through 15.

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 19, 2019

BridgeAR requested a review from Trott December 19, 2019 18:11

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Dec 19, 2019

Trott approved these changes Dec 19, 2019

View reviewed changes

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 19, 2019