A more intuitive fs.mkdtemp()

[sholladay]

• Version: 5.10.1
• Platform: Darwin 15.3.0
• Subsystem: fs

I am happy that the Node team decided to implement fs.mkdtemp() - the security concerns around this behavior make me glad to see it in core.

However, the API feels awkward to me because:

• The randomness it provides is hard coded to 6 characters, which is just enough that it's probably fine, but not enough that I don't want to add more.

• Using the prefix argument sounds like a good way to add random characters, but doing half the work is strange if you read code that actually does this. Additionally, if I'm going to implement my own name pattern, I don't really want to also have to keep in mind the one Node is using. For example, should I have to re-implement the entire mkdtemp just to get valid UUIDv4 directory names? That is genuinely useful for unit tests and also for being able to easily mv the temporary directory to a cache-proof URL on my server without "renaming" it ... lest I have to undo the suffix every time or teach my client code about it.

• Using prefix as a directory path is messy, What would you expect this code to do?

  'use strict';
  
  const os = require('os');
  const fs = require('fs');
  
  fs.mkdtemp(os.tmpdir(), (err, dir) => {
      if (err) {
          throw err;
      }
      console.log('Created directory:', dir);
  });

  If os.tmpdir() returns /tmp for you, that will actually create a directory at the very root of your filesystem, rather than inside of /tmp. And it will end up being named something like tmp-e0ew3m. To "fix" this you have to use path.join(os.tmpdir(), '/').

I would like to propose making the API more intuitive and useful for its intended purpose: unique, convenient, secure creation of temporary directories.

1. Have an explicit cwd argument, which is internally path.join()'d with the name. This is so that the name can be computed in isolation. And to prevent accidentally creating directories out in the open, where they won't actually be cleaned up.
2. Replace prefix with (or just add) a name option, which is used as-is when provided. Possibly increase the default randomness for the case where one is not provided. This makes it easy to opt-out of the pattern that Node happens to use currently, which is not exposed via any kind of constant (and I don't think that would provide much value, anyway).

I think these changes are best done as a breaking change, mainly to make the API less surprising. But compatibility could probably be maintained by only enabling these semantics on the options object.

[bnoordhuis]

The randomness it provides is hard coded to 6 characters, which is just enough that it's probably fine, but not enough that I don't want to add more.

That's a limitation of libc's mkdtemp(3) implementation, which fs.mkdtemp() is a fairly thin binding to.

It's possible to implement it from scratch but then we miss out on platform-specific tweaks (which might be good for consistency but could be bad for security or interoperability.)

[benjamingr]

ping @ralt

[jasnell]

I'm not sure if there's much more we should do other than improve the documentation to make this clearer. Providing a separate tmpdir argument would be inconsistent with the other fs methods and as @bnoordhuis points out there's little we can do as far as the randomness of the prefix. A PR that expands the documentation would be quite helpful!

@nodejs/documentation

[sholladay]

... would be inconsistent with the other fs methods

I am not aware of any other fs methods that have this prefix argument / behavior. I am actually trying to propose something that is more consistent. Namely, I believe that passing os.tempdir() as the first argument should do something reasonable, as the other methods do (see my third bullet point).

Better API ideas aside, I found the current behavior awkward as a library author. I am willing to put up with my paths being misunderstood by the path module, but not by fs. It gets to walk to the filesystem, it needs to behave the same with or without a trailing slash.