amadan
mail: narrow ambiguity refusal to accept display-name-with-comma
The initial refusal of multi-address sources was too coarse: it rejected 'Smith, Bob <bob@example.com>', a standard display-name form, because the raw source contained a comma. Changed logic to extract the address first (stripping display-name wrapper), then test the extracted address for ambiguity. This rejects: - Multiple @ symbols in the extracted address - Commas in the extracted address (multi-address after display name is removed) - Multiple bracket pairs (detected by another < before the final one) But accepts: - Commas inside a display name (like 'Smith, Bob') - Single addresses with plus-sign tags (noreply+tag@example.com) Control-character check stays on the raw source: any injected control char anywhere in the field is rejected. Added test case TestParseEventAcceptsDisplayNameWithComma with four valid cases, including the standard 'Smith, Bob <bob@example.com>' form.
2 files changed,
+51
−8
internal/mail/events.go+25 −8internal/mail/events_test.go+26 −0
diff --git a/internal/mail/events.go b/internal/mail/events.go| index 1a3d7460b..959d5f4d9 100644 |
| --- a/internal/mail/events.go |
| +++ b/internal/mail/events.go |
| @@ -81,19 +81,36 @@ 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. |
| + // Refuse control characters on the raw source: an injected newline anywhere |
| + // in the field indicates malformation and must not be trusted. |
| 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) |
| } |
| } |
| + // Extract the address part (stripping display-name wrapper if present) and |
| + // test it for ambiguity. A single envelope MAIL FROM must extract to exactly |
| + // one @ and no commas. |
| + extractedAddr := raw.Mail.Source |
| + s := strings.TrimSpace(raw.Mail.Source) |
| + if i := strings.LastIndex(s, "<"); i >= 0 { |
| + if j := strings.LastIndex(s, ">"); j > i { |
| + // Has a bracketed span; refuse if there are multiple bracket pairs |
| + // (indicated by another < before the final one). |
| + if strings.Contains(s[:i], "<") { |
| + return Event{}, fmt.Errorf("mail: ses event source %q is ambiguous — multiple bracketed address spans", raw.Mail.Source) |
| + } |
| + extractedAddr = strings.TrimSpace(s[i+1 : j]) |
| + } |
| + } |
| + // Check the extracted address for ambiguity: commas or multiple @ symbols. |
| + // Note: "a@b"@example.com (a quoted local part containing @) is refused here. |
| + // This is deliberate: SES mail.source is a bare envelope MAIL FROM with no |
| + // display name, and quoted local parts with @ essentially never appear there. |
| + // Quote-aware parsing is not worth the machinery for a case that does not occur. |
| + if strings.Contains(extractedAddr, ",") || strings.Count(extractedAddr, "@") > 1 { |
| + return Event{}, fmt.Errorf("mail: ses event source %q is ambiguous — cannot reliably attribute to one tenant", raw.Mail.Source) |
| + } |
| 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 99e928b5d..6b645ea3c 100644 |
| --- a/internal/mail/events_test.go |
| +++ b/internal/mail/events_test.go |
| @@ -90,8 +90,11 @@ func TestDomainOfStripsTrailingDot(t *testing.T) { |
| // the event belongs to. |
| func TestParseEventRefusesAmbiguousMultiAddressSources(t *testing.T) { |
| testCases := []string{ |
| + // Comma inside raw source (not in display name) indicates multiple addresses |
| `{"eventType":"Send","mail":{"source":"a@b.com,evil@attacker.com"}}`, |
| + // Multiple bracket pairs separated by comma outside brackets |
| `{"eventType":"Send","mail":{"source":"Legit <legit@good.com>, Evil <evil@bad.com>"}}`, |
| + // Multiple bracket pairs with no space between them |
| `{"eventType":"Send","mail":{"source":"<a@good.com><b@evil.com>"}}`, |
| } |
| for _, tc := range testCases { |
| @@ -101,6 +104,29 @@ func TestParseEventRefusesAmbiguousMultiAddressSources(t *testing.T) { |
| } |
| } |
| +// Display-name-with-comma is standard and must be accepted. A comma inside |
| +// a display name (like "Smith, Bob") does not indicate multiple addresses. |
| +func TestParseEventAcceptsDisplayNameWithComma(t *testing.T) { |
| + testCases := []struct { |
| + source string |
| + want string |
| + }{ |
| + {`Smith, Bob <bob@example.com>`, `example.com`}, |
| + {`Bob <bob@example.com>`, `example.com`}, |
| + {`noreply+tag@example.com`, `example.com`}, |
| + {`noreply@mail-eu-west-1.seapointish.bag.oncarlos.com`, `mail-eu-west-1.seapointish.bag.oncarlos.com`}, |
| + } |
| + for _, tc := range testCases { |
| + ev, err := ParseEvent([]byte(`{"eventType":"Send","mail":{"source":"` + tc.source + `"}}`)) |
| + if err != nil { |
| + t.Fatalf("ParseEvent rejected valid address %q: %v", tc.source, err) |
| + } |
| + if ev.Domain != tc.want { |
| + t.Fatalf("Domain for %q = %q, want %q", tc.source, ev.Domain, tc.want) |
| + } |
| + } |
| +} |
| + |
| // Control characters in the source indicate malformation or injection. |
| func TestParseEventRefusesControlCharacters(t *testing.T) { |
| testCases := []struct { |