search

node-formidable / formidable

The most used, flexible, fast and streaming parser for multipart form data. Supports uploading to serverless environments, AWS S3, Azure, GCP or the filesystem. Used in production.
MIT License
7k stars 680 forks source link

uniq filenames #808

Closed jimmywarting closed 2 years ago

jimmywarting commented 2 years ago

Whenever i encounter a "need to generate a uniq filename" and including dependencies and so forth like hexoid to generate them https://github.com/node-formidable/formidable/blob/1618ea060326d8c510cf19f06600b79186c84f98/package.json#L34 and seeing stuff like: https://github.com/node-formidable/formidable/blob/1618ea060326d8c510cf19f06600b79186c84f98/src/Formidable.js#L517

Then i might sometimes link to this article: https://advancedweb.hu/secure-tempfiles-in-nodejs-without-dependencies/

fsPromise.mkdtemp is suppose to help you with this...

how about instead generate one uniq dir with mkdtemp and simple use the filename n+1 instead (without extension)?

const uploadDir = await fs.mkdtemp(await fs.realpath(os.tmpdir()) + path.sep);

my proposal: remove hexoid

GrosSacASac commented 2 years ago

Replace hexoid with crypto module ?

As for directory traversal attacks tentative, they can only occur if options.filename is used, in which case the dev already opted out of formidable name generation.

The uploadDir should not be temp because files are made for multiple reuse, the default however is os.tmpdir() which contradicts , but I think only for tests reason (I think most people don't use the default)

If the dev wants to not store the file he can use options.fileWriteStreamHandler.

tunnckoCore commented 2 years ago

Hexoid is used only for filenames I think, and the reason is better naming and safer defaults. Avoiding the bad names and characters, Windows defaults, and what not.

I think using something small and efficient like cuid or hexoid is a lot better than the crypto module. They are optimized, they are small, they does great job exactly for such cases. And it's better for bundling cases.

The defaults are safe and secure. Changing comes with responsibility to handle additional things, and reading more docs, as always. We can't, should not, and won't do everything on user's behalf. If you don't want the files or want to "clean up after", just don't write them to the disk in the first place, that's the options.fileWriteStreamHandler option for.

But thanks for the suggestions :wink:

tunnckoCore commented 2 years ago

User responsibility. We provide sane defaults. And yes, it helps for such attacks, at least to some extent. There's far more stupid shit out there that does NOTHING about such things.

Things will change soon anyway.

Closing.