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

[rosa]

I wanted to summarise here some issues we've found when upgrading to Rails 7.0 RC1, related to the changes introduced in #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). 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, 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) and even if they're enabled, the global token still works. 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 (a39098d 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 #43895 (comment), 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 #37501 (comment), 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:

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

[dhh]

I concur with this analysis. Having separate paths for each configured service seems like the way to go. Then in the default case, where there are not multiple services, we just use the existing root path.

Either way, what's been put in place now is busted, and we need to either revert it quickly for 7.0.1, or we need to rapidly replace it with this other proposal.

cc @rafaelfranca

[DmitryTsepelev]

Thank you for the research, @rosa!

Server–side handling would be ideal, but currently it's not possible, because at the moment when Blob is created, we don't know the service name, so we have to pass it from the client. Any workaround for this could help us to get rid from tokens completely.

We added tokens to all direct uploads just for the consistency, but technically we need them only when there is a custom service used. In my initial implementation I used this approach, and sounds like we could go back to it since it would minimise the damage, here are possible scenarios:

• if you don't use custom service (even with Turbo, which is broken now) — everything will work as before
• if you do use custom service without direct uploads — everything is fine
• if you use custom service with direct uploads using plain Rails — everything is fine
• if you use custom service with direct uploads with Turbo — it won't work, but will be reasonable tradeoff (it didn't work in Rails 6 too!)

Please let me know if it sounds reasonable and I can quickly come up with this interim solution while we think about the server–side approach.

[rosa]

Thank you so much for the detailed reply @DmitryTsepelev, and for all the time you've spent already working on this 🙇
I didn't realise your first approach had been to exclude non-multiple services from this approach. That would definitely work, since after all, direct uploads with multiple services were already broken before your PR. Big +1 for that.

About keeping this approach for direct uploads with multiple services, I think that, even if you don't use Turbo, you would still run into issues with caching and stale tokens. I think to fully prevent these we would need a server-side approach.

Server–side handling would be ideal, but currently it's not possible, because at the moment when Blob is created, we don't know the service name, so we have to pass it from the client. Any workaround for this could help us to get rid from tokens completely.

Would it be possible to use different endpoints to provide the direct upload URL, one per service, depending on where the direct upload is initiated? I think there'd need to be more work by the user instead of these controllers being provided directly by Rails, as Rails wouldn't know the usage of each service, but I think it might work. That is, for example, you'd have avatars that go to a specific service. You'd implement an Avatars::DirectUploadsController that extends ActiveStorage::DirectUploadsController and sets the service you want to use for avatars. Then, any form used to upload avatars would use this controller. That'd be the simplest version I can think of, probably can be made more sophisticated so that you don't need to write your own controller but simply specify it, and Active Storage would generate that for you.

This is a very hand-wavy idea as I haven't tried to implement this. You know this part of the code much better so please let me know any problems you see and I'll try to think of alternatives.

[DmitryTsepelev]

Would it be possible to use different endpoints to provide the direct upload URL, one per service, depending on where the direct upload is initiated? I think there'd need to be more work by the user instead of these controllers being provided directly by Rails, as Rails wouldn't know the usage of each service, but I think it might work.

I think so! Maybe we could even generate these controllers and routes based on configured attachments.

[rafaelfranca]

Please let me know if it sounds reasonable and I can quickly come up with this interim solution while we think about the server–side approach.

I think this solution makes sense. It would be great if you could work on that.

Either way, what's been put in place now is busted, and we need to either revert it quickly for 7.0.1, or we need to rapidly replace it with this other proposal.

Sorry I missed this one. I'm ok with reverting the change in 7.0.2 and release it ASAP, but if @DmitryTsepelev can implement an interim solution that works for all cases we can just wait for that.

[DmitryTsepelev]

Will do my best to come up with the temp solution next week, but I'm fine with reverting if it's blocking the release (I'll just reimplement it as soon as I'm ready).

[dhh]

Should we proceed with the revert for now?

[gmcgibbon]

Reverting in #44287. @DmitryTsepelev let me know if I can help. Thanks all looking to improve this feature! 😄

[Laykou]

I just found this issue during migration to Rails 7. Is this going to be released as new Rails version after the revert?
Or should we checkout from the commit sha?

[gmcgibbon]

I just found this issue during migration to Rails 7. Is this going to be released as new Rails version after the revert?
Or should we checkout from the commit sha?

The revert will likely be ported on 7-0-stable until a release is made with the fix.

[Laykou]

What is the purpose of attachment_name here? I just randomly generated it like:

attachment_name = "direct_upload#attachments"
direct_upload_token = ActiveStorage::DirectUploadToken.generate_direct_upload_token(
  attachment_name,
  ActiveStorage::Blob.service.name,
  Current.session
)

html = <<~HTML
  <input type="file"
         data-direct-upload-url="#{Rails.application.routes.url_helpers.rails_direct_uploads_url}"
         data-direct-upload-attachment-name="#{attachment_name}"
         data-direct-upload-token="#{direct_upload_token"
         data-reflex-permanent="1"/>
HTML

and it is working. I did not need to pass real model or real name.

Direct upload created just ActiveStorageBlob and after the form is submitted, correct ActiveStorageAttachment is created too.

[olegdizhak1]

Пока не знаю

[rafaelfranca]

Now that the commit was reverted, should we close this issue?

[rosa]

Yes, @rafaelfranca, sorry for not doing it earlier!

[rafaelfranca]

Cool! I'll work to release 7.0.2 with this revert today

[tomprats]

While the revert seemed to fix the issue, actiontext has a javascript dependency of >= 7.0.0-alpha1 which creates an issue. Simply installing 7.0.2 for both somehow still pulled the alpha. I ended up having to uninstall and reinstall actiontext and activestorage for it to work.

[eljojo]

For anyone running into this issue, @rosa's proposed solution (#43971 (comment)) worked beautifully on 7.0.4:

class VideoUploadsController < ActiveStorage::DirectUploadsController
  def blob_args
     super.merge(service_name: "aws_video_originals")
  end
end


# form
form.file_field :original, direct_upload: true, data: { direct_upload_url: video_uploads_url }

# routes
resources(:video_uploads, only: [:create])

[yoelcabo]

A more configurable version of the same workaround:

class DirectUploadsController < ActiveStorage::DirectUploadsController
  private
    def blob_args
      service_name = params.require(:service_name)
      super.merge(service_name: service_name)
    end
end

# route
post "/direct_uploads/:service_name" => "direct_uploads#create", as: :direct_uploads

# form
 file_field_tag :field, direct_upload: true,  data: { direct_upload_url: direct_uploads_url(:cloudflare_r2) }

[MarcoPrins]

Adding service_name to blob args will work but there's a security issue.

This workaround is susceptible to attacks where the user can modify the service name on the client side and therefore upload files to different buckets.

Is there still no secure solution?

@rosa Is there any reason why the service name needs to be specified on the blob record before the attachment record is created? Would it be realistic in a future implementation for the blob to be saved with a blank service_name and for that field to be updated when the attachment is created?

[lizdeika]

it is still an issue, direct uploads do not work with custom service for has_one_attached
why is this closed?

[rosa]

@lizdeika what do you mean? This issue was about direct uploads broken due to the support for multiple services introduced in #38957. That was reverted, so this issue was closed.