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)
:
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 | 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 | 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 | Review

Description 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 Julian Reschke 2010-09-07 06:05:44 PDT
Created attachment 472610 [details]
test case
Comment 2 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 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 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 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 :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 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 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 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 Boris Zbarsky [:bz] (Out June 25-July 6) 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 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 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 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 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 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 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 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 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 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 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-23 05:09:32 PST
This is NOT going into Fx4.  See comment 13.
Comment 21 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 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 23 :Ehsan Akhgari (out sick) 2011-03-23 20:45:45 PDT
http://hg.mozilla.org/projects/cedar/rev/67856ed940f8
Comment 24 :Ehsan Akhgari (out sick) 2011-03-24 12:41:42 PDT
http://hg.mozilla.org/mozilla-central/rev/67856ed940f8
Comment 25 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 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 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 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.