diff --git a/app/controllers/admin/signatures_controller.rb b/app/controllers/admin/signatures_controller.rb index 1441f9388..79df98b25 100644 --- a/app/controllers/admin/signatures_controller.rb +++ b/app/controllers/admin/signatures_controller.rb @@ -1,4 +1,6 @@ class Admin::SignaturesController < Admin::AdminController + include BulkVerification + before_action :fetch_signature, except: [:index, :bulk_validate, :bulk_invalidate, :bulk_destroy] before_action :fetch_signatures, only: [:index] @@ -10,7 +12,7 @@ def index def bulk_validate begin - Signature.validate!(signature_ids) + Signature.validate!(selected_ids) redirect_to admin_signatures_url(q: params[:q]), notice: :signatures_validated rescue StandardError => e Appsignal.send_exception e @@ -30,7 +32,7 @@ def validate def bulk_invalidate begin - Signature.invalidate!(signature_ids) + Signature.invalidate!(selected_ids) redirect_to admin_signatures_url(q: params[:q]), notice: :signatures_invalidated rescue StandardError => e Appsignal.send_exception e @@ -50,7 +52,7 @@ def invalidate def bulk_destroy begin - Signature.destroy!(signature_ids) + Signature.destroy!(selected_ids) redirect_to admin_signatures_url(q: params[:q]), notice: :signatures_deleted rescue StandardError => e Appsignal.send_exception e @@ -75,8 +77,4 @@ def fetch_signatures def fetch_signature @signature = Signature.find(params[:id]) end - - def signature_ids - params[:ids].to_s.split(",").map(&:to_i).reject(&:zero?) - end end diff --git a/app/controllers/concerns/bulk_verification.rb b/app/controllers/concerns/bulk_verification.rb new file mode 100644 index 000000000..0d812efc3 --- /dev/null +++ b/app/controllers/concerns/bulk_verification.rb @@ -0,0 +1,49 @@ +module BulkVerification + extend ActiveSupport::Concern + + class InvalidBulkRequest < RuntimeError; end + + included do + before_action :verify_bulk_request, if: :bulk_request? + + helper_method :bulk_verifier + + rescue_from ActiveSupport::MessageVerifier::InvalidSignature do + raise BulkVerification::InvalidBulkRequest, "Invalid bulk request for #{selected_ids.inspect}" + end + end + + private + + def bulk_request? + action_name =~ /\Abulk_/ + end + + def bulk_verification_token + session[:_bulk_verification_token] ||= SecureRandom.base64(32) + end + + def bulk_verifier + @_bulk_verifer ||= ActiveSupport::MessageVerifier.new(bulk_verification_token, serializer: JSON) + end + + def selected_ids + @_selected_ids ||= params[:selected_ids].to_s.split(",").map(&:to_i).reject(&:zero?).take(50) + end + + def all_ids + @_all_ids ||= bulk_verifier.verify(params[:all_ids]) + end + + def verify_bulk_request + selected_ids.all?(&method(:verify_bulk_request_id)) + end + + def verify_bulk_request_id(id) + all_ids.include?(id) || raise_bad_request(id) + end + + def raise_bad_request(id) + raise BulkVerification::InvalidBulkRequest, "Invalid bulk request - #{id} not present in #{all_ids.inspect}" + end +end diff --git a/app/views/admin/signatures/_signature.html.erb b/app/views/admin/signatures/_signature.html.erb index 1ef2a2288..914ffe873 100644 --- a/app/views/admin/signatures/_signature.html.erb +++ b/app/views/admin/signatures/_signature.html.erb @@ -1,7 +1,7 @@ <%= link_to signature.petition.action, admin_petition_path(signature.petition) %> diff --git a/app/views/admin/signatures/index.html.erb b/app/views/admin/signatures/index.html.erb index 250186322..d519e344f 100644 --- a/app/views/admin/signatures/index.html.erb +++ b/app/views/admin/signatures/index.html.erb @@ -37,18 +37,23 @@ <%= will_paginate @signatures %>
+ <% signature_ids = bulk_verifier.generate(@signatures.map(&:id)) %> + <%= form_tag validate_admin_signatures_path(q: params[:q]), method: :post, class: 'action validate-action' do %> - <%= hidden_field_tag 'ids', '' %> + <%= hidden_field_tag 'selected_ids', '' %> + <%= hidden_field_tag 'all_ids', signature_ids %> <%= submit_tag 'Validate', name: nil, class: 'button', disabled: true, data: { confirm: 'Validate selected signatures?' } %> <% end %> <%= form_tag invalidate_admin_signatures_path(q: params[:q]), method: :post, class: 'action invalidate-action' do %> - <%= hidden_field_tag 'ids', '' %> + <%= hidden_field_tag 'selected_ids', '' %> + <%= hidden_field_tag 'all_ids', signature_ids %> <%= submit_tag 'Invalidate', name: nil, class: 'button', disabled: true, data: { confirm: 'Invalidate selected signatures?' } %> <% end %> <%= form_tag admin_signatures_path(q: params[:q]), method: :delete, class: 'action delete-action' do %> - <%= hidden_field_tag 'ids', '' %> + <%= hidden_field_tag 'selected_ids', '' %> + <%= hidden_field_tag 'all_ids', signature_ids %> <%= submit_tag 'Delete', name: nil, class: 'button-warning', disabled: true, data: { confirm: 'Delete selected signatures?' } %> <% end %>
@@ -56,7 +61,7 @@ diff --git a/config/application.rb b/config/application.rb index 639373be2..7e008b902 100644 --- a/config/application.rb +++ b/config/application.rb @@ -38,9 +38,10 @@ class Application < Rails::Application # Remove the error wrapper from around the form element config.action_view.field_error_proc = -> (html_tag, instance) { html_tag } - # Add 503 Service Unavailable to the rescue response + # Add additional exceptions to the rescue responses config.action_dispatch.rescue_responses.merge!( - 'Site::ServiceUnavailable' => :service_unavailable + 'Site::ServiceUnavailable' => :service_unavailable, + 'BulkVerification::InvalidBulkRequest' => :bad_request ) config.action_dispatch.default_headers.merge!('X-UA-Compatible' => 'IE=edge') diff --git a/spec/controllers/admin/signatures_controller_spec.rb b/spec/controllers/admin/signatures_controller_spec.rb index e7e001e8a..4cc9b4797 100644 --- a/spec/controllers/admin/signatures_controller_spec.rb +++ b/spec/controllers/admin/signatures_controller_spec.rb @@ -41,7 +41,14 @@ context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } - before { login_as(user) } + let(:token) { SecureRandom.base64(32) } + let(:verifier) { ActiveSupport::MessageVerifier.new(token, serializer: JSON) } + let(:signature_ids) { verifier.generate([signature.id]) } + + before do + login_as(user) + session[:_bulk_verification_token] = token + end describe "GET /admin/signatures" do before { get :index, q: "Alice" } @@ -168,7 +175,7 @@ before do expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:validate!).and_return(true) - post :bulk_validate, ids: signature.id, q: "user@example.com" + post :bulk_validate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -187,7 +194,7 @@ expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:validate!).and_raise(exception) expect(Appsignal).to receive(:send_exception).with(exception) - post :bulk_validate, ids: signature.id, q: "user@example.com" + post :bulk_validate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -198,6 +205,30 @@ expect(flash[:alert]).to eq("Signatures could not be validated - please contact support") end end + + context "when the signature ids hmac is missing" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_validate, selected_ids: signature.id, all_ids: "", q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request for \[\d+\]/) + end + end + + context "when the selected_ids param contains an invalid id" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_validate, selected_ids: "1,2", all_ids: signature_ids, q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/) + end + end end describe "POST /admin/signatures/invalidate" do @@ -205,7 +236,7 @@ before do expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:invalidate!).and_return(true) - post :bulk_invalidate, ids: signature.id, q: "user@example.com" + post :bulk_invalidate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -224,7 +255,7 @@ expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:invalidate!).and_raise(exception) expect(Appsignal).to receive(:send_exception).with(exception) - post :bulk_invalidate, ids: signature.id, q: "user@example.com" + post :bulk_invalidate, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -235,6 +266,30 @@ expect(flash[:alert]).to eq("Signatures could not be invalidated - please contact support") end end + + context "when the signature ids hmac is missing" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_invalidate, selected_ids: signature.id, all_ids: "", q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request for \[\d+\]/) + end + end + + context "when the selected_ids param contains an invalid id" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_invalidate, selected_ids: "1,2", all_ids: signature_ids, q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/) + end + end end describe "DELETE /admin/signatures" do @@ -242,7 +297,7 @@ before do expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:destroy!).and_return(true) - delete :bulk_destroy, ids: signature.id, q: "user@example.com" + delete :bulk_destroy, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -261,7 +316,7 @@ expect(Signature).to receive(:find).with([signature.id]).and_return([signature]) expect(signature).to receive(:destroy!).and_raise(exception) expect(Appsignal).to receive(:send_exception).with(exception) - delete :bulk_destroy, ids: signature.id, q: "user@example.com" + delete :bulk_destroy, selected_ids: signature.id, all_ids: signature_ids, q: "user@example.com" end it "redirects to the search page" do @@ -272,6 +327,30 @@ expect(flash[:alert]).to eq("Signatures could not be removed - please contact support") end end + + context "when the signature ids hmac is missing" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_destroy, selected_ids: signature.id, all_ids: "", q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request for \[\d+\]/) + end + end + + context "when the selected_ids param contains an invalid id" do + before do + expect(Signature).not_to receive(:find) + end + + it "returns a 400 Bad Request" do + expect { + delete :bulk_destroy, selected_ids: "1,2", all_ids: signature_ids, q: "user@example.com" + }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/) + end + end end end end