Kitura / Swift-SMTP

Swift SMTP client
Apache License 2.0
261 stars 63 forks source link

Allow headers to be replaced #26

Closed collinhundley closed 7 years ago

collinhundley commented 7 years ago

I tried sending an email formatted as HTML, by including an extra header:

additionalHeaders: [(header: "Content-Type", value: "text/html")]

However, we get this error:

554 Transaction failed: Duplicate header 'Content-Type'.

It looks like SMTPSocket is still sending Content-Type: text/plain along with the new header, which is invalid.

Is there a way we can update the API to overwrite headers rather than just appending them? Or, for my specific use case, an alternative method for sending HTML emails?

quanvo87 commented 7 years ago

We will change the APIs to overwrite headers instead of append them. This is how it originally was, but was changed for safety. But now I see it's necessary to give users this power.

One alternative for sending HTML emails is creating an Attachment of type HTML and attach that along with the Mail you are sending.

collinhundley commented 7 years ago

@quanvo87 it looks like there's still one lingering issue: in DataSender.swift there's a string that seems to get prepended to every message:

static let plainTextHeader = "CONTENT-TYPE: text/plain; charset=utf-8\(CRLF)CONTENT-TRANSFER-ENCODING: 7bit\(CRLF)CONTENT-DISPOSITION: inline\(CRLF)"

Since we're still hard-coding Content-Type, there's a duplicate and it gets rejected by the server.

Are these headers actually necessary? I wonder if we could add them to the main headers dictionary prior to sending (if the user hasn't set them already), that way we avoid all conflicts.

quanvo87 commented 7 years ago

It seems that header was not necessary. In master now, it is removed completely. Can you see if that solved the issue?

Also, was sending the HTML as an attachment not an option?

collinhundley commented 7 years ago

Hey @quanvo87, we'll need a new release to test it out. However - I made the same changes on my local revision but it doesn't seem to work... the email sends, but the message body is empty. Not sure why.

quanvo87 commented 7 years ago

Could you post the email and any attachments you are sending?

collinhundley commented 7 years ago

@quanvo87 here's an example:

// Create users
let toUser = SwiftSMTP.User(email: toAddress)
let fromUser = SwiftSMTP.User(email: fromAddress)

// Create SSL credentials
let ssl = SSL(withChainFilePath: "/my/cert/path",
              withPassword: "myCertPassword",
              usingSelfSignedCerts: true)

// Create SMTP instance
let smtp = SMTP(hostname: "email-smtp.us-west-2.amazonaws.com",
                   email: "myAWSUsername",
                   password: "myAWSPassword",
                   port: 587,
                   ssl: ssl)

// Create email object
let body = "This is a test email.<br><br>It is formatted as HTML<br><br>It should work."
let email = SwiftSMTP.Mail(from: fromUser, to: [toUser],
                           subject: "This is a test", text: body,
                           additionalHeaders: [
                            "Content-Type" : "text/html; charset=utf-8"
                           ])

// Send email via AWS
smtp.send(email) { (error) in
    // ...
}

The message comes through with the correct subject, but a completely empty body.

It looks like plainTextHeader might have been important... if I keep plainTextHeader, but remove the Content-Type portion, everything works correctly.

quanvo87 commented 7 years ago

I ran this code on the latest release, and the email text was correctly formatted (I connected with my Gmail account). However when I removed the Content-Type header, the body was not blank, but also not correctly formatted.

So you're right, the plain text portion of an email should have a header in by default, but it should be text/html, not text/plain.

I don't know why the body was empty, and I couldn't reproduce that. Possibly an issue with AWS?

collinhundley commented 7 years ago

@quanvo87 I've switched over to my Gmail account to make sure it wasn't an AWS issue, but the results are the same. However, I think I've pinpointed part of the issue...

The email body that I was actually sending is slightly different. Try running the same test with this body:

"This is a test HTML message.<br><br>Here is some more text:<br>Hello."

Empty message.

But, if you remove the colon:

"This is a test HTML message.<br><br>Here is some more text<br>Hello."

It renders correctly.

It seems like the presence of a colon anywhere in the message throws everything off. But, if you prepend plainTextHeader to the message like how it used to be (but remove the Content-Type portion while keeping your additional text/html header), the message renders correctly. For example:

body = "CONTENT-TRANSFER-ENCODING: 7bit\(CRLF)CONTENT-DISPOSITION: inline\(CRLF)\(CRLF)This is a test HTML message.<br><br>Here is some more text:<br>Hello."

So, it looks like the email servers require there to be some sort of header directly attached to the message body; however, I'm not sure what the spec actually calls for. Any ideas?

quanvo87 commented 7 years ago

@collinhundley You're right! The colon caused the body to be blank. I have no idea why.

We have a PR open that addresses this. In summary,

By the way, when I sent:

body = "CONTENT-TRANSFER-ENCODING: 7bit\(CRLF)CONTENT-DISPOSITION: inline\(CRLF)\(CRLF)This is a test HTML message.<br><br>Here is some more text:<br>Hello."

My email rendered like this:

CONTENT-TRANSFER-ENCODING: 7bit CONTENT-DISPOSITION: inline This is a test HTML message.

Here is some more text: Hello.

Is this what you saw as well?

collinhundley commented 7 years ago

@quanvo87 great to hear! I glanced over the PR; it looks like this should work nicely.

With the body mentioned above, did you still set additionalHeaders to ["Content-Type" : "text/html; charset=utf-8"]? When I do that, it renders correctly:

This is a test HTML message.

Here is some more text:
Hello.
collinhundley commented 7 years ago

@quanvo87 the new release seems to mostly work - I'm now able to send HTML emails.

However, attempting to change the Content-Type gives us this error:

554 Transaction failed: Duplicate header 'Content-Type'

This happens for example if I try to send a plaintext email and set additionalHeaders to this:

additionalHeaders: ["Content-Type" : "text/plain; charset=utf-8"]

I'm not 100% sure, but it looks like we're sending headers in 2 places: in the actual request headers, and prepended to the message body. Since Content-Type is being sent in both of those places, if they don't match up we get an error.

quanvo87 commented 7 years ago

@collinhundley What should be happening is the request headers and any additionalHeaders are sent first, and these headers apply to the entire email. The rest of the email consists of subsections, which will have their own headers based on what type content they are (plaintext, mixed, alternative, related).

For plaintext, we check for Content-Type, Content-Transfer-Encoding, and Content-Disposition in the additionalHeaders. If they are not present, we use default values. So yes, headers of these types will be used twice, once for the overall email, and again for the "plaintext" section of the email. This seems to be causing problems for some servers (Gmail seems to be ok with duplicate headers), so the current implementation will not work.

Maybe any headers in additionalHeaders should not be applied to the entire section of the email, but to each subsection. This would break things where additionalHeaders are misused (but maybe we can check for this, and throw away invalid headers). We still need to think of the full implications of this.

What is the reason to set the Content-Type to text/plain for a plaintext header? It should send correctly without doing so.

collinhundley commented 7 years ago

@quanvo87 I have no reason to use test/plain myself, but was just testing stuff. However, the same error occurs if you add Content-Type: text/html; charset=utf-8 to additionalHeaders.

quanvo87 commented 7 years ago

This is addressed in the latest release https://github.com/IBM-Swift/Swift-SMTP/releases/tag/v1.0.8

collinhundley commented 7 years ago

@quanvo87 Everything seems to work great. Thanks!

quanvo87 commented 7 years ago

@collinhundley Thanks for your feedback! There's surely more improvements to be made and please don't hesitate to open up another issue

quanvo87 commented 7 years ago

Issue moved to IBM-Swift/Kitura #1131 via ZenHub