RFC2231/5987 decoding should not tolerate missing single quotes

REOPENED
Assigned to

Status

()

REOPENED
8 years ago
3 years ago

People

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

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
See 

  http://greenbytes.de/tech/tc2231/#attwithfn2231singleqmissing

only FF is tolerant here. We should ignore the param instead.
(Assignee)

Updated

8 years ago
Blocks: 609667
(Assignee)

Updated

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

Comment 1

8 years ago
depends on patch for 686429
Attachment #565797 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Target Milestone: --- → mozilla10
This code seems to be pretty purposeful.  Have you checked the history to see when/why it was added?

Also why is the aCharset null-check ok to remove?
(Assignee)

Comment 3

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #2)
> This code seems to be pretty purposeful.  Have you checked the history to
> see when/why it was added?

Apparently it predates 2003/2004, when the code was moved over from the mail component. I can't find anything specific to this in Bugzilla.

> Also why is the aCharset null-check ok to remove?

It's not (unfortunately this is something we can't test from JS, right?). Will update the patch. (And thanks for checking!)
(Assignee)

Comment 4

8 years ago
(In reply to Julian Reschke from comment #3)
> (In reply to Boris Zbarsky (:bz) from comment #2)
> > This code seems to be pretty purposeful.  Have you checked the history to
> > see when/why it was added?
> 
> Apparently it predates 2003/2004, when the code was moved over from the mail
> component. I can't find anything specific to this in Bugzilla.
> ...

As far as I can tell, this has been around in the initial checkin for bug 162765. I *assume* it was done in the spirit of being lenient, but as far as I can tell, it doesn't need to, and being lenient in only one implementation actually is bad for interop.
(Assignee)

Comment 5

8 years ago
(restores the missing aCharset check)
Attachment #565797 - Attachment is obsolete: true
Attachment #565797 - Flags: review?(bzbarsky)
Attachment #566546 - Flags: review?(bzbarsky)
> As far as I can tell, this has been around in the initial checkin for bug 162765

Perfect, thanks.  The mailnews code that was removed in that checkin looked like this:

-  const char *s_quote1 = PL_strchr(value_start, 0x27);
-  const char *s_quote2 = (char *) (s_quote1 ? PL_strchr(s_quote1+1, 0x27) : NULL);
-  NS_ASSERTION(s_quote1 && s_quote2, "1.1 <rhp@netscape.com> 19 Mar 1999 12:00");
-  if (charset && s_quote1 > value_start && s_quote1 < value_end)
-    /* set *charset */
....
-  if (language && s_quote1 && sa_quote2 && s_quote2 > s_quote1+1 &&
-    s_quote2 < value_end)
-    /* set *language */
...
-  if (s_quote2 && s_quote2+1 < value_end)
-    /* set s, etc */

So it looks like it set *charset no matter what, but asserted if there wasn't a second quote and pressed on.  <sigh>.

Let's hope that mail doesn't depend on this behavior...

> unfortunately this is something we can't test from JS, right?

That's correct.
Comment on attachment 566546 [details] [diff] [review]
Proposed patch, incl test case

r=me
Attachment #566546 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 9

8 years ago
https://hg.mozilla.org/mozilla-central/rev/4b87afed1d1c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
(Assignee)

Comment 10

7 years ago
Reopened because the change was backed out for now due to bug 704989.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

3 years ago
Component: File Handling → File Handling
Product: Core → Firefox
Target Milestone: mozilla10 → ---
You need to log in before you can comment on or make changes to this bug.