diff --git a/Gemfile.lock b/Gemfile.lock index 23d22a0797..14784f94b4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -104,11 +104,12 @@ GEM rails-html-sanitizer (~> 1.6) active_model_serializers (0.8.4) activemodel (>= 3.0) - active_storage_validations (1.1.4) - activejob (>= 5.2.0) - activemodel (>= 5.2.0) - activestorage (>= 5.2.0) - activesupport (>= 5.2.0) + active_storage_validations (3.0.2) + activejob (>= 6.1.4) + activemodel (>= 6.1.4) + activestorage (>= 6.1.4) + activesupport (>= 6.1.4) + marcel (>= 1.0.3) activejob (7.1.6) activesupport (= 7.1.6) globalid (>= 0.3.6) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 1211ed61ee..bf8e52d56d 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -111,14 +111,14 @@ class Enterprise < ApplicationRecord end validates :logo, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :promo_image, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :white_label_logo, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :terms_and_conditions, content_type: { in: "application/pdf", message: I18n.t(:enterprise_terms_and_conditions_type_error), diff --git a/app/models/enterprise_group.rb b/app/models/enterprise_group.rb index e6208e3000..cb2252b2d4 100644 --- a/app/models/enterprise_group.rb +++ b/app/models/enterprise_group.rb @@ -29,11 +29,11 @@ class EnterpriseGroup < ApplicationRecord has_one_attached :promo_image, service: image_service validates :logo, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :promo_image, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES scope :by_position, -> { order('position ASC') } scope :on_front_page, -> { where(on_front_page: true) } diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb index 350b1c5b4c..431b1679b0 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -2,6 +2,8 @@ module Spree class Image < Asset + ACCEPTED_CONTENT_TYPES = %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + has_one_attached :attachment, service: image_service do |attachment| attachment.variant :mini, resize_to_fill: [48, 48] attachment.variant :small, resize_to_fill: [227, 227] @@ -11,8 +13,8 @@ module Spree validates :attachment, attached: true, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ACCEPTED_CONTENT_TYPES validate :no_attachment_errors def self.default_image_url(size) diff --git a/app/services/image_importer.rb b/app/services/image_importer.rb index 9cef10548a..78f525323a 100644 --- a/app/services/image_importer.rb +++ b/app/services/image_importer.rb @@ -11,7 +11,9 @@ class ImageImporter image = Spree::Image.create do |img| PrivateAddressCheck.only_public_connections do - img.attachment.attach(io: valid_url.open, filename:, metadata:) + io = valid_url.open + content_type = Marcel::MimeType.for(io) + img.attachment.attach(io:, filename:, metadata:, content_type:) end end product.image = image if image diff --git a/config/locales/en.yml b/config/locales/en.yml index 4e6dd4dcde..246c2e2f71 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -138,26 +138,66 @@ en: # Used by active_storage_validations errors: messages: - content_type_invalid: "has an invalid content type" - file_size_out_of_range: "size %{file_size} is not between required range" - limit_out_of_range: "total number is out of range" - image_metadata_missing: "is not a valid image" - dimension_min_inclusion: "must be greater than or equal to %{width} x %{height} pixel." - dimension_max_inclusion: "must be less than or equal to %{width} x %{height} pixel." - dimension_width_inclusion: "width is not included between %{min} and %{max} pixel." - dimension_height_inclusion: "height is not included between %{min} and %{max} pixel." - dimension_width_greater_than_or_equal_to: "width must be greater than or equal to %{length} pixel." - dimension_height_greater_than_or_equal_to: "height must be greater than or equal to %{length} pixel." - dimension_width_less_than_or_equal_to: "width must be less than or equal to %{length} pixel." - dimension_height_less_than_or_equal_to: "height must be less than or equal to %{length} pixel." - dimension_width_equal_to: "width must be equal to %{length} pixel." - dimension_height_equal_to: "height must be equal to %{length} pixel." - aspect_ratio_not_square: "must be a square image" - aspect_ratio_not_portrait: "must be a portrait image" - aspect_ratio_not_landscape: "must be a landscape image" - aspect_ratio_is_not: "must have an aspect ratio of %{aspect_ratio}" - aspect_ratio_unknown: "has an unknown aspect ratio" - image_not_processable: "is not a valid image" + content_type_invalid: + one: "has an invalid content type (authorized content type is %{authorized_human_content_types})" + other: "has an invalid content type (authorized content types are %{authorized_human_content_types})" + content_type_spoofed: + one: "has a content type that is not equivalent to the one that is detected through its content (authorized content type is %{authorized_human_content_types})" + other: "has a content type that is not equivalent to the one that is detected through its content (authorized content types are %{authorized_human_content_types})" + file_size_not_less_than: "file size must be less than %{max} (current size is %{file_size})" + file_size_not_less_than_or_equal_to: "file size must be less than or equal to %{max} (current size is %{file_size})" + file_size_not_greater_than: "file size must be greater than %{min} (current size is %{file_size})" + file_size_not_greater_than_or_equal_to: "file size must be greater than or equal to %{min} (current size is %{file_size})" + file_size_not_between: "file size must be between %{min} and %{max} (current size is %{file_size})" + file_size_not_equal_to: "file size must be equal to %{exact} (current size is %{file_size})" + total_file_size_not_less_than: "total file size must be less than %{max} (current size is %{total_file_size})" + total_file_size_not_less_than_or_equal_to: "total file size must be less than or equal to %{max} (current size is %{total_file_size})" + total_file_size_not_greater_than: "total file size must be greater than %{min} (current size is %{total_file_size})" + total_file_size_not_greater_than_or_equal_to: "total file size must be greater than or equal to %{min} (current size is %{total_file_size})" + total_file_size_not_between: "total file size must be between %{min} and %{max} (current size is %{total_file_size})" + total_file_size_not_equal_to: "total file size must be equal to %{exact} (current size is %{total_file_size})" + duration_not_less_than: "duration must be less than %{max} (current duration is %{duration})" + duration_not_less_than_or_equal_to: "duration must be less than or equal to %{max} (current duration is %{duration})" + duration_not_greater_than: "duration must be greater than %{min} (current duration is %{duration})" + duration_not_greater_than_or_equal_to: "duration must be greater than or equal to %{min} (current duration is %{duration})" + duration_not_between: "duration must be between %{min} and %{max} (current duration is %{duration})" + duration_not_equal_to: "duration must be equal to %{exact} (current duration is %{duration})" + limit_out_of_range: + zero: "no files attached (must have between %{min} and %{max} files)" + one: "only 1 file attached (must have between %{min} and %{max} files)" + other: "total number of files must be between %{min} and %{max} files (there are %{count} files attached)" + limit_min_not_reached: + zero: "no files attached (must have at least %{min} files)" + one: "only 1 file attached (must have at least %{min} files)" + other: "%{count} files attached (must have at least %{min} files)" + limit_max_exceeded: + zero: "no files attached (maximum is %{max} files)" + one: "too many files attached (maximum is %{max} files, got %{count})" + other: "too many files attached (maximum is %{max} files, got %{count})" + attachment_missing: "is missing its attachment" + media_metadata_missing: "is not a valid media file" + dimension_min_not_included_in: "must be greater than or equal to %{width} x %{height} pixels" + dimension_max_not_included_in: "must be less than or equal to %{width} x %{height} pixels" + dimension_width_not_included_in: "width is not included between %{min} and %{max} pixels" + dimension_height_not_included_in: "height is not included between %{min} and %{max} pixels" + dimension_width_not_greater_than_or_equal_to: "width must be greater than or equal to %{length} pixels" + dimension_height_not_greater_than_or_equal_to: "height must be greater than or equal to %{length} pixels" + dimension_width_not_less_than_or_equal_to: "width must be less than or equal to %{length} pixels" + dimension_height_not_less_than_or_equal_to: "height must be less than or equal to %{length} pixels" + dimension_width_not_equal_to: "width must be equal to %{length} pixels" + dimension_height_not_equal_to: "height must be equal to %{length} pixels" + aspect_ratio_not_square: "must be square (current file is %{width}x%{height}px)" + aspect_ratio_not_portrait: "must be portrait (current file is %{width}x%{height}px)" + aspect_ratio_not_landscape: "must be landscape (current file is %{width}x%{height}px)" + aspect_ratio_not_x_y: "must be %{authorized_aspect_ratios} (current file is %{width}x%{height}px)" + aspect_ratio_invalid: "has an invalid aspect ratio (valid aspect ratios are %{authorized_aspect_ratios})" + file_not_processable: "is not identified as a valid media file" + pages_not_less_than: "page count must be less than %{max} (current page count is %{pages})" + pages_not_less_than_or_equal_to: "page count must be less than or equal to %{max} (current page count is %{pages})" + pages_not_greater_than: "page count must be greater than %{min} (current page count is %{pages})" + pages_not_greater_than_or_equal_to: "page count must be greater than or equal to %{min} (current page count is %{pages})" + pages_not_between: "page count must be between %{min} and %{max} (current page count is %{pages})" + pages_not_equal_to: "page count must be equal to %{exact} (current page count is %{pages})" not_found: title: "The page you were looking for doesn't exist (404)" message_html: "Please try again diff --git a/engines/dfc_provider/app/services/image_builder.rb b/engines/dfc_provider/app/services/image_builder.rb index 06ab056e0c..e9da4aa36a 100644 --- a/engines/dfc_provider/app/services/image_builder.rb +++ b/engines/dfc_provider/app/services/image_builder.rb @@ -19,12 +19,14 @@ class ImageBuilder < DfcBuilder def self.import(image_link) url = URI.parse(image_link) - filename = File.basename(image_link) + filename = File.basename(url.path) metadata = { custom: { origin: image_link } } Spree::Image.new.tap do |image| PrivateAddressCheck.only_public_connections do - image.attachment.attach(io: url.open, filename:, metadata:) + io = url.open + content_type = Marcel::MimeType.for(io) + image.attachment.attach(io:, filename:, metadata:, content_type:) end end rescue StandardError diff --git a/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb b/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb index 815a1c6897..07a8d1bbda 100644 --- a/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb +++ b/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb @@ -251,7 +251,7 @@ RSpec.describe SuppliedProductImporter do supplied_product.isVariantOf << tomatoes imported_product = importer.import_variant(supplied_product, supplier).product - expect(imported_product.image.attachment.filename).to eq "tomato.png?v=1" + expect(imported_product.image.attachment.filename).to eq "tomato.png" expect { importer.import_variant(supplied_product, supplier).product @@ -266,7 +266,7 @@ RSpec.describe SuppliedProductImporter do } .to change { imported_product.image } - expect(imported_product.image.attachment.filename).to eq "tomato.png?v=2" + expect(imported_product.image.attachment.filename).to eq "tomato.png" end context "when spree_product_uri doesn't match the server host" do diff --git a/spec/controllers/api/v0/product_images_controller_spec.rb b/spec/controllers/api/v0/product_images_controller_spec.rb index 5fe0e4bb24..ac46c412f4 100644 --- a/spec/controllers/api/v0/product_images_controller_spec.rb +++ b/spec/controllers/api/v0/product_images_controller_spec.rb @@ -45,7 +45,8 @@ RSpec.describe Api::V0::ProductImagesController do expect(response).to have_http_status :unprocessable_entity expect(product_without_image.image).to be_nil expect(json_response["id"]).to eq nil - expect(json_response["errors"]).to include "Attachment has an invalid content type" + expect(json_response["errors"]).to include "Attachment is " \ + "not identified as a valid media file" end end end diff --git a/spec/fixtures/files/logo.bmp b/spec/fixtures/files/logo.bmp new file mode 100644 index 0000000000..f03e68b138 Binary files /dev/null and b/spec/fixtures/files/logo.bmp differ diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb index f53e72b021..0f3152abd5 100644 --- a/spec/models/enterprise_spec.rb +++ b/spec/models/enterprise_spec.rb @@ -396,24 +396,8 @@ RSpec.describe Enterprise do let(:content_type) { 'image/png' } before do - blob = instance_double( - "ActiveStorage::Blob", - filename: ActiveStorage::Filename.new('white-label-logo.png'), - content_type:, - byte_size: 1024 - ) - - # InstanceDouble is not working for attachment case as the blob method is not yet defined - # on instantiation. - attachment = double( - "ActiveStorage::Attached::One", - blank?: false, - attached?: true, - blob: - ) - - allow(enterprise) - .to receive(:white_label_logo).and_return(attachment) + blob = Rack::Test::UploadedFile.new('spec/fixtures/files/logo.png', content_type) + enterprise.white_label_logo.attach(blob) end context 'when the file attached is a PNG image' do @@ -424,6 +408,12 @@ RSpec.describe Enterprise do context 'when the file attached is a BMP image' do let(:content_type) { 'image/bmp' } + + before do + blob = Rack::Test::UploadedFile.new('spec/fixtures/files/logo.bmp', content_type) + enterprise.white_label_logo.attach(blob) + end + it 'is not valid' do expect(enterprise).not_to be_valid end diff --git a/spec/services/image_importer_spec.rb b/spec/services/image_importer_spec.rb index 67fa4793d2..d9c7e5cd0b 100644 --- a/spec/services/image_importer_spec.rb +++ b/spec/services/image_importer_spec.rb @@ -7,7 +7,7 @@ RSpec.describe ImageImporter do let(:product) { create(:product) } describe "#import" do - it "downloads from the Internet", :vcr do + it "downloads from the Internet", :vcr, :aggregate_failures do expect { subject.import(ofn_url, product) }.to change { diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 8f10416261..93a699892b 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -604,7 +604,7 @@ RSpec.describe ' click_button "Create" expect(page).to have_text "Attachment has an invalid content type" - expect(page).to have_text "Attachment is not a valid image" + expect(page).to have_text "Attachment is not identified as a valid media file" end it "deleting product images" do diff --git a/spec/system/admin/products_v3/update_spec.rb b/spec/system/admin/products_v3/update_spec.rb index 76e96b60eb..320f15d3b2 100644 --- a/spec/system/admin/products_v3/update_spec.rb +++ b/spec/system/admin/products_v3/update_spec.rb @@ -770,12 +770,12 @@ RSpec.describe 'As an enterprise user, I can update my products' do end end - it 'shows a modal telling not a valid image when uploading a non valid image file' do + it 'shows a modal telling not a valid image when uploading an invalid image file' do within ".reveal-modal" do attach_file 'image[attachment]', Rails.public_path.join('invalid_image.jpg'), visible: false - expect(page).to have_content /Attachment is not a valid image/ + expect(page).to have_content /Attachment is not identified as a valid media file/ end end end