From 0ecd09b06beac943bdfbae285737fcf2029d5deb Mon Sep 17 00:00:00 2001 From: ulferts Date: Fri, 20 Jun 2025 17:25:15 +0200 Subject: [PATCH] rename to allowlist --- app/contracts/attachments/create_contract.rb | 24 +++++++++---------- app/services/attachments/base_service.rb | 4 ++-- app/workers/backup_job.rb | 2 +- app/workers/exports/export_job.rb | 2 +- config/locales/en.yml | 2 +- .../controllers/bim/bcf/issues_controller.rb | 2 +- .../bim/ifc_models/ifc_models_controller.rb | 12 +++++----- modules/bim/app/models/bim/bcf/viewpoint.rb | 2 +- .../app/models/bim/ifc_models/ifc_model.rb | 2 +- .../bim/ifc_models/set_attributes_service.rb | 2 +- .../ifc_models/shared_contract_examples.rb | 4 ++-- .../work_packages/exports/export_job_spec.rb | 19 +++++++++++---- .../attachments/create_contract_spec.rb | 14 +++++------ .../attachment_resource_shared_examples.rb | 8 +++---- ...nish_direct_upload_job_integration_spec.rb | 6 ++--- .../work_packages/exports/export_job_spec.rb | 16 +++++++++---- 16 files changed, 68 insertions(+), 53 deletions(-) diff --git a/app/contracts/attachments/create_contract.rb b/app/contracts/attachments/create_contract.rb index 2ec0d41d3fe..a3be099f6bc 100644 --- a/app/contracts/attachments/create_contract.rb +++ b/app/contracts/attachments/create_contract.rb @@ -68,29 +68,29 @@ module Attachments end ## - # Validates the content type, if a whitelist is set + # Validates the content type, if a allowlist is set def validate_content_type - # If the whitelist is empty, assume all files are allowed + # If the allowlist is empty, assume all files are allowed # as before - unless matches_whitelist?(attachment_whitelist) - Rails.logger.info { "Uploaded file #{model.filename} with type #{model.content_type} does not match whitelist" } - errors.add :content_type, :not_whitelisted, value: model.content_type + unless matches_allowlist?(attachment_allowlist) + Rails.logger.info { "Uploaded file #{model.filename} with type #{model.content_type} does not match allowlist" } + errors.add :content_type, :not_allowlisted, value: model.content_type end end ## - # Get the user-defined whitelist or a custom whitelist + # Get the user-defined allowlist or a custom allowlist # defined for this invocation - def attachment_whitelist - Array(options.fetch(:whitelist, Setting.attachment_whitelist)) + def attachment_allowlist + Array(options.fetch(:allowlist, Setting.attachment_whitelist)) end ## - # Returns whether the attachment matches the whitelist - def matches_whitelist?(whitelist) - return true if whitelist.empty? + # Returns whether the attachment matches the allowlist + def matches_allowlist?(allowlist) + return true if allowlist.empty? - whitelist.include?(model.content_type) || whitelist.include?("*#{model.extension}") + allowlist.include?(model.content_type) || allowlist.include?("*#{model.extension}") end end end diff --git a/app/services/attachments/base_service.rb b/app/services/attachments/base_service.rb index 3ebf934fd84..fda62e662b3 100644 --- a/app/services/attachments/base_service.rb +++ b/app/services/attachments/base_service.rb @@ -36,8 +36,8 @@ module Attachments # @param whitelist A custom whitelist to validate with, or empty to disable validation # # Warning: When passing an empty whitelist, this results in no validations on the content type taking place. - def self.bypass_whitelist(user:, whitelist: []) - new(user:, contract_options: { whitelist: whitelist.map(&:to_s) }) + def self.bypass_allowlist(user:, allowlist: []) + new(user:, contract_options: { allowlist: allowlist.map(&:to_s) }) end end end diff --git a/app/workers/backup_job.rb b/app/workers/backup_job.rb index 69e75e75b58..9be533fb694 100644 --- a/app/workers/backup_job.rb +++ b/app/workers/backup_job.rb @@ -134,7 +134,7 @@ class BackupJob < ApplicationJob def store_backup(file_name, backup:, user:) File.open(file_name) do |file| call = Attachments::CreateService - .bypass_whitelist(user:) + .bypass_allowlist(user:) .call(container: backup, filename: file_name, file:, description: "OpenProject backup") call.on_success do diff --git a/app/workers/exports/export_job.rb b/app/workers/exports/export_job.rb index 95b28bc110e..fac92e52533 100644 --- a/app/workers/exports/export_job.rb +++ b/app/workers/exports/export_job.rb @@ -117,7 +117,7 @@ module Exports filename = clean_filename(export_result) call = Attachments::CreateService - .bypass_whitelist(user: User.current) + .bypass_allowlist(user: User.current) .call(container:, file:, filename:, description: "") call.on_success do diff --git a/config/locales/en.yml b/config/locales/en.yml index c97dc0b8886..4674703d554 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1286,7 +1286,7 @@ en: attributes: content_type: blank: "The content type of the file cannot be blank." - not_whitelisted: "The file was rejected by an automatic filter. '%{value}' is not whitelisted for upload." + not_allowlisted: "The file was rejected by an automatic filter. '%{value}' is not allowed for upload." format: "%{message}" capability: context: diff --git a/modules/bim/app/controllers/bim/bcf/issues_controller.rb b/modules/bim/app/controllers/bim/bcf/issues_controller.rb index 11949e96083..178b2a7cb77 100644 --- a/modules/bim/app/controllers/bim/bcf/issues_controller.rb +++ b/modules/bim/app/controllers/bim/bcf/issues_controller.rb @@ -215,7 +215,7 @@ module Bim def create_attachment filename = params[:bcf_file].original_filename call = Attachments::CreateService - .bypass_whitelist(user: current_user, whitelist: %w[application/zip]) + .bypass_allowlist(user: current_user, allowlist: %w[application/zip]) .call(file: params[:bcf_file], filename:, description: filename) diff --git a/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb b/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb index 32d47c39c97..af418af6e6b 100644 --- a/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb +++ b/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb @@ -49,6 +49,10 @@ module Bim .includes(:project, :uploader) end + def show + frontend_redirect params[:id].to_i + end + def new @ifc_model = @project.ifc_models.build prepare_form(@ifc_model) @@ -58,10 +62,6 @@ module Bim prepare_form(@ifc_model) end - def show - frontend_redirect params[:id].to_i - end - def defaults frontend_redirect @ifc_models.defaults.pluck(:id).uniq end @@ -116,7 +116,7 @@ module Bim if service_result.success? ::Attachments::FinishDirectUploadJob.perform_later attachment.id, - whitelist: false + allowlist: false flash[:notice] = if new_model t("ifc_models.flash_messages.upload_successful") @@ -183,7 +183,7 @@ module Bim return unless OpenProject::Configuration.direct_uploads? call = ::Attachments::PrepareUploadService - .bypass_whitelist(user: current_user) + .bypass_allowlist(user: current_user) .call(filename: "model.ifc", filesize: 0) call.on_failure { flash[:error] = call.message } diff --git a/modules/bim/app/models/bim/bcf/viewpoint.rb b/modules/bim/app/models/bim/bcf/viewpoint.rb index 668ed857a09..921a6ae134c 100644 --- a/modules/bim/app/models/bim/bcf/viewpoint.rb +++ b/modules/bim/app/models/bim/bcf/viewpoint.rb @@ -73,7 +73,7 @@ module Bim::Bcf def build_snapshot(file, user: User.current) ::Attachments::BuildService - .bypass_whitelist(user:) + .bypass_allowlist(user:) .call(file:, container: self, filename: file.original_filename, description: "snapshot") .result end diff --git a/modules/bim/app/models/bim/ifc_models/ifc_model.rb b/modules/bim/app/models/bim/ifc_models/ifc_model.rb index e878716a430..307d0b2b139 100644 --- a/modules/bim/app/models/bim/ifc_models/ifc_model.rb +++ b/modules/bim/app/models/bim/ifc_models/ifc_model.rb @@ -41,7 +41,7 @@ module Bim delete_attachment name filename = file.respond_to?(:original_filename) ? file.original_filename : File.basename(file.path) call = ::Attachments::CreateService - .bypass_whitelist(user: User.current) + .bypass_allowlist(user: User.current) .call(file:, container: self, filename:, description: name) call.on_failure { Rails.logger.error "Failed to add #{name} attachment: #{call.message}" } diff --git a/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb b/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb index 8e9cc0746d2..eb7c1e79f91 100644 --- a/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb +++ b/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb @@ -78,7 +78,7 @@ module Bim def build_ifc_attachment(ifc_attachment) ::Attachments::BuildService - .bypass_whitelist(user:) + .bypass_allowlist(user:) .call(file: ifc_attachment, container: model, filename: ifc_attachment.original_filename, description: "ifc") .on_failure do |build_attachment_result| build_attachment_result.errors.each do |error| diff --git a/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb b/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb index 28db250bbab..50ab9b411cb 100644 --- a/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb +++ b/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb @@ -102,7 +102,7 @@ RSpec.shared_examples_for "ifc model contract" do let(:ifc_file) { FileHelpers.mock_uploaded_file name: "model.ifc", content_type: "application/binary", binary: true } let(:ifc_attachment) do Attachments::BuildService - .bypass_whitelist(user: current_user) + .bypass_allowlist(user: current_user) .call(file: ifc_file, filename: "model.ifc") .result end @@ -118,7 +118,7 @@ RSpec.shared_examples_for "ifc model contract" do end let(:ifc_attachment) do Attachments::BuildService - .bypass_whitelist(user: current_user) + .bypass_allowlist(user: current_user) .call(file: ifc_file, filename: "model.ifc") .result end diff --git a/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb b/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb index 491f18d6798..0909582fb4b 100644 --- a/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb +++ b/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb @@ -66,15 +66,14 @@ RSpec.describe WorkPackages::ExportJob do service = double("attachments create service") - expect(Attachments::CreateService) - .to receive(:bypass_whitelist) - .with(user:) + allow(Attachments::CreateService) + .to receive(:bypass_allowlist) .and_return(service) - expect(Exports::CleanupOutdatedJob) + allow(Exports::CleanupOutdatedJob) .to receive(:perform_after_grace) - expect(service) + allow(service) .to(receive(:call)) .and_return(ServiceResult.success(result: attachment)) @@ -85,6 +84,16 @@ RSpec.describe WorkPackages::ExportJob do expect(subject.job_status).to be_present expect(subject.job_status[:status]).to eq "success" expect(subject.job_status[:payload]["download"]).to eq "/api/v3/attachments/1234/content" + + expect(Attachments::CreateService) + .to have_received(:bypass_allowlist) + .with(user:) + + expect(Exports::CleanupOutdatedJob) + .to have_received(:perform_after_grace) + + expect(service) + .to have_received(:call) end end end diff --git a/spec/contracts/attachments/create_contract_spec.rb b/spec/contracts/attachments/create_contract_spec.rb index e2e0367981a..723ab76823b 100644 --- a/spec/contracts/attachments/create_contract_spec.rb +++ b/spec/contracts/attachments/create_contract_spec.rb @@ -104,28 +104,28 @@ RSpec.describe Attachments::CreateContract do end end - context "with an empty whitelist", + context "with an empty allowlist", with_settings: { attachment_whitelist: %w[] } do it_behaves_like "contract is valid" end - context "with a matching mime whitelist", + context "with a matching mime allowlist", with_settings: { attachment_whitelist: %w[image/png] } do it_behaves_like "contract is valid" end - context "with a matching extension whitelist", + context "with a matching extension allowlist", with_settings: { attachment_whitelist: %w[*.png] } do it_behaves_like "contract is valid" end - context "with a non-matching whitelist", + context "with a non-matching allowlist", with_settings: { attachment_whitelist: %w[*.jpg image/jpeg] } do - it_behaves_like "contract is invalid", content_type: :not_whitelisted + it_behaves_like "contract is invalid", content_type: :not_allowlisted context "when disabling the whitelist check" do let(:contract_options) do - { whitelist: [] } + { allowlist: [] } end it_behaves_like "contract is valid" @@ -133,7 +133,7 @@ RSpec.describe Attachments::CreateContract do context "when overriding the whitelist" do let(:contract_options) do - { whitelist: %w[*.png] } + { allowlist: %w[*.png] } end it_behaves_like "contract is valid" diff --git a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb index 19be033f486..5391a8a539a 100644 --- a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb +++ b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb @@ -157,7 +157,7 @@ RSpec.shared_examples "it supports direct uploads" do end end - context "with an attachment whitelist", with_settings: { attachment_whitelist: ["text/csv"] } do + context "with an attachment allowlist", with_settings: { attachment_whitelist: ["text/csv"] } do context "with an allowed content type" do let(:metadata) { { fileName: "cats.csv", fileSize: file.size, contentType: "text/csv" } } @@ -171,14 +171,14 @@ RSpec.shared_examples "it supports direct uploads" do it "fails" do expect(subject.status).to eq 422 - expect(subject.body).to include "not whitelisted" + expect(subject.body).to include "'text/plain' is not allowed for upload." end end - context "with a non-specific content type not on the whitelist" do + context "with a non-specific content type not on the allowlist" do let(:metadata) { { fileName: "cats.bin", fileSize: file.size, contentType: "application/binary" } } - # the actual whitelist check will be performed in the FinishDirectUpload job in this case + # the actual allowlist check will be performed in the FinishDirectUpload job in this case it "still succeeds" do expect(subject.status).to eq 201 end diff --git a/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb b/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb index 08097ce47d1..2df7fb0b761 100644 --- a/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb +++ b/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb @@ -136,7 +136,7 @@ RSpec.describe Attachments::FinishDirectUploadJob, "integration", type: :job do it_behaves_like "adding a journal to the attachment in the name of the attachment's author" end - context "with an incompatible attachment whitelist", + context "with an incompatible attachment allowlist", with_settings: { attachment_whitelist: %w[image/png] } do let!(:container) { create(:work_package) } let!(:container_timestamp) { container.updated_at } @@ -156,9 +156,9 @@ RSpec.describe Attachments::FinishDirectUploadJob, "integration", type: :job do expect { pending_attachment.reload }.to raise_error(ActiveRecord::RecordNotFound) end - context "when the job is getting a whitelist override" do + context "when the job is getting a allowlist override" do it "Does save the attachment" do - job.perform(pending_attachment.id, whitelist: false) + job.perform(pending_attachment.id, allowlist: false) container.reload diff --git a/spec/workers/work_packages/exports/export_job_spec.rb b/spec/workers/work_packages/exports/export_job_spec.rb index 9480c7b2e28..114b492904b 100644 --- a/spec/workers/work_packages/exports/export_job_spec.rb +++ b/spec/workers/work_packages/exports/export_job_spec.rb @@ -68,15 +68,14 @@ RSpec.describe WorkPackages::ExportJob do let(:exporter_instance) { instance_double(exporter) } it "exports" do - expect(Attachments::CreateService) - .to receive(:bypass_whitelist) - .with(user:) + allow(Attachments::CreateService) + .to receive(:bypass_allowlist) .and_return(service) - expect(Exports::CleanupOutdatedJob) + allow(Exports::CleanupOutdatedJob) .to receive(:perform_after_grace) - expect(service) + allow(service) .to receive(:call) do |file:, **_args| expect(File.basename(file)) .to start_with "some_title" @@ -96,6 +95,13 @@ RSpec.describe WorkPackages::ExportJob do expect(subject.job_status.reference).to eq export expect(subject.job_status[:status]).to eq "success" expect(subject.job_status[:payload]["download"]).to eq "/api/v3/attachments/1234/content" + + expect(Attachments::CreateService) + .to have_received(:bypass_allowlist) + .with(user:) + + expect(Exports::CleanupOutdatedJob) + .to have_received(:perform_after_grace) end end