sec(email): stop broadcasting approval tokens via SNS (closes #1015)

internal/email/coverage_extra_test.go

@@ -11,32 +11,43 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// Tests for containsColon
-func TestContainsColon(t *testing.T) {
-	assert.True(t, containsColon("arn:aws:secretsmanager:us-east-1:123:secret:foo"))
-	assert.True(t, containsColon("a:b"))
-	assert.True(t, containsColon(":"))
-	assert.False(t, containsColon(""))
-	assert.False(t, containsColon("nodivider"))
-	assert.False(t, containsColon("just/a/path"))
+// Tests for isSecretManagerReference (replaces the deleted containsColon helper -- 07-L2/L3)
+func TestIsSecretManagerReference(t *testing.T) {
+	// AWS ARN
+	assert.True(t, isSecretManagerReference("arn:aws:secretsmanager:us-east-1:123:secret:foo"))
+	// GCP resource path
+	assert.True(t, isSecretManagerReference("projects/my-project/secrets/my-secret/versions/latest"))
+	// Azure Key Vault URL
+	assert.True(t, isSecretManagerReference("https://myvault.vault.azure.net/secrets/mysecret"))
+	// Generic slash path
+	assert.True(t, isSecretManagerReference("/my/secret/path"))
+	// Plain API key -- not a reference
+	assert.False(t, isSecretManagerReference("SG.abcdefghijklmnopqrstuvwx"))
+	// Empty string
+	assert.False(t, isSecretManagerReference(""))
+	// Value with colon but not an ARN/Vault URL
+	assert.False(t, isSecretManagerReference("user:password"))
 }
 
 // Tests for warnIfPlaintext – exercising all branches
 func TestWarnIfPlaintext(t *testing.T) {
 	// Empty value: should be a no-op (no panic)
 	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "") })
 
-	// Short value (< 20 chars, no colon, no leading slash) → warning branch
-	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "short") })
+	// Plaintext API key (not a secret-manager reference) -- warning branch
+	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "SG.averylongplaintextpassword") })
 
-	// Long value with colon → secret-manager reference, no warning
+	// AWS ARN -- treated as secret-manager reference, no warning
 	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "arn:aws:secretsmanager:us-east-1:123456789012:secret:mykey") })
 
-	// Long value starting with slash → secret-manager reference, no warning
-	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "/my/secret/path/that/is/long") })
+	// GCP resource path -- treated as secret-manager reference, no warning
+	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "projects/my-project/secrets/my-secret") })
+
+	// Azure Key Vault URL -- treated as secret-manager reference, no warning
+	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "https://myvault.vault.azure.net/secrets/foo") })
 
-	// Long value without colon and not starting with slash → warning branch
-	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "averylongplaintextpasswordwithnospecialchars") })
+	// Generic leading slash path -- treated as secret-manager reference, no warning
+	assert.NotPanics(t, func() { warnIfPlaintext("VAR", "/my/secret/path/that/is/long") })
 }
 
 // Tests for renderTemplate error path
@@ -408,25 +419,29 @@ func TestSMTPSender_SendRIExchangePendingApproval_WithNotifyEmail(t *testing.T)
 // Tests for Sender.SendRIExchangePendingApproval, SendRIExchangeCompleted, SendPurchaseApprovalRequest
 // using the mock SNS sender (no SNS topic → no-op path)
 
-func TestSender_SendRIExchangePendingApproval_NoTopic(t *testing.T) {
+func TestSender_SendRIExchangePendingApproval_NoRecipient(t *testing.T) {
+	// When RecipientEmail is empty the send must be rejected — the body carries
+	// per-exchange approval tokens that must not be broadcast via SNS.
 	mockSNS := &mockSNSPublisher{}
 	mockSES := &mockSESEmailSender{}
 	sender := NewSenderWithClients(mockSNS, mockSES, SenderConfig{
-		// TopicARN is empty → SendNotification is a no-op
 		FromEmail: "from@example.com",
+		TopicARN:  "arn:aws:sns:us-east-1:123:topic",
 	})
 
 	data := RIExchangeNotificationData{
 		DashboardURL: "https://dashboard.example.com",
 		TotalPayment: "300.00",
 		Exchanges: []RIExchangeItem{
 			{RecordID: "r4", SourceRIID: "ri-4", SourceInstanceType: "m4.large",
-				TargetInstanceType: "m4.xlarge", TargetCount: 1, PaymentDue: "300.00", UtilizationPct: 55.0},
+				TargetInstanceType: "m4.xlarge", TargetCount: 1, PaymentDue: "300.00",
+				ApprovalToken: "secret-token", UtilizationPct: 55.0},
 		},
+		// RecipientEmail intentionally empty
 	}
 
 	err := sender.SendRIExchangePendingApproval(context.Background(), data)
-	require.NoError(t, err)
+	require.ErrorIs(t, err, ErrNoRecipient)
 }
 
 func TestSender_SendRIExchangeCompleted_NoTopic(t *testing.T) {

internal/email/coverage_test.go

@@ -632,7 +632,9 @@ func TestSMTPSender_SendToEmail_ConnectionFails(t *testing.T) {
 	assert.Contains(t, err.Error(), "failed to send email via SMTP")
 }
 
-// TestSMTPSender_SendToEmail_ConnectionFails_NoTLS tests with useTLS=false
+// TestSMTPSender_SendToEmail_ConnectionFails_NoTLS tests with useTLS=false and
+// credentials configured. Since 07-H2 the cleartext-auth guard fires before
+// any network dial, so the error message differs from a connection failure.
 func TestSMTPSender_SendToEmail_ConnectionFails_NoTLS(t *testing.T) {
 	sender := &SMTPSender{
 		host:      "localhost",
@@ -642,14 +644,15 @@ func TestSMTPSender_SendToEmail_ConnectionFails_NoTLS(t *testing.T) {
 		fromEmail: "sender@example.com",
 		fromName:  "",
 		useTLS:    false,
+		// allowInsecure intentionally false: the cleartext-auth guard must fire.
 	}
 
 	ctx := context.Background()
 	err := sender.SendToEmail(ctx, "recipient@example.com", "Test Subject", "Test Body")
 
-	// This should fail because there's no SMTP server on that port
+	// 07-H2: guard fires before any network dial.
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "failed to send email via SMTP")
+	assert.Contains(t, err.Error(), "SMTP auth over non-TLS connection is refused")
 }
 
 // TestSMTPSender_SendToEmail_NoAuth tests without authentication
@@ -1305,13 +1308,15 @@ func TestSender_SendMethods_ErrorPaths(t *testing.T) {
 		assert.Contains(t, err.Error(), "failed to publish to SNS")
 	})
 
-	t.Run("ScheduledPurchase_propagates_error", func(t *testing.T) {
+	t.Run("ScheduledPurchase_no_recipient", func(t *testing.T) {
+		// RecipientEmail is empty: must return ErrNoRecipient, not broadcast via SNS.
 		err := sender.SendScheduledPurchaseNotification(ctx, NotificationData{
-			DashboardURL: "https://test.com",
-			PlanName:     "Test",
+			DashboardURL:  "https://test.com",
+			PlanName:      "Test",
+			ApprovalToken: "tok",
+			// RecipientEmail intentionally empty
 		})
-		require.Error(t, err)
-		assert.Contains(t, err.Error(), "failed to publish to SNS")
+		require.ErrorIs(t, err, ErrNoRecipient)
 	})
 
 	t.Run("PurchaseConfirmation_propagates_error", func(t *testing.T) {

internal/email/factory.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"strconv"
+	"strings"
 
 	"github.com/LeanerCloud/CUDly/internal/secrets"
 	"github.com/LeanerCloud/CUDly/pkg/logging"
@@ -86,6 +87,22 @@ func NewSenderFromEnvironment(ctx context.Context) (SenderInterface, error) {
 	}
 }
 
+// isSecretManagerReference reports whether value looks like a secret manager
+// reference rather than a plaintext credential (07-L3). Recognised patterns:
+//   - AWS ARN:       starts with "arn:"
+//   - GCP resource:  starts with "projects/"
+//   - Azure Key Vault secret URL: contains ".vault.azure.net/"
+//   - Generic path:  starts with "/" (local-file or Vault path)
+//
+// A value that does not match any pattern is treated as a potential plaintext
+// credential and triggers a warning via warnIfPlaintext.
+func isSecretManagerReference(value string) bool {
+	return strings.HasPrefix(value, "arn:") ||
+		strings.HasPrefix(value, "projects/") ||
+		strings.Contains(value, ".vault.azure.net/") ||
+		strings.HasPrefix(value, "/")
+}
+
 // warnIfPlaintext logs a warning when a credential value is set as a plaintext
 // environment variable rather than a secret manager reference (ARN/resource name).
 // TODO: migrate all email credentials to AWS Secrets Manager / GCP Secret Manager /
@@ -94,20 +111,9 @@ func warnIfPlaintext(envVar, value string) {
 	if value == "" {
 		return
 	}
-	// Heuristic: secret manager references contain ":" or "/" (ARN, resource path, secret name)
-	if len(value) < 20 || (value[0] != '/' && !containsColon(value)) {
-		logging.Warnf("security: %s is set as a plaintext env var; consider using a Secrets Manager reference instead", envVar)
-	}
-}
-
-// containsColon returns true if s contains a colon character.
-func containsColon(s string) bool {
-	for _, c := range s {
-		if c == ':' {
-			return true
-		}
+	if !isSecretManagerReference(value) {
+		logging.Warnf("security: %s is set as a plaintext env var; consider using a Secrets Manager reference (ARN, projects/..., vault URL, or /path) instead", envVar)
 	}
-	return false
 }
 
 // newGCPSenderFromEnv creates a SendGrid-based email sender from environment variables

internal/email/sender.go

@@ -27,6 +27,12 @@ var (
 	// out. Distinct from ErrNoRecipient so the caller can report which side
 	// of the wire is unconfigured.
 	ErrNoFromEmail = errors.New("email: no from address")
+
+	// ErrTokenInBroadcast is returned by SendNotification when the message
+	// body contains a "token=" query parameter. Broadcasting a body with an
+	// approval token leaks it to every SNS subscriber. Use SendToEmailWithCC
+	// (targeted SES) for any message that carries an approval URL.
+	ErrTokenInBroadcast = errors.New("email: message body contains an approval token; use targeted SES send, not SNS broadcast")
 )
 
 // SenderConfig holds configuration for the email sender
@@ -100,8 +106,26 @@ func NewSenderWithClients(snsClient SNSPublisher, sesClient SESEmailSender, cfg
 	}
 }
 
-// SendNotification sends a notification email via SNS
+// snsMaxSubjectLen is the maximum byte length SNS accepts for a Subject.
+// Subjects longer than 100 bytes are rejected with InvalidParameter at runtime.
+const snsMaxSubjectLen = 100
+
+// SendNotification sends a notification email via SNS.
+//
+// Guard: messages whose body contains "token=" (case-insensitive) are rejected
+// with ErrTokenInBroadcast. Approval tokens must never reach the SNS broadcast
+// topic because every subscriber would receive a working action link. Use
+// SendToEmailWithCC for any message that carries an approval URL.
+//
+// Subject sanitization (07-M3): the subject is passed through sanitizeHeader
+// (strips CR/LF to prevent SNS parameter injection) and truncated to 100 bytes
+// (SNS limit). Subjects built from user-controlled fields such as PlanName can
+// otherwise cause an InvalidParameter error at publish time.
 func (s *Sender) SendNotification(ctx context.Context, subject, message string) error {
+	if strings.Contains(strings.ToLower(message), "token=") {
+		return ErrTokenInBroadcast
+	}
+
 	if s.topicARN == "" {
 		logging.Debug("No SNS topic configured, skipping email notification")
 		return nil
@@ -111,16 +135,22 @@ func (s *Sender) SendNotification(ctx context.Context, subject, message string)
 		return fmt.Errorf("SNS client not initialized")
 	}
 
+	// Sanitize and truncate the SNS Subject (07-M3).
+	safeSubject := sanitizeHeader(subject)
+	if len(safeSubject) > snsMaxSubjectLen {
+		safeSubject = safeSubject[:snsMaxSubjectLen]
+	}
+
 	_, err := s.snsClient.Publish(ctx, &sns.PublishInput{
 		TopicArn: aws.String(s.topicARN),
-		Subject:  aws.String(subject),
+		Subject:  aws.String(safeSubject),
 		Message:  aws.String(message),
 	})
 	if err != nil {
 		return fmt.Errorf("failed to publish to SNS: %w", err)
 	}
 
-	logging.Debugf("Sent notification: %s", subject)
+	logging.Debugf("Sent notification: %s", safeSubject)
 	return nil
 }
 
@@ -283,15 +313,26 @@ func (s *Sender) ensureSandboxRecipientVerified(ctx context.Context, toEmail str
 	logging.Warnf("Recipient email %s is not verified in sandbox mode - creating verification request", redactEmail(toEmail))
 	if err := s.createVerificationRequest(ctx, toEmail); err != nil {
 		logging.Warnf("Failed to create verification request: %v", err)
+		// Use the real address in the user-facing message so the recipient
+		// knows which inbox to check; log only the redacted form (07-M2).
 		return fmt.Errorf("recipient email %s is not verified in SES sandbox mode. A verification email has been sent - please check inbox and click the verification link before trying again", toEmail)
 	}
-	return fmt.Errorf("recipient email %s is not verified in SES sandbox mode. A verification email has been sent to %s - please check inbox and click the verification link, then try the password reset again", toEmail, toEmail)
+	// The user-facing message intentionally includes the full address so the
+	// recipient can verify the correct inbox. Wrapped errors/logs use the
+	// redacted form per the package's PII stance (07-M2).
+	return fmt.Errorf("recipient email %s is not verified in SES sandbox mode. A verification email has been sent - please check inbox and click the verification link, then try again", toEmail)
 }
 
 // buildSESSendEmailInputMultipart constructs a sesv2.SendEmailInput with
 // both a plain-text and an HTML alternative body. SES handles the
 // multipart/alternative MIME assembly server-side when both Text and Html
 // fields are populated on types.Body.
+//
+// Header injection note (07-M1): SES v2 SendEmail accepts structured fields
+// (Subject.Data, Body.Text.Data, Body.Html.Data) and builds the MIME envelope
+// server-side. CR/LF injection via Subject.Data is not possible through the
+// structured API. Any future raw-MIME path MUST sanitize the subject with
+// sanitizeHeader before composing the header string, matching the SMTP path.
 func buildSESSendEmailInputMultipart(fromEmail, toEmail string, cc []string, subject, textBody, htmlBody string) *sesv2.SendEmailInput {
 	destination := &types.Destination{
 		ToAddresses: []string{toEmail},
@@ -325,6 +366,7 @@ func buildSESSendEmailInputMultipart(fromEmail, toEmail string, cc []string, sub
 
 // buildSESSendEmailInput constructs a sesv2.SendEmailInput with the
 // destination To + (optional) Cc list and a plain-text body.
+// See buildSESSendEmailInputMultipart for the header-injection safety note (07-M1).
 func buildSESSendEmailInput(fromEmail, toEmail string, cc []string, subject, body string) *sesv2.SendEmailInput {
 	destination := &types.Destination{
 		ToAddresses: []string{toEmail},
@@ -464,6 +506,15 @@ type RIExchangeNotificationData struct {
 	Exchanges    []RIExchangeItem
 	Skipped      []SkippedExchange
 	TotalPayment string
+	// RecipientEmail is the primary (To) inbox for the approval-required flow.
+	// Must be set when Exchanges contain ApprovalToken values; leave empty
+	// only for completion/broadcast notifications that carry no tokens.
+	// When non-empty, SendRIExchangePendingApproval routes through targeted
+	// SES (not the SNS broadcast topic). Mirrors NotificationData.RecipientEmail.
+	RecipientEmail string
+	// CCEmails carries additional recipients informed of the pending exchanges
+	// but not the authorised approvers. Deduplicated against RecipientEmail.
+	CCEmails []string
 }
 
 // RIExchangeItem represents a single exchange in an email notification