Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 N°7917 SF#2272 EmailLaminas.php: Fix Message-ID format #671

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

vlk-charles
Copy link
Contributor

Base information

Question Answer
Related to a SourceForge thead / Another PR / Combodo ticket? SourceForge ticket 2272
Type of change? Bug fix

Symptom

iTop since version 3.1 (after switching to Laminas-mail, N°4307) sends emails with incorrectly formatted Message-ID. This sometimes results in Gmail classifying them as spam or even outright refusing to accept them for delivery. In another case, a client was reporting issues with their (possibly custom) system for automated email processing. More information is in the above-linked SourceForge issue.

Reproduction procedure

  1. On iTop 3.1.1-1
  2. With PHP 7.4.33
  3. Make iTop send you an email (assuming Gmail inbox).
  4. If the email arrives at all, examine its message ID ("Show original" in Gmail web application).
  5. See that the message ID got changed to <[email protected]> and an X-Google-Original-Message-ID header was added.

Cause

After switching to the Laminas-mail library, the addHeaderLine function is used to add the Message-ID header to the email that is about to be sent. This function can add arbitrary headers and uses Q-encoding to encode the entire value. This is not allowed for structured headers such as Message-ID according to the relevant RFCs (RFC 2047, RFC 2822).

Proposed solution

Laminas-mail provides a MessageId class. An object of this type is created and added using addHeader.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailed enough so anyone can understand digging in the code?

Checklist of things to do before PR is ready to merge

@Molkobain
Copy link
Contributor

Kudos for the detailed PR 🙌

@steffunky steffunky self-assigned this Oct 22, 2024
@jf-cbd jf-cbd added the bug Something isn't working label Oct 22, 2024
@steffunky
Copy link
Member

Thanks for your contribution
I've logged your bug in our internal bug tracker under ref N°7917

@steffunky steffunky changed the title 🐛 N°2272 EmailLaminas.php: Fix Message-ID format 🐛 N°7917 SF#2272 EmailLaminas.php: Fix Message-ID format Oct 23, 2024
@vlk-charles
Copy link
Contributor Author

Below is a comparison of the header between emails sent out from different versions of iTop.

iTop 3.0:

Message-ID: <iTop_UserRequest_112260_1721928197.377377@ffb5bc6b0d3ad4e791061d65c153a36b-production.openitop.org>

iTop 3.1.1-1:

Message-ID: =?UTF-8?Q?iTop=5FUserRequest=5F9142=5F1823562304.671192@a968b15fb014ecc1b0fa085ab5c5c2f6-production.openitop.org?=

@steffunky
Copy link
Member

I just tested it, and this indeed fixes the Message-Id.

support/3.1 before this PR

Message-ID: =?UTF-8?Q?iTop=5FUserRequest=5F288=5F1729847352.614665@ca38c053d295a42819b13ca584962411-production.openitop.org?=

support/3.1 after this PR

Message-ID: <iTop_UserRequest_288_1729846346.464874@ca38c053d295a42819b13ca584962411-production.openitop.org>

I noticed In-Reply-To is also messed up but I'll try to fix it in a different PR as it's more likely the data's fault.

@steffunky steffunky merged commit 58e964f into Combodo:support/3.1 Nov 4, 2024
@steffunky
Copy link
Member

Thanks for your PR @vlk-charles!! Could you send me your full name and postal address to stephen.abello at combodo.com ? We would like to send you some of our contributor stickers 🙌

@vlk-charles vlk-charles deleted the issue/2272 branch November 4, 2024 18:16
@vlk-charles
Copy link
Contributor Author

@steffunky Thank you for the stickers. Address sent.

Will the fix be forward-ported to 3.2 and future versions?

@steffunky
Copy link
Member

Sure thing, this has been merged under 2519456 for 3.2.1 and under d0f9e57 for 3.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Finished
Development

Successfully merging this pull request may close these issues.

5 participants