Harden bulk signature actions against attack

Use a per-session secret key to encrypt and sign a list of ids presented
to the moderator and ensure that when the bulk action is requested the
selected set of ids is contained within the signed list of ids and that
the list of ids hasn't been tampered with.

This provides an additional layer of security should any vulnerabilities
be discovered within the authlogic gem.

Also limit the number of selected ids accepted by the bulk action to a
maximum of fifty. However, this is somewhat redundant given the fact
that there will be a maximum of fifty ids in the signed list of ids.

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

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

app/views/admin/signatures/_signature.html.erb

@@ -1,7 +1,7 @@
 <tr>
   <td class="petition-id">
     <label>
-      <%= check_box_tag 'ids', signature.id, false, disabled: signature.creator? %> <%= signature.petition.id %>
+      <%= check_box_tag 'id', signature.id, false, disabled: signature.creator? %> <%= signature.petition.id %>
     </label>
   </td>
   <td class="petition-action"><%= link_to signature.petition.action, admin_petition_path(signature.petition) %></td>

app/views/admin/signatures/index.html.erb

@@ -37,26 +37,31 @@
   <%= will_paginate @signatures %>
 
   <div class="inline-actions">
+    <% 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 %>
   </div>
 
   <script>
     $(document).ready(function() {
       var $list = $('.signature-list');
-      var $checkboxes = $list.find('input[name="ids"]');
+      var $checkboxes = $list.find('input[name="id"]');
       var $selectAll = $list.find('input[name="select_all"]');
 
       var selectedIds = function() {
@@ -116,7 +121,7 @@
       });
 
       $('.inline-actions form').on('submit', function() {
-        $(this).find('input[name="ids"]').val(selectedIds());
+        $(this).find('input[name="selected_ids"]').val(selectedIds());
       });
     });
   </script>

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')

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: "[email protected]"
+          post :bulk_validate, selected_ids: signature.id, all_ids: signature_ids, q: "[email protected]"
         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: "[email protected]"
+          post :bulk_validate, selected_ids: signature.id, all_ids: signature_ids, q: "[email protected]"
         end
 
         it "redirects to the search page" do
@@ -198,14 +205,38 @@
           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: "[email protected]"
+          }.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: "[email protected]"
+          }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/)
+        end
+      end
     end
 
     describe "POST /admin/signatures/invalidate" do
       context "when the signature is invalidated" do
         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: "[email protected]"
+          post :bulk_invalidate, selected_ids: signature.id, all_ids: signature_ids, q: "[email protected]"
         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: "[email protected]"
+          post :bulk_invalidate, selected_ids: signature.id, all_ids: signature_ids, q: "[email protected]"
         end
 
         it "redirects to the search page" do
@@ -235,14 +266,38 @@
           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: "[email protected]"
+          }.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: "[email protected]"
+          }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/)
+        end
+      end
     end
 
     describe "DELETE /admin/signatures" do
       context "when the signature is destroyed" do
         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: "[email protected]"
+          delete :bulk_destroy, selected_ids: signature.id, all_ids: signature_ids, q: "[email protected]"
         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: "[email protected]"
+          delete :bulk_destroy, selected_ids: signature.id, all_ids: signature_ids, q: "[email protected]"
         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: "[email protected]"
+          }.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: "[email protected]"
+          }.to raise_error(BulkVerification::InvalidBulkRequest, /Invalid bulk request - \d+ not present in \[\d+\]/)
+        end
+      end
     end
   end
 end