qoobaa / s3

Library for accessing S3 objects and buckets, supports EU and US buckets
MIT License
258 stars 112 forks source link

Problems with resources that contain spaces #36

Closed moiristo closed 12 years ago

moiristo commented 13 years ago

I'm using your gem in combination with paperclip. Unfortunately, the temporary URL generation breaks when there are spaces in the requested file. The problem here is that the passed resource is not escaped and URI.escape is imo broken. My suggestion is to use Addressable::URI instead: https://github.com/sporkmonger/addressable.

My workaround for now can be found below. It uses Addressable::URI to escape the resource and it ensures the URL is not escaped again using URI.escape, as that breaks the resulting string again.

require "addressable/uri"

module Paperclip
  module Storage
    module S3
      def path *args
        Addressable::URI.escape(super)
      end
    end
  end
end

module S3
  class Object    
    def url
      "#{protocol}#{host}/#{path_prefix}#{key}"
    end
  end
end

Exception:

bad URI(is not URI?): /vergunningversneller-staging/files/28/1/Schermafbeelding 2010-12-22 om 14.55.45.png
 /opt/ruby-enterprise-1.8.7-2010.02/lib/ruby/1.8/uri/common.rb:436:in `split'
 /opt/ruby-enterprise-1.8.7-2010.02/lib/ruby/1.8/uri/common.rb:485:in `parse'
 s3 (0.3.7) lib/s3/signature.rb:217:in `canonicalized_resource'
 s3 (0.3.7) lib/s3/signature.rb:105:in `canonicalized_signature'
 s3 (0.3.7) lib/s3/signature.rb:60:in `generate_temporary_url_signature'
 s3 (0.3.7) lib/s3/object.rb:115:in `temporary_url'
 config/initializers/s3_paperclip.rb:63:in `expiring_url'
moiristo commented 13 years ago

Please ignore the solution described above; you'll end up in encoding hell, as the uploaded filenames will be encoded incorrectly. I only needed to make sure the resource was escaped when requesting a temporary url. Just add this at the end of s3_paperclip.rb:

module S3
  class Signature        
    def self.generate_temporary_url_signature(options)
      bucket = options[:bucket]
      resource = options[:resource]
      secret_access_key = options[:secret_access_key]
      expires = options[:expires_at]

      headers = options[:headers] || {}
      headers.merge!('date' => expires.to_i.to_s)

      options.merge!(:resource => "/#{bucket}/#{URI.escape(resource)}",
                     :method => options[:method] || :get,
                     :headers => headers)
      signature = canonicalized_signature(options)

      CGI.escape(signature)
    end
  end
end  
qoobaa commented 13 years ago

It should be possible to solve it without monkey patching of the gem. Can you describe the problem more precisely? I'll try to reproduce it when I find some time.

Thanks for reporting the issue.

moiristo commented 13 years ago

The problem is that I cannot request a temporary URL for uploaded files that contain spaces, e.g. 'test test with spaces.and.dots.txt'. Uploading works fine, but requesting a temporary URL results in the exception I described in my first post. I cannot url-escape the resource in s3_paperclip.rb, because that results in a bad signature.

mike-park commented 13 years ago

The same problem occurs when a key includes German umlaut characters: ie ä, ö, ß etc.

jhuckabee commented 13 years ago

+1

Also getting the bad URI error when referencing files with special characters in them.

qoobaa commented 13 years ago

Please create a proper pull request if anybody has time and idea to fix this issue.

moiristo commented 13 years ago

For me, the patch I added fixed the problem; so basically if you add the URI escape ('...options.merge!(:resource => "/#{bucket}/#{URI.escape(resource)}",...'), the problem should be solved. If everyone agrees with this solution, I can create a pull request from it.

crystalneth commented 13 years ago

I believe I've fixed all these issues in my upcoming pull request, although there are no tests because I couldn't figure out how to get yours to run.

There were two issues: 1) The paperclip interpolators were not URL encoding - this was the issue with spaces. I added proper encoding (as noted below) to these interpolators. 2) The core s3 connection was improperly encoding the key - it was using the default URI.escape (same as encode) which does not encode reserved characters such as [ and ]. This can generate invalid URIs as well, but was not a problem for spaces. I fixed this by encoding all characters except explicitly legal ones and /. If for some god forsaken reason you have a slash in your filename, this may break.

moiristo commented 13 years ago

Cool, thanks for the effort!

crystalneth commented 13 years ago

Thanks for responding so quickly.

I'd also document the change, because I'm guessing a lot of people worked around this by calling URI.encode on the generated URLs, which would have worked most of the time.

qoobaa commented 12 years ago

I suggest moving paperclip/attachment_fu integration to separate gems.