Filename can be a nested path by sebdeckers · Pull Request #355 · form-data/form-data

sebdeckers · 2017-06-05T11:37:22Z

TL;DR This patch allows setting of arbitrary filename to support uploads of directories. This matches current spec and behaviour of Chrome.

Example

for (const filepath of ['foo/bar.txt', 'foo/baz.txt']) {
  form.append('uploads', createReadStream(filepath), {filepath})
}
// Content-Disposition: form-data; name="uploads"; filename="foo/bar.txt"
// Content-Disposition: form-data; name="uploads"; filename="foo/baz.txt"

Edit: Updated to reflect latest patch.

Description

The FormData .append spec defers to the File API spec which states:

There are numerous file name variations and conventions used by different underlying OS file systems; this is merely the name of the file, without path information.

However, the HTMLInputElement.webkitdirectory spec supports relative paths, and Chrome sets them as the filename=... in the form field.

This is also supported by Multer, for example.

Caveat Emptor

~~Breaking change for anyone manually specifying filename as a full path. Does not change behaviour on auto-detecting of filename from stream, http, etc.~~

Edit: The filepath option overrides filename. This PR is no longer a breaking change.

Different purpose:

path.normalise cleans up .. and de-dupes path separators, but retains platform-specific path separators.

normalize-path converts Windows \ path separator to forward slash.

TBH I didn't check if the spec calls for forward slashes but seems consistent with all web-related paths.

I see.

Probably we still want to clean up don't, './photos/toys/unicycle.jpg' -> 'photos/toys/unicycle.jpg', as for win -> unix, we could do something like:

filename = filename.replace('\\', '/');

alexindigo · 2017-06-11T04:46:38Z

  // fs- and request- streams have path property
  // formidable and the browser add a name property.
-  var filename = options.filename || value.name || value.path;
+  var filename = options.filepath || options.filename || value.name || value.path;


What do you think doing it in "one-stroke" fashion?

Something like:

var filename = options.filepath ? path.normalize(options.filepath) : path.basename(options.filename || value.name || value.path);

And add basename call to the streams.

The main reason is to handle options here and don't "leak" them further into the function.

Totally agree. Will clean this up.

alexindigo · 2017-06-11T04:48:02Z

Thanks for the update. Looks good in general, just want to discuss implementation details.

coveralls · 2017-06-11T05:46:38Z

Coverage increased (+0.04%) to 97.99% when pulling a8f6303 on sebdeckers:patch-1 into d7398c3 on form-data:master.

sebdeckers · 2017-06-11T05:50:42Z

(Sorry for the coveralls spam ... It seems to leave a comment on every rebase.)

Fixed as per review:

Removed normalize-path dependency in favour of .replace('\\', '/')
Refactored filename assignment to avoid mutating the variable.

Also fixed:

contentType detection based on both options.filepath and options.filename

Edit: Oops, broke again in Windows. Hang on.

coveralls · 2017-06-11T05:52:24Z

Coverage increased (+0.03%) to 97.98% when pulling 7f0fc80 on sebdeckers:patch-1 into d7398c3 on form-data:master.

alexindigo · 2017-06-11T05:54:22Z

Don't worry about coveralls :) With greenkeeper gone from this repo, it's all nice and quiet :)
And thank you for walking extra mile :)

alexindigo · 2017-06-11T06:07:34Z

-    filename = value.client._httpMessage.path;
+  if (typeof options.filepath === 'string') {
+    // custom filepath for relative paths
+    filename = path.normalize(options.filepath.replace(/\\/g, '/'));


Haha :) My bad. Sorry 👍

coveralls · 2017-06-11T06:08:23Z

Coverage increased (+0.03%) to 97.98% when pulling b9f38fb on sebdeckers:patch-1 into d7398c3 on form-data:master.

coveralls · 2017-06-11T06:15:46Z