2
\$\begingroup\$

I'm trying to make a class for safe redirection of URLs. Redirecting URLs is something you can have in a get parameter, for example:

http://localhost/login?return_url=/landing1

So the attacker can't change it to something external and even internal. I implemented it this way for a Rails project:

class SafeReturnUrl
  WHITELIST = [
    '/landing1',
    '/landing2'
  ].freeze

  def initialize(url)
    @url = url
  end

  def to_str
    return nil if @url.blank?
    raise "Url #{@url} should be whitelisted" unless WHITELIST.include?(@url)
    @url
  end
end

An alternative implementation is to have Rails URL helpers, but in this case a whitelist can be initialized only when the class is initialized:

class SafeReturnUrl
  include Rails.application.routes.url_helpers

  def initialize(url)
    @whitelist = [
      landing1_path,
      landing2_path
    ]
    @url = url
  end

  def to_str
    return nil if @url.blank?
    raise "Url #{@url} should be whitelisted" unless @whitelist.include?(@url)
    @url
  end
end

It's not very scalable and the price for that - the whitelistis initialized every time when SafeReturnUrl is initialized.

I covered the first version with tests, but for my team it's not enough ("So unmaintainable"). I don't think it's unmaintainable, because it's covered with tests, and there is no performance penalty.

I'm not using HashSet here - it's another type of optimization. We'll add it when we'll have a list with more than 10 elements.

Which version looks better and is there any way it can be optimized?

\$\endgroup\$
2
\$\begingroup\$

You could put the whitelist in an initializer since you're using Rails, then the whitelist is available globally.

WHITELIST = [
  '/landing1',
  '/landing2'
]

Then it would be possible in your login controller to do something like this:

redirect_to WHITELIST.include?(params[:return_url]) ? params[:return_url] : root_path

The downside would be that you need to restart your rails app if you need to update the whitelist.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.