Add support for ActiveStorage expiring URLs by aki77 · Pull Request #42410 · rai...
source link: https://github.com/rails/rails/pull/42410
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
New issue
Add support for ActiveStorage expiring URLs #42410
Merged
Merged
Conversation
@@ -1,3 +1,7 @@
* Add support for ActiveStorage expiring URLs.
activestorage/config/routes.rb
Outdated
@@ -34,12 +34,12 @@
if model.respond_to?(:signed_id)
route_for(
:rails_service_blob_proxy,
model.signed_id,
model.signed_id(expires_in: options.delete(:expires_in)),
aki77 22 days ago
Author
Contributor
I think it would be more useful to make the default value configurable as follows, what do you think about that?
model.signed_id(expires_in: options.delete(:expires_in) { ActiveStorage.rails_urls_expire_in }),zzak 22 days ago
Member
Probably an application/global setting + an argument that overrides it makes sense. 
activestorage/lib/active_storage.rb
Outdated
@@ -60,6 +60,7 @@ module ActiveStorage
mattr_accessor :content_types_allowed_inline, default: []
mattr_accessor :service_urls_expire_in, default: 5.minutes
mattr_accessor :rails_urls_expire_in
pixeltrix 17 days ago
Member
Maybe just urls_expire_in? Unsure whether that's too generic but definitely not keen on rails_urls_expire_in
@aki77 Could you rebase the CHANGELOG to a single entry and squash the commits? 
I've changed the CHANGELOG and squashed it into one commit.
#38843 changed the whole behavior of ActiveStorage::Blob.signed_id, but this time it will be expired only if the expired_in argument is specified in the url helper.
I don't think anyone would bother to use an expired URL in a cached fragment.
The differences between #38843 and this PR are as follows.
38843
- Breaking the default behavior
- The
signed_idis always set to expire, and there is no way to work around it, which causes problems in the following cases.- If the direct upload takes more than 5 minutes
- When ActiveStorage URLs are used in the fragment cache
- There is no way to create a persistent url.
This PR
- The default behavior is not changed in any way.
- ActiveStorage URLs can be set to expire only in cases where they are needed.
- It is also possible to set ActiveStorage URLs to expire by default in the app-wide settings, which easily promotes overall security improvement.
My motivation is similar to the following comment.
#38843 (comment)
@pixeltrix @zzak Thanks for the review!
What else do I need to do to get this PR merged?
@aki77 thanks 👍🏻
Thanks!
No one assigned
None yet
No milestone
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK