content-disposition: escaped characters in quoted-string give funny results

RESOLVED FIXED in mozilla6

Status

defect
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: julian.reschke, Assigned: julian.reschke)

Tracking

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

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

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

Given this C-D header field:

  Content-Disposition: attachment; filename="f\oo.html"

the expected filename would be

  foo.html

(in RFC 2616 quoted string, "\" is an escape character, and "\o" just stands for "o")

but the result is

  f_oo.html

(which maybe caused by something replacing "funny" characters *before* unescaping backslashes)

Reproducible: Always

Steps to Reproduce:
1. Run test at http://greenbytes.de/tech/tc2231/#attwithasciifnescapedchar

Actual Results:  
suggested file named is "f_oo.html"

Expected Results:  
suggested file should be "foo.html"
Component: General → File Handling
Product: Firefox → Core
QA Contact: general → file-handling
Attachment #473519 - Flags: review?(cbiesinger)
Attachment #473519 - Flags: review?(cbiesinger) → review?(bzbarsky)
Not sure if this should be File Handling or Networking...
Assignee: nobody → julian.reschke
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Comment on attachment 473519 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

I'd prefer ++c to c+=1, but more importantly the OOM check needs to come before you start messing with the pointer returned from ToNewCString.

r=me with that fixed.
Attachment #473519 - Flags: review?(bzbarsky) → review+
Attachment #473023 - Attachment is obsolete: true
Attachment #473519 - Attachment is obsolete: true
Attachment #481228 - Flags: review?(bzbarsky)
Comment on attachment 481228 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

r=me
Attachment #481228 - Flags: review?(bzbarsky) → review+
Attachment #481228 - Flags: approval2.0?
What's the risk and reward here? Offhand it doesn't look like this is worth taking now.
Blocks: 609667
Comment on attachment 481228 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

Clearing nomination, please renominate when my question is addressed.
Attachment #481228 - Flags: approval2.0?
(In reply to comment #7)
> What's the risk and reward here? Offhand it doesn't look like this is worth
> taking now.

It's like with most conformance bug fixes; a risk may be there, but it's hard to predict. The reward is that messages do not get misinterpreted when using escapes.

That being said; this has been broken for so long that it's definitively not urgent enough to get into FF4, agreed.
This patch doesn't apply cleanly on trunk any more.
Whiteboard: not-ready
Probably as it is in conflict with the change for 588781; will check.
this includes a stand-alone JS test file; maybe it should be integrated into Jason's test_MIME_params.js?
Attachment #481228 - Attachment is obsolete: true
(In reply to comment #7)
> What's the risk and reward here? Offhand it doesn't look like this is worth
> taking now.

Benjamin, this was pre-FF4. Is it ok to go ahead now it's out of the door?
Yes, this can land.
Keywords: checkin-needed
Whiteboard: not-ready
Ehsan says this has landed on cedar, on its way to mozilla-central.
This is about to get backed out from cedar, because it's failing tests.  In particular, I believe it's failing these tests in content/media/test/test_can_play_type_wave.html (the actual test calls are in content/media/test/can_play_type_wave.js):

15   check("audio/wave; codecs=", "probably");
16   check("audio/wave; codecs=\"\"", "probably");
Backed out from cedar.

Chris, could you take a look at this patch and the media test and see whether the problem is in the test or in the patch or in the media code?
Attachment #524066 - Flags: feedback?(chris)
Comment on attachment 524066 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

>diff --git a/netwerk/mime/nsMIMEHeaderParamImpl.cpp b/netwerk/mime/nsMIMEHeaderParamImpl.cpp
>--- a/netwerk/mime/nsMIMEHeaderParamImpl.cpp
>+++ b/netwerk/mime/nsMIMEHeaderParamImpl.cpp
>@@ -246,18 +268,24 @@ nsMIMEHeaderParamImpl::GetParameterInter
>     // If so, copy it and return.
>     if (tokenEnd - tokenStart == paramLen &&
>         !nsCRT::strncasecmp(tokenStart, aParamName, paramLen))
>     {
>       // if the parameter spans across multiple lines we have to strip out the
>       //     line continuation -- jht 4/29/98 
>       nsCAutoString tempStr(valueStart, valueEnd - valueStart);
>       tempStr.StripChars("\r\n");
>-      *aResult = ToNewCString(tempStr);
>-      NS_ENSURE_TRUE(*aResult, NS_ERROR_OUT_OF_MEMORY);

aResult is a char**

>+      char *res = ToNewCString(tempStr);

res is a char*

>+      NS_ENSURE_TRUE(*res, NS_ERROR_OUT_OF_MEMORY);

so this should be 

>+      NS_ENSURE_TRUE(res, NS_ERROR_OUT_OF_MEMORY);

Because if the string is "", as in the media test, *res==0, and so NS_ENSURE_TRUE returns NS_ERROR_OUT_OF_MEMORY in that case.
Attachment #524066 - Flags: feedback?(chris) → feedback-
Ah, good catch!  Sounds like we should add that to the necko tests here too.
Will add more tests and fix.
fixed the broken null check, added two related test cases
Attachment #524851 - Flags: review?(chris)
Comment on attachment 524851 [details] [diff] [review]
do backslash unescaping when the parameter uses quoted-string syntax

r+ for the NS_ENSURE_TRUE fix, that makes the test pass for me locally.
Attachment #524851 - Flags: review?(chris) → review+
Landed:  http://hg.mozilla.org/mozilla-central/rev/5375d6280c92

I replaced "c += 1" with "++c" (per comment 4) in this line...
> for (char *c = src; *c; c += 1)
...before landing (with bz's approval over IRC)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Version: unspecified → Trunk
Given that this header isn't documented anywhere yet to speak of, I've added a note about this change to Firefox 6 for developers:

https://developer.mozilla.org/en/Firefox_6_for_developers#HTTP
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?).
(In reply to comment #25)
> I just noticed that this caused an unplanned change in behavior for certain
> malformed headers, such as when there are multiple "filename" parameters.
> ...

Wrong bug; see https://bugzilla.mozilla.org/show_bug.cgi?id=588781#c27 instead.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.