Closed Bug 1440677 Opened 4 years ago Closed 11 months ago

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

Categories

(Core :: Networking, defect, P2)

58 Branch
defect

Tracking

()

RESOLVED FIXED
81 Branch
Webcompat Priority P2
Tracking Status
firefox81 --- fixed

People

(Reporter: hartmantam.enc, Assigned: Gijs)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-triaged])

Attachments

(1 file)

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`
This error occur when the filename parameter contains space. Other browser such as Chrome, Edge, Vivaldi doesn't have this parsing problem.
NI :bz because bug 221028 is now in Core Graveyard, so it doesn't seem right to dupe this to it.
Has Regression Range: --- → irrelevant
Has STR: --- → yes
Component: Untriaged → File Handling
Flags: needinfo?(bzbarsky)
OS: Unspecified → All
Hardware: Unspecified → All
Summary: content-disposition parsing error → herokuapp.com - Problems with spaces in filenames (content-disposition not quoted)
It's still the right place; I have no idea why someone stuck it in "graveyard".
Flags: needinfo?(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.
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.
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.
(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.
Anne, has this come up in the fetch/etc work?
Flags: needinfo?(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?
Flags: needinfo?(annevk) → needinfo?(julian.reschke)
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...
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.
Flags: needinfo?(julian.reschke)
So how will Firefox dev team handle this? Fix it or following the standard?
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.
Then obviously Firefox behavior of handling spaces in Content-Disposition is not equal to other majority of browsers such as Chrome.
I have no idea what comment 14 is trying to say, for what that's worth.
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.
In addition to comment 16, should the dev team make Firefox behave like other browsers?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Duplicate of this bug: 1476758
Duplicate of this bug: 1568707

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

Component: File Handling → Networking
Flags: needinfo?(kershaw)
Priority: P3 → --
Product: Firefox → Core

(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.

Flags: needinfo?(kershaw)
Priority: -- → P2
Whiteboard: [necko-triaged]
Duplicate of this bug: 1580520
Duplicate of this bug: 1334274

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.

Duplicate of this bug: 1629406
Duplicate of this bug: 1656509
Webcompat Priority: --- → P2
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

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.

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.)

Blocks: 609667
Attachment #9170115 - Attachment description: Bug 1440677 - accept spaces in Content-Disposition header filename parameters because all the other browsers do, r?annevk!,valentin! → Bug 1440677 - accept spaces and tabs in Content-Disposition header filename parameters because all the other browsers do, r?annevk!,valentin!
Pushed by gijskruitbosch@gmail.com:
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
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Regressions: 1586890
See Also: → 1710579
You need to log in before you can comment on or make changes to this bug.