- 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.
- 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.
- 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.
5.10.1Darwin 15.3.0fsI 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
prefixargument 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 entiremkdtempjust to get valid UUIDv4 directory names? That is genuinely useful for unit tests and also for being able to easilymvthe 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
prefixas a directory path is messy, What would you expect this code to do?If
os.tmpdir()returns/tmpfor 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 liketmp-e0ew3m. To "fix" this you have to usepath.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.
cwdargument, which is internallypath.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.prefixwith (or just add) anameoption, 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.