From 22ed97f562106fc5b97899fb90b0f48c7ebeb18a Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sat, 27 Jun 2026 17:36:51 +0100 Subject: [PATCH] fix(email): eliminate data race on provider username resolution sendViaProviderWithAddresses mutated the shared e.config.Username for provider-specific defaults (SendGrid, Postmark, SparkPost, Resend). If concurrent goroutines sent email simultaneously, this was a data race on the config struct. Move the resolution into negotiateAuth via a local variable (resolveProviderUsername helper) so the shared config is never mutated. --- internal/notifications/email_enhanced.go | 56 ++++++++----------- internal/notifications/email_enhanced_test.go | 26 ++++----- 2 files changed, 37 insertions(+), 45 deletions(-) diff --git a/internal/notifications/email_enhanced.go b/internal/notifications/email_enhanced.go index 21783a334..30f0a5b27 100644 --- a/internal/notifications/email_enhanced.go +++ b/internal/notifications/email_enhanced.go @@ -237,32 +237,45 @@ func buildMultipartEmailMessage(addresses resolvedEmailAddresses, subject, htmlB // and returns the best smtp.Auth to use. Prefers PLAIN, falls back to LOGIN. // Returns nil if auth is not configured. func (e *EnhancedEmailManager) negotiateAuth(client *smtp.Client) (smtp.Auth, error) { - if !e.config.AuthRequired || e.config.Username == "" || e.config.Password == "" { + username := resolveProviderUsername(e.config) + if !e.config.AuthRequired || username == "" || e.config.Password == "" { return nil, nil } - // Check what the server advertises after EHLO. - // Extension returns (ok bool, params string). hasAuth, mechanismsRaw := client.Extension("AUTH") mechanisms := strings.ToUpper(mechanismsRaw) if strings.Contains(mechanisms, "PLAIN") { - return &plainAuth{"", e.config.Username, e.config.Password}, nil + return &plainAuth{"", username, e.config.Password}, nil } if strings.Contains(mechanisms, "LOGIN") { - return LoginAuth(e.config.Username, e.config.Password), nil + return LoginAuth(username, e.config.Password), nil } - // Server didn't advertise AUTH at all — try PLAIN as a default since - // it's the most widely supported. This handles servers that don't - // properly advertise their capabilities. if !hasAuth || mechanisms == "" { - return &plainAuth{"", e.config.Username, e.config.Password}, nil + return &plainAuth{"", username, e.config.Password}, nil } return nil, fmt.Errorf("server advertises AUTH mechanisms [%s] but none are supported (PLAIN, LOGIN)", mechanisms) } +func resolveProviderUsername(config EmailProviderConfig) string { + if config.Username != "" { + return config.Username + } + switch config.Provider { + case "SendGrid": + return "apikey" + case "Postmark": + return config.Password + case "SparkPost": + return "SMTP_Injection" + case "Resend": + return "resend" + } + return "" +} + // EnhancedEmailManager extends email functionality with provider support type EnhancedEmailManager struct { config EmailProviderConfig @@ -391,29 +404,8 @@ func (e *EnhancedEmailManager) sendViaProvider(msg []byte) error { func (e *EnhancedEmailManager) sendViaProviderWithAddresses(msg []byte, addresses resolvedEmailAddresses) error { addr := net.JoinHostPort(e.config.SMTPHost, strconv.Itoa(e.config.SMTPPort)) - // Special handling for specific providers - switch e.config.Provider { - case "SendGrid": - // SendGrid uses "apikey" as username - if e.config.Username == "" { - e.config.Username = "apikey" - } - case "Postmark": - // Postmark uses API token for both username and password - if e.config.Password != "" && e.config.Username == "" { - e.config.Username = e.config.Password - } - case "SparkPost": - // SparkPost uses specific username - if e.config.Username == "" { - e.config.Username = "SMTP_Injection" - } - case "Resend": - // Resend uses "resend" as username - if e.config.Username == "" { - e.config.Username = "resend" - } - } + // Provider-specific usernames are resolved in negotiateAuth without + // mutating the shared config struct. // Send with TLS configuration — auth is negotiated after connection if e.config.TLS || e.config.SMTPPort == 465 { diff --git a/internal/notifications/email_enhanced_test.go b/internal/notifications/email_enhanced_test.go index 906c19cbb..02c14bf3f 100644 --- a/internal/notifications/email_enhanced_test.go +++ b/internal/notifications/email_enhanced_test.go @@ -138,7 +138,7 @@ func TestSendViaProvider_ProviderUsernameDefaults(t *testing.T) { t.Run(tt.name, func(t *testing.T) { config := EmailProviderConfig{ EmailConfig: EmailConfig{ - SMTPHost: "invalid.host.test", // Will fail to connect + SMTPHost: "invalid.host.test", SMTPPort: 587, Username: tt.initialUsername, Password: "test", @@ -149,12 +149,12 @@ func TestSendViaProvider_ProviderUsernameDefaults(t *testing.T) { AuthRequired: true, } - manager := NewEnhancedEmailManager(config) - // Call sendViaProvider - it will fail on connection, but will have set username - _ = manager.sendViaProvider([]byte("test")) - - if manager.config.Username != tt.expectedUsername { - t.Errorf("expected username %q, got %q", tt.expectedUsername, manager.config.Username) + got := resolveProviderUsername(config) + if got != tt.expectedUsername { + t.Errorf("expected username %q, got %q", tt.expectedUsername, got) + } + if config.Username != tt.initialUsername { + t.Errorf("config was mutated: expected %q, got %q", tt.initialUsername, config.Username) } }) } @@ -174,12 +174,12 @@ func TestSendViaProvider_PostmarkUsernameFromPassword(t *testing.T) { AuthRequired: true, } - manager := NewEnhancedEmailManager(config) - _ = manager.sendViaProvider([]byte("test")) - - // Postmark copies password to username when username is empty - if manager.config.Username != "postmark-api-token" { - t.Errorf("expected username to be set from password, got %q", manager.config.Username) + got := resolveProviderUsername(config) + if got != "postmark-api-token" { + t.Errorf("expected username to be set from password, got %q", got) + } + if config.Username != "" { + t.Errorf("config was mutated: expected empty, got %q", config.Username) } }