Use filename in Content-Disposition header when present #1056

Merged
merged 1 commit into from Apr 14, 2013

Conversation

Projects
None yet
7 participants
Contributor

awmichel commented Apr 9, 2013

Carrierwave should respect the filename set in the Content-Disposition
header when downloading remote files. This fixes issue #1047 as well.

Included test specs for checking the content-disposition filename against the extension white list or extension black list.

This also solves the case of using a service like Filepicker.io to handle uploads and then passing them off to Carrierwave to process.

Use filename in Content-Disposition header when present.
Carrierwave should respect the filename set in the Content-Disposition
header when downloading remote files. This fixes issue #1047 as well.

This looks good to me. The patch is simple and the specs look good. @bensie, can you take a look?

Owner

thiagofm commented Apr 14, 2013

This indeed looks very good and is a better solution than the one proposed by @superp, @bensie thoughts?

superp commented Apr 14, 2013

In my case meta tag "Content-Disposition" isn't present at response.

Contributor

awmichel commented Apr 14, 2013

@superp How is the file saved if you make the request from a browser? Is it given a custom filename or is the file saved as the request URL? Could you share any more details about the source you are using?

superp commented Apr 14, 2013

I have image by url "http://avt.appsmail.ru/mail/developer_bdu/_avatarbig".
Saved it by remote_url.
I write pull request for this moment:
jnicklas#1061

@@ -17,6 +17,10 @@ def initialize(uri)
end
def original_filename
+ if file.meta.include? 'content-disposition'
@bensie

bensie Apr 14, 2013

Owner

Do we need to worry about case sensitivity here? Want to make sure this doesn't fall apart if the header is "Content-Disposition". I personally haven't used this meta hash so I'm not sure how it's built.

@awmichel

awmichel Apr 14, 2013

Contributor

From the testing I've done, the keys in the meta hash are already lowercased. The test spec uses "Content-Disposition" as this seems to be the most common casing and still passes.

bensie added a commit that referenced this pull request Apr 14, 2013

Merge pull request #1056 from awmichel/fix-content-disposition-remote…
…-file

Use filename in Content-Disposition header when present

@bensie bensie merged commit 8a2e430 into carrierwaveuploader:master Apr 14, 2013

1 check passed

default The Travis build passed
Details

@awmichel awmichel deleted the awmichel:fix-content-disposition-remote-file branch Apr 14, 2013

mrichie commented Jun 2, 2013

@bensie
I have a remote image url
http://mmsns.qpic.cn/mmsns/iar5eumkt82Sg8XNV8KtPtt99iaxuP57Rib0outFFjsQ4Cq5EwrdmCnOg/0
without content-disposition, and it cannot be saved by this solution

synth commented Oct 1, 2013

I believe I'm hitting the same issue. I'm trying to grab avatar when user oauth's with google, and the link to the avatar has the following content-disposition header

Content-Disposition:inline;filename=""

avatar in question(me): http://lh5.googleusercontent.com/-qVbp8V1Qr2Q/AAAAAAAAAAI/AAAAAAAAHig/RhObI102-3Q/photo.jpg

UPDATE: fixed by adding '' empty string to extension white list, ugh :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment