21 Dec, 2015

What to Expect When You're Overriding

by Ernie Miller

We’ve all been there. You’re using some library or framework that’s saving you SO MUCH TIME… until you run into that one little thing it does wrong. Whether “wrong” means buggy or just “different than the way I would do it,” you’re now faced with a choice: do you override behavior or do you live with it? Sometimes the obvious choice can have unintended consequences.

Some Background

As “the Rails guy” around these parts, sometimes folks working on an assessment will ping me with questions when something they’re absolutely sure should be working just… isn’t. Last week, such a question arose in chat:

@ernie: I’m doing an assessment, and for the life of me, I cannot understand why Devise’s paranoid mode won’t work

Devise is a library that provides a highly-configurable authentication solution for Rails. So long as you’re not doing anything really out of the ordinary, it’s much, much faster than rolling your own authentication solution, so it sees lots of real-world usage, which means lots of real-world testing.

One of Devise’s configuration options is “paranoid mode”. It tells Devise that it should be intentionally vague in its error messages involving user credentials in order to prevent a user enumeration attack.

So, for instance, if you visit a “reset password” page on a stock Devise configuration and enter an e-mail address that doesn’t exist in the system, you might see a message saying: E-mail not found.

With paranoid mode enabled, you’d instead see the following: If your email address exists in our database, you will receive a password recovery link at your email address in a few minutes.

In this way, we avoid letting a potential attacker know whether an e-mail address exists in our system.

So, our client’s application was vulnerabile to user enumeration, and used Devise. Our consultant was about to recommend that they enable paranoid mode. But, we don’t just make recommendations without verifying they’ll work. So, he enabled paranoid mode.

And it didn’t work.

I could understand his confusion. In my experience, Devise is generally a pretty solid piece of software that does what it says it will do.

What happened?

Subclassing Happened

When you reset a password in Devise, it happens through the create method of Devise::PasswordsController. That method looks like this:

def create
  self.resource = resource_class.send_reset_password_instructions(resource_params)
  yield resource if block_given?

  if successfully_sent?(resource)
    respond_with(
      {},
      location: after_sending_reset_password_instructions_path_for(resource_name)
    )
  else
    respond_with(resource)
  end
end

So, if successfully_sent? is truthy, we respond with a redirect. Here’s successfully_sent? (from DeviseController, Devise::PasswordsController’s parent class):

def successfully_sent?(resource)
  notice = if Devise.paranoid
    resource.errors.clear
    :send_paranoid_instructions
  elsif resource.errors.empty?
    :send_instructions
  end

  if notice
    set_flash_message :notice, notice if is_flashing_format?
    true
  end
end

You’ll notice here that if Devise.paranoid is true, we’ll clear any errors on the model and set a value for notice, which results in a flash message being displayed to the user, and (more importantly for this discussion) a true value being returned from this method…

…which would be great, if this method were ever being called.

This project also used Spree, an e-commerce library implemented as a Rails engine, and spree_auth_devise for its authentication.

One of the nice things about Devise is that if you need to do something very different from its default behavior, it supports subclassing its controllers. spree_auth_devise did this to Devise::PasswordsController. Here’s the relevant method:

# Overridden due to bug in Devise.
#   respond_with resource, :location => new_session_path(resource_name)
# is generating bad url /session/new.user
#
# overridden to:
#   respond_with resource, :location => spree.login_path
#
def create
  self.resource = resource_class.send_reset_password_instructions(params[resource_name])

  if resource.errors.empty?
    set_flash_message(:notice, :send_instructions) if is_navigational_format?
    respond_with resource, :location => spree.admin_login_path
  else
    respond_with_navigational(resource) { render :new }
  end
end

Reasonable enough. This code was written back in 2012, to fix a bug that obviously existed at that point.

However, this gem uses the latest version of Devise, now. Notice anything missing from this controller?

Right, it’s not calling successfully_sent?, because there was no such thing back when this code was written.

The Problem

Here’s the problem: when you override behavior of third-party code, you now own responsibility for that functionality.

There’s no escaping this reality. Part of their code is now your code. This means that you should be testing its behavior and paying attention to your dependency’s commits, especially those that touch code you’re overriding.

This is a lot of work. As a developer, there are a few things you can do to make life simpler for yourself.

Call super (and write code that makes this possible)

In Ruby, you can invoke a superclass’s implementation with the super keyword, or the super() method.

If you’re writing code intended for reuse via subclassing, try to write it in such a way that portions of its functionality can be overridden without requiring the reproduction of lots of existing code.

This means writing shorter methods, generally, and structuring code in such a way that it lends itself to overridden behavior. One way is to make use of the template method design pattern where applicable.

If you’re looking to override some behavior, check to see if the library you’re working with makes this easy – if you’re not able to call super in your code, there’s a higher-than-usual chance you’re going to miss out on changes upstream.

Reduce Dependencies

Are you using a library for a tiny piece of functionality? Maybe it would be better to implement the functionality yourself. Yes, then you still own the maintenance of this code, but now that’s obvious to you and everyone else that works with the codebase.

Contribute Back

If you’re working with a library that’s open source, send fixes upstream, implement any overrides you need to solve your immediate need, and put a link to the pull request with the fixes in a comment within your code. This way, if and when the fix is merged, you can remove your workaround, and relieve yourself of the burden of maintaining that functionality. No, this doesn’t relieve you of the burden of doing security audits on your dependencies, but I know a company that can help with that.

In fact, this is what we did in this case. We’ve put together a pull request with an update to the spree_auth_devise gem to allow it to work properly with Devise’s paranoid mode. Now, hopefully, fewer people will have to troubleshoot this issue going forward!