Active Storage - Direct Upload Validation #41178

ghiculescu · 2021-01-19T19:15:35Z

This PR is a proposed architecture for validation in Active Storage. It builds on @georgeclaghorn's proposed architecture here: #35390 (comment)

It leans heavily on #35390 - most of the code from that PR is included in it, with some light refactoring.

How it works

There were four high level points in the proposed architecture:

To start, we’d offer validations for the most commonly-validated blob attributes, content_type and byte_size

I included the validators in #35390 mostly unchanged.

The direct upload preflight blob creation request already sends content_type and byte_size attributes. We’d validate these attributes upfront, refusing to create a blob record and return a signed direct upload URL if they’re invalid.

To do this, Active Storage needs to know the model to run validations for. This is passed as a new data attribute, data-direct-upload-model. It's handled automatically if you use FormBuilder#file_field, or you can add it manually.

This attribute eventually makes its way to ActiveStorage::Blob#valid_with?, which returns true if:

There's no Active Storage validators on the model, or:
All Active Storage validators would pass with the given blob.

A validator is an "Active Storage validator" if it's a subclass of ActiveStorage::Validations::BaseValidator.

Tests for this are here.

because only server-side validation is secure, we’d need to validate content_type again on attach

I haven't explicitly tested for this yet but I believe #35390 handles it.

I don’t think we need to validate byte_size again on attach because signed direct upload URLs account for the Content-Length header. If I’m wrong about that and we have to validate byte_size again, fine. Same as above.

If the validator is defined, we validate it :)

I'm adding @abhchand as a co-author since I used so much of #35390

Co-authored-by: Abhishek Chandrasekhar me@abhchand.me

ghiculescu · 2021-01-19T19:25:26Z

activestorage/app/models/active_storage/blob.rb

+    return true if klass_name.nil?
+    return true unless defined?(ActiveRecord::Base)
+
+    model = ActiveRecord::Base.const_get(klass_name) rescue nil


I don't love this, but can't think of a safer way to get the model here.

louim · 2021-01-19T19:35:35Z

Hey @ghiculescu,

See here how to setup the commit(s) with co-authoring. In your case, that would mean adding: Co-authored-by: Abhishek Chandrasekhar <me@abhchand.me> at the bottom of the commits messages you want to co-author.

ghiculescu · 2021-01-19T23:08:36Z

@louim done - thanks!

activestorage/app/models/active_storage/blob.rb

abhchand · 2021-02-09T02:58:28Z

Hey @ghiculescu , just stopping by to say thanks for including me as an author on the commit. That's nice of you!

If I find time between work and personal life I'll try to weigh in here, but those two things are keeping me pretty busy at the moment :)

ghiculescu · 2021-02-09T17:56:15Z

@georgeclaghorn I would love to include this in Rails 7. Do you have any feedback on the direction taken here or anything else you'd like to see included? (let me know if there's a better person to direct this question to)

rails-bot · 2021-07-29T14:34:21Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

Co-authored-by: Abhishek Chandrasekhar <me@abhchand.me>

brunoprietog · 2021-09-05T08:11:42Z

Any news on this? I would love to have it in rails 7

reckerswartz · 2021-09-24T22:52:00Z

@ghiculescu can you also add other validators?

like

presence: true
limit: { min: 1, max: 3 }

tomrossi7 · 2021-10-28T18:41:57Z

Anything I can do to help get this PR moving along? We would love to see this get merged into Rails and are happy to commit resources to help!

ghiculescu · 2021-10-28T19:41:22Z

@tomrossi7 if you're interested in fixing up the conflicts and trying to move this forward, I can give you access to my fork?

attachment presence validator

seanpdoyle · 2021-12-21T20:51:24Z

actionview/lib/action_view/helpers/form_tag_helper.rb

+            if options[:direct_upload_model]
+              options["data-direct-upload-model"] = options.delete(:direct_upload_model)
+            elsif options[:object]
+              options["data-direct-upload-model"] = options[:object].class.name


Could this be a signed reference (like what Turbo Rails does for Stream names) to prevent tampering?

Similarly, could the model: Post option be signed in the same way?

Would also need a helper method for when not using FormBuilder:

<input type=file data-direct-upload-url="<%= rails_direct_uploads_url %>" data-direct-upload-model="<%= rails_direct_uploads_signed_model('Post') %>" />

I also don't see how just the model is sufficient. Shouldn't it be model.attribute as models can have more than one attached attribute:

class User < ApplicationRecord has_one_attached :avatar has_one_attached :cover_photo has_many_attached :highlights end

Could this be a signed reference (like what Turbo Rails does for Stream names) to prevent tampering?

Similarly, could the model: Post option be signed in the same way?

Yes, it could use ActiveStorage.verifier.

Could do something like:

if options[:direct_upload_model_and_attribute] # Note change in name from direct_upload_model to direct_upload_model_and_attribute options["data-direct-upload-model-and-attribute"] = ActiveStorage.verifier.generate(options.delete(:direct_upload_model_and_attribute))

ActiveStorage.verifier.generate("User.avatar") => "BAhJIhBVc2VyLmF2YXRhcgY6BkVU--a6f0ee7e289bbf19f91634986e33c15298a6f408" ActiveStorage.verifier.verified("BAhJIhBVc2VyLmF2YXRhcgY6BkVU--a6f0ee7e289bbf19f91634986e33c15298a6f408") => "User.avatar"

rails-bot · 2022-03-21T20:55:06Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

seanabrahams

Let's see if we can make some progress on this.

seanabrahams · 2022-06-01T18:10:21Z

guides/source/active_storage_overview.md

+    validates :avatar, attachment_byte_content_type: { in: %w[image/jpeg image/png] }
+    validates :avatar, attachment_byte_content_type: { not: %w[application/pdf] }


Should this be:

validates :avatar, attachment_content_type: { in: %w[image/jpeg image/png] } validates :avatar, attachment_content_type: { not: %w[application/pdf] }

?

seanabrahams · 2022-06-01T18:10:59Z

guides/source/active_storage_overview.md

+    validates :avatar, attachment_byte_content_type: %w[image/jpeg image/png]
+    validates :avatar, attachment_byte_content_type: "image/jpeg"


validates :avatar, attachment_content_type: %w[image/jpeg image/png] validates :avatar, attachment_content_type: "image/jpeg"

?

seanabrahams · 2022-06-01T18:14:28Z

actionview/lib/action_view/helpers/form_tag_helper.rb

+            if options[:direct_upload_model]
+              options["data-direct-upload-model"] = options.delete(:direct_upload_model)
+            elsif options[:object]
+              options["data-direct-upload-model"] = options[:object].class.name


Would also need a helper method for when not using FormBuilder:

<input type=file data-direct-upload-url="<%= rails_direct_uploads_url %>" data-direct-upload-model="<%= rails_direct_uploads_signed_model('Post') %>" />

seanabrahams · 2022-06-01T18:49:02Z

actionview/lib/action_view/helpers/form_tag_helper.rb

+            if options[:direct_upload_model]
+              options["data-direct-upload-model"] = options.delete(:direct_upload_model)
+            elsif options[:object]
+              options["data-direct-upload-model"] = options[:object].class.name


I also don't see how just the model is sufficient. Shouldn't it be model.attribute as models can have more than one attached attribute:

class User < ApplicationRecord has_one_attached :avatar has_one_attached :cover_photo has_many_attached :highlights end

ghiculescu · 2022-06-02T00:32:42Z

@seanabrahams i don’t have much capacity to work on this at the moment, would you like to take it over?

seanabrahams · 2022-06-08T16:25:30Z

@ghiculescu I'll see what I can do 👍

rails-bot bot added actiontext actionview activestorage docs labels Jan 19, 2021

ghiculescu force-pushed the as-direct-validation branch 6 times, most recently from 921fb6e to 892f685 Compare January 19, 2021 19:24

ghiculescu marked this pull request as ready for review January 19, 2021 19:24

ghiculescu commented Jan 19, 2021

View reviewed changes

ghiculescu force-pushed the as-direct-validation branch 5 times, most recently from 2e99dda to c1a0e81 Compare January 20, 2021 00:12

ghiculescu commented Jan 21, 2021

View reviewed changes

activestorage/app/models/active_storage/blob.rb Outdated Show resolved Hide resolved

ghiculescu force-pushed the as-direct-validation branch 2 times, most recently from 912f87d to c898d46 Compare January 21, 2021 22:41

ghiculescu force-pushed the as-direct-validation branch from c898d46 to 72b9109 Compare February 9, 2021 18:00

georgeclaghorn self-assigned this Mar 5, 2021

ghiculescu force-pushed the as-direct-validation branch from 72b9109 to 15426e6 Compare April 13, 2021 20:28

georgeclaghorn removed their assignment Apr 30, 2021

rails-bot bot added the stale label Jul 29, 2021

rails-bot bot removed the stale label Jul 29, 2021

ghiculescu force-pushed the as-direct-validation branch 2 times, most recently from af9cb12 to fbe8adf Compare July 29, 2021 15:39

Active Storage - Direct Upload Validation

280397c

Co-authored-by: Abhishek Chandrasekhar <me@abhchand.me>

ghiculescu force-pushed the as-direct-validation branch from fbe8adf to 280397c Compare July 29, 2021 15:40

Ricoch mentioned this pull request Aug 10, 2021

[Questions] how to validate file size rootstrap/active-storage-base64#62

Closed

attachment presence validator

f200140

Merge pull request #1 from reckerswartz/extra_validation

32cfe3a

attachment presence validator

seanpdoyle reviewed Dec 21, 2021

View reviewed changes

rails-bot bot added the stale label Mar 21, 2022

rails-bot bot closed this Mar 28, 2022

seanabrahams reviewed Jun 1, 2022

View reviewed changes

seanabrahams mentioned this pull request Jun 21, 2022

ActiveStorage Validations: content_type, byte_size, and presence Countable-us/rails#1

Draft

		validates :avatar, attachment_byte_content_type: { in: %w[image/jpeg image/png] }
		validates :avatar, attachment_byte_content_type: { not: %w[application/pdf] }

Active Storage - Direct Upload Validation #41178

Active Storage - Direct Upload Validation #41178

Uh oh!

Conversation

ghiculescu commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghiculescu Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

louim commented Jan 19, 2021

Uh oh!

ghiculescu commented Jan 19, 2021

Uh oh!

Uh oh!

abhchand commented Feb 9, 2021

Uh oh!

ghiculescu commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rails-bot bot commented Jul 29, 2021

Uh oh!

brunoprietog commented Sep 5, 2021

Uh oh!

reckerswartz commented Sep 24, 2021

Uh oh!

tomrossi7 commented Oct 28, 2021

Uh oh!

ghiculescu commented Oct 28, 2021

Uh oh!

seanpdoyle Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanabrahams Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

seanabrahams Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

seanabrahams Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rails-bot bot commented Mar 21, 2022

Uh oh!

seanabrahams left a comment

Choose a reason for hiding this comment

Uh oh!

seanabrahams Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

seanabrahams Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

seanabrahams Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

seanabrahams Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

ghiculescu commented Jun 2, 2022

Uh oh!

seanabrahams commented Jun 8, 2022

Uh oh!

Uh oh!

ghiculescu commented Jan 19, 2021 •

edited

Loading

ghiculescu commented Feb 9, 2021 •

edited

Loading

seanpdoyle Dec 21, 2021 •

edited

Loading

seanabrahams Jun 1, 2022 •

edited

Loading