fs: loosen validation to allow objects with an own toString function by ljharb · Pull Request #34993 · nodejs/node · GitHub

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

This is a loosening of #31030 (which is only in v14 atm). It allows an object that has an own toString function, which does not conflict with the OP of #31030 which is primarily concerned with accidental stringification.

This unbreaks uglify-js v2, which is the last version that retains IE 8 support, which a few of my packages need. I'm hoping this strikes the right balance to avoid footguns while allowing idiomatic JS behavior.

I personally would rather not support this.

@BridgeAR can you elaborate on why not? It's highly idiomatic in JS to throw on null/undefined but then to stringify arguments, and this PR isn't even as loose as that - the only additional cases it allows are objects with an own toString, iow, which were explicitly intended to be stringified.

@nodejs/fs

@ljharb I think if you run make lint-js-fix and update your branch, that will resolve the minor lint errors.

LGTM % lint errors, but as far as I understand, you'd need @BridgeAR to unblock this? (unless #34993 (comment) is non-blocking)

Updated. I believe the updated policy is that things are only blocking with "request changes", but obv I'd want @BridgeAR to confirm that.

The CI failures look unrelated to this PR, is it something known?

@BridgeAR Can you elaborate on your opposition? Is your opinion that we shouldn't maintain code for IE8 compatibility? Or something else? (And is it -1 opposition or -0 opposition?)

@nodejs/collaborators This could use additional informed reviews.

this change means that supplying a Buffer is no longer guaranteed to have a simple path if I understand this? @ljharb do you know if the fix needs to run on Buffer/TypedArrayss or just random objects? I'd prefer we not invoke a toString on the special cased types.

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

pending feedback on specialized types calling toString

@bmeck the case i'm concerned about is just a plain object, i am quite happy to only do the stringification on "not Buffers or TAs", if that would change your -1 to a >= 0 :-p

I'd be +1 with that change and a blurb in the docs about this special toString case

@bmeck so, looking again at my diff - all this does is doesn't throw on an object with an own toString function, inside validateStringAfterArrayBufferView - iow, i believe this is only called for non-Buffers/non-TypedArrays in the first place?

@ljharb confirmed, just looks like docs don't state this special case

Currently I see type signatures in the docs like * buffer {Buffer|TypedArray|DataView}, which doesn't even mention string - should i add string? how would i denote { toString: () => string }?

lgtm

Thanks for the ping, I am also mostly +1 on this.

Actual code looks fine to me - though I would prefer it if we explicitly documented what happens when an object with toString is passed (like bmeck asked).

I would also appreciate (doesn't have to be here in this PR) a test that explicitly tests the behavior of an object with toString on the prototype.

I also think we might want to explore relaxing this behavior in the future (to allow toString on the prototype but not Object.prototype.toString which would result in errors)

@BridgeAR note this is only for objects that explicitly opt-in by defining toString on the object itself at the moment.

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

PR-URL: nodejs#34993 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>

Landed in 9cf9e4a

PR-URL: #34993 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>

Confirmed that node v4.12 solves my issue. Thanks!

PR-URL: nodejs#34993 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>

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.

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: #34993 PR-URL: #41677 Refs: #41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: nodejs#34993 PR-URL: nodejs#41677 Refs: nodejs#41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: #34993 PR-URL: #41677 Refs: #41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: nodejs#34993 PR-URL: nodejs#41677 Refs: nodejs#41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: #34993 PR-URL: #41677 Backport-PR-URL: #42631 Refs: #41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: nodejs#34993 PR-URL: nodejs#41677 Refs: nodejs#41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: #34993 PR-URL: #41677 Refs: #41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: #42603

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: #34993 PR-URL: #41677 Refs: #41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: nodejs/node#34993 PR-URL: nodejs/node#41677 Refs: nodejs/node#41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ljharb added the fs Issues and PRs related to the fs subsystem / file system. label Aug 31, 2020

ljharb requested a review from BridgeAR August 31, 2020 04:24

ljharb mentioned this pull request Aug 31, 2020

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

Closed

4 tasks

ljharb force-pushed the ljharb/fs_allow_stringifiable_object branch from 2a420f6 to 1e941c0 Compare August 31, 2020 04:26

mscdex reviewed Aug 31, 2020

View reviewed changes

Comment thread lib/internal/fs/utils.js Outdated

ljharb force-pushed the ljharb/fs_allow_stringifiable_object branch from 1e941c0 to f31b352 Compare August 31, 2020 04:56

ljharb requested a review from mscdex August 31, 2020 04:57

joyeecheung approved these changes Sep 2, 2020

View reviewed changes

ljharb force-pushed the ljharb/fs_allow_stringifiable_object branch from f31b352 to bfab9d0 Compare September 2, 2020 19:58

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2020

bmeck suggested changes Sep 9, 2020

View reviewed changes

ljharb force-pushed the ljharb/fs_allow_stringifiable_object branch from bfab9d0 to 7a169e8 Compare September 9, 2020 19:56

mcollina approved these changes Sep 10, 2020

View reviewed changes

benjamingr approved these changes Sep 10, 2020

View reviewed changes

Trott force-pushed the ljharb/fs_allow_stringifiable_object branch from 21d5020 to 9cf9e4a Compare September 15, 2020 23:47

Trott merged commit 9cf9e4a into nodejs:master Sep 15, 2020

ljharb deleted the ljharb/fs_allow_stringifiable_object branch September 16, 2020 04:42

ruyadorno mentioned this pull request Sep 21, 2020

V14.12.0 proposal #35285

Merged

4 tasks

chipx86 mentioned this pull request Jan 9, 2021

Fix a breakage when using the plugin with --source-map on Node 14+. less/less-plugin-autoprefix#41

Open

This was referenced Jun 25, 2021

writeFileSync data as Object throws type error despite docs listing Object as a supported type #39152

Closed

doc: add annotation to writeFile data as Object #39167

Closed

aduh95 mentioned this pull request Feb 13, 2022

fs: fix write methods param validation and docs #41677

Merged

aduh95 mentioned this pull request Feb 28, 2022

doc: deprecate coercion to string in fs.write, fs.writeFile, fs.appendFile #42149

Merged

fs: loosen validation to allow objects with an own toString function by ljharb · Pull Request #34993 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

fs: loosen validation to allow objects with an own toString function#34993Trott merged 1 commit intonodejs:masternodejs/node:masterfrom ljharb:ljharb/fs_allow_stringifiable_objectljharb/node:ljharb/fs_allow_stringifiable_objectCopy head branch name to clipboard

Conversation

ljharb commented Aug 31, 2020 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

BridgeAR commented Aug 31, 2020

Uh oh!

ljharb commented Aug 31, 2020

Uh oh!

Trott commented Sep 2, 2020

Uh oh!

Trott commented Sep 2, 2020

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

ljharb commented Sep 2, 2020

Uh oh!

ljharb commented Sep 2, 2020

Uh oh!

Trott commented Sep 9, 2020

Uh oh!

Trott commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeck commented Sep 9, 2020

Uh oh!

nodejs-github-bot commented Sep 9, 2020

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

ljharb commented Sep 9, 2020

Uh oh!

bmeck commented Sep 9, 2020

Uh oh!

ljharb commented Sep 9, 2020

Uh oh!

bmeck commented Sep 9, 2020

Uh oh!

ljharb commented Sep 9, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 15, 2020

Uh oh!

Trott commented Sep 15, 2020

Uh oh!

ljharb commented Sep 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

fs: loosen validation to allow objects with an own toString function#34993
Trott merged 1 commit into
nodejs:masterfrom
ljharb:ljharb/fs_allow_stringifiable_object

ljharb commented Aug 31, 2020 •
edited by Trott
Loading

Trott commented Sep 9, 2020 •
edited
Loading