[Validation] spoofing_protection on has_many_attached runs on unchanged attachments

[sakif-imtiaz]

When I've got this on a model, And I add another attach another photo
validates :photos, content_type: { with: %r{\Aimage/.*\z}, spoofing_protection: true }

It'll run the validation on all the existing photos too. It looks like that means it would completely download each of the photos. I'm wondering we can skip blobs that we can tell aren't changed?

I tried adding a reject at the end of ActiveStorageValidations::ASVAttachable#attachables_from_changes that seems to work:

def attachables_from_changes(record, attribute)
  changes = record.attachment_changes[attribute.to_s]
  return [] if changes.blank?

  Array.wrap(
     changes.is_a?(ActiveStorage::Attached::Changes::CreateMany) ? changes.attachables : changes.attachable
  ).uniq.reject do |attachable|
    attachable.try(:changed?) == false
  end
end

[Mth0158]

Hi again @sakif-imtiaz,
You meant that you did something like:

x.attach(a)
x.save

...

x.attach(b)
x.save

And that the validations were run on both a and b right?
As of my understanding, it should only run on a during the first validation, and then b during the second validation because we are using attachables, so things that were 'attached', ie stored in attachment_changes before saved, then cleared out of it after the transaction. See below the relevant code in Rails ActiveStorage Attached Model:

        after_save { attachment_changes[name.to_s]&.save }


        after_commit(on: %i[ create update ]) { attachment_changes.delete(name.to_s).try(:upload) }

https://github.com/rails/rails/blob/096fb3be5a6b92993a0dcffc455c7e32f6f92373/activestorage/lib/active_storage/attached/model.rb#L142C1-L144C100

Or maybe I got your issue wrong?

[evaniainbrooks]

I encountered the same issue since 1.3.5. I don't believe it is due to the spoofing protection, but rather the content type validation.

> object = Object.first
=> #<Object:0x00007f40b0e08fd8>

> object.images
=> #<ActiveStorage::Attached::Many:0x00007f40a7668368>
>object.images.size
=> 4

> object.attachment_changes
=> {}

> object.images.new
=> #<ActiveStorage::Attachment:0x00007f40b0e00518>

> object.images.attach
=> nil

> object.attachment_changes["images"].attachables.count
=> 4

> object.attachment_changes["images"].attachables.map(&:changed?)
=> [false, false, false, false]

As you can see above, I have attempted to attach a bogus/blank image, and that causes all of the existing attachments to show up in the changes list.

Since version 1.3.5 we are downloading the blob in order to determine the content type. So, as above, we download the all of the existing attachments again.

[sakif-imtiaz]

I can't reproduce this anymore ( I promise I checked a couple ways before I wrote this ). But I'm beginning to suspect what I saw was a problem with how we implemented active-storage direct upload.

I'm on vacation now for a couple weeks, I could try and verify it with fresh eyes after that and resubmit if I find anything?

[sakif-imtiaz]

I encountered the same issue since 1.3.5. I don't believe it is due to the spoofing protection, but rather the content type validation.

> object = Object.first
=> #<Object:0x00007f40b0e08fd8>
...
Since version 1.3.5 we are downloading the blob in order to determine the content type. So, as above, we download the all of the existing attachments again.

Oh! I didn't see your comment before my latest one. Then it's not just the crazy pills I've been taking diligently, I probably saw the same thing you did.
Ok I'll try and reproduce in a pristine rails app as soon as I can, thanks for saying you saw it too.

[Mth0158]

@evaniainbrooks @sakif-imtiaz thanks for adding context, I think we need to change the way we validate the content_type again for better perf and not having this issue.
I was sick this week so I couldn't make any code... I'll try to do my best to solve this soon

[Mth0158]

Alright I got it,

    # Attaches one or more +attachables+ to the record.
    #
    # If the record is persisted and unchanged, the attachments are saved to
    # the database immediately. Otherwise, they'll be saved to the DB when the
    # record is next saved.
    #
    #   document.images.attach(params[:images]) # Array of ActionDispatch::Http::UploadedFile objects
    #   document.images.attach(params[:signed_blob_id]) # Signed reference to blob from direct upload
    #   document.images.attach(io: File.open("/path/to/racecar.jpg"), filename: "racecar.jpg", content_type: "image/jpeg")
    #   document.images.attach([ first_blob, second_blob ])
    def attach(*attachables)
      record.public_send("#{name}=", blobs + attachables.flatten)
      if record.persisted? && !record.changed?
        return if !record.save
      end
      record.public_send("#{name}")
    end

https://github.com/rails/rails/blob/main/activestorage/lib/active_storage/attached/many.rb#L4

Therefore it adds previously saved blobs to the attachment_changes down the road, will fix this easily

[Mth0158]

Hi @evaniainbrooks @sakif-imtiaz
I have merged #332 that should solve part of your issue (plus enhancing perf). It will be released as part of 1.3.6, let me know if the issue persists :d

[Mth0158]

Hi @evaniainbrooks & @sakif-imtiaz,
FYI, the v2 of the gem will definitely fix this perf issue thanks to this PR #341.
It should be released pretty soon (1 week or 2 max), let me know if you still have this issue afterwards :)