From 83a8a542e3f9f9561640d7266b5bef4ab43befa1 Mon Sep 17 00:00:00 2001 From: Mth0158 Date: Thu, 14 Nov 2024 10:14:34 +0100 Subject: [PATCH] [Validator] Remove extension check on content_type validator (#282/#291/#292/#293) --- .../content_type_validator.rb | 35 ++-- .../validators/content_type_validator_test.rb | 187 +++++++++--------- 2 files changed, 117 insertions(+), 105 deletions(-) diff --git a/lib/active_storage_validations/content_type_validator.rb b/lib/active_storage_validations/content_type_validator.rb index 48472a20..7da5491b 100644 --- a/lib/active_storage_validations/content_type_validator.rb +++ b/lib/active_storage_validations/content_type_validator.rb @@ -56,23 +56,32 @@ def set_attachable_cached_values(attachable) @attachable_filename = attachable_filename(attachable).to_s end + # Check if the provided content_type is authorized and not spoofed against + # the file io. def is_valid?(record, attribute, attachable) - extension_matches_content_type?(record, attribute, attachable) && - authorized_content_type?(record, attribute, attachable) && + authorized_content_type?(record, attribute, attachable) && not_spoofing_content_type?(record, attribute, attachable) end - def extension_matches_content_type?(record, attribute, attachable) - return true if !@attachable_filename || !@attachable_content_type - - extension = @attachable_filename.split('.').last - possible_extensions = Marcel::TYPE_EXTS[@attachable_content_type] - return true if possible_extensions && extension.downcase.in?(possible_extensions) - - errors_options = initialize_and_populate_error_options(options, attachable) - add_error(record, attribute, ERROR_TYPES.first, **errors_options) - false - end + # Dead code that we keep here for some time, maybe we will find a solution + # to this check later? (November 2024) + # + # We do not perform any validations against the extension because it is an + # unreliable source of truth. For example, a `.csv` file could have its + # `text/csv` content_type changed to `application/vnd.ms-excel` because + # it had been opened by Excel at some point, making the file extension vs + # file content_type check invalid. + # def extension_matches_content_type?(record, attribute, attachable) + # return true if !@attachable_filename || !@attachable_content_type + + # extension = @attachable_filename.split('.').last + # possible_extensions = Marcel::TYPE_EXTS[@attachable_content_type] + # return true if possible_extensions && extension.downcase.in?(possible_extensions) + + # errors_options = initialize_and_populate_error_options(options, attachable) + # add_error(record, attribute, ERROR_TYPES.first, **errors_options) + # false + # end def authorized_content_type?(record, attribute, attachable) attachable_content_type_is_authorized = @authorized_content_types.any? do |authorized_content_type| diff --git a/test/validators/content_type_validator_test.rb b/test/validators/content_type_validator_test.rb index 571f900e..1a04c7e9 100644 --- a/test/validators/content_type_validator_test.rb +++ b/test/validators/content_type_validator_test.rb @@ -70,98 +70,101 @@ let(:model) { validator_test_class::Check.new(params) } - describe "#extension_matches_content_type?" do - describe "when the attachable content_type and extension cannot match (e.g. 'text/plain' and .png)" do - subject { model.public_send(attribute).attach(file_with_issue) and model } - - let(:attribute) { :extension_content_type_mismatch } - let(:file_with_issue) { extension_content_type_mismatch_file } - let(:error_options) do - { - authorized_types: "PNG", - content_type: file_with_issue[:content_type], - filename: file_with_issue[:filename] - } - end - - it { is_expected_not_to_be_valid } - it { is_expected_to_have_error_message("content_type_invalid", error_options: error_options) } - it { is_expected_to_have_error_options(error_options) } - end - - describe "when the file has 2 extensions (e.g. hello.docx.pdf)" do - describe "and we only allow the first extension content_type (e.g. 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' (docx))" do - subject { model.public_send(attribute).attach(docx_file_with_two_extensions) and model } - - let(:attribute) { :extension_two_extensions_docx } - let(:docx_file_with_two_extensions) do - docx_file.tap do |file| - file[:filename] += ".pdf" - end - end - let(:error_options) do - { - authorized_types: "DOCX", - content_type: docx_file_with_two_extensions[:content_type], - filename: docx_file_with_two_extensions[:filename] - } - end - - it { is_expected_not_to_be_valid } - it { is_expected_to_have_error_message("content_type_invalid", error_options: error_options) } - it { is_expected_to_have_error_options(error_options) } - end - - describe "and we allow the last extension content_type (e.g. 'application/pdf')" do - subject { model.public_send(attribute).attach(docx_file_with_two_extensions) and model } - - let(:attribute) { :extension_two_extensions_pdf } - let(:docx_file_with_two_extensions) do - docx_file.tap do |file| - file[:filename] += ".pdf" - file[:content_type] = "application/pdf" - end - end - - it { is_expected_to_be_valid } - end - end - - describe "when the extension is in uppercase" do - subject { model.public_send(attribute).attach(pdf_file_with_extension_in_uppercase) and model } - - let(:attribute) { :extension_upcase_extension } - let(:pdf_file_with_extension_in_uppercase) do - pdf_file.tap do |file| - file[:filename][".pdf"] = ".PDF" - end - end - - it { is_expected_to_be_valid } - end - - describe "when the extension is missing" do - subject { model.public_send(attribute).attach(pdf_file_without_extension) and model } - - let(:attribute) { :extension_missing_extension } - let(:pdf_file_without_extension) do - pdf_file.tap do |file| - file[:filename][".pdf"] = "" - end - end - let(:error_options) do - { - authorized_types: "PDF", - content_type: pdf_file_without_extension[:content_type], - filename: pdf_file_without_extension[:filename] - } - end - - it { is_expected_not_to_be_valid } - it { is_expected_to_have_error_message("content_type_invalid", error_options: error_options) } - it { is_expected_to_have_error_options(error_options) } - end - end + # Dead code that we keep here for some time, maybe we will find a solution + # to this check later? (November 2024) + # + # describe "#extension_matches_content_type?" do + # describe "when the attachable content_type and extension cannot match (e.g. 'text/plain' and .png)" do + # subject { model.public_send(attribute).attach(file_with_issue) and model } + + # let(:attribute) { :extension_content_type_mismatch } + # let(:file_with_issue) { extension_content_type_mismatch_file } + # let(:error_options) do + # { + # authorized_types: "PNG", + # content_type: file_with_issue[:content_type], + # filename: file_with_issue[:filename] + # } + # end + + # it { is_expected_not_to_be_valid } + # it { is_expected_to_have_error_message("content_type_invalid", error_options: error_options) } + # it { is_expected_to_have_error_options(error_options) } + # end + + # describe "when the file has 2 extensions (e.g. hello.docx.pdf)" do + # describe "and we only allow the first extension content_type (e.g. 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' (docx))" do + # subject { model.public_send(attribute).attach(docx_file_with_two_extensions) and model } + + # let(:attribute) { :extension_two_extensions_docx } + # let(:docx_file_with_two_extensions) do + # docx_file.tap do |file| + # file[:filename] += ".pdf" + # end + # end + # let(:error_options) do + # { + # authorized_types: "DOCX", + # content_type: docx_file_with_two_extensions[:content_type], + # filename: docx_file_with_two_extensions[:filename] + # } + # end + + # it { is_expected_not_to_be_valid } + # it { is_expected_to_have_error_message("content_type_invalid", error_options: error_options) } + # it { is_expected_to_have_error_options(error_options) } + # end + + # describe "and we allow the last extension content_type (e.g. 'application/pdf')" do + # subject { model.public_send(attribute).attach(docx_file_with_two_extensions) and model } + + # let(:attribute) { :extension_two_extensions_pdf } + # let(:docx_file_with_two_extensions) do + # docx_file.tap do |file| + # file[:filename] += ".pdf" + # file[:content_type] = "application/pdf" + # end + # end + + # it { is_expected_to_be_valid } + # end + # end + + # describe "when the extension is in uppercase" do + # subject { model.public_send(attribute).attach(pdf_file_with_extension_in_uppercase) and model } + + # let(:attribute) { :extension_upcase_extension } + # let(:pdf_file_with_extension_in_uppercase) do + # pdf_file.tap do |file| + # file[:filename][".pdf"] = ".PDF" + # end + # end + + # it { is_expected_to_be_valid } + # end + + # describe "when the extension is missing" do + # subject { model.public_send(attribute).attach(pdf_file_without_extension) and model } + + # let(:attribute) { :extension_missing_extension } + # let(:pdf_file_without_extension) do + # pdf_file.tap do |file| + # file[:filename][".pdf"] = "" + # end + # end + # let(:error_options) do + # { + # authorized_types: "PDF", + # content_type: pdf_file_without_extension[:content_type], + # filename: pdf_file_without_extension[:filename] + # } + # end + + # it { is_expected_not_to_be_valid } + # it { is_expected_to_have_error_message("content_type_invalid", error_options: error_options) } + # it { is_expected_to_have_error_options(error_options) } + # end + # end describe ":with" do # validates :with_string, content_type: 'png'