Image attachment content type validation fix for ActiveStorage #4021

cpfergus1 · 2021-04-08T22:02:22Z

Description

This PR aimed to solve the bugs described in Issue #4017 by implementing a validation for content type and to handle a FileNotFound error when ActiveStorage fails to upload to a remote service such as S3.

The validation allows file types jpeg, png, jpg and gif. More file types can be added and removed by editing the accepted_image_types definition found in active_storage_attachment.rb. Validation tests have been added to the image_spec.rb in core.

The handling of the FileNotFound error maybe a temporary fix as we are aiming to discover another pathway to handle the exception and remove the invalid objects simultaneously. Currently when the error is raised on method call, it returns a corrupted image file path.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

cpfergus1 · 2021-04-08T22:22:41Z

Failed on paperclip

Failure/Error: image.attachment.attach(io: invalid_image_file, filename: 'file.txt')

NoMethodError:
  undefined method `attach' for #<Paperclip::Attachment:0x0000557665d96ff8>
./spec/models/spree/image_spec.rb:23:in `block (3 levels) in <top (required)>'

kennyadsl · 2021-04-09T09:06:45Z

core/spec/models/spree/image_spec.rb

+  describe 'validations' do
+    let(:valid_image_file) { File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) }
+    let(:invalid_image_file) { File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'file.txt')) }
+    let(:image) { create(:image) }


I think that using this syntax the specs should work with both adapters:

subject { create(:image, attachment: valid_image_file) }

kennyadsl · 2021-04-09T09:11:40Z

core/app/models/concerns/spree/active_storage_adapter.rb

@@ -107,6 +107,8 @@ def filename

    def url(style = default_style)
      attachment.url(style)
+    rescue ActiveStorage::FileNotFoundError
+      '/images/corrupted-image.png'


This image is not provided by the system, and I think it shouldn't because the frontend and the backend have separate assets folders while this concern code would serve both of them. One alternative could be using a base64 string here with a standard broken image icon, but it's not ideal as well.

On a side note: what about removing this fix from this PR and opening another one just for the other issue?

Removed temporary fix to be brought up in another branch

cpfergus1 · 2021-04-12T01:35:24Z

Removed AS:FileNotFileError catch from this PR. Will patch that fix in another PR for further Discussion.

kennyadsl

Left a couple of suggestions. When done, can you please also squash all the commits into a single one for a better git history when the PR will get merged?

kennyadsl · 2021-04-12T07:46:20Z

core/spec/models/spree/image_spec.rb

@@ -9,6 +9,26 @@
    let(:default_style) { :product }
  end

+  describe 'validations' do


I feel like there's no need for the describe blocks here since we don't have any shared object between the test cases into it. What about just using two different tests as:

it 'is valid when attachment has allowed content type' do image = build(:image, attachment: File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) ) expect(image).to be_valid end it 'is not valid when attachment has restricted content type' do image = build(:image, attachment: File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'file.txt')) ) expect(image).to_not be_valid end

This syntax has been implemented!

core/app/models/spree/image/active_storage_attachment.rb

kennyadsl

Thanks, @cpfergus1! 🎉

waiting-for-dev · 2021-04-13T12:55:38Z

core/app/models/spree/image/active_storage_attachment.rb

+    end
+
+    def accepted_image_types
+      %w(image/jpeg image/jpg image/png image/gif)


What do you think about having the list of accepted mime types in a constant?

Defined in a ENV constant or where would the constant be defined?

No, no... I mean as a constant in Spree::Image::ActiveStorageAttachment, like in:

module Spree::Image::ActiveStorageAttachment ACCEPTED_IMAGE_TYPES = %w(....).freeze end

I mean, it's information that doesn't depend on the state of an object, and it has no computational logic, so in my view, it belongs to the constants domain.

Sorry not having been clearer.

I see, thank you for clarifying

waiting-for-dev · 2021-04-13T12:59:01Z

core/spec/lib/search/base_spec.rb

@@ -22,11 +22,12 @@

  context "when include_images is included in the initialization params" do
    let(:params) { { include_images: true, keyword: @product1.name, taxon: @taxon.id } }
+    let(:image_file) { File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) }


As we are using File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', **) in several place, what do you think about extracting it to a test helper method?

I think this is a great idea, I can certainly implement this now.

waiting-for-dev · 2021-04-13T13:00:16Z

core/spec/models/spree/image_spec.rb

@@ -9,6 +9,18 @@
    let(:default_style) { :product }
  end

+  it 'is valid when attachment has allowed content type' do
+    image = build(:image,
+                  attachment: File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')))


I like to put a blank line between Given/Then/When contexts in the specs... However, I think this is not consistently used in the codebase, so feel free to ignore the comment

I am not noticing this in the rest of the examples, I think I will leave it as is to conform for the time being. Thanks for the suggestion!

waiting-for-dev

Good job @cpfergus1 . I think this validation is definitely the first step to have in place before attempting to fix edge cases 👏 👏

kennyadsl · 2021-04-14T15:21:04Z

core/app/models/spree/image/active_storage_attachment.rb

@@ -3,10 +3,15 @@
 module Spree::Image::ActiveStorageAttachment
  extend ActiveSupport::Concern
  include Spree::ActiveStorageAdapter
+  ACCEPTED_IMAGE_TYPES = %w(image/jpeg image/jpg image/png image/gif).freeze


What about adding an application configuration for this so that it's easier to configure for stores that want to restrict the list further?

On a side note: we can see also how Paperclip is restricting file types and use the same config for Paperclip as well.

I replaced both %() in paperclipattachment and activestorage attachment with an new application configuration: Spree::Config.allowed_image_mime_types.

core/lib/spree/app_configuration.rb

This commit implements validations for content type. Specifically,this new validation ensures image file types .png/.jpeg/.jpg/.gif are the only accepted file formats at this time, but can be edited by altering the accepted_image_types method to allow other formats should the developer desire to implement that functionality. This method was tested by building the objects and testing validity with both a valid image file and an invalid .txt file. Commit History: Created validation tests for images Implemented validation of filetypes Renamed validation Patched file not found error for url Core test update to be compatible with new validation Core test update to be compatible with new validation Updated test to have file names Implemented compatible syntax for AS and Paperclip Separate NoFileError to another branch Refactored validation tests Returned mistakenly deleted white space Removed describe block Changed accepted image types method to constant Implemented application configuration for Image MIME types Added documentation to configuration

Commit History: Changed file name to represent module name Removed unnecessary line breaks

kennyadsl reviewed Apr 9, 2021

View reviewed changes

cpfergus1 changed the title ~~Image attachment content type validation and FileNotFound Fix~~ Image attachment content type validation Fix Apr 12, 2021

cpfergus1 requested a review from kennyadsl April 12, 2021 04:11

kennyadsl reviewed Apr 12, 2021

View reviewed changes

cpfergus1 force-pushed the Issue-4017 branch 3 times, most recently from b5af38f to 60c6b56 Compare April 12, 2021 19:30

kennyadsl approved these changes Apr 13, 2021

View reviewed changes

kennyadsl added this to the 2.11 milestone Apr 13, 2021

kennyadsl added Needs Backport changelog:solidus_core Changes to the solidus_core gem labels Apr 13, 2021

kennyadsl changed the title ~~Image attachment content type validation Fix~~ Image attachment content type validation fix for ActiveStorage Apr 13, 2021

waiting-for-dev reviewed Apr 13, 2021

View reviewed changes

cpfergus1 force-pushed the Issue-4017 branch 2 times, most recently from 08f1a4d to 3ddee2d Compare April 14, 2021 13:21

cpfergus1 requested a review from kennyadsl April 14, 2021 14:24

cpfergus1 force-pushed the Issue-4017 branch from 3ddee2d to 60b68e1 Compare April 14, 2021 14:31

kennyadsl reviewed Apr 14, 2021

View reviewed changes

cpfergus1 force-pushed the Issue-4017 branch from 60b68e1 to fcc1bfe Compare April 14, 2021 16:01

kennyadsl reviewed Apr 14, 2021

View reviewed changes

core/lib/spree/app_configuration.rb Show resolved Hide resolved

cpfergus1 added 2 commits April 14, 2021 10:32

Added a method helper for image specs to DRY code

e7e8ecc

Commit History: Changed file name to represent module name Removed unnecessary line breaks

cpfergus1 force-pushed the Issue-4017 branch from fcc1bfe to e7e8ecc Compare April 14, 2021 16:33

jarednorman approved these changes Apr 14, 2021

View reviewed changes

kennyadsl merged commit e09aef7 into solidusio:master Apr 15, 2021

kennyadsl deleted the Issue-4017 branch April 15, 2021 08:42

kennyadsl removed the Needs Backport label Jun 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image attachment content type validation fix for ActiveStorage #4021

Image attachment content type validation fix for ActiveStorage #4021

cpfergus1 commented Apr 8, 2021 •

edited

Loading

cpfergus1 commented Apr 8, 2021 •

edited

Loading

kennyadsl Apr 9, 2021

kennyadsl Apr 9, 2021

cpfergus1 Apr 12, 2021

cpfergus1 commented Apr 12, 2021 •

edited

Loading

kennyadsl left a comment

kennyadsl Apr 12, 2021 •

edited

Loading

cpfergus1 Apr 12, 2021

kennyadsl left a comment

waiting-for-dev Apr 13, 2021

cpfergus1 Apr 13, 2021

waiting-for-dev Apr 13, 2021

cpfergus1 Apr 13, 2021

waiting-for-dev Apr 13, 2021

cpfergus1 Apr 13, 2021

waiting-for-dev Apr 13, 2021

cpfergus1 Apr 13, 2021

waiting-for-dev left a comment

kennyadsl Apr 14, 2021

kennyadsl Apr 14, 2021

cpfergus1 Apr 14, 2021 •

edited

Loading

Image attachment content type validation fix for ActiveStorage #4021

Image attachment content type validation fix for ActiveStorage #4021

Conversation

cpfergus1 commented Apr 8, 2021 • edited Loading

cpfergus1 commented Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpfergus1 commented Apr 12, 2021 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpfergus1 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

cpfergus1 commented Apr 8, 2021 •

edited

Loading

cpfergus1 commented Apr 8, 2021 •

edited

Loading

cpfergus1 commented Apr 12, 2021 •

edited

Loading

kennyadsl Apr 12, 2021 •

edited

Loading

cpfergus1 Apr 14, 2021 •

edited

Loading