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

Add configurable mime boundary generator #1481

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

Conversation

dugancathal
Copy link

@dugancathal dugancathal commented May 2, 2022

Hello!

This PR aims to add the ability to customize the generator used for
mime-boundaries when creating mails.

Recently, after converting our application to use ActionMailer (and thus, Mail),
my team found that several of our customers stopped receiving our emails due to
"malformed structures". Upon digging in, it appears their systems may be
flagging our emails because of the new email's mime-boundary containing an equal
sign (=). While the RFC states that this character is valid in the boundary
(provided it's quoted), we have found a couples instances noting that it's
merely safer to not include them at all.

Example 1
Example 2

Rather than request a change to the boundary generation algorithm here, we
instead make the algorithm configurable to hopefully prevent any impact to
current consumers.

Feedback is, of course, welcome on the code structures presented here and/or any
other alternative options that may help us diagnose the issue.

Thank you!


Tested on Ruby 3.0.2, 2.7.3, and 2.6.5

@mikel
Copy link
Owner

mikel commented Nov 30, 2022

Hi there @dugancathal this is excellent, though I think we can make this change to the random tag generator to remove the = sign from it as well. Could you please modify the random_tag method to exclude = from it? Also, please add a changelog entry and I'll get this deployed in the next 2.9 release.

@dugancathal
Copy link
Author

Hey @mikel! Thanks for the review.

I'm not 100% sure I follow, so I'd like to clarify. Are you looking for me to:

  1. Undo the change here and instead completely remove the = from the generate_boundary method?
  2. Update the Mail.random_tag method to, for instance, gsub('=', '') (or similar)?
  3. Something else?

Looking at the code, I'm not sure that [2] would do anything as all the components of that string are numeric and will not include equal signs (=).

Just lemme know which you're lookin' for and I'll get this updated for release.

Cheers!

@mikel
Copy link
Owner

mikel commented Dec 3, 2022

Hey there @dugancathal - you mention the reason you created the custom mime boundry generator capability is because Mail is including a = in the mime boundry generated.

So what I'm proposing is:

  1. Keep these changes (allowing people to override the mime boundry generator is a good thing, maybe they want a custom boundry for some reason)
  2. ALSO modify the default mail mime boundry generator itself so that it does not include an = in it to remove the need for 99% of people to ever need to implement their own custom one (including you) :)

Make sense?

@mikel mikel added the MIME label Dec 3, 2022
@mikel mikel added this to the 2.9.0 milestone Dec 3, 2022
@dugancathal dugancathal force-pushed the dugancathal/add-configurable-mime-boundary branch from 5d573f9 to 414b98f Compare December 7, 2022 06:26
@dugancathal
Copy link
Author

Makes sense, @mikel. Made those changes. Lemme know if this needs any other work. :)

@mikel
Copy link
Owner

mikel commented Dec 14, 2022

OK thanks @dugancathal - any chance you could look into the spec failures?

@sebbASF
Copy link
Collaborator

sebbASF commented Dec 14, 2022

AFAICT the failures are all in the Ruby setup step. This should have been solved by the fix to install libyaml-dev (not needed for JRuby, hence they succeeded).
Just trigger a re-run and tests should all succeed.

@dugancathal
Copy link
Author

Hey folks, sorry for the radio silence on this. Is there anything y'all need from me? Or is it possible for me to rebase and re-run CI somehow to see if this is still an issue?

@sebbASF
Copy link
Collaborator

sebbASF commented Feb 1, 2023

It would be helpful if you could trigger a CI rebuild by making a trivial change.
Rebasing would probably also work, though I'm not sure it's necessary here.

@dugancathal dugancathal force-pushed the dugancathal/add-configurable-mime-boundary branch from 261a310 to e9f0a99 Compare February 23, 2023 00:47
@sebbASF
Copy link
Collaborator

sebbASF commented Feb 23, 2023

I think there needs to be a test to show that the original boundary can still be generated and used.

Also: if certain characters need to be quoted, who is responsible for doing this?
Seems to me that this is the responsibility of the mail Gem, so needs to be handled in the code that wraps the user-provided boundary.

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.

None yet

3 participants