Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows (escaped) double quotes and semicolons in the filename. #86

Closed
wants to merge 1 commit into from

Conversation

sled
Copy link

@sled sled commented Jul 26, 2011

Hi,

I ran in the problem that I couldn't upload files with semicolons in the filename. I tried the patch from abram (#58) but this patch raises problems with filenames that have double quotes in their filename.

So I came up with this patch to fix both issues.

As an example:

The file b'e";e.JPG on my local machine is being uploaded, so the multipart header looks like (wireshark excerpt):

Content-Disposition: form-data; name="file"; filename="b'e\";e.JPG"

Note that the browser did escape the double quotes properly.

Thanks for the great module!

Simon

@sled
Copy link
Author

sled commented Jul 26, 2011

Hi,

I just discovered that there are browser specific differences how the encoding of the filename is handled:

Firefox just escapes the double quotes like "

Safari Webkit applies encodeURIComponent on the filename so the double quote becomes %22

@felixge
Copy link
Collaborator

felixge commented Jul 27, 2011

Does your patch handle those differences in browser encodings?

@@ -278,7 +278,8 @@ IncomingForm.prototype._initMultipart = function(boundary) {
part.name = m[1];
}

if (m = headerValue.match(/filename="([^;]+)"/i)) {
if (m = headerValue.match(/filename="([^"\\]*(?:\\.[^"\\]*)*)"/i)) {
m[1] = m[1].replace(/\\"/ig, '"');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

The main thing is Line 281 which does nothing else than prevent formidable from crashing when a " occurs in the filename (and it allows semicolons too) because it literally just takes everyhing between the quotes of filename="......"

I handle the encoding for FF only. On Line 282 I turn " into " but I didn't knew that there are difference between browsers at this time.

So there are two options:

  1. Handle all browser specific differences inside of formidable (means detecting user agent etc.)
  2. Remove line 282 so formidable delivers the raw string between the quotes of filename="....."

I'd vote for second solution so the coder is responsible for decoding the filename properly. So my patch just allows escaped quotes in the filename and prevents cutting of the filename if a semicolon occurs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solution 2 sounds good. Want to modify the patch for that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

sorry I was wrong you need Line 282 because of Line 283:

283: part.filename = m[1].substr(m[1].lastIndexOf('\\') + 1);

this cuts off leading paths like c:\windows\file.pdf but if the filename is a \"simple\" file.pdf it would result in " file.pdf if you remove Line 282.

So this means you can apply the patch as it is if all tests pass.

@OrangeDog
Copy link
Contributor

Having looked at this myself (bug #84) I don't think a regular expression is going to be able to do the job. A simple char-by-char parser is probably required in order to deal with all the different ways browsers encode different characters.

@sled
Copy link
Author

sled commented Aug 16, 2011

Hi,

I think the regex should solve your problem #84, could you give it a try? The regex does not parse anything except replacing " with " which is needed to remove the preceeding path correctly.

The parsing should be done after line #282 where the plain filename is available.

@tunnckoCore
Copy link
Member

Thanks for this, but we will work on something more general, as discussed in #465.

It will land in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants