After updated to 1.1.1, ActionText attachments are not showing #340
Comments
|
Can't replicate that on a new Rails app with turbo-rails 1.1.1 and Action Text. Need more detail. Look in the logs, look in the JavaScript console for errors. |
|
I also have the same issue |
|
I have the same problem on an app with around 125 attachments. Here is a link to a demo app, as requested above. The demo uses Rails 7.0.3, and turbo-rails with importmaps. This was setup using ruby 3.1.2. To demonstrate the problem, first use the app and attach an image using turbo-rails 1.0.1. Then upgrade the v 1.1.1. The image does does not display. I get 404 errors in the console and in the rails log. If you re-attach the same image using turbo-rails 1.1.1, it displays properly. But this process is impractical for apps with many attachments. |
|
@ccasabona - I just tried the demo app on ruby 3.1.2. It worked for me on turbo-rails 1.0.1 and then again on and turbo-rails 1.1.1. I am unable to reproduce the issue. I tested on MacOS using the latest Chrome and Firefox, worked on both. Note: I do have "disable caching" in the browser enabled. Is it possible there might be a cached javascript asset? |
|
Still the same problem with caching disabled on latest Safari, Firefox, and Chrome using MacOS. I notice in the rails log that after upgrading to 1.1.1, the ActiveStorage::Blob Load statement is not present. Here is my log under v 1.0.1: After installing v 1.1.1: There is no I get the same problem if I reverse the conditions. If I attach an image under v 1.1.1 and change turbo-rails to 1.0.1, the log result is the same. |
|
Cc @aaronjensen |
|
I don’t know anything about active storage but if the key generator is involved and you have Rails 7 defaults on an existing app you probably need to explicitly go back to SHA1 where you were before. |
|
This makes me think that this should have been a major version with an upgrade guide. It’s more than just invalidating sessions apparently. |
|
Is there a way to migrate attachment hashes? |
|
I reproduced the issue. Reverting #335 resolves it for me as well: |
|
All #335 does is cause Rails to respect the hash algorithm default. If you are affected by this, set it to SHA1. |
|
Yeah the broken images are due to the fact that their keys were generated with a key generator that's not the same as the one using to decode it. So somehow this default change screwed with that. |
|
@tleish could you look at the different key generators that are set with and without that change? Something clearly got moved. |
|
No way to migrate hashes easily in already generated content. So can't have this change on people. Need to revert this or amend so there's no change for existing folks, then release. |
|
In Rails 7, the difference is based on a the caching of module ActiveSupport
# KeyGenerator is a simple wrapper around OpenSSL's implementation of PBKDF2.
# It can be used to derive a number of keys for various purposes from a given secret.
# This lets Rails applications have a single secure secret, but avoid reusing that
# key in multiple incompatible contexts.
class KeyGenerator
class << self
def hash_digest_class=(klass)
if klass.kind_of?(Class) && klass < OpenSSL::Digest
@hash_digest_class = klass
else
raise ArgumentError, "#{klass} is expected to be an OpenSSL::Digest subclass"
end
end
def hash_digest_class
@hash_digest_class ||= OpenSSL::Digest::SHA1
end
endBEFOREPrevious turbo-rails code executed initializer "turbo.signed_stream_verifier_key" do
Turbo.signed_stream_verifier_key = config.turbo.signed_stream_verifier_key ||
Rails.application.key_generator.generate_key("turbo/signed_stream_verifier_key")
endAFTERNow, the code first sets the cached variable module ActiveSupport
class Railtie < Rails::Railtie
#...
initializer "active_support.set_key_generator_hash_digest_class" do |app|
config.after_initialize do
if klass = app.config.active_support.key_generator_hash_digest_class # OpenSSL::Digest::SHA256
ActiveSupport::KeyGenerator.hash_digest_class = klass
end
end
endafter which the turbo-rails change uses the cached var: initializer "turbo.signed_stream_verifier_key" do
config.after_initialize do
Turbo.signed_stream_verifier_key = config.turbo.signed_stream_verifier_key ||
Rails.application.key_generator.generate_key("turbo/signed_stream_verifier_key")
end
endThe following does fix the issue, but doesn't seem to be the right fix. |
@dhh I'm sorry I didn't make it clear what this change did, as I thought it was clear from the original issue in #325. I only talked about invalidating sessions because I didn't know that it would also break attachments somehow. As an aside, does this mean that secret key rotation is impossible/challenging? As for this change, reverting it will revert to the buggy behavior: When turbo-rails is installed, a Rails 7 app will go from SHA256 to SHA1 for its keys. In short, the same thing will happen that's happening to people now. But, since turbo-rails is often installed with Rails, it's likely to affect fewer people. It affected us because we used Rails without turbo in production and then added Turbo later. If I may, I can suggest a couple alternatives, though I have no way of knowing how palatable any of them will be given so I won't recommend one. A. Pull the current version of turbo-rails and release a new major version with documentation about what happened, what was fixed, and what you must do to explicitly opt back in to SHA1 if you are impacted. if ActiveSupport::KeyGenerator.hash_digest_class != app.config.active_support.key_generator_hash_digest_class
raise "some explanatory error"
endThoughts? Any other ideas? |
|
Is this possibly a Ruby on Rails bug? The fact that you can incorrectly override the default One other question on the fallback. Would it be possible to use a try/catch on the read of the active storage blob? If reading the blob fails AND This allows an upgrade while still reading old active storage blobs. |
|
@tleish yes, I think this is likely a Rails bug/design flaw (see my original issue). It’s not just storage blobs though, it’s sessions and probably other things. I don’t know what the full scope is. |
|
Yeah, it is unfortunately not easy to migrate these existing Action Text embeds for key rotation. Seems to me that we're better off restoring the previous behavior, release a new minor version, then do a major version with the change, and that version detects the clash and asks you to resolve. Makes sense? |
|
That sounds like a good plan to me. How can I help? Do you think we could get the minor and the major teed up to release at the same time or should we give it some time? |
|
Where did this leave off? I recently upgraded to Rails 7 defaults from 6.1, upgraded @rails/activestorage & @rails/actiontext & @rails/actioncable from 6.0.3-4 to 7.0.3-1. After doing so---I found that all ActionText embeds were then broken links. After following this conversation, I added "ActiveSupport::KeyGenerator.hash_digest_class = OpenSSL::Digest::SHA1" to an initializer---which did not resolve the issue. I then downgraded the above items back to 6.0.3-4---which also did not resolve the issue. My embeds are still broken. I also followed GoRails #452 "How To Update ActionText Attachments"---which also did not resolve the broken embeds. I anticipated downgrading to resolve the issue, but it did not. Is there any further guidance or progress on this issue? THANKS! |
|
Downgrading the turbo-rails gem from 1.1.1 to 1.1.0 did resolve broken embeds. |
|
@atstockland try putting that snippet in your application.rb either before or inside the configure block. An initializer would be too late most likely. |
|
Thanks @aaronjensen that did it. I reset the gem to 1.1.1, and move the snippet to application.rb and embeds are still loading ok. :) |
|
This change seems to affect not only action text, it also affects sessions, signed id, among others. Reverting the key generator to SHA1 will cause problems for those of us who are on SHA256 now. In my case, it would break some links with signed ids that I am mailing. Thanks |
|
We started out project with rails 6, later we upgraded to rails 7. and never had a broken attachment until now. We upgraded to turbo 1.1.1 and all our trix attachments are broken. We didn't notice at first so our users started to upload new attachments So now I have:
Going back to 1.1.0 fixes the big bunch of broken attachments, but breaks the new bunch of attachments. Is there a code snippet to at least go through all actiontexts and update old sgids with the new ones? I wonder why this wasn't a problem for basecamp folks or other companies? aren't they also using sha256 (default from rails7) and upgraded this gem so went back to sha1 ? |
|
Was hoping that the |
|
@jeremiemv I wouldn't recommend this. I would recommend configuring the hash algorithm you want explicitly in existing (non-greenfield) applications. Add this to the top of you application.rb's class Application < Rails::Application
config.load_defaults 7.0
ActiveSupport::KeyGenerator.hash_digest_class = OpenSSL::Digest::SHA1
# ...
endThis should allow you to upgrade safely, but be sure to test. |
|
@aaronjensen he did say "non greenfield" |
|
@marcelolx ah, my mistake! GitHub had greenfield on its own line and I missed the non. Thanks. |
|
Thanks @aaronjensen for being so reactive, and @marcelolx. I added an hyphen, non-greenfield, to avoid potential confusion with formatting. Going to look into configuring |
|
Im also struggling with this issue. I'm unsure if i have some other permutation due to using google as storage. Other storage elements works with fine, as an example i have image upload using activestorage and those images are found and shown correctly. This makes me believe that that im still facing issues with actiontext. My log: From my gem file My application.rb has the following as per this issue:
Any other ideas on what to do to overcome this issue? Thanks in advance |
|
I just needed to supply more in the application file: Works for me on my setup. |
|
This is a nasty issue for active text. I've ended up in a state where active text users have two different hash functions going on, and there doesn't seen to be a workaround for that. While this issue touches a lot of parts of Rails - I think it should also be a sign that Active Text could be made more robust, too. |
|
Yeah this is really annoying. What we need to do to fix this proper is to have a way to have multiple verifiers configured, so we can deal with multiple different signings. If anyone is interested in working on that, please do. |
|
same issue here.. I'm using rails 7, & after uploading an image via trix editor, still image not displaying on web page.. please help me. |
|
just come across this issue ourselves in the exact same way as @silva96 - not yet found a solution that works for us |
|
@800a7b32 Yes. This is an issue and had no solution yet. I still using version 1.0.1 |
|
This is a configuration setting. If you relied on the old one and later versions change it, set it to the old one manually. See my tips above. I can’t think of a reason someone wouldn’t be able to upgrade unless they are trying to do it without respecting the fact that the new version effectively changes the default. |
|
You can also use a key rotation to ensure that your old asset links don't break when upgrading to a later turbo-rails version. I think this is the only option if you've ended up with assets using both the SHA1 and SHA256 based keys. Putting the following in an initializer should solve it, by adding a SHA1-based key as the fallback to the new SHA256 one: Rails.application.config.after_initialize do |app|
key_generator = ActiveSupport::KeyGenerator.new app.secret_key_base,
iterations: 1000,
hash_digest_class: OpenSSL::Digest::SHA1
app.message_verifier("ActiveStorage").rotate(key_generator.generate_key("ActiveStorage"))
end(Note that you need to do this in I'll add something to the turbo-rails docs about this, too. |
Applications that are affected by secrets changing when upgrading pass 1.1.0 can use a key rotation to avoid ActiveStorage assets breaking. This adds documentation to explain the issue, with code showing how to rotate the key.
Applications that are affected by secrets changing when upgrading pass 1.1.0 can use a key rotation to avoid ActiveStorage assets breaking. This adds documentation to explain the issue, with code showing how to rotate the key.
|
We've added some information about this to the docs. Hopefully those docs, and this issue, give everyone affected enough information to adjust their configuration to resolve the issue. I think we can close this issue now since there's not any more action for us to take on it. @aaronjensen's fix in #335 remains the correct solution to the original problem, so Turbo Rails now behaves properly in that regard. It's disappointing that fixing the bug leaves some people with assets stored using a different key than expected, for sure. But keeping the old behaviour would only be prolonging the problem. I don't think Turbo Rails should try to account for any data that might have been affected (e.g. by adding a key rotation automatically). Many applications using Turbo Rails won't have been affected by this issue, and it would be hard for us to know with certainty what resolution each app needs. It might also be helpful to consult the Rails upgrading docs about the digest class change, as it addresses the same situation in another context. |
|
That’s great, thank you @kevinmcconnell |
|
@kevinmcconnell Thank you very much! It's worked for me Just added this to /config/initializers Rails.application.config.after_initialize do |app|
key_generator = ActiveSupport::KeyGenerator.new app.secret_key_base,
iterations: 1000,
hash_digest_class: OpenSSL::Digest::SHA1
app.message_verifier("ActiveStorage").rotate(key_generator.generate_key("ActiveStorage"))
end |
|
Add this line to config/application.rb config.active_storage.variant_processor = :mini_magick |
|
This problem is truly a Beldar Conehead “Mebbs, I cannot think of everything!“ programming mishap.
So far, there is no solution to this vexing problem meaning that you simply can’t have it both ways. I have an a production app with over 200 pdf images attached using SHA1. Until such time as some underlying change in Rails or some other gem blocks rendering these images, this app is going to stay on SHA1. I could go in and delete the attachments and upload them again under SHA256, but as long as they can render somehow, that isn’t going to happen.
It does give one the uneasy feeling that sometime in the future SHA1 projects will be forced to switch to SHA256 for some as yet unforeseen but equally complex reason.
… On May 8, 2023, at 9:07 AM, Matt Budz ***@***.***> wrote:
I have an app in production in a state where images were stored using SHA1 and SHA256.
Currently, only new (SHA256) images are rendered. If I use the solution suggested by @kevinmcconnell <https://github.com/kevinmcconnell>
Rails.application.config.after_initialize do |app|
key_generator = ActiveSupport::KeyGenerator.new app.secret_key_base,
iterations: 1000,
hash_digest_class: OpenSSL::Digest::SHA1
app.message_verifier("ActiveStorage").rotate(key_generator.generate_key("ActiveStorage"))
end
then only the old (SHA1) images are rendered. While the new (SHA256) images are broken. Unfortunately re-uploading images isn't possible. Is there a solution where the images will render regardless if they were stored with SHA1 or SHA256?
—
Reply to this email directly, view it on GitHub <#340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQTUIUMRHXB4JNFMOK3BWLXFDVYXANCNFSM5W7R5VCA>.
You are receiving this because you were mentioned.
|
This was the ticket for me @aaronjensen . Thanks for this! 🥳 I ended up adding the following to my config.after_initialize do |app|
key_generator = ActiveSupport::KeyGenerator.new app.secret_key_base,
iterations: 1000,
hash_digest_class: OpenSSL::Digest::SHA1
app.message_verifier('ActiveStorage').rotate(key_generator.generate_key('ActiveStorage'))
endAdding the above to an initializer didn't work for me, probably related to something in the overall app initialization process (local issue). |
|
Is there a way to migrate my existing SHA1 active storage blobs over to SHA256 so that I can move my app to SHA256 altogether? I'm in the same situation as Matt B where now (because we didn't notice this issue for a month) I have some images in each hash digest. The config change added to the documentation (thanks @kevinmcconnell) does help us get our old images rendering, but leaves our new ones broken. And there's another side effect we've come across - if I edit the record that references the old image (like in a trix editor), the old image will no longer render, presumably because when the save occurs, it uses SHA256. |
|
@bekkii77 have you tried what @aaronjensen suggested? Putting the snippet in the I have to go back to my old project and give this a whirl myself, but it sounds like it's the solution here. |
|
None of the above worked for me, but I managed to migrate all ActionText attachments thanks to GoRails - How To Update ActionText Attachments. I find that it's an even cleaner solution. Only new digests are in the DB, and no rotation needed. |
|
The Rake Task works!. I was able to successfully migrate all the ActionText attachments made under turbo-rails prior to v1.1.1. I then removed the When the task runs under mysql, this warning is generated:
This doesn't prevent the rake task from converting the attachment digests (apparently) correctly, but I'm not sure what that means once the deprecation takes place. Cheers |
and others
After updated turbo-rails to version 1.1.1. persisted ActionText attachments are not showing anymore, it looks like this:
When I restored to version 1.0.1, it works fine.
The text was updated successfully, but these errors were encountered: