From 17c4d83f62d238fc12b87dc0ad74fab782185866 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 01:40:24 -0700 Subject: [PATCH 1/4] sec(email): route scheduled-purchase and RI-exchange approval tokens through targeted SES, not SNS broadcast (closes #1015) SendScheduledPurchaseNotification and SendRIExchangePendingApproval on the SES/SNS Sender previously rendered bodies that embed live approve/pause/cancel tokens and shipped them via SendNotification (SNS broadcast). Every topic subscriber received working action links, matching the exact threat the purchase-approval path was hardened against in sender.go:386-391 and templates.go:698-704. Fix: - Both methods now require data.RecipientEmail and route through SendToEmailWithCC (targeted SES), mirroring SendPurchaseApprovalRequest. Return ErrNoRecipient when the field is empty rather than silently broadcasting. - Add RecipientEmail/CCEmails fields to RIExchangeNotificationData. - Update call sites (purchase/notifications.go, handler_ri_exchange.go) to resolve GlobalConfig.NotificationEmail and pass it as RecipientEmail. When the global email is unconfigured, log a warning and skip the notification rather than broadcast. - Add structural guard in SendNotification: reject any message body that contains "token=" with ErrTokenInBroadcast, preventing future regressions regardless of how a token-bearing body reaches the broadcast path. - Add 8 regression tests: guard rejects token bodies, guard allows clean bodies, both methods use SES not SNS when RecipientEmail is set, both methods return ErrNoRecipient when RecipientEmail is absent. --- internal/email/coverage_extra_test.go | 12 +- internal/email/coverage_test.go | 12 +- internal/email/sender.go | 26 ++- internal/email/sender_test.go | 195 ++++++++++++++++++++ internal/email/templates.go | 29 ++- internal/email/templates_test.go | 73 +++++--- internal/purchase/notifications.go | 21 ++- internal/purchase/notifications_test.go | 8 +- internal/server/handler_ri_exchange.go | 24 ++- internal/server/handler_ri_exchange_test.go | 14 +- 10 files changed, 357 insertions(+), 57 deletions(-) diff --git a/internal/email/coverage_extra_test.go b/internal/email/coverage_extra_test.go index 8be4f25b9..10bd4d8f2 100644 --- a/internal/email/coverage_extra_test.go +++ b/internal/email/coverage_extra_test.go @@ -408,12 +408,14 @@ 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{ @@ -421,12 +423,14 @@ func TestSender_SendRIExchangePendingApproval_NoTopic(t *testing.T) { 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) { diff --git a/internal/email/coverage_test.go b/internal/email/coverage_test.go index b13d0d354..c40215f44 100644 --- a/internal/email/coverage_test.go +++ b/internal/email/coverage_test.go @@ -1305,13 +1305,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) { diff --git a/internal/email/sender.go b/internal/email/sender.go index 75b3d3011..25818649e 100644 --- a/internal/email/sender.go +++ b/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,17 @@ func NewSenderWithClients(snsClient SNSPublisher, sesClient SESEmailSender, cfg } } -// SendNotification sends a notification email via SNS +// SendNotification sends a notification email via SNS. +// +// Guard: messages whose body contains "token=" 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. func (s *Sender) SendNotification(ctx context.Context, subject, message string) error { + if strings.Contains(message, "token=") { + return ErrTokenInBroadcast + } + if s.topicARN == "" { logging.Debug("No SNS topic configured, skipping email notification") return nil @@ -464,6 +479,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 diff --git a/internal/email/sender_test.go b/internal/email/sender_test.go index c5fa24262..6f1de09b4 100644 --- a/internal/email/sender_test.go +++ b/internal/email/sender_test.go @@ -878,3 +878,198 @@ func TestSender_SendToEmailWithCCMultipart_FallsBackWhenHTMLEmpty_Issue287(t *te require.NotNil(t, captured.Content.Simple.Body.Text) assert.Nil(t, captured.Content.Simple.Body.Html, "empty HTML body should NOT be sent as multipart") } + +// --------------------------------------------------------------------------- +// Regression tests for issue #1015: approval tokens broadcast via SNS +// --------------------------------------------------------------------------- + +// TestSendNotification_RejectsTokenBearingBody verifies the structural guard: +// SendNotification must return ErrTokenInBroadcast when the message body +// contains "token=" so a body with an approval URL can never reach the SNS +// broadcast topic, regardless of how it got there. +func TestSendNotification_RejectsTokenBearingBody(t *testing.T) { + t.Parallel() + mockSNS := new(MockSNSClient) + // No Publish calls expected — the guard fires before the client is touched. + t.Cleanup(func() { mockSNS.AssertExpectations(t) }) + + sender := NewSenderWithClients(mockSNS, nil, SenderConfig{ + TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + }) + + ctx := context.Background() + tokenBodies := []string{ + "Review: https://app.example.com/approve?token=abc123", + "token=somesecret", + "?token=xyz&action=pause", + } + for _, body := range tokenBodies { + err := sender.SendNotification(ctx, "Test", body) + assert.ErrorIsf(t, err, ErrTokenInBroadcast, + "expected ErrTokenInBroadcast for body %q", body) + } +} + +// TestSendNotification_AllowsTokenFreeBody verifies that a well-formed +// broadcast body that contains no token reaches SNS normally. +func TestSendNotification_AllowsTokenFreeBody(t *testing.T) { + t.Parallel() + mockSNS := new(MockSNSClient) + mockSNS.On("Publish", mock.Anything, mock.AnythingOfType("*sns.PublishInput")). + Return(&sns.PublishOutput{MessageId: aws.String("msg-ok")}, nil).Once() + t.Cleanup(func() { mockSNS.AssertExpectations(t) }) + + sender := NewSenderWithClients(mockSNS, nil, SenderConfig{ + TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + }) + + ctx := context.Background() + err := sender.SendNotification(ctx, "Savings found", "3 new recommendations. Sign in to review.") + require.NoError(t, err) +} + +// TestSendScheduledPurchaseNotification_UsesSESNotSNS verifies that a +// scheduled-purchase notification with RecipientEmail set reaches SES +// (targeted delivery) and does NOT call the SNS publisher. +// +// Regression test for issue #1015: previously the method called +// s.SendNotification which broadcast the token-bearing body to every +// SNS subscriber. +func TestSendScheduledPurchaseNotification_UsesSESNotSNS(t *testing.T) { + t.Parallel() + mockSNS := new(MockSNSClient) + // SNS Publish must NOT be called. + t.Cleanup(func() { mockSNS.AssertExpectations(t) }) + + mockSES := new(MockSESClient) + mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil).Once() + mockSES.On("SendEmail", mock.Anything, mock.AnythingOfType("*sesv2.SendEmailInput")). + Return(&sesv2.SendEmailOutput{MessageId: aws.String("ses-msg-1")}, nil).Once() + t.Cleanup(func() { mockSES.AssertExpectations(t) }) + + sender := NewSenderWithClients(mockSNS, mockSES, SenderConfig{ + TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + FromEmail: "noreply@example.com", + }) + + data := NotificationData{ + DashboardURL: "https://app.example.com", + ApprovalToken: "supersecret-token", + RecipientEmail: "notify@example.com", + TotalSavings: 800.0, + DaysUntilPurchase: 3, + PlanName: "Test Plan", + } + + ctx := context.Background() + err := sender.SendScheduledPurchaseNotification(ctx, data) + require.NoError(t, err) +} + +// TestSendScheduledPurchaseNotification_ErrNoRecipientWhenEmpty verifies that +// omitting RecipientEmail returns ErrNoRecipient, not a silent broadcast. +func TestSendScheduledPurchaseNotification_ErrNoRecipientWhenEmpty(t *testing.T) { + t.Parallel() + mockSNS := new(MockSNSClient) + mockSES := new(MockSESClient) + // Neither SNS Publish nor SES SendEmail should be called. + t.Cleanup(func() { + mockSNS.AssertExpectations(t) + mockSES.AssertExpectations(t) + }) + + sender := NewSenderWithClients(mockSNS, mockSES, SenderConfig{ + TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + FromEmail: "noreply@example.com", + }) + + data := NotificationData{ + DashboardURL: "https://app.example.com", + ApprovalToken: "supersecret-token", + PlanName: "Test Plan", + // RecipientEmail intentionally absent + } + + err := sender.SendScheduledPurchaseNotification(context.Background(), data) + require.ErrorIs(t, err, ErrNoRecipient) +} + +// TestSendRIExchangePendingApproval_UsesSESNotSNS verifies that an RI-exchange +// pending-approval notification with RecipientEmail set reaches SES (targeted +// delivery) and does NOT call the SNS publisher. +// +// Regression test for issue #1015: previously the method called +// s.SendNotification which broadcast per-exchange approve/reject tokens to +// every SNS subscriber, allowing unauthorised spend approval. +func TestSendRIExchangePendingApproval_UsesSESNotSNS(t *testing.T) { + t.Parallel() + mockSNS := new(MockSNSClient) + // SNS Publish must NOT be called. + t.Cleanup(func() { mockSNS.AssertExpectations(t) }) + + mockSES := new(MockSESClient) + mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil).Once() + mockSES.On("SendEmail", mock.Anything, mock.AnythingOfType("*sesv2.SendEmailInput")). + Return(&sesv2.SendEmailOutput{MessageId: aws.String("ses-msg-2")}, nil).Once() + t.Cleanup(func() { mockSES.AssertExpectations(t) }) + + sender := NewSenderWithClients(mockSNS, mockSES, SenderConfig{ + TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + FromEmail: "noreply@example.com", + }) + + data := RIExchangeNotificationData{ + DashboardURL: "https://app.example.com", + Mode: "manual", + RecipientEmail: "notify@example.com", + TotalPayment: "150.00", + Exchanges: []RIExchangeItem{ + { + RecordID: "rec-1", + ApprovalToken: "exchange-secret-token", + SourceRIID: "ri-abc", + SourceInstanceType: "m5.large", + TargetInstanceType: "m5.xlarge", + TargetCount: 1, + PaymentDue: "150.00", + UtilizationPct: 45.0, + }, + }, + } + + ctx := context.Background() + err := sender.SendRIExchangePendingApproval(ctx, data) + require.NoError(t, err) +} + +// TestSendRIExchangePendingApproval_ErrNoRecipientWhenEmpty verifies that +// omitting RecipientEmail returns ErrNoRecipient, not a silent broadcast. +func TestSendRIExchangePendingApproval_ErrNoRecipientWhenEmpty(t *testing.T) { + t.Parallel() + mockSNS := new(MockSNSClient) + mockSES := new(MockSESClient) + // Neither SNS Publish nor SES SendEmail should be called. + t.Cleanup(func() { + mockSNS.AssertExpectations(t) + mockSES.AssertExpectations(t) + }) + + sender := NewSenderWithClients(mockSNS, mockSES, SenderConfig{ + TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + FromEmail: "noreply@example.com", + }) + + data := RIExchangeNotificationData{ + DashboardURL: "https://app.example.com", + Mode: "manual", + // RecipientEmail intentionally absent + Exchanges: []RIExchangeItem{ + {RecordID: "rec-1", ApprovalToken: "exchange-secret-token"}, + }, + } + + err := sender.SendRIExchangePendingApproval(context.Background(), data) + require.ErrorIs(t, err, ErrNoRecipient) +} diff --git a/internal/email/templates.go b/internal/email/templates.go index 824218557..a71de0f26 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -350,15 +350,24 @@ func (s *Sender) SendNewRecommendationsNotification(ctx context.Context, data No return s.SendNotification(ctx, subject, body) } -// SendScheduledPurchaseNotification sends a notification about upcoming automated purchase +// SendScheduledPurchaseNotification sends a notification about an upcoming +// automated purchase. The rendered body embeds approve/pause/cancel links +// that carry a live token, so it must be delivered to a specific recipient +// via targeted SES, never broadcast through the SNS topic. +// +// Returns ErrNoRecipient when data.RecipientEmail is empty so the caller can +// surface a precise reason rather than silently dropping the notification. func (s *Sender) SendScheduledPurchaseNotification(ctx context.Context, data NotificationData) error { + if data.RecipientEmail == "" { + return ErrNoRecipient + } body, err := RenderScheduledPurchaseEmail(data) if err != nil { return fmt.Errorf("failed to render scheduled purchase email: %w", err) } subject := fmt.Sprintf("CUDly - Scheduled Purchase in %d Days: %s", data.DaysUntilPurchase, data.PlanName) - return s.SendNotification(ctx, subject, body) + return s.SendToEmailWithCC(ctx, data.RecipientEmail, data.CCEmails, subject, body) } // SendPurchaseConfirmation sends a confirmation after successful purchases @@ -435,15 +444,27 @@ func (s *Sender) SendUserInviteEmail(ctx context.Context, email, setupURL string ) } -// SendRIExchangePendingApproval sends an email with RI exchange approval links +// SendRIExchangePendingApproval sends an email with RI exchange approval links. +// The rendered body contains per-exchange approve/reject links that carry live +// tokens; any subscriber of the SNS topic who receives this body can +// approve spend they were never authorised for. This method therefore routes +// through targeted SES (SendToEmailWithCC), mirroring the hardened path used +// by SendPurchaseApprovalRequest. +// +// Returns ErrNoRecipient when data.RecipientEmail is empty. Callers must +// resolve a recipient (e.g. the global notification email from GlobalConfig) +// before invoking this method. func (s *Sender) SendRIExchangePendingApproval(ctx context.Context, data RIExchangeNotificationData) error { + if data.RecipientEmail == "" { + return ErrNoRecipient + } body, err := RenderRIExchangePendingApprovalEmail(data) if err != nil { return fmt.Errorf("failed to render ri exchange pending approval email: %w", err) } subject := fmt.Sprintf("CUDly - RI Exchange Approval Required (%d exchanges)", len(data.Exchanges)) - return s.SendNotification(ctx, subject, body) + return s.SendToEmailWithCC(ctx, data.RecipientEmail, data.CCEmails, subject, body) } // SendRIExchangeCompleted sends a notification about completed RI exchanges diff --git a/internal/email/templates_test.go b/internal/email/templates_test.go index a4388fac5..ac2d1b219 100644 --- a/internal/email/templates_test.go +++ b/internal/email/templates_test.go @@ -44,17 +44,24 @@ func TestSender_SendNewRecommendationsNotification_Success(t *testing.T) { } func TestSender_SendScheduledPurchaseNotification_Success(t *testing.T) { - mockSNS := new(MockSNSClient) - mockSNS.On("Publish", mock.Anything, mock.AnythingOfType("*sns.PublishInput")). - Return(&sns.PublishOutput{MessageId: aws.String("msg-123")}, nil) + // Scheduled purchase notifications carry approval tokens and must be + // delivered via targeted SES, not the SNS broadcast topic. + t.Cleanup(func() {}) + mockSES := new(MockSESClient) + mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) + mockSES.On("SendEmail", mock.Anything, mock.AnythingOfType("*sesv2.SendEmailInput")). + Return(&sesv2.SendEmailOutput{MessageId: aws.String("msg-123")}, nil) + t.Cleanup(func() { mockSES.AssertExpectations(t) }) - sender := NewSenderWithClients(mockSNS, nil, SenderConfig{ - TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + sender := NewSenderWithClients(nil, mockSES, SenderConfig{ + FromEmail: "noreply@example.com", }) data := NotificationData{ DashboardURL: "https://dashboard.example.com", ApprovalToken: "token-123", + RecipientEmail: "notify@example.com", TotalSavings: 1000.00, TotalUpfrontCost: 3000.00, PurchaseDate: "February 1, 2024", @@ -66,7 +73,6 @@ func TestSender_SendScheduledPurchaseNotification_Success(t *testing.T) { err := sender.SendScheduledPurchaseNotification(ctx, data) require.NoError(t, err) - mockSNS.AssertExpectations(t) } func TestSender_SendPurchaseConfirmation_Success(t *testing.T) { @@ -192,19 +198,24 @@ func TestSender_SendNewRecommendationsNotification_NoTopic(t *testing.T) { require.NoError(t, err) } -func TestSender_SendScheduledPurchaseNotification_NoTopic(t *testing.T) { +func TestSender_SendScheduledPurchaseNotification_NoRecipient(t *testing.T) { + // When RecipientEmail is empty the send must be rejected — the body carries + // approval tokens that must not be broadcast via SNS. sender := &Sender{ - topicARN: "", + topicARN: "arn:aws:sns:us-east-1:123456789012:topic", + fromEmail: "noreply@example.com", } data := NotificationData{ - DashboardURL: "https://dashboard.example.com", + DashboardURL: "https://dashboard.example.com", + ApprovalToken: "secret-token", + // RecipientEmail intentionally empty } ctx := context.Background() err := sender.SendScheduledPurchaseNotification(ctx, data) - require.NoError(t, err) + require.ErrorIs(t, err, ErrNoRecipient) } func TestSender_SendPurchaseConfirmation_NoTopic(t *testing.T) { @@ -282,19 +293,27 @@ func TestSender_SendNewRecommendationsNotification_SNSError(t *testing.T) { require.Error(t, err) } -func TestSender_SendScheduledPurchaseNotification_SNSError(t *testing.T) { - mockSNS := new(MockSNSClient) - sender := &Sender{ - snsClient: mockSNS, - topicARN: "arn:aws:sns:us-east-1:123456789:topic", - } +func TestSender_SendScheduledPurchaseNotification_SESError(t *testing.T) { + // SES send error must propagate to the caller. + mockSES := new(MockSESClient) + mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) + mockSES.On("SendEmail", mock.Anything, mock.AnythingOfType("*sesv2.SendEmailInput")). + Return(nil, assert.AnError) + t.Cleanup(func() { mockSES.AssertExpectations(t) }) - mockSNS.On("Publish", mock.Anything, mock.Anything).Return(nil, assert.AnError) + sender := NewSenderWithClients(nil, mockSES, SenderConfig{ + FromEmail: "noreply@example.com", + }) ctx := context.Background() data := NotificationData{ - DashboardURL: "https://example.com", - TotalSavings: 500.0, + DashboardURL: "https://example.com", + RecipientEmail: "notify@example.com", + ApprovalToken: "token-abc", + TotalSavings: 500.0, + DaysUntilPurchase: 3, + PlanName: "Plan A", } err := sender.SendScheduledPurchaseNotification(ctx, data) @@ -420,17 +439,22 @@ func TestSender_SendNewRecommendationsNotification_MultipleRecommendations(t *te } func TestSender_SendScheduledPurchaseNotification_WithUpfrontCost(t *testing.T) { - mockSNS := new(MockSNSClient) - mockSNS.On("Publish", mock.Anything, mock.AnythingOfType("*sns.PublishInput")). - Return(&sns.PublishOutput{MessageId: aws.String("msg-123")}, nil) + // Verify that a well-formed data payload with RecipientEmail succeeds via SES. + mockSES := new(MockSESClient) + mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) + mockSES.On("SendEmail", mock.Anything, mock.AnythingOfType("*sesv2.SendEmailInput")). + Return(&sesv2.SendEmailOutput{MessageId: aws.String("msg-123")}, nil) + t.Cleanup(func() { mockSES.AssertExpectations(t) }) - sender := NewSenderWithClients(mockSNS, nil, SenderConfig{ - TopicARN: "arn:aws:sns:us-east-1:123456789012:topic", + sender := NewSenderWithClients(nil, mockSES, SenderConfig{ + FromEmail: "noreply@example.com", }) data := NotificationData{ DashboardURL: "https://dashboard.example.com", ApprovalToken: "token-456", + RecipientEmail: "notify@example.com", TotalSavings: 1500.00, TotalUpfrontCost: 6000.00, PurchaseDate: "April 1, 2024", @@ -452,7 +476,6 @@ func TestSender_SendScheduledPurchaseNotification_WithUpfrontCost(t *testing.T) err := sender.SendScheduledPurchaseNotification(ctx, data) require.NoError(t, err) - mockSNS.AssertExpectations(t) } func TestSender_SendPurchaseConfirmation_WithMultipleRecommendations(t *testing.T) { diff --git a/internal/purchase/notifications.go b/internal/purchase/notifications.go index 3a9c3150a..fa0292b27 100644 --- a/internal/purchase/notifications.go +++ b/internal/purchase/notifications.go @@ -73,8 +73,20 @@ func (m *Manager) sendPlanNotification(ctx context.Context, plan *config.Purchas return false } + // Resolve the notification recipient. The rendered body embeds action tokens + // and must be delivered via targeted SES, not broadcast via SNS. Log a + // warning and skip rather than broadcasting if no recipient is configured. + notifyEmail := "" + if globalCfg, cfgErr := m.config.GetGlobalConfig(ctx); cfgErr == nil && globalCfg != nil && globalCfg.NotificationEmail != nil { + notifyEmail = *globalCfg.NotificationEmail + } + if notifyEmail == "" { + logging.Warnf("Skipping scheduled-purchase notification for plan %s: no notification email configured in global settings", plan.Name) + return false + } + // Send notification - data := m.buildNotificationData(*plan, execution, daysUntil) + data := m.buildNotificationData(*plan, execution, daysUntil, notifyEmail) if err := m.email.SendScheduledPurchaseNotification(ctx, data); err != nil { logging.Errorf("Failed to send notification: %v", err) return false @@ -127,8 +139,10 @@ func (m *Manager) getOrCreateExecution(ctx context.Context, plan *config.Purchas return execution, nil } -// buildNotificationData creates notification data from plan and execution -func (m *Manager) buildNotificationData(plan config.PurchasePlan, exec *config.PurchaseExecution, daysUntil int) email.NotificationData { +// buildNotificationData creates notification data from plan and execution. +// notifyEmail is the global notification address from GlobalConfig; it is set +// as RecipientEmail so the token-bearing body routes through targeted SES. +func (m *Manager) buildNotificationData(plan config.PurchasePlan, exec *config.PurchaseExecution, daysUntil int, notifyEmail string) email.NotificationData { data := email.NotificationData{ DashboardURL: m.dashboardURL, ApprovalToken: exec.ApprovalToken, @@ -139,6 +153,7 @@ func (m *Manager) buildNotificationData(plan config.PurchasePlan, exec *config.P PurchaseDate: exec.ScheduledDate.Format("January 2, 2006"), DaysUntilPurchase: daysUntil, PlanName: plan.Name, + RecipientEmail: notifyEmail, } for _, rec := range exec.Recommendations { diff --git a/internal/purchase/notifications_test.go b/internal/purchase/notifications_test.go index 57acc2fce..f4a296822 100644 --- a/internal/purchase/notifications_test.go +++ b/internal/purchase/notifications_test.go @@ -147,7 +147,7 @@ func TestManager_BuildNotificationData(t *testing.T) { }, } - data := manager.buildNotificationData(plan, execution, 5) + data := manager.buildNotificationData(plan, execution, 5, "notify@example.com") assert.Equal(t, "https://dashboard.example.com", data.DashboardURL) assert.Equal(t, "token-abc", data.ApprovalToken) @@ -314,10 +314,13 @@ func TestManager_SendUpcomingPurchaseNotifications_WithNotification(t *testing.T }, } + notifyEmailStr := "notify@example.com" + globalCfg := &config.GlobalConfig{NotificationEmail: ¬ifyEmailStr} mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) // No existing execution found mockStore.On("GetExecutionByPlanAndDate", ctx, "plan-123", nextExec).Return(nil, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) mockEmail.On("SendScheduledPurchaseNotification", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) @@ -458,10 +461,13 @@ func TestManager_SendUpcomingPurchaseNotifications_EmailFails(t *testing.T) { }, } + notifyEmailStr := "notify@example.com" + globalCfg := &config.GlobalConfig{NotificationEmail: ¬ifyEmailStr} mockStore.On("ListPurchasePlans", ctx, config.PurchasePlanFilter{}).Return(plans, nil) // No existing execution found mockStore.On("GetExecutionByPlanAndDate", ctx, "plan-123", nextExec).Return(nil, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) mockEmail.On("SendScheduledPurchaseNotification", ctx, mock.AnythingOfType("email.NotificationData")).Return(errors.New("email failed")) manager := &Manager{ diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index 8f2cd72b9..0fa714857 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -113,7 +113,11 @@ func (app *Application) executeRIExchangeReshape(ctx context.Context, cfg *confi return nil, fmt.Errorf("auto exchange failed: %w", err) } - app.sendExchangeNotification(ctx, result) + notifyEmail := "" + if cfg.NotificationEmail != nil { + notifyEmail = *cfg.NotificationEmail + } + app.sendExchangeNotification(ctx, result, notifyEmail) log.Printf("RI exchange reshape complete: mode=%s completed=%d pending=%d failed=%d skipped=%d", result.Mode, len(result.Completed), len(result.Pending), len(result.Failed), len(result.Skipped)) @@ -136,8 +140,10 @@ func resolveAccountID(ctx context.Context, awsCfg aws.Config) string { } // sendExchangeNotification sends email notifications based on the exchange result. -// Email errors are logged but don't fail the task. -func (app *Application) sendExchangeNotification(ctx context.Context, result *exchange.AutoExchangeResult) { +// notifyEmail is the global notification address from GlobalConfig; it becomes +// the RecipientEmail for pending-approval messages that carry approval tokens +// and must not be broadcast via SNS. Email errors are logged but don't fail the task. +func (app *Application) sendExchangeNotification(ctx context.Context, result *exchange.AutoExchangeResult, notifyEmail string) { if app.Email == nil { return } @@ -146,7 +152,7 @@ func (app *Application) sendExchangeNotification(ctx context.Context, result *ex return } - data := buildExchangeNotificationData(result, app.appConfig.DashboardURL) + data := buildExchangeNotificationData(result, app.appConfig.DashboardURL, notifyEmail) var err error if result.Mode == "manual" && len(result.Pending) > 0 { @@ -165,10 +171,14 @@ func exchangeHasResults(result *exchange.AutoExchangeResult) bool { } // buildExchangeNotificationData converts AutoExchangeResult into email notification data. -func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboardURL string) email.RIExchangeNotificationData { +// notifyEmail is the global notification address from GlobalConfig; it is set as +// RecipientEmail so token-bearing pending-approval emails route to a specific inbox +// rather than the SNS broadcast topic. +func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboardURL, notifyEmail string) email.RIExchangeNotificationData { data := email.RIExchangeNotificationData{ - DashboardURL: dashboardURL, - Mode: result.Mode, + DashboardURL: dashboardURL, + Mode: result.Mode, + RecipientEmail: notifyEmail, } allOutcomes := make([]exchange.ExchangeOutcome, 0, len(result.Completed)+len(result.Pending)+len(result.Failed)) diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index 8fe51b412..7d0bf902b 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -99,7 +99,7 @@ func TestBuildExchangeNotificationData(t *testing.T) { }, } - data := buildExchangeNotificationData(result, "https://dashboard.example.com") + data := buildExchangeNotificationData(result, "https://dashboard.example.com", "") testutil.AssertEqual(t, "https://dashboard.example.com", data.DashboardURL) testutil.AssertEqual(t, "manual", data.Mode) @@ -121,7 +121,7 @@ func TestBuildExchangeNotificationData(t *testing.T) { func TestBuildExchangeNotificationData_Empty(t *testing.T) { result := &exchange.AutoExchangeResult{Mode: "auto"} - data := buildExchangeNotificationData(result, "https://example.com") + data := buildExchangeNotificationData(result, "https://example.com", "") testutil.AssertEqual(t, "auto", data.Mode) testutil.AssertEqual(t, 0, len(data.Exchanges)) @@ -252,7 +252,7 @@ func TestSendExchangeNotification_NoEmailSender(t *testing.T) { app := &Application{Email: nil} result := &exchange.AutoExchangeResult{Mode: "auto"} // Should not panic when Email is nil - app.sendExchangeNotification(context.Background(), result) + app.sendExchangeNotification(context.Background(), result, "") } func TestSendExchangeNotification_NoResults(t *testing.T) { @@ -267,7 +267,7 @@ func TestSendExchangeNotification_NoResults(t *testing.T) { } result := &exchange.AutoExchangeResult{Mode: "auto"} - app.sendExchangeNotification(context.Background(), result) + app.sendExchangeNotification(context.Background(), result, "") if emailSent { t.Error("expected no email to be sent for empty results") @@ -291,7 +291,7 @@ func TestSendExchangeNotification_ManualPending(t *testing.T) { {SourceRIID: "ri-1"}, }, } - app.sendExchangeNotification(context.Background(), result) + app.sendExchangeNotification(context.Background(), result, "") if !approvalSent { t.Error("expected approval email to be sent") @@ -315,7 +315,7 @@ func TestSendExchangeNotification_AutoCompleted(t *testing.T) { {SourceRIID: "ri-1", ExchangeID: "exch-1"}, }, } - app.sendExchangeNotification(context.Background(), result) + app.sendExchangeNotification(context.Background(), result, "") if !completedSent { t.Error("expected completed email to be sent") @@ -338,7 +338,7 @@ func TestSendExchangeNotification_EmailFailure(t *testing.T) { }, } // Should not panic on email failure — just logs - app.sendExchangeNotification(context.Background(), result) + app.sendExchangeNotification(context.Background(), result, "") } // --- Tests for executeRIExchangeReshape (end-to-end handler tests) --- From 4a9d9fdf31cc10291c48d96f0e0d0db645a698f0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:04:20 -0700 Subject: [PATCH 2/4] fix(email): harden token-broadcast guard and tighten RI-exchange recipient contract Address CodeRabbit pass-1 nitpicks on PR #1036: - sender.go: make the SNS token-broadcast guard case-insensitive so a future template that capitalizes the query param (Token=, TOKEN=) cannot slip a token-bearing body past the guard. Add mixed-case regression cases to TestSendNotification_RejectsTokenBearingBody (fails pre-fix, passes post-fix). - handler_ri_exchange.go: only populate RIExchangeNotificationData.RecipientEmail for the pending-approval branch. The completed/failed path is transport-driven (SNS broadcast / SMTP notifyEmail) and ignores RecipientEmail, so setting it there misrepresented the struct contract. Update the buildExchangeNotificationData doc comment accordingly. - handler_ri_exchange_test.go: add coverage for a configured NotificationEmail, asserting the pending branch receives RecipientEmail populated and the completed branch leaves it empty. - templates_test.go: drop the leftover no-op t.Cleanup(func() {}). --- internal/email/sender.go | 6 +- internal/email/sender_test.go | 5 ++ internal/email/templates_test.go | 1 - internal/server/handler_ri_exchange.go | 16 ++++-- internal/server/handler_ri_exchange_test.go | 63 +++++++++++++++++++++ 5 files changed, 83 insertions(+), 8 deletions(-) diff --git a/internal/email/sender.go b/internal/email/sender.go index 25818649e..14bcb3028 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -108,12 +108,12 @@ func NewSenderWithClients(snsClient SNSPublisher, sesClient SESEmailSender, cfg // SendNotification sends a notification email via SNS. // -// Guard: messages whose body contains "token=" are rejected with -// ErrTokenInBroadcast. Approval tokens must never reach the SNS broadcast +// 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. func (s *Sender) SendNotification(ctx context.Context, subject, message string) error { - if strings.Contains(message, "token=") { + if strings.Contains(strings.ToLower(message), "token=") { return ErrTokenInBroadcast } diff --git a/internal/email/sender_test.go b/internal/email/sender_test.go index 6f1de09b4..e575ceaa2 100644 --- a/internal/email/sender_test.go +++ b/internal/email/sender_test.go @@ -902,6 +902,11 @@ func TestSendNotification_RejectsTokenBearingBody(t *testing.T) { "Review: https://app.example.com/approve?token=abc123", "token=somesecret", "?token=xyz&action=pause", + // Mixed-case variants: the guard is case-insensitive so future + // template changes that capitalize the query param can't slip a + // token-bearing body past the broadcast guard. + "Review: https://app.example.com/approve?Token=abc123", + "?TOKEN=xyz&action=pause", } for _, body := range tokenBodies { err := sender.SendNotification(ctx, "Test", body) diff --git a/internal/email/templates_test.go b/internal/email/templates_test.go index ac2d1b219..1ba02b4a5 100644 --- a/internal/email/templates_test.go +++ b/internal/email/templates_test.go @@ -46,7 +46,6 @@ func TestSender_SendNewRecommendationsNotification_Success(t *testing.T) { func TestSender_SendScheduledPurchaseNotification_Success(t *testing.T) { // Scheduled purchase notifications carry approval tokens and must be // delivered via targeted SES, not the SNS broadcast topic. - t.Cleanup(func() {}) mockSES := new(MockSESClient) mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index 0fa714857..e687d2505 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -152,10 +152,17 @@ func (app *Application) sendExchangeNotification(ctx context.Context, result *ex return } - data := buildExchangeNotificationData(result, app.appConfig.DashboardURL, notifyEmail) + // RecipientEmail is only meaningful for the pending-approval branch: + // SendRIExchangePendingApproval routes token-bearing emails to a specific + // inbox and rejects an empty recipient with ErrNoRecipient. The completed + // path (SendRIExchangeCompleted) ignores RecipientEmail and is + // transport-driven (SNS broadcast / SMTP notifyEmail), so leave it empty + // there to keep the struct contract honest. + data := buildExchangeNotificationData(result, app.appConfig.DashboardURL, "") var err error if result.Mode == "manual" && len(result.Pending) > 0 { + data.RecipientEmail = notifyEmail err = app.Email.SendRIExchangePendingApproval(ctx, data) } else if len(result.Completed)+len(result.Failed) > 0 { err = app.Email.SendRIExchangeCompleted(ctx, data) @@ -171,9 +178,10 @@ func exchangeHasResults(result *exchange.AutoExchangeResult) bool { } // buildExchangeNotificationData converts AutoExchangeResult into email notification data. -// notifyEmail is the global notification address from GlobalConfig; it is set as -// RecipientEmail so token-bearing pending-approval emails route to a specific inbox -// rather than the SNS broadcast topic. +// notifyEmail populates RecipientEmail. Callers should pass it only for the +// pending-approval branch (where token-bearing emails must route to a specific +// inbox rather than the SNS broadcast topic) and pass "" for the completed +// branch, which is transport-driven and ignores RecipientEmail. func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboardURL, notifyEmail string) email.RIExchangeNotificationData { data := email.RIExchangeNotificationData{ DashboardURL: dashboardURL, diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index 7d0bf902b..0b5f0c85c 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -322,6 +322,69 @@ func TestSendExchangeNotification_AutoCompleted(t *testing.T) { } } +// TestSendExchangeNotification_PendingSetsRecipientEmail verifies that a +// configured notification address is routed to the pending-approval branch as +// RecipientEmail, so the token-bearing approval email is delivered to a +// specific inbox rather than the SNS broadcast topic. +func TestSendExchangeNotification_PendingSetsRecipientEmail(t *testing.T) { + var gotRecipient string + approvalSent := false + app := &Application{ + Email: &mockEmailSender{ + sendApprovalFunc: func(ctx context.Context, data email.RIExchangeNotificationData) error { + approvalSent = true + gotRecipient = data.RecipientEmail + return nil + }, + }, + } + + result := &exchange.AutoExchangeResult{ + Mode: "manual", + Pending: []exchange.ExchangeOutcome{ + {SourceRIID: "ri-1"}, + }, + } + app.sendExchangeNotification(context.Background(), result, "admin@example.com") + + if !approvalSent { + t.Fatal("expected approval email to be sent") + } + testutil.AssertEqual(t, "admin@example.com", gotRecipient) +} + +// TestSendExchangeNotification_CompletedOmitsRecipientEmail verifies that the +// completed/failed branch leaves RecipientEmail empty even when a notification +// address is configured: that path is transport-driven (SNS broadcast / SMTP +// notifyEmail) and ignores RecipientEmail, so populating it would misrepresent +// the struct contract. +func TestSendExchangeNotification_CompletedOmitsRecipientEmail(t *testing.T) { + var gotRecipient string + completedSent := false + app := &Application{ + Email: &mockEmailSender{ + sendCompletedFunc: func(ctx context.Context, data email.RIExchangeNotificationData) error { + completedSent = true + gotRecipient = data.RecipientEmail + return nil + }, + }, + } + + result := &exchange.AutoExchangeResult{ + Mode: "auto", + Completed: []exchange.ExchangeOutcome{ + {SourceRIID: "ri-1", ExchangeID: "exch-1"}, + }, + } + app.sendExchangeNotification(context.Background(), result, "admin@example.com") + + if !completedSent { + t.Fatal("expected completed email to be sent") + } + testutil.AssertEqual(t, "", gotRecipient) +} + func TestSendExchangeNotification_EmailFailure(t *testing.T) { app := &Application{ Email: &mockEmailSender{ From 439bb3bd10ef5d7a15b34ef5fe69ddfe54f404e9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:43:14 -0700 Subject: [PATCH 3/4] fix(email): use text/template for plain-text bodies, refuse SMTP auth over non-TLS (07-H1, 07-H2) Plain-text email renderers were using html/template which HTML-encodes special characters in data fields -- "O'Brien" rendered as "O'Brien" and "a&b@x.com" as "a&b@x.com" in recipients' inboxes (07-H1). Add renderTextTemplate backed by text/template and route all non-HTML renderers through it. HTML renderers retain html/template for XSS safety. Regression tests assert that &, ', and < survive verbatim in plain-text bodies and that html/template still escapes ` + data := NotificationData{ + DashboardURL: "https://example.com", + ApprovalToken: "tok", + ExecutionID: "exec-2", + RequestedByName: xssName, + Recommendations: []RecommendationSummary{{Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1}}, + } + html, err := RenderPurchaseApprovalRequestEmailHTML(data) + require.NoError(t, err) + assert.NotContains(t, html, xssName, "html/template must escape