Merge pull request #1280 from AlchemyCMS/sanitize-remarkable-type

Sanitize remarkable_type in clipboard controller

app/controllers/alchemy/admin/clipboard_controller.rb

@@ -1,6 +1,10 @@
+# frozen_string_literal: true
+
 module Alchemy
   module Admin
     class ClipboardController < Alchemy::Admin::BaseController
+      REMARKABLE_TYPES = %w(elements pages)
+
       authorize_resource class: :alchemy_admin_clipboard
       before_action :set_clipboard
 
@@ -12,10 +16,10 @@ def index
       end
 
       def insert
-        @item = model_class.find(params[:remarkable_id])
-        unless @clipboard.detect { |item| item['id'] == params[:remarkable_id] }
+        @item = model_class.find(remarkable_params[:remarkable_id])
+        unless @clipboard.detect { |item| item['id'] == remarkable_params[:remarkable_id] }
           @clipboard << {
-            'id' => params[:remarkable_id],
+            'id' => remarkable_params[:remarkable_id],
             'action' => params[:remove] ? 'cut' : 'copy'
           }
         end
@@ -25,8 +29,8 @@ def insert
       end
 
       def remove
-        @item = model_class.find(params[:remarkable_id])
-        @clipboard.delete_if { |item| item['id'] == params[:remarkable_id] }
+        @item = model_class.find(remarkable_params[:remarkable_id])
+        @clipboard.delete_if { |item| item['id'] == remarkable_params[:remarkable_id] }
         respond_to do |format|
           format.js
         end
@@ -39,11 +43,20 @@ def clear
       private
 
       def set_clipboard
-        @clipboard = get_clipboard(params[:remarkable_type])
+        @clipboard = get_clipboard(remarkable_type)
       end
 
       def model_class
-        "alchemy/#{params[:remarkable_type]}".classify.constantize
+        raise ActionController::BadRequest unless remarkable_type
+        "alchemy/#{remarkable_type}".classify.constantize
+      end
+
+      def remarkable_params
+        params.permit(:remarkable_type, :remarkable_id)
+      end
+
+      def remarkable_type
+        remarkable_params.keep_if { |_k, type| type.in? REMARKABLE_TYPES }[:remarkable_type]
       end
     end
   end

config/brakeman.ignore

@@ -0,0 +1,46 @@
+{
+  "ignored_warnings": [
+    {
+      "warning_type": "Mass Assignment",
+      "warning_code": 70,
+      "fingerprint": "1dd8f69d9b1bdd4017212f38098f03d2ecb2db06269fb940090f209eee7570c6",
+      "check_name": "MassAssignment",
+      "message": "Parameters should be whitelisted for mass assignment",
+      "file": "app/controllers/alchemy/admin/resources_controller.rb",
+      "line": 124,
+      "link": "http://brakemanscanner.org/docs/warning_types/mass_assignment/",
+      "code": "params.require(resource_handler.namespaced_resource_name).permit!",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "Alchemy::Admin::ResourcesController",
+        "method": "resource_params"
+      },
+      "user_input": null,
+      "confidence": "Medium",
+      "note": "Because we actually can't know all attributes each inheriting controller supports, we permit all resource model params. It is adviced that all inheriting controllers implement this method and provide its own set of permitted attributes. As this all happens inside the password protected /admin namespace this can be considered a false positive."
+    },
+    {
+      "warning_type": "Mass Assignment",
+      "warning_code": 70,
+      "fingerprint": "4b4dc24a6f5251bc1a6851597dfcee39608a2932eb7f81a4a241c00fca8a3043",
+      "check_name": "MassAssignment",
+      "message": "Parameters should be whitelisted for mass assignment",
+      "file": "app/controllers/alchemy/admin/elements_controller.rb",
+      "line": 166,
+      "link": "http://brakemanscanner.org/docs/warning_types/mass_assignment/",
+      "code": "params.fetch(:contents, {}).permit!",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "Alchemy::Admin::ElementsController",
+        "method": "contents_params"
+      },
+      "user_input": null,
+      "confidence": "Medium",
+      "note": "`Alchemy::Content` is a polymorphic association of any kind of model extending `Alchemy::Essence`. Since we can't know the attributes of all potential essences we need to permit all attributes. As this all happens inside the password protected /admin namespace this can be considered a false positive."
+    }
+  ],
+  "updated": "2017-07-17 11:09:19 +0200",
+  "brakeman_version": "3.7.0"
+}

spec/controllers/alchemy/admin/clipboard_controller_spec.rb

@@ -11,6 +11,23 @@ module Alchemy
       session[:alchemy_clipboard] = {}
     end
 
+    describe '#index' do
+      context 'with `remarkable_type` being an allowed type' do
+        it 'is successful' do
+          alchemy_get :index, {remarkable_type: 'elements'}
+          expect(response).to be_success
+        end
+      end
+
+      context 'with `remarkable_type` not an allowed type' do
+        it 'raises 400 Bad Request' do
+          expect {
+            alchemy_get :index, {remarkable_type: 'evil'}
+          }.to raise_error(ActionController::BadRequest)
+        end
+      end
+    end
+
     context 'for elements' do
       before do
         expect(Element).to receive(:find).and_return(element)