pboling / sanitize_email

An Email Condom for your Ruby Server
http://railsbling.com/sanitize_email/
MIT License
165 stars 36 forks source link

Not automatically intercepting in Rails any longer #110

Closed GUI closed 2 weeks ago

GUI commented 1 month ago

After upgrading from sanitize_email 2.0.4 to 2.0.6, it looks like e-mails have stopped being intercepted in a Rails 7.1 app by default. If I manually call Mail.register_interceptor(SanitizeEmail::Bleach), this does seem to fix the interception in the app, but I think something is maybe amiss with the new Engine integration for Rails 6+ in 2.0.6:

https://github.com/pboling/sanitize_email/compare/v2.0.4...v2.0.6#diff-c6ad5f4fb841393b5d4fee8dac9a3be490baf334851b4cae3adcb90c1096a4e2 https://github.com/pboling/sanitize_email/compare/v2.0.4...v2.0.6#diff-6b14c6f0901cbec9229126c0ae73f28b72421a354fc0aead148fa406f5f665ca

I think the issue is maybe related to the new EngineV6 calling: https://github.com/pboling/sanitize_email/blob/2ee17b815d7bff4dcba975ea2410d7cbbc48b1c2/lib/sanitize_email/engine_v6.rb#L13

I believe the issue is that register_interceptor may not actually be doing anything in this case. At least in the Rails 7.1 app I'm testing, Rails.application.config.action_mailer is actually an instance of ActiveSupport::OrderedOptions, so this is more like a hash that responds to method calls as hash lookups too. But that means that I don't think register_interceptor() is actually calling any type of underlying method. Instead, I think it's just looking for a :register_interceptor attribute on the hash of mailer config data, and returning nil since it doesn't exist.

So I think this means that calling Rails.application.config.action_mailer.register_interceptor(SanitizeEmail::Bleach) doesn't actually seem to ever call the underlying ActionMailer::Base.register_interceptor or Mail.register_interceptor methods. You can see how any method passed to action_mailer will not raise an exception, but I don't believe it's actually calling anything (I think it's just acting more like a hash accessor). This differs from how a real call to ActionMailer::Base.register_interceptor will react:

[1] pry(main)> Rails.application.config.action_mailer.register_interceptor(SanitizeEmail::Bleach)
=> nil
[2] pry(main)> Rails.application.config.action_mailer.register_interceptor
=> nil
[3] pry(main)> Rails.application.config.action_mailer.register_interceptor_foo(SanitizeEmail::Bleach)
=> nil
[4] pry(main)> ActionMailer::Base.register_interceptor
ArgumentError: wrong number of arguments (given 0, expected 1)
from /dev-cache/usr/local/bundle/gems/actionmailer-7.1.3.4/lib/action_mailer/base.rb:548:in `register_interceptor'

So I think this might mean that this Engine for Rails 6+ isn't actually setting the interceptor by default in version 2.0.6. It sounds like maybe a deprecation warning is the reason for the new approach in Rails 6+, but for whatever it's worth I don't actually see any deprecation warnings when using sanitize_email 2.0.4 under Rails 7.1 (but I realize I might not be exercising the right thing, since there's probably a lot of different possible app setups).

Thanks!

pboling commented 1 month ago

Interesting. Thanks for the report. I'll get it fixed ASAP.

pboling commented 1 month ago

Turns out part of the Rails documentation was bad. https://github.com/rails/rails/issues/42170

Fixing.

pboling commented 1 month ago

Fix released in 2.0.7. Please give it a try @GUI

anthonyklue commented 3 weeks ago

We're still experiencing this issue in version 2.0.7 & rails 7.1.3.4

Changing the code in engine_v6.rb to

    ActiveSupport.on_load(:action_mailer) do
      ActionMailer::Base.register_interceptor(SanitizeEmail::Bleach)
    end

as suggested here appears to solve the problem

pboling commented 2 weeks ago

Fix is released in v2.0.8 @anthonyklue @GUI