Configures Combustion app and adds failing test

[tulak]

The Problem

Currently, the ahoy_email initializer calls app.key_generator during the engine initialization phase. This creates a significant "global side effect" in Rails applications because the key_generator is a singleton.

In modern Rails (7.0+), ActiveSupport sets the key_generator_hash_digest_class (to SHA256) inside a config.after_initialize block in active_support.set_key_generator_hash_digest_class initializer. If app.key_generator is invoked before that block runs, the generator is instantiated with the legacy Rails default (SHA1) and memoized.

Impact

This effectively "poisons" the global key generator for the entire application:

1. Security: The app is forced to use SHA1 for all cryptographic operations even if configured for SHA256.
2. Broken Sessions: Since ActionDispatch::Cookies relies on this same generator, adding this gem to an existing Rails 7+ app can break session verification, effectively logging out all users because the app can no longer verify cookies signed with the modern default.

The Fix

This PR delays the initialization of AhoyEmail.secret_token by:

1. Scheduling the initializer to run after: :load_config_initializers.
2. Wrapping the token generation inside app.config.after_initialize.

This ensures that all framework and user-defined configurations are fully loaded before the key_generator is touched.

module AhoyEmail
  class Engine < ::Rails::Engine
    initializer "ahoy_email", after: :load_config_initializers do |app|
      app.config.after_initialize do
        AhoyEmail.secret_token ||= app.key_generator.generate_key("ahoy_email")
      end
    end
  end
end

[ankane]

Hi @tulak, thanks for reporting. I think a minimal fix would be:

 module AhoyEmail
   class Engine < ::Rails::Engine
     initializer "ahoy_email" do |app|
-      AhoyEmail.secret_token ||= app.key_generator.generate_key("ahoy_email")
+      AhoyEmail.secret_token ||= ActiveSupport::KeyGenerator.new(app.secret_key_base, iterations: 1000, hash_digest_class: OpenSSL::Digest::SHA1).generate_key("ahoy_email")
     end
   end
 end

However, any fix will impact applications that rely on the current behavior, so I'd like to think about the best way forward.

Also, I think this is a bug with Rails (this problem will happen with any application calling Rails.application.key_generator in an initializer).

[tulak]

I've found that the same problem happend in hotwired/turbo-rails#325 and DHH himself suggested wrapping the call to app.key_generator in config.after_initialize block. This fix is in the turbo-rails repository till today.

But @ankane you're right, we're in a different situation here. The change to call app.key_generator has been introduced in 5d7797b over 1.5 year ago so a lot of users of the gem are probably using this with silently being "downgraded" to use SHA1 for app.key_generator.

So even if we fix it your way, when we explicitly use ActiveSupport::KeyGenerator with values that are backwards compatible with current behaviour, projects that have been using the version with the bug and upgrade the gem will experience users signing out their Rails apps because the whole Rails app will switch from SHA1 to SHA256. This should be fixable by setting up cookie rotations however I think this should not be set up by ahoy_email gem rather the gem should issue a warning about this and maintainers upgrading their Gemfiles should act on this.

I'm look forward your thoughts on this @ankane

[tulak]

I've just read through hotwired/turbo-rails#340 where people discussed how the fix hotwired/turbo-rails#325 affected users that were running on the buggy version of turbo_rails for a while and then upgraded to the fixed version. There's much more stuff that can break just inside rails (ActionText attachments, Signed IDs) and some of them are much works than cookies. When cookies get broken, users can just sign-in again, but for ActionText Attachments a maintainer needs to migrate the assets with new message verifier.

I didn't find any comprehensive fix that Rails would implement to avoid this from happening. turbo-rails added a mention about this to their upgrade docs which probably something we'll have to do for ahoy_email.

But since the same problem happened in another gem (ahoy_email) and has been sitting in the codebase for years it can happen in buch of other gems we don't know about. I think some defensive mechanism should be implemented in Rails to prevent this. I have some ideas about guarding call to app.key_generator until the rails configuration initializes properly. I'll try to open PR to rails itself.

[ankane]

Thanks @tulak. I've applied the fix in the commit above, but still need to figure out the best way to communicate it (existing installations will need to set config.active_support.key_generator_hash_digest_class = OpenSSL::Digest::SHA1 when upgrading to avoid breakage).

This also impacts Mailkick, so will apply the fix there as well.

[tulak]

@ankane I'd not rush into releasing new gem versions with this fix until we have some strategy on how to communicate this properly. Seems like a lot of people were affected by this kind of fix for turbo_rails. As discussed in hotwired/turbo-rails#340 some of them had to run migration scripts for fixing signed ActionText Attachments.

[ankane]

Yes, that's the plan / why I mentioned communication above.

At the moment, I think the best way is probably a major version bump (and docs) to call attention to a potentially breaking change. It'd also be good to see if there are plans to address this in Rails.

Unfortunately, applications that add the gem today will experience similar breakage.

[ankane]

Reported to Rails: rails/rails#56736