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

use MIME::Entity and MIME::Parser from MIME::Tools instead of Email::MIME #217

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mirespace
Copy link

Hello and Happy New year,

as I raised issue #216 before the Xmas holidays (I was off that period, sorry), here I made the PR for it.

Could you take a look on it and share your opinion on this? Thank you.

@mirespace mirespace marked this pull request as ready for review January 9, 2024 09:54
@msimerson msimerson closed this Mar 13, 2024
@mirespace
Copy link
Author

mirespace commented Mar 20, 2024

Hi,

After talking to the maintainer of Email::MIME, I understand that this PR could be unnecessary from a security point of view, but I was happy to see progress in exploring the use of this PR (thank you for that, Matt!).

I continue using this approach as:

  • it implies fewer dependencies
  • In Ubuntu, there are packages already in the main repository that do the same as Email::MIME.

I know this PR could still be improved if the part of the email composition is also fully switched (from Email::Simple).

Could I do something to help with this?

@msimerson
Copy link
Owner

it implies fewer dependencies

If one was to just look at this PR, and see one dependency replaced with four, they might conclude otherwise. Perhaps something a bit less squishy such as this table which compares the dependencies to be installed on an otherwise bare OS with only perl installed:

Delta OS Total Dependencies Ubuntu packages from CPAN
Email::MIME Ubuntu 20 N N N
MIME::Tools Ubuntu 20 N N N

Then I'd be inclined to do a similar comparison on another platform (like FreeBSD) and perhaps the results would indicate that making a substantial change like this is worth the risk. (Mail::DMARC is a dependency of SpamAssassin 4.0 so I wouldn't be surprised to see a large increase in installations).

The test coverage for packaging up an email are rather thin. If such a change were deemed advisable, I'd like to see a before-and-after test that compares the packaged email message before and after this PR.

I might also go back and read the latest DMARC RFC (I wrote this module before the RFC was published) and make sure what we're doing is still abiding by the RFC. That's probably out-of-scope for this PR.

@marcbradshaw
Copy link
Collaborator

I might also go back and read the latest DMARC RFC (I wrote this module before the RFC was published) and make sure what we're doing is still abiding by the RFC. That's probably out-of-scope for this PR.

DMARCbis is rapidly approaching done, and there will be changes to be made to be compliant with the new spec. But also clearly out of scope for this PR.

@marcbradshaw
Copy link
Collaborator

I think both MIME::Tools and Email::MIME are just fine for this use case, what I don't see is a compelling case to justify switching. The case made in #216 was uncertainty over if an issue had been fixed, and we addressed that. It has been fixed, and I'm also confident I could push any other critical fixes through should they arise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants