685192 - in RFC2231/5987 encoding, a missing charset field should be treated as error

Status: NEW

(Core :: Networking, defect, P5)

Description

[Julian Reschke]

See 

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

which tests

   Content-Disposition: attachment; filename*=''foo-%c3%a4-%e2%82%ac.html

Here, the charset field is missing; apparently FF falls back to UTF-8. Opera falls back to something else. IE9, Chrome, and Konqueror ignore the parameter.

I think it would be good to align with IE, Chrome, and Konqueror.

Comment 1

[Jason Duell]

once bug 589292 lands, this should be a matter of fixing NS_GetFilenameFromDisposition() to not pass the channel's fallback charset, nor pass true for aTryLocaleCharset, and perhaps removing the call to IsUTF8() from nsMIMEHeaderParamImpl::GetParameter().  To do the latter we'd need to make sure we don't break mail header parsing.

Comment 2

[Julian Reschke]

This might be one of the cases where we need to distinguish MIME and HTTP.

Comment 3

[Julian Reschke]

(In reply to Jason Duell (:jduell) from comment #1)
> once bug 589292 lands, this should be a matter of fixing
> NS_GetFilenameFromDisposition() to not pass the channel's fallback charset,
> nor pass true for aTryLocaleCharset, and perhaps removing the call to
> IsUTF8() from nsMIMEHeaderParamImpl::GetParameter().  To do the latter we'd
> need to make sure we don't break mail header parsing.

Maybe. We can also change the 2231/5987 parsing code to just skip values where the charset is missing; that should be less intrusive.

On the other hand, it would be cool if we heuristics like aTryLocaleCharset -- maybe we can do that as part of a separate bug? I also have no idea how this will affect mail as opposed to http...

Comment 4

[Julian Reschke]

Attachment: Proposed patch, incl test case

Applies on top of patch for bug 692574

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 6

[Julian Reschke]

Attachment: Proposed patch, incl test case

Updated to that it applies on top of bug 692574

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 8

[Ed Morley [:emorley]]

Missing patch author added locally, now in my queue with a few other bits that are being sent to try first and then onto inbound :-)

https://tbpl.mozilla.org/?tree=Try&rev=488d4a5f274a

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa04ab6995e5

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/fa04ab6995e5

Comment 11

[Julian Reschke]

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

Comment 12

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 13

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

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