added a keepFilenames option to IncomingForm constructor #488
Conversation
|
Could somebody else add this to the docs? It's a nice-to-have feature :) |
|
i would love this option @xarguments |
kvz
left a comment
There was a problem hiding this comment.
Just wondering about your usecase, how do you handle two users uploading the same avatar.jpg filename? Or even one user?
|
@kvz this is a locally hosted web form so there is only 1 user. Keeping the file name |
|
I’m a bit concerned that newcomers will read this option thinking this is obviously what they’ll want to enable but then end up with deleted/overwritten files in their datastore by the time it hits production, and then will need to come up with more elaborate handling in a very short time (?)
Sent from mobile, pardon the brevity.
… On 14 Oct 2019, at 19:46, Peter Stenger ***@***.***> wrote:
@kvz this is a locally hosted web form so there is only 1 user. Keeping the file name
Would be a nice option.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
I agree, but I don’t have a good solution. |
felixge
left a comment
There was a problem hiding this comment.
This patch introduces an easily exploitable security hole.
| IncomingForm.prototype._uploadPath = function(filename) { | ||
| var buf = crypto.randomBytes(16); | ||
| var name = 'upload_' + buf.toString('hex'); | ||
| var name = this.keepFilenames ? filename : 'upload_' + buf.toString('hex'); |
There was a problem hiding this comment.
Merging this patch would expose users of this flag to giving write access to their whole file system. 🤕
You can try for yourself:
curl -F "upload=@$(pwd)/harmless.jpg;filename=../hahah.exploit" 'localhost:8080/upload'
Please don't merge this or similar patches. If somebody wants to use the original filenames, they can implement it themselves at their own risk.
|
Partly agree with @felixge. We will think something better for next minor or major release. In reality, even if we merge it, code already in production env won't be vulnerable, until it enable it explicitly which won't happen until when they see that option was added. |
Hi all, I found formidable to be a very useful library, but did not see a way to simply use the original filename as the destination pathname, so I am making that pull request in case you feel it can be included.
From what I can tell the code would otherwise use content-disposition headers if they carried filenames with them, but my barebones HTML form didn't send those headers, so I get the randomly generated hex filenames, even though the original name is sitting right on that File object (as
.name).Maybe I'm missing something simple in how to include filenames from the clientside, but either way, let me know!