Securely and dynamically calling methods based on user input in Rails

Question

I have an API that I built in Rails. It runs some methods I've defined in a module and renders their return values as JSON. While I've been developing, the entire code for the API has been the module itself (contents irrelevant), a single route:

  controller :cool do
    get "cool/query/*args" => :query
  end

and this:

class CoolController < ApplicationController
  include CoolModule
  def query
    args = params[:args].split("/")

    # convert the API URL to the method name
    method_symbol = args[0].tr("-","_").to_sym

    if !CoolModule.method_defined?(method_symbol)
      return nil
    end

    # is calling self.method a good idea here, or is there a better way?
    render json: self.method(method_symbol).call(args[1], args[2])

  end
end

My API (i.e. the module) contains ~30 functions each accepting a variable number of arguments, the routing logic for which I'd like to keep nicely wrapped in the module (as it is now).

It will be used as a "mid-end" (one might say) between my cool ajax front-end and another API which I don't control and is really the back-end proper. So special concern needs to be given since it both receives user input and sends queries to a third party (which I am accountable for).

My questions specifically are:

Will this general strategy (method names directly from queries) be secure/stable for production?
If the strategy is acceptable but my implementation is not, what changes are necessary?
If the strategy is fundamentally flawed, what alternatives should I pursue?

The pessimist in me says 'miles of case-when,' but I'll thank you for your input.

By the way, you can shorten self.method(method_symbol).call(args[1], args[2]) to self.send(method_symbol, *args[1..2]), by the way. And it looks nicer. — Jwosty
– Jwosty, Commented Mar 19, 2012 at 22:45
What's wrong with a simple whitelist of methods that can be called? — mu is too short
– mu is too short, Commented Mar 19, 2012 at 22:50
I'd considered it, but I'd like to keep the controller from knowing that much about the module. Also, a thirty+ element whitelist would be unsightly compared to the compactness of this code. A suitable option, if all else fails. — bkconrad
– bkconrad, Commented Mar 19, 2012 at 23:12

dbenhur · Accepted Answer · 2012-03-20 01:28:50Z

The problem with Module#method_defined? is it may return true on indirect method definitions (other included modules, inherited methods if module is a Class) as well as private methods. This means you (and importantly anyone else who touches the code) will have to be very careful what you do with that module.

So, you could use this approach, but you need to be super explicit to your future maintainers that any method in the module is automatically an external interface. Personally, I would opt for something more explicit, like a simple whitelist of allowed api method names, eg:

require 'set'

module CoolModule
  ALLOWED_API_METHODS = Set[
    :foo,
    :bar,
    ...
  ]

  def self.api_allowed? meth
    ALLOWED_API_METHODS.include? meth.to_sym
  end
end

Yeah, you have to maintain the list, but it's not unsightly, it's documentation of an explicit interface; and means you wont get bit by a later coder deciding he needs to add some utility methods to the module for convenience and thus accidentally exporting them to your external api.

Alternately to the single list, you could have a define_for_api method and use that instead of def to declare the api interface methods

module CoolModule
  @registered_api_methods = Set.new
  def self.define_for_api meth, &block
    define method meth, &block
    @registered_api_methods << meth
  end

  def self.api_allowed? meth
    @registered_api_methods.include? meth.to_sym
  end

  def api_dispatch meth, *args
    raise ArgumentError unless self.class.api_allowed? meth
    send(meth *args)
  end

  define_for_api :foo do |*args|
    do_something_common
    ...
  end

  define_for_api :bar do
    do_something_common
    ...
  end

  # this one is just ordinary method internal to module
  private
  def do_something_common
  end
end

I liked the idea of define_for_api, so I implemented it. Works like a charm. sorry for the wait.

Collectives™ on Stack Overflow

Securely and dynamically calling methods based on user input in Rails

1 Answer 1

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related