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

Why does tmp remove quotes from paths ? #268

Closed
8 tasks done
zvin opened this issue Jan 29, 2021 · 13 comments · Fixed by #278
Closed
8 tasks done

Why does tmp remove quotes from paths ? #268

zvin opened this issue Jan 29, 2021 · 13 comments · Fixed by #278
Assignees

Comments

@zvin
Copy link

zvin commented Jan 29, 2021

Operating System

  • Linux
  • Windows 7
  • Windows 10
  • MacOS
  • other:

NodeJS Version

  • 0.x
  • 4.x
  • 6.x
  • 7.x
  • other:

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
Copy link
Author

zvin commented Jan 29, 2021

This was introduced in c8823e5

@zvin
Copy link
Author

zvin commented Jan 29, 2021

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
Copy link
Collaborator

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:

  • t\ e
  • t" "e (also valid on windows/dos)
  • t' 'e (valid on linux and no way to tell whether the quotes are part of the name or used for quoting the whitespace)
  • 't e' (here, one could argue that enclosing single quotes are meant for quoting the whitespace)
  • "t e"

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

  • 't/f 'oo\ '/ 'bar

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
Copy link
Author

zvin commented Feb 5, 2021

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
Copy link
Collaborator

To recap

  • tmp must not alter the node provided / environment provided path, i.e. it must not remove any occurrences of " or '.
  • 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)
  • usage of ` must be strictly forbidden (linux and other systems use it as a backtick operator)

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
Copy link
Author

zvin commented Feb 10, 2021

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
Copy link
Collaborator

silkentrance commented Jul 31, 2021

@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
Copy link
Collaborator

silkentrance commented Jul 31, 2021

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
Copy link
Collaborator

@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
Copy link
Author

zvin commented Aug 4, 2021

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
Copy link

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
Copy link
Contributor

mbargiel commented Aug 23, 2022

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
Copy link
Contributor

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.

mbargiel added a commit to mbargiel/node-tmp that referenced this issue Aug 23, 2022
…gle quotes from os.tmpdir also sanitize dir option, the template option and the name option"

This reverts commit c8823e5.

Single quotes must not be removed from paths because they are valid (even if hard to use)
on all OSes. Double quotes are only disallowed on Windows, but tmp should not change any
arg it gets; instead, it should rely on the underlying fs API to fail with an error
that the user needs to fix.
@silkentrance silkentrance self-assigned this Aug 25, 2022
mbargiel added a commit to mbargiel/node-tmp that referenced this issue Aug 26, 2022
silkentrance added a commit that referenced this issue Aug 26, 2022
…-name

fix #268: Revert "fix #246: remove any double quotes or single quotes…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants