New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix malformed asset_url in ActionController::Renderer #28250

Merged
merged 1 commit into from Mar 6, 2017

Conversation

Projects
None yet
3 participants
@tzabaman
Contributor

tzabaman commented Mar 2, 2017

Summary

#28151
Fix malformed asset_url when rendering a template with ActionController::Renderer

actionpack/test/controller/renderer_test.rb Outdated
renderer = ApplicationController.renderer
content = renderer.render template: "test/render_file_with_asset"
assert_equal "http://example.org/asset.jpg", content

This comment has been minimized.

@Erol

Erol Mar 2, 2017

Contributor

Can you also test the scenario where a renderer is initialized with url_scheme: "https"?

This comment has been minimized.

@tzabaman

tzabaman Mar 2, 2017

Contributor

@Erol Done!

@pixeltrix pixeltrix added the actionpack label Mar 4, 2017

@pixeltrix pixeltrix added this to the 5.1.0 milestone Mar 4, 2017

@pixeltrix pixeltrix self-assigned this Mar 4, 2017

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 4, 2017

What's the need to switch to url_scheme as the API? Has something changed elsewhere? I assume that https: true used to work? Did something change in Rack?

@tzabaman

This comment has been minimized.

Contributor

tzabaman commented Mar 4, 2017

@pixeltrix Take a look at issue #28151 . The problem is described inside the conversation. I don't know if something has changed in Rack but the asset_url was not working properly. asset_url was correct only if https was set to true. We had a malformed url if https was set to false. So scheme_url with params 'http' or 'https' seemed to be solving the problem. At the end of the day https param was was no longer necessary!

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 6, 2017

@tzabaman that's a good first step - you've found out the how, but we need to know when and why too.

e7633bedf897bb24ce668ac9c5df6bf88a58ff7e114d27606a756f4c4888a3f1

So the first step is to write a standalone test case that we can use to test against multiple versions:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", "5.0.2"
end

require "rack/test"
require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = File.dirname(__FILE__)
  config.session_store :cookie_store, key: "cookie_store_key"
  secrets.secret_token    = "secret_token"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
  end
end

class ApplicationController < ActionController::Base; end

require "minitest/autorun"

class BugTest < Minitest::Test
  def test_default_renderer_with_asset_url
    template = "<%= asset_url('avatar.jpg') %>"
    renderer = ApplicationController.renderer
    rendered = renderer.render inline: template

    assert_equal "http://example.org/avatar.jpg", rendered
  end
end

So starting with Rails 5.0.2 what do we get?

  1) Failure:
BugTest#test_default_renderer_with_asset_url [rails-28151.rb:41]:
--- expected
+++ actual
@@ -1 +1 @@
-"http://example.org/avatar.jpg"
+"http://://example.org:80/avatar.jpg"

Okay, so what about Rails 5.0.0? Change the gemspec to the following:

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", "5.0.0"
end

Re-run the test and what do we get?

  1) Failure:
BugTest#test_default_renderer_with_asset_url [rails-28151.rb:41]:
--- expected
+++ actual
@@ -1 +1 @@
-"http://example.org/avatar.jpg"
+"http://://example.org:80/avatar.jpg"

Nope, still broken. Looking at the history for actionpack/lib/action_controller/renderer.rb we can see a significant refactoring took place in 2db7304 - what if we go back earlier than that and test it worked when first committed. To do this we need to use the following gemspec:

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", github: "rails/rails", ref: "801e399" # commit where feature was introduced
  gem "arel", github: "rails/arel", ref: "56040f2" # arel master HEAD at the time
end

So surely this will work, right?

  1) Failure:
BugTest#test_default_renderer_with_asset_url [rails-28151.rb:41]:
--- expected
+++ actual
@@ -1 +1 @@
-"http://example.org/avatar.jpg"
+"http://://example.org:80/avatar.jpg"

Nope, seems like this bug has aways been around. 😞

Okay, we now know when - it's always been broken. The why that obviously follows on is that no-one wrote a test for it.

So what about fixing it? Well, since https: true works then it seems a bit unfortunate that to fix the case where people haven't set anything we need to get the people who did to change their code. If we search elsewhere in the Rails code base we find this line in action_dispatch/testing/integration.rb

"rack.url_scheme" => https? ? "https" : "http"

Can we use a similar technique to fix this problem? Turns out if we change the normalize_keys method to this:

def normalize_keys(env)
  new_env = {}
  env.each_pair { |k,v| new_env[rack_key_for(k)] = rack_value_for(k, v) }
  new_env['rack.url_scheme'] = new_env['HTTPS'] == 'on' ? 'https' : 'http'
  new_env
end

and re-run our test then we get:

.

Finished in 0.130983s, 7.6346 runs/s, 7.6346 assertions/s.

1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

🎉 - the bug is fixed without anyone having to change their code.

So @tzabaman can you update your PR with this fix and remove the deprecation? Thanks 👍

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 6, 2017

@tzabaman I think something went wrong with your rebase - it's showing 86 commits.

@tzabaman

This comment has been minimized.

Contributor

tzabaman commented Mar 6, 2017

@pixeltrix Maybe that happened because I rebased my branch with rails master... Do you propose to open a new PR and drop this one?

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 6, 2017

@tzabaman try squashing all your commits into a single commit and then rebase that one against master

@tzabaman tzabaman force-pushed the tzabaman:fix_malformed_asset_url_in_ac_renderer branch to 32046de Mar 6, 2017

@tzabaman

This comment has been minimized.

Contributor

tzabaman commented Mar 6, 2017

@pixeltrix Thank you very much for your precious help! The methodology with which you approached and finally solved the issue will be very useful for future similar situations! 👍

@pixeltrix pixeltrix merged commit 5ebbb62 into rails:master Mar 6, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 6, 2017

@tzabaman thanks! 👍

pixeltrix added a commit that referenced this pull request Mar 6, 2017

pixeltrix added a commit that referenced this pull request Mar 6, 2017

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 6, 2017

Backported to 5-0-stable in f69edbe

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 6, 2017

@tzabaman for future reference we normally add a CHANGELOG entry, thanks again.

@tzabaman

This comment has been minimized.

Contributor

tzabaman commented Mar 6, 2017

@pixeltrix Thank you, I will have it in mind next time!

@tzabaman tzabaman deleted the tzabaman:fix_malformed_asset_url_in_ac_renderer branch Mar 6, 2017

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