Pass service_name param to DirectUploadsController #38957

DmitryTsepelev · 2020-04-15T11:16:44Z

Summary

While working on configuring individual services per attachment (#34935), I completely forgot about Direct Uploads (thanks @scottjg for finding it early in #38940). The problem is that during direct uploading blob is created without connection with the attachment (and record), so we cannot figure out, what service was configured, which leads to the situation when blob is uploaded to the default storage.

Other Information

I think we could solve it by passing data-direct-upload-service to the form input and submitting it to the DirectUploadsController#create. It's not safe to let the client know what services we have configured, so service name is hidden using ActiveStorage.verifier.generate.

Please let me know if there is any easy way to access record during blob creation or if my approach is not secure enough 🙂

scottjg

Seems reasonable to me (Granted I’m not a rails contributor or qualified to approve it)

DmitryTsepelev · 2020-04-28T19:14:47Z

@georgeclaghorn may I ask you to take a look at this issue? 🙂

rails-bot · 2020-07-27T19:23:36Z

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.

DmitryTsepelev · 2020-07-28T06:29:57Z

It's still an issue

neiljohari · 2020-07-29T20:55:02Z

I would like to see this merged, my project is currently affected by #38940 and this seems to resolve it!

rails-bot · 2020-10-27T21:37:27Z

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.

neiljohari · 2020-10-27T21:38:46Z

Still an issue

adeeb1 · 2020-11-02T18:37:16Z

I just noticed this issue and was about to submit a PR for it. Thank you for doing this. I really need this functionality, and it'd be half-baked if this were not also included in the 6.1 release. Can someone please review?

DmitryTsepelev · 2020-11-03T12:59:44Z

@georgeclaghorn May I ask you for a review? I guess many people will be happy to see it in the next release 🙂

activestorage/app/controllers/active_storage/direct_uploads_controller.rb

scottjg · 2020-11-21T05:34:16Z

hey @gmcgibbon -- sorry to bug you, but i saw you helped review the original pr that introduced this great feature. maybe you could help review this fix for it.

gmcgibbon · 2020-11-21T06:01:27Z

This looks like a more complete version of #37501. @javan had concerns over service name injection, so we may have to refactor the approach slightly. I'll give this some more thought. Thank you for re-raising this, we really should support multi-service in direct uploads.

DmitryTsepelev · 2020-12-04T12:08:06Z

@artisr did you run the upgrade migrations? 🙂

gmcgibbon · 2021-01-04T20:58:20Z

actionview/lib/action_view/helpers/form_tag_helper.rb

+              attachment_reflection = options[:object].class.reflect_on_attachment(name)
+
+              if service_name = attachment_reflection.options[:service_name]
+                options["data-direct-upload-service"] = ActiveStorage.verifier.generate(service_name)


Encrypting the service name is good, but it is still susceptible to injection in other forms. Since ActiveStorage.verifier.generate("something") == ActiveStorage.verifier.generate("something") is always true, the client doesn't know what the value is, but they could save the encrypted service token and send it in another request meant for another service.

You can mitigate the potential damage a client could do by adding an expires_in option to the generate call. This has serious downsides though.

The client is now time-boxed to submitting the direct upload.

The client can still use the token with expiry for other forms/requests while time permits.

I can't think of a clear workaround for this, but the ideal solution would key the encrypted service token to the form it is meant for. Sort of like how per_form_csrf_tokens works.

Keeping in mind the option to use expires_in, I've spent some trying to figure out as a form unique identifier and how to access it from #file_field_tag without passing any additional options to it.

attachment_reflection, attachment_name and service_nameare available in the context. I've tried to build a key as a #{attachment_name}##{service_name}, but there is no way to check that attachment_name matches token in the DirectUploadsController#create. I guess I could do some validations at the moment when Attachment is created (e.g., add the validation to the Attachment to make sure, that record.class.reflect_on_attachment(name).options[:service_name] is nil or equals to Blob#service_name) but it sounds brittle.

Any ideas?

You could encrypt the following:

{ class_name: options[:object].class, association_name: name }.to_json

and then in the controller you would decrypt and constantize the class to get the service_name from the current attachment reflection. This effectively allows the client to select the attachment, and not the service_name, but that is just be pushing the problem one level deeper. The token with attachment association metadata could still be injected in other forms, but you are limited to only services in use.

Validation on attachment to match the blob service_name with the one in the attachment reflection sounds like a good idea, but might be addressing the problem at the wrong time (the blob is already persisted and uploaded).

Right now this feels like this is an impossible problem. As soon as we give the client a multiple tokens for a general uploading endpoint (to create a blob that is meant for a specific attachment), we have a problem. The controller and/or models will likely have to change more considerably in order to support this.

I've committed an attempt to solve this (sorry, it took a bit longer than I expected). This is how I expect it to work (most part of token generation code was shamelessly stolen from forgery protection module):

We generate token based on session[:_direct_upload_token] (exactly the same way as CSRF token works) and include service name, class name and attachment name (service name can be decrypted later).

We add service_name and token to the form input and submit them along with upload.

In the upload controller we decrypt token and make sure it matches token we expect (based on input data attributes, so it's impossible to use another user's token)

If everything is fine we extract service_name from token and believe that it's a correct service_name

The only way to abuse this token is to use it to upload the same attachment of the same entity to the different form (which sounds like a very rare scenario). Everything is in the separate commit to make reviewing easier, I'll squash them later if we decide that this approach works. What do you think?

@gmcgibbon @georgeclaghorn may I ask you to take a look at this one more time?

Why go through the effort of encrypting the service name? Am I not right in assuming that the service_name is included in the response body for DirectUploadsController#create anyways? So it's already exposed to the client?

@DmitryTsepelev @gmcgibbon

santib · 2021-02-14T19:38:15Z

@dhh would you mind weighing in here? This looks to me like an important bug in Rails 6.1, because people could be uploading files to the incorrect service without noticing.

t27duck · 2021-11-04T12:20:25Z

Would these javascript examples need to be updated in the guide as well since the function definition changed with this?

https://github.com/rails/rails/blob/main/guides/source/active_storage_overview.md?plain=1#L1153
https://github.com/rails/rails/blob/main/guides/source/active_storage_overview.md?plain=1#L1182

DmitryTsepelev · 2021-11-08T19:04:11Z

Good point! Updated docs

gmcgibbon

LGTM after squashing your changes 👍

Thanks for your patience on this ❤️

gmcgibbon · 2021-11-13T01:14:14Z

actiontext/app/assets/javascripts/actiontext.js

@@ -506,14 +506,16 @@ var activestorage = {exports: {}};
    }
  }
  class BlobRecord {
-    constructor(file, checksum, url) {
+    constructor(file, checksum, url, directUploadToken, attachmentName) {


Not entirely sure what the rules are regarding changing public JavaScript API, but the amount of positional parameters here is too much. We should make this a object/hash, but we can figure this later. Same with DirectUpload.

neiljohari · 2021-11-15T05:55:36Z

Congrats on getting this merged in!

I've been following this for 2 years, and I am so happy to see the persistence pay off

One day when I get a chance, I can go remove my monkey patches in old code that worked around this 🎉

tbcooney · 2021-12-20T19:46:26Z

This doesn't jive well with Turbo. We allow users to leave notes on customer profiles and support for tagging users with @mentions with ActionText, so the form includes a rich_text_field. When a new note is created, it'll trigger the call to broadcast_later which enqueues a Turbo::Streams::ActionBroadcastJob for the prepend.

@DmitryTsepelev, it seems that this PR generates tokens with the session to now support multiple upload services, however, there won't be a session for Turbo::Streams::ActionBroadcastJob, and this raises a fatal error. Check out the trace in the photo below,

tbcooney · 2022-01-15T19:02:09Z

@DmitryTsepelev Can you revert this for 7.0.2? This is busted and we're nearing the one month mark after missing a fix for 7.0.1.

DmitryTsepelev · 2022-01-15T20:13:28Z

@tbcooney I cannot revert anything since I'm just a commiter 🙂 What I can do is to implement the temp solution described here, but I failed to fit it into my last week

* Revert "Pass service_name param to DirectUploadsController" This reverts commit 193289d. * Revert "Multi-service direct uploads in Action Text attachment uploads" This reverts commit 0b69ad4.

rails-bot bot added actionview activestorage labels Apr 15, 2020

DmitryTsepelev force-pushed the store-per-attachment-direct-upload branch from 384e95c to aac15a9 Compare April 15, 2020 11:36

DmitryTsepelev mentioned this pull request Apr 15, 2020

ActiveStorage direct uploads can't specify a service_name? #38940

Open

scottjg reviewed Apr 17, 2020

View reviewed changes

rails-bot bot added the stale label Jul 27, 2020

rails-bot bot removed the stale label Jul 28, 2020

rails-bot bot added the stale label Oct 27, 2020

rails-bot bot removed the stale label Oct 27, 2020

adeeb1 reviewed Nov 8, 2020

View reviewed changes

activestorage/app/controllers/active_storage/direct_uploads_controller.rb Show resolved Hide resolved

gmcgibbon self-requested a review November 21, 2020 06:01

gmcgibbon reviewed Jan 4, 2021

View reviewed changes

DmitryTsepelev force-pushed the store-per-attachment-direct-upload branch 3 times, most recently from 0491c58 to 30ec67c Compare January 13, 2021 13:54

Base automatically changed from master to main January 14, 2021 17:01

clwy-cn mentioned this pull request Jan 26, 2021

direct_upload在rails 6.1下有问题 huacnlee/activestorage-aliyun#70

Closed

DmitryTsepelev force-pushed the store-per-attachment-direct-upload branch from dd14d5c to 9f65882 Compare January 28, 2021 09:14

DmitryTsepelev force-pushed the store-per-attachment-direct-upload branch from c45caee to d0465ea Compare November 2, 2021 19:33

DmitryTsepelev requested a review from gmcgibbon November 3, 2021 07:28

rails-bot bot added the docs label Nov 8, 2021

DmitryTsepelev force-pushed the store-per-attachment-direct-upload branch from ac9a9ab to 81c040b Compare November 8, 2021 20:22

gmcgibbon approved these changes Nov 13, 2021

View reviewed changes

DmitryTsepelev force-pushed the store-per-attachment-direct-upload branch from 81c040b to 29a5cdc Compare November 14, 2021 08:31

Support direct uploads to multiple services

193289d

DmitryTsepelev force-pushed the store-per-attachment-direct-upload branch from 29a5cdc to 193289d Compare November 14, 2021 08:49

gmcgibbon merged commit fb36712 into rails:main Nov 15, 2021

DmitryTsepelev mentioned this pull request Nov 15, 2021

Fix flaky test in HasManyThroughDisableJoinsAssociationsTest #43649

Merged

gmcgibbon mentioned this pull request Nov 15, 2021

Fixes for multi-service direct uploads #43650

Merged

brenogazzola mentioned this pull request Nov 23, 2021

file_field helper raises an exception if attribute does not match an active storage attachment #43698

Closed

gmcgibbon mentioned this pull request Nov 24, 2021

Fix invalid direct upload form attribute #43707

Closed

tbcooney mentioned this pull request Dec 20, 2021

Turbo stream partial with button_to tag throws ActionView::Template::Error hotwired/turbo-rails#243

Closed

rosa mentioned this pull request Dec 22, 2021

Issues with new direct upload support for multiple storage services and an alternative #43971

Closed

brenogazzola mentioned this pull request Jan 10, 2022

Delivering assets through CloudFlare requires monkey-patch #44136

Closed

p8 mentioned this pull request Jan 25, 2022

Rename serviceName to directUploadToken in AS' DirectUpload #44249

Closed

gmcgibbon mentioned this pull request Jan 28, 2022

Revert #38957 #44287

Merged

miquelbarba mentioned this pull request Aug 17, 2022

activestorage debug miquelbarba/rails#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass service_name param to DirectUploadsController #38957

Pass service_name param to DirectUploadsController #38957

DmitryTsepelev commented Apr 15, 2020

scottjg left a comment

DmitryTsepelev commented Apr 28, 2020

rails-bot bot commented Jul 27, 2020

DmitryTsepelev commented Jul 28, 2020

neiljohari commented Jul 29, 2020 •

edited

rails-bot bot commented Oct 27, 2020

neiljohari commented Oct 27, 2020

adeeb1 commented Nov 2, 2020 •

edited

DmitryTsepelev commented Nov 3, 2020

scottjg commented Nov 21, 2020

gmcgibbon commented Nov 21, 2020

DmitryTsepelev commented Dec 4, 2020

gmcgibbon Jan 4, 2021

DmitryTsepelev Jan 13, 2021

gmcgibbon Jan 20, 2021

DmitryTsepelev Jan 28, 2021

DmitryTsepelev Apr 9, 2021

MarcoPrins Oct 19, 2023 •

edited

santib commented Feb 14, 2021

t27duck commented Nov 4, 2021

DmitryTsepelev commented Nov 8, 2021

gmcgibbon left a comment

gmcgibbon Nov 13, 2021

neiljohari commented Nov 15, 2021

tbcooney commented Dec 20, 2021

tbcooney commented Jan 15, 2022

DmitryTsepelev commented Jan 15, 2022

Pass service_name param to DirectUploadsController #38957

Pass service_name param to DirectUploadsController #38957

Conversation

DmitryTsepelev commented Apr 15, 2020

Summary

Other Information

scottjg left a comment

Choose a reason for hiding this comment

DmitryTsepelev commented Apr 28, 2020

rails-bot bot commented Jul 27, 2020

DmitryTsepelev commented Jul 28, 2020

neiljohari commented Jul 29, 2020 • edited

rails-bot bot commented Oct 27, 2020

neiljohari commented Oct 27, 2020

adeeb1 commented Nov 2, 2020 • edited

DmitryTsepelev commented Nov 3, 2020

scottjg commented Nov 21, 2020

gmcgibbon commented Nov 21, 2020

DmitryTsepelev commented Dec 4, 2020

gmcgibbon Jan 4, 2021

Choose a reason for hiding this comment

DmitryTsepelev Jan 13, 2021

Choose a reason for hiding this comment

gmcgibbon Jan 20, 2021

Choose a reason for hiding this comment

DmitryTsepelev Jan 28, 2021

Choose a reason for hiding this comment

DmitryTsepelev Apr 9, 2021

Choose a reason for hiding this comment

MarcoPrins Oct 19, 2023 • edited

Choose a reason for hiding this comment

santib commented Feb 14, 2021

t27duck commented Nov 4, 2021

DmitryTsepelev commented Nov 8, 2021

gmcgibbon left a comment

Choose a reason for hiding this comment

gmcgibbon Nov 13, 2021

Choose a reason for hiding this comment

neiljohari commented Nov 15, 2021

tbcooney commented Dec 20, 2021

tbcooney commented Jan 15, 2022

DmitryTsepelev commented Jan 15, 2022

neiljohari commented Jul 29, 2020 •

edited

adeeb1 commented Nov 2, 2020 •

edited

MarcoPrins Oct 19, 2023 •

edited