fix(email): plain-text renderers, address redaction, per-scope mute failure and SMTP no-recipient parity

go.mod

@@ -59,7 +59,7 @@ require (
 	go.opentelemetry.io/otel/metric v1.45.0 // indirect
 	go.opentelemetry.io/otel/trace v1.45.0 // indirect
 	golang.org/x/crypto v0.58.0
-	golang.org/x/net v0.61.0 // indirect
+	golang.org/x/net v0.62.0 // indirect
 	golang.org/x/oauth2 v0.37.0
 	golang.org/x/sync v0.24.0
 	golang.org/x/sys v0.49.0 // indirect

go.sum

@@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54=
 golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w=
 golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg=
 golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs=
-golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw=
-golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
+golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY=
+golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI=
 golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98=
 golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58=
 golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA=

internal/email/coverage_extra_test.go

@@ -393,16 +393,16 @@ func TestSMTPSender_SendPurchaseApprovalRequest_NoRecipient(t *testing.T) {
 	require.ErrorIs(t, err, ErrNoRecipient)
 }
 
-// Tests for SMTPSender using notifyEmail (not fromEmail).
-func TestSMTPSender_SendRIExchangePendingApproval_WithNotifyEmail(t *testing.T) {
+// The static SMTP notify address is not a fallback recipient for the RI
+// exchange approval email: its body carries live approval tokens, so an empty
+// data.RecipientEmail is ErrNoRecipient even when notifyEmail is configured,
+// matching the SES sender (issue #111). No connection is attempted.
+func TestSMTPSender_SendRIExchangePendingApproval_NotifyEmailIsNotAFallback(t *testing.T) {
 	sender := &SMTPSender{
-		host:        "smtp.example.com",
-		port:        587,
+		host:        "127.0.0.1",
+		port:        1, // never dialed
 		fromEmail:   "from@example.com",
 		notifyEmail: "notify@example.com",
-		// useTLS is false so STARTTLS is not required; a plain send will be attempted
-		// but fail — we just verify the method reaches the send path (not a no-op)
-		useTLS: false,
 	}
 
 	data := RIExchangeNotificationData{
@@ -414,12 +414,8 @@ func TestSMTPSender_SendRIExchangePendingApproval_WithNotifyEmail(t *testing.T)
 		},
 	}
 
-	// Will fail with a network error — just verify it's not the "no from email" no-op.
 	err := sender.SendRIExchangePendingApproval(context.Background(), data)
-	// The error will be a connection refused / network error (not nil, not "no from email")
-	if err != nil {
-		assert.NotContains(t, err.Error(), "no from email")
-	}
+	require.ErrorIs(t, err, ErrNoRecipient)
 }
 
 // Tests for Sender.SendRIExchangePendingApproval, SendRIExchangeCompleted, SendPurchaseApprovalRequest

internal/email/executed_notification_test.go

@@ -42,13 +42,17 @@ func executedNotificationData() NotificationData {
 	}
 }
 
-// escapedToken is the exact rendered form of the test token after
+// escapedToken is the exact rendered form of the test token in the HTML half:
 // url.QueryEscape (the {{urlquery}} func) followed by html/template's
 // contextual auto-escaping: space -> '+' -> '+', '/' -> %2F, '+' -> %2B,
-// '=' -> %3D. Both the text and HTML halves use html/template, so both render
-// the token identically.
+// '=' -> %3D. A mail client decodes the + entity back to '+'.
 const escapedToken = "tok+en%2Fwith%2Bspecial%3Dchars"
 
+// textToken is the rendered form of the test token in the plain-text half,
+// which uses text/template: url.QueryEscape only, no HTML entities, so the
+// link a recipient copies is the working one (issue #111).
+const textToken = "tok+en%2Fwith%2Bspecial%3Dchars"
+
 // rawTokenFragment is a substring of the un-escaped token that MUST NOT appear
 // verbatim in a correctly-escaped render (it contains a raw '/').
 const rawTokenFragment = "tok en/with+special=chars"
@@ -66,7 +70,8 @@ func TestRenderPurchaseExecutedNotificationEmail_RevokeURLPresent(t *testing.T)
 	assert.Contains(t, body, "%2F", "'/' must be percent-encoded")
 	assert.Contains(t, body, "%2B", "'+' must be percent-encoded")
 	assert.Contains(t, body, "%3D", "'=' must be percent-encoded")
-	assert.Contains(t, body, escapedToken)
+	assert.Contains(t, body, "?token="+textToken+"\n", "plain-text revoke link must be the working URL, without HTML entities")
+	assert.NotContains(t, body, "+")
 	// ...and the raw token never leaks verbatim into the URL.
 	assert.NotContains(t, body, rawTokenFragment)
 
@@ -165,7 +170,7 @@ func TestRenderPurchaseExecutedNotification_Partial(t *testing.T) {
 	require.NoError(t, err)
 	assert.True(t, strings.HasPrefix(text, "[CUDly] Purchase partially executed"))
 	assert.Contains(t, text, "Not every commitment in this purchase went through.")
-	assert.Contains(t, text, escapedToken, "the partial run stays revocable")
+	assert.Contains(t, text, textToken, "the partial run stays revocable")
 
 	html, err := RenderPurchaseExecutedNotificationEmailHTML(data)
 	require.NoError(t, err)

internal/email/log_redaction_test.go

@@ -0,0 +1,80 @@
+package email
+
+import (
+	"bytes"
+	"context"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/logging"
+	"github.com/aws/aws-sdk-go-v2/service/sesv2"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// captureDebugLogs routes the default logger to a buffer at Debug level and
+// restores the previous output and level when the test ends.
+func captureDebugLogs(t *testing.T) *bytes.Buffer {
+	t.Helper()
+	var logs bytes.Buffer
+	prevOut, prevLevel := logging.SetOutput(&logs), logging.GetLevel()
+	t.Cleanup(func() {
+		logging.SetOutput(prevOut)
+		logging.SetLevelValue(prevLevel)
+	})
+	logging.SetLevelValue(logging.LevelDebug)
+	return &logs
+}
+
+// TestCreateVerificationRequest_RedactsAddressInLog covers issue #111: the SES
+// sandbox verification path logged the full recipient address at Info.
+func TestCreateVerificationRequest_RedactsAddressInLog(t *testing.T) {
+	logs := captureDebugLogs(t)
+	ses := new(MockSESClient)
+	ses.On("CreateEmailIdentity", mock.Anything, mock.AnythingOfType("*sesv2.CreateEmailIdentityInput")).
+		Return(&sesv2.CreateEmailIdentityOutput{}, nil)
+	s := &Sender{sesClient: ses, fromEmail: "noreply@example.com"}
+
+	require.NoError(t, s.createVerificationRequest(context.Background(), "customer.person@example.com"))
+
+	out := logs.String()
+	assert.Contains(t, out, "Created email verification request for cu***@example.com")
+	assert.NotContains(t, out, "customer.person@example.com")
+}
+
+// TestSMTPSender_SendLogsRedactRecipient covers issue #111 (A06-012): the SMTP
+// single-part and multipart send paths logged the raw To address at Debug,
+// with and without CC recipients.
+func TestSMTPSender_SendLogsRedactRecipient(t *testing.T) {
+	const to = "customer.person@example.com"
+	cases := []struct {
+		name string
+		send func(s *SMTPSender, cc []string) error
+	}{
+		{name: "text", send: func(s *SMTPSender, cc []string) error {
+			return s.SendToEmailWithCC(context.Background(), to, cc, "Subject", "body")
+		}},
+		{name: "multipart", send: func(s *SMTPSender, cc []string) error {
+			return s.SendToEmailWithCCMultipart(context.Background(), to, cc, "Subject", "body", "<p>body</p>")
+		}},
+	}
+	for _, tc := range cases {
+		for _, cc := range [][]string{nil, {"other.person@example.com"}} {
+			t.Run(tc.name, func(t *testing.T) {
+				logs := captureDebugLogs(t)
+				server := newMockSMTPServer(t, false)
+				server.start(t)
+				s := &SMTPSender{host: "127.0.0.1", port: server.port, fromEmail: "notifications@example.com"}
+
+				err := tc.send(s, cc)
+				server.stop()
+				require.NoError(t, err)
+
+				out := logs.String()
+				assert.Contains(t, out, "via SMTP to cu***@example.com", "send must be logged with the redacted address")
+				assert.NotContains(t, out, to)
+				assert.NotContains(t, out, "other.person@example.com")
+			})
+		}
+	}
+}

internal/email/mute.go

@@ -14,24 +14,43 @@ import (
 // List-Unsubscribe headers; keeping the logic here (instead of duplicating it
 // per transport) prevents the two paths from diverging.
 
-// isRecipientMuted returns true when (email, scope) is muted. A nil checker or a
-// store error is treated as "not muted" (fail-open) so a transient DB outage
-// does not silently block approval emails.
+// isRecipientMuted returns true when (email, scope) is muted. A nil checker
+// means muting is not configured, so nothing is muted.
+//
+// When the mute store errors, the answer depends on the scope (see
+// muteCheckFailsOpen): approval scopes fail open (send), so a transient DB
+// outage does not strand a purchase or exchange waiting on its approval email;
+// every other scope is informational and fails closed (don't send), so the
+// outage does not re-mail someone who used the one-click unsubscribe link.
 func isRecipientMuted(ctx context.Context, mc MuteChecker, email, scope string) bool {
 	if mc == nil {
 		return false
 	}
 	muted, err := mc.IsNotificationMuted(ctx, email, scope)
 	if err != nil {
-		logging.Warnf("email: mute check failed for scope=%s: %v", scope, err)
-		return false
+		failOpen := muteCheckFailsOpen(scope)
+		logging.Warnf("email: mute check failed for scope=%s (sending anyway: %t): %v", scope, failOpen, err)
+		return !failOpen
 	}
 	return muted
 }
 
+// muteCheckFailsOpen reports whether a mute-store error should let an email
+// through for scope. Only the approval scopes do: their emails carry the links
+// a pending purchase or RI exchange is waiting on. Any other scope, including
+// one added later, is treated as informational and fails closed.
+func muteCheckFailsOpen(scope string) bool {
+	switch common.MuteNotifScope(scope) {
+	case common.ScopePurchaseApprovals, common.ScopeRIExchangeApprovals:
+		return true
+	default:
+		return false
+	}
+}
+
 // filterMutedRecipients returns a copy of addrs with any address muted for scope
-// removed. The original slice is not modified. Errors from the mute store are
-// treated as "not muted" (fail-open).
+// removed. The original slice is not modified. A mute-store error follows the
+// per-scope direction of isRecipientMuted.
 func filterMutedRecipients(ctx context.Context, mc MuteChecker, addrs []string, scope string) []string {
 	if mc == nil || len(addrs) == 0 {
 		return addrs

internal/email/mute_test.go

@@ -316,3 +316,58 @@ func TestVerifyMuteToken_Forged(t *testing.T) {
 	key := []byte("test-secret")
 	assert.False(t, common.VerifyMuteToken(key, "user@example.com", "purchase_approvals", "forgedtoken"))
 }
+
+// TestIsRecipientMuted_StoreErrorDirectionPerScope pins the per-scope failure
+// direction of a mute-store error (issue #111). The approval scopes fail open
+// (send), because dropping an approval email strands a purchase or exchange.
+// Every other scope is informational and fails closed (don't send), so a DB
+// blip cannot re-mail someone who used the one-click unsubscribe link.
+func TestIsRecipientMuted_StoreErrorDirectionPerScope(t *testing.T) {
+	const addr = "ops@example.com"
+	cases := []struct {
+		scope     string
+		wantMuted bool
+	}{
+		{scope: string(common.ScopePurchaseApprovals), wantMuted: false},
+		{scope: string(common.ScopeRIExchangeApprovals), wantMuted: false},
+		// No informational scope is defined yet; any scope outside the
+		// approval set is treated as informational.
+		{scope: "purchase_executed", wantMuted: true},
+		{scope: "new_recommendations", wantMuted: true},
+		{scope: "", wantMuted: true},
+	}
+	for _, tc := range cases {
+		t.Run("scope="+tc.scope, func(t *testing.T) {
+			mc := new(mockMuteChecker)
+			mc.On("IsNotificationMuted", mock.Anything, addr, tc.scope).
+				Return(false, errors.New("db error"))
+
+			assert.Equal(t, tc.wantMuted, isRecipientMuted(context.Background(), mc, addr, tc.scope))
+
+			// The CC filter and the primary-recipient check follow the same direction.
+			kept := filterMutedRecipients(context.Background(), mc, []string{addr}, tc.scope)
+			if tc.wantMuted {
+				assert.Empty(t, kept, "informational scope must drop a CC whose mute state is unknown")
+			} else {
+				assert.Equal(t, []string{addr}, kept, "approval scope must keep a CC whose mute state is unknown")
+			}
+			_, _, _, muted := prepareMuteAwareDelivery(context.Background(), mc, "", addr, nil, tc.scope)
+			assert.Equal(t, tc.wantMuted, muted)
+			mc.AssertExpectations(t)
+		})
+	}
+}
+
+// TestIsRecipientMuted_NoErrorFollowsStore checks that the per-scope error
+// policy does not change the answer when the store responds normally.
+func TestIsRecipientMuted_NoErrorFollowsStore(t *testing.T) {
+	for _, scope := range []string{string(common.ScopePurchaseApprovals), "new_recommendations"} {
+		for _, stored := range []bool{true, false} {
+			mc := new(mockMuteChecker)
+			mc.On("IsNotificationMuted", mock.Anything, "a@example.com", scope).Return(stored, nil)
+			assert.Equal(t, stored, isRecipientMuted(context.Background(), mc, "a@example.com", scope), "scope=%s stored=%v", scope, stored)
+		}
+	}
+	// A nil checker means muting is not configured, so nothing is muted.
+	assert.False(t, isRecipientMuted(context.Background(), nil, "a@example.com", "new_recommendations"))
+}

internal/email/no_recipient_parity_test.go

@@ -0,0 +1,85 @@
+package email
+
+import (
+	"context"
+	"testing"
+
+	"github.com/aws/aws-sdk-go-v2/service/sesv2"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// tokenBearingSend is one of the senders' methods whose body carries a live,
+// recipient-scoped link (approve, revoke, exchange approval).
+type tokenBearingSend struct {
+	name string
+	call func(ctx context.Context, s SenderInterface) error
+}
+
+func tokenBearingSends() []tokenBearingSend {
+	recs := []RecommendationSummary{{Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1}}
+	return []tokenBearingSend{
+		{name: "SendRIExchangePendingApproval", call: func(ctx context.Context, s SenderInterface) error {
+			return s.SendRIExchangePendingApproval(ctx, RIExchangeNotificationData{
+				DashboardURL: "https://dashboard.example.com",
+				Exchanges:    []RIExchangeItem{{RecordID: "r1", ApprovalToken: "tok-1", SourceRIID: "ri-1"}},
+			})
+		}},
+		{name: "SendPurchaseApprovalRequest", call: func(ctx context.Context, s SenderInterface) error {
+			return s.SendPurchaseApprovalRequest(ctx, NotificationData{
+				DashboardURL: "https://dashboard.example.com", ApprovalToken: "tok", ExecutionID: "exec-1", Recommendations: recs,
+			})
+		}},
+		{name: "SendPurchaseScheduledNotification", call: func(ctx context.Context, s SenderInterface) error {
+			return s.SendPurchaseScheduledNotification(ctx, NotificationData{
+				DashboardURL: "https://dashboard.example.com", ExecutionID: "exec-1",
+				RevokeURL: "https://dashboard.example.com/purchases#history?execution=exec-1", Recommendations: recs,
+			})
+		}},
+		{name: "SendPurchaseExecutedNotification", call: func(ctx context.Context, s SenderInterface) error {
+			return s.SendPurchaseExecutedNotification(ctx, NotificationData{
+				DashboardURL: "https://dashboard.example.com", ExecutionID: "exec-1", RevocationToken: "tok", Recommendations: recs,
+			})
+		}},
+	}
+}
+
+// TestSenders_NoRecipientParity asserts that SES and SMTP agree on the
+// no-recipient contract for every token-bearing email (issue #111, A06-014):
+// both return ErrNoRecipient and neither delivers anything. Before the fix the
+// SMTP sender fell back to its static notify inbox and mailed the live link
+// there.
+func TestSenders_NoRecipientParity(t *testing.T) {
+	for _, tc := range tokenBearingSends() {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := context.Background()
+
+			ses := new(MockSESClient)
+			ses.On("GetAccount", mock.Anything, mock.Anything).
+				Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil).Maybe()
+			sesSender := &Sender{sesClient: ses, fromEmail: "noreply@example.com"}
+			sesErr := tc.call(ctx, sesSender)
+			require.ErrorIs(t, sesErr, ErrNoRecipient, "SES sender")
+			ses.AssertNotCalled(t, "SendEmail", mock.Anything, mock.Anything)
+
+			// A live SMTP server and a configured notify inbox: any fallback
+			// would deliver successfully and show up in receivedMsg.
+			server := newMockSMTPServer(t, false)
+			server.start(t)
+			smtpSender := &SMTPSender{
+				host:        "127.0.0.1",
+				port:        server.port,
+				fromEmail:   "notifications@example.com",
+				notifyEmail: "static-notify@example.com",
+			}
+			smtpErr := tc.call(ctx, smtpSender)
+			server.stop()
+			require.ErrorIs(t, smtpErr, ErrNoRecipient, "SMTP sender")
+			server.mu.Lock()
+			received := server.receivedMsg
+			server.mu.Unlock()
+			assert.Empty(t, received, "SMTP sender must not open a session to the notify inbox")
+		})
+	}
+}

internal/email/sender.go

@@ -247,7 +247,7 @@ func (s *Sender) createVerificationRequest(ctx context.Context, email string) er
 		return fmt.Errorf("failed to create email identity verification: %w", err)
 	}
 
-	logging.Infof("Created email verification request for %s - check inbox for verification email", email)
+	logging.Infof("Created email verification request for %s - check inbox for verification email", redactEmail(email))
 	return nil
 }