From 6d422e20eacf98eafdcc84278e3ae98d3a66edeb Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:07:29 +0000 Subject: [PATCH 1/2] fix(email): render plain-text bodies verbatim, redact logged addresses, per-scope mute failure, SMTP no-recipient parity - templates: RenderPurchaseScheduledDelayEmail and RenderPurchaseExecutedNotificationEmail render through text/template like every other plain-text body. html/template turned & into & in a copied revoke URL, entity-encoded requester names and addresses, and rendered the executed notice's plain-text revoke token as +. - sender/smtp_sender: redact the recipient in the SES verification log line and in the four SMTP SendToEmailWithCC/Multipart Debugf lines. - mute: a mute-store error fails open (send) only for the purchase_approvals and ri_exchange_approvals scopes; any other scope is informational and fails closed (don't send). - smtp_sender: SendRIExchangePendingApproval, SendPurchaseApprovalRequest, SendPurchaseScheduledNotification and SendPurchaseExecutedNotification return ErrNoRecipient on an empty data.RecipientEmail, as SES does, instead of mailing the token-bearing body to the static notify inbox. Refs #111 Co-Authored-By: Claude Opus 5.5 --- internal/email/coverage_extra_test.go | 20 ++--- internal/email/executed_notification_test.go | 15 ++-- internal/email/log_redaction_test.go | 80 ++++++++++++++++++ internal/email/mute.go | 33 ++++++-- internal/email/mute_test.go | 55 +++++++++++++ internal/email/no_recipient_parity_test.go | 85 ++++++++++++++++++++ internal/email/sender.go | 2 +- internal/email/smtp_sender.go | 61 ++++++-------- internal/email/template_renderers_test.go | 45 +++++++++++ internal/email/templates.go | 4 +- 10 files changed, 338 insertions(+), 62 deletions(-) create mode 100644 internal/email/log_redaction_test.go create mode 100644 internal/email/no_recipient_parity_test.go diff --git a/internal/email/coverage_extra_test.go b/internal/email/coverage_extra_test.go index 4cdf19182..1c45fc3ba 100644 --- a/internal/email/coverage_extra_test.go +++ b/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 diff --git a/internal/email/executed_notification_test.go b/internal/email/executed_notification_test.go index 41db6f099..4346eb341 100644 --- a/internal/email/executed_notification_test.go +++ b/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) diff --git a/internal/email/log_redaction_test.go b/internal/email/log_redaction_test.go new file mode 100644 index 000000000..3a5717909 --- /dev/null +++ b/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", "

body

") + }}, + } + 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") + }) + } + } +} diff --git a/internal/email/mute.go b/internal/email/mute.go index 8bf8f4d0c..ad649d5b8 100644 --- a/internal/email/mute.go +++ b/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 diff --git a/internal/email/mute_test.go b/internal/email/mute_test.go index 1010eea0e..7ee519c42 100644 --- a/internal/email/mute_test.go +++ b/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")) +} diff --git a/internal/email/no_recipient_parity_test.go b/internal/email/no_recipient_parity_test.go new file mode 100644 index 000000000..8613b3243 --- /dev/null +++ b/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") + }) + } +} diff --git a/internal/email/sender.go b/internal/email/sender.go index 9ffbfe40e..2adf8a1b1 100644 --- a/internal/email/sender.go +++ b/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 } diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index b53cb2583..59987d5ed 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -150,9 +150,9 @@ func (s *SMTPSender) SendToEmailWithCCMultipart(ctx context.Context, toEmail str } if len(sanitizedCC) > 0 { - logging.Debugf("Sent multipart email via SMTP to %s (cc %d): %s", toEmail, len(sanitizedCC), subject) + logging.Debugf("Sent multipart email via SMTP to %s (cc %d): %s", redactEmail(toEmail), len(sanitizedCC), subject) } else { - logging.Debugf("Sent multipart email via SMTP to %s: %s", toEmail, subject) + logging.Debugf("Sent multipart email via SMTP to %s: %s", redactEmail(toEmail), subject) } return nil } @@ -181,9 +181,9 @@ func (s *SMTPSender) SendToEmailWithCC(ctx context.Context, toEmail string, ccEm } if len(sanitizedCC) > 0 { - logging.Debugf("Sent email via SMTP to %s (cc %d): %s", toEmail, len(sanitizedCC), subject) + logging.Debugf("Sent email via SMTP to %s (cc %d): %s", redactEmail(toEmail), len(sanitizedCC), subject) } else { - logging.Debugf("Sent email via SMTP to %s: %s", toEmail, subject) + logging.Debugf("Sent email via SMTP to %s: %s", redactEmail(toEmail), subject) } return nil } @@ -519,15 +519,12 @@ func (s *SMTPSender) SendPurchaseFailedNotification(ctx context.Context, data No // SendRIExchangePendingApproval sends an RI exchange approval email via SMTP // as multipart/alternative (plain-text + styled HTML). The body carries live -// approval tokens, so it is sent only to the resolved recipient (the -// submitter's notification email, falling back to the static SMTP notify -// address) plus the deduplicated CC list; it never broadcasts. Returns -// ErrNoRecipient when neither address is configured. Issue #296. +// approval tokens, so it is sent only to data.RecipientEmail plus the +// deduplicated CC list; it never broadcasts. Returns ErrNoRecipient when +// data.RecipientEmail is empty, matching the SES sender: the static notify +// inbox is not a fallback for token-bearing mail (issue #111). Issue #296. func (s *SMTPSender) SendRIExchangePendingApproval(ctx context.Context, data RIExchangeNotificationData) error { recipient := data.RecipientEmail - if recipient == "" { - recipient = s.notifyEmail - } if recipient == "" { return ErrNoRecipient } @@ -557,10 +554,11 @@ func (s *SMTPSender) SendRIExchangeCompleted(ctx context.Context, data RIExchang return s.SendToEmail(ctx, s.notifyEmail, subject, body) } -// SendPurchaseApprovalRequest sends a purchase approval request email via SMTP. -// Prefers data.RecipientEmail (the submitter's notification email from app -// settings) over the static SMTP-configured s.notifyEmail so the approval token -// lands in the right inbox per submitter. +// SendPurchaseApprovalRequest sends a purchase approval request email via SMTP +// to data.RecipientEmail (the submitter's notification email from app +// settings). Returns ErrNoRecipient when it is empty, matching the SES sender: +// the one-time approval token must not fall back to the static notify inbox +// (issue #111). // // Mute check + List-Unsubscribe mirror the SES (*Sender) path: if the recipient // has opted out of purchase_approvals the email is silently skipped, muted CC @@ -568,9 +566,6 @@ func (s *SMTPSender) SendRIExchangeCompleted(ctx context.Context, data RIExchang // when an unsubscribe base URL is configured. func (s *SMTPSender) SendPurchaseApprovalRequest(ctx context.Context, data NotificationData) error { recipient := data.RecipientEmail - if recipient == "" { - recipient = s.notifyEmail - } if recipient == "" { return ErrNoRecipient } @@ -653,36 +648,32 @@ func buildListUnsubscribeHeaderLines(headerValue, postValue string) string { } // SendPurchaseScheduledNotification sends the Gmail-style pre-fire delay -// notification email via SMTP. Mirrors the Sender implementation's behavior. +// notification email via SMTP. Mirrors the Sender implementation: the body +// carries a live, execution-scoped revoke link, so an empty +// data.RecipientEmail returns ErrNoRecipient instead of falling back to the +// static notify inbox (issue #111). func (s *SMTPSender) SendPurchaseScheduledNotification(ctx context.Context, data NotificationData) error { + if data.RecipientEmail == "" { + return ErrNoRecipient + } body, err := RenderPurchaseScheduledDelayEmail(data) if err != nil { return fmt.Errorf("failed to render purchase scheduled delay email: %w", err) } subject := fmt.Sprintf("CUDly - Purchase Scheduled for %s", data.RevocationWindowClosesAt) - recipient := data.RecipientEmail - if recipient == "" { - recipient = s.notifyEmail - } - if recipient == "" { - return ErrNoRecipient - } - return s.SendToEmailWithCC(ctx, recipient, data.CCEmails, subject, body) + return s.SendToEmailWithCC(ctx, data.RecipientEmail, data.CCEmails, subject, body) } // SendPurchaseExecutedNotification sends the post-execution notification email -// via SMTP. Mirrors SendPurchaseApprovalRequest: prefers data.RecipientEmail -// over the static s.notifyEmail. Issue #291. +// via SMTP to data.RecipientEmail. The body can carry a revocation token, so an +// empty recipient returns ErrNoRecipient, matching the SES sender, instead of +// falling back to the static notify inbox (issue #111). Issue #291. func (s *SMTPSender) SendPurchaseExecutedNotification(ctx context.Context, data NotificationData) error { - recipient := data.RecipientEmail - if recipient == "" { - recipient = s.notifyEmail - } - if recipient == "" { + if data.RecipientEmail == "" { return ErrNoRecipient } subject := buildExecutedNotificationSubject(data) - return sendPurchaseExecutedNotificationVia(ctx, s, recipient, subject, data) + return sendPurchaseExecutedNotificationVia(ctx, s, data.RecipientEmail, subject, data) } // SendRegistrationReceivedNotification sends an email to CUDly administrators diff --git a/internal/email/template_renderers_test.go b/internal/email/template_renderers_test.go index 5cc0c7ec4..eecb40d93 100644 --- a/internal/email/template_renderers_test.go +++ b/internal/email/template_renderers_test.go @@ -554,6 +554,45 @@ func TestPlainTextTemplates_NoHTMLEscaping(t *testing.T) { assert.NotContains(t, body, "&") }) + t.Run("PurchaseScheduledDelay_revokeURL_verbatim", func(t *testing.T) { + // A revoke link with a second query parameter must stay copyable: + // html/template would turn the & into & and break the link. + const revokeURL = "https://example.com/purchases#history?execution=exec-3&source=email" + data := NotificationData{ + DashboardURL: "https://example.com", + ExecutionID: "exec-3", + RevokeURL: revokeURL, + RevocationWindowClosesAt: "2026-10-28 <09:00>", + Recommendations: []RecommendationSummary{{Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1}}, + } + body, err := RenderPurchaseScheduledDelayEmail(data) + require.NoError(t, err) + assert.Contains(t, body, revokeURL, "revoke URL must survive verbatim in the plain-text scheduled-delay body") + assert.Contains(t, body, "2026-10-28 <09:00>") + assert.NotContains(t, body, "&") + assert.NotContains(t, body, "<") + }) + + t.Run("PurchaseExecutedNotification_requestedBy_verbatim", func(t *testing.T) { + data := NotificationData{ + DashboardURL: "https://example.com", + ExecutionID: "exec-4", + RequestedByName: name, + RequestedByEmail: email, + RevocationToken: "tok+/=", + Recommendations: []RecommendationSummary{{Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1}}, + } + body, err := RenderPurchaseExecutedNotificationEmail(data) + require.NoError(t, err) + assert.Contains(t, body, "Requested by: "+name+" <"+email+">", + "requester name and address must be verbatim in the plain-text executed notice") + // The revoke token is still query-escaped by urlquery, exactly once. + assert.Contains(t, body, "/api/purchases/revoke/exec-4?token=tok%2B%2F%3D") + assert.NotContains(t, body, "'") + assert.NotContains(t, body, "&") + assert.NotContains(t, body, "<") + }) + t.Run("HTML_renderers_still_escape", func(t *testing.T) { // HTML halves MUST still escape data -- that is the XSS defense. // A name with a script tag must not survive literally in the HTML body. @@ -568,6 +607,12 @@ func TestPlainTextTemplates_NoHTMLEscaping(t *testing.T) { html, err := RenderPurchaseApprovalRequestEmailHTML(data) require.NoError(t, err) assert.NotContains(t, html, xssName, "html/template must escape