From cfadfeff87ae37523744a458540edccf610de3c5 Mon Sep 17 00:00:00 2001 From: Raul Riera Date: Tue, 18 Aug 2026 10:52:22 -0400 Subject: [PATCH] fix(notes): scope noteable lookup to the current practice find_noteable constantized any *_id param and looked it up unscoped, so a logged-in user could attach or delete notes on another practice's patients (and load arbitrary classes from crafted param names). Notes are only nested under patients in routes, so resolve the patient through the practice scope instead. --- app/controllers/notes_controller.rb | 5 +---- test/functional/notes_controller_test.rb | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index c6604659..bd37b312 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -32,10 +32,7 @@ def destroy private def find_noteable - params.each do |name, value| - return Regexp.last_match(1).classify.constantize.find(value) if name =~ /(.+)_id$/ - end - nil + Patient.with_practice(current_user.practice_id).find(params[:patient_id]) end def note_params diff --git a/test/functional/notes_controller_test.rb b/test/functional/notes_controller_test.rb index bb6896ab..58aa265b 100644 --- a/test/functional/notes_controller_test.rb +++ b/test/functional/notes_controller_test.rb @@ -33,6 +33,24 @@ class NotesControllerTest < ActionController::TestCase end end + test 'should not create a note for a patient in another practice' do + assert_no_difference 'Note.count' do + assert_raises(ActiveRecord::RecordNotFound) do + post :create, params: { patient_id: patients(:three).id, note: { notes: 'Cross-practice note' }, format: :js } + end + end + end + + test 'should not destroy a note for a patient in another practice' do + foreign_note = Note.create!(notes: 'Private note', user: users(:user_in_yet_another_practice), noteable: patients(:three)) + + assert_no_difference 'Note.count' do + assert_raises(ActiveRecord::RecordNotFound) do + delete :destroy, params: { patient_id: patients(:three).id, id: foreign_note.id, format: :js } + end + end + end + test 'requires authentication' do @controller.session['user'] = nil