1440677 - herokuapp.com - Problems with spaces in filenames (content-disposition not quoted)

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[hartmantam.enc]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180206200532

Steps to reproduce:

Access https://fathomless-shelf-94559.herokuapp.com/


Actual results:

Download the file with filename `Junk`


Expected results:

The file name should be `Junk Text.txt`

Comment 1

[hartmantam.enc]

This error occur when the filename parameter contains space. Other browser such as Chrome, Edge, Vivaldi doesn't have this parsing problem.

Comment 2

[Gingerbread Man]

NI :bz because bug 221028 is now in Core Graveyard, so it doesn't seem right to dupe this to it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

It's still the right place; I have no idea why someone stuck it in "graveyard".

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that if there were a clear spec for how to support space here we could consider switching to it.  But right now, the only spec for this stuff explicitly days spaces are only allowed inside a quoted string.

Comment 5

[hartmantam.enc]

For connivance to other users, why stuck with spec while other vendors provide the functionality? I understand the importance of following spec. However, it will just drive end user crazy then switch to others. I think user experience take more place in this case than following spec.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Last I checked, there were cases in which the other browsers have incorrect behavior when the server is doing the right thing, as a result of them wanting to support this spec deviation...

Again, if there is a behavior that does _not_ have that problem and is clearly described switching to it would not be a problem.  But fixing up this sort of server error at the expense of breaking other things is not necessarily a great tradeoff.

Comment 7

[Gingerbread Man]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> It's still the right place; I have no idea why someone stuck it in
> "graveyard".

The entirety of Core :: File Handling was moved to Core Graveyard :: File Handling. So bug 221028 and its myriad of dupes ended up there. Your previous decision is at bug 221028, comment 7 and your explanation at bug 221028, comment 88.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Anne, has this come up in the fetch/etc work?

Comment 9

[Anne (:annevk)]

No, I actually thought Julian had solved Content-Disposition by creating rather exhaustive tests for it, but I guess other browsers didn't bother to fix their bugs?

Comment 10

[Anne (:annevk)]

Looking at comment 7 I suspect Julian wouldn't disagree with Julian from 6 years ago, but if other browsers are forcing our hand here, it might be time to reconsider? It does seem like there's a huge number of duplicates for this bug...

Comment 11

[Julian Reschke]

My position on this hasn't changed. Get servers to send valid header fields instead of competing on lazy parsing.

If a sender doesn't quote whitespace, it'll likely make other mistakes, such as handling non-ASCII incorrectly, or including other disallowed ASCII characters.

Comment 12

[hartmantam.enc]

So how will Firefox dev team handle this? Fix it or following the standard?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think that to "fix it" there needs to be a specific behavior proposed.  Then we can at least evaluate whether that behavior is reasonable...  Ideally all browsers would implement the same behavior here, obviously.

Comment 14

[hartmantam.enc]

Then obviously Firefox behavior of handling spaces in Content-Disposition is not equal to other majority of browsers such as Chrome.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I have no idea what comment 14 is trying to say, for what that's worth.

Comment 16

[hartmantam.enc]

I'm trying to respond comment 13, where you said 
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> Ideally all browsers would implement the same behavior here, obviously.
Firefox don't do the same as other browsers like Chrome if there were spaces in Content-Disposition header.

Comment 17

[hartmantam.enc]

In addition to comment 16, should the dev team make Firefox behave like other browsers?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Probably yes, if the other browsers actually agree on all the edge cases (last I checked they didn't) and if there is a clear description of how they behave, instead of us implementing something that doesn't match them anyway.  That's what I've said repeatedly in this issue.

Comment 21

[:Gijs (he/him)]

This is networking code, not frontend code, so moving and needinfo'ing so this gets triaged despite being 2 years old.

Comment 22

[Kershaw Chang [:kershaw]]

(In reply to :Gijs (he/him) from comment #21)

This is networking code, not frontend code, so moving and needinfo'ing so this gets triaged despite being 2 years old.

Set this for P2.

Comment 25

[Anne (:annevk)]

https://bugs.chromium.org/p/chromium/issues/detail?id=1006345 has some discussion with Googlers on perhaps agreeing on a processing model. Nothing concrete thus far though.

Comment 28

[:Gijs (he/him)]

Attachment: Bug 1440677 - accept spaces and tabs in Content-Disposition header filename parameters because all the other browsers do, r?annevk!,valentin!

Comment 29

[:Gijs (he/him)]

So... I really don't know how to spec this. But as far as I can tell, this passes the mime header unit test so fallout should be OK, right? 😅

Less briefly, re comment #18, I looked at https://source.chromium.org/chromium/chromium/src/+/master:net/http/http_util.cc;l=1051;drc=aed22775d3d02301b9046b23a55dd84fef2b0caa and the associated code. As far as I can tell, chromium implements the header parsing by first stripping the inline/attachment value (which is the same thing we do) and then tokenizing on ; in a way that ignores the ones that are in quotes (and ignores escaped quotes when looking at quotes). It then splits the resulting tokens by = and then unquotes as necessary.

As a result, I think as long as we still split unquoted tokens by ;, this should be a more-compatible change.

I don't think I have the expertise or the cycles to develop this into a more-in-tune-with-reality version of the venerable RFCs here. But this continues to be one of the most-reported bugs in Firefox's file handling code, and because I believe the code post-patch is more cross-browser-compatible than pre-patch, and because chromium have already indicated they are unlikely to change behaviour to move closer to the original RFC, I figured it was worth putting up as-is.

Comment 30

[Anne (:annevk)]

I filed https://github.com/httpwg/http-extensions/issues/1252 for the specification side of things. (It might have to move to Fetch or HTML if they are not willing.)

Comment 31

[:Gijs (he/him)]

Intent to ship: https://groups.google.com/u/1/g/mozilla.dev.platform/c/-Z72qTjto9I

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1f68e042451
accept spaces and tabs in Content-Disposition header filename parameters because all the other browsers do, r=annevk,valentin,necko-reviewers

Comment 33

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d1f68e042451

Comment 34

[Tom S. (please needinfo tschuster)]

Mentioned on https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/81.