The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

Core Graveyard
File Handling
RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: Julian Reschke, Assigned: Julian Reschke)

Tracking

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

Trunk
mozilla6
dev-doc-complete

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

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

Comment 1

7 years ago
Created attachment 473023 [details]
test case for unescaping quoted-string
(Assignee)

Comment 2

7 years ago
Created attachment 473519 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax
(Assignee)

Updated

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

Comment 5

7 years ago
Created attachment 481228 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax
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?
(Assignee)

Comment 9

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

Comment 11

6 years ago
Probably as it is in conflict with the change for 588781; will check.
(Assignee)

Comment 12

6 years ago
Created attachment 524066 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

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
(Assignee)

Comment 13

6 years ago
(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)
Keywords: checkin-needed
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.
(Assignee)

Comment 20

6 years ago
Will add more tests and fix.
(Assignee)

Comment 21

6 years ago
Created attachment 524851 [details] [diff] [review]
do backslash unescaping when the parameter uses quoted-string syntax

fixed the broken null check, added two related test cases
Keywords: dev-doc-needed
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 25

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 26

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