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 additional MimeFilters to the FormatOptions #546

Conversation

EssentialNRG
Copy link

Adds a new configuration option AdditionalMimeFilters to the FormatOptions. This can be used to filter and amend the output that gets written.

The reason behind is that we (at my company) use this to overwrite the From/To/Cc/Bcc headers (using a custom filter) to always have quoted printable names. This is because our (smarthost) mail server overrides these headers to always have quoted printable names (e.g.: =?UTF-8?Q?info?= [email protected]). This poses a problem because the message is signed using the normal unquoted variant, but then gets actually sent using the quoted variant. This makes the DKIM signature invalid, marking the mail as spam.

Tried several other changes (making methods public, virtual, non-static, etc). But this seems to be the least intrusive solution.

@jstedfast
Copy link
Owner

jstedfast commented Mar 26, 2020

I don't think this will work if you end up writing a complex message because the same filters will be reused when writing the content of each part and their state won't be properly reset.

What you'd really need, at a minimum, is a way for the FormatOptions to create new filters for each part.

That said, I still wouldn't really like approach.

(Although I do like the FilteredStream.AddRange() method - so I'll probably merge that part regardless)

If my understanding is correct - you only need this for DKIM-signing your messages so that you can work around a bug in your SMTP mail server (which re-formats the headers)?

This seems like such a niche requirement, I'm kind of tempted to suggest just iterating over the message.Headers and reformatting the To/From/Cc/Bcc headers manually.

foreach (var header in message.Headers) {
    switch (header.Id) {
    case HeaderId.From:
    case HeaderId.To:
    case HeaderId.Cc:
    case HeaderId.Bcc:
        var reformatted = ReformatAddressHeader (header.Value); // or use header.RawValue
        header.Value = reformatted;
        break;
    }
}

At some point, I was thinking of making the Header class overridable (which would actually be the best solution for your scenario).

@EssentialNRG
Copy link
Author

Thanks for your quick reply!

Unfortunately setting the header.Value doesn't work. Since setting this value will invoke SetValue which will then invoke FormatRawValue and reformats/encodes the addresses by invoking InternetAddressList.TryParse.

The best solution would be indeed to make Header class overridable so the GetRawValue can get overriden. But this means making the GetRawValue method public instead of internal, since it is also used outside the Header class. We thought that that would be too much of a change.

@jstedfast
Copy link
Owner

I don't think GetRawValue() would need to be made public. My idea would be to make the FormatRawValue() be virtual instead.

Unfortunately, this would require a large change...

If you can wait a bit, I can look into this as soon as I finish the preview/snippet generator that I'm currently working on.

This is something I was planning on doing for a few years now, just didn't have a good excuse to do it.

@EssentialNRG
Copy link
Author

Making FormatRawValue() virtual would be even better and would really help us here. I don't think that only making the method a protected virtual would be a large change, but I think you also have some other changes in mind.

I can wait a bit. Will keep an eye on the project then!

@jstedfast
Copy link
Owner

I just had a thought:

Another (shorter-term) option might be to allow setting the RawValue...

Long-term, I still want to have subclasses of Header for each of the specific types of headers that need special formatting. E.g. AddressListHeader, MessageIdHeader, DkimSignatureHeader, ReferencesHeader, ReceivedHeader, etc.

@EssentialNRG
Copy link
Author

Ah, I know what you mean. Different subclasses of the Header class would indeed mean a large number of changes.
Allowing the RawValue of the Header would be a good short-term solution until then.

@jstedfast
Copy link
Owner

Yea, I had other changes in mind... like basically moving all of the header-specific formatting logic out of the base Header class and into header-specific subclasses of Header.

I started to do this:

using System;
using System.Text;

namespace MimeKit.Headers
{
	public class AddressListHeader : Header
	{
		public AddressListHeader (HeaderId id, string value) : base (id, value)
		{
		}

		public AddressListHeader (string field, string value) : base (field, value)
		{
		}

		public AddressListHeader (Encoding encoding, HeaderId id, string value) : base (encoding, id, value)
		{
		}

		public AddressListHeader (string charset, HeaderId id, string value) : base (charset, id, value)
		{
		}

		public AddressListHeader (Encoding encoding, string field, string value) : base (encoding, field, value)
		{
		}

		public AddressListHeader (string charset, string field, string value) : base (charset, field, value)
		{
		}

		protected AddressListHeader (ParserOptions options, HeaderId id, string name, byte[] field, byte[] value) : base (options, id, name, field, value)
		{
		}

		protected internal AddressListHeader (ParserOptions options, byte[] field, byte[] value, bool invalid) : base (options, field, value, invalid)
		{
		}

		protected internal AddressListHeader (ParserOptions options, HeaderId id, string field, byte[] value) : base (options, id, field, value)
		{
		}

		protected override byte[] FormatRawValue (FormatOptions format, Encoding encoding, string value)
		{
			var encoded = new StringBuilder (" ");
			int lineLength = Field.Length + 2;
			InternetAddressList list;

			if (!InternetAddressList.TryParse (Options, value, out list))
				return (byte[]) format.NewLineBytes.Clone ();

			list.Encode (format, encoded, true, ref lineLength);
			encoded.Append (format.NewLine);

			if (format.International)
				return Encoding.UTF8.GetBytes (encoded.ToString ());

			return Encoding.ASCII.GetBytes (encoded.ToString ());
		}
	}
}

The downside is that I would need to add some sort of registrar so that the parser and other parts of MimeKit (MimeMessage, MimeEntity, Header.TryParse(), etc) could all instantiate the correct sublcass.

The next problem is that the current API allows developers to add headers to messages or parts via:

message.Headers.Add (new Header (...));

If I move all of the FormatRawValue() logic out of the base Header class and into the subclasses, then developers already using the above way of doing things will suddenly start having broken header value formatting which I don't want to happen.

That doesn't mean I can't make Header more overridable by e.g. making FormatRawValue() virtual, it just means I need to leave all of the existing logic in the base class.

So I think I'll commit the Header changes I've made so far to facilitate my original idea, I just won't bother proceeding to write up a bunch of subclasses (which is a shame because I would have loved to move that logic out of Header to make it cleaner).

@jstedfast
Copy link
Owner

I'm also adding Header.SetRawValue(byte[] value) which may actually be an easier solution for you.

I also had to add an internal bool explicitRawValue which, if true, disallows re-formatting.

jstedfast added a commit that referenced this pull request Mar 29, 2020
@jstedfast jstedfast closed this Mar 29, 2020
@EssentialNRG
Copy link
Author

Thanks for the changes @jstedfast, they look promising and workable for me!

Refactoring the Header classes would indeed introduce an incompatibility. It would be a nice change of course, but more something for a new major release instead of a minor update.

@jstedfast
Copy link
Owner

It will probably be a few more days before I'm able to make a 2.6.0 release. It's almost ready now, but I'd like to resolve the BouncyCastleCryptographyContext feature request that someone just submitted on Friday. Hopefully I hear back what exactly is needed early this week and can have it implemented and push out a release by the end of the week.

In the meantime, you can try out the latest build @ https://www.myget.org/feed/mimekit/package/nuget/MimeKit and verify that things are workable for you. If not, you have some time to raise the red flag before I make a release :-)

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

Successfully merging this pull request may close these issues.

2 participants