776339 - remove support of Content-Disposition "name" parameter

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Julian Reschke]

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]

Attachment: Proposed patch

try results at https://tbpl.mozilla.org/?tree=Try&rev=48da8bd2804e

Comment 2

[Jason Duell]

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]

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

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

> 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]

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

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

Comment on attachment 669515 [details] [diff] [review]
Proposed patch

Seems fine to me.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc2166686ba

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 11

[Tom S [:evilpie]]

Somebody wrote this down in https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_19.

Comment 12

[Kohei Yoshino [:kohei]]

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