From 1d1b34edaf8b7d1e5df328fe896f8a0d31e50b9c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 20:24:57 +0200 Subject: [PATCH 1/3] feat(email): multipart HTML+plaintext for RI exchange pending approval (#296) Mirror the purchaseApprovalRequestHTMLTemplate pattern for the SendRIExchangePendingApproval flow: - Add riExchangePendingApprovalHTMLTemplate: inline-styled table of exchanges with per-row Approve/Reject CTA buttons, summary block, optional requested-by line, configurable cancellation-window note, and skipped-exchanges section. - Extend riExchangePendingApprovalTemplate (plaintext): labelled Approve/Reject URLs (replacing bracket notation), optional requested-by block, configurable cancellation-window note. - Extend RIExchangeNotificationData with RequestedByName, RequestedByEmail, RequestedAt, CancellationWindowNote fields alongside the RecipientEmail/CCEmails targeting fields. - Add RenderRIExchangePendingApprovalEmailHTML renderer. - Add sendRIExchangePendingApprovalVia helper (shared by Sender and SMTPSender, non-fatal HTML degrade on render error), now CC-aware. - Sender.SendRIExchangePendingApproval: requires RecipientEmail and routes through targeted SES multipart (SendToEmailWithCCMultipart), never broadcasting approval tokens via the SNS topic. - SMTPSender.SendRIExchangePendingApproval: resolves RecipientEmail (falling back to the static notify address), fails loud with ErrNoRecipient when neither is configured, then sends multipart. - 7 new test cases covering both halves, empty/custom cancellation notes, missing requested-by, and skipped-exchanges rendering. Rebased onto feat/multicloud-web-frontend: reconciled with the #1015 / multipart send now preserves that fail-loud, targeted-SES contract instead of falling back to the SNS broadcast topic. Updated the SMTP no-recipient coverage test to assert ErrNoRecipient accordingly. --- internal/email/coverage_extra_test.go | 18 ++- internal/email/sender.go | 12 ++ internal/email/smtp_sender.go | 20 ++- internal/email/template_renderers.go | 13 +- internal/email/template_renderers_test.go | 164 ++++++++++++++++++++++ internal/email/templates.go | 123 +++++++++++++--- 6 files changed, 319 insertions(+), 31 deletions(-) diff --git a/internal/email/coverage_extra_test.go b/internal/email/coverage_extra_test.go index 1d8baf778..b2a04d0ea 100644 --- a/internal/email/coverage_extra_test.go +++ b/internal/email/coverage_extra_test.go @@ -323,24 +323,30 @@ func TestRenderRegistrationReceivedEmail_NoAdminApprovers(t *testing.T) { // Tests for SMTPSender RI exchange and approval request methods -func TestSMTPSender_SendRIExchangePendingApproval_NoFromEmail(t *testing.T) { +func TestSMTPSender_SendRIExchangePendingApproval_NoRecipient(t *testing.T) { + // The RI exchange approval body carries live approval tokens, so with no + // data.RecipientEmail and no notifyEmail fallback the SMTP sender must + // surface ErrNoRecipient rather than silently dropping (or broadcasting) + // the email. Mirrors the hardened SendPurchaseApprovalRequest contract + // (#1015 / #1036) and the multipart path added in #296. sender := &SMTPSender{ - host: "smtp.example.com", - port: 587, - fromEmail: "", + host: "smtp.example.com", + port: 587, + fromEmail: "", + notifyEmail: "", } data := RIExchangeNotificationData{ DashboardURL: "https://dashboard.example.com", TotalPayment: "100.00", Exchanges: []RIExchangeItem{ - {RecordID: "r1", SourceRIID: "ri-1", SourceInstanceType: "m5.large", + {RecordID: "r1", ApprovalToken: "tok-1", SourceRIID: "ri-1", SourceInstanceType: "m5.large", TargetInstanceType: "m5.xlarge", TargetCount: 1, PaymentDue: "100.00"}, }, } err := sender.SendRIExchangePendingApproval(context.Background(), data) - require.NoError(t, err) + require.ErrorIs(t, err, ErrNoRecipient) } func TestSMTPSender_SendRIExchangeCompleted_NoFromEmail(t *testing.T) { diff --git a/internal/email/sender.go b/internal/email/sender.go index c0cdf5526..07bfbd854 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -480,6 +480,18 @@ type RIExchangeNotificationData struct { // CCEmails carries additional recipients informed of the pending exchanges // but not the authorized approvers. Deduplicated against RecipientEmail. CCEmails []string + // RequestedByName is the human-readable display name of the user who + // triggered the exchange run. Empty falls back to RequestedByEmail. + RequestedByName string + // RequestedByEmail is the requester's email address. Empty omits the + // requested-by block from the approval email. + RequestedByEmail string + // RequestedAt is the ISO-8601 / RFC3339 timestamp the exchange was + // submitted. Empty omits the timestamp from the summary. + RequestedAt string + // CancellationWindowNote is short text rendered below the approve/reject + // buttons. Empty falls back to a generic 6-hour note. + CancellationWindowNote string } // RIExchangeItem represents a single exchange in an email notification. diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index 3968934c6..d9a1f74fb 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -403,14 +403,22 @@ func (s *SMTPSender) SendPurchaseFailedNotification(ctx context.Context, data No return s.SendToEmail(ctx, s.notifyEmail, subject, body) } -// SendRIExchangePendingApproval sends an RI exchange approval email via SMTP. +// 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. func (s *SMTPSender) SendRIExchangePendingApproval(ctx context.Context, data RIExchangeNotificationData) error { - subject := fmt.Sprintf("CUDly - RI Exchange Approval Required (%d exchanges)", len(data.Exchanges)) - body, err := RenderRIExchangePendingApprovalEmail(data) - if err != nil { - return fmt.Errorf("failed to render ri exchange pending approval email: %w", err) + recipient := data.RecipientEmail + if recipient == "" { + recipient = s.notifyEmail } - return s.SendToEmail(ctx, s.notifyEmail, subject, body) + if recipient == "" { + return ErrNoRecipient + } + subject := fmt.Sprintf("CUDly - RI Exchange Approval Required (%d exchanges)", len(data.Exchanges)) + return sendRIExchangePendingApprovalVia(ctx, s, recipient, data.CCEmails, subject, data) } // SendRIExchangeCompleted sends an RI exchange completion email via SMTP. diff --git a/internal/email/template_renderers.go b/internal/email/template_renderers.go index 3af5c40aa..b6259d702 100644 --- a/internal/email/template_renderers.go +++ b/internal/email/template_renderers.go @@ -131,11 +131,22 @@ func RenderPurchaseFailedEmail(data NotificationData) (string, error) { return renderTextTemplate("failed", purchaseFailedTemplate, data) } -// RenderRIExchangePendingApprovalEmail renders the plain-text RI exchange pending approval email template. +// RenderRIExchangePendingApprovalEmail renders the plain-text RI exchange +// pending approval email template. Pair with +// RenderRIExchangePendingApprovalEmailHTML for multipart/alternative delivery. func RenderRIExchangePendingApprovalEmail(data RIExchangeNotificationData) (string, error) { return renderTextTemplate("ri-exchange-pending", riExchangePendingApprovalTemplate, data) } +// RenderRIExchangePendingApprovalEmailHTML renders the HTML half of the RI +// exchange pending approval email -- inline-styled per email-client constraints. +// The plain-text half (RenderRIExchangePendingApprovalEmail) carries the same +// content; receiving clients pick whichever they support via the +// multipart/alternative wrapper assembled by the sender. Issue #296. +func RenderRIExchangePendingApprovalEmailHTML(data RIExchangeNotificationData) (string, error) { + return renderTemplate("ri-exchange-pending-html", riExchangePendingApprovalHTMLTemplate, data) +} + // RenderRIExchangeCompletedEmail renders the plain-text RI exchange completed email template. func RenderRIExchangeCompletedEmail(data RIExchangeNotificationData) (string, error) { return renderTextTemplate("ri-exchange-completed", riExchangeCompletedTemplate, data) diff --git a/internal/email/template_renderers_test.go b/internal/email/template_renderers_test.go index 44e377d9c..8895d2237 100644 --- a/internal/email/template_renderers_test.go +++ b/internal/email/template_renderers_test.go @@ -570,3 +570,167 @@ func TestPlainTextTemplates_NoHTMLEscaping(t *testing.T) { assert.NotContains(t, html, xssName, "html/template must escape into RequestedByName, CancellationWindowNote, and Skipped.Reason and asserts the literal tag is absent from the rendered HTML (html/template auto-escaping). - TestRenderRIExchangePendingApprovalEmail_PlainTextVerbatim: feeds O'Brien & Co and a plus-addressed email into the plain-text renderer and asserts no ' / & entity encoding appears (text/template emits values verbatim). Also replace em-dash with double-hyphen in the HTML template comment (style rule). --- internal/email/template_renderers_test.go | 56 +++++++++++++++++++++++ internal/email/templates.go | 2 +- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/internal/email/template_renderers_test.go b/internal/email/template_renderers_test.go index 8895d2237..871df475f 100644 --- a/internal/email/template_renderers_test.go +++ b/internal/email/template_renderers_test.go @@ -734,3 +734,59 @@ func TestRenderRIExchangePendingApprovalEmailHTML_SkippedBlock(t *testing.T) { assert.Contains(t, html, "no matching target available") assert.Contains(t, html, "Skipped") } + +// Issue #296 / XSS guard: html/template must escape hostile payloads in the +// RIExchange HTML renderer. Tests RequestedByName, CancellationWindowNote, and +// Skipped.Reason -- the three free-text fields most likely to carry attacker +// input. Mirrors the analogous sub-test in TestPlainTextTemplates_NoHTMLEscaping. +func TestRenderRIExchangePendingApprovalEmailHTML_EscapesHostilePayload(t *testing.T) { + const xssPayload = `` + data := RIExchangeNotificationData{ + DashboardURL: "https://dashboard.example.com", + TotalPayment: "0.00", + RequestedByName: xssPayload, + RequestedByEmail: "attacker@evil.example", + CancellationWindowNote: xssPayload, + Exchanges: []RIExchangeItem{}, + Skipped: []SkippedExchange{{ + SourceRIID: "ri-skip-xss", + SourceInstanceType: "m5.large", + Reason: xssPayload, + }}, + } + + html, err := RenderRIExchangePendingApprovalEmailHTML(data) + require.NoError(t, err) + + // The literal script tag must not appear verbatim in the HTML output. + assert.NotContains(t, html, xssPayload, "html/template must escape