Security Fixes

features/step_definitions/symbol_leak_steps.rb

@@ -0,0 +1,3 @@
+Then /^"(.*?)" shouldn't be a symbol$/ do |sym|
+  Symbol.all_symbols.map(&:to_s).should_not include(sym), 'symbol detected!'
+end

features/step_definitions/web_steps.rb

@@ -27,6 +27,10 @@ def with_scope(locator)
   visit path_to(page_name)
 end
 
+When /^(?:I )POST to "(.*?)" with params "(.*?)"$/ do |url, params|
+  post "#{url}#{params}"
+end
+
 When /^(?:I )press "([^"]*)"$/ do |button|
   click_button(button)
 end

features/support/paths.rb

@@ -6,7 +6,10 @@ module NavigationHelpers
   # step definition in web_steps.rb
   #
   def path_to(page_name)
-    case page_name
+    params = page_name.scan(/with params "(.*?)"/).flatten[0] || ''
+    page_name.sub! /\ with params.*/, ''
+
+    url = case page_name
 
     when /the home\s?page/
       '/'
@@ -16,21 +19,25 @@ def path_to(page_name)
       "/admin/posts/new"
     when /the login page/
       "/admin/login"
+    when /the first post show page/
+      "/admin/posts/1"
+    when /the first post edit page/
+      "/admin/posts/1/edit"
     when /the admin password reset form with reset password token "([^"]*)"/
       "/admin/password/edit?reset_password_token=#{$1}"
 
     # the index page for posts in the root namespace
     # the index page for posts in the user_admin namespace
     when /^the index page for (.*) in the (.*) namespace$/
       if $2 != 'root'
-        send(:"#{$2}_#{$1}_path")
+        send "#{$2}_#{$1}_path"
       else
-        send(:"#{$1}_path")
+        send "#{$1}_path"
       end
 
     # same as above, except defaults to admin namespace
     when /^the index page for (.*)$/
-      send(:"admin_#{$1}_path")
+      send "admin_#{$1}_path"
 
     when /^the last author's posts$/
       admin_user_posts_path(User.last)
@@ -51,12 +58,13 @@ def path_to(page_name)
       begin
         page_name =~ /the (.*) page/
         path_components = $1.split(/\s+/)
-        self.send(path_components.push('path').join('_').to_sym)
+        self.send path_components.push('path').join('_')
       rescue Object => e
         raise "Can't find mapping from \"#{page_name}\" to a path.\n" +
           "Now, go and add a mapping in #{__FILE__}"
       end
     end
+    url + params
   end
 end
 

features/symbol_leak.feature

@@ -0,0 +1,35 @@
+Feature: User input shouldn't be symbolized
+
+  Background:
+    Given a configuration of:
+    """
+      ActiveAdmin.register Post
+    """
+    Given I am logged in
+    And 1 post exists
+
+  Scenario: The dashboard doesn't leak
+    Given I am on the dashboard with params "?really_long_malicious_key0"
+    Then "really_long_malicious_key0" shouldn't be a symbol
+
+  Scenario: The index page doesn't leak
+    Given I am on the index page for posts with params "?really_long_malicious_key1"
+    Then "really_long_malicious_key1" shouldn't be a symbol
+
+  @allow-rescue
+  Scenario: The filter query hash doesn't leak
+    Given I am on the index page for posts with params "?q[really_long_malicious_key2]"
+    Then "really_long_malicious_key2" shouldn't be a symbol
+
+  Scenario: The show page doesn't leak
+    Given I go to the first post show page with params "?really_long_malicious_key3"
+    Then "really_long_malicious_key3" shouldn't be a symbol
+
+  Scenario: The edit page doesn't leak
+    Given I go to the first post edit page with params "?really_long_malicious_key4"
+    Then "really_long_malicious_key4" shouldn't be a symbol
+
+  @allow-rescue
+  Scenario: Batch Actions don't leak
+    Given I POST to "admin/posts/batch_action" with params "?batch_action=really_long_malicious_key5"
+    Then "really_long_malicious_key5" shouldn't be a symbol

lib/active_admin/batch_actions/controller.rb

@@ -5,8 +5,7 @@ module Controller
       # Controller Action that get's called when submitting the batch action form
       def batch_action
         if selected_batch_action
-          selected_ids = params[:collection_selection]
-          selected_ids ||= []
+          selected_ids = params[:collection_selection] || []
           instance_exec selected_ids, &selected_batch_action.block
         else
           raise "Couldn't find batch action \"#{params[:batch_action]}\""
@@ -17,7 +16,7 @@ def batch_action
 
       def selected_batch_action
         return unless params[:batch_action].present?
-        active_admin_config.batch_actions.select { |action| action.sym == params[:batch_action].to_sym }.first
+        active_admin_config.batch_actions.detect { |action| action.sym.to_s == params[:batch_action] }
       end
 
     end

lib/active_admin/view_helpers/download_format_links_helper.rb

@@ -33,9 +33,8 @@ def add_format extension
 
       # TODO: Refactor to new HTML DSL
       def build_download_format_links(formats = self.class.formats)
-        links = formats.collect do |format|
-          link_to format.to_s.upcase, { :format => format}.merge(request.query_parameters.except(:commit, :format))
-        end
+        params = request.query_parameters.except :format, :commit
+        links = formats.map { |format| link_to format.to_s.upcase, params: params, format: format }
         div :class => "download_links" do
           text_node [I18n.t('active_admin.download'), links].flatten.join(" ").html_safe
         end

lib/active_admin/views/components/paginated_collection.rb

@@ -78,7 +78,7 @@ def build_pagination_with_formats(options)
       end
 
       def build_pagination
-        options =  request.query_parameters.except(:commit, :format)
+        options = request.path_parameters
         options[:param_name] = @param_name if @param_name
 
         text_node paginate(collection, options.symbolize_keys)

lib/active_admin/views/components/scopes.rb

@@ -31,11 +31,12 @@ def build(scopes, options = {})
 
       def build_scope(scope, options)
         li :class => classes_for_scope(scope) do
-          scope_name = I18n.t("active_admin.scopes.#{scope.id}", :default => scope.name)
+          scope_name = I18n.t "active_admin.scopes.#{scope.id}", default: scope.name
+          params     = request.query_parameters.except :page, :scope, :commit, :format
 
-          a :href => url_for(params.merge(:scope => scope.id, :page => 1)), :class => "table_tools_button" do
+          a href: url_for(scope: scope.id, params: params), class: 'table_tools_button' do
             text_node scope_name
-            span :class => 'count' do
+            span class: 'count' do
               "(#{get_scope_count(scope)})"
             end if options[:scope_count] && scope.show_count
           end

lib/active_admin/views/components/table_for.rb

@@ -60,19 +60,20 @@ def build_table_head
       end
 
       def build_table_header(col)
-        classes = Arbre::HTML::ClassList.new
+        classes  = Arbre::HTML::ClassList.new
         sort_key = sortable? && col.sortable? && col.sort_key
+        params   = request.query_parameters.except :page, :order, :commit, :format
 
         classes << 'sortable'                         if sort_key
         classes << "sorted-#{current_sort[1]}"        if sort_key && current_sort[0] == sort_key
         classes << col.html_class
 
         if sort_key
-          th :class => classes do
-            link_to(col.pretty_title, params.merge(:order => "#{sort_key}_#{order_for_sort_key(sort_key)}").except(:page))
+          th class: classes do
+            link_to col.pretty_title, params: params, order: "#{sort_key}_#{order_for_sort_key(sort_key)}"
           end
         else
-          th(col.pretty_title, :class => classes)
+          th col.pretty_title, class: classes
         end
       end
 

spec/unit/views/components/paginated_collection_spec.rb

@@ -10,8 +10,8 @@
 
     let(:view) do
       view = mock_action_view
-      view.request.stub(:query_parameters).and_return({:controller => 'admin/posts', :action => 'index', :page => '1'})
-      view.controller.params = {:controller => 'admin/posts', :action => 'index'}
+      view.request.stub(:query_parameters).and_return page: '1'
+      view.request.stub(:path_parameters).and_return  controller: 'admin/posts', action: 'index'
       view
     end
 
@@ -49,7 +49,7 @@ def paginated_collection(*args)
 
     context "when specifying :param_name option" do
       let(:collection) do
-        posts = 10.times.inject([]) {|m, _| m << Post.new }
+        posts = 10.times.map{ Post.new }
         Kaminari.paginate_array(posts).page(1).per(5)
       end
 
@@ -62,7 +62,7 @@ def paginated_collection(*args)
 
     context "when specifying :download_links => false option" do
       let(:collection) do
-        posts = 10.times.inject([]) {|m, _| m << Post.new }
+        posts = 10.times.map{ Post.new }
         Kaminari.paginate_array(posts).page(1).per(5)
       end
 
@@ -188,8 +188,7 @@ def paginated_collection(*args)
 
     context "when viewing the last page of a collection that has multiple pages" do
       let(:collection) do
-        posts = [Post.new] * 81
-        Kaminari.paginate_array(posts).page(3).per(30)
+        Kaminari.paginate_array([Post.new] * 81).page(3).per(30)
       end
 
       let(:pagination) { paginated_collection(collection) }
@@ -200,48 +199,29 @@ def paginated_collection(*args)
       end
     end
 
-    context "when having the param :pagination_total set to true " do
-      let(:view) do
-        view = mock_action_view
-        view.request.stub(:query_parameters).and_return({:controller => 'admin/stores', :action => 'index', :page => '1'})
-        view.controller.params = {:controller => 'admin/stores', :action => 'index'}
-        view
-      end
-
+    context "with :pagination_total" do
       let(:collection) do
-        stores = [Store.new] * 1000
-        Kaminari.paginate_array(stores).page(1).per(30)
-      end
-
-      let(:pagination) { paginated_collection(collection, :pagination_total => true) }
-
-      it "should show the total item counts" do
-        info = pagination.find_by_class('pagination_information').first.content.gsub('&nbsp;',' ')
-        info.should eq "Displaying Bookstores <b>1 - 30</b> of <b>1000</b> in total"
+        Kaminari.paginate_array([Post.new] * 256).page(1).per(30)
       end
 
-    end
-
-    context "when having the param :pagination_total set to false " do
-      let(:view) do
-        view = mock_action_view
-        view.request.stub(:query_parameters).and_return({:controller => 'admin/stores', :action => 'index', :page => '1'})
-        view.controller.params = {:controller => 'admin/stores', :action => 'index'}
-        view
-      end
+      describe "set to false" do
+        let(:pagination) { paginated_collection(collection, pagination_total: false) }
 
-      let(:collection) do
-        stores = [Store.new] * 1000
-        Kaminari.paginate_array(stores).page(1).per(30)
+        it "should not show the total item counts" do
+          info = pagination.find_by_class('pagination_information').first.content.gsub('&nbsp;',' ')
+          info.should eq "Displaying posts <b>1 - 30</b>"
+        end
       end
 
-      let(:pagination) { paginated_collection(collection, :pagination_total => false) }
+      describe "set to true" do
+        let(:pagination) { paginated_collection(collection, pagination_total: true) }
 
-      it "should not show the total item counts" do
-        info = pagination.find_by_class('pagination_information').first.content.gsub('&nbsp;',' ')
-        info.should eq "Displaying Bookstores <b>1 - 30</b>"
+        it "should show the total item counts" do
+          info = pagination.find_by_class('pagination_information').first.content.gsub('&nbsp;',' ')
+          info.should eq "Displaying posts <b>1 - 30</b> of <b>256</b> in total"
+        end
       end
-
     end
+
   end
 end