Shrine: remote_url plugin doesn't handle URLs with spaces or certain URL-encodings

[abevoelker]

Perhaps you'll consider this something the user needs to handle at the application level (URI parsing/encoding), but I thought I'd throw it out there anyway as it surprised me:

require "active_record"
require "shrine"
require "shrine/storage/file_system"
require "tmpdir"
require "open-uri"

Shrine.plugin :activerecord
Shrine.plugin :remote_url, max_size: nil
Shrine.storages = {
  cache: Shrine::Storage::FileSystem.new(Dir.tmpdir, prefix: "cache"),
  store: Shrine::Storage::FileSystem.new(Dir.tmpdir, prefix: "store"),
}

class MyUploader < Shrine
  # plugins and uploading logic
end

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.connection.create_table(:posts) { |t| t.text :image_data }
# make errors propagate when raised in callbacks
ActiveRecord::Base.raise_in_transactional_callbacks = true

class Post < ActiveRecord::Base
  include MyUploader[:image]
end

# reproduction follows:
p = Post.new

# URL with spaces fails:
url = "https://www.atlanticfirearms.com/images/stories/virtuemart/product/vepr12 (20 of 22).jpg"
p.image_remote_url = url
p.image # => nil
p.image_attacher.errors # => ["download failed: file not found"]

# full CGI encoding the URL fails:
require 'cgi'
cgi_url = CGI::escape(url) # => "https%3A%2F%2Fwww.atlanticfirearms.com%2Fimages%2Fstories%2Fvirtuemart%2Fproduct%2Fvepr12+%2820+of+22%29.jpg"
p.image_remote_url = cgi_url
p.image # => nil
p.image_attacher.errors # => ["download failed: file not found"]

# partial URL-encoding (encoding just the spaces) with the "addressable" gem works! :
require 'addressable/uri'
addr_url = Addressable::URI.escape(url) # => "https://www.atlanticfirearms.com/images/stories/virtuemart/product/vepr12%20(20%20of%2022).jpg"
p.image_remote_url = addr_url
p.image # => => #<MyUploader::UploadedFile:0x0056407c7804e8 @data={"id"=>"a53f32dc4b9e82e3736537fb17a70da5.jpg", "storage"=>"cache", "metadata"=>{"filename"=>"vepr12 (20 of 22).jpg", "size"=>197243, "mime_type"=>"image/jpeg"}}>

I have to say it's pretty awesome how configurable you made Shrine so that I easily solved this in my own application with a custom downloader:

require "down"
Shrine.plugin :remote_url, max_size: 20*1024*1024, downloader: ->(url, max_size:) do
  Down.download(Addressable::URI.escape(url), max_size: max_size, max_redirects: 10, read_timeout: 3)
end

(although one other quick note - without require "down" I was getting ["download failed: uninitialized constant Down"] - the require is not in the plugin's usage comments)

Shrine is a really great gem (fixed so many headaches I had with unclear CarrierWave code), so thanks for making it!

[janko]

@abevoelker @jnylen That's a good question, there was actually a history about this in Down, which initially supported both encoded and non-encoded URLs, but then this behaviour got removed in Down 2.0 due to instability.

So, Down was initially doing this:

url = URI.encode(URI.decode(url))

This way if an already encoded URL is passed in, we first URI.unescape it so that it doesn't end up being double-encoded, and if someone passes an non-encoded URL, URI.unescape will be a no-op and URI.escape would encode it. However, URI.encode doesn't properly encode all URLs:

URI.encode("http://example.com/foo[1].jpg") #=> "http://example.com/foo[1].jpg"
URI.encode("http://example.com/#foo")       #=> "http://example.com/%23foo"

In the above example we see that [] wasn't properly encoded, the result isn't a valid URI (URI.parse would raise an error). We also see that # was encoded although it shouldn't be.

The natural idea would be to hack it like CarrierWave did, however, I didn't consider that a good idea because URI.encode and URI.decode are in fact deprecated for a long time already (7 years), seemingly without a valid alternative (see the discussion).

So I removed the support for passing non-encoded URLs, because I couldn't imagine a scenario where the user would copy-paste a non-encoded URL (whenever you're copying links from the browser, they are automatically encoded AFAIK). However, I'm probably wrong with this assumption, since the reason why this was added to CarrierWave in the first place seems to be because of production errors (see https://github.com/carrierwaveuploader/carrierwave/pull/115).

Addressable would I think be ideal to use, because it seems that all URI related issues are solved by just using Addressable. However, Addressable isn't a particularly light dependency (see https://github.com/sdsykes/fastimage/issues/45), and for me it's really important that Shrine is as light as possible, so I was reluctant to add it as a Down dependency.

I didn't read the whole Ruby discussion about deprecating URI.encode/URI.decode, and I haven't found any recent discussions about what are valid alternatives, perhaps it would be worth investigating that further.

See also https://github.com/janko-m/shrine/issues/38 for the original issue.

[janko]

Btw, the reason why remote_url isn't always requiring Down is because the custom downloader doesn't have to use use Down, so that if users want to use something else they don't have to require Down.

[abevoelker]

I see, thanks for the very in-depth info! Looks like there's no easy win then. If you want to leave URI encode/decode up to the gem user that makes sense to me, feel free to close. Maybe just with a caveat in the remote_url plugin comments (up to you) to that effect? Or perhaps in the CarrierWave migration doc, as that is where I came from and thanks to their hack you referenced this isn't something I had encountered before with these URLs? All up to you, I don't feel strongly about anything since my issue is resolved, hehe.

[janko]

The freshly released Down 3.0.0 adds a HTTP.rb backend for downloading files (as an alternative to open-uri + Net::HTTP). Among other advantages, HTTP.rb uses Addressable::URI for parsing URLs, so if you require "down/http" before requiring Shrine (and add the HTTP.rb gem to you Gemfile), you don't need the additional step of escaping the URL.

[abevoelker]

Excellent! I will migrate to that method when I have time. Thanks for the follow-up.