Last Comment Bug 776339 - remove support of Content-Disposition "name" parameter
: remove support of Content-Disposition "name" parameter
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core Graveyard
Classification: Graveyard
Component: File Handling (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: Julian Reschke
:
Mentors:
http://greenbytes.de/tech/tc2231/#att...
Depends on:
Blocks: 609667 229785
  Show dependency treegraph
 
Reported: 2012-07-22 03:05 PDT by Julian Reschke
Modified: 2016-06-22 12:16 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (1.38 KB, patch)
2012-10-09 07:04 PDT, Julian Reschke
jduell.mcbugs: review+
cbiesinger: superreview+
Details | Diff | Splinter Review

Description Julian Reschke 2012-07-22 03:05:52 PDT
See 

  http://greenbytes.de/tech/tc2231/#attwithnamepct

It is

(a) not specified,

(b) duplicates what "filename" does, and

(c) not implemented in UAs other than Firefox and Chrome.
Comment 1 Julian Reschke 2012-10-09 07:04:35 PDT
Created attachment 669515 [details] [diff] [review]
Proposed patch

try results at https://tbpl.mozilla.org/?tree=Try&rev=48da8bd2804e
Comment 2 Jason Duell [:jduell] (needinfo me) 2012-10-09 11:40:08 PDT
Comment on attachment 669515 [details] [diff] [review]
Proposed patch

Review of attachment 669515 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, so this patch does 2 things:

1) Removes support for "name=" and "file=" being equivalent to "filename=" when getting contentDispositionFilename

2) If "file=" or "name=" are present w/o any 'attachment;" we used to special-case this as inferring "inline".  We wouldn't do that anymore.

Apparently only FF and Chrome are doing this, so unless sites are basing the C-D header on UserAgent there shouldn't be harm in removing these. But it's the Internet, so some sites might well be doing that.  I'm torn between just making the fix or requiring us to do some telemetry to see if we're actually seeing these headers in practice.  After being bit by lots of seemingly safe/trivial fixes in the past, I'm leaning towards the telemetry approach.

bz/biesi: you added this "feature": how do you feel about removing it?
Comment 3 Julian Reschke 2012-10-10 02:41:25 PDT
(In reply to Jason Duell (:jduell) from comment #2)
> Comment on attachment 669515 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 669515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, so this patch does 2 things:
> 
> 1) Removes support for "name=" and "file=" being equivalent to "filename="
> when getting contentDispositionFilename

Just "name=". 

> 2) If "file=" or "name=" are present w/o any 'attachment;" we used to
> special-case this as inferring "inline".  We wouldn't do that anymore.

I don't think that's the case, but I will check again. I agree that we shouldn't change this in *this* context (there's a separate bug about that).

> Apparently only FF and Chrome are doing this, so unless sites are basing the
> C-D header on UserAgent there shouldn't be harm in removing these. But it's
> the Internet, so some sites might well be doing that.  I'm torn between just
> making the fix or requiring us to do some telemetry to see if we're actually
> seeing these headers in practice.  After being bit by lots of seemingly
> safe/trivial fixes in the past, I'm leaning towards the telemetry approach.
> 
> bz/biesi: you added this "feature": how do you feel about removing it?

Yes, it might break sites that sniff. I'd be very surprised though if somebody went through the trouble to sniff for a UA in order to use an undocumented feature instead of the standard one.

Will the telemetry tell us *where* this happens so we get a remote chance to get those sites fixed?
Comment 4 Julian Reschke 2012-10-10 05:58:15 PDT
(In reply to Julian Reschke from comment #3)
> > 2) If "file=" or "name=" are present w/o any 'attachment;" we used to
> > special-case this as inferring "inline".  We wouldn't do that anymore.
> 
> I don't think that's the case, but I will check again. I agree that we
> shouldn't change this in *this* context (there's a separate bug about that).

Checked with <http://greenbytes.de/tech/tc2231/#attmissingdisposition>, and as far as I can tell, the behavior is as before.
Comment 5 Jason Duell [:jduell] (needinfo me) 2012-10-10 15:19:38 PDT
> Just "name=". 

Right you are.

> 2) If "name=" is present w/o any 'attachment;" we used to
> special-case this as inferring "inline".  We wouldn't do that anymore.

try

  Content-Disposition: name=foo.html

and I'm guessing you'll see it treated as attachment instead of inline with this patch.  That said I don't know how much we need to worry about this if no other UAs support this format any more (that case is not in your testsuite AFAICT)

> Will the telemetry tell us *where* this happens so we get a remote chance to get those sites fixed?

Nope.  Which is another reason, if all other UAs have scrapped this, why I think we probably can too w/o telemetry investigation.
Comment 6 Julian Reschke 2012-10-11 02:29:53 PDT
(In reply to Jason Duell (:jduell) from comment #5)
> > 2) If "name=" is present w/o any 'attachment;" we used to
> > special-case this as inferring "inline".  We wouldn't do that anymore.
> 
> try
> 
>   Content-Disposition: name=foo.html
> 
> and I'm guessing you'll see it treated as attachment instead of inline with
> this patch.  That said I don't know how much we need to worry about this if
> no other UAs support this format any more (that case is not in your
> testsuite AFAICT)

You are right, tried it with http://greenbytes.de/tech/tc2231/776339.asis (and I'd prefer not to fill my test suite with edge cases like these :-).

> > Will the telemetry tell us *where* this happens so we get a remote chance to get those sites fixed?
> 
> Nope.  Which is another reason, if all other UAs have scrapped this, why I
> think we probably can too w/o telemetry investigation.

They haven't scapped this; they never did it. The only reason Chrome does it as well is because they borrowed from Firefox (not code but logic, I assume).
Comment 7 Jason Duell [:jduell] (needinfo me) 2012-10-11 12:00:10 PDT
Comment on attachment 669515 [details] [diff] [review]
Proposed patch

Review of attachment 669515 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this change.
Comment 8 Christian :Biesinger (don't email me, ping me on IRC) 2012-11-02 16:58:06 PDT
Comment on attachment 669515 [details] [diff] [review]
Proposed patch

Seems fine to me.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:28:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc2166686ba
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-11-04 03:04:37 PST
https://hg.mozilla.org/mozilla-central/rev/ccc2166686ba
Comment 11 AWAY Tom Schuster [:evilpie] 2013-03-22 14:07:49 PDT
Somebody wrote this down in https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_19.
Comment 12 Kohei Yoshino [:kohei] 2013-03-22 20:54:07 PDT
(In reply to Tom Schuster [:evilpie] from comment #11)
> Somebody wrote this down in
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_19.

Yes I did. Please correct and clarify the description if needed.

Note You need to log in before you can comment on or make changes to this bug.