Content-Disposition: suboptimal fallback behavior when both "filename" and "filename*" params are present

RESOLVED FIXED in mozilla5

Status

()

Core
Networking
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Julian Reschke, Assigned: jduell)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla5
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 472610 [details]
test case
(Reporter)

Comment 2

7 years ago
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.
(Reporter)

Updated

7 years ago
Attachment #472622 - Flags: review?(cbiesinger)
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)
Attachment #472622 - Flags: review?(cbiesinger) → review-

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

7 years ago
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.
Attachment #472610 - Attachment is obsolete: true
Attachment #472622 - Attachment is obsolete: true
Attachment #472645 - Flags: review?(cbiesinger)
Attachment #472645 - Flags: review?(cbiesinger) → review+
(Reporter)

Updated

7 years ago
Attachment #472645 - Flags: approval2.0?
(Reporter)

Updated

7 years ago
OS: Windows 7 → All
Hardware: x86 → All
(Reporter)

Comment 5

7 years ago
(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.
Assignee: nobody → julian.reschke
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).
(Reporter)

Comment 7

7 years ago
(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.
(Reporter)

Comment 8

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
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
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.  :(
(Reporter)

Comment 11

7 years ago
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).
(Assignee)

Comment 12

7 years ago
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

7 years ago
This seems risky enough that I don't want to take it for FF4 now.
Attachment #472645 - Flags: approval2.0? → approval2.0-
(Reporter)

Comment 14

7 years ago
(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.
(Assignee)

Updated

7 years ago
Blocks: 609667
(Assignee)

Comment 15

7 years ago
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.
(Assignee)

Comment 16

7 years ago
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.
(Reporter)

Comment 17

7 years ago
(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...
(Assignee)

Updated

7 years ago
Blocks: 610054
(Assignee)

Comment 18

7 years ago
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.
Attachment #472645 - Attachment is obsolete: true
Attachment #488588 - Flags: superreview+
Attachment #488588 - Flags: review+
(Assignee)

Updated

7 years ago
Assignee: julian.reschke → nobody
Component: File Handling → Networking
QA Contact: file-handling → networking

Comment 19

7 years ago
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.
This is NOT going into Fx4.  See comment 13.
I'll drive this for our next release, which will be out 3 months after Firefox 4.
(Assignee)

Comment 22

7 years ago
Yes, this should just land when the tree opens post-FF4.  I doubt it's going to cause any trouble.
http://hg.mozilla.org/projects/cedar/rev/67856ed940f8
Whiteboard: fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/67856ed940f8
Assignee: nobody → jduell.mcbugs
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
(Reporter)

Comment 25

7 years ago
Tested <http://greenbytes.de/tech/tc2231/#attfnboth> with nightly build of 2011-03-25 and works fine. Thanks!
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Comment 27

6 years ago
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?).
(Assignee)

Comment 28

6 years ago
Yes, open a new bug if there's a refinement to be made to a patch that's already landed and RESOLVED FIXED.
You need to log in before you can comment on or make changes to this bug.