692574 - RFC2231/5987 decoding should not tolerate missing single quotes

Status: REOPENED

(Firefox :: File Handling, defect)

Description

[Julian Reschke]

See 

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

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

Comment 1

[Julian Reschke]

Attachment: Proposed patch, incl test case

depends on patch for 686429

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Julian Reschke]

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

Comment 4

[Julian Reschke]

(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.

Comment 5

[Julian Reschke]

Attachment: Proposed patch, incl test case

(restores the missing aCharset check)

Comment 6

[Boris Zbarsky [: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 <[e-mail]> 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 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 566546 [details] [diff] [review]
Proposed patch, incl test case

r=me

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4b87afed1d1c

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4b87afed1d1c

Comment 10

[Julian Reschke]

Reopened because the change was backed out for now due to bug 704989.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.