Open Bug 685192 Opened 9 years ago Updated 3 years ago

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

Categories

(Core :: Networking, defect, P5)

defect

Tracking

()

REOPENED
mozilla10

People

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

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed, Whiteboard: [necko-would-take])

Attachments

(1 file, 1 obsolete file)

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.
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.
Depends on: 589292
This might be one of the cases where we need to distinguish MIME and HTTP.
Assignee: nobody → 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...
Attached patch Proposed patch, incl test case (obsolete) — Splinter Review
Applies on top of patch for bug 692574
Attachment #566315 - Flags: review?(bzbarsky)
Comment on attachment 566315 [details] [diff] [review]
Proposed patch, incl test case

r=me
Attachment #566315 - Flags: review?(bzbarsky) → review+
Updated to that it applies on top of bug 692574
Attachment #566581 - Flags: review?(bzbarsky)
Comment on attachment 566581 [details] [diff] [review]
Proposed patch, incl test case

r=me
Attachment #566581 - Flags: review?(bzbarsky) → review+
Target Milestone: --- → mozilla10
Keywords: checkin-needed
Attachment #566315 - Attachment is obsolete: true
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
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fa04ab6995e5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reopened because the change was backed out for now due to bug 704989.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.