Filenames with a '\' are not returned as entered

[mclark-newvistas]

What I tried: uploading a file foo\n.csv.xls to test my errors logs

What happened: .name was set to n.csv.xls, so my error logs show incorrect data

What I expected to happen: .name set to foo\n.csv.xls

Line causing the problem: https://github.com/felixge/node-formidable/blob/2f9db9fd87a833c1bd095041e78317557172b7f7/lib/incoming_form.js#L438

[xarguments]

File.name should contain name only, its expected behavior.
Otherwise it can cause security issues when file path is written as file name. So this is the case - it's not a bug, its a feature. (Perhaps this issue needs to be closed).

[xarguments]

@mclark-newvistas, if you agree and there is no other (missed) case, then I'm going to close this.
(Note: In case of not getting response, this issue will get closed after 10 days)

[mclark-newvistas]

@xarguments that was and is the file name. There is no path element in the file named foo\n.csv.xls. This is a valid file name in the operating system I am running. If you intend for your code to save the file name in a Windows-safe environment, you aren't filtering enough characters; conversely, if you mean to accept valid file names, you are changing the uploaded file name without warning, hence the bug report.

[tunnckoCore]

var filename = match.substr(match.lastIndexOf('\') + 1);

Definitely don't looks like correct way to get filename. We should change that, asbolutely, path.basename.

Thanks for the report.

[xarguments]

If you intend for your code to save the file name in a Windows-safe environment, you aren't filtering enough characters

@mclark-newvistas Agree with this, file name security checks are not that good, and can be improved. Yet allowing \ would directly impose serious security issues on Windows systems. So we need to find other solutions.

Definitely don't looks like correct way to get filename. We should change that, asbolutely, path.basename.

@tunnckoCore, checked its code, the path.basename will check for slash and backslash, depended on server OS:

1. It will not have consistent behaviour between OS-es. It will still disallow \ (which OP wanted) when server is running on Windows.
2. When server OS is a Unix/Linuxoid - then Windows clients may be under risk of serious security issue if they receive file names with \.
3. Not sure of this one, but path.basename seems to have complex logic with extra function calls. And AFAIK in Formidable - filename is set when handling each part (?) so it may have impact on performance (?) Perhaps will need more investigation and benchmark.

Generally, no matter which way we go - we should choose to either allow or disallow \
on all OSes consistently, and still keep backward compatibility so that older apps don't change behaviours.
The solution may be to defer that choice to the library user, by adding a config, like allowBackslashInFileName. And add a very big note in docs like this:

Warning: It may pose Windows servers/clients under risk when they receive file names with `\`.
Make sure to handle/sanitize file name properly before using it as an actual file name.

What would you say?

[tunnckoCore]

O, seems like I didn't get that issue correctly back then.

Agree. Instead of this option it can be getFilename(match) function, so allow them do whatever they want.

[mclark-newvistas]

@xarguments I'm fine w/OS dependent behavior personally, though I recognize you might feel it violates the Principle of Least Surprise for enough of your users to matter.

Given your reasonable preference for consistency, I'd recommend taking the same approach I've seen web browsers take when downloading an invalid filename. Replace the offending character with something else - typically a _. I would not have been very surprised had my filename coming from Formidable been foo_n.csv.xls, but was extremely surprised that it was truncated.

Adding a flag is typically not the best approach - increases configuration complexity, documentation surface area, etc. Adding a flag which can introduce a security hole even more so.

[mclark-newvistas]

Another option along the lines of "validate your inputs, sanitize your outputs" would be to have some kind of error if the filename fails to match internal validation rules, and ask the library user to report an error to their user for them to chose another filename. The above suggestion implements "sanitize your outputs"; this one would be to "validate your inputs".

The current approach amounts more to "sanitize your inputs", which has a nasty habit of munging data, as observed.

[xarguments]

@mclark-newvistas, thanks a lot for useful suggestions.

About replacing slashes with with "_": it seems to be another efficient way to go without breaking most of old apps. That way very poorly written apps may get broken only.

About "validate your inputs": Agree, it would enforce more security to the apps. Yet the lib itself is "low-level package" that is used by many many other libs. It shouldn't do or enforce much validations/sanitizations/security since it's primary job is file uploads, not file security.
Unix way - "Write components that do one thing and do it well; write components that work together".

Personally, I think we need to create (or use) a file-name validation/sanitization library, and use it in conjuction with Formidable (not "from within Formidable"). Afterwards, add an example to docs so that users were aware of it.
I had created such library for file name input validation/sanitization which conforms to many file name RFCs and security checklists. Perhaps some day I'll port it to JS and open-source it (when have time).

We will still come back to this when implementing newer version, and - based on backward compatibility of the change - we will add it to v1.3 or v2.0.

Keeping this ticket open for more suggestions/discussions.
Thanks.

[GrosSacASac]

I think that top security should be the default. The file name converter and validator should be configurable as well.

[tunnckoCore]

@xarguments: I had created such library for file name input validation/sanitization which conforms to many file name RFCs and security checklists. Perhaps some day I'll port it to JS and open-source it (when have time).

Great! <3

For now we can use something like https://npm.im/slugify

[GrosSacASac]

or https://github.com/sindresorhus/filenamify
as opposed to slugifiy it does not convert valid filename characters like $ or space

[tunnckoCore]

Oh yea, that was it, i was trying to remember how it was called.

Plus https://github.com/sindresorhus/unused-filename.

[tunnckoCore]

#94 & #358 are kind of related too

[GrosSacASac]

related #664