From 4206f3033f4b216c9f708bbb82beebc833fe052b Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 17 Jul 2017 10:54:30 +0200 Subject: [PATCH 1/2] Sanitize remarkable_type in clipboard controller Although inside the password protected admin namespace we should sanitize the `remarkable_type` param in `admin/clipboard_controller`. --- .../alchemy/admin/clipboard_controller.rb | 27 ++++++++++++++----- .../admin/clipboard_controller_spec.rb | 17 ++++++++++++ 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/app/controllers/alchemy/admin/clipboard_controller.rb b/app/controllers/alchemy/admin/clipboard_controller.rb index b4efc155b0..1c81b0ac49 100644 --- a/app/controllers/alchemy/admin/clipboard_controller.rb +++ b/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 diff --git a/spec/controllers/alchemy/admin/clipboard_controller_spec.rb b/spec/controllers/alchemy/admin/clipboard_controller_spec.rb index 047b458b3c..2bfc83ab08 100644 --- a/spec/controllers/alchemy/admin/clipboard_controller_spec.rb +++ b/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) From b1585574915ad350cfe208ad3798ecc3cea8d4fc Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 17 Jul 2017 11:11:41 +0200 Subject: [PATCH 2/2] Add a brakeman config file And add two false positives --- config/brakeman.ignore | 46 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 config/brakeman.ignore diff --git a/config/brakeman.ignore b/config/brakeman.ignore new file mode 100644 index 0000000000..b5fe61f2e3 --- /dev/null +++ b/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" +}