mail icon indicating copy to clipboard operation
mail copied to clipboard

Add configurable mime boundary generator

Open dugancathal opened this issue 3 years ago • 9 comments

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

dugancathal avatar May 02 '22 07:05 dugancathal

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.

mikel avatar Nov 30 '22 08:11 mikel

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!

dugancathal avatar Dec 01 '22 00:12 dugancathal

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 avatar Dec 03 '22 06:12 mikel

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

dugancathal avatar Dec 07 '22 06:12 dugancathal

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

mikel avatar Dec 14 '22 09:12 mikel

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.

sebbASF avatar Dec 14 '22 09:12 sebbASF

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?

dugancathal avatar Feb 01 '23 16:02 dugancathal

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.

sebbASF avatar Feb 01 '23 17:02 sebbASF

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.

sebbASF avatar Feb 23 '23 09:02 sebbASF