Last Comment Bug 588781 - Content-Disposition: suboptimal fallback behavior when both "filename" and "filename*" params are present
: Content-Disposition: suboptimal fallback behavior when both "filename" and "f...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
-- normal with 1 vote (vote)
: mozilla5
Assigned To: Jason Duell [:jduell] (needinfo me)
:
: Patrick McManus [:mcmanus]
Mentors:
http://greenbytes.de/tech/tc2231/#att...
Depends on:
Blocks: 609667 610054
  Show dependency treegraph
 
Reported: 2010-08-19 08:20 PDT by Julian Reschke
Modified: 2011-04-25 23:38 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (719 bytes, text/javascript)
2010-09-07 06:05 PDT, Julian Reschke
no flags Details
proposed patch (1.20 KB, patch)
2010-09-07 07:15 PDT, Julian Reschke
cbiesinger: review-
Details | Diff | Splinter Review
priorize "param*" (RFC 2231/5987) over "param" (3.59 KB, patch)
2010-09-07 09:01 PDT, Julian Reschke
cbiesinger: review+
benjamin: approval2.0-
Details | Diff | Splinter Review
restore "else if": change test file name, convert from dos->unix; add test cases (6.17 KB, patch)
2010-11-05 15:52 PDT, Jason Duell [:jduell] (needinfo me)
jduell.mcbugs: review+
jduell.mcbugs: superreview+
Details | Diff | Splinter Review

Description User image Julian Reschke 2010-08-19 08:20:18 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)
Build Identifier: 3.6.8

In order to deploy RFC2231/5987 encoding, there may be cases where servers want to send both. The format allows this, and optimally UAs would ignore parameters they do not understand, and prefer "filename*" over "filename" when both are present and understood.

Unfortunately, FF takes the first one it finds, not the "better" one.

Reproducible: Always

Steps to Reproduce:
1. See http://greenbytes.de/tech/tc2231/#attfnboth
2. Run the test http://greenbytes.de/tech/tc2231/attfnboth.asis

Actual Results:  
Suggests "foo-ae.html" as filename.

Expected Results:  
Should suggest "foo-รค.html" as filename.

Of the UAs that support RFC 2231/5987 (FF/Opera/Konqueror), currently only Konqueror gets this right.
Comment 1 User image Julian Reschke 2010-09-07 06:05:44 PDT
Created attachment 472610 [details]
test case
Comment 2 User image Julian Reschke 2010-09-07 07:15:17 PDT
Created attachment 472622 [details] [diff] [review]
proposed patch

This patch makes the InternalGetParameter continue after finding a matching parameter, looking for an instance of a RFC 2231 encoded parameter.
Comment 3 User image Christian :Biesinger (don't email me, ping me on IRC) 2010-09-07 07:47:58 PDT
Comment on attachment 472622 [details] [diff] [review]
proposed patch

Looks pretty good, but please fix the two issues below, and add the test file to the patch.

+++ b/netwerk/mime/nsMIMEHeaderParamImpl.cpp	Tue Sep 07 15:09:22 2010 +0100
       *aResult = ToNewCString(tempStr);
-      NS_ENSURE_TRUE(*aResult, NS_ERROR_OUT_OF_MEMORY);

You should keep that check - on OOM we do want to return failure.

-          NS_ASSERTION(!*aResult, "This is the 1st line. result buffer should be null.");
+          if (!*aResult)
+          {
+            // drop non-2231-encoded value
+            nsMemory::Free(*aResult);

This doesn't look right - don't you want the if to be if (*aResult)?


(at some point this entire file should be changed to use NS_Alloc, NS_Free etc instead of nsMemory::... but not in this patch)
Comment 4 User image Julian Reschke 2010-09-07 09:01:50 PDT
Created attachment 472645 [details] [diff] [review]
priorize "param*" (RFC 2231/5987) over "param"

This patch makes the InternalGetParameter continue after finding a matching
parameter, looking for an instance of a RFC 2231/5987 encoded parameter.
Comment 5 User image Julian Reschke 2010-10-05 06:41:01 PDT
(In reply to comment #0)
> Of the UAs that support RFC 2231/5987 (FF/Opera/Konqueror), currently only
> Konqueror gets this right.

This has been fixed with the RC of Opera 10.63, so now FF is the only UA to get this wrong.
Comment 6 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2010-10-12 21:39:08 PDT
biesi, can you comment with a risk evaluation? Should we take it for 2.0 at this stage? The fix is straightforward enough, but I'm not sure exactly what its potential compatibility effects are (e.g. what its potential is for causing regressions due to different parsing of other parameters).
Comment 7 User image Julian Reschke 2010-10-13 00:28:07 PDT
(In reply to comment #5)
> This has been fixed with the RC of Opera 10.63, so now FF is the only UA to get
> this wrong.

...and Opera 10.63 is now officially shipping.
Comment 8 User image Julian Reschke 2010-10-15 00:07:56 PDT
(In reply to comment #6)
> biesi, can you comment with a risk evaluation? Should we take it for 2.0 at
> this stage? The fix is straightforward enough, but I'm not sure exactly what
> its potential compatibility effects are (e.g. what its potential is for causing
> regressions due to different parsing of other parameters).

Clarifying: the patch doesn't really affect parsing, but the interpretation of the parsing results. Unless there's something wrong with it, it will only change the behavior for header field instances that contain *both* the standard form and the RFC2231/5987 form of a parameter.

The only reason I can think of why this would occur in practice is in order to provide a fallback for recipients that do not understand the 2231/5987 encoding, in which case the proposed change will do exactly what the sender intended.
Comment 9 User image Jason Duell [:jduell] (needinfo me) 2010-10-15 19:33:21 PDT
cc-ing bz in case he has an opinion on this.  I agree that logically speaking, we should be fine if we always prefer "paramname*" over an initial "paramname".  Of course that's no guarantee that it couldn't actually cause problems.

I just ran this patch through tryserver (hmm, doesn't seem to be any way to link to a specific tbpl entry.  meh).  There's lots of orange, much of it clearly random, but some of I'm not clear about (doesn't seem obviously related to this patch, but tpbl isn't finding any bugzilla bugs for them).  For example:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287155695.1287156258.23748.gz
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-15 20:46:58 PDT
Generally speaking, taking this for 2.0 seems ok to me...  but I can also see us just holding off until after branch at this point.  We need to get the actual blocker list down.  :(
Comment 11 User image Julian Reschke 2010-10-16 00:40:55 PDT
This problem blocks deployment of "filename*" with "filename" as fallback. As long as it's not fixed, senders have no choice but doing UA sniffing, ot to wait until IE/Chrome/Safari are fixed (which need the fallback).
Comment 12 User image Jason Duell [:jduell] (needinfo me) 2010-10-19 10:48:44 PDT
JST: how about we mark approved for 2.0, try to land this, and if there's nontrivial problems we mark post 2.0?
Comment 13 User image Benjamin Smedberg [:bsmedberg] 2010-10-29 13:50:18 PDT
This seems risky enough that I don't want to take it for FF4 now.
Comment 14 User image Julian Reschke 2010-10-30 09:03:48 PDT
(In reply to comment #13)
> This seems risky enough that I don't want to take it for FF4 now.

Trying to understand where you see the risk...

The patch is expected to change the behavior if and only if a header field contains both "foo" and "foo*", in which case it will pick the latter. Do you think *that* change may have undesired consequences?

Or do you fear that there are more behavioral changes? That would indeed be a bug, and I would appreciate some hints about what to look for.
Comment 15 User image Jason Duell [:jduell] (needinfo me) 2010-11-04 12:06:11 PDT
Comment on attachment 472645 [details] [diff] [review]
priorize "param*" (RFC 2231/5987) over "param"

> Trying to understand where you see the risk...

It's nothing specific, just that we're late in the release cycle, and have to make hard choices about not landing new stuff.  I'd personally like to see this in 2.0/FF 4, but ultimately it's up the drivers.


>diff -r e14130b81b5f netwerk/mime/nsMIMEHeaderParamImpl.cpp

>     // case B, C, and D
>-    else if (tokenEnd - tokenStart > paramLen &&
>+    if (tokenEnd - tokenStart > paramLen &&

I think you should keep the 'else if' here.  The enclosing while loop will already ensure you'll check for the next parameter.  Changing this to 'if' just means we'll do a useless check for cases B,C,D when we already know that the param under consideration is type A.
Comment 16 User image Jason Duell [:jduell] (needinfo me) 2010-11-04 12:13:03 PDT
Also please get rid of comment above function:

 // The format of these header lines  is
 // <token> [ ';' <token> '=' <token-or-quoted-string> ]*          

which is inaccurate. RFC 2231 comment later on suffices.
Comment 17 User image Julian Reschke 2010-11-04 15:10:55 PDT
(In reply to comment #16)
> Also please get rid of comment above function:
> 
>  // The format of these header lines  is
>  // <token> [ ';' <token> '=' <token-or-quoted-string> ]*          
> 
> which is inaccurate. RFC 2231 comment later on suffices.

Can't see what's not accurate here...
Comment 18 User image Jason Duell [:jduell] (needinfo me) 2010-11-05 15:52:42 PDT
Created attachment 488588 [details] [diff] [review]
restore "else if": change test file name, convert from dos->unix; add test cases

Fixed up patch: moving +r/+sr forward.

Followup bug and patch in bug 610054.

Julian/mnot:  I'd appreciate it if either of you could look over the test cases her and in bug 610054 to see if they're all 2231/5987 compliant.
Comment 19 User image mnot 2011-02-23 02:46:19 PST
Julian is the master of the test cases.

I'd urge Moz to consider getting this into FF4, because this bug is the one significant barrier to a good interop / i18n story for Content-Disposition.
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2011-02-23 05:09:32 PST
This is NOT going into Fx4.  See comment 13.
Comment 21 User image Christopher Blizzard (:blizzard) 2011-02-24 16:10:42 PST
I'll drive this for our next release, which will be out 3 months after Firefox 4.
Comment 22 User image Jason Duell [:jduell] (needinfo me) 2011-02-24 21:04:34 PST
Yes, this should just land when the tree opens post-FF4.  I doubt it's going to cause any trouble.
Comment 25 User image Julian Reschke 2011-03-25 10:31:08 PDT
Tested <http://greenbytes.de/tech/tc2231/#attfnboth> with nightly build of 2011-03-25 and works fine. Thanks!
Comment 26 User image Eric Shepherd [:sheppy] 2011-04-16 16:43:39 PDT
We have a mention of this on Firefox 5 for developers now; there is no documentation for content-disposition, which is a separate issue entirely; see bug 650562.
Comment 27 User image Julian Reschke 2011-04-17 00:35:22 PDT
I just noticed that this caused an unplanned change in behavior for certain
malformed headers, such as when there are multiple "filename" parameters.

See

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

(FF4 picks the first one, FF5aurora picks the second one)

I do not believe this should cause problems in practice, as this is a test case
for an edge case, not a header seen in practice. My suggestion is to leave the
fix in, but to be prepared to tune it (should this be raised as separate
issue?).
Comment 28 User image Jason Duell [:jduell] (needinfo me) 2011-04-25 23:38:00 PDT
Yes, open a new bug if there's a refinement to be made to a patch that's already landed and RESOLVED FIXED.

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