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

REOPENED
Assigned to

Status

()

P5
minor
REOPENED
8 years ago
a year ago

People

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

Tracking

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

unspecified
mozilla10
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-would-take], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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

8 years ago
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
(Assignee)

Comment 2

8 years ago
This might be one of the cases where we need to distinguish MIME and HTTP.
(Assignee)

Updated

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

Comment 3

8 years ago
(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...
(Assignee)

Comment 4

7 years ago
Created attachment 566315 [details] [diff] [review]
Proposed patch, incl test case

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+
(Assignee)

Comment 6

7 years ago
Created attachment 566581 [details] [diff] [review]
Proposed patch, incl test case

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+
(Assignee)

Updated

7 years ago
Target Milestone: --- → mozilla10
(Assignee)

Updated

7 years ago
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

Comment 10

7 years ago
https://hg.mozilla.org/mozilla-central/rev/fa04ab6995e5
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
(Assignee)

Comment 11

7 years ago
Reopened because the change was backed out for now due to bug 704989.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [necko-would-take]
You need to log in before you can comment on or make changes to this bug.