From 8479323fc9eee64f05a8d72990ff1e56ba93408f Mon Sep 17 00:00:00 2001 From: Linda Goldstein Date: Tue, 28 Apr 2026 22:08:42 -0700 Subject: [PATCH 1/2] ci: chown mounts to app user before web container starts The web image now runs as a non-root `app` user, but the bind mount (`.:/usr/src/app`) and named volumes (`bundle_data`, `node_modules`) override the Dockerfile's `chown`. `bin/dev` then fails on permissions and the container exits, so `docker compose exec ... rails db:setup` returns 137. Bring deps up first, run a one-shot root container to chown the mount points, then start `web`. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/docker.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 9ba770abca..55fcd87c38 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -33,7 +33,10 @@ jobs: mkdir -p tmp/downloads chmod 777 tmp tmp/downloads - name: docker UP - run: docker compose up -d + run: | + docker compose up -d database selenium_chrome + docker compose run --rm --no-deps --user 0:0 --entrypoint sh web -c "chown -R app:app /usr/src/app /usr/local/bundle/gems" + docker compose up -d web - name: db:setup run: docker compose exec -T web rails db:setup - name: compile assets From 5054433086f9891b8be7afc4040870a8fd2b88c7 Mon Sep 17 00:00:00 2001 From: Linda Goldstein Date: Tue, 28 Apr 2026 22:58:26 -0700 Subject: [PATCH 2/2] refactor: centralize controller rescues and enable index policy scoping Move ActiveRecord::RecordNotFound rescue from ContactTypeGroupsController up to ApplicationController so every controller renders a consistent 404 (HTML or JSON). Drop the StandardError rescue and KNOWN_ERRORS / log_and_reraise pair: Bugsnag's Rack middleware already auto-reports unhandled exceptions, so the manual notify-and-reraise caused duplicate notifications. Enable `after_action :verify_policy_scoped, only: :index` to surface multi-tenancy regressions like the recent ContactTypeGroup cross-org bug. Add `skip_after_action :verify_policy_scoped` with a TODO to controllers whose index does not currently call `policy_scope` (CSV reports, public endpoints, admin-scope controllers, etc.), so behavior is preserved while each gap is explicit and trackable. Update specs that asserted `raise_error(ActiveRecord::RecordNotFound)` to assert a 404 response, since the error is now rescued centrally. Add new coverage in application_controller_spec for the HTML and JSON 404 paths. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/all_casa_admins_controller.rb | 1 + .../android_app_associations_controller.rb | 1 + app/controllers/application_controller.rb | 19 +++++++-------- app/controllers/banners_controller.rb | 1 + .../case_contact_reports_controller.rb | 1 + .../case_court_reports_controller.rb | 1 + .../contact_type_groups_controller.rb | 9 ------- app/controllers/error_controller.rb | 1 + .../followup_reports_controller.rb | 1 + app/controllers/health_controller.rb | 1 + app/controllers/imports_controller.rb | 1 + .../learning_hours_reports_controller.rb | 1 + app/controllers/mileage_rates_controller.rb | 1 + app/controllers/mileage_reports_controller.rb | 1 + .../missing_data_reports_controller.rb | 1 + app/controllers/notifications_controller.rb | 1 + app/controllers/other_duties_controller.rb | 1 + .../placement_reports_controller.rb | 1 + app/controllers/reports_controller.rb | 1 + app/controllers/static_controller.rb | 1 + .../application_controller_spec.rb | 24 +++++++++++++++++++ .../emancipations_controller_spec.rb | 7 +++--- spec/requests/bulk_court_dates_spec.rb | 5 ++-- spec/requests/case_contacts/followups_spec.rb | 10 ++++---- spec/requests/users_spec.rb | 5 ++-- 25 files changed, 66 insertions(+), 31 deletions(-) diff --git a/app/controllers/all_casa_admins_controller.rb b/app/controllers/all_casa_admins_controller.rb index 93a462abd5..2c544544e6 100644 --- a/app/controllers/all_casa_admins_controller.rb +++ b/app/controllers/all_casa_admins_controller.rb @@ -4,6 +4,7 @@ class AllCasaAdminsController < ApplicationController before_action :set_custom_error_heading, only: [:update_password] after_action :reset_custom_error_heading, only: [:update_password] skip_after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def new @all_casa_admin = AllCasaAdmin.new diff --git a/app/controllers/android_app_associations_controller.rb b/app/controllers/android_app_associations_controller.rb index 8fd3b7e2c9..1f8e7484cb 100644 --- a/app/controllers/android_app_associations_controller.rb +++ b/app/controllers/android_app_associations_controller.rb @@ -1,5 +1,6 @@ class AndroidAppAssociationsController < ApplicationController skip_before_action :authenticate_user! + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index android_asset_link_data = [ diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 024dd07985..811e48f9fc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -12,13 +12,12 @@ class ApplicationController < ActionController::Base before_action :set_current_organization before_action :set_active_banner after_action :verify_authorized, except: :index, unless: :devise_controller? - # after_action :verify_policy_scoped, only: :index + after_action :verify_policy_scoped, only: :index, unless: :devise_controller? - KNOWN_ERRORS = [Pundit::NotAuthorizedError, Organizational::UnknownOrganization] - rescue_from StandardError, with: :log_and_reraise rescue_from Pundit::NotAuthorizedError, with: :not_authorized rescue_from Organizational::UnknownOrganization, with: :not_authorized rescue_from ActionController::UnknownFormat, with: :unsupported_media_type + rescue_from ActiveRecord::RecordNotFound, with: :record_not_found impersonates :user @@ -157,6 +156,13 @@ def not_authorized end end + def record_not_found + respond_to do |format| + format.json { render json: {error: "Record not found"}, status: :not_found } + format.any { render file: Rails.public_path.join("404.html"), status: :not_found, layout: false } + end + end + def unsupported_media_type respond_to do |format| format.json do @@ -169,13 +175,6 @@ def unsupported_media_type end end - def log_and_reraise(error) - unless KNOWN_ERRORS.include?(error.class) - Bugsnag.notify(error) - end - raise - end - def check_unconfirmed_email_notice(user) notice = "#{user.role} was successfully updated." if user.saved_changes.include?("unconfirmed_email") diff --git a/app/controllers/banners_controller.rb b/app/controllers/banners_controller.rb index 613d0126bc..59e7f12768 100644 --- a/app/controllers/banners_controller.rb +++ b/app/controllers/banners_controller.rb @@ -1,5 +1,6 @@ class BannersController < ApplicationController after_action :verify_authorized, except: %i[dismiss] + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does before_action :set_banner, only: %i[edit update destroy dismiss] def index diff --git a/app/controllers/case_contact_reports_controller.rb b/app/controllers/case_contact_reports_controller.rb index ead360ef90..86b714e8ad 100644 --- a/app/controllers/case_contact_reports_controller.rb +++ b/app/controllers/case_contact_reports_controller.rb @@ -2,6 +2,7 @@ class CaseContactReportsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize :application, :see_reports_page? diff --git a/app/controllers/case_court_reports_controller.rb b/app/controllers/case_court_reports_controller.rb index 81484266c0..247051d94d 100644 --- a/app/controllers/case_court_reports_controller.rb +++ b/app/controllers/case_court_reports_controller.rb @@ -3,6 +3,7 @@ class CaseCourtReportsController < ApplicationController before_action :set_casa_case, only: %i[show] after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize CaseCourtReport diff --git a/app/controllers/contact_type_groups_controller.rb b/app/controllers/contact_type_groups_controller.rb index 780687c654..91f171e07e 100644 --- a/app/controllers/contact_type_groups_controller.rb +++ b/app/controllers/contact_type_groups_controller.rb @@ -2,8 +2,6 @@ class ContactTypeGroupsController < ApplicationController before_action :set_contact_type_group, except: [:new, :create] after_action :verify_authorized - rescue_from ActiveRecord::RecordNotFound, with: :record_not_found - def new authorize ContactTypeGroup @contact_type_group = ContactTypeGroup.new @@ -35,13 +33,6 @@ def update private - def record_not_found - respond_to do |format| - format.json { render json: {error: "Record not found"}, status: :not_found } - format.any { render file: Rails.public_path.join("404.html"), status: :not_found, layout: false } - end - end - def contact_type_group_params params.require(:contact_type_group).permit(:name, :active) end diff --git a/app/controllers/error_controller.rb b/app/controllers/error_controller.rb index abdf11f264..7aa5294dfa 100644 --- a/app/controllers/error_controller.rb +++ b/app/controllers/error_controller.rb @@ -3,6 +3,7 @@ class ErrorController < ApplicationController skip_before_action :authenticate_user! skip_after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index end diff --git a/app/controllers/followup_reports_controller.rb b/app/controllers/followup_reports_controller.rb index 09524d1fef..894ad5aaa8 100644 --- a/app/controllers/followup_reports_controller.rb +++ b/app/controllers/followup_reports_controller.rb @@ -2,6 +2,7 @@ class FollowupReportsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize :application, :see_reports_page? diff --git a/app/controllers/health_controller.rb b/app/controllers/health_controller.rb index 3a20af680b..bbc16f505c 100644 --- a/app/controllers/health_controller.rb +++ b/app/controllers/health_controller.rb @@ -3,6 +3,7 @@ class HealthController < ApplicationController skip_before_action :authenticate_user! skip_after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does before_action :verify_token_for_old_object_stats, only: [:old_objects] def index diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index cc445ff249..628d15d34a 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -4,6 +4,7 @@ class ImportsController < ApplicationController include ActionView::Helpers::UrlHelper before_action :failed_csv_service, only: [:create, :download_failed] after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does ERR_FAILED_IMPORT_NOTE = "Note: An additional 'error' column has been added to the file. " \ "Please note the failure reason and remove the column when resubmitting." diff --git a/app/controllers/learning_hours_reports_controller.rb b/app/controllers/learning_hours_reports_controller.rb index 4d8ffc6c10..701d33b247 100644 --- a/app/controllers/learning_hours_reports_controller.rb +++ b/app/controllers/learning_hours_reports_controller.rb @@ -1,5 +1,6 @@ class LearningHoursReportsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize :application, :see_reports_page? diff --git a/app/controllers/mileage_rates_controller.rb b/app/controllers/mileage_rates_controller.rb index 4d41b3d902..c1c27ca230 100644 --- a/app/controllers/mileage_rates_controller.rb +++ b/app/controllers/mileage_rates_controller.rb @@ -1,5 +1,6 @@ class MileageRatesController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does before_action :set_mileage_rate, only: %i[edit update] def index diff --git a/app/controllers/mileage_reports_controller.rb b/app/controllers/mileage_reports_controller.rb index a975eded58..487baeca98 100644 --- a/app/controllers/mileage_reports_controller.rb +++ b/app/controllers/mileage_reports_controller.rb @@ -4,6 +4,7 @@ class MileageReportsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize :application, :see_reports_page? diff --git a/app/controllers/missing_data_reports_controller.rb b/app/controllers/missing_data_reports_controller.rb index 318df1f9d6..d93fc33002 100644 --- a/app/controllers/missing_data_reports_controller.rb +++ b/app/controllers/missing_data_reports_controller.rb @@ -1,5 +1,6 @@ class MissingDataReportsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize :application, :see_reports_page? diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 0f7df4037d..8726d2d21f 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -1,5 +1,6 @@ class NotificationsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does before_action :set_notification, only: %i[mark_as_read] def index diff --git a/app/controllers/other_duties_controller.rb b/app/controllers/other_duties_controller.rb index 7f3e1935f3..8e6e22e66c 100644 --- a/app/controllers/other_duties_controller.rb +++ b/app/controllers/other_duties_controller.rb @@ -1,6 +1,7 @@ class OtherDutiesController < ApplicationController before_action :set_other_duty, except: [:new, :create, :index] before_action :convert_duration_minutes, only: [:update, :create] + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize OtherDuty diff --git a/app/controllers/placement_reports_controller.rb b/app/controllers/placement_reports_controller.rb index ffb43d2aeb..42f8f83c71 100644 --- a/app/controllers/placement_reports_controller.rb +++ b/app/controllers/placement_reports_controller.rb @@ -1,5 +1,6 @@ class PlacementReportsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize :application, :see_reports_page? diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index b023ed6c2b..df0685c6ab 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -1,5 +1,6 @@ class ReportsController < ApplicationController after_action :verify_authorized + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does def index authorize :application, :see_reports_page? diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index 3da738c07c..3dc42aa4e6 100644 --- a/app/controllers/static_controller.rb +++ b/app/controllers/static_controller.rb @@ -3,6 +3,7 @@ class StaticController < ApplicationController skip_before_action :authenticate_user! skip_before_action :set_current_user skip_before_action :set_current_organization + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does layout false diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 1225e2619a..2259bb1996 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -5,6 +5,8 @@ let(:volunteer) { create(:volunteer) } controller do + skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does + def index render plain: "hello there..." end @@ -26,6 +28,10 @@ def not_authorized_error def unknown_organization raise Organizational::UnknownOrganization end + + def missing_record + raise ActiveRecord::RecordNotFound + end end before do @@ -82,6 +88,24 @@ def unknown_organization end end + describe "rescue_from ActiveRecord::RecordNotFound" do + before do + routes.draw { get :missing_record, to: "anonymous#missing_record" } + end + + it "renders the static 404 page for HTML requests" do + get :missing_record + expect(response).to have_http_status(:not_found) + expect(response.body).to eq(File.read(Rails.public_path.join("404.html"))) + end + + it "renders a JSON error for JSON requests" do + get :missing_record, format: :json + expect(response).to have_http_status(:not_found) + expect(response.parsed_body).to eq("error" => "Record not found") + end + end + describe "After signin path" do it "is equal to initial path" do routes.draw { get :index, to: "anonymous#index" } diff --git a/spec/controllers/emancipations_controller_spec.rb b/spec/controllers/emancipations_controller_spec.rb index b457ce706c..bbe5453e4b 100644 --- a/spec/controllers/emancipations_controller_spec.rb +++ b/spec/controllers/emancipations_controller_spec.rb @@ -30,10 +30,9 @@ end context "when case does not exist" do - it "raises a record not found error" do - expect { - get :show, params: {casa_case_id: "nonexistent-case"} - }.to raise_error(ActiveRecord::RecordNotFound) + it "responds with 404" do + get :show, params: {casa_case_id: "nonexistent-case"} + expect(response).to have_http_status(:not_found) end end diff --git a/spec/requests/bulk_court_dates_spec.rb b/spec/requests/bulk_court_dates_spec.rb index 300dffa3a3..41cbaa293d 100644 --- a/spec/requests/bulk_court_dates_spec.rb +++ b/spec/requests/bulk_court_dates_spec.rb @@ -56,8 +56,9 @@ context "when different casa org's case group" do let(:case_group) { create :case_group, case_count:, casa_org: build(:casa_org) } - it "raises ActiveRecord::RecordNotFound" do - expect { subject }.to raise_error(ActiveRecord::RecordNotFound) + it "responds with 404" do + subject + expect(response).to have_http_status(:not_found) end end diff --git a/spec/requests/case_contacts/followups_spec.rb b/spec/requests/case_contacts/followups_spec.rb index 6bf0594ced..97f60533d6 100644 --- a/spec/requests/case_contacts/followups_spec.rb +++ b/spec/requests/case_contacts/followups_spec.rb @@ -52,8 +52,9 @@ end context "with invalid case_contact" do - it "raises ActiveRecord::RecordNotFound" do - expect { post case_contact_followups_path(444444) }.to raise_error(ActiveRecord::RecordNotFound) + it "responds with 404" do + post case_contact_followups_path(444444) + expect(response).to have_http_status(:not_found) end end end @@ -116,8 +117,9 @@ end context "followup doesn't exists" do - it "raises ActiveRecord::RecordNotFound" do - expect { patch resolve_followup_path(444444) }.to raise_error(ActiveRecord::RecordNotFound) + it "responds with 404" do + patch resolve_followup_path(444444) + expect(response).to have_http_status(:not_found) end end end diff --git a/spec/requests/users_spec.rb b/spec/requests/users_spec.rb index ef9d103d7b..293a519963 100644 --- a/spec/requests/users_spec.rb +++ b/spec/requests/users_spec.rb @@ -508,8 +508,9 @@ } end - it "raises error when Language do not exist" do - expect { delete remove_language_users_path(999) }.to raise_error(ActiveRecord::RecordNotFound) + it "responds with 404 when Language does not exist" do + delete remove_language_users_path(999) + expect(response).to have_http_status(:not_found) end end end