Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-57474

Invalid Message-ID header in forum post notifications

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 3.2, 3.2 regressions
    • Fix Version/s: 3.2.1
    • Component/s: Email, Forum, Libraries
    • Labels:
    • Testing Instructions:
      Hide

      To test this, you need outgoing emailing working. You may find $CFG->divertallemailsto useful.

      1. As a user, subscribe into a forum, using the default forum subscription preference to receive one email per one post.
      2. As an admin/teacher, post into the forum, while having checked "Send forum post notifications with no editing-time delay". R
      3. Reply to that post, again with the checkbox checked.
      4. Run cron
      5. Check the subscribed user's inbox
      6. TEST: The forum post notification email should have two emails. Their SMTP header Message-ID must have a value like <some-long-SHA1-hash@wwwroot> and not like <some-short-MD5-hash@hostname>.
      7. TEST: The reply should ideally be displayed as an email reply, but this may be depend on your client settings.
      Show
      To test this, you need outgoing emailing working. You may find $CFG->divertallemailsto useful. As a user, subscribe into a forum, using the default forum subscription preference to receive one email per one post. As an admin/teacher, post into the forum, while having checked "Send forum post notifications with no editing-time delay". R Reply to that post, again with the checkbox checked. Run cron Check the subscribed user's inbox TEST: The forum post notification email should have two emails. Their SMTP header Message-ID must have a value like <some-long-SHA1-hash@wwwroot> and not like <some-short-MD5-hash@hostname> . TEST: The reply should ideally be displayed as an email reply, but this may be depend on your client settings.
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_32_STABLE
    • Fixed Branches:
      MOODLE_32_STABLE
    • Pull from Repository:
    • Pull 3.2 Branch:
      MDL-57474-32-messageid
    • Pull Master Branch:
      MDL-57474-master-messageid

      Description

      Hi,

      I detected this because, since some days ago, my email client (Thunderbird) that strongly relies on the "Message-ID" and their counterparts ("In-Reply-To" & "References") are not matching anymore, so I'm getting here all the forum messages without proper threading.

      Looking to old mails, they were coming with these type of "Message-ID" headers:

      <e1e0ae7cfa4b8e8d003b9e34b793b88f5a76706c4c193b0891aca1b2cc89b625@moodle.org>
      (that are the usual ones, calculated by us in moodle, see generate_email_messageid())

      But, since some days ago, those "Message-ID" headers look like:

      <56bf7dccec6b7e666ea933e354721f73@education-primary.srv.in.moodle.com>

      (completely different from the ones generated by Moodle and obviously, "In-Reply-To" & "References" - also generated by us - do not match anymore).

      I've been looking for code changes but have found nothing justifyng that change. So I think this is something related with some moodle.org configuration change (within moodle, or php.ini mail setting, or mail server itself, or gmail...). But it seems clear that now our (moodle-generated) headers are not being sent anymore, but replaced by another one, completely different.

      MDL-56000 has been identified as a potential change affecting this.

      So matching does not happen anymore and threads are destroyed for some email clients.

      Ciao

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Attaching raw_examples.zip file, per Paul's request, that contains:

              A) CORRECT folder: 1st and 2nd messages of the discussion:
              https://moodle.org/mod/forum/discuss.php?d=343793 (December 1st)
              (message-id header in first message is correct and matches the in-reply-to/references of the second)

              B) INCORRECT folder: 1st and 2nd messages of the discussion:
              https://moodle.org/mod/forum/discuss.php?d=343835 (December 4th, immediately after upgrade to 3.2rcX)
              (message-id header in first message is incorrect and does not match the in-reply-to/references of the second)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Attaching raw_examples.zip file, per Paul's request, that contains: A) CORRECT folder: 1st and 2nd messages of the discussion: https://moodle.org/mod/forum/discuss.php?d=343793 (December 1st) (message-id header in first message is correct and matches the in-reply-to/references of the second) B) INCORRECT folder: 1st and 2nd messages of the discussion: https://moodle.org/mod/forum/discuss.php?d=343835 (December 4th, immediately after upgrade to 3.2rcX) (message-id header in first message is incorrect and does not match the in-reply-to/references of the second)
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Possible interesting reference (upgrade PHPMailer for Moodle 3.2 rom 5.2.14 to 5.2.16): MDL-56000

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Possible interesting reference (upgrade PHPMailer for Moodle 3.2 rom 5.2.14 to 5.2.16): MDL-56000
              Hide
              prg3 Paul Greidanus added a comment -

              I can't find any infrastructure changes during this time that relate to email.

              Show
              prg3 Paul Greidanus added a comment - I can't find any infrastructure changes during this time that relate to email.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Ok, at this point, and after discussing and agreeing with Paul, I'm moving this issue to MDL because it seems to be a real regression, maybe introduced with PHPMailer upgrade or so - MDL-56000.

              We also have checked recent mails from other moodle.org/com sites (partners...) still using older Moodle versions... and the same mail server is sending the Message-ID headers properly. So, for some reason, Moodle 3.2 is not and PHPMailer imposes its own.

              Thanks Paul, moving...

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Ok, at this point, and after discussing and agreeing with Paul, I'm moving this issue to MDL because it seems to be a real regression, maybe introduced with PHPMailer upgrade or so - MDL-56000 . We also have checked recent mails from other moodle.org/com sites (partners...) still using older Moodle versions... and the same mail server is sending the Message-ID headers properly. So, for some reason, Moodle 3.2 is not and PHPMailer imposes its own. Thanks Paul, moving...
              Hide
              mudrd8mz David Mudrák added a comment -

              I can second the findings and symptoms in my Mutt client. It seems that the Message-ID is using id
              ...@education-primary.srv.in.moodle.com whereas remaining headers like In-Reply-To and References are using ...@moodle.org identifiers. So they do not match any more.

              Show
              mudrd8mz David Mudrák added a comment - I can second the findings and symptoms in my Mutt client. It seems that the Message-ID is using id ...@education-primary.srv.in.moodle.com whereas remaining headers like In-Reply-To and References are using ...@moodle.org identifiers. So they do not match any more.
              Hide
              mudrd8mz David Mudrák added a comment -

              It looks like this is happening to us:

              All other emails though don't have a message id set in moodle so it gets autogenerated somewhere up the chain which defaults to the OS hostname box which is often different to the moodle domain

              From the description of MDL-53102

              Show
              mudrd8mz David Mudrák added a comment - It looks like this is happening to us: All other emails though don't have a message id set in moodle so it gets autogenerated somewhere up the chain which defaults to the OS hostname box which is often different to the moodle domain From the description of MDL-53102
              Hide
              mudrd8mz David Mudrák added a comment -

              I can confirm this is a 3.2 regression as I am able to reproduce it locally on my notebook.

              • Forum posts in 3.1 have the Message-ID header correctly set (forum does it explicitly). All other emails (such as forgotten password one) get Message-ID auto-generated using the uniqid() in the function generate_email_messageid().
              • Forum posts in 3.2 have the Message-ID generated by phpmailer itself, using its uniqueid property (which is basically MD5 of the time).

              The relevant code that has been changed upstream in phpmailer and is likely to cause this is createHeader() method:

              -        if ($this->MessageID != '') {
              +        if ('' != $this->MessageID and preg_match('/^<.*@.*>$/', $this->MessageID)) {
                           $this->lastMessageID = $this->MessageID;
                       } else {
                           $this->lastMessageID = sprintf('<%s@%s>', $this->uniqueid, $this->serverHostname());
              

              The preg_match() part added in recent phpmailer upgrade does not pass because of heading whitespace in our custom message-id which comes from our own phpmailer wrapper. Fix is easy and on the way.

              Show
              mudrd8mz David Mudrák added a comment - I can confirm this is a 3.2 regression as I am able to reproduce it locally on my notebook. Forum posts in 3.1 have the Message-ID header correctly set (forum does it explicitly). All other emails (such as forgotten password one) get Message-ID auto-generated using the uniqid() in the function generate_email_messageid() . Forum posts in 3.2 have the Message-ID generated by phpmailer itself, using its uniqueid property (which is basically MD5 of the time). The relevant code that has been changed upstream in phpmailer and is likely to cause this is createHeader() method: - if ($this->MessageID != '') { + if ('' != $this->MessageID and preg_match('/^<.*@.*>$/', $this->MessageID)) { $this->lastMessageID = $this->MessageID; } else { $this->lastMessageID = sprintf('<%s@%s>', $this->uniqueid, $this->serverHostname()); The preg_match() part added in recent phpmailer upgrade does not pass because of heading whitespace in our custom message-id which comes from our own phpmailer wrapper. Fix is easy and on the way.
              Hide
              mudrd8mz David Mudrák added a comment -

              Patch for 3.2 and 3.3 submitted for peer-review and integration. I have also hot-fixed moodle.org so we should have this tested in real world even before it is integrated.

              Show
              mudrd8mz David Mudrák added a comment - Patch for 3.2 and 3.3 submitted for peer-review and integration. I have also hot-fixed moodle.org so we should have this tested in real world even before it is integrated.
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks with warnings.

              Checked MDL-57474 using repository: git://github.com/mudrd8mz/moodle.git

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-57474 using repository: git://github.com/mudrd8mz/moodle.git MOODLE_32_STABLE (0 errors / 1 warnings) [branch: MDL-57474-32-messageid | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , master (0 errors / 1 warnings) [branch: MDL-57474-master-messageid | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , Should these errors be fixed?
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Great finding David! Some hours ago I found this that put me on advise about the validation, but then, noob-me I looked to upstream diff @ github and failed to find it because... it's collapsed! grrr. I should have looked to our repo instead.

              Anyway, changes make sense, if I start receiving the correct message-id headers here, I'll send this to integration. Testing instructions et all seem correct and the thirdparty warning reported by CiBoT, 100% ignorable, because we are only modifying our extended class, not imported one.

              Thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Great finding David! Some hours ago I found this that put me on advise about the validation, but then, noob-me I looked to upstream diff @ github and failed to find it because... it's collapsed! grrr. I should have looked to our repo instead. Anyway, changes make sense, if I start receiving the correct message-id headers here, I'll send this to integration. Testing instructions et all seem correct and the thirdparty warning reported by CiBoT, 100% ignorable, because we are only modifying our extended class, not imported one. Thanks!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Ok, just to confirm that the fix seems to be working:

              From https://moodle.org/mod/forum/discuss.php?d=344709

              1st message: correct

              Message-ID: <1afb9ca92403e81b426a08e0f4a1e78c85bf2beca1a3074aefd2ef3547ffdc45@moodle.org>
              

              Replies: correct

              In-Reply-To: <1afb9ca92403e81b426a08e0f4a1e78c85bf2beca1a3074aefd2ef3547ffdc45@moodle.org>
              References: <1afb9ca92403e81b426a08e0f4a1e78c85bf2beca1a3074aefd2ef3547ffdc45@moodle.org>
              

              And getting it properly threaded in my mail client too. So... sending to integration, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Ok, just to confirm that the fix seems to be working: From https://moodle.org/mod/forum/discuss.php?d=344709 1st message: correct Message-ID: <1afb9ca92403e81b426a08e0f4a1e78c85bf2beca1a3074aefd2ef3547ffdc45@moodle.org> Replies: correct In-Reply-To: <1afb9ca92403e81b426a08e0f4a1e78c85bf2beca1a3074aefd2ef3547ffdc45@moodle.org> References: <1afb9ca92403e81b426a08e0f4a1e78c85bf2beca1a3074aefd2ef3547ffdc45@moodle.org> And getting it properly threaded in my mail client too. So... sending to integration, thanks!
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks with warnings.

              Checked MDL-57474 using repository: git://github.com/mudrd8mz/moodle.git

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-57474 using repository: git://github.com/mudrd8mz/moodle.git MOODLE_32_STABLE (0 errors / 1 warnings) [branch: MDL-57474-32-messageid | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , master (0 errors / 1 warnings) [branch: MDL-57474-master-messageid | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , Should these errors be fixed?
              Hide
              poltawski Dan Poltawski added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.
              TIA and ciao

              Show
              poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks with warnings.

              Checked MDL-57474 using repository: git://github.com/mudrd8mz/moodle.git

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-57474 using repository: git://github.com/mudrd8mz/moodle.git MOODLE_32_STABLE (0 errors / 1 warnings) [branch: MDL-57474-32-messageid | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , master (0 errors / 1 warnings) [branch: MDL-57474-master-messageid | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/1) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , Should these errors be fixed?
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks David, integrated to master and 32

              Show
              poltawski Dan Poltawski added a comment - Thanks David, integrated to master and 32
              Hide
              abgreeve Adrian Greeve added a comment -

              Tested on 3.2, and master.
              I received results similar to what Eloy posted above.
              Test passed.

              Show
              abgreeve Adrian Greeve added a comment - Tested on 3.2, and master. I received results similar to what Eloy posted above. Test passed.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

              “We should not only use all the brains we have, but all that we can borrow.”
              — Woodrow Wilson

              Show
              poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. “We should not only use all the brains we have, but all that we can borrow.” — Woodrow Wilson

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  4 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    9/Jan/17