Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Validator] Remove extension check on content_type validator (#282/#291/#292/#293) #296

Merged
merged 1 commit into from
Nov 14, 2024
Merged

[Validator] Remove extension check on content_type validator (#282/#291/#292/#293) #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions lib/active_storage_validations/content_type_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
187 changes: 95 additions & 92 deletions test/validators/content_type_validator_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
Loading