Merge pull request #1678 from padrino/mass-assign

implement query parameters filtering, can prevent mass-assignment, closes #905

padrino-core/lib/padrino-core/application.rb

@@ -3,6 +3,7 @@
 require 'padrino-core/application/show_exceptions'
 require 'padrino-core/application/authenticity_token'
 require 'padrino-core/application/application_setup'
+require 'padrino-core/application/params_protection'
 
 module Padrino
   ##
@@ -14,6 +15,7 @@ module Padrino
   class Application < Sinatra::Base
     register Padrino::ApplicationSetup
     register Padrino::Routing
+    register Padrino::ParamsProtection
 
     ##
     # Returns the logger for this application.

padrino-core/lib/padrino-core/application/params_protection.rb

@@ -0,0 +1,125 @@
+require 'active_support/core_ext/object/deep_dup'
+
+module Padrino
+  ##
+  # Padrino application module providing means for mass-assignment protection.
+  #
+  module ParamsProtection
+    class << self
+      def registered(app)
+        included(app)
+      end
+
+      def included(base)
+        base.send(:include, InstanceMethods)
+        base.extend(ClassMethods)
+      end
+    end
+
+    module ClassMethods
+      ##
+      # Implements filtering of url query params. Can prevent mass-assignment.
+      #
+      # @example
+      #   post :update, :params => [:name, :email]
+      #   post :update, :params => [:name, :id => Integer]
+      #   post :update, :params => [:name => proc{ |v| v.reverse }]
+      #   post :update, :params => [:name, :parent => [:name, :position]]
+      #   post :update, :params => false
+      #   post :update, :params => true
+      # @example
+      #   params :name, :email, :password => prox{ |v| v.reverse }
+      #   post :update
+      # @example
+      #   App.controller :accounts, :params => [:name, :position] do
+      #     post :create
+      #     post :update, :with => [ :id ], :params => [:name, :position, :addition]
+      #     get :show, :with => :id, :params => false
+      #     get :search, :params => true
+      #   end
+      #
+      def params(*allowed_params)
+        allowed_params = prepare_allowed_params(allowed_params)
+        condition do
+          @original_params = params.deep_dup
+          filter_params!(params, allowed_params)
+        end
+      end
+
+      private
+
+      def prepare_allowed_params(allowed_params)
+        param_filter = {}
+        allowed_params.each do |key,value|
+          case
+          when key.kind_of?(Hash) && !value
+            param_filter.update(prepare_allowed_params(key))
+          when value.kind_of?(Hash) || value.kind_of?(Array)
+            param_filter[key.to_s] = prepare_allowed_params(value)
+          else
+            param_filter[key.to_s] = value == false ? false : (value || true)
+          end
+        end
+        param_filter.freeze
+      end
+    end
+
+    module InstanceMethods
+      ##
+      # Filters a hash of parameters leaving only allowed ones and possibly
+      # typecasting and processing the others.
+      #
+      # @param [Hash] params
+      #   Parameters to filter.
+      #   Warning: this hash will be changed by deleting or replacing its values.
+      # @param [Hash] allowed_params
+      #   A hash of allowed keys and value classes or processing procs. Supported
+      #   scalar classes are: Integer (empty string is cast to nil).
+      #
+      # @example
+      #   filter_params!( { "a" => "1", "b" => "abc", "d" => "drop" },
+      #                   { "a" => Integer, "b" => true } )
+      #   # => { "a" => 1, "b" => "abc" }
+      #   filter_params!( { "id" => "", "child" => { "name" => "manny" } },
+      #                   { "id" => Integer, "child" => { "name" => proc{ |v| v.camelize } } } )
+      #   # => { "id" => nil, "child" => { "name" => "Manny" } }
+      #   filter_params!( { "a" => ["1", "2", "3"] },
+      #                   { "a" => true } )
+      #   # => { "a" => ["1", "2", "3"] }
+      #   filter_params!( { "persons" => {"p-1" => { "name" => "manny", "age" => "50" }, "p-2" => { "name" => "richard", "age" => "50" } } },
+      #                   { "persons" => { "name" => true } } )
+      #   # => { "persons" => {"p-1" => { "name" => "manny" }, "p-2" => { "name" => "richard" } } }
+      #
+      def filter_params!(params, allowed_params)
+        params.each do |key,value|
+          type = allowed_params[key]
+          next if value.kind_of?(Array) && type
+          case
+          when type.kind_of?(Hash)
+            if key == key.pluralize
+              value.each do |array_index,array_value|
+                value[array_index] = filter_params!(array_value, type)
+              end
+            else
+              params[key] = filter_params!(value, type)
+            end
+          when type == Integer
+            params[key] = value.empty? ? nil : value.to_i
+          when type.kind_of?(Proc)
+            params[key] = type.call(value)
+          when type == true
+          else
+            params.delete(key)
+          end
+        end
+      end
+
+      ##
+      # Returns the original unfiltered query parameters hash.
+      #
+      def original_params
+        @original_params || params
+      end
+    end
+  end
+end

padrino-core/lib/padrino-core/application/routing.rb

@@ -399,6 +399,7 @@ def controller(*args, &block)
           @_conditions, original_conditions = options.delete(:conditions), @_conditions
           @_defaults,   original_defaults   = options,                     @_defaults
           @_accepts,    original_accepts    = options.delete(:accepts),    @_accepts
+          @_params,     original_params     = options.delete(:params),     @_params
 
           # Application defaults.
           @filters,     original_filters    = { :before => @filters[:before].dup, :after => @filters[:after].dup }, @filters
@@ -414,6 +415,7 @@ def controller(*args, &block)
           @_controller, @_parents,  @_cache     = original_controller, original_parent,   original_cache
           @_defaults,   @_provides, @_map       = original_defaults,   original_provides, original_map
           @_conditions, @_use_format, @_accepts = original_conditions, original_use_format, original_accepts
+          @_params                              = original_params
         else
           include(*args) if extensions.any?
         end
@@ -710,6 +712,7 @@ def route(verb, path, *args, &block)
         route_options = options.dup
         route_options[:provides] = @_provides if @_provides
         route_options[:accepts]  = @_accepts if @_accepts
+        route_options[:params] = @_params unless @_params.nil? || route_options.include?(:params)
 
         # Add Sinatra condition to check rack-protection failure.
         if protect_from_csrf && (report_csrf_failure || allow_disabled_csrf)
@@ -784,6 +787,13 @@ def route(verb, path, *args, &block)
       def parse_route(path, options, verb)
         route_options = {}
 
+        if options[:params] == true
+          options.delete(:params)
+        elsif options.include?(:params)
+          options[:params] ||= []
+          options[:params] += options[:with] if options[:with]
+        end
+
         # We need check if path is a symbol, if that it's a named route.
         map = options.delete(:map)
 

padrino-core/test/test_params_protection.rb

@@ -0,0 +1,159 @@
+require File.expand_path(File.dirname(__FILE__) + '/helper')
+require 'active_support/core_ext/hash/conversions'
+
+describe "Padrino::ParamsProtection" do
+  before do
+    @teri = { 'name' => 'Teri Bauer', 'position' => 'baby' }
+    @kim = { 'name' => 'Kim Bauer', 'position' => 'daughter', 'child' => @teri }
+    @jack = { 'name' => 'Jack Bauer', 'position' => 'terrorist', 'child' => @kim }
+    @family = { 'name' => 'Bauer', 'persons' => { 1 => @teri, 2 => @kim, 3 => @jack } }
+  end
+
+  it 'should drop all parameters except allowed ones' do
+    result = nil
+    mock_app do
+      post :basic, :params => [ :name ] do
+        result = params
+        ''
+      end
+    end
+    post '/basic?' + @jack.to_query
+    assert_equal({ 'name' => @jack['name'] }, result)
+  end
+
+  it 'should preserve original params' do
+    result = nil
+    mock_app do
+      post :basic, :params => [ :name ] do
+        result = original_params
+        ''
+      end
+    end
+    post '/basic?' + @jack.to_query
+    assert_equal(@jack, result)
+  end
+
+  it 'should work with recursive data' do
+    result = nil
+    mock_app do
+      post :basic, :params => [ :name, :child => [ :name, :child => [ :name ] ] ] do
+        result = [params, original_params]
+        ''
+      end
+    end
+    post '/basic?' + @jack.to_query
+    assert_equal(
+      [
+        { 'name' => @jack['name'], 'child' => { 'name' => @kim['name'], 'child' => { 'name' => @teri['name'] } } },
+        @jack
+      ],
+      result
+    )
+  end
+
+  it 'should be able to process the data' do
+    result = nil
+    mock_app do
+      post :basic, :params => [ :name, :position => proc{ |v| 'anti-'+v } ] do
+        result = params
+        ''
+      end
+    end
+    post '/basic?' + @jack.to_query
+    assert_equal({ 'name' => @jack['name'], 'position' => 'anti-terrorist' }, result)
+  end
+
+  it 'should pass :with parameters' do
+    result = nil
+    mock_app do
+      post :basic, :with => [:id, :tag], :params => [ :name ] do
+        result = params
+        ''
+      end
+    end
+    post '/basic/24/42?' + @jack.to_query
+    assert_equal({ 'name' => @jack['name'], 'id' => '24', 'tag' => '42' }, result)
+  end
+
+  it 'should understand true or false values' do
+    result = nil
+    mock_app do
+      get :hide, :with => [ :id ], :params => false do
+        result = params
+        ''
+      end
+      get :show, :with => [ :id ], :params => true do
+        result = params
+        ''
+      end
+    end
+    get '/hide/1?' + @jack.to_query
+    assert_equal({"id"=>"1"}, result)
+    get '/show/1?' + @jack.to_query
+    assert_equal({"id"=>"1"}.merge(@jack), result)
+  end
+
+  it 'should be configurable with controller options' do
+    result = nil
+    mock_app do
+      controller :persons, :params => [ :name ] do
+        post :create, :params => [ :name, :position ] do
+          result = params
+          ''
+        end
+        post :update, :with => [ :id ] do
+          result = params
+          ''
+        end
+        post :delete, :params => true do
+          result = params
+          ''
+        end
+        post :destroy, :with => [ :id ], :params => false do
+          result = params
+          ''
+        end
+      end
+      controller :noparam, :params => false do
+        get :index do
+          result = params
+          ''
+        end
+      end
+    end
+    post '/persons/create?' + @jack.to_query
+    assert_equal({ 'name' => @jack['name'], 'position' => 'terrorist' }, result)
+    post '/persons/update/1?name=Chloe+O\'Brian&position=hacker'
+    assert_equal({ 'id' => '1', 'name' => 'Chloe O\'Brian' }, result)
+    post '/persons/delete?' + @jack.to_query
+    assert_equal(@jack, result)
+    post '/persons/destroy/1?' + @jack.to_query
+    assert_equal({"id"=>"1"}, result)
+    get '/noparam?a=1;b=2'
+    assert_equal({}, result)
+  end
+
+  it 'should successfully filter hashes' do
+    result = nil
+    mock_app do
+      post :family, :params => [ :persons => [ :name ] ] do
+        result = params
+        ''
+      end
+    end
+    post '/family?' + @family.to_query
+    assert_equal({"persons" => {"3" => {"name" => @jack["name"]}, "2" => {"name" => @kim["name"]}, "1" => {"name" => @teri["name"]}}}, result)
+  end
+
+  it 'should pass arrays' do
+    result = nil
+    mock_app do
+      post :family, :params => [ :names => [] ] do
+        result = params
+        ''
+      end
+    end
+    post '/family?names[]=Jack&names[]=Kim&names[]=Teri'
+    assert_equal({"names" => %w[Jack Kim Teri]}, result)
+  end
+end

padrino-core/test/test_reloader_simple.rb

@@ -84,7 +84,7 @@
       assert_equal 2, @app.filters[:after].size # app + content-type + padrino-flash
       assert_equal 0, @app.middleware.size
       assert_equal 4, @app.routes.size # GET+HEAD of "/" + GET+HEAD of "/rand" = 4
-      assert_equal 3, @app.extensions.size # [Padrino::ApplicationSetup, Padrino::Routing, Padrino::Flash]
+      assert_equal 4, @app.extensions.size # [Padrino::ApplicationSetup, Padrino::ParamsProtection, Padrino::Routing, Padrino::Flash]
       assert_equal 0, @app.templates.size
       @app.reload!
       get "/rand"
@@ -94,7 +94,7 @@
       assert_equal 2, @app.filters[:after].size
       assert_equal 0, @app.middleware.size
       assert_equal 4, @app.routes.size # GET+HEAD of "/" = 2
-      assert_equal 3, @app.extensions.size # [Padrino::ApplicationSetup, Padrino::Routing, Padrino::Flash]
+      assert_equal 4, @app.extensions.size # [Padrino::ApplicationSetup, Padrino::ParamsProtection, Padrino::Routing, Padrino::Flash]
       assert_equal 0, @app.templates.size
     end
   end