Merge pull request #638 from alphagov/apply-constraints-bulk-signature-ops

Harden bulk signature actions against attack

Author: pixeltrix

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: "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,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: "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
       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: "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,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: "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
       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: "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