raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
732 stars 93 forks source link

Why does tmp remove quotes from paths ? #268

Closed zvin closed 1 year ago

zvin commented 3 years ago

Operating System

NodeJS Version

Tmp Version

0.2.1

Expected Behavior

Don't remove quotes from paths

Experienced Behavior

Quotes are being removed

See https://github.com/raszi/node-tmp/blob/master/lib/tmp.js#L564-L575

For example on Windows the tmpdir is in general in C:\Users\username\AppData\Local\Temp. A username may contain a quote and / or a double quote (Bob's Computer for example).

_getTmpName calls _sanitizeName on os.tmpdir() and returns a folder that does not exist.

zvin commented 3 years ago

This was introduced in c8823e549280e11697a510184a69b63bf5bfef7a

zvin commented 3 years ago

The correct fix for #246 was just to use set TEMP=C:\users\xxx\T M P as the issue author commented. Please revert

silkentrance commented 3 years ago

Double quotes are reserved characters and must not be used in filenames.

As for single quotes they have also been eliminated since on linux there is no way to tell whether the quotes are part of the name or are being used for quoting a string with whitespace in it.

E.g. these are valid path names on linux for a directory called t e that contains a single whitespace between t and e:

And it becomes even more complicated when spanning directory tree nodes, e.g.

so if you fix the other then you introduce a whole lot of possibilities for the user to break things by passing in weirdly quoted paths.

Any ideas?

zvin commented 3 years ago

You don't need to / must not alter paths in this library. If they come from an env var, just escape it properly in your shell.

In bash:

$ X="t e" node -e "console.log(process.env.X)"
t e
$ X='t e' node -e "console.log(process.env.X)"
t e
$ X=t\ e node -e "console.log(process.env.X)"
t e

In cmd:

> set X=t e
> node -e "console.log(process.env.X)"
t e

A directory named t e is named t e, nothing else. All quotes, double quotes, antislashes or anything else is just a way to escape special characters that depends on your shell.

silkentrance commented 3 years ago

To recap

What about whitespace normalization?

In the past, whitespace has been used for "hiding" directories on the server once that it was compromised, e.g. / / /zero-warez/ware1.iso.part1 and so on.

ATM tmp normalizes the whitespace so that it will reduce all surrounding whitespace to just one whitespace and prevents use of whitespace only components in a given path.

zvin commented 3 years ago

usage of ` must be strictly forbidden (linux and other systems use it as a backtick operator)

This is not true:

$ touch '`'
$ ls
'`'

Note that the file is named `, the quotes around it are added by ls Capture d’écran de 2021-02-10 18-20-24

usage of " is restricted under windows and cannot be used as part of a file or directory name whereas in linux and other such systems this is a valid character (properly escaped that is)

What about whitespace normalization?

In the past, whitespace has been used for "hiding" directories on the server once that it was compromised, e.g. / / /zero-warez/ware1.iso.part1 and so on.

Why should tmp care ?

ATM tmp normalizes the whitespace so that it will reduce all surrounding whitespace to just one whitespace and prevents use of whitespace only components in a given path.

Just don't change the filenames / paths, there is no good reason to do so.

silkentrance commented 2 years ago

@zvin user names, when added through the local users and groups management cannot contain any of these special characters

image

what you are talking about is an abstraction that was added by windows 10 or before, where users can have arbitrary names, yet, these will be slugged into a filesystem compatible name.

so if you created your user, like i did in a previous installation of windows 10, using for example the email address, this will then be truncated to just a few characters of that address, say foobar@example.org will then be slugged down to just foo. And this is where your home directory is located in. And which is also the path that node or other apps try to resolve the root of your home directory from.

And even if you use " or / in your user name, these will still not be available in the filesystem, as these characters are invalid.

Regardless, we, that is I stand by the decision that quotes (and other stuff) must be eliminated from tmp generated names or directories unless you prove me wrong.

silkentrance commented 2 years ago

however, i do not know, why the { and } are not prohibited also, as they represent special characters in the filesystem, too, which will then be interpreted by windows... say guids for registered components and whatnot, seems to be a security issue here.

silkentrance commented 2 years ago

@zvin and we did not have any complaints since the changes were in effect. however, i do not know whether anyone uses the new version as we do not get any feedback, except for yours, of course.

zvin commented 2 years ago

Alright, keep your bugs then.

users can have arbitrary names, yet, these will be slugged into a filesystem compatible name

This is not true, you can create users with ampersands and quotes in the username on Windows 10, these characters will remain in the folder name on the filesystem.

we did not have any complaints since the changes were in effect.

There was at least me, the author of #246 and a thumbs up above.

stand by the decision that quotes (and other stuff) must be eliminated from tmp generated names or directories.

And thus pointing to files or directories that do not exist.

katrinalui commented 2 years ago

Just want to chime in with my "complaint." This is causing issues for us as some Windows users have apostrophes in their profile name, leading to temp file paths with nonexistent directories when using this library.

mbargiel commented 1 year ago

My project recently updated tmp from 0.1.0. We ran into this very issue as well. We use arbitrary paths to create temporary folders with random names and those folders can include apostrophes (yes, including in Windows user names) or potentially other legal characters.

I agree with @zvin. Whatever tmpdir option value is passed, please do not change it, let consumers do whatever normalization they want (if any) or let underlying fs API calls fail. Don't make assumptions about what's good for users; they know better.

mbargiel commented 1 year ago

Issue #246 was invalid. The reporter did not use SET correctly. If they had tried any fs.mkdirSync(process.env.TEMP) after using SET TEMP="C:\bad path" it would have failed with EINVAL: invalid argument, mkdir '"C:\bad path"' due to the extraneous quotes.

Fun fact: mkdir 'test' in cmd.exe on Windows creates a folder named 'test', single-quotes included. Same for mkdir "'test'" on Mac. So stripping single quotes, at any rate, is incorrect.