blueimp / aws-smtp-relay

SMTP server to relay emails via Amazon SES or Amazon Pinpoint using IAM roles.
MIT License
76 stars 34 forks source link

Problem dealing with multivalue inputs for destination #3

Closed fubarhouse closed 5 years ago

fubarhouse commented 5 years ago

Could I please get some advice on the following issue which I've come across?

I've been brought an issue and tried to reproduce the problems in the test suite you've provided with this module/binary. Note that the changes simply adapt the existing test to support multi-value to fields with checks against those values as the existing check only checked the first value.

Code changes

func TestSend(t *testing.T) {
    origin := net.TCPAddr{IP: []byte{127, 0, 0, 1}}
    from := "alice@example.org"
    to := []string{"bob@example.org", "jim@example.net", "fred@example.dev"}
    data := []byte{'T', 'E', 'S', 'T'}
    setName := ""
    timeBefore := time.Now()
    input, out, err := sendHelper(&origin, &from, &to, &data, &setName)
    timeAfter := time.Now()
    if *input.Source != from {
        t.Errorf(
            "Unexpected source: %s. Expected: %s",
            *input.Source,
            from,
        )
    }
    for _, y := range input.Destinations {
        fmt.Println(string(*y))
    }
    for i := range to {
        if *input.Destinations[i] != to[i] {
            t.Errorf(
                "Unexpected destination: %s. Expected: %s",
                *input.Destinations[i],
                to[i],
            )
        }
    }

Console output

 01:46 PM:~/Projects/aws-smtp-relay $ make test
go test ./...
ok      github.com/blueimp/aws-smtp-relay       (cached)
fred@example.dev
fred@example.dev
fred@example.dev
--- FAIL: TestSend (0.00s)
    relay_test.go:80: Unexpected destination: fred@example.dev. Expected: bob@example.org
    relay_test.go:80: Unexpected destination: fred@example.dev. Expected: jim@example.net
FAIL
FAIL    github.com/blueimp/aws-smtp-relay/internal/relay        0.019s
make: *** [test] Error 1
 01:50 PM:~/Projects/aws-smtp-relay $ 

Observations

tobybellwood commented 5 years ago

For us it corresponds with the real world scenario:

The "to" names reported from https://github.com/blueimp/aws-smtp-relay/blob/master/internal/relay/relay.go#L50-L57 looks ok

{"Time":"2019-05-01T00:54:56.66669728Z","IP":"10.1.32.1","From":"source@email.com","To":["dest1@email.com","dest2@email.com"],"Error":""}

but when it gets to the send step, and creates "destinations" - we see the second address twice in the AWS Kinesis log

"destination": ["dest2@email.com", "dest2@email.com"],

Is there a safer way to assemble the destinations in https://github.com/blueimp/aws-smtp-relay/blob/master/internal/relay/relay.go#L58-L61 ?

Karl's test replicates this

fubarhouse commented 5 years ago

Thank you! 🎉

tobybellwood commented 5 years ago

Wow! Perfect!

blueimp commented 5 years ago

Thanks for the report @fubarhouse and @tobybellwood!

That was actually a very embarrassing bug in the destination handling. The previous code iterated over the list of recipients assigning to a local variable. However since ses.SendRawEmailInput expects a list of string references (instead of a list of strings), passing in the local variable effectively set all destinations to the last one in the iteration.

Thanks again for letting me know. ☺️

fubarhouse commented 5 years ago

Thank you for being so prompt!

blueimp commented 5 years ago

You're welcome!

I just moved from Germany to Tokyo, so we're effectively in the same timezone now and I can potentially respond quicker. 🇯🇵🇦🇺

tobybellwood commented 5 years ago

I spent an embarrassing amount of hours poring through logs (in S3 via Kinesis - thanks to #2!) before I discovered the issue!