Wrong filename on file upload

Extraction of the filename from a URL handles the parameters differently now by returning the string after the last slash till its following semicolon as FileName.

BUG=323156
Committed: https://crrev.com/dec80a672081aa2993723e0b9f8ff14a05693505
Cr-Commit-Position: refs/heads/master@{#299848}

[arun87.kumar, 2014-09-11 13:24:24]

arun87.kumar@samsung.com changed reviewers:
+ asanka@chromium.org, saswat@chromium.org

[arun87.kumar, 2014-09-11 13:25:31]

On 2014/09/11 13:24:24, arun87.kumar wrote:
> mailto:arun87.kumar@samsung.com changed reviewers:
> + mailto:asanka@chromium.org, mailto:saswat@chromium.org



Hi Asanka and Saswat,

I would like to work on Wrong FileName Upload issue (323156). I have uploaded a
patch for the same.

With this patch, DoExtractFileName() will now return the string after the last
slash as FileName. 

Also, the patch handles if semicolons are present in filename, it treats them as
undesired segment and doesnot include them as part of FileName. (Please let me
know if this part of code change is not necessary).


PTAL.


Regards,
Arun

[Joao da Silva, 2014-09-11 13:36:02]

joaodasilva@chromium.org changed reviewers:
+ brettw@chromium.org

[Joao da Silva, 2014-09-11 13:36:02]

Most of this code was written by Brett. I think he's the most qualified reviewer
for this change.

[arun87.kumar, 2014-09-11 13:54:45]

On 2014/09/11 13:36:02, Joao da Silva wrote:
> Most of this code was written by Brett. I think he's the most qualified
reviewer
> for this change.

Thanks Joao da Silva for your info.

[brettw, 2014-09-11 17:06:13]

We would need to get some good nit tests for this. Looking at the examples in
the test I'll be able to more easily see whether I think this new behavior looks
correct.

[arun87.kumar, 2014-09-12 13:53:40]

On 2014/09/11 17:06:13, brettw wrote:
> We would need to get some good nit tests for this. Looking at the examples in
> the test I'll be able to more easily see whether I think this new behavior
looks
> correct.


Hi brettw,

I have added few unit testcases and also modified code to consider substring
before last semicolon in the filename segment, as part of filename.


PTAL.




Regards,
Arun

[arun87.kumar, 2014-09-15 07:27:02]

@brettw,

I am newbie to chromium opensource. Please let me know if nit cases are
different from unit test cases.
If so, please share some pointers on how to write one.





Regards,
Arun

[brettw, 2014-09-16 22:37:10]

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
File url/third_party/mozilla/url_parse.cc (right):

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:632: if (!is_file_end_fixed && spec[i] ==
';') {
It seems that this behavior takes everything between the last slash and (if it
exists) the last semicolon. Are we sure it's not supposed to be the first
semicolon?

i.e. given "foo/bar;baz;lala" your patch returns "bar;baz" and I'm wondering if
it shouldn't be "bar". I think "bar" would also be easier to code since you
could remove the is_file_end_fixed flag and just change file_end (this would
also have the effect of trimming parameters for the "degenerate" case at the
bottom of this function, which I guess is good.

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:635: }else if (IsURLSlash(spec[i])) {
Space before "else"

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:636: // search backwards from the filename
end to the previous slash
This comment should be updated (this code no longer does any searching).

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:643: 
Watch out for extra spaces.

https://codereview.chromium.org/560283003/diff/20001/url/url_parse_unittest.cc
File url/url_parse_unittest.cc (right):

https://codereview.chromium.org/560283003/diff/20001/url/url_parse_unittest.c...
url/url_parse_unittest.cc:510: // Test cases added to verify Issue - 323156
I don't think you need this comment. Can you add some tests with multiple
semicolons also?

[arun87.kumar, 2014-09-17 11:39:13]

Dear brettw,

Thanks for your valuable comments. 

Regarding, whether semicolon should be included in the filename or not, i too
felt that incase of "foo/bar;baz;lala", the filename should be "bar".
But as per one of the already existing unit testcases (9th case) in
url_parse_unittest.cc, the input and output were as below.
{"http://www.google.com/foo/bar.html;foo;param#ref", "bar.html;foo"}.

So i had added is_file_end_fixed logic to pass this test. Also same beahviour
was seeen in other browsers like Firefox and IE.

Anyways, now i have modified the patch to exclude semicolon and its subsequent
string from filename.


PTAL.


Regards,
Arun

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
File url/third_party/mozilla/url_parse.cc (right):

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:632: if (!is_file_end_fixed && spec[i] ==
';') {
i too felt that incase of "foo/bar;baz;lala", the filename should be "bar".
But as per one of the already existing unit testcases (9th case) in
url_parse_unittest.cc, the input and output were as below.
{"http://www.google.com/foo/bar.html;foo;param#ref", "bar.html;foo"}.

So i had added is_file_end_fixed logic to pass this test. Also same behaviour
was seeen in other browsers like Firefox and IE.

Anyways, now i have modified the patch to exclude semicolon and its subsequent
string from filename.

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:635: }else if (IsURLSlash(spec[i])) {
On 2014/09/16 22:37:10, brettw wrote:
> Space before "else"

Done.

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:636: // search backwards from the filename
end to the previous slash
On 2014/09/16 22:37:09, brettw wrote:
> This comment should be updated (this code no longer does any searching).

Done.

https://codereview.chromium.org/560283003/diff/20001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:643: 
On 2014/09/16 22:37:09, brettw wrote:
> Watch out for extra spaces.

Done.

https://codereview.chromium.org/560283003/diff/20001/url/url_parse_unittest.cc
File url/url_parse_unittest.cc (right):

https://codereview.chromium.org/560283003/diff/20001/url/url_parse_unittest.c...
url/url_parse_unittest.cc:510: // Test cases added to verify Issue - 323156
On 2014/09/16 22:37:10, brettw wrote:
> I don't think you need this comment. Can you add some tests with multiple
> semicolons also?

Done.

[asanka, 2014-09-17 20:56:25]

https://codereview.chromium.org/560283003/diff/60001/url/third_party/mozilla/...
File url/third_party/mozilla/url_parse.cc (right):

https://codereview.chromium.org/560283003/diff/60001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:628: // the first one.
Update the comment?

I'd suggest adding a reference to http://tools.ietf.org/html/rfc3986#section-3.3
which states some caveats about the assumptions we are making here.

Also, the now obsolete http://tools.ietf.org/html/rfc2396#section-3.3 might be
relevant since the concept of a path segment specific parameter existed in RFC
2396 but not in RFC 3986. The latter just mentions that the ';' and ','
characters are sometimes used as delimiters for segment specific parameters.
However a ';' is no longer considered a reserved character.

Twitter uses a ':' as a delimiter for path segment parameters. Should we handle
that too? (https://code.google.com/p/chromium/issues/detail?id=172529)

[arun87.kumar, 2014-09-18 05:24:02]

Dear asanka,

Thank you for your valuable comments. Now i understand why Firefox and IE
include semicolon as part of filenames as it is no more reserved character as
per RFC 3986.

So should we also exhibit the same behaviour?


Also, regarding twitter issue, should we consider ':' as delimiter along with
present ';' logic?




Regards,
Arun

[asanka, 2014-09-19 18:40:23]

On 2014/09/18 05:24:02, arun87.kumar wrote:
> Dear asanka,
> 
> Thank you for your valuable comments. Now i understand why Firefox and IE
> include semicolon as part of filenames as it is no more reserved character as
> per RFC 3986.
> 
> So should we also exhibit the same behaviour?
> 
> 
> Also, regarding twitter issue, should we consider ':' as delimiter along with
> present ';' logic?

RFC 3986 doesn't really specify how to do filename derivation. I brought it up
because the concept of a parameter delimiter for path segments doesn't exist
anymore other than by convention. Filename derivation, as far as I know, is up
to the UA and isn't specified anywhere. The HTML5 recommendation is also to
derive a name from a URL in a "user-agent-defined manner."
(http://www.w3.org/TR/html5/links.html#downloading-resources) The definite and
spec compliant way to specify a filename for a HTTP response is to use a
Content-Disposition header.

The only constraint for us is to respect the path segment boundaries (which are
in the spec) and accommodate expectations of existing content. And the latter
generally follow the behavior of popular browsers.

How do the other browsers handle ';', ':' and ','? It might be that IE et. al.
use more convoluted heuristics than is immediately discoverable (e.g. picking
foo.jpg over foo.jpg;bar if the Content-Type is image/jpeg).

> 
> Regards,
> Arun

[arun87.kumar, 2014-09-20 06:20:35]

Thanks asanka for more detailed information. 


@brettw,

I have included all your suggestions in the latest patch.


PTAL



regards,
Arun

[arun87.kumar, 2014-10-06 04:12:05]

arun87.kumar@samsung.com changed reviewers:
+ abarth@chromium.org

[arun87.kumar, 2014-10-06 04:17:52]

On 2014/09/20 06:20:35, arun87.kumar wrote:

Added abarth@chromium.org to reviewers list. 

abarth and brettw@ - PTAL

[brettw, 2014-10-14 18:28:55]

Code looks right, please update the comment.

https://codereview.chromium.org/560283003/diff/60001/url/third_party/mozilla/...
File url/third_party/mozilla/url_parse.cc (right):

https://codereview.chromium.org/560283003/diff/60001/url/third_party/mozilla/...
url/third_party/mozilla/url_parse.cc:628: // the first one.
On 2014/09/17 20:56:25, asanka wrote:
> Update the comment?

Yes, this needs updating. The whole thing should be rewritten. This loop is no
longer searching backwards for the parameter, but finding the range between the
last slash and the following semicolon.


> Twitter uses a ':' as a delimiter for path segment parameters. Should we
handle
> that too? (https://code.google.com/p/chromium/issues/detail?id=172529)

No, this has nothing to do with how some sites may decide to use certain
characters in a URL.

[arun87.kumar, 2014-10-15 12:02:09]

On 2014/10/14 18:28:55, brettw wrote:
> Code looks right, please update the comment.
> 
>
https://codereview.chromium.org/560283003/diff/60001/url/third_party/mozilla/...
> File url/third_party/mozilla/url_parse.cc (right):
> 
>
https://codereview.chromium.org/560283003/diff/60001/url/third_party/mozilla/...
> url/third_party/mozilla/url_parse.cc:628: // the first one.
> On 2014/09/17 20:56:25, asanka wrote:
> > Update the comment?
> 
> Yes, this needs updating. The whole thing should be rewritten. This loop is no
> longer searching backwards for the parameter, but finding the range between
the
> last slash and the following semicolon.
> 
> 
> > Twitter uses a ':' as a delimiter for path segment parameters. Should we
> handle
> > that too? (https://code.google.com/p/chromium/issues/detail?id=172529)
> 
> No, this has nothing to do with how some sites may decide to use certain
> characters in a URL.

Hi brettw,

I have updated the comments.

PTAL


Regards,
Arun

[brettw, 2014-10-15 17:52:44]

lgtm

[brettw, 2014-10-15 17:53:47]

Before you land, can you update the description? The first line should say
what's changing (extraction of the filename from a URL handles the parameters
different) and not reference the bug number. The bug number is referenced below.

[arun87.kumar, 2014-10-16 03:18:17]

The CQ bit was checked by arun87.kumar@samsung.com

[commit-bot: I haz the power, 2014-10-16 03:20:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/560283003/80001

[commit-bot: I haz the power, 2014-10-16 05:10:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-16 05:11:36]

Patchset 5 (id:??) landed as
https://crrev.com/dec80a672081aa2993723e0b9f8ff14a05693505
Cr-Commit-Position: refs/heads/master@{#299848}