Project

General

Profile

Actions
Copy link

Defect #43255

open

mail_handler : may fail to invoke reception hooks if plugins monkey patch some methods (with patch)

Added by Vincent Caron 22 days ago. Updated 10 days ago.

Status:
Needs feedback
Priority:
Normal
Assignee:
-
Category:
Email receiving
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Affected version:
5.1.10

Description

I recently realized that my incoming emails were partially processed : reply to issues would be ignore.

It turns out that the plugin redmine_per_project_formatting I'm using monkey patch the MailHandler::receive_issue_reply (and an another) and this code has to be changed to check if a method is callable instead of only searching for it in the Redmine module :

--- app/models/mail_handler.rb.orig    2025-03-11 00:30:03.000000000 +0100
+++ app/models/mail_handler.rb    2025-09-26 18:42:40.504660223 +0200
@@ -155,7 +155,7 @@
     if headers.detect {|h| h.to_s =~ MESSAGE_ID_RE}
       klass, object_id = $1, $2.to_i
       method_name = "receive_#{klass}_reply" 
-      if self.class.private_instance_methods.collect(&:to_s).include?(method_name)
+      if self.class.method_defined?(method_name)
         send method_name, object_id
       else
         # ignoring it
Actions
Copy link
 #1

Updated by Go MAEDA 19 days ago

  • Description updated (diff)
Actions
Copy link
 #2

Updated by Go MAEDA 19 days ago

  • Status changed from New to Needs feedback

The change switches the if condition from checking for private methods to checking for public or protected methods, which breaks the existing behavior of MailHandler. Therefore, it cannot be included in the Redmine core.

Before the change, the condition returns true if a private method matching method_name exists. After the change, only public or protected methods are considered, and private methods are ignored.

If you apply the change and run ruby test/unit/mail_handler_test.rb, many tests will fail.

To fix the problem you are currently experiencing in your environment, please contact the plugin's author.

Actions
Copy link
 #3

Updated by Vincent Caron 17 days ago

Would it be acceptable if in the patch we replaced self.class.method_defined?(method_name) with self.class.private_method_defined?(method_name) ?

I tried to run the test but I only have the production environment and got the error "The `test` database is not configured for the `test` environment".

Actions
Copy link
 #4

Updated by Holger Just 17 days ago

Vincent Caron wrote in #note-3:

Would it be acceptable if in the patch we replaced self.class.method_defined?(method_name) with self.class.private_method_defined?(method_name) ?

That would still be wrong as this would check if there is a private class method, rather than an instance method. However, a cleaner version with equivalent functionality would be this:

--- app/models/mail_handler.rb.orig    2025-03-11 00:30:03.000000000 +0100
+++ app/models/mail_handler.rb    2025-09-26 18:42:40.504660223 +0200
@@ -155,7 +155,7 @@
     if headers.detect {|h| h.to_s =~ MESSAGE_ID_RE}
       klass, object_id = $1, $2.to_i
       method_name = "receive_#{klass}_reply" 
-      if self.class.private_instance_methods.collect(&:to_s).include?(method_name)
+      if respond_to?(method_name, true)
         send method_name, object_id
       else
         # ignoring it

Here, we allow any visibility of the respective receive method, i.e. public, protected or private. To quote the documentation of Object#respond_to?:

Returns true if obj responds to the given method. Private and protected methods are included in the search only if the optional second parameter evaluates to true.

Actions
Copy link
 #5

Updated by Vincent Caron 10 days ago

I've tested Holger Just's patch and it works fine for me (ie. compatible with a plugin which monkey patches MailHandler).

Actions
Copy link

Also available in: Atom PDF