The default bug view has changed. See this FAQ.

escape characters in content-type charset param not parsed properly

RESOLVED FIXED in mozilla11

Status

()

Core
Networking: HTTP
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Julian Reschke, Assigned: Julian Reschke)

Tracking

unspecified
mozilla11
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 

According to RFC 2616, Section 3.7, the following headers are equivalent:

  Content-Type: text/plain; charset="U\TF-8"
  Content-Type: text/plain; charset="UTF-8"

However, Mozilla seems no to un-escape the string value, and thus doesn't see the charset value of UTF-8.

Yes, this is an edge case. It's a bug nevertheless.
  

Reproducible: Always
(Assignee)

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

6 years ago
See test case at

  http://greenbytes.de/tech/tc/httpcontenttype/#textplainutf8qsescaped

(currently Konqueror and Safari passing).

See also

  http://trac.tools.ietf.org/wg/httpbis/trac/ticket/270
(Assignee)

Updated

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

Comment 2

6 years ago
The problem is caused by

  net_ParseContentType

returning the unescaped charset; however this is non-trivial to change because it just returns pointers into the raw header field, and that header field is const.

The right approach seems to be to change the method signature to return a new string, however this means touching multiple other places.
(Assignee)

Comment 3

6 years ago
(In reply to Julian Reschke from comment #2)
> The problem is caused by
> 
>   net_ParseContentType
> 
> returning the unescaped charset; however this is non-trivial to change
> because it just returns pointers into the raw header field, and that header
> field is const.
> ...

Turns out I was too pessimistic, please ignore above comment.
(Assignee)

Updated

6 years ago
Assignee: nobody → julian.reschke
(Assignee)

Comment 4

6 years ago
Created attachment 573312 [details] [diff] [review]
Proposed patch, incl test case

Applies on top of patch for bug 700589
Attachment #573312 - Flags: review?(jduell.mcbugs)
Comment on attachment 573312 [details] [diff] [review]
Proposed patch, incl test case

Review of attachment 573312 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsURLHelper.cpp
@@ +888,4 @@
>          } while (curParamStart < flatStr.Length());
>      }
>  
> +    bool charsetNeedsUnquote = false;

From my understanding of the code, this variable isn't actually controlling whether we need to find a matching " at the end of the string, but rather just whether we need to ignore any backslashes in the charset.  Correct?  If so, maybe rename to 'charsetRemoveBackslashes'?

@@ +926,4 @@
>  
>          if ((!eq && *aHadCharset) || typeHasCharset) {
>              *aHadCharset = true;
> +            if (charsetNeedsUnquote) {

Maybe add comment here:

   // RFC 2616, Section 3.7 says we must ignore XYZ

Actually, looking at section 3.7, I don't see how it says that we need to be ignoring the backslashes.  But that probably just means I haven't chased enough RFC definitions far enough (is the behavior implied by 'parameter'?  I only see it saying the match needs to be case-insensitive).
(Assignee)

Comment 6

5 years ago
Created attachment 577587 [details] [diff] [review]
Proposed patch, incl test case

Addresses review comments (made name of flag more precise, add comment why we're unescaping here)
Attachment #573312 - Attachment is obsolete: true
Attachment #577587 - Flags: review?(jduell.mcbugs)
Attachment #573312 - Flags: review?(jduell.mcbugs)
Comment on attachment 577587 [details] [diff] [review]
Proposed patch, incl test case

Review of attachment 577587 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Julian!
Attachment #577587 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 8

5 years ago
Try results: https://tbpl.mozilla.org/?tree=Try&rev=74b71078993b
http://hg.mozilla.org/integration/mozilla-inbound/rev/abe0e4c2c403
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/abe0e4c2c403
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.