wneessen / go-mail

📧 Easy to use, yet comprehensive library for sending mails with Go
https://go-mail.dev
MIT License
551 stars 42 forks source link

Occasionally Missing "Content-Disposition: attachment" and Line 1 is "Content-Type: multipart/mixed" instead of Date #240

Open jameshueston opened 2 months ago

jameshueston commented 2 months ago

Description

200 EML files were written to disk from uniquely separate IoT devices running the same code over the last several months. 198 were "Good" - opened in Email clients from users on separate domains, viewed attachments & body. 2 were "Bad", meaning

  1. they do not show attachments in Email Clients, and
  2. their body shows multiple equal signs throughout.

The outcome is the same across 3 Email Clients:

"Good" emails:

"Bad" emails:

All emails (Good and Bad) were built the same - fields REDACTED / refactored for clarity:

msg := mail.NewMsg()
_ = msg.From("REDACTED@REDACTED.com")
_ = msg.To(REDACTEDConfig.Mail.ToAddresses...)   // which is []string
msg.Subject(getMailSubjectForREDACTED(...))
msg.SetBodyString(getMailBodyForREDACTED(o))

// Attach XLSX file and JSON file
for i := range filepaths {
    msg.AttachFile(filepaths[i], mail.WithFileName(filenames[i]))
}

...

func getMailBodyForREDACTED(o *REDACTED) (ct mail.ContentType, body string) {
    ct = mail.TypeTextPlain

    body = body + "Attached is the REDACTED." + mail.SingleNewLine
    body = body + mail.SingleNewLine
    body = body + "REDACTEDID: " + o.REDACTEDID + mail.SingleNewLine
    ...
    // 40 lines total lines building the body as one long string

    return
}

To Reproduce

I haven't been able to reproduce this locally yet.

Expected behaviour

All emails would be consistently generated under the "Good" section above.

Screenshots

No response

Attempted Fixes

I haven't done this yet -- and since I haven't been able to recreate yet:

  1. On Msg Body, explicitly set:
    Content-Transfer-Encoding: quoted-printable  // ensure email clients don't print equal signs and wrap at 76 characters; instead treat as continuous text
    Content-Type: text/plain; charset=UTF-8
  2. On Excel XLSX Attachment, explicitly set:
    filename="######-MonthlyReport-XXXXXXXX-XXX_X-##.xlsx" // shorter than before
    Content-Transfer-Encoding: base64  // even though base64 is the default
    Content-Type: application/octet-stream // even though this hasn't been misinterpreted
    name="######-MonthlyReport-XXXXXXXX-XXX_X-##.xlsx" // shorter than before
  3. On JSON Attachment, explicitly set:
    filename="######-MonthlyReport-XXXXXXXX-XXX_X-##.json"
    Content-Transfer-Encoding: base64
    Content-Type: application/json name="######-MonthlyReport-XXXXXXXX-XXX_X-##.json"
  4. Verify after writing to disk:
    1. read the EML from disk
    2. if first four characters are not "Date" OR if "Content-Disposition: attachment"' is not contained, then
      1. Log ERROR
      2. move file with timestamp at end for later debugging
      3. re-generate MSG and write EML again
    3. repeat 1 & 2 again to see if we strike gold

Additional context

No response

wneessen commented 2 months ago

Hi @jameshueston,

thanks for the report. I'll have a look into it and let you know my findings.

wneessen commented 2 months ago

@jameshueston Would you be able to provide me with an (redacted) EML of each a good and a bad mail? As I understand this issue you have concerns regarding the order of the header fields in the EML, but according to the RFC the order of the header fields may appear in any order, so it should not make a difference is the first field is "Date:" or "Content-Type:". If I get examples of the EML files, I might be able to find the reason why they are not interpreted correctly by the mail clients. You don't have to attach the files to the issue, you can also mail them to me (GPG encryption is supported as well). Please let me know.

jameshueston commented 2 months ago

@wneessen - I have redacted files ready; let me know which email you prefer. If you'd rather not post your address here, feel free to message me either at the address I used on the message when I bought you a coffee :) or at the address on my profile.

Thanks in advance!

wneessen commented 2 months ago

You can use the address in the https://go-mail.dev/security/ page.

PS: Thanks a lot for the Coffee! Much appreciated!

wneessen commented 2 months ago

Hi @jameshueston,

thanks for providing me with the mail examples. I was able to identify the reason for the mail clients not being able "understand" the mails, yet I cannot find a logical reason for this happening at all.

The reason for the mail appearing to be broken is a double Content-Type header. In particular there seems to be a complete mix-up of the multipart Content-Type headers.

In your example of a bad mail we have:

Content-Type: multipart/mixed;
 boundary=54793082b051293332802e7cb7085ce36ed118ecdcfa0216a0015e3c2ca6
Date: Wed, 01 May 2024 01:20:17 +0000
[...]
Content-Type: multipart/alternative;
 boundary=e1cb711bc948256b16dc8aa7d3b1c32adf396581aeb11fe64f58c25dd095

This makes absolutely no sense to even occur. The generic headers (Date, MIME-Version, etc.) are all collected in a map, then sorted and written. The thing is though, that the Content-Type should not appear in the map at this time at all. The CT is set in a later part of the code, when the generic headers are already set and sorted. Also there shouldn't be two different sets of boundaries and in fact the CT for the mail (the correct one) should not be multipart/alternative but multipart/mixed (as it is for the first header in the bad mail).

I tried a couple of different scenarios in which I intetionally corrupted headers and attachments/mail parts but in none of them I was able to reproduce this behaviour. I used the test code you provided (in similar form) and wrote 100k test mails and not one of them showed this behaviour you are experiencing.

Some things (except of what I already mentioned) about the corrupted mail I can see:

So in conclusion, the only explanation I can come up with at this point is that maybe some strange file locking or race condition must have happend that must have combined two mail generation processes into one or maybe some kind of file system corruption. Code-wise I don't see any scenario in which this could happen except maybe intentionally messing up things in the internals of the code that are not publicly accessible. I'd like to give you a better exmplanation but at this point I'm clueless.

wneessen commented 2 months ago

Here is the quick and dirty test code I used (based on your example) for trying to reproduce:

package main

import (
    "fmt"
    "math/rand"
    "time"

    "github.com/wneessen/apg-go"

    "github.com/wneessen/go-mail"
)

func main() {
    for i := 1; i <= 1000; i++ {
        id := rand.Int63()
        fmt.Println("Writing mail no. ", i, " with ID: ", id)
        writeMail(i, id)
    }
}

func writeMail(no int, id int64) {
    toaddr := []string{"toni@tester.com", "antonia@example.com"}
    filepaths := []string{"test.json", "test.xlsx"}
    now := time.Now()

    msg := mail.NewMsg()
    if err := msg.From("test@test.com"); err != nil {
        fmt.Printf("failed to set FROM: %s\n", err)
    }
    if err := msg.To(toaddr...); err != nil {
        fmt.Printf("failed to set TO: %s\n", err)
    }
    msg.Subject(getMailSubject(id))
    msg.SetBodyString(getMailBody(id))

    // Attach XLSX file and JSON file
    for i := range filepaths {
        msg.AttachFile(filepaths[i], mail.WithFileName(fmt.Sprintf(`attachment_%d_%d_%s`, id,
            now.UnixMilli(), filepaths[i])))
    }

    if err := msg.WriteToFile(fmt.Sprintf("output/testmail_%d.eml", no)); err != nil {
        fmt.Println("failed to write mail:", err)
    }
}

func getMailBody(id int64) (mail.ContentType, string) {
    gen := apg.New(apg.NewConfig(apg.WithFixedLength(80), apg.WithAlgorithm(apg.AlgoRandom)))
    body := "Attached is the REDACTED." + mail.SingleNewLine
    body = body + mail.SingleNewLine
    body = body + fmt.Sprintf("%s: %d%s", "REDACTEDID: ", id, mail.SingleNewLine)
    for i := 0; i < 40; i++ {
        text, err := gen.Generate()
        if err != nil {
            continue
        }
        body = body + text + mail.SingleNewLine
    }

    return mail.TypeTextPlain, body
}

func getMailSubject(id int64) string {
    return fmt.Sprintf("This is the subject for Mail: %d", id)
}

(will require the two test files being present as well as a output/ directory)

To check if any of the mails is corrupt you can use the following code snipptes:

$ grep -i alternative output/*
$ head -1 output/* | grep -v '==>' | grep -v Date | perl -ne '/^\n$/ || print'

Both should not return anything. Maybe you want to run the testcode on one of the devices that has seen the corrupt mails and see if you can re-produce it that way? You can change the amount of generated mails in the for loop of the main() method.

jameshueston commented 2 months ago

Thanks, @wneessen, for looking into this!

I may indeed have strange file locking that I dismissed early on, because:

wneessen commented 2 months ago

Hi @jameshueston,

I don't think that reusing the *mail.Client would cause this issue but I could imagine that somewhere else in the process something might have broken. Especially given that we make use of some maps in the *Msg which are known to be not concurrency-safe. I could see a concurrency issue in the "go routine monitors the folder and picks up EML" part of the process. Not sure if you're already using some kind of mutex/locking mechanismn in that process but if not maybe it's worth a try?

At this point i'm not sure if there is something in the go-mail codebase right now that would actively cause this issue - execept of it not being concurrency-safe (which could be an improvement for a future release at some point)

wneessen commented 2 months ago

Hi again @jameshueston,

while working on the EML code this weekend, I identified a weird case in which I was able to replicate the Content-Type: multipart/mixed header in the first line. It was basically an issue where the EML parser would create a new part with that content type, even though it must not do this, as the msgwriter will take care of setting these kind of headers.

The latest code in the branch is now more mature and that issue should be fixed. I can't 100% confirm if that was the issue causeing the weird behaviour on your end but it is a possiblity. The branch should now also be able to handle multipart message with attachments and embeds without issues.

Hope this helps.

jameshueston commented 1 month ago

Thanks @wneessen, good discovery! More coffee sent :) It may be some time before I can try the branch, but when I do, I'll provide an update here.

wneessen commented 1 month ago

Thanks again James, Meanwhile the branch has been merged into v0.4.2. So instead give that a try ;)