233798 - RFC 2231/2047 encoded filename parameter in content-disposition header should be decoded

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Jungshik Shin]

I found that camino/src/download/SaveHeaderSniffer.mm uses a simple string
'index/search' APIs to obtain the value of 'filename' parameter in
Content-Disposition header. This wouldn't work well for RFC 2231/RFC 2047
encoded value (used in filename parameter). In bug 162765, I implemented
nsIMIMEHeaderParam to use to decode RFC 2047/2231-encoded words in situations
like this.

Comment 1

[Jungshik Shin]

Attachment: patch

This should work, but I can't test it because I don't have a Mac.

Comment 2

[Josh Aas]

You have to include "nsIMIMEHeaderParam.h" in order for the patch to compile. I
have two questions about this patch:

1. if the mURL test fails, then fallbackCharset will be uninitialized when it
gets passed to GetParameter (mimehdrpar->GetParameter(...)). Is that OK?

2. What the hell does this patch actually do? I have no idea how to test it. :)

Comment 3

[Jungshik Shin]

Thanks for trying it.

1. It's all right to leave fallbackCharset empty. 

2. What it does is:
 
   a. retrieve filename parameter in Content-Disposition header
   b. if it's RFC 2231-encoded, decode it and convert to UTF-16 and return
   c. if it's RFC 2047-encoded, decode it and convert to UTF-16 and return
   d. if it's UTF-8, convert it to UTF-16 and return
   e. if fallbackCharset is not empty, try to interpret it as if it's in that   
      character encoding and convert to UTF-16 and return

Over a dozen test cases are at the URL given in the URL field. See also bug
162765. Try to save files of type 'applicaton/x-random'. All but those marked
with 'raw 8bit EUC-JP' should bring up the download box with Korean or Japanese
filenames like 한글문서1.yyy and  芸術3.yyy


[1] http://www.faqs.org/rfcs/rfc2231.html
    http://www.faqs.org/rfcs/rfc2047.html

Comment 4

[Jasper]

Could you post some testcases or url's for us to test this patch?

Comment 5

[Josh Aas]

I can't make the build with your patch do anything different than our current
build with those test cases. I like the patch in the sense that its clean and it
seems more robust than our current implementation, but I want to wait to do
review+ until I understand more about what it does. I'm inexperienced in this
area, and need more explanation.

Comment 6

[Josh Aas]

Never mind - I get it now. Your patch does work - it fixes the filename
displayed in the save window. r=josha (with the header included as well)

Comment 7

[Mike Pinkerton (not reading bugmail)]

landed.

Comment 8

[Jungshik Shin]

Attachment: embedding patch (more or less the same)

embedding/browser/cocoa/src/SaveHeaderSniffer.mm
has the same problem. It also uses 'AssignWithConversion' to convert UTF-16 to
UTF-8. 'AssignWithConversion' works only for characters below U+0100. It should
use CopyUTF16toUTF8, instead.

Comment 9

[Jungshik Shin]

Comment on attachment 141574 [details] [diff] [review]
embedding patch (more or less the same)

asking for r/sr.

Comment 10

[Jungshik Shin]

*** Bug 233772 has been marked as a duplicate of this bug. ***

Comment 11

[Jungshik Shin]

Comment on attachment 141574 [details] [diff] [review]
embedding patch (more or less the same)

somehow, sr request was sent to a wrong person.

Comment 12

[Mike Pinkerton (not reading bugmail)]

landed embedding patch ads well.

Comment 13

[Stuart Morgan]

Comment on attachment 141133 [details] [diff] [review]
patch

Clearing leftover sr?