upgrade from 1.3.1 to 2.0.0 breaks remote_x_url when coming from s3

[Paul-Yves]

Something weird happens when doing

 order_attachment.remote_document_file_url = template.document_url
 order_attachment.save!

where template.document_url is an URL from another uploader (using s3, yes we are basically copying a file from an uploader to an other). We have a could not download file: 400 Bad Request error. The weird part is that when using a url from somewhere else (any image on the internet for example) there is no issue.

What could go wrong?

[jaredbeck]

I tested #2419 but unfortunately, it does not seem to fix this issue. I tried

gem 'carrierwave',
  branch: 'master',
  github: 'carrierwaveuploader/carrierwave',
  ref: 'c4b6d91bd7ed759bb83a440800c450c79a987dd8'

but still getting could not download file: 400 Bad Request.

Downgrading to 1.3.1 fixes the issue.

My usage is something like this:

# user.rb
mount_uploader :redacted_file, RedactedUploader
# controller
url = user.redacted_file.url
RedactedJob.perform_later(url)
# in redacted_job, about 10s later
RedactedUploader.new.download!(url) # 400 Bad Request

Hope that helps.

[fonglh]

@trangmei and I found that the URL from our other uploader is already encoded, but CarrierWave encodes it again when trying to download it.

This causes the / separator, already encoded as %2F, to be double encoded as %252F, breaking the URLs.

[jaredbeck]

.. the URL from our other uploader is already encoded, CarrierWave encodes it again when trying to downloading it.

Thanks @fonglh, using CGI.unescape worked for me.

Do you know which half of this changed between 1.3 and 2.0? I.e. did the url method change or did download!? We should add the breaking change to the changelog.

[hasimo]

I got same error and I realize that Addressable::URI#normalize method replace the some characters.
For example %EF%BC%A8 (FULLWIDTH H) is replaced to H (half H).

related: sporkmonger/addressable#196

[nitrotm]

Right, I just don't understand why CarrierWave::Downloader::Base.process_uri is present at all.

Can't we just assume the url provided is valid? I think that's the responsibility of the caller to ensure that and eventually encode the url properly if needed.

There is no reliable fix possible to process_uri, because for instance it's ambiguous if a '%' character is intended as an encoded character or part of a key or value. Assuming the url was previously encoded is the only choice I think.

https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/downloader/base.rb

[yesthatguy]

+1 for assuming the URI provided is valid

If it turns out we really want to encode in some cases because callers are depending on it, then it seems like the best check is whether Addressable::URI.unencode(uri_parts.join('?')) == uri_parts.join('?'). If that check is false, the URI is already encoded, so we shouldn't encode it again. If that check is true, the URI may need encoding, or it may have no characters that need encoding, and in both cases it's safe to encode it.
More discussion here:
https://stackoverflow.com/questions/2295223/how-to-find-out-if-string-has-already-been-url-encoded

[mshibuya]

I believe 3faf749 have fixed this, please try with the master.