From fba70baa9a2924479bc03570c25eed362e622239 Mon Sep 17 00:00:00 2001 From: Leo Kacenjar Date: Mon, 27 Jul 2026 16:53:07 -0600 Subject: [PATCH 1/2] security: fix security issues (#539) * security: authenticate and tenant-scope feedback endpoints FeedbackItemsController was unauthenticated and trusted a body-supplied user_id, allowing unauthenticated writes and user impersonation (pentest finding f-9a83). Inherit AuthenticatedController, derive user_id from current_user, and reject inferences outside the caller's site. Adds request specs (forgery protection disabled within that spec only, so it can reach the auth path) plus a factory. Co-Authored-By: Claude Opus 4.8 (1M context) * fix: keep header dropdown open on click The dropdown controller's document-level clickOutside handler fired on the same click that opened the menu and immediately re-hid it under real browser clicks (synthetic .click() was unaffected, which is why it passed manually but failed in Capybara). Stop propagation in toggle so the opening click never reaches the outside-click handler. Fixes the consistently-failing admin_spec.rb:24 feature test. Co-Authored-By: Claude Opus 4.8 (1M context) * security: scope audit-report downloads to the site's bucket and prefix workflow_audit_report passed user-controlled bucket_name and key straight to S3, allowing cross-tenant reads and arbitrary-bucket access (pentest finding f-832d5cf3). Constrain the bucket to default_s3_bucket and the key to the requesting site's own reports// prefix, returning an identical 404 on mismatch so no bucket/key oracle leaks. Extract Site#machine_name (dedup) and guard against two sites resolving to the same slug. Remove the vestigial duplicate :id route segment so authorization pins to the actual site. Co-Authored-By: Claude Opus 4.8 (1M context) * security: bump llm to 0.31.1 to fix code-injection vuln (CVE-2026-31236) ci/requirements.txt pinned llm==0.26, in the vulnerable range (<= 0.27.1) for GHSA-g76p-4vg5-f4qh. Bump to 0.31.1 (first fixed release was 0.28). This was the only pinned vulnerable copy: document_inference depends on llm only transitively via llm-gemini/llm-anthropic (llm>=0.26, unpinned), so its Docker build already resolves to the latest safe llm; evaluation has no llm dependency. Also aligns CI with the llm version prod actually installs. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- app/controllers/application_controller.rb | 2 - app/controllers/feedback_items_controller.rb | 36 +++-- app/controllers/sites_controller.rb | 17 ++- .../controllers/dropdown_controller.js | 3 +- app/models/site.rb | 21 ++- config/routes.rb | 2 +- python_components/ci/requirements.txt | 2 +- spec/factories/feedback_items.rb | 8 ++ spec/models/site_spec.rb | 15 +++ .../feedback_items_controller_spec.rb | 124 ++++++++++++++++++ spec/requests/sites_controller_spec.rb | 42 +++++- 11 files changed, 248 insertions(+), 24 deletions(-) create mode 100644 spec/factories/feedback_items.rb create mode 100644 spec/requests/feedback_items_controller_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e3641dea..09705d12 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,4 +1,2 @@ class ApplicationController < ActionController::Base - # TODO: REMOVE ME!!! - # skip_before_action :verify_authenticity_token end diff --git a/app/controllers/feedback_items_controller.rb b/app/controllers/feedback_items_controller.rb index 6cef2586..63e68f3b 100644 --- a/app/controllers/feedback_items_controller.rb +++ b/app/controllers/feedback_items_controller.rb @@ -1,27 +1,41 @@ -class FeedbackItemsController < ApplicationController +class FeedbackItemsController < AuthenticatedController wrap_parameters false + def update_feedback_items - begin - batch_params["feedback_items"].each do |patched_item| - item = FeedbackItem.where(document_inference_id: patched_item["document_inference_id"], user_id: patched_item["user_id"]).first_or_create - item.assign_attributes(patched_item) - item.save! - end - rescue - return render json: {error: "Error updating feedback items."}, status: :unprocessable_entity + batch_params["feedback_items"].each do |patched_item| + inference = authorized_inference!(patched_item["document_inference_id"]) + item = FeedbackItem.where(document_inference_id: inference.id, user_id: current_user.id).first_or_create + item.assign_attributes(patched_item) + item.save! end render json: {success: true} + rescue ActiveRecord::RecordNotFound + render json: {error: "Feedback target not found."}, status: :not_found + rescue + render json: {error: "Error updating feedback items."}, status: :unprocessable_entity end def delete_items batch_params["feedback_items"].each do |patched_item| - FeedbackItem.where(document_inference_id: patched_item["document_inference_id"], user_id: patched_item["user_id"]).destroy_all + inference = authorized_inference!(patched_item["document_inference_id"]) + FeedbackItem.where(document_inference_id: inference.id, user_id: current_user.id).destroy_all end + head :no_content + rescue ActiveRecord::RecordNotFound + render json: {error: "Feedback target not found."}, status: :not_found end private + def authorized_inference!(document_inference_id) + inference = DocumentInference.find(document_inference_id) + unless current_user.is_site_admin? || current_user.site == inference.document.site + raise ActiveRecord::RecordNotFound + end + inference + end + def batch_params - params.permit(feedback_items: [:id, :document_inference_id, :user_id, :sentiment, :comment]).to_h + params.permit(feedback_items: [:document_inference_id, :sentiment, :comment]).to_h end end diff --git a/app/controllers/sites_controller.rb b/app/controllers/sites_controller.rb index c4fd1b1d..89e7acab 100644 --- a/app/controllers/sites_controller.rb +++ b/app/controllers/sites_controller.rb @@ -60,10 +60,13 @@ def create_workflow_audit_report end def workflow_audit_report + key = params[:key] + key = key.start_with?("/") ? key : "/#{key}" + unless authorized_report_location?(params[:bucket_name], key) + return render plain: "File not found", status: 404 + end s3_manager = AwsS3Manager.new begin - key = params[:key] - key = key.start_with?("/") ? key : "/#{key}" response = s3_manager.get_object!(params[:bucket_name], key) send_data response[:body].read, filename: File.basename(key), @@ -79,6 +82,16 @@ def workflow_audit_report private + # Constrain audit-report downloads to the app's own bucket and to the + # requesting site's own reports// prefix. @site is the URL's + # site, already authorized by ensure_user_site_access. The trailing slash is + # load-bearing: without it, slug "revenue" could read "revenue_dept"'s keys. + # Returns the same 404 as a missing key so no bucket/key oracle leaks. + def authorized_report_location?(bucket_name, normalized_key) + bucket_name == Rails.application.config.default_s3_bucket && + normalized_key.start_with?("/reports/#{@site.machine_name}/") + end + def site_params params.require(:site).permit(:name, :location, :primary_url) end diff --git a/app/javascript/controllers/dropdown_controller.js b/app/javascript/controllers/dropdown_controller.js index 3c9852d8..ece46040 100644 --- a/app/javascript/controllers/dropdown_controller.js +++ b/app/javascript/controllers/dropdown_controller.js @@ -3,7 +3,8 @@ import { Controller } from "@hotwired/stimulus" export default class extends Controller { static targets = ["menu"] - toggle() { + toggle(event) { + event.stopPropagation() this.menuTarget.classList.toggle("hidden") } diff --git a/app/models/site.rb b/app/models/site.rb index 9755ccb7..1fa1c8ba 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -129,6 +129,13 @@ class Site < ApplicationRecord validates :location, presence: true validates :primary_url, presence: true, uniqueness: true validate :ensure_safe_url + validate :machine_name_must_be_unique + + # Filesystem/S3-safe identifier derived from the name. Used to scope this + # site's audit-report keys under reports//. + def machine_name + name.to_s.downcase.gsub(/\W+/, "_") + end after_initialize :after_initialize @@ -299,7 +306,7 @@ def process_archive_or_csv(file_path, is_archive) def export_document_audit!(current_user) assert_s3_manager bucket_name = Rails.application.config.default_s3_bucket - machine_site_name = name.downcase.gsub(/\W+/, "_") + machine_site_name = machine_name report_name = "audit_export_#{machine_site_name}_#{Time.now.strftime("%Y-%m-%dT%H-%M-%S")}" Tempfile.create([report_name, ".csv"]) do |temp_file| CSV.open(temp_file.path, "wb") do |csv| @@ -319,7 +326,7 @@ def export_document_audit!(current_user) def get_document_audit_exports! assert_s3_manager bucket_name = Rails.application.config.default_s3_bucket - machine_site_name = name.downcase.gsub(/\W+/, "_") + machine_site_name = machine_name { bucket_name: bucket_name, files: @s3_manager.get_files!(bucket_name, "/reports/#{machine_site_name}") @@ -345,6 +352,16 @@ def get_document_audit_link_hashes! private + # Two distinct names can normalize to the same machine_name (e.g. "SLC Gov" + # and "SLC.gov" both become "slc_gov"), which would let them share a + # reports// prefix and defeat per-site scoping. Guard against it. + def machine_name_must_be_unique + return if name.blank? + if Site.where.not(id: id).any? { |other| other.machine_name == machine_name } + errors.add(:name, "conflicts with an existing site's report identifier") + end + end + def assert_s3_manager if @s3_manager.nil? raise StandardError.new("Failed to connect to AWS environment (AwsS3Manager failed to initialize).") diff --git a/config/routes.rb b/config/routes.rb index c5c25f7d..1eb0c218 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -13,7 +13,7 @@ resources :sites do member do - get "workflow_audit_report/:id/:bucket_name/*key", to: "sites#workflow_audit_report", format: false, as: :workflow_audit_report + get "workflow_audit_report/:bucket_name/*key", to: "sites#workflow_audit_report", format: false, as: :workflow_audit_report post :create_workflow_audit_report end resources :documents do diff --git a/python_components/ci/requirements.txt b/python_components/ci/requirements.txt index c03b19b5..37f6393e 100644 --- a/python_components/ci/requirements.txt +++ b/python_components/ci/requirements.txt @@ -10,7 +10,7 @@ inflect==7.3.1 isort==6.0.1 jaraco-functools==4.2.1 jaraco.collections==5.1.0 -llm==0.26 +llm==0.31.1 pandas==2.2.3 pip-chill==1.0.3 pymupdf==1.25.5 diff --git a/spec/factories/feedback_items.rb b/spec/factories/feedback_items.rb new file mode 100644 index 00000000..4d7564c1 --- /dev/null +++ b/spec/factories/feedback_items.rb @@ -0,0 +1,8 @@ +FactoryBot.define do + factory :feedback_item do + sentiment { "positive" } + comment { "Looks right." } + document_inference + user + end +end diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb index d13c9d73..ddeafcdf 100644 --- a/spec/models/site_spec.rb +++ b/spec/models/site_spec.rb @@ -14,4 +14,19 @@ it { is_expected.to validate_uniqueness_of(:primary_url) } it { is_expected.to validate_uniqueness_of(:name) } + + describe "#machine_name" do + it "lowercases the name and collapses non-word runs into underscores" do + expect(build(:site, name: "SLC.gov").machine_name).to eq("slc_gov") + end + end + + describe "report-identifier uniqueness" do + it "rejects a second site whose name resolves to the same machine_name" do + create(:site, name: "SLC Gov") + dup = build(:site, name: "SLC.gov") + expect(dup).not_to be_valid + expect(dup.errors[:name]).to be_present + end + end end diff --git a/spec/requests/feedback_items_controller_spec.rb b/spec/requests/feedback_items_controller_spec.rb new file mode 100644 index 00000000..bc36187e --- /dev/null +++ b/spec/requests/feedback_items_controller_spec.rb @@ -0,0 +1,124 @@ +require "rails_helper" + +RSpec.describe FeedbackItemsController, type: :request do + include Warden::Test::Helpers + + after { Warden.test_reset! } + + # Request specs carry no CSRF token, so disable forgery protection for this + # spec only. Turning it off globally would strip the csrf-token meta tag that + # the app's JS depends on, breaking the JS feature specs. + around do |example| + original = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = false + example.run + ActionController::Base.allow_forgery_protection = original + end + + let(:site) { create(:site) } + let(:document) { create(:document, site: site) } + let(:inference) { create(:document_inference, document: document) } + let(:user) { create(:user, site: site) } + + describe "authentication" do + it "returns 401 for an unauthenticated PATCH" do + patch update_feedback_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: inference.id, sentiment: "positive"}]}, + as: :json + + expect(response).to have_http_status(:unauthorized) + expect(FeedbackItem.count).to eq(0) + end + + it "returns 401 for an unauthenticated DELETE" do + create(:feedback_item, document_inference: inference, user: user) + + delete delete_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: inference.id}]}, + as: :json + + expect(response).to have_http_status(:unauthorized) + expect(FeedbackItem.count).to eq(1) + end + end + + describe "PATCH update_feedback_items" do + before { login_as(user, scope: :user) } + + it "creates feedback owned by the current user" do + patch update_feedback_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: inference.id, sentiment: "negative", comment: "off"}]}, + as: :json + + expect(response).to have_http_status(:ok) + item = FeedbackItem.sole + expect(item.user).to eq(user) + expect(item.sentiment).to eq("negative") + end + + it "ignores a body user_id that names another user" do + other_user = create(:user, site: site) + + patch update_feedback_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: inference.id, user_id: other_user.id, sentiment: "positive"}]}, + as: :json + + expect(response).to have_http_status(:ok) + expect(FeedbackItem.sole.user).to eq(user) + end + + it "refuses feedback on an inference belonging to another site" do + other_inference = create(:document_inference, document: create(:document, site: create(:site))) + + patch update_feedback_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: other_inference.id, sentiment: "positive"}]}, + as: :json + + expect(response).to have_http_status(:not_found) + expect(FeedbackItem.count).to eq(0) + end + + it "lets a site admin leave feedback across sites" do + admin = create(:user, :site_admin) + login_as(admin, scope: :user) + other_inference = create(:document_inference, document: create(:document, site: create(:site))) + + patch update_feedback_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: other_inference.id, sentiment: "positive"}]}, + as: :json + + expect(response).to have_http_status(:ok) + expect(FeedbackItem.sole.user).to eq(admin) + end + end + + describe "DELETE delete_items" do + before { login_as(user, scope: :user) } + + it "removes only the caller's own feedback" do + mine = create(:feedback_item, document_inference: inference, user: user) + theirs = create(:feedback_item, document_inference: inference, user: create(:user, site: site)) + + delete delete_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: inference.id}]}, + as: :json + + expect(response).to have_http_status(:no_content) + expect(FeedbackItem.exists?(mine.id)).to be(false) + expect(FeedbackItem.exists?(theirs.id)).to be(true) + end + + it "refuses to act on an inference belonging to another site" do + other_site = create(:site) + other_inference = create(:document_inference, document: create(:document, site: other_site)) + row = create(:feedback_item, document_inference: other_inference, user: user) + + delete delete_items_feedback_items_path, + params: {feedback_items: [{document_inference_id: other_inference.id}]}, + as: :json + + expect(response).to have_http_status(:not_found) + expect(FeedbackItem.exists?(row.id)).to be(true) + end + end +end diff --git a/spec/requests/sites_controller_spec.rb b/spec/requests/sites_controller_spec.rb index 29389244..7df25718 100644 --- a/spec/requests/sites_controller_spec.rb +++ b/spec/requests/sites_controller_spec.rb @@ -5,8 +5,8 @@ describe "GET workflow_audit_report" do let(:site) { create(:site) } - let(:bucket_name) { "test-bucket" } - let(:file_key) { "reports/audit.csv" } + let(:bucket_name) { Rails.application.config.default_s3_bucket } + let(:file_key) { "reports/#{site.machine_name}/audit.csv" } let(:file_content) { "id,name,status\n1,doc1,complete" } let(:s3_response) do @@ -16,8 +16,9 @@ } end + let(:s3_manager) { instance_double(AwsS3Manager) } + before do - s3_manager = instance_double(AwsS3Manager) allow(AwsS3Manager).to receive(:new).and_return(s3_manager) allow(s3_manager).to receive(:get_object!).and_return(s3_response) end @@ -42,12 +43,45 @@ before { login_as(user, scope: :user) } - it "serves the file" do + it "serves a file under its own site's prefix" do get workflow_audit_report_site_path(site, bucket_name: bucket_name, key: file_key) expect(response).to have_http_status(:ok) expect(response.body).to eq(file_content) end + + it "refuses a non-default bucket without hitting S3" do + expect(s3_manager).not_to receive(:get_object!) + + get workflow_audit_report_site_path(site, bucket_name: "some-other-bucket", key: file_key) + + expect(response).to have_http_status(:not_found) + end + + it "refuses a key under another site's prefix" do + other_site = create(:site) + + get workflow_audit_report_site_path(site, bucket_name: bucket_name, key: "reports/#{other_site.machine_name}/audit.csv") + + expect(response).to have_http_status(:not_found) + end + + it "refuses a key outside any reports prefix" do + get workflow_audit_report_site_path(site, bucket_name: bucket_name, key: "secrets/credentials.csv") + + expect(response).to have_http_status(:not_found) + end + + it "enforces the trailing-slash boundary against sibling prefixes" do + revenue = create(:site, name: "Revenue") + revenue_user = create(:user, site: revenue) + login_as(revenue_user, scope: :user) + + # "revenue" is a prefix of "revenue_dept" — must not be readable. + get workflow_audit_report_site_path(revenue, bucket_name: bucket_name, key: "reports/revenue_dept/audit.csv") + + expect(response).to have_http_status(:not_found) + end end context "as non-admin without site access" do From 37cfefd7b62ab34236dfdc9abb5f4fae3d633a86 Mon Sep 17 00:00:00 2001 From: Leo Kacenjar Date: Mon, 3 Aug 2026 15:58:21 -0600 Subject: [PATCH 2/2] security: more pentest fixes (#556) * Add failing test for reporting endpoint. * Add reporting endpoint to site protected list. * Add failing test for header manipulation attack. * Add fix for header manipulation attack. --- app/controllers/sites_controller.rb | 2 +- app/models/document.rb | 9 +++++++ spec/models/document_spec.rb | 24 +++++++++++++++++++ spec/requests/sites_controller_spec.rb | 33 ++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 1 deletion(-) diff --git a/app/controllers/sites_controller.rb b/app/controllers/sites_controller.rb index 89e7acab..4a8a92ae 100644 --- a/app/controllers/sites_controller.rb +++ b/app/controllers/sites_controller.rb @@ -3,7 +3,7 @@ class SitesController < AuthenticatedController include ParamsHelper before_action :find_site, only: [:show, :edit, :update, :destroy, :create_workflow_audit_report, :workflow_audit_report] - before_action :ensure_user_site_access, only: [:show, :edit, :update, :destroy, :workflow_audit_report] + before_action :ensure_user_site_access, only: [:show, :edit, :update, :destroy, :create_workflow_audit_report, :workflow_audit_report] def index @sites = if current_user.is_site_admin? diff --git a/app/models/document.rb b/app/models/document.rb index 3c0755e8..8351d363 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -198,6 +198,7 @@ def inference_summary!(api_host = nil) else aws_env = (Rails.env == "production") ? "prod" : Rails.env lambda_manager = AwsLambdaManager.new(function_name: "asap-pdf-document-inference-#{aws_env}") + api_host = callback_base_url end payload = { model_name: "gemini-2.5-flash", @@ -230,6 +231,7 @@ def inference_recommendation!(api_host = nil) else aws_env = (Rails.env == "production") ? "prod" : Rails.env lambda_manager = AwsLambdaManager.new(function_name: "asap-pdf-document-inference-#{aws_env}") + api_host = callback_base_url end payload = { model_name: "gemini-2.5-pro", @@ -299,6 +301,13 @@ def get_crawl_status_display private + # Base URL the inference Lambda posts results back to. Derived from server-side + # config (never the request), so a spoofed X-Forwarded-Host can't redirect the + # Lambda's outbound callback to an attacker (SSRF). + def callback_base_url + "https://#{Rails.application.config.action_mailer.default_url_options[:host]}" + end + def recursive_decode(url) decoded_url = URI::DEFAULT_PARSER.unescape(url) if url != decoded_url diff --git a/spec/models/document_spec.rb b/spec/models/document_spec.rb index 3b43955d..f433ddd2 100644 --- a/spec/models/document_spec.rb +++ b/spec/models/document_spec.rb @@ -81,4 +81,28 @@ expect(complex_document_tables.complexity).to eq(Document::COMPLEX_STATUS) end end + + describe "inference callback endpoint (SSRF guard)" do + let(:document) { create(:document) } + + it "derives asap_endpoint from the configured host, ignoring a caller-supplied host" do + # Force the non-local (staging/prod) branch, where the callback host matters. + allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("staging")) + + lambda_manager = instance_double(AwsLambdaManager) + allow(AwsLambdaManager).to receive(:new).and_return(lambda_manager) + + captured = nil + allow(lambda_manager).to receive(:invoke_lambda!) do |payload| + captured = payload + double("response", body: {statusCode: 200, body: "ok"}.to_json) + end + + configured_host = Rails.application.config.action_mailer.default_url_options[:host] + document.inference_recommendation!("http://attacker.example.com") + + expect(captured[:asap_endpoint]).to eq("https://#{configured_host}/api/documents/#{document.id}/inference") + expect(captured[:asap_endpoint]).not_to include("attacker.example.com") + end + end end diff --git a/spec/requests/sites_controller_spec.rb b/spec/requests/sites_controller_spec.rb index 7df25718..45b3ec4f 100644 --- a/spec/requests/sites_controller_spec.rb +++ b/spec/requests/sites_controller_spec.rb @@ -107,4 +107,37 @@ end end end + + describe "POST create_workflow_audit_report" do + let(:site) { create(:site) } + + after { Warden.test_reset! } + + # Request specs carry no CSRF token; disable forgery protection for this + # spec only so the POST reaches the authorization check instead of being + # rejected with a 422 first. + around do |example| + original = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = false + example.run + ActionController::Base.allow_forgery_protection = original + end + + context "as a non-admin assigned to a different site" do + let(:other_site) { create(:site) } + let(:user) { create(:user, site: other_site) } + + before { login_as(user, scope: :user) } + + it "refuses to generate a report for a site the user cannot access" do + expect_any_instance_of(Site).not_to receive(:export_document_audit!) + + post create_workflow_audit_report_site_path(site) + + expect(response).to redirect_to(sites_path) + follow_redirect! + expect(response.body).to include("You don't have permission to access that site.") + end + end + end end