amadan
mail: harden SES event parsing — defend against ambiguous sources, control characters, stray brackets, and FQDN dots
Added four defensive improvements to DomainOf and ParseEvent: - Refuse sources with multiple @ symbols or commas (ambiguous attribution) - Strip stray trailing > from unbalanced brackets - Strip trailing FQDN dot for normalisation - Refuse control characters (< 32 or == 127) that indicate malformation Added nine test cases covering edge cases, including the three ambiguous multi-address patterns identified by robustness testing. Both original design tensions remain intact: unknown event types parse (SES adds types), missing source is refused (unattributable).
2 files changed,
+98
−2
internal/mail/events.go+28 −2internal/mail/events_test.go+70 −0
diff --git a/internal/mail/events.go b/internal/mail/events.go| index 1eb646d29..1a3d7460b 100644 |
| --- a/internal/mail/events.go |
| +++ b/internal/mail/events.go |
| @@ -28,7 +28,8 @@ type Event struct { |
| } |
| // DomainOf extracts the lowercase domain from an address, tolerating a |
| -// display-name wrapper. Empty when there is no domain to find. |
| +// display-name wrapper. Empty when there is no domain to find. Strips one |
| +// trailing dot and any stray angle bracket. |
| func DomainOf(address string) string { |
| s := strings.TrimSpace(address) |
| if i := strings.LastIndex(s, "<"); i >= 0 { |
| @@ -40,7 +41,19 @@ func DomainOf(address string) string { |
| if at < 0 || at == len(s)-1 { |
| return "" |
| } |
| - return strings.ToLower(strings.TrimSpace(s[at+1:])) |
| + dom := strings.TrimSpace(s[at+1:]) |
| + // Strip stray trailing > from unbalanced brackets |
| + if len(dom) > 0 && dom[len(dom)-1] == '>' { |
| + dom = strings.TrimRight(dom, ">") |
| + if dom == "" { |
| + return "" |
| + } |
| + } |
| + // Strip exactly one trailing dot (FQDN normalisation) |
| + if len(dom) > 1 && dom[len(dom)-1] == '.' { |
| + dom = dom[:len(dom)-1] |
| + } |
| + return strings.ToLower(dom) |
| } |
| // ParseEvent reads one SES notification. |
| @@ -68,6 +81,19 @@ func ParseEvent(body []byte) (Event, error) { |
| if raw.Mail.Source == "" { |
| return Event{}, fmt.Errorf("mail: ses event has no mail.source — it can be attributed to no tenant") |
| } |
| + // Refuse sources with commas or multiple @ symbols: they indicate multiple |
| + // address-shaped substrings, making attribution ambiguous. A single envelope |
| + // MAIL FROM must have exactly one @ and no commas. |
| + if strings.Contains(raw.Mail.Source, ",") || strings.Count(raw.Mail.Source, "@") > 1 { |
| + return Event{}, fmt.Errorf("mail: ses event source %q is ambiguous — cannot reliably attribute to one tenant", raw.Mail.Source) |
| + } |
| + // Refuse control characters, which would pass through to the domain and indicate |
| + // malformation or injection. |
| + for _, c := range raw.Mail.Source { |
| + if c < 32 || c == 127 { |
| + return Event{}, fmt.Errorf("mail: ses event source contains control character (0x%02x)", c) |
| + } |
| + } |
| dom := DomainOf(raw.Mail.Source) |
| if dom == "" { |
| return Event{}, fmt.Errorf("mail: ses event source %q has no domain", raw.Mail.Source) |
diff --git a/internal/mail/events_test.go b/internal/mail/events_test.go| index 083f33967..99e928b5d 100644 |
| --- a/internal/mail/events_test.go |
| +++ b/internal/mail/events_test.go |
| @@ -65,3 +65,73 @@ func TestDomainOfIsCaseFoldedAndAngleTolerant(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// Stray trailing > from unbalanced brackets should be stripped. |
| +func TestDomainOfStripsStrayTrailingBracket(t *testing.T) { |
| + for _, c := range []struct{ in, want string }{ |
| + {"a@b.com>", "b.com"}, |
| + {"<<a@b.com>>", "b.com"}, |
| + } { |
| + if got := DomainOf(c.in); got != c.want { |
| + t.Fatalf("DomainOf(%q) = %q, want %q", c.in, got, c.want) |
| + } |
| + } |
| +} |
| + |
| +// Trailing FQDN dots should be normalised away. |
| +func TestDomainOfStripsTrailingDot(t *testing.T) { |
| + if got := DomainOf("a@b.com."); got != "b.com" { |
| + t.Fatalf("DomainOf(\"a@b.com.\") = %q, want \"b.com\"", got) |
| + } |
| +} |
| + |
| +// Multi-address sources are ambiguous and must be refused. Attribution is by |
| +// domain, and multiple addresses would make it impossible to know which tenant |
| +// the event belongs to. |
| +func TestParseEventRefusesAmbiguousMultiAddressSources(t *testing.T) { |
| + testCases := []string{ |
| + `{"eventType":"Send","mail":{"source":"a@b.com,evil@attacker.com"}}`, |
| + `{"eventType":"Send","mail":{"source":"Legit <legit@good.com>, Evil <evil@bad.com>"}}`, |
| + `{"eventType":"Send","mail":{"source":"<a@good.com><b@evil.com>"}}`, |
| + } |
| + for _, tc := range testCases { |
| + if _, err := ParseEvent([]byte(tc)); err == nil { |
| + t.Fatalf("ParseEvent accepted ambiguous multi-address source: %s", tc) |
| + } |
| + } |
| +} |
| + |
| +// Control characters in the source indicate malformation or injection. |
| +func TestParseEventRefusesControlCharacters(t *testing.T) { |
| + testCases := []struct { |
| + name string |
| + body string |
| + }{ |
| + {"newline", `{"eventType":"Send","mail":{"source":"a@b.com\nX-Injected: true"}}`}, |
| + {"null", `{"eventType":"Send","mail":{"source":"a@b.com\x00injected"}}`}, |
| + } |
| + for _, tc := range testCases { |
| + if _, err := ParseEvent([]byte(tc.body)); err == nil { |
| + t.Fatalf("ParseEvent accepted control character in source (%s)", tc.name) |
| + } |
| + } |
| +} |
| + |
| +// A malformed timestamp must be silently ignored, not fail the parse. Timestamp |
| +// is unrelated to attribution, so failing loudly would block valid events. |
| +func TestParseEventIgnoresMalformedTimestamp(t *testing.T) { |
| + ev, err := ParseEvent([]byte(`{ |
| + "eventType":"Send", |
| + "mail":{ |
| + "timestamp":"not-a-time", |
| + "messageId":"msg123", |
| + "source":"a@b.com" |
| + } |
| + }`)) |
| + if err != nil { |
| + t.Fatalf("ParseEvent failed on malformed timestamp: %v", err) |
| + } |
| + if !ev.At.IsZero() { |
| + t.Fatalf("At = %v, want zero (timestamp ignored)", ev.At) |
| + } |
| +} |