Malformed asset url when rendering a view partial with ActionController::Renderer

[tzabaman]

I am facing an issue with malformed url of assets when rendering a partial template through ActionController::Renderer.

1. Put an image inside app/assets/images folder, let's name it noavatar.jpg
2. Create a _test.text.erb partial under app/views/shared folder (shared is a new folder)

This partial contains the following code:
<%= asset_url('noavatar.jpg') %> or it can be <%= path_url('noavatar.jpg') %>

Now open a rails console and run the following code:

ApplicationController.render(partial: 'shared/test')

You will get the following result:

http://://example.org:80/assets/noavatar-04b48dfebdd9a9ec5971b22b3bfe8144c04e2de8909adbb5a52897a881dea8df.jpg

Notice that not only the protocol inside the url is malformed but also we are getting and a non relevant 80 port.

Now lets set the https parameter to true

renderer = ApplicationController.renderer.new(https: true)
renderer.render(partial: 'shared/test')

Now everything seems to be working perfectly!

https://example.org/assets/noavatar-04b48dfebdd9a9ec5971b22b3bfe8144c04e2de8909adbb5a52897a881dea8df.jpg

Notice: This malfunction is not happening with normal page urls. For example if you insert the following code inside 'test' partial:

<%= url_for(action: 'index', controller: 'products', only_path: false) %>

and run in console once again

ApplicationController.render(partial: 'shared/test')

you will get the expected result

http://example.org/products

System configuration

Rails version: 5.1.0.beta1

Ruby version: 2.3.1p112 (2016-04-26 revision 54768) [x86_64-linux]

[Erol]

This seems to be a problem with Rack::Request#scheme. When none of the below headers are set - which is the case when using ApplicationController.render - scheme returns nil and Rack::Request#base_url returns a malformed URL:

HTTPS
HTTP_X_FORWARDED_SSL
HTTP_X_FORWARDED_SCHEME
HTTP_X_FORWARDED_PROTO
RACK_URL_SCHEME

I'm thinking this might be addressed with a separate implementation of scheme in ActionDispatch::Request:

module ActionDispatch
  class Request
    def scheme
      super || protocol.gsub("://", "")
    end
  end
end

WDYT @rafaelfranca? If you're good with this solution, I can open a PR.

[tzabaman]

@Erol : Researching the problem by myself, I also reached the scheme method inside rack gem that returns nil.

The fact is that when rendering a partial with the "traditional" way we get the correct asset_url.

So I am thinking that instead of 'patching' the scheme method inside ActionDispatch::Request, maybe it would be better to create the appropriate environment when using ApplicationController.render in order scheme method to respond correctly to any request without having to override it inside rails.

This is just a theoretical approach, I don't know what the limitations are, so excuse me for any mistakes!

[Erol]

Come to think of it, this can also be addressed by setting:

config.action_controller.asset_host = "http://my.assets.com"

[tzabaman]

Configuration with config.action_controller.asset_host seems to be working.

The only problem is that someone must add this line manually inside

config/environments/development.rb

and uncomment this line inside

config/environments/production.rb

So this is not very "convention over configuration"!

I did a deeper research and I came across the following solution.

I put a byebug inside Rack::Request#scheme. Running the following code from console when the execution stopped: env.keys.map{ |k| "#{k} => #{env[k]}" } I instantly realized that the env variable is way more richer when let's say a partial is being parsed with the "traditional" way. On the other hand when a partial is being parsed via ApplicationController.render the env variable contains only the following keys:

HTTP_HOST => example.org,
HTTPS => off,
REQUEST_METHOD => GET,
SCRIPT_NAME => ,
rack.input => ,
action_dispatch.routes => #<ActionDispatch::Routing::RouteSet:0x000000017c2828>,
action_controller.instance => #<ApplicationController:0x00000003f25a78>

So that's it! Why don't we add more parameters into ActionController::Renderer#env when instantiating it?

Finally my solution looks like this:

module ActionController
  class Renderer

    DEFAULTS = {
      http_host: "example.org",
      # https: false, # WE DON'T NEED THIS LINE ANY MORE
      method: "get",
      script_name: "",
      input: "",
      url_scheme: "http" # <----- THIS LINE ADDED
    }.freeze

    private

      RACK_KEY_TRANSLATION = {
        http_host:   "HTTP_HOST",
        # https:       "HTTPS", # WE DON'T NEED THIS LINE ANY MORE
        method:      "REQUEST_METHOD",
        script_name: "SCRIPT_NAME",
        input:       "rack.input",
        url_scheme:  "rack.url_scheme" # <----- THIS LINE ADDED
      }

  end
end

WDYT @rafaelfranca and @Erol ?

[tzabaman]

Added a PR for review! #28250

[Erol]

So that's it! Why don't we add more parameters into ActionController::Renderer#env when instantiating it?

If the API is going to be changed - a parameter was dropped - I think at the very least a deprecation notice should be served.

So this is not very "convention over configuration"!

I have a different opinion on this. Similar to ActionMailer instances, ApplicationController.renderer does not have context about the request and cannot infer the host on its own. Hence you need to provide configuration to let it know what the asset host should be.

[tzabaman]

I have a different opinion on this. Similar to ActionMailer instances, ApplicationController.renderer does not have context about the request and cannot infer the host on its own. Hence you need to provide configuration to let it know what the asset host should be.

I agree that ApplicationController.renderer does not have its own context so it may need a little a bit help from external configuration. What am I saying is that we must use an extra configuration in case we want to override the default behaviour (e.g. url), instead of use some kind of configuration in order to overlap an error, at this case our malformed url. So let it work, then override it! I think we say the same thing with different way!

If the API is going to be changed - a parameter was dropped - I think at the very least a deprecation notice should be served.

Sorry I don't understand exactly which API do you mean. So at your opinion we must write additional tests in order to check if existing parameters are valid or check parameters validity inside the actual class during runtime?

[Erol]

I agree that ApplicationController.renderer does not have its own context so it may need a little a bit help from external configuration. What am I saying is that we must use an extra configuration in case we want to override the default behaviour (e.g. url), instead of use some kind of configuration in order to overlap an error, at this case our malformed url.

I think I wasn't able to make my point clear. The default behavior as it stands is not entirely correct, malformed URL or not. I doubt anyone is using example.org as their host. 😉

But of course, the malformed URL being generated by asset_url needs to be addressed.

Sorry I don't understand exactly which API do you mean.

You're proposing a change on how new renderers are instantiated, such that ApplicationController.renderer.new(https: true) will no longer be valid. The deprecation would be for others who are already using this and to notify them to switch from using the https parameter to url_scheme.

[tzabaman]

The default behavior as it stands is not entirely correct, malformed URL or not. I doubt anyone is using example.org as their host.

I agree 100%. Parameter host will be changed for sure because inside rendered partial, except form an asset url, we may also need to add a typical http url.

You're proposing a change on how new renderers are instantiated, such that ApplicationController.renderer.new(https: true) will no longer be valid. The deprecation would be for others who are already using this and to notify them to switch from using the https parameter to url_scheme.

You are absolutely right, I haven't thought about that scenario. I just made an additional commit on my merge request. Thanks again for your advice!

[pixeltrix]

Fixed by merging #28250