dlazar
January 19, 2022, 2:33pm
1
Trying out activestorage (7.0.1), with the JS “@rails /activestorage”: “^7.0.1”,
Have a Shop model with a file attachment. Using the form_with(model: @shop , direct_upload: true) and a form.file_field helper, I am seeing most of the info needed for a successful Direct Upload, except the critical two fields:
direct-upload-token is not rendered and there is no direct-upload-attachment-name generated either, when I inspect the generated DOM element.
So when I go to upload, the server throws a 500 error on invalid upload token. I am at a loss as to why this. How can I get the file_field helper to provide those values in a Rails 7 project?
Don’t bother trying to fix your code. This is because of a new feature whose implementation the core devs decide to redo because of how much trouble it is causing. For now, simply monkey patch your app to force rails back to the old behaviour. Basically, create this initializer:
Rails.application.config.after_initialize do
ActiveStorage::DirectUploadsController.class_eval do
private
def verified_service_name
ActiveStorage::Blob.service.name
end
end
ActionView::Helpers::FormTagHelper.module_eval do
private
def convert_direct_upload_option_to_url(_, options)
if options.delete(:direct_upload) && respond_to?(:rails_direct_uploads_url)
options["data-direct-upload-url"] = rails_direct_uploads_url
end
options
end
end
end
1 Like
For reference, here’s the discussion about scrapping the current implementation:
opened 04:27PM - 22 Dec 21 UTC
closed 04:53PM - 08 Feb 22 UTC
activestorage
I wanted to summarise here some issues we've found when upgrading to Rails 7.0 R… C1, related to the changes introduced in https://github.com/rails/rails/pull/38957/: Pass service_name param to DirectUploadsController, for discussion of possible alternatives and also in case others run into these issues. First of all, thanks to everyone who worked on that, these aren't obvious issues and not something everyone will run into. The part of that approach that's problematic is the one that shares similarities with Rails' forgery protection implementation.
- A token is generated when rendering the direct upload form, and stored in `session[:_direct_upload_token]`, and stored in a `data-direct-upload-token` attribute. This token includes the service name.
- When the form is submitted, the service name is extracted from the token, and the token's signature is verified to ensure it hasn't been tampered with. If it's valid, the service name is used to select the service for which the direct upload link is created.
- If it's not valid, an `ActiveStorage::InvalidDirectUploadTokenError` is raised.
This has some of the same issues with CSRF tokens handled in this way. First of all, the token is stored in the session, which normally lives for the duration of the browser session. However, the token included in the form can potentially live beyond that, as the form could be cached either via server-side caching (such as fragment caching) or HTTP cached. Any direct upload attempted after this will fail as the session won't have a direct upload token when directly loading a cached form after opening the browser.
For CSRF, this is a very known problem and there are workarounds in place (like the one described [here](https://coderwall.com/p/0sctaa/forms-csrf-authenticity-token-and-fragment-caching-in-rails)). With Turbo also, since forms might end up being rendered via turbo stream, they won't set a session so a token rendered there would be always invalid when the user goes and uses the form to submit. That's not a problem for general CSRF protection because [Turbo handles it](https://github.com/hotwired/turbo-rails/blob/6958e0e2742d4744da80128d178e7255989386a4/app/assets/javascripts/turbo.js#L663-L665), but it does prevent per-form CSRF tokens from working at all. That's not an issue because they can be enabled/disabled by a configuration option ([they're disabled by default](https://github.com/rails/rails/blob/310d62a443b9157bb48d778f49ba236e18b06b63/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L85-L87)) and even if they're enabled, [the global token still works](https://github.com/rails/rails/blob/310d62a443b9157bb48d778f49ba236e18b06b63/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L403-L405). However, none of those workarounds is available currently for direct token uploads, and the direct token is required also for apps that don't use multiple storage services and don't really need this. I imagine that's the majority.
In our case, we upgraded our app from Rails 7.0.0.alpha (https://github.com/rails/rails/commit/a39098dc856b72a1aff2348b9de805fba71f7b92 more concretely, we were targeting `main`). We had to make some changes to our direct upload code in order to support this, as described by @lewispb in https://github.com/rails/rails/issues/43895#issuecomment-996677440, and not long after shipping this we had to patch `ActiveStorage:: DirectUploadToken#verify_direct_upload_token` to return our single service name as a quite significant percentage of our uploads were failing due to these problems.
I think support for multiple services can't depend on the client in this way. Like Javan mentioned in https://github.com/rails/rails/pull/37501#issuecomment-543813481, I think this should live server-side exclusively, and in a way that doesn't add extra complexity for apps that don't use multiple services. @gmcgibbon [mentioned](https://github.com/rails/rails/pull/37501#issuecomment-543843053):
> We could alternatively make separate direct upload endpoints for each service, but that would likely be a breaking change.
I think that'd work and wouldn't be a more breaking change than the approach taken, since this one still requires changing the client-side code to include the attachment name and the token with the service name. It could be equally changed to point to the proper endpoint for the service. I might be missing something here, though, so apologies for that. I think it'd be more work but I believe it can be done, while still preventing the client from uploading files to the service they choose and not the one it should be used, and avoiding all the issues with Turbo and caching.
If all that makes sense, I'd be quite happy to work on this myself in the coming weeks, but I'd be super grateful to hear other ideas.
cc @DmitryTsepelev, @gmcgibbon @dhh @lewispb @jeremy
It’s been reverted. If you are using Rails 7.0.2 you don’t have to worry about this.