Re-parse the virtual_host URI in presigned_url after changing the scheme. Fixes #1400, #1401

[kjwierenga]

Presigner#presigned_url should not mutate params hash when it processes paramaters such as virtual_host and secure. This is fixed by duplicating the hash. Fixes #1400

[kjwierenga]

This PR is dependent on PR #1402

[awood45]

Update on this - I'm trying to get an environment set up to properly test this. I believe that the port 80 explicit mention may have been for a specific bug, and I would want to avoid a regression of the same. We only have an integration test around this currently.

[awood45]

Okay, there does appear to be a problem which makes this issue a bit more complicated than the included PR. Consider the original purpose of this code snippet, which is to switch the URI to http. For your use case of a non-standard port, your PR is fine:

uri = URI("https://foo.bar:3000") #=> #<URI::HTTPS https://foo.bar:3000>
uri.port #=> 3000
uri.scheme = "http"
uri #=> #<URI::HTTPS http://foo.bar:3000>
uri.port #=> 3000

However, it doesn't work for the standard HTTP/HTTPS ports - that explicit setter is needed:

uri = URI("https://foo.bar") #=> #<URI::HTTPS https://foo.bar>
uri.port #=> 443
uri.scheme = "http"
uri #=> #<URI::HTTPS http://foo.bar>
uri.port #=> 443 - wrong

That doesn't mean we can't fix this issue - I'd like to support your ability to set ports, but we need to do a bit more. I'll copy this comment to the PR.

[kjwierenga]

You can change the scheme of a URI object, but that doesn't change the class of the object. This is what is causing the default port to be appended.

uri = URI("https://foo.bar") #=> #<URI::HTTPS https://foo.bar> 
uri.to_s # => "https://foo.bar" 
uri.scheme='http'
uri.port=80
uri.to_s # => "http://foo.bar:80" 
URI.parse(uri.to_s).to_s
# Fix like this
URI(uri.to_s) # => #<URI::HTTP http://foo.bar> 

Same goes the other way around:

uri=URI("http://foo.bar") # => #<URI::HTTP http://foo.bar> 
uri.to_s # => "http://foo.bar" 
uri.scheme='https'
uri.port=443
uri.to_s # => "https://foo.bar:443" 
# Fix like this
URI(uri.to_s) # => #<URI::HTTPS https://foo.bar> 

Note how the URI subclass is determined at first parse of the input URL.

Fix is to re-parse the URI after changing the scheme to a URI object with the appropriate subclass gets created.

[kjwierenga]

This is a better way of fixing this issue I think. The solution was there all along in sign_but_dont_send which applies exactly the same technique.

[awood45]

Indeed, a topic of discussion in our team right now is to potentially move a lot of this logic into a single class to avoid exactly these kinds of inconsistencies. Let's change the PR to use this kind of approach and then we should be good to go.

…

On Feb 1, 2017, at 8:35 AM, Klaas Jan Wierenga ***@***.***> wrote: This is a better way of fixing this issue I think. The solution was there all along in sign_but_dont_send which applies exactly the same technique. ― You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[cjyclaire]

PR #1477 has opened including support for this PR, closing, tracking in #1477 instead.
The spec test has been included as well.
Feel free to chime in and add comment in that PR, thanks : )