Fix IDOR vulnerabilities in Kits, BroadcastAnnouncements, and Distributions controllers

Scope all find-by-id queries through current_organization to prevent
cross-organization data access (CWE-639).

KitsController: deactivate, reactivate, allocations, allocate
BroadcastAnnouncementsController: set_broadcast_announcement (edit/update/destroy)
DistributionsController: print, show, edit, update, destroy

Closes #5509 (partial - 3 of 5 controllers)

Author: costajohnt

app/controllers/broadcast_announcements_controller.rb

@@ -45,7 +45,7 @@ def destroy
   private
 
   def set_broadcast_announcement
-    @broadcast_announcement = BroadcastAnnouncement.find(params[:id])
+    @broadcast_announcement = BroadcastAnnouncement.where(organization_id: current_organization.id).find(params[:id])
   end
 
   def broadcast_announcement_params

app/controllers/distributions_controller.rb

@@ -14,7 +14,7 @@ class DistributionsController < ApplicationController
   skip_before_action :require_organization, only: %i(calendar)
 
   def print
-    @distribution = Distribution.find(params[:id])
+    @distribution = current_organization.distributions.find(params[:id])
     respond_to do |format|
       format.any do
         pdf = DistributionPdf.new(current_organization, @distribution)
@@ -27,7 +27,8 @@ def print
   end
 
   def destroy
-    service = DistributionDestroyService.new(params[:id])
+    distribution = current_organization.distributions.find(params[:id])
+    service = DistributionDestroyService.new(distribution.id)
     result = service.call
 
     if result.success?
@@ -167,7 +168,7 @@ def new
   end
 
   def show
-    @distribution = Distribution.includes(:storage_location, line_items: :item).find(params[:id])
+    @distribution = current_organization.distributions.includes(:storage_location, line_items: :item).find(params[:id])
     @line_items = @distribution.line_items
 
     @total_quantity = @distribution.total_quantity
@@ -178,7 +179,7 @@ def show
   end
 
   def edit
-    @distribution = Distribution.includes(:line_items).includes(:storage_location).find(params[:id])
+    @distribution = current_organization.distributions.includes(:line_items).includes(:storage_location).find(params[:id])
     @distribution.initialize_request_items
     if (!@distribution.complete? && @distribution.future?) ||
         current_user.has_cached_role?(Role::ORG_ADMIN, current_organization)
@@ -201,7 +202,7 @@ def edit
   end
 
   def update
-    @distribution = Distribution.includes(:line_items).includes(:storage_location).find(params[:id])
+    @distribution = current_organization.distributions.includes(:line_items).includes(:storage_location).find(params[:id])
     result = DistributionUpdateService.new(@distribution, distribution_params).call
 
     if result.success?

app/controllers/kits_controller.rb

@@ -44,13 +44,13 @@ def create
   end
 
   def deactivate
-    @kit = Kit.find(params[:id])
+    @kit = current_organization.kits.find(params[:id])
     @kit.deactivate
     redirect_back(fallback_location: dashboard_path, notice: "Kit has been deactivated!")
   end
 
   def reactivate
-    @kit = Kit.find(params[:id])
+    @kit = current_organization.kits.find(params[:id])
     if @kit.can_reactivate?
       @kit.reactivate
       redirect_back(fallback_location: dashboard_path, notice: "Kit has been reactivated!")
@@ -60,15 +60,15 @@ def reactivate
   end
 
   def allocations
-    @kit = Kit.find(params[:id])
+    @kit = current_organization.kits.find(params[:id])
     @storage_locations = current_organization.storage_locations.active
     @inventory = View::Inventory.new(current_organization.id)
 
     load_form_collections
   end
 
   def allocate
-    @kit = Kit.find(params[:id])
+    @kit = current_organization.kits.find(params[:id])
     @storage_location = current_organization.storage_locations.active.find(kit_adjustment_params[:storage_location_id])
     @change_by = kit_adjustment_params[:change_by].to_i
     begin

spec/requests/broadcast_announcements_spec.rb

@@ -107,4 +107,36 @@
       expect(response).to have_http_status(:redirect)
     end
   end
+
+  context "when accessing an announcement from another organization" do
+    let(:other_organization) { create(:organization) }
+    let(:other_announcement) {
+      BroadcastAnnouncement.create!(
+        expiry: Time.zone.today,
+        link: "http://example.com",
+        message: "other org announcement",
+        user_id: create(:user, organization: other_organization).id,
+        organization_id: other_organization.id
+      )
+    }
+
+    it "does not allow editing an announcement from another organization" do
+      get edit_broadcast_announcement_url(other_announcement)
+      expect(response.status).to eq(404)
+    end
+
+    it "does not allow updating an announcement from another organization" do
+      patch broadcast_announcement_url(other_announcement), params: {broadcast_announcement: {message: "hacked"}}
+      expect(response.status).to eq(404)
+      expect(other_announcement.reload.message).to eq("other org announcement")
+    end
+
+    it "does not allow destroying an announcement from another organization" do
+      other_announcement # ensure created
+      expect {
+        delete broadcast_announcement_url(other_announcement)
+      }.not_to change(BroadcastAnnouncement, :count)
+      expect(response.status).to eq(404)
+    end
+  end
 end

spec/requests/distributions_requests_spec.rb

@@ -55,6 +55,14 @@
         end
       end
 
+      context "when accessing a distribution from another organization" do
+        it "returns 404" do
+          other_distribution = create(:distribution, organization: create(:organization))
+          get print_distribution_path(id: other_distribution.id)
+          expect(response.status).to eq(404)
+        end
+      end
+
       include_examples "restricts access to organization users/admins"
     end
 
@@ -473,6 +481,14 @@
         end
       end
 
+      context "when accessing a distribution from another organization" do
+        it "returns 404" do
+          other_distribution = create(:distribution, organization: create(:organization))
+          get distribution_path(id: other_distribution.id)
+          expect(response.status).to eq(404)
+        end
+      end
+
       include_examples "restricts access to organization users/admins"
     end
 
@@ -670,6 +686,14 @@
         end
       end
 
+      context "when accessing a distribution from another organization" do
+        it "returns 404" do
+          other_distribution = create(:distribution, organization: create(:organization))
+          patch distribution_path(id: other_distribution.id), params: {distribution: {comment: "hacked"}}
+          expect(response.status).to eq(404)
+        end
+      end
+
       include_examples "restricts access to organization users/admins"
     end
 
@@ -890,6 +914,14 @@
         end
       end
 
+      context "when accessing a distribution from another organization" do
+        it "returns 404" do
+          other_distribution = create(:distribution, organization: create(:organization))
+          get edit_distribution_path(id: other_distribution.id)
+          expect(response.status).to eq(404)
+        end
+      end
+
       include_examples "restricts access to organization users/admins"
     end
 
@@ -927,6 +959,16 @@
         expect(flash[:error]).to eq("We can't delete distributions entered before #{1.day.ago.to_date}.")
       end
 
+      context "when accessing a distribution from another organization" do
+        it "returns 404" do
+          other_distribution = create(:distribution, organization: create(:organization))
+          expect {
+            delete distribution_path(id: other_distribution.id)
+          }.not_to change { Distribution.count }
+          expect(response.status).to eq(404)
+        end
+      end
+
       include_examples "restricts access to organization users/admins"
     end
   end

spec/requests/kit_requests_spec.rb

@@ -100,5 +100,32 @@
         expect(flash[:notice]).to eq("Kit has been reactivated!")
       end
     end
+
+    context "when accessing a kit from another organization" do
+      let(:other_organization) { create(:organization) }
+      let(:other_kit) { create_kit(organization: other_organization) }
+
+      it "does not allow deactivating a kit from another organization" do
+        put deactivate_kit_url(other_kit)
+        expect(response.status).to eq(404)
+      end
+
+      it "does not allow reactivating a kit from another organization" do
+        other_kit.deactivate
+        put reactivate_kit_url(other_kit)
+        expect(response.status).to eq(404)
+      end
+
+      it "does not allow viewing allocations for a kit from another organization" do
+        get allocations_kit_url(other_kit)
+        expect(response.status).to eq(404)
+      end
+
+      it "does not allow allocating for a kit from another organization" do
+        storage_location = create(:storage_location, organization: organization)
+        post allocate_kit_url(other_kit), params: {kit_adjustment: {storage_location_id: storage_location.id, change_by: 5}}
+        expect(response.status).to eq(404)
+      end
+    end
   end
 end