From 91fecf4acfc919140a00ada51a7c1dfdc1104101 Mon Sep 17 00:00:00 2001 From: John Costa Date: Thu, 12 Mar 2026 21:24:43 -0700 Subject: [PATCH 1/2] 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) --- .../broadcast_announcements_controller.rb | 2 +- app/controllers/distributions_controller.rb | 11 ++--- app/controllers/kits_controller.rb | 8 ++-- spec/requests/broadcast_announcements_spec.rb | 32 ++++++++++++++ spec/requests/distributions_requests_spec.rb | 42 +++++++++++++++++++ spec/requests/kit_requests_spec.rb | 27 ++++++++++++ 6 files changed, 112 insertions(+), 10 deletions(-) diff --git a/app/controllers/broadcast_announcements_controller.rb b/app/controllers/broadcast_announcements_controller.rb index bf7fc69c16..e68cabfce6 100644 --- a/app/controllers/broadcast_announcements_controller.rb +++ b/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 diff --git a/app/controllers/distributions_controller.rb b/app/controllers/distributions_controller.rb index fac24c946f..da7f0395eb 100644 --- a/app/controllers/distributions_controller.rb +++ b/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? diff --git a/app/controllers/kits_controller.rb b/app/controllers/kits_controller.rb index d1d43918b0..e0257ffe7a 100644 --- a/app/controllers/kits_controller.rb +++ b/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,7 +60,7 @@ 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) @@ -68,7 +68,7 @@ def allocations 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 diff --git a/spec/requests/broadcast_announcements_spec.rb b/spec/requests/broadcast_announcements_spec.rb index 9ed0a3e746..391087b272 100644 --- a/spec/requests/broadcast_announcements_spec.rb +++ b/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 diff --git a/spec/requests/distributions_requests_spec.rb b/spec/requests/distributions_requests_spec.rb index 6da8da99bd..bd0c0ea1c0 100644 --- a/spec/requests/distributions_requests_spec.rb +++ b/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 diff --git a/spec/requests/kit_requests_spec.rb b/spec/requests/kit_requests_spec.rb index b2ab7da695..8683568f82 100644 --- a/spec/requests/kit_requests_spec.rb +++ b/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 From ea7552c5c54f71cc8b533e2c0505b42c859b3bc0 Mon Sep 17 00:00:00 2001 From: John Costa Date: Thu, 12 Mar 2026 21:33:13 -0700 Subject: [PATCH 2/2] Add data-integrity assertion to distribution update cross-org test Verify the distribution record is unchanged after a rejected cross-org update attempt, matching the pattern used in the broadcast announcements spec. --- spec/requests/distributions_requests_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/requests/distributions_requests_spec.rb b/spec/requests/distributions_requests_spec.rb index bd0c0ea1c0..d117bd6dda 100644 --- a/spec/requests/distributions_requests_spec.rb +++ b/spec/requests/distributions_requests_spec.rb @@ -689,8 +689,10 @@ context "when accessing a distribution from another organization" do it "returns 404" do other_distribution = create(:distribution, organization: create(:organization)) + original_comment = other_distribution.comment patch distribution_path(id: other_distribution.id), params: {distribution: {comment: "hacked"}} expect(response.status).to eq(404) + expect(other_distribution.reload.comment).to eq(original_comment) end end