From 43b03b6cb60e558b0199a9416d5cde8d35e2e887 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 21:07:26 +0200 Subject: [PATCH 01/10] feat(notifications): per-recipient mute + List-Unsubscribe (closes #297) Migration 000061 adds muted_recipients table keyed on (email, scope). GET /api/notifications/unsubscribe verifies an HMAC-signed token and upserts the mute row (AuthPublic, mirrors approve/cancel pattern). SendPurchaseApprovalRequest checks the mute table before each send and attaches List-Unsubscribe + List-Unsubscribe-Post headers (RFC 8058) when an unsubscribe base URL is configured. Token signing uses HMAC-SHA256 over (lower(email)|scope) via NOTIFICATION_MUTE_SECRET. --- internal/api/handler_notifications.go | 126 +++++++++ internal/api/handler_notifications_test.go | 207 ++++++++++++++ internal/api/middleware.go | 1 + internal/api/router.go | 8 + internal/config/interfaces.go | 9 + internal/config/store_postgres.go | 37 +++ .../000078_muted_recipients.down.sql | 1 + .../migrations/000078_muted_recipients.up.sql | 20 ++ internal/email/interfaces.go | 7 + internal/email/mute_test.go | 215 ++++++++++++++ internal/email/sender.go | 266 +++++++++++++++--- internal/email/templates.go | 50 +++- pkg/common/tokens.go | 41 +++ 13 files changed, 949 insertions(+), 39 deletions(-) create mode 100644 internal/api/handler_notifications.go create mode 100644 internal/api/handler_notifications_test.go create mode 100644 internal/database/postgres/migrations/000078_muted_recipients.down.sql create mode 100644 internal/database/postgres/migrations/000078_muted_recipients.up.sql create mode 100644 internal/email/mute_test.go diff --git a/internal/api/handler_notifications.go b/internal/api/handler_notifications.go new file mode 100644 index 000000000..ee0eca9d7 --- /dev/null +++ b/internal/api/handler_notifications.go @@ -0,0 +1,126 @@ +package api + +import ( + "context" + "fmt" + "html/template" + "os" + "strings" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" + "github.com/aws/aws-lambda-go/events" +) + +// mutePageCSP is the Content-Security-Policy for the unsubscribe confirmation +// page. The page is intentionally minimal (no external scripts or styles), so +// we can lock it down tightly. +const mutePageCSP = "default-src 'none'; style-src 'unsafe-inline'; frame-ancestors 'none'" + +// unsubscribeConfirmTmpl is the HTML confirmation page rendered after a +// successful one-click unsubscribe. No user-supplied data is interpolated via +// {{.}} without html/template escaping, so there is no XSS vector. +var unsubscribeConfirmTmpl = template.Must(template.New("unsub").Parse(` + + + + +Unsubscribed + + + +

You have been unsubscribed.

+

You will no longer receive {{.ScopeLabel}} emails at this address.

+

This preference is saved. You do not need to click again.

+ + +`)) + +// scopeLabel returns a human-readable label for a notification scope. +func scopeLabel(scope string) string { + switch scope { + case string(common.ScopePurchaseApprovals): + return "purchase approval request" + case string(common.ScopeRIExchangeApprovals): + return "RI exchange approval request" + default: + return "notification" + } +} + +// muteSecretKey loads the NOTIFICATION_MUTE_SECRET env var as bytes. Returns +// nil when unset, which causes DeriveMuteToken to fall back to its dev key. +func muteSecretKey() []byte { + v := os.Getenv("NOTIFICATION_MUTE_SECRET") + if v == "" { + return nil + } + return []byte(v) +} + +// unsubscribeHandler handles GET /api/notifications/unsubscribe. +// The URL carries a signed token that encodes (email, scope); the handler +// verifies the HMAC, upserts the mute row, and returns a confirmation page. +// +// Auth: AuthPublic (token-based, no login required — mirrors approve/cancel). +func (h *Handler) unsubscribeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + token := req.QueryStringParameters["token"] + email := req.QueryStringParameters["email"] + scope := req.QueryStringParameters["scope"] + + if token == "" || email == "" || scope == "" { + return nil, NewClientError(400, "token, email and scope are required") + } + + // Reject unknown scopes early so we never create phantom rows. + validScope := scope == string(common.ScopePurchaseApprovals) || + scope == string(common.ScopeRIExchangeApprovals) + if !validScope { + return nil, NewClientError(400, fmt.Sprintf("unknown notification scope: %s", scope)) + } + + if !common.VerifyMuteToken(muteSecretKey(), email, scope, token) { + logging.Warnf("notifications/unsubscribe: invalid token for scope=%s", scope) + return nil, NewClientError(401, "invalid or expired unsubscribe token") + } + + if err := h.config.UpsertNotificationMute(ctx, email, scope, token); err != nil { + logging.Errorf("notifications/unsubscribe: store error: %v", err) + return nil, fmt.Errorf("could not save unsubscribe preference: %w", err) + } + + logging.Infof("notifications/unsubscribe: muted scope=%s for %s", scope, redactEmailLocal(email)) + + var buf strings.Builder + if err := unsubscribeConfirmTmpl.Execute(&buf, struct{ ScopeLabel string }{ + ScopeLabel: scopeLabel(scope), + }); err != nil { + return nil, fmt.Errorf("render unsubscribe page: %w", err) + } + + return &rawResponse{ + contentType: "text/html; charset=utf-8", + body: buf.String(), + csp: mutePageCSP, + }, nil +} + +// redactEmailLocal returns just the domain part with the local masked, e.g. +// "us***@example.com". Reuses the same masking logic as email/sender.go but +// without importing that package into api (avoids a dependency cycle). +func redactEmailLocal(email string) string { + at := strings.LastIndex(email, "@") + if at < 0 { + return "***" + } + local := email[:at] + domain := email[at:] // includes '@' + if len(local) <= 2 { + return "***" + domain + } + return local[:2] + "***" + domain +} diff --git a/internal/api/handler_notifications_test.go b/internal/api/handler_notifications_test.go new file mode 100644 index 000000000..d9fef11ab --- /dev/null +++ b/internal/api/handler_notifications_test.go @@ -0,0 +1,207 @@ +package api + +import ( + "context" + "strings" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// GET /api/notifications/unsubscribe +// --------------------------------------------------------------------------- + +func validUnsubToken(email, scope string) string { + return common.DeriveMuteToken(nil, email, scope) +} + +func TestUnsubscribeHandler_Success(t *testing.T) { + ctx := context.Background() + email := "user@example.com" + scope := string(common.ScopePurchaseApprovals) + token := validUnsubToken(email, scope) + + mockStore := new(MockConfigStore) + mockStore.On("UpsertNotificationMute", ctx, email, scope, token).Return(nil) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + h := &Handler{config: mockStore} + r := newTestRouter(h) + + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{ + "token": token, + "email": email, + "scope": scope, + }, + } + result, err := r.unsubscribeHandler(ctx, req, nil) + require.NoError(t, err) + raw, ok := result.(*rawResponse) + require.True(t, ok, "expected *rawResponse") + assert.Equal(t, "text/html; charset=utf-8", raw.contentType) + assert.Contains(t, raw.body, "Unsubscribed") + assert.Contains(t, raw.body, "purchase approval request") +} + +func TestUnsubscribeHandler_ForgedToken_Returns401(t *testing.T) { + ctx := context.Background() + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{ + "token": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "email": "attacker@example.com", + "scope": string(common.ScopePurchaseApprovals), + }, + } + h := &Handler{config: new(MockConfigStore)} + r := newTestRouter(h) + + _, err := r.unsubscribeHandler(ctx, req, nil) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 401, ce.code) +} + +func TestUnsubscribeHandler_MissingParams_Returns400(t *testing.T) { + ctx := context.Background() + h := &Handler{config: new(MockConfigStore)} + r := newTestRouter(h) + + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{ + "token": "something", + // email and scope missing + }, + } + _, err := r.unsubscribeHandler(ctx, req, nil) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} + +func TestUnsubscribeHandler_UnknownScope_Returns400(t *testing.T) { + ctx := context.Background() + email := "user@example.com" + scope := "unknown_scope" + // Use the dev-key token so HMAC passes but scope guard fires first. + token := common.DeriveMuteToken(nil, email, scope) + + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{ + "token": token, + "email": email, + "scope": scope, + }, + } + h := &Handler{config: new(MockConfigStore)} + r := newTestRouter(h) + + _, err := r.unsubscribeHandler(ctx, req, nil) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} + +func TestUnsubscribeHandler_StoreError_Returns500(t *testing.T) { + ctx := context.Background() + email := "user@example.com" + scope := string(common.ScopePurchaseApprovals) + token := validUnsubToken(email, scope) + + mockStore := new(MockConfigStore) + mockStore.On("UpsertNotificationMute", mock.Anything, email, scope, token). + Return(assert.AnError) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + h := &Handler{config: mockStore} + r := newTestRouter(h) + + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{ + "token": token, + "email": email, + "scope": scope, + }, + } + _, err := r.unsubscribeHandler(ctx, req, nil) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "store error should be a 500, not a client error") +} + +// --------------------------------------------------------------------------- +// scopeLabel helper +// --------------------------------------------------------------------------- + +func TestScopeLabel_KnownScopes(t *testing.T) { + assert.Equal(t, "purchase approval request", scopeLabel(string(common.ScopePurchaseApprovals))) + assert.Equal(t, "RI exchange approval request", scopeLabel(string(common.ScopeRIExchangeApprovals))) + assert.Equal(t, "notification", scopeLabel("bogus")) +} + +// --------------------------------------------------------------------------- +// redactEmailLocal +// --------------------------------------------------------------------------- + +func TestRedactEmailLocal(t *testing.T) { + cases := []struct { + in, want string + }{ + {"user@example.com", "us***@example.com"}, + {"ab@x.com", "***@x.com"}, + {"a@x.com", "***@x.com"}, + {"noemail", "***"}, + } + for _, c := range cases { + assert.Equal(t, c.want, redactEmailLocal(c.in), "input: %s", c.in) + } +} + +// --------------------------------------------------------------------------- +// isPublicEndpoint includes /api/notifications/unsubscribe +// --------------------------------------------------------------------------- + +func TestIsPublicEndpoint_UnsubscribePath(t *testing.T) { + h := &Handler{} + assert.True(t, h.isPublicEndpoint("/api/notifications/unsubscribe")) + assert.True(t, h.isPublicEndpoint("/api/notifications/unsubscribe?token=x&email=y&scope=z")) +} + +// --------------------------------------------------------------------------- +// Route is registered (router smoke test) +// --------------------------------------------------------------------------- + +func TestRouter_UnsubscribeRoute_Registered(t *testing.T) { + // Verify the route is wired: an unsigned token returns 401, which can only + // happen if the router dispatched to the correct handler. + ctx := context.Background() + h := &Handler{config: new(MockConfigStore)} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "GET", + Path: "/api/notifications/unsubscribe", + }, + }, + QueryStringParameters: map[string]string{ + "token": strings.Repeat("a", 64), + "email": "user@example.com", + "scope": string(common.ScopePurchaseApprovals), + }, + } + _, err := r.Route(ctx, "GET", "/api/notifications/unsubscribe", req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 401, ce.code) +} diff --git a/internal/api/middleware.go b/internal/api/middleware.go index ee28a9d3a..b170d9003 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -31,6 +31,7 @@ func (h *Handler) isPublicEndpoint(path string) bool { "/api/auth/forgot-password", "/api/auth/reset-password", "/api/register/", // GET /api/register/:token (trailing slash avoids matching /api/registrations) + "/api/notifications/unsubscribe", "/docs", "/api/docs", } diff --git a/internal/api/router.go b/internal/api/router.go index 81e5c5ffc..47f538b76 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -320,6 +320,10 @@ func (r *Router) registerRoutes() { {ExactPath: "/api/ladder/configs", Method: "GET", Handler: r.getLadderConfigsHandler, Auth: AuthUser}, {ExactPath: "/api/ladder/configs", Method: "PUT", Handler: r.upsertLadderConfigHandler, Auth: AuthUser}, + // Notification one-click unsubscribe (RFC 8058). AuthPublic: the signed + // token in the query string is the credential (mirrors approve/cancel). + {ExactPath: "/api/notifications/unsubscribe", Method: "GET", Handler: r.unsubscribeHandler, Auth: AuthPublic}, + // Account self-registration (public, called by Terraform during federation IaC apply) {ExactPath: "/api/register", Method: "POST", Handler: r.submitRegistrationHandler, Auth: AuthPublic}, {PathPrefix: "/api/register/", Method: "GET", Handler: r.getRegistrationStatusHandler, Auth: AuthPublic}, @@ -962,3 +966,7 @@ func (r *Router) getLadderConfigsHandler(ctx context.Context, req *events.Lambda func (r *Router) upsertLadderConfigHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { return r.h.upsertLadderConfig(ctx, req) } + +func (r *Router) unsubscribeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.unsubscribeHandler(ctx, req, params) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index f2f726336..54d30aaab 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -436,4 +436,13 @@ type StoreInterface interface { SaveLadderTranches(ctx context.Context, tranches []LadderTrancheDB) error LatestLadderRunStartedAt(ctx context.Context, configID string) (*time.Time, error) TransitionLadderRunStatus(ctx context.Context, id string, fromStatuses []ladder.RunStatus, toStatus ladder.RunStatus) (*LadderRunDB, error) + + // Notification mutes (issue #297 / migration 000078). + // UpsertNotificationMute inserts or updates a mute row for (email, scope). + // Idempotent: calling it again for an already-muted address is a no-op on + // muted_at but does replace unmute_token if the token changes. + UpsertNotificationMute(ctx context.Context, recipientEmail, scope, unmuteToken string) error + // IsNotificationMuted returns true when (email, scope) has a row in + // muted_recipients. The email comparison is case-insensitive. + IsNotificationMuted(ctx context.Context, recipientEmail, scope string) (bool, error) } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index d7cbfbfd8..7412b0353 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3457,3 +3457,40 @@ func nullPtrFromNullString(ns sql.NullString) *string { s := ns.String return &s } + +// ========================================== +// NOTIFICATION MUTES (issue #297) +// ========================================== + +// UpsertNotificationMute inserts or replaces the mute row for (recipientEmail, +// scope). ON CONFLICT updates muted_at and unmute_token so a repeated +// one-click opt-out resets the audit timestamp without error. +func (s *PostgresStore) UpsertNotificationMute(ctx context.Context, recipientEmail, scope, unmuteToken string) error { + _, err := s.db.Exec(ctx, ` + INSERT INTO muted_recipients (recipient_email, scope, muted_at, unmute_token) + VALUES (LOWER($1), $2, NOW(), $3) + ON CONFLICT (recipient_email, scope) + DO UPDATE SET muted_at = NOW(), unmute_token = EXCLUDED.unmute_token + `, recipientEmail, scope, unmuteToken) + if err != nil { + return fmt.Errorf("upsert notification mute: %w", err) + } + return nil +} + +// IsNotificationMuted returns true when (email, scope) has a matching row +// in muted_recipients. The email lookup is case-insensitive (LOWER on insert +// plus LOWER($1) here). +func (s *PostgresStore) IsNotificationMuted(ctx context.Context, recipientEmail, scope string) (bool, error) { + var exists bool + err := s.db.QueryRow(ctx, ` + SELECT EXISTS( + SELECT 1 FROM muted_recipients + WHERE recipient_email = LOWER($1) AND scope = $2 + ) + `, recipientEmail, scope).Scan(&exists) + if err != nil { + return false, fmt.Errorf("check notification mute: %w", err) + } + return exists, nil +} diff --git a/internal/database/postgres/migrations/000078_muted_recipients.down.sql b/internal/database/postgres/migrations/000078_muted_recipients.down.sql new file mode 100644 index 000000000..2c582be69 --- /dev/null +++ b/internal/database/postgres/migrations/000078_muted_recipients.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS muted_recipients; diff --git a/internal/database/postgres/migrations/000078_muted_recipients.up.sql b/internal/database/postgres/migrations/000078_muted_recipients.up.sql new file mode 100644 index 000000000..41d398a07 --- /dev/null +++ b/internal/database/postgres/migrations/000078_muted_recipients.up.sql @@ -0,0 +1,20 @@ +-- Per-recipient notification mute table (issue #297). +-- +-- A row here means the named recipient has opted out of a notification +-- scope via the List-Unsubscribe one-click link. The send path consults +-- this table before adding any address to To/Cc; muted addresses are +-- silently skipped. The mute is per-scope so opting out of +-- "purchase_approvals" does NOT suppress "ri_exchange_approvals". +-- +-- unmute_token is the HMAC-signed token embedded in the unsubscribe URL +-- and is stored here only for auditability / idempotency (the handler +-- derives the same token on every request and compares in constant time; +-- storing it does NOT make the endpoint stateful in the sense of +-- single-use tokens). +CREATE TABLE IF NOT EXISTS muted_recipients ( + recipient_email TEXT NOT NULL, + scope TEXT NOT NULL, + muted_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + unmute_token TEXT NOT NULL, + CONSTRAINT muted_recipients_pkey PRIMARY KEY (recipient_email, scope) +); diff --git a/internal/email/interfaces.go b/internal/email/interfaces.go index 2fb1eae3e..08438fdb8 100644 --- a/internal/email/interfaces.go +++ b/internal/email/interfaces.go @@ -54,6 +54,13 @@ type SESEmailSender interface { CreateEmailIdentity(ctx context.Context, params *sesv2.CreateEmailIdentityInput, optFns ...func(*sesv2.Options)) (*sesv2.CreateEmailIdentityOutput, error) } +// MuteChecker is a narrow interface the send path uses to consult the +// muted_recipients table. Isolating it from the full config.StoreInterface +// keeps the email package free of a direct dependency on the config package. +type MuteChecker interface { + IsNotificationMuted(ctx context.Context, recipientEmail, scope string) (bool, error) +} + // Ensure concrete types implement interfaces. var _ SNSPublisher = (*sns.Client)(nil) var _ SESEmailSender = (*sesv2.Client)(nil) diff --git a/internal/email/mute_test.go b/internal/email/mute_test.go new file mode 100644 index 000000000..3f7cd28ef --- /dev/null +++ b/internal/email/mute_test.go @@ -0,0 +1,215 @@ +package email + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/aws/aws-sdk-go-v2/service/sesv2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// Stub MuteChecker +// --------------------------------------------------------------------------- + +type mockMuteChecker struct { + mock.Mock +} + +func (m *mockMuteChecker) IsNotificationMuted(ctx context.Context, email, scope string) (bool, error) { + args := m.Called(ctx, email, scope) + return args.Bool(0), args.Error(1) +} + +// --------------------------------------------------------------------------- +// SendPurchaseApprovalRequest + mute check +// --------------------------------------------------------------------------- + +// newSenderWithMute builds a testable *Sender with a mock SES client and a +// mock MuteChecker, bypassing sandbox checks by having GetAccount return +// production mode. +func newSenderWithMute(ses *MockSESClient, mc *mockMuteChecker) *Sender { + // GetAccount returning ProductionAccessEnabled=true means no sandbox path. + ses.On("GetAccount", mock.Anything, mock.Anything). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil).Maybe() + return &Sender{ + sesClient: ses, + fromEmail: "noreply@example.com", + muteChecker: mc, + } +} + +func TestSendPurchaseApprovalRequest_MutedRecipient_NoSESCall(t *testing.T) { + ctx := context.Background() + ses := new(MockSESClient) + mc := new(mockMuteChecker) + + mc.On("IsNotificationMuted", mock.Anything, "approver@example.com", string(common.ScopePurchaseApprovals)). + Return(true, nil) + t.Cleanup(func() { + mc.AssertExpectations(t) + ses.AssertNotCalled(t, "SendEmail") + }) + + s := newSenderWithMute(ses, mc) + + data := NotificationData{ + RecipientEmail: "approver@example.com", + Recommendations: []RecommendationSummary{ + {Service: "ec2", Region: "us-east-1", Count: 1, MonthlySavings: 100}, + }, + DashboardURL: "https://dashboard.example.com", + ApprovalToken: "tok", + } + err := s.SendPurchaseApprovalRequest(ctx, data) + require.NoError(t, err) +} + +func TestSendPurchaseApprovalRequest_NotMuted_SendsEmail(t *testing.T) { + ctx := context.Background() + ses := new(MockSESClient) + mc := new(mockMuteChecker) + + mc.On("IsNotificationMuted", mock.Anything, "approver@example.com", string(common.ScopePurchaseApprovals)). + Return(false, nil) + mc.On("IsNotificationMuted", mock.Anything, mock.Anything, mock.Anything). + Return(false, nil).Maybe() // for CC filter if any + ses.On("GetAccount", mock.Anything, mock.Anything). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) + ses.On("SendEmail", mock.Anything, mock.Anything). + Return(&sesv2.SendEmailOutput{}, nil) + t.Cleanup(func() { + mc.AssertExpectations(t) + ses.AssertExpectations(t) + }) + + s := newSenderWithMute(ses, mc) + data := NotificationData{ + RecipientEmail: "approver@example.com", + Recommendations: []RecommendationSummary{ + {Service: "ec2", Region: "us-east-1", Count: 1, MonthlySavings: 100}, + }, + DashboardURL: "https://dashboard.example.com", + ApprovalToken: "tok", + } + err := s.SendPurchaseApprovalRequest(ctx, data) + require.NoError(t, err) + ses.AssertCalled(t, "SendEmail", mock.Anything, mock.Anything) +} + +func TestSendPurchaseApprovalRequest_MuteCheckError_FailOpen(t *testing.T) { + // When the mute store returns an error, the email is still sent (fail-open + // so a DB hiccup doesn't permanently block approval notifications). + ctx := context.Background() + ses := new(MockSESClient) + mc := new(mockMuteChecker) + + mc.On("IsNotificationMuted", mock.Anything, "approver@example.com", string(common.ScopePurchaseApprovals)). + Return(false, errors.New("db error")) + mc.On("IsNotificationMuted", mock.Anything, mock.Anything, mock.Anything). + Return(false, nil).Maybe() + ses.On("GetAccount", mock.Anything, mock.Anything). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) + ses.On("SendEmail", mock.Anything, mock.Anything). + Return(&sesv2.SendEmailOutput{}, nil) + t.Cleanup(func() { + ses.AssertCalled(t, "SendEmail", mock.Anything, mock.Anything) + }) + + s := newSenderWithMute(ses, mc) + data := NotificationData{ + RecipientEmail: "approver@example.com", + Recommendations: []RecommendationSummary{ + {Service: "ec2", Region: "us-east-1", Count: 1, MonthlySavings: 100}, + }, + DashboardURL: "https://dashboard.example.com", + ApprovalToken: "tok", + } + err := s.SendPurchaseApprovalRequest(ctx, data) + require.NoError(t, err) +} + +// --------------------------------------------------------------------------- +// List-Unsubscribe header injection +// --------------------------------------------------------------------------- + +func TestBuildUnsubscribeURL_EmptyBaseURL_ReturnsEmpty(t *testing.T) { + s := &Sender{} + u, _ := s.buildUnsubscribeURL("user@example.com", "purchase_approvals") + assert.Empty(t, u) +} + +func TestBuildUnsubscribeURL_WithBaseURL_ContainsParams(t *testing.T) { + s := &Sender{unsubscribeBaseURL: "https://dash.example.com"} + u, _ := s.buildUnsubscribeURL("user@example.com", "purchase_approvals") + assert.Contains(t, u, "email=user%40example.com") + assert.Contains(t, u, "scope=purchase_approvals") + assert.Contains(t, u, "token=") +} + +func TestListUnsubscribeHeaders_EmptyBase_ReturnsEmpty(t *testing.T) { + s := &Sender{} + hdr, post := s.listUnsubscribeHeaders("u@e.com", "purchase_approvals") + assert.Empty(t, hdr) + assert.Empty(t, post) +} + +func TestListUnsubscribeHeaders_WithBase(t *testing.T) { + s := &Sender{unsubscribeBaseURL: "https://dash.example.com"} + hdr, post := s.listUnsubscribeHeaders("u@e.com", "purchase_approvals") + assert.Contains(t, hdr, "", "List-Unsubscribe=One-Click") + require.Len(t, hdrs, 2) + assert.Equal(t, "List-Unsubscribe", *hdrs[0].Name) + assert.Equal(t, "List-Unsubscribe-Post", *hdrs[1].Name) + assert.Equal(t, "List-Unsubscribe=One-Click", *hdrs[1].Value) +} + +// --------------------------------------------------------------------------- +// DeriveMuteToken / VerifyMuteToken +// --------------------------------------------------------------------------- + +func TestDeriveMuteToken_Stable(t *testing.T) { + key := []byte("test-secret") + t1 := common.DeriveMuteToken(key, "user@example.com", "purchase_approvals") + t2 := common.DeriveMuteToken(key, "user@example.com", "purchase_approvals") + assert.Equal(t, t1, t2) +} + +func TestDeriveMuteToken_CaseInsensitive(t *testing.T) { + key := []byte("test-secret") + lower := common.DeriveMuteToken(key, "user@example.com", "purchase_approvals") + upper := common.DeriveMuteToken(key, "USER@EXAMPLE.COM", "purchase_approvals") + assert.Equal(t, lower, upper, "token must be case-insensitive on email") +} + +func TestDeriveMuteToken_DiffersByScope(t *testing.T) { + key := []byte("test-secret") + t1 := common.DeriveMuteToken(key, "user@example.com", "purchase_approvals") + t2 := common.DeriveMuteToken(key, "user@example.com", "ri_exchange_approvals") + assert.NotEqual(t, t1, t2) +} + +func TestVerifyMuteToken_Valid(t *testing.T) { + key := []byte("test-secret") + tok := common.DeriveMuteToken(key, "user@example.com", "purchase_approvals") + assert.True(t, common.VerifyMuteToken(key, "user@example.com", "purchase_approvals", tok)) +} + +func TestVerifyMuteToken_Forged(t *testing.T) { + key := []byte("test-secret") + assert.False(t, common.VerifyMuteToken(key, "user@example.com", "purchase_approvals", "forgedtoken")) +} diff --git a/internal/email/sender.go b/internal/email/sender.go index 07bfbd854..a453f67ad 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -5,8 +5,11 @@ import ( "context" "errors" "fmt" + "net/url" + "os" "strings" + "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -49,6 +52,12 @@ type Sender struct { topicARN string fromEmail string emailAddress string + // muteChecker consults the muted_recipients table before each send. + // Nil disables mute checking (e.g. when no DB is wired in tests). + muteChecker MuteChecker + // unsubscribeBaseURL is the dashboard base URL used to construct the + // List-Unsubscribe header value. Empty disables the header. + unsubscribeBaseURL string } // NewSender creates a new email sender with default context. @@ -103,6 +112,93 @@ func NewSenderWithClients(snsClient SNSPublisher, sesClient SESEmailSender, cfg } } +// WithMuteChecker returns a shallow copy of s with the given MuteChecker wired +// in. Callers that have a DB-backed store use this to enable per-recipient mute +// suppression on outbound SES sends. +func (s *Sender) WithMuteChecker(mc MuteChecker) *Sender { + c := *s + c.muteChecker = mc + return &c +} + +// WithUnsubscribeBaseURL returns a shallow copy of s with the given base URL +// set. When non-empty the sender appends List-Unsubscribe / List-Unsubscribe-Post +// headers (RFC 8058) to outbound SES messages for applicable scopes. +func (s *Sender) WithUnsubscribeBaseURL(u string) *Sender { + c := *s + c.unsubscribeBaseURL = u + return &c +} + +// muteKey reads NOTIFICATION_MUTE_SECRET from env for token derivation. Returns +// nil when unset so DeriveMuteToken uses the dev fallback. +func muteKey() []byte { + v := os.Getenv("NOTIFICATION_MUTE_SECRET") + if v == "" { + return nil + } + return []byte(v) +} + +// buildUnsubscribeURL constructs the one-click unsubscribe URL for the given +// (email, scope) pair. Returns ("", "") when unsubscribeBaseURL is empty. +func (s *Sender) buildUnsubscribeURL(email, scope string) (unsubURL, mailtoURL string) { + if s.unsubscribeBaseURL == "" { + return "", "" + } + token := common.DeriveMuteToken(muteKey(), email, scope) + q := url.Values{ + "token": {token}, + "email": {email}, + "scope": {scope}, + } + unsubURL = s.unsubscribeBaseURL + "/api/notifications/unsubscribe?" + q.Encode() + return unsubURL, "" +} + +// listUnsubscribeHeaders returns the List-Unsubscribe and List-Unsubscribe-Post +// header values for the given (email, scope) pair (RFC 8058). +// Returns ("", "") when no base URL is configured. +func (s *Sender) listUnsubscribeHeaders(email, scope string) (headerValue, postValue string) { + unsubURL, _ := s.buildUnsubscribeURL(email, scope) + if unsubURL == "" { + return "", "" + } + return "<" + unsubURL + ">", "List-Unsubscribe=One-Click" +} + +// isMuted returns true when the given address is muted for this scope. When the +// mute checker is nil or returns an error the address is treated as not muted so +// a transient DB outage doesn't silently block approval emails. +func (s *Sender) isMuted(ctx context.Context, email, scope string) bool { + if s.muteChecker == nil { + return false + } + muted, err := s.muteChecker.IsNotificationMuted(ctx, email, scope) + if err != nil { + logging.Warnf("email: mute check failed for scope=%s: %v", scope, err) + return false + } + return muted +} + +// filterMutedAddresses returns a copy of addrs with any muted (for scope) +// entries removed. The original slice is not modified. Errors from the mute +// store are treated as "not muted" (fail-open) so a DB hiccup does not +// silently suppress approval emails. +func (s *Sender) filterMutedAddresses(ctx context.Context, addrs []string, scope string) []string { + if s.muteChecker == nil || len(addrs) == 0 { + return addrs + } + out := make([]string, 0, len(addrs)) + for _, addr := range addrs { + if !s.isMuted(ctx, addr, scope) { + out = append(out, addr) + } + } + return out +} + // snsMaxSubjectLen is the maximum byte length SNS accepts for a Subject. // Subjects longer than 100 bytes are rejected with InvalidParameter at runtime. const snsMaxSubjectLen = 100 @@ -230,7 +326,7 @@ func (s *Sender) SendToEmailWithCCMultipart(ctx context.Context, toEmail string, cc := dedupeCCAgainstTo(toEmail, ccEmails) - input := buildSESSendEmailInputMultipart(s.fromEmail, toEmail, cc, subject, textBody, htmlBody) + input := buildSESSendEmailInputMultipart(s.fromEmail, toEmail, cc, subject, textBody, htmlBody, nil) _, err := s.sesClient.SendEmail(ctx, input) if err != nil { @@ -265,7 +361,7 @@ func (s *Sender) SendToEmailWithCC(ctx context.Context, toEmail string, ccEmails cc := dedupeCCAgainstTo(toEmail, ccEmails) - input := buildSESSendEmailInput(s.fromEmail, toEmail, cc, subject, body) + input := buildSESSendEmailInput(s.fromEmail, toEmail, cc, subject, body, nil) _, err := s.sesClient.SendEmail(ctx, input) if err != nil { @@ -323,74 +419,168 @@ func (s *Sender) ensureSandboxRecipientVerified(ctx context.Context, toEmail str // buildSESSendEmailInputMultipart constructs a sesv2.SendEmailInput with // both a plain-text and an HTML alternative body. SES handles the // multipart/alternative MIME assembly server-side when both Text and Html -// fields are populated on types.Body. +// fields are populated on types.Body. extraHeaders are appended as-is; use +// addListUnsubscribeHeaders to build the RFC 8058 pair. // // Header injection note (07-M1): SES v2 SendEmail accepts structured fields // (Subject.Data, Body.Text.Data, Body.Html.Data) and builds the MIME envelope // server-side. CR/LF injection via Subject.Data is not possible through the // structured API. Any future raw-MIME path MUST sanitize the subject with // sanitizeHeader before composing the header string, matching the SMTP path. -func buildSESSendEmailInputMultipart(fromEmail, toEmail string, cc []string, subject, textBody, htmlBody string) *sesv2.SendEmailInput { +func buildSESSendEmailInputMultipart(fromEmail, toEmail string, cc []string, subject, textBody, htmlBody string, extraHeaders []types.MessageHeader) *sesv2.SendEmailInput { destination := &types.Destination{ ToAddresses: []string{toEmail}, } if len(cc) > 0 { destination.CcAddresses = cc } - return &sesv2.SendEmailInput{ - Destination: destination, - Content: &types.EmailContent{ - Simple: &types.Message{ - Subject: &types.Content{ - Charset: aws.String("UTF-8"), - Data: aws.String(subject), - }, - Body: &types.Body{ - Text: &types.Content{ - Charset: aws.String("UTF-8"), - Data: aws.String(textBody), - }, - Html: &types.Content{ - Charset: aws.String("UTF-8"), - Data: aws.String(htmlBody), - }, - }, + msg := &types.Message{ + Subject: &types.Content{ + Charset: aws.String("UTF-8"), + Data: aws.String(subject), + }, + Body: &types.Body{ + Text: &types.Content{ + Charset: aws.String("UTF-8"), + Data: aws.String(textBody), + }, + Html: &types.Content{ + Charset: aws.String("UTF-8"), + Data: aws.String(htmlBody), }, }, + } + if len(extraHeaders) > 0 { + msg.Headers = extraHeaders + } + return &sesv2.SendEmailInput{ + Destination: destination, + Content: &types.EmailContent{Simple: msg}, FromEmailAddress: aws.String(fromEmail), } } // buildSESSendEmailInput constructs a sesv2.SendEmailInput with the -// destination To + (optional) Cc list and a plain-text body. +// destination To + (optional) Cc list and a plain-text body. extraHeaders are +// appended as-is; use addListUnsubscribeHeaders to build the RFC 8058 pair. // See buildSESSendEmailInputMultipart for the header-injection safety note (07-M1). -func buildSESSendEmailInput(fromEmail, toEmail string, cc []string, subject, body string) *sesv2.SendEmailInput { +func buildSESSendEmailInput(fromEmail, toEmail string, cc []string, subject, body string, extraHeaders []types.MessageHeader) *sesv2.SendEmailInput { destination := &types.Destination{ ToAddresses: []string{toEmail}, } if len(cc) > 0 { destination.CcAddresses = cc } - return &sesv2.SendEmailInput{ - Destination: destination, - Content: &types.EmailContent{ - Simple: &types.Message{ - Subject: &types.Content{ - Charset: aws.String("UTF-8"), - Data: aws.String(subject), - }, - Body: &types.Body{ - Text: &types.Content{ - Charset: aws.String("UTF-8"), - Data: aws.String(body), - }, - }, + msg := &types.Message{ + Subject: &types.Content{ + Charset: aws.String("UTF-8"), + Data: aws.String(subject), + }, + Body: &types.Body{ + Text: &types.Content{ + Charset: aws.String("UTF-8"), + Data: aws.String(body), }, }, + } + if len(extraHeaders) > 0 { + msg.Headers = extraHeaders + } + return &sesv2.SendEmailInput{ + Destination: destination, + Content: &types.EmailContent{Simple: msg}, FromEmailAddress: aws.String(fromEmail), } } +// addListUnsubscribeHeaders returns the RFC 8058 List-Unsubscribe pair as +// sesv2 MessageHeader values. Returns nil when headerValue is empty. +func addListUnsubscribeHeaders(headerValue, postValue string) []types.MessageHeader { + if headerValue == "" { + return nil + } + hdrs := []types.MessageHeader{ + {Name: aws.String("List-Unsubscribe"), Value: aws.String(headerValue)}, + } + if postValue != "" { + hdrs = append(hdrs, types.MessageHeader{ + Name: aws.String("List-Unsubscribe-Post"), + Value: aws.String(postValue), + }) + } + return hdrs +} + +// sendToEmailWithCCMultipartHeaders is the internal variant of +// SendToEmailWithCCMultipart that also accepts custom message headers (e.g. +// List-Unsubscribe). It is used by the mute-aware send path in +// SendPurchaseApprovalRequest so we don't expose a wider public API. +func (s *Sender) sendToEmailWithCCMultipartHeaders( + ctx context.Context, + toEmail string, + ccEmails []string, + subject, textBody, htmlBody string, + extraHeaders []types.MessageHeader, +) error { + if htmlBody == "" { + // Degrade to plain text; headers still carried via the non-multipart path. + return s.sendToEmailWithCCHeaders(ctx, toEmail, ccEmails, subject, textBody, extraHeaders) + } + if s.fromEmail == "" { + logging.Debug("No from email configured, skipping direct email") + return nil + } + if s.sesClient == nil { + return fmt.Errorf("SES client not initialized") + } + if err := s.ensureSandboxRecipientVerified(ctx, toEmail); err != nil { + return err + } + cc := dedupeCCAgainstTo(toEmail, ccEmails) + input := buildSESSendEmailInputMultipart(s.fromEmail, toEmail, cc, subject, textBody, htmlBody, extraHeaders) + if _, err := s.sesClient.SendEmail(ctx, input); err != nil { + return fmt.Errorf("failed to send email via SES: %w", err) + } + if len(cc) > 0 { + logging.Debugf("Sent multipart email to %s (cc %d): %s", redactEmail(toEmail), len(cc), subject) + } else { + logging.Debugf("Sent multipart email to %s: %s", redactEmail(toEmail), subject) + } + return nil +} + +// sendToEmailWithCCHeaders is the plain-text variant of +// sendToEmailWithCCMultipartHeaders. +func (s *Sender) sendToEmailWithCCHeaders( + ctx context.Context, + toEmail string, + ccEmails []string, + subject, body string, + extraHeaders []types.MessageHeader, +) error { + if s.fromEmail == "" { + logging.Debug("No from email configured, skipping direct email") + return nil + } + if s.sesClient == nil { + return fmt.Errorf("SES client not initialized") + } + if err := s.ensureSandboxRecipientVerified(ctx, toEmail); err != nil { + return err + } + cc := dedupeCCAgainstTo(toEmail, ccEmails) + input := buildSESSendEmailInput(s.fromEmail, toEmail, cc, subject, body, extraHeaders) + if _, err := s.sesClient.SendEmail(ctx, input); err != nil { + return fmt.Errorf("failed to send email via SES: %w", err) + } + if len(cc) > 0 { + logging.Debugf("Sent email to %s (cc %d): %s", redactEmail(toEmail), len(cc), subject) + } else { + logging.Debugf("Sent email to %s: %s", redactEmail(toEmail), subject) + } + return nil +} + // dedupeCCAgainstTo returns cc with the to-address removed (case-insensitive) // and duplicate entries collapsed, preserving input order. Empty strings are // dropped so a caller can freely pass optional slots without sanitizing. diff --git a/internal/email/templates.go b/internal/email/templates.go index a782246e8..1e158f856 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -4,7 +4,9 @@ import ( "context" "fmt" + "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" + "github.com/aws/aws-sdk-go-v2/service/sesv2/types" ) // Email templates @@ -812,6 +814,11 @@ func sendMultipartVia( // ErrNoRecipient when data.RecipientEmail is empty and ErrNoFromEmail when // FROM_EMAIL is unconfigured, so the caller can surface a precise reason in // the API response instead of the prior silent no-op. +// +// Mute check: if the recipient has opted out of purchase_approvals via the +// List-Unsubscribe link, the email is silently skipped and nil is returned. +// A List-Unsubscribe / List-Unsubscribe-Post header pair (RFC 8058) is added +// to the outbound SES message when an unsubscribe base URL is configured. func (s *Sender) SendPurchaseApprovalRequest(ctx context.Context, data NotificationData) error { if data.RecipientEmail == "" { return ErrNoRecipient @@ -824,8 +831,49 @@ func (s *Sender) SendPurchaseApprovalRequest(ctx context.Context, data Notificat if !isValidFromEmail(s.fromEmail) { return ErrNoFromEmail } + + scope := string(common.ScopePurchaseApprovals) + + // Per-recipient mute check: skip silently if the approver has opted out. + if s.isMuted(ctx, data.RecipientEmail, scope) { + logging.Infof("email: purchase approval skipped for muted recipient (scope=%s)", scope) + return nil + } + + // Filter CC list against mutes so no muted address receives a copy. + filteredCC := s.filterMutedAddresses(ctx, data.CCEmails, scope) + + // Build RFC 8058 List-Unsubscribe headers scoped to the primary recipient. + unsubHdr, postHdr := s.listUnsubscribeHeaders(data.RecipientEmail, scope) + extraHeaders := addListUnsubscribeHeaders(unsubHdr, postHdr) + subject := fmt.Sprintf("CUDly - Purchase Approval Required (%d commitment(s))", len(data.Recommendations)) - return sendPurchaseApprovalRequestVia(ctx, s, data.RecipientEmail, subject, data) + return sendPurchaseApprovalRequestWithCC(ctx, s, data.RecipientEmail, filteredCC, subject, data, extraHeaders) +} + +// sendPurchaseApprovalRequestWithCC is the low-level send helper that accepts +// a pre-filtered CC list and extra message headers. Extracted from +// sendPurchaseApprovalRequestVia so the mute/unsub path can inject headers +// without duplicating the render logic. +func sendPurchaseApprovalRequestWithCC( + ctx context.Context, + s *Sender, + recipient string, + ccEmails []string, + subject string, + data NotificationData, + extraHeaders []types.MessageHeader, +) error { + textBody, err := RenderPurchaseApprovalRequestEmail(data) + if err != nil { + return fmt.Errorf("failed to render purchase approval request email (text): %w", err) + } + htmlBody, htmlErr := RenderPurchaseApprovalRequestEmailHTML(data) + if htmlErr != nil { + logging.Warnf("email: HTML approval-request render failed, falling back to text-only: %v", htmlErr) + htmlBody = "" + } + return s.sendToEmailWithCCMultipartHeaders(ctx, recipient, ccEmails, subject, textBody, htmlBody, extraHeaders) } // --------------------------------------------------------------------------- diff --git a/pkg/common/tokens.go b/pkg/common/tokens.go index 28ded0faf..33d158bb8 100644 --- a/pkg/common/tokens.go +++ b/pkg/common/tokens.go @@ -1,6 +1,7 @@ package common import ( + "crypto/hmac" "crypto/rand" "crypto/sha256" "encoding/hex" @@ -95,3 +96,43 @@ func ReservationOrderID(token, fallback string) string { } return fallback } + +// MuteNotifScope is the set of valid notification scopes for per-recipient +// muting (issue #297). Each scope corresponds to one category of outbound email; +// a mute row suppresses only that category for its holder. +type MuteNotifScope string + +const ( + // ScopePurchaseApprovals suppresses purchase-approval-request emails. + ScopePurchaseApprovals MuteNotifScope = "purchase_approvals" + // ScopeRIExchangeApprovals suppresses RI-exchange pending-approval emails. + ScopeRIExchangeApprovals MuteNotifScope = "ri_exchange_approvals" +) + +// DeriveMuteToken returns a 32-byte HMAC-SHA256 token (hex-encoded) that +// signs the (email, scope) tuple. The token is embedded in the +// List-Unsubscribe URL; the handler re-derives it from the query params and +// compares in constant time, so a forged URL cannot mute a different address. +// +// key must come from a deployment secret (NOTIFICATION_MUTE_SECRET env var). +// When key is empty a static fallback is used so local-dev / test environments +// still produce a deterministic token without crashing; production deployments +// MUST set the env var. +func DeriveMuteToken(key []byte, email, scope string) string { + if len(key) == 0 { + // Fallback for local dev / tests: deterministic but clearly insecure. + key = []byte("dev-mute-secret-not-for-production") + } + mac := hmac.New(sha256.New, key) + mac.Write([]byte(strings.ToLower(email))) + mac.Write([]byte("|")) + mac.Write([]byte(scope)) + return hex.EncodeToString(mac.Sum(nil)) +} + +// VerifyMuteToken returns true when token equals the HMAC for (email, scope) +// under key. Comparison is constant-time to prevent timing attacks. +func VerifyMuteToken(key []byte, email, scope, token string) bool { + want := DeriveMuteToken(key, email, scope) + return hmac.Equal([]byte(want), []byte(token)) +} From 5d8f459e19c156d402371fcf2c218bd971b9e78a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 19:21:32 +0200 Subject: [PATCH 02/10] fix(ci): add IsNotificationMuted to all config.StoreInterface mocks (#828) UpsertNotificationMute and IsNotificationMuted were added to config.StoreInterface (issue #297) but the stub implementations in five test files were not updated, causing go vet to fail with interface satisfaction errors. Affected mocks: - internal/analytics/collector_test.go (mockConfigStore) - internal/purchase/mocks_test.go (MockConfigStore) - internal/scheduler/scheduler_overrides_test.go (mockOverrideStore) - internal/scheduler/scheduler_suppressions_test.go (mockSuppressionStore) - internal/scheduler/scheduler_test.go (MockConfigStore) - internal/server/test_helpers_test.go (mockConfigStoreForHealth) --- internal/analytics/collector_test.go | 6 ++++++ internal/mocks/stores.go | 20 +++++++++++++++++++ .../scheduler/scheduler_overrides_test.go | 6 ++++++ .../scheduler/scheduler_suppressions_test.go | 6 ++++++ internal/server/test_helpers_test.go | 8 ++++++++ 5 files changed, 46 insertions(+) diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index a988e7a87..25e9f34db 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -429,6 +429,12 @@ func (m *mockConfigStore) GetRIUtilizationCache(_ context.Context, _ string, _ i func (m *mockConfigStore) UpsertRIUtilizationCache(_ context.Context, _ string, _ int, _ []byte, _ time.Time) error { return nil } +func (m *mockConfigStore) UpsertNotificationMute(_ context.Context, _, _, _ string) error { + return nil +} +func (m *mockConfigStore) IsNotificationMuted(_ context.Context, _, _ string) (bool, error) { + return false, nil +} func (m *mockConfigStore) UpdatePurchaseHistoryListing(_ context.Context, _, _, _ string) error { return nil diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index ed446b09a..38e8b80d3 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1390,6 +1390,7 @@ func (m *MockConfigStore) UpsertLadderConfig(ctx context.Context, cfg *config.La return v, args.Error(1) } +<<<<<<< HEAD // SaveLadderRun mocks the SaveLadderRun operation. // Returns (nil, nil) when no expectation is registered. func (m *MockConfigStore) SaveLadderRun(ctx context.Context, run *config.LadderRunDB) (*config.LadderRunDB, error) { @@ -1504,6 +1505,25 @@ func (m *MockConfigStore) TransitionLadderRunStatus(ctx context.Context, id stri return v, args.Error(1) } +// UpsertNotificationMute mocks the UpsertNotificationMute operation. +// Defaults to nil when no expectation is registered. +func (m *MockConfigStore) UpsertNotificationMute(ctx context.Context, recipientEmail, scope, unmuteToken string) error { + if !isExpected(&m.Mock, "UpsertNotificationMute") { + return nil + } + return m.Called(ctx, recipientEmail, scope, unmuteToken).Error(0) +} + +// IsNotificationMuted mocks the IsNotificationMuted operation. +// Defaults to (false, nil) when no expectation is registered. +func (m *MockConfigStore) IsNotificationMuted(ctx context.Context, recipientEmail, scope string) (bool, error) { + if !isExpected(&m.Mock, "IsNotificationMuted") { + return false, nil + } + args := m.Called(ctx, recipientEmail, scope) + return args.Bool(0), args.Error(1) +} + // isExpected reports whether mock has any .On() expectation for method. func isExpected(m *mock.Mock, method string) bool { for _, call := range m.ExpectedCalls { diff --git a/internal/scheduler/scheduler_overrides_test.go b/internal/scheduler/scheduler_overrides_test.go index f88a06347..01119cdcb 100644 --- a/internal/scheduler/scheduler_overrides_test.go +++ b/internal/scheduler/scheduler_overrides_test.go @@ -72,6 +72,12 @@ func (m *mockOverrideStore) GetGlobalConfig(_ context.Context) (*config.GlobalCo RecommendationsLookbackDays: config.DefaultRecommendationsLookbackDays, }, nil } +func (m *mockOverrideStore) UpsertNotificationMute(_ context.Context, _, _, _ string) error { + return nil +} +func (m *mockOverrideStore) IsNotificationMuted(_ context.Context, _, _ string) (bool, error) { + return false, nil +} func boolPtr(b bool) *bool { return &b } diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index 9d64411bc..70b11c66e 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -58,6 +58,12 @@ func (m *mockSuppressionStore) GetGlobalConfig(_ context.Context) (*config.Globa RecommendationsLookbackDays: config.DefaultRecommendationsLookbackDays, }, nil } +func (m *mockSuppressionStore) UpsertNotificationMute(_ context.Context, _, _, _ string) error { + return nil +} +func (m *mockSuppressionStore) IsNotificationMuted(_ context.Context, _, _ string) (bool, error) { + return false, nil +} func strPtr(s string) *string { return &s } diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 52618d416..e93057a79 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -378,3 +378,11 @@ func (m *mockConfigStoreForHealth) ClaimMarketplaceListingSlot(_ context.Context func (m *mockConfigStoreForHealth) StampOfferingClass(_ context.Context, _, _ string) error { return nil } + +func (m *mockConfigStoreForHealth) UpsertNotificationMute(_ context.Context, _, _, _ string) error { + return nil +} + +func (m *mockConfigStoreForHealth) IsNotificationMuted(_ context.Context, _, _ string) (bool, error) { + return false, nil +} From b6882d68d40c653431d698841c5ee7a73631bf14 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 16:48:53 +0200 Subject: [PATCH 03/10] fix(notifications): wire per-recipient mute + List-Unsubscribe into production sender (refs #297) The per-recipient mute + List-Unsubscribe feature was dead in production: the sender returned by email.NewSenderFromEnvironment was never decorated with the mute checker or the unsubscribe base URL, so the bare sender's mute checker was always nil and no List-Unsubscribe header was ever emitted. The SMTP transport bypassed the mute logic entirely. - app.go: decorate the factory-produced sender via a new decorateSenderWithMute helper that type-switches on *email.Sender / *email.SMTPSender and calls WithMuteChecker(configStore) + WithUnsubscribeBaseURL(trimmed DASHBOARD_URL). configStore (config.StoreInterface) satisfies email.MuteChecker via IsNotificationMuted. The base URL reuses the same DASHBOARD_URL the email templates already use, trimmed to match resolveOIDCIssuerURL. - email/mute.go: extract the transport-agnostic mute + List-Unsubscribe decision logic (isRecipientMuted, filterMutedRecipients, unsubscribeURLFor, unsubscribeHeaderValuesFor) into shared free functions so the SES and SMTP paths cannot diverge; *Sender delegates to them. - email/smtp_sender.go: add muteChecker + unsubscribeBaseURL fields and WithMuteChecker/WithUnsubscribeBaseURL mirroring *Sender, and apply the mute skip + CC filter + RFC 8058 List-Unsubscribe headers in SendPurchaseApprovalRequest. - Wiring tests exercise the real factory/decoration path (decorateSenderWithMute on a factory-shaped sender) and the SMTP transport: a muted recipient is suppressed and the List-Unsubscribe header is emitted with the wired base URL. Both fail against the pre-wiring behavior (nil checker, no base URL). --- internal/email/mute.go | 72 +++++++++++ internal/email/sender.go | 41 +------ internal/email/smtp_mute_test.go | 91 ++++++++++++++ internal/email/smtp_sender.go | 156 +++++++++++++++++++++++- internal/server/app.go | 24 ++++ internal/server/app_mute_wiring_test.go | 113 +++++++++++++++++ 6 files changed, 458 insertions(+), 39 deletions(-) create mode 100644 internal/email/mute.go create mode 100644 internal/email/smtp_mute_test.go create mode 100644 internal/server/app_mute_wiring_test.go diff --git a/internal/email/mute.go b/internal/email/mute.go new file mode 100644 index 000000000..1d797d853 --- /dev/null +++ b/internal/email/mute.go @@ -0,0 +1,72 @@ +package email + +import ( + "context" + "net/url" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" +) + +// This file holds the transport-agnostic mute + List-Unsubscribe logic shared +// by the SES (*Sender) and SMTP (*SMTPSender) paths. Both transports must apply +// the same per-recipient mute suppression, CC filtering, and RFC 8058 +// 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. +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 + } + return muted +} + +// 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). +func filterMutedRecipients(ctx context.Context, mc MuteChecker, addrs []string, scope string) []string { + if mc == nil || len(addrs) == 0 { + return addrs + } + out := make([]string, 0, len(addrs)) + for _, addr := range addrs { + if !isRecipientMuted(ctx, mc, addr, scope) { + out = append(out, addr) + } + } + return out +} + +// unsubscribeURLFor constructs the one-click unsubscribe URL for the given +// (email, scope) pair. Returns "" when baseURL is empty. +func unsubscribeURLFor(baseURL, email, scope string) string { + if baseURL == "" { + return "" + } + token := common.DeriveMuteToken(muteKey(), email, scope) + q := url.Values{ + "token": {token}, + "email": {email}, + "scope": {scope}, + } + return baseURL + "/api/notifications/unsubscribe?" + q.Encode() +} + +// unsubscribeHeaderValuesFor returns the List-Unsubscribe and +// List-Unsubscribe-Post header values (RFC 8058) for the given (email, scope) +// pair. Returns ("", "") when baseURL is empty. +func unsubscribeHeaderValuesFor(baseURL, email, scope string) (headerValue, postValue string) { + unsubURL := unsubscribeURLFor(baseURL, email, scope) + if unsubURL == "" { + return "", "" + } + return "<" + unsubURL + ">", "List-Unsubscribe=One-Click" +} diff --git a/internal/email/sender.go b/internal/email/sender.go index a453f67ad..ccaf5edb5 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -5,11 +5,9 @@ import ( "context" "errors" "fmt" - "net/url" "os" "strings" - "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -143,43 +141,21 @@ func muteKey() []byte { // buildUnsubscribeURL constructs the one-click unsubscribe URL for the given // (email, scope) pair. Returns ("", "") when unsubscribeBaseURL is empty. func (s *Sender) buildUnsubscribeURL(email, scope string) (unsubURL, mailtoURL string) { - if s.unsubscribeBaseURL == "" { - return "", "" - } - token := common.DeriveMuteToken(muteKey(), email, scope) - q := url.Values{ - "token": {token}, - "email": {email}, - "scope": {scope}, - } - unsubURL = s.unsubscribeBaseURL + "/api/notifications/unsubscribe?" + q.Encode() - return unsubURL, "" + return unsubscribeURLFor(s.unsubscribeBaseURL, email, scope), "" } // listUnsubscribeHeaders returns the List-Unsubscribe and List-Unsubscribe-Post // header values for the given (email, scope) pair (RFC 8058). // Returns ("", "") when no base URL is configured. func (s *Sender) listUnsubscribeHeaders(email, scope string) (headerValue, postValue string) { - unsubURL, _ := s.buildUnsubscribeURL(email, scope) - if unsubURL == "" { - return "", "" - } - return "<" + unsubURL + ">", "List-Unsubscribe=One-Click" + return unsubscribeHeaderValuesFor(s.unsubscribeBaseURL, email, scope) } // isMuted returns true when the given address is muted for this scope. When the // mute checker is nil or returns an error the address is treated as not muted so // a transient DB outage doesn't silently block approval emails. func (s *Sender) isMuted(ctx context.Context, email, scope string) bool { - if s.muteChecker == nil { - return false - } - muted, err := s.muteChecker.IsNotificationMuted(ctx, email, scope) - if err != nil { - logging.Warnf("email: mute check failed for scope=%s: %v", scope, err) - return false - } - return muted + return isRecipientMuted(ctx, s.muteChecker, email, scope) } // filterMutedAddresses returns a copy of addrs with any muted (for scope) @@ -187,16 +163,7 @@ func (s *Sender) isMuted(ctx context.Context, email, scope string) bool { // store are treated as "not muted" (fail-open) so a DB hiccup does not // silently suppress approval emails. func (s *Sender) filterMutedAddresses(ctx context.Context, addrs []string, scope string) []string { - if s.muteChecker == nil || len(addrs) == 0 { - return addrs - } - out := make([]string, 0, len(addrs)) - for _, addr := range addrs { - if !s.isMuted(ctx, addr, scope) { - out = append(out, addr) - } - } - return out + return filterMutedRecipients(ctx, s.muteChecker, addrs, scope) } // snsMaxSubjectLen is the maximum byte length SNS accepts for a Subject. diff --git a/internal/email/smtp_mute_test.go b/internal/email/smtp_mute_test.go new file mode 100644 index 000000000..e204c3bea --- /dev/null +++ b/internal/email/smtp_mute_test.go @@ -0,0 +1,91 @@ +package email + +import ( + "context" + "strings" + "testing" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// smtpApprovalData builds a minimal purchase-approval NotificationData. +func smtpApprovalData(recipient string) NotificationData { + return NotificationData{ + RecipientEmail: recipient, + DashboardURL: "https://dash.example.com", + ApprovalToken: "tok", + Recommendations: []RecommendationSummary{ + {Service: "ec2", Region: "us-east-1", Count: 1, MonthlySavings: 100}, + }, + } +} + +// TestSMTPSender_PurchaseApproval_MutedRecipient_NoSend verifies the SMTP +// transport applies the same per-recipient mute suppression as SES: a muted +// recipient never reaches the wire (no DATA section is transmitted). +func TestSMTPSender_PurchaseApproval_MutedRecipient_NoSend(t *testing.T) { + server := newMockSMTPServer(t, false) + server.start(t) + defer server.stop() + + base := &SMTPSender{ + host: "127.0.0.1", + port: server.port, + fromEmail: "sender@test.com", + useTLS: false, + } + mc := &wiringMuteCheckerSMTP{muted: map[string]bool{"muted@test.com": true}} + sender := base.WithMuteChecker(mc).WithUnsubscribeBaseURL("https://dash.example.com") + + err := sender.SendPurchaseApprovalRequest(context.Background(), smtpApprovalData("muted@test.com")) + require.NoError(t, err) + + server.stop() // flush the connection goroutine before reading receivedMsg + server.mu.Lock() + got := server.receivedMsg + server.mu.Unlock() + assert.NotContains(t, got, "Purchase Approval Required", + "muted recipient must not have a message body transmitted over SMTP") +} + +// TestSMTPSender_PurchaseApproval_EmitsListUnsubscribe verifies the SMTP +// approval send attaches the RFC 8058 List-Unsubscribe header sourced from the +// wired-in unsubscribe base URL. +func TestSMTPSender_PurchaseApproval_EmitsListUnsubscribe(t *testing.T) { + server := newMockSMTPServer(t, false) + server.start(t) + defer server.stop() + + base := &SMTPSender{ + host: "127.0.0.1", + port: server.port, + fromEmail: "sender@test.com", + useTLS: false, + } + mc := &wiringMuteCheckerSMTP{muted: map[string]bool{}} + sender := base.WithMuteChecker(mc).WithUnsubscribeBaseURL("https://dash.example.com") + + err := sender.SendPurchaseApprovalRequest(context.Background(), smtpApprovalData("approver@test.com")) + require.NoError(t, err) + + server.stop() + server.mu.Lock() + got := server.receivedMsg + server.mu.Unlock() + assert.Contains(t, got, "List-Unsubscribe:", + "SMTP approval send must carry a List-Unsubscribe header") + assert.Contains(t, got, "https://dash.example.com/api/notifications/unsubscribe") + assert.Contains(t, got, "scope="+string(common.ScopePurchaseApprovals)) + assert.True(t, strings.Contains(got, "List-Unsubscribe-Post:"), + "SMTP approval send must carry a List-Unsubscribe-Post header") +} + +type wiringMuteCheckerSMTP struct { + muted map[string]bool +} + +func (w *wiringMuteCheckerSMTP) IsNotificationMuted(_ context.Context, recipientEmail, _ string) (bool, error) { + return w.muted[recipientEmail], nil +} diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index d9a1f74fb..7f3ba0faf 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -11,6 +11,7 @@ import ( "net/smtp" "strings" + "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" ) @@ -38,6 +39,30 @@ type SMTPSender struct { port int useTLS bool allowInsecure bool + // muteChecker consults the muted_recipients table before each send. + // Nil disables mute checking (e.g. when no DB is wired in tests). + muteChecker MuteChecker + // unsubscribeBaseURL is the dashboard base URL used to construct the + // List-Unsubscribe header value. Empty disables the header. + unsubscribeBaseURL string +} + +// WithMuteChecker returns a shallow copy of s with the given MuteChecker wired +// in, mirroring (*Sender).WithMuteChecker so the SMTP transport applies the same +// per-recipient mute suppression as SES. +func (s *SMTPSender) WithMuteChecker(mc MuteChecker) *SMTPSender { + c := *s + c.muteChecker = mc + return &c +} + +// WithUnsubscribeBaseURL returns a shallow copy of s with the given base URL +// set, mirroring (*Sender).WithUnsubscribeBaseURL. When non-empty the SMTP +// approval send emits RFC 8058 List-Unsubscribe headers. +func (s *SMTPSender) WithUnsubscribeBaseURL(u string) *SMTPSender { + c := *s + c.unsubscribeBaseURL = u + return &c } // NewSMTPSender creates a new SMTP email sender. @@ -205,6 +230,32 @@ func (s *SMTPSender) buildSMTPMessageMultipart(toEmail string, cc []string, subj } headers += fmt.Sprintf("Subject: %s\r\nMIME-Version: 1.0\r\nContent-Type: multipart/alternative; boundary=%q\r\n\r\n", subject, boundary) + return []byte(headers + buildMultipartBody(boundary, textBody, htmlBody)) +} + +// buildSMTPMessageMultipartWithHeaders is buildSMTPMessageMultipart with extra +// pre-sanitized header lines (e.g. List-Unsubscribe) inserted before the +// header-terminating blank line. extraHeaders must already be CRLF-terminated. +func (s *SMTPSender) buildSMTPMessageMultipartWithHeaders(toEmail string, cc []string, subject, textBody, htmlBody, extraHeaders string) []byte { + const boundary = "cudly-mp-7e3b1c89af04d2" + from := s.fromEmail + if s.fromName != "" { + from = fmt.Sprintf("%s <%s>", sanitizeHeader(s.fromName), s.fromEmail) + } + headers := fmt.Sprintf("From: %s\r\nTo: %s\r\n", from, toEmail) + if len(cc) > 0 { + headers += fmt.Sprintf("Cc: %s\r\n", strings.Join(cc, ", ")) + } + headers += fmt.Sprintf("Subject: %s\r\nMIME-Version: 1.0\r\nContent-Type: multipart/alternative; boundary=\"%s\"\r\n", subject, boundary) + headers += extraHeaders + headers += "\r\n" + + return []byte(headers + buildMultipartBody(boundary, textBody, htmlBody)) +} + +// buildMultipartBody assembles the multipart/alternative body (text + HTML +// parts) for the given boundary. Shared by the plain and header-aware builders. +func buildMultipartBody(boundary, textBody, htmlBody string) string { var body strings.Builder body.WriteString("--") body.WriteString(boundary) @@ -218,7 +269,25 @@ func (s *SMTPSender) buildSMTPMessageMultipart(toEmail string, cc []string, subj body.WriteString(boundary) body.WriteString("--\r\n") - return []byte(headers + body.String()) + return body.String() +} + +// buildSMTPMessageWithHeaders is buildSMTPMessage with extra pre-sanitized +// header lines (e.g. List-Unsubscribe) inserted before the header-terminating +// blank line. extraHeaders must already be CRLF-terminated. +func (s *SMTPSender) buildSMTPMessageWithHeaders(toEmail string, cc []string, subject, body, extraHeaders string) []byte { + from := s.fromEmail + if s.fromName != "" { + from = fmt.Sprintf("%s <%s>", sanitizeHeader(s.fromName), s.fromEmail) + } + headers := fmt.Sprintf("From: %s\r\nTo: %s\r\n", from, toEmail) + if len(cc) > 0 { + headers += fmt.Sprintf("Cc: %s\r\n", strings.Join(cc, ", ")) + } + headers += fmt.Sprintf("Subject: %s\r\nMIME-Version: 1.0\r\nContent-Type: text/plain; charset=UTF-8\r\n", subject) + headers += extraHeaders + headers += "\r\n" + return []byte(headers + body + "\r\n") } // buildSMTPMessage assembles the RFC-5322 message bytes (headers + blank @@ -435,6 +504,11 @@ func (s *SMTPSender) SendRIExchangeCompleted(ctx context.Context, data RIExchang // 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. +// +// 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 +// addresses are dropped, and an RFC 8058 List-Unsubscribe header pair is added +// when an unsubscribe base URL is configured. func (s *SMTPSender) SendPurchaseApprovalRequest(ctx context.Context, data NotificationData) error { recipient := data.RecipientEmail if recipient == "" { @@ -443,8 +517,86 @@ func (s *SMTPSender) SendPurchaseApprovalRequest(ctx context.Context, data Notif if recipient == "" { return ErrNoRecipient } + + scope := string(common.ScopePurchaseApprovals) + + // Per-recipient mute check: skip silently if the approver has opted out. + if isRecipientMuted(ctx, s.muteChecker, recipient, scope) { + logging.Infof("email/smtp: purchase approval skipped for muted recipient (scope=%s)", scope) + return nil + } + + // Filter CC list against mutes so no muted address receives a copy. + filteredCC := filterMutedRecipients(ctx, s.muteChecker, data.CCEmails, scope) + + // Build RFC 8058 List-Unsubscribe headers scoped to the primary recipient. + unsubHdr, postHdr := unsubscribeHeaderValuesFor(s.unsubscribeBaseURL, recipient, scope) + subject := fmt.Sprintf("CUDly - Purchase Approval Required (%d commitment(s))", len(data.Recommendations)) - return sendPurchaseApprovalRequestVia(ctx, s, recipient, subject, data) + + textBody, err := RenderPurchaseApprovalRequestEmail(data) + if err != nil { + return fmt.Errorf("failed to render purchase approval request email (text): %w", err) + } + htmlBody, htmlErr := RenderPurchaseApprovalRequestEmailHTML(data) + if htmlErr != nil { + logging.Warnf("email: HTML approval-request render failed, falling back to text-only: %v", htmlErr) + htmlBody = "" + } + return s.sendMultipartWithUnsubscribe(ctx, recipient, filteredCC, subject, textBody, htmlBody, unsubHdr, postHdr) +} + +// sendMultipartWithUnsubscribe sends a multipart/alternative (text + HTML) +// message via SMTP, optionally injecting the RFC 8058 List-Unsubscribe / +// List-Unsubscribe-Post headers. htmlBody == "" degrades to a single-part text +// send. unsubHdr == "" omits the unsubscribe headers entirely. +func (s *SMTPSender) sendMultipartWithUnsubscribe(ctx context.Context, toEmail string, ccEmails []string, subject, textBody, htmlBody, unsubHdr, postHdr string) error { + if unsubHdr == "" { + // No List-Unsubscribe headers required: reuse the existing send path. + return s.SendToEmailWithCCMultipart(ctx, toEmail, ccEmails, subject, textBody, htmlBody) + } + if s.fromEmail == "" { + logging.Debug("No from email configured, skipping email") + return nil + } + + toEmail = sanitizeHeader(toEmail) + subject = sanitizeHeader(subject) + sanitizedCC := sanitizeCCList(toEmail, ccEmails) + + extra := buildListUnsubscribeHeaderLines(unsubHdr, postHdr) + var msg []byte + if htmlBody == "" { + msg = s.buildSMTPMessageWithHeaders(toEmail, sanitizedCC, subject, textBody, extra) + } else { + msg = s.buildSMTPMessageMultipartWithHeaders(toEmail, sanitizedCC, subject, textBody, htmlBody, extra) + } + rcpts := append([]string{toEmail}, sanitizedCC...) + + if err := s.dispatchSMTP(rcpts, msg); err != nil { + return err + } + + if len(sanitizedCC) > 0 { + logging.Debugf("Sent approval email via SMTP to %s (cc %d): %s", toEmail, len(sanitizedCC), subject) + } else { + logging.Debugf("Sent approval email via SMTP to %s: %s", toEmail, subject) + } + return nil +} + +// buildListUnsubscribeHeaderLines returns the RFC 8058 List-Unsubscribe header +// lines (each terminated with CRLF), already sanitized against header +// injection. Returns "" when headerValue is empty. +func buildListUnsubscribeHeaderLines(headerValue, postValue string) string { + if headerValue == "" { + return "" + } + lines := fmt.Sprintf("List-Unsubscribe: %s\r\n", sanitizeHeader(headerValue)) + if postValue != "" { + lines += fmt.Sprintf("List-Unsubscribe-Post: %s\r\n", sanitizeHeader(postValue)) + } + return lines } // SendPurchaseScheduledNotification sends the Gmail-style pre-fire delay diff --git a/internal/server/app.go b/internal/server/app.go index fbd8b687c..e9264ab6d 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -232,6 +232,24 @@ func resolveOIDCIssuerURL(cfg ApplicationConfig) string { return strings.TrimRight(cfg.DashboardURL, "/") } +// decorateSenderWithMute wires per-recipient mute suppression and the +// List-Unsubscribe base URL into the concrete SES (*email.Sender) or SMTP +// (*email.SMTPSender) sender. Other implementations (e.g. the no-op sender used +// when EMAIL_ENABLED=false) are returned unchanged. configStore satisfies +// email.MuteChecker via its IsNotificationMuted method. dashboardURL is the +// already-trimmed dashboard base URL; an empty value disables the +// List-Unsubscribe header (the mute checker is still wired in). +func decorateSenderWithMute(sender email.SenderInterface, mc email.MuteChecker, dashboardURL string) email.SenderInterface { + switch s := sender.(type) { + case *email.Sender: + return s.WithMuteChecker(mc).WithUnsubscribeBaseURL(dashboardURL) + case *email.SMTPSender: + return s.WithMuteChecker(mc).WithUnsubscribeBaseURL(dashboardURL) + default: + return sender + } +} + // LoadApplicationConfig reads all configuration from environment variables. func LoadApplicationConfig() ApplicationConfig { version := os.Getenv("VERSION") @@ -530,6 +548,12 @@ func NewApplication(ctx context.Context, version string) (*Application, error) { if err != nil { return nil, fmt.Errorf("failed to initialize email sender: %w", err) } + // Wire per-recipient mute suppression + List-Unsubscribe into the production + // sender. Without this the mute-aware send logic in the email package is dead + // code (the bare sender has a nil mute checker and no unsubscribe base URL). + // The dashboard base URL is sourced from the same DASHBOARD_URL the email + // templates already use, trimmed of a trailing slash to match resolveOIDCIssuerURL. + emailSender = decorateSenderWithMute(emailSender, configStore, strings.TrimRight(cfg.DashboardURL, "/")) // Initialize AWS config for STS client awsCfg, err := awsconfig.LoadDefaultConfig(ctx) diff --git a/internal/server/app_mute_wiring_test.go b/internal/server/app_mute_wiring_test.go new file mode 100644 index 000000000..0f52a84fb --- /dev/null +++ b/internal/server/app_mute_wiring_test.go @@ -0,0 +1,113 @@ +package server + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/email" + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/aws/aws-sdk-go-v2/service/sesv2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// wiringMockSES is a minimal SESEmailSender used by the wiring tests. It records +// the SendEmail inputs it receives so the test can assert both that a send did +// (or did not) happen and what headers it carried. GetAccount returns +// production mode so the sandbox-verification path is skipped. +type wiringMockSES struct { + sent []*sesv2.SendEmailInput +} + +func (m *wiringMockSES) SendEmail(_ context.Context, in *sesv2.SendEmailInput, _ ...func(*sesv2.Options)) (*sesv2.SendEmailOutput, error) { + m.sent = append(m.sent, in) + return &sesv2.SendEmailOutput{}, nil +} + +func (m *wiringMockSES) GetAccount(_ context.Context, _ *sesv2.GetAccountInput, _ ...func(*sesv2.Options)) (*sesv2.GetAccountOutput, error) { + return &sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil +} + +func (m *wiringMockSES) GetEmailIdentity(_ context.Context, _ *sesv2.GetEmailIdentityInput, _ ...func(*sesv2.Options)) (*sesv2.GetEmailIdentityOutput, error) { + return &sesv2.GetEmailIdentityOutput{VerifiedForSendingStatus: true}, nil +} + +func (m *wiringMockSES) CreateEmailIdentity(_ context.Context, _ *sesv2.CreateEmailIdentityInput, _ ...func(*sesv2.Options)) (*sesv2.CreateEmailIdentityOutput, error) { + return &sesv2.CreateEmailIdentityOutput{}, nil +} + +// wiringMuteChecker is a fixed-set mute checker keyed by recipient email. +type wiringMuteChecker struct { + muted map[string]bool +} + +func (w *wiringMuteChecker) IsNotificationMuted(_ context.Context, recipientEmail, _ string) (bool, error) { + return w.muted[recipientEmail], nil +} + +// approvalData builds a minimal purchase-approval NotificationData for the +// given recipient. +func approvalData(recipient string) email.NotificationData { + return email.NotificationData{ + RecipientEmail: recipient, + DashboardURL: "https://dash.example.com", + ApprovalToken: "tok", + Recommendations: []email.RecommendationSummary{ + {Service: "ec2", Region: "us-east-1", Count: 1, MonthlySavings: 100}, + }, + } +} + +// TestDecorateSenderWithMute_SuppressesMutedRecipient verifies the real app +// wiring path (decorateSenderWithMute, the same call NewApplication makes) +// actually enables per-recipient mute suppression on a factory-shaped sender. +// +// This test exercises the decoration the production factory applies rather than +// hand-injecting the mute checker into a Sender literal. Before the wiring fix +// the production sender's mute checker was always nil, so a muted recipient +// would still be sent an approval email; this test would fail in that state. +func TestDecorateSenderWithMute_SuppressesMutedRecipient(t *testing.T) { + ctx := context.Background() + ses := &wiringMockSES{} + // NewSenderWithClients yields the same concrete *email.Sender type the AWS + // branch of NewSenderFromEnvironment produces, so the type switch in + // decorateSenderWithMute takes the same path it does in production. + base := email.NewSenderWithClients(nil, ses, email.SenderConfig{FromEmail: "noreply@example.com"}) + mc := &wiringMuteChecker{muted: map[string]bool{"muted@example.com": true}} + + decorated := decorateSenderWithMute(base, mc, "https://dash.example.com") + + err := decorated.SendPurchaseApprovalRequest(ctx, approvalData("muted@example.com")) + require.NoError(t, err) + assert.Empty(t, ses.sent, "muted recipient must not receive a SendEmail call after wiring") +} + +// TestDecorateSenderWithMute_EmitsListUnsubscribe verifies the decorated +// production sender both sends to a non-muted recipient and attaches the RFC +// 8058 List-Unsubscribe header sourced from the wired-in dashboard base URL. +// Pre-fix the production sender had no unsubscribe base URL, so no such header +// was emitted. +func TestDecorateSenderWithMute_EmitsListUnsubscribe(t *testing.T) { + ctx := context.Background() + ses := &wiringMockSES{} + base := email.NewSenderWithClients(nil, ses, email.SenderConfig{FromEmail: "noreply@example.com"}) + mc := &wiringMuteChecker{muted: map[string]bool{}} + + decorated := decorateSenderWithMute(base, mc, "https://dash.example.com") + + err := decorated.SendPurchaseApprovalRequest(ctx, approvalData("approver@example.com")) + require.NoError(t, err) + require.Len(t, ses.sent, 1, "non-muted recipient must receive exactly one SendEmail call") + + msg := ses.sent[0].Content.Simple + require.NotNil(t, msg) + var foundUnsub bool + for _, h := range msg.Headers { + if h.Name != nil && *h.Name == "List-Unsubscribe" { + foundUnsub = true + assert.Contains(t, *h.Value, "https://dash.example.com/api/notifications/unsubscribe", "List-Unsubscribe must use the wired dashboard base URL") + assert.Contains(t, *h.Value, "scope="+string(common.ScopePurchaseApprovals)) + } + } + assert.True(t, foundUnsub, "decorated production sender must emit a List-Unsubscribe header") +} From ed4500d24e6c8c02ef781cd888a71e27794bd846 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 23:22:21 +0200 Subject: [PATCH 04/10] fix(notifications): fix hardcoded MIME boundary in SMTP+List-Unsubscribe path buildSMTPMessageMultipartWithHeaders used a fixed boundary string instead of mimeRandBoundary(), unlike its sibling buildSMTPMessageMultipart. A body containing the literal boundary causes MIME corruption. Also fix two US-spelling violations (behaviour->behavior, Centralising->Centralizing) in pkg/common/tokens.go introduced by this PR. --- internal/email/smtp_sender.go | 4 +++- pkg/common/tokens.go | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index 7f3ba0faf..568f1004e 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -236,8 +236,10 @@ func (s *SMTPSender) buildSMTPMessageMultipart(toEmail string, cc []string, subj // buildSMTPMessageMultipartWithHeaders is buildSMTPMessageMultipart with extra // pre-sanitized header lines (e.g. List-Unsubscribe) inserted before the // header-terminating blank line. extraHeaders must already be CRLF-terminated. +// A per-message random boundary is generated via mimeRandBoundary to avoid +// the body-collision risk of a fixed literal (07-N2). func (s *SMTPSender) buildSMTPMessageMultipartWithHeaders(toEmail string, cc []string, subject, textBody, htmlBody, extraHeaders string) []byte { - const boundary = "cudly-mp-7e3b1c89af04d2" + boundary := mimeRandBoundary() from := s.fromEmail if s.fromName != "" { from = fmt.Sprintf("%s <%s>", sanitizeHeader(s.fromName), s.fromEmail) diff --git a/pkg/common/tokens.go b/pkg/common/tokens.go index 33d158bb8..07d567682 100644 --- a/pkg/common/tokens.go +++ b/pkg/common/tokens.go @@ -72,7 +72,7 @@ func MaskToken(token string) string { // It uses the first 32 hex characters (128 bits) of the token, which is itself a // SHA-256 hex digest, so the GUID is deterministic and collision-free at any // realistic purchase volume. Returns "" when token is shorter than 32 hex chars -// (e.g. empty) so callers keep their prior non-idempotent ID behaviour. +// (e.g. empty) so callers keep their prior non-idempotent ID behavior. func IdempotencyGUID(token string) string { if len(token) < 32 { return "" @@ -87,7 +87,7 @@ func IdempotencyGUID(token string) string { // ReservationOrderID returns the Azure reservationOrderID to PUT for a purchase: // the deterministic GUID derived from token when one is supplied (issue #641, so // a re-drive re-PUTs the same idempotent order), otherwise fallback (the caller's -// prior non-idempotent ID, e.g. a random GUID or a timestamp). Centralising the +// prior non-idempotent ID, e.g. a random GUID or a timestamp). Centralizing the // choice keeps each executor's PurchaseCommitment a single statement and avoids // repeating the same empty-token guard across every Azure service. func ReservationOrderID(token, fallback string) string { From 5d143d4cd095381294651988bbfa58b205057b10 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 23:58:59 +0200 Subject: [PATCH 05/10] fix(notifications): harden mute token + close CC unsubscribe IDOR Address CodeRabbit review findings on the per-recipient mute feature: - Fail closed on a missing mute secret. ResolveMuteSecret returns the NOTIFICATION_MUTE_SECRET bytes when set, the dev key only in non-production, and ErrMuteSecretMissing in production. DeriveMuteToken no longer silently signs with a well-known fallback on an empty key (returns ""), and VerifyMuteToken fails closed so a missing secret can never accept a forged token. The unsubscribe handler returns a 500 (not a 401) when the production secret is absent, and the send path emits no List-Unsubscribe header rather than a tokenless or forgeable one. - Suppress List-Unsubscribe when CC recipients share the envelope. The header token is bound to the primary recipient only; sending it to a CC list let any CC recipient one-click-mute the primary address. Both the SES and SMTP paths now omit the header whenever a CC list exists. - Redact the recipient email in the SMTP approval-delivery debug logs. - Correct the UpsertNotificationMute interface doc to match the implemented refresh-on-conflict semantics (muted_at = NOW()). --- internal/api/handler_notifications.go | 21 ++++----- internal/api/handler_notifications_test.go | 48 +++++++++++++++++--- internal/config/interfaces.go | 4 +- internal/email/mute.go | 7 ++- internal/email/mute_test.go | 42 ++++++++++++++++++ internal/email/sender.go | 15 ++++--- internal/email/smtp_mute_test.go | 29 ++++++++++++ internal/email/smtp_sender.go | 14 ++++-- internal/email/templates.go | 12 ++++- pkg/common/tokens.go | 51 +++++++++++++++++++--- pkg/common/tokens_test.go | 40 +++++++++++++++++ 11 files changed, 243 insertions(+), 40 deletions(-) diff --git a/internal/api/handler_notifications.go b/internal/api/handler_notifications.go index ee0eca9d7..5f489f447 100644 --- a/internal/api/handler_notifications.go +++ b/internal/api/handler_notifications.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "html/template" - "os" "strings" "github.com/LeanerCloud/CUDly/pkg/common" @@ -52,16 +51,6 @@ func scopeLabel(scope string) string { } } -// muteSecretKey loads the NOTIFICATION_MUTE_SECRET env var as bytes. Returns -// nil when unset, which causes DeriveMuteToken to fall back to its dev key. -func muteSecretKey() []byte { - v := os.Getenv("NOTIFICATION_MUTE_SECRET") - if v == "" { - return nil - } - return []byte(v) -} - // unsubscribeHandler handles GET /api/notifications/unsubscribe. // The URL carries a signed token that encodes (email, scope); the handler // verifies the HMAC, upserts the mute row, and returns a confirmation page. @@ -83,7 +72,15 @@ func (h *Handler) unsubscribeHandler(ctx context.Context, req *events.LambdaFunc return nil, NewClientError(400, fmt.Sprintf("unknown notification scope: %s", scope)) } - if !common.VerifyMuteToken(muteSecretKey(), email, scope, token) { + // Resolve the HMAC key with the fail-closed production policy: a missing + // NOTIFICATION_MUTE_SECRET in production is a server misconfiguration, not a + // client error, and must never silently verify against a well-known key. + key, err := common.ResolveMuteSecret() + if err != nil { + logging.Errorf("notifications/unsubscribe: %v", err) + return nil, fmt.Errorf("unsubscribe is not configured: %w", err) + } + if !common.VerifyMuteToken(key, email, scope, token) { logging.Warnf("notifications/unsubscribe: invalid token for scope=%s", scope) return nil, NewClientError(401, "invalid or expired unsubscribe token") } diff --git a/internal/api/handler_notifications_test.go b/internal/api/handler_notifications_test.go index d9fef11ab..57f299182 100644 --- a/internal/api/handler_notifications_test.go +++ b/internal/api/handler_notifications_test.go @@ -16,15 +16,20 @@ import ( // GET /api/notifications/unsubscribe // --------------------------------------------------------------------------- -func validUnsubToken(email, scope string) string { - return common.DeriveMuteToken(nil, email, scope) +func validUnsubToken(t *testing.T, email, scope string) string { + t.Helper() + // Resolve the key the same way the handler does so the generated token + // verifies. With ENVIRONMENT unset this yields the deterministic dev key. + key, err := common.ResolveMuteSecret() + require.NoError(t, err) + return common.DeriveMuteToken(key, email, scope) } func TestUnsubscribeHandler_Success(t *testing.T) { ctx := context.Background() email := "user@example.com" scope := string(common.ScopePurchaseApprovals) - token := validUnsubToken(email, scope) + token := validUnsubToken(t, email, scope) mockStore := new(MockConfigStore) mockStore.On("UpsertNotificationMute", ctx, email, scope, token).Return(nil) @@ -68,6 +73,35 @@ func TestUnsubscribeHandler_ForgedToken_Returns401(t *testing.T) { assert.Equal(t, 401, ce.code) } +func TestUnsubscribeHandler_ProductionMissingSecret_FailsClosed(t *testing.T) { + // With ENVIRONMENT=production and no NOTIFICATION_MUTE_SECRET, the handler + // must NOT verify against a well-known dev key (which would accept forged + // tokens). It returns a server-side error (500), never a 401/200, and never + // reaches the store. + t.Setenv("ENVIRONMENT", "production") + t.Setenv("NOTIFICATION_MUTE_SECRET", "") + ctx := context.Background() + + mockStore := new(MockConfigStore) + t.Cleanup(func() { + mockStore.AssertNotCalled(t, "UpsertNotificationMute") + }) + h := &Handler{config: mockStore} + r := newTestRouter(h) + + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{ + "token": strings.Repeat("a", 64), + "email": "user@example.com", + "scope": string(common.ScopePurchaseApprovals), + }, + } + _, err := r.unsubscribeHandler(ctx, req, nil) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "missing secret in production is a 500, not a client error") +} + func TestUnsubscribeHandler_MissingParams_Returns400(t *testing.T) { ctx := context.Background() h := &Handler{config: new(MockConfigStore)} @@ -90,12 +124,12 @@ func TestUnsubscribeHandler_UnknownScope_Returns400(t *testing.T) { ctx := context.Background() email := "user@example.com" scope := "unknown_scope" - // Use the dev-key token so HMAC passes but scope guard fires first. - token := common.DeriveMuteToken(nil, email, scope) req := &events.LambdaFunctionURLRequest{ QueryStringParameters: map[string]string{ - "token": token, + // The scope guard fires before token verification, so any non-empty + // token reaches it; the value is irrelevant to this test. + "token": strings.Repeat("a", 64), "email": email, "scope": scope, }, @@ -114,7 +148,7 @@ func TestUnsubscribeHandler_StoreError_Returns500(t *testing.T) { ctx := context.Background() email := "user@example.com" scope := string(common.ScopePurchaseApprovals) - token := validUnsubToken(email, scope) + token := validUnsubToken(t, email, scope) mockStore := new(MockConfigStore) mockStore.On("UpsertNotificationMute", mock.Anything, email, scope, token). diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 54d30aaab..8a3b65db8 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -439,8 +439,8 @@ type StoreInterface interface { // Notification mutes (issue #297 / migration 000078). // UpsertNotificationMute inserts or updates a mute row for (email, scope). - // Idempotent: calling it again for an already-muted address is a no-op on - // muted_at but does replace unmute_token if the token changes. + // Idempotent for row existence: calling it again for an already-muted + // address refreshes muted_at and replaces unmute_token if the token changes. UpsertNotificationMute(ctx context.Context, recipientEmail, scope, unmuteToken string) error // IsNotificationMuted returns true when (email, scope) has a row in // muted_recipients. The email comparison is case-insensitive. diff --git a/internal/email/mute.go b/internal/email/mute.go index 1d797d853..04efe71a7 100644 --- a/internal/email/mute.go +++ b/internal/email/mute.go @@ -46,12 +46,17 @@ func filterMutedRecipients(ctx context.Context, mc MuteChecker, addrs []string, } // unsubscribeURLFor constructs the one-click unsubscribe URL for the given -// (email, scope) pair. Returns "" when baseURL is empty. +// (email, scope) pair. Returns "" when baseURL is empty or when no signing key +// is available (e.g. NOTIFICATION_MUTE_SECRET unset in production), so a +// tokenless, non-functional unsubscribe link is never emitted. func unsubscribeURLFor(baseURL, email, scope string) string { if baseURL == "" { return "" } token := common.DeriveMuteToken(muteKey(), email, scope) + if token == "" { + return "" + } q := url.Values{ "token": {token}, "email": {email}, diff --git a/internal/email/mute_test.go b/internal/email/mute_test.go index 3f7cd28ef..fce255e8e 100644 --- a/internal/email/mute_test.go +++ b/internal/email/mute_test.go @@ -133,6 +133,48 @@ func TestSendPurchaseApprovalRequest_MuteCheckError_FailOpen(t *testing.T) { require.NoError(t, err) } +// TestSendPurchaseApprovalRequest_WithCC_SuppressesListUnsubscribe verifies the +// List-Unsubscribe header (whose token is bound to the primary recipient) is NOT +// emitted when the message also goes to CC recipients. A shared-envelope CC +// recipient could otherwise one-click-mute the primary recipient. +func TestSendPurchaseApprovalRequest_WithCC_SuppressesListUnsubscribe(t *testing.T) { + ctx := context.Background() + ses := new(MockSESClient) + mc := new(mockMuteChecker) + + mc.On("IsNotificationMuted", mock.Anything, mock.Anything, mock.Anything). + Return(false, nil) + ses.On("GetAccount", mock.Anything, mock.Anything). + Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) + + var captured *sesv2.SendEmailInput + ses.On("SendEmail", mock.Anything, mock.MatchedBy(func(in *sesv2.SendEmailInput) bool { + captured = in + return true + })).Return(&sesv2.SendEmailOutput{}, nil) + t.Cleanup(func() { mc.AssertExpectations(t) }) + + s := newSenderWithMute(ses, mc).WithUnsubscribeBaseURL("https://dash.example.com") + data := NotificationData{ + RecipientEmail: "approver@example.com", + CCEmails: []string{"observer@example.com"}, + Recommendations: []RecommendationSummary{ + {Service: "ec2", Region: "us-east-1", Count: 1, MonthlySavings: 100}, + }, + DashboardURL: "https://dashboard.example.com", + ApprovalToken: "tok", + } + require.NoError(t, s.SendPurchaseApprovalRequest(ctx, data)) + require.NotNil(t, captured) + require.NotNil(t, captured.Content.Simple) + for _, h := range captured.Content.Simple.Headers { + if h.Name != nil { + assert.NotEqual(t, "List-Unsubscribe", *h.Name, + "List-Unsubscribe must be suppressed when CC recipients are present") + } + } +} + // --------------------------------------------------------------------------- // List-Unsubscribe header injection // --------------------------------------------------------------------------- diff --git a/internal/email/sender.go b/internal/email/sender.go index ccaf5edb5..db5036dfc 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -5,9 +5,9 @@ import ( "context" "errors" "fmt" - "os" "strings" + "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -128,14 +128,17 @@ func (s *Sender) WithUnsubscribeBaseURL(u string) *Sender { return &c } -// muteKey reads NOTIFICATION_MUTE_SECRET from env for token derivation. Returns -// nil when unset so DeriveMuteToken uses the dev fallback. +// muteKey resolves the NOTIFICATION_MUTE_SECRET HMAC key via the shared +// fail-closed policy (common.ResolveMuteSecret). In production a missing secret +// yields a nil key (and an error), so the send path emits no List-Unsubscribe +// header rather than a forgeable one; non-production falls back to the dev key. func muteKey() []byte { - v := os.Getenv("NOTIFICATION_MUTE_SECRET") - if v == "" { + key, err := common.ResolveMuteSecret() + if err != nil { + logging.Warnf("email: %v; List-Unsubscribe header suppressed", err) return nil } - return []byte(v) + return key } // buildUnsubscribeURL constructs the one-click unsubscribe URL for the given diff --git a/internal/email/smtp_mute_test.go b/internal/email/smtp_mute_test.go index e204c3bea..51b935c01 100644 --- a/internal/email/smtp_mute_test.go +++ b/internal/email/smtp_mute_test.go @@ -82,6 +82,35 @@ func TestSMTPSender_PurchaseApproval_EmitsListUnsubscribe(t *testing.T) { "SMTP approval send must carry a List-Unsubscribe-Post header") } +// TestSMTPSender_PurchaseApproval_WithCC_SuppressesListUnsubscribe verifies the +// SMTP transport suppresses the primary-recipient-bound List-Unsubscribe header +// when CC recipients share the envelope, mirroring the SES path. +func TestSMTPSender_PurchaseApproval_WithCC_SuppressesListUnsubscribe(t *testing.T) { + server := newMockSMTPServer(t, false) + server.start(t) + defer server.stop() + + base := &SMTPSender{ + host: "127.0.0.1", + port: server.port, + fromEmail: "sender@test.com", + useTLS: false, + } + mc := &wiringMuteCheckerSMTP{muted: map[string]bool{}} + sender := base.WithMuteChecker(mc).WithUnsubscribeBaseURL("https://dash.example.com") + + data := smtpApprovalData("approver@test.com") + data.CCEmails = []string{"observer@test.com"} + require.NoError(t, sender.SendPurchaseApprovalRequest(context.Background(), data)) + + server.stop() + server.mu.Lock() + got := server.receivedMsg + server.mu.Unlock() + assert.NotContains(t, got, "List-Unsubscribe:", + "List-Unsubscribe must be suppressed when CC recipients are present") +} + type wiringMuteCheckerSMTP struct { muted map[string]bool } diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index 568f1004e..3bb3669fd 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -532,7 +532,15 @@ func (s *SMTPSender) SendPurchaseApprovalRequest(ctx context.Context, data Notif filteredCC := filterMutedRecipients(ctx, s.muteChecker, data.CCEmails, scope) // Build RFC 8058 List-Unsubscribe headers scoped to the primary recipient. - unsubHdr, postHdr := unsubscribeHeaderValuesFor(s.unsubscribeBaseURL, recipient, scope) + // The token in the header is bound to recipient only, but the SMTP envelope + // delivers a single shared message to recipient + all CC addresses. Emitting + // the header when CC recipients exist would let any of them one-click-mute + // the primary recipient (an authorization-boundary violation), so suppress + // the header entirely whenever there is a CC list. + var unsubHdr, postHdr string + if len(filteredCC) == 0 { + unsubHdr, postHdr = unsubscribeHeaderValuesFor(s.unsubscribeBaseURL, recipient, scope) + } subject := fmt.Sprintf("CUDly - Purchase Approval Required (%d commitment(s))", len(data.Recommendations)) @@ -580,9 +588,9 @@ func (s *SMTPSender) sendMultipartWithUnsubscribe(ctx context.Context, toEmail s } if len(sanitizedCC) > 0 { - logging.Debugf("Sent approval email via SMTP to %s (cc %d): %s", toEmail, len(sanitizedCC), subject) + logging.Debugf("Sent approval email via SMTP to %s (cc %d): %s", redactEmail(toEmail), len(sanitizedCC), subject) } else { - logging.Debugf("Sent approval email via SMTP to %s: %s", toEmail, subject) + logging.Debugf("Sent approval email via SMTP to %s: %s", redactEmail(toEmail), subject) } return nil } diff --git a/internal/email/templates.go b/internal/email/templates.go index 1e158f856..5dab0cafb 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -844,8 +844,16 @@ func (s *Sender) SendPurchaseApprovalRequest(ctx context.Context, data Notificat filteredCC := s.filterMutedAddresses(ctx, data.CCEmails, scope) // Build RFC 8058 List-Unsubscribe headers scoped to the primary recipient. - unsubHdr, postHdr := s.listUnsubscribeHeaders(data.RecipientEmail, scope) - extraHeaders := addListUnsubscribeHeaders(unsubHdr, postHdr) + // The token in the header is bound to RecipientEmail only, but a SES message + // with a CC list delivers one shared message to all addresses. Emitting the + // header when CC recipients exist would let any of them one-click-mute the + // primary recipient (an authorization-boundary violation), so suppress the + // header entirely whenever there is a CC list. + var extraHeaders []types.MessageHeader + if len(filteredCC) == 0 { + unsubHdr, postHdr := s.listUnsubscribeHeaders(data.RecipientEmail, scope) + extraHeaders = addListUnsubscribeHeaders(unsubHdr, postHdr) + } subject := fmt.Sprintf("CUDly - Purchase Approval Required (%d commitment(s))", len(data.Recommendations)) return sendPurchaseApprovalRequestWithCC(ctx, s, data.RecipientEmail, filteredCC, subject, data, extraHeaders) diff --git a/pkg/common/tokens.go b/pkg/common/tokens.go index 07d567682..53c7176ff 100644 --- a/pkg/common/tokens.go +++ b/pkg/common/tokens.go @@ -5,10 +5,43 @@ import ( "crypto/rand" "crypto/sha256" "encoding/hex" + "errors" "fmt" + "os" "strings" ) +// muteSecretEnvVar is the environment variable holding the HMAC key used to +// sign notification mute / List-Unsubscribe tokens. +const muteSecretEnvVar = "NOTIFICATION_MUTE_SECRET" + +// devMuteSecret is the deterministic fallback key used ONLY in non-production +// environments so local-dev and tests produce stable tokens without requiring +// an env var. It is intentionally well-known and MUST NOT be relied on in +// production: ResolveMuteSecret fails closed there instead of using it. +const devMuteSecret = "dev-mute-secret-not-for-production" + +// ErrMuteSecretMissing is returned by ResolveMuteSecret when running in a +// production environment with NOTIFICATION_MUTE_SECRET unset. Falling back to a +// well-known key in production would make unsubscribe tokens forgeable for any +// (email, scope) tuple, so the caller must fail closed. +var ErrMuteSecretMissing = errors.New("common: NOTIFICATION_MUTE_SECRET is required in production") + +// ResolveMuteSecret returns the HMAC key for notification mute tokens, applying +// a fail-closed policy: when NOTIFICATION_MUTE_SECRET is set its bytes are +// returned in every environment; when unset, non-production environments get +// the deterministic dev fallback while production (ENVIRONMENT=production) +// returns ErrMuteSecretMissing rather than silently using a forgeable key. +func ResolveMuteSecret() ([]byte, error) { + if v := os.Getenv(muteSecretEnvVar); v != "" { + return []byte(v), nil + } + if os.Getenv("ENVIRONMENT") == "production" { + return nil, ErrMuteSecretMissing + } + return []byte(devMuteSecret), nil +} + // GenerateApprovalToken returns a 32-byte cryptographically secure random // token, hex-encoded (64 chars). Used for purchase + RI exchange + plan // approval flows where the token is the only credential in a one-click @@ -114,14 +147,14 @@ const ( // List-Unsubscribe URL; the handler re-derives it from the query params and // compares in constant time, so a forged URL cannot mute a different address. // -// key must come from a deployment secret (NOTIFICATION_MUTE_SECRET env var). -// When key is empty a static fallback is used so local-dev / test environments -// still produce a deterministic token without crashing; production deployments -// MUST set the env var. +// key must be resolved by the caller via ResolveMuteSecret, which applies the +// fail-closed production policy. An empty key is treated as a configuration +// error and yields the empty string so the caller emits no usable token (and a +// later VerifyMuteToken comparison against it fails), rather than silently +// signing with a well-known fallback. func DeriveMuteToken(key []byte, email, scope string) string { if len(key) == 0 { - // Fallback for local dev / tests: deterministic but clearly insecure. - key = []byte("dev-mute-secret-not-for-production") + return "" } mac := hmac.New(sha256.New, key) mac.Write([]byte(strings.ToLower(email))) @@ -131,8 +164,12 @@ func DeriveMuteToken(key []byte, email, scope string) string { } // VerifyMuteToken returns true when token equals the HMAC for (email, scope) -// under key. Comparison is constant-time to prevent timing attacks. +// under key. Comparison is constant-time to prevent timing attacks. A non-empty +// token never matches when key is empty, so a missing secret fails closed. func VerifyMuteToken(key []byte, email, scope, token string) bool { want := DeriveMuteToken(key, email, scope) + if want == "" { + return false + } return hmac.Equal([]byte(want), []byte(token)) } diff --git a/pkg/common/tokens_test.go b/pkg/common/tokens_test.go index a03fb91fd..4da1cd93c 100644 --- a/pkg/common/tokens_test.go +++ b/pkg/common/tokens_test.go @@ -125,3 +125,43 @@ func TestReservationOrderID(t *testing.T) { assert.Equal(t, "fallback-id", ReservationOrderID("", "fallback-id")) assert.Equal(t, "fallback-id", ReservationOrderID("abc", "fallback-id")) } + +func TestDeriveMuteToken_EmptyKeyReturnsEmpty(t *testing.T) { + // An empty key is a configuration error: the function must not silently sign + // with a well-known fallback, so it yields the empty string. + assert.Empty(t, DeriveMuteToken(nil, "user@example.com", "purchase_approvals")) + assert.Empty(t, DeriveMuteToken([]byte{}, "user@example.com", "purchase_approvals")) +} + +func TestVerifyMuteToken_EmptyKeyFailsClosed(t *testing.T) { + // With no key, verification of any non-empty token must fail (fail closed) so + // a missing secret cannot be exploited to accept a forged token. Crucially, + // even passing the empty string DeriveMuteToken(nil,...) returns must NOT + // verify true. + assert.False(t, VerifyMuteToken(nil, "user@example.com", "purchase_approvals", "anytoken")) + assert.False(t, VerifyMuteToken(nil, "user@example.com", "purchase_approvals", "")) +} + +func TestResolveMuteSecret_EnvSetWins(t *testing.T) { + t.Setenv("NOTIFICATION_MUTE_SECRET", "the-real-secret") + t.Setenv("ENVIRONMENT", "production") + key, err := ResolveMuteSecret() + require.NoError(t, err) + assert.Equal(t, []byte("the-real-secret"), key) +} + +func TestResolveMuteSecret_ProductionMissingFailsClosed(t *testing.T) { + t.Setenv("NOTIFICATION_MUTE_SECRET", "") + t.Setenv("ENVIRONMENT", "production") + key, err := ResolveMuteSecret() + require.ErrorIs(t, err, ErrMuteSecretMissing) + assert.Nil(t, key) +} + +func TestResolveMuteSecret_NonProductionFallsBackToDevKey(t *testing.T) { + t.Setenv("NOTIFICATION_MUTE_SECRET", "") + t.Setenv("ENVIRONMENT", "") + key, err := ResolveMuteSecret() + require.NoError(t, err) + assert.Equal(t, []byte(devMuteSecret), key) +} From b92ae0456efcceb29fe191066ee4025d4fe8cc31 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 11 Jul 2026 00:09:48 +0200 Subject: [PATCH 06/10] fix(notifications): defer mute-sender wiring to reinitializeAfterConnect NewApplication no longer has a config store at startup (initConfigStore was refactored on main to return only dbConfig+secretResolver for lazy DB init). Move the decorateSenderWithMute call into reinitializeAfterConnect where pgStore is live, so the mute checker is wired once the DB connection is established rather than at cold-start with a nil store. --- internal/mocks/stores.go | 1 - internal/server/app.go | 12 +++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 38e8b80d3..2eef764ec 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1390,7 +1390,6 @@ func (m *MockConfigStore) UpsertLadderConfig(ctx context.Context, cfg *config.La return v, args.Error(1) } -<<<<<<< HEAD // SaveLadderRun mocks the SaveLadderRun operation. // Returns (nil, nil) when no expectation is registered. func (m *MockConfigStore) SaveLadderRun(ctx context.Context, run *config.LadderRunDB) (*config.LadderRunDB, error) { diff --git a/internal/server/app.go b/internal/server/app.go index e9264ab6d..975bc0770 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -548,13 +548,6 @@ func NewApplication(ctx context.Context, version string) (*Application, error) { if err != nil { return nil, fmt.Errorf("failed to initialize email sender: %w", err) } - // Wire per-recipient mute suppression + List-Unsubscribe into the production - // sender. Without this the mute-aware send logic in the email package is dead - // code (the bare sender has a nil mute checker and no unsubscribe base URL). - // The dashboard base URL is sourced from the same DASHBOARD_URL the email - // templates already use, trimmed of a trailing slash to match resolveOIDCIssuerURL. - emailSender = decorateSenderWithMute(emailSender, configStore, strings.TrimRight(cfg.DashboardURL, "/")) - // Initialize AWS config for STS client awsCfg, err := awsconfig.LoadDefaultConfig(ctx) if err != nil { @@ -723,6 +716,11 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da return fmt.Errorf("failed to create PostgreSQL config store") } app.Config = pgStore + // Wire per-recipient mute suppression + List-Unsubscribe now that the config + // store (which implements email.MuteChecker via IsNotificationMuted) is live. + // Moving this here from NewApplication is correct: mute lookups require DB, + // so decorating before the store is available would panic on the first call. + app.Email = decorateSenderWithMute(app.Email, pgStore, strings.TrimRight(app.appConfig.DashboardURL, "/")) // Initialize auth store with the connection authStore := auth.NewPostgresStore(dbConn) From 8df363a04bffad818820bf81285468016916d5df Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 20:10:53 +0200 Subject: [PATCH 07/10] fix(lint): gocritic/unparam/unused + migration renumber in notifications PR Address four linter findings in the mute/unsubscribe feature: - smtp_sender: use %q instead of escaped-quote "%s" (sprintfQuotedString) - sender: remove unused mailtoURL return from buildUnsubscribeURL (unparam) - templates: remove dead sendPurchaseApprovalRequestVia function (unused) - migration: renumber 000078_muted_recipients to 000091 to resolve conflict with 000078_monthly_summary_nested_rollup already on main --- internal/config/interfaces.go | 2 +- ...n.sql => 000091_muted_recipients.down.sql} | 0 ....up.sql => 000091_muted_recipients.up.sql} | 0 internal/email/mute_test.go | 4 ++-- internal/email/sender.go | 6 ++--- internal/email/smtp_sender.go | 2 +- internal/email/templates.go | 24 ------------------- 7 files changed, 7 insertions(+), 31 deletions(-) rename internal/database/postgres/migrations/{000078_muted_recipients.down.sql => 000091_muted_recipients.down.sql} (100%) rename internal/database/postgres/migrations/{000078_muted_recipients.up.sql => 000091_muted_recipients.up.sql} (100%) diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 8a3b65db8..bd7bc3317 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -437,7 +437,7 @@ type StoreInterface interface { LatestLadderRunStartedAt(ctx context.Context, configID string) (*time.Time, error) TransitionLadderRunStatus(ctx context.Context, id string, fromStatuses []ladder.RunStatus, toStatus ladder.RunStatus) (*LadderRunDB, error) - // Notification mutes (issue #297 / migration 000078). + // Notification mutes (issue #297 / migration 000091). // UpsertNotificationMute inserts or updates a mute row for (email, scope). // Idempotent for row existence: calling it again for an already-muted // address refreshes muted_at and replaces unmute_token if the token changes. diff --git a/internal/database/postgres/migrations/000078_muted_recipients.down.sql b/internal/database/postgres/migrations/000091_muted_recipients.down.sql similarity index 100% rename from internal/database/postgres/migrations/000078_muted_recipients.down.sql rename to internal/database/postgres/migrations/000091_muted_recipients.down.sql diff --git a/internal/database/postgres/migrations/000078_muted_recipients.up.sql b/internal/database/postgres/migrations/000091_muted_recipients.up.sql similarity index 100% rename from internal/database/postgres/migrations/000078_muted_recipients.up.sql rename to internal/database/postgres/migrations/000091_muted_recipients.up.sql diff --git a/internal/email/mute_test.go b/internal/email/mute_test.go index fce255e8e..965d561a0 100644 --- a/internal/email/mute_test.go +++ b/internal/email/mute_test.go @@ -181,13 +181,13 @@ func TestSendPurchaseApprovalRequest_WithCC_SuppressesListUnsubscribe(t *testing func TestBuildUnsubscribeURL_EmptyBaseURL_ReturnsEmpty(t *testing.T) { s := &Sender{} - u, _ := s.buildUnsubscribeURL("user@example.com", "purchase_approvals") + u := s.buildUnsubscribeURL("user@example.com", "purchase_approvals") assert.Empty(t, u) } func TestBuildUnsubscribeURL_WithBaseURL_ContainsParams(t *testing.T) { s := &Sender{unsubscribeBaseURL: "https://dash.example.com"} - u, _ := s.buildUnsubscribeURL("user@example.com", "purchase_approvals") + u := s.buildUnsubscribeURL("user@example.com", "purchase_approvals") assert.Contains(t, u, "email=user%40example.com") assert.Contains(t, u, "scope=purchase_approvals") assert.Contains(t, u, "token=") diff --git a/internal/email/sender.go b/internal/email/sender.go index db5036dfc..304cbb479 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -142,9 +142,9 @@ func muteKey() []byte { } // buildUnsubscribeURL constructs the one-click unsubscribe URL for the given -// (email, scope) pair. Returns ("", "") when unsubscribeBaseURL is empty. -func (s *Sender) buildUnsubscribeURL(email, scope string) (unsubURL, mailtoURL string) { - return unsubscribeURLFor(s.unsubscribeBaseURL, email, scope), "" +// (email, scope) pair. Returns "" when unsubscribeBaseURL is empty. +func (s *Sender) buildUnsubscribeURL(email, scope string) string { + return unsubscribeURLFor(s.unsubscribeBaseURL, email, scope) } // listUnsubscribeHeaders returns the List-Unsubscribe and List-Unsubscribe-Post diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index 3bb3669fd..d1a240ff2 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -248,7 +248,7 @@ func (s *SMTPSender) buildSMTPMessageMultipartWithHeaders(toEmail string, cc []s if len(cc) > 0 { headers += fmt.Sprintf("Cc: %s\r\n", strings.Join(cc, ", ")) } - headers += fmt.Sprintf("Subject: %s\r\nMIME-Version: 1.0\r\nContent-Type: multipart/alternative; boundary=\"%s\"\r\n", subject, boundary) + headers += fmt.Sprintf("Subject: %s\r\nMIME-Version: 1.0\r\nContent-Type: multipart/alternative; boundary=%q\r\n", subject, boundary) headers += extraHeaders headers += "\r\n" diff --git a/internal/email/templates.go b/internal/email/templates.go index 5dab0cafb..4e02e2e1d 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -759,30 +759,6 @@ const purchaseApprovalRequestHTMLTemplate = ` ` -// sendPurchaseApprovalRequestVia composes the plain-text + HTML approval-request -// bodies and ships them through s.SendToEmailWithCCMultipart. HTML render -// failures are non-fatal and degrade to single-part text so a template bug -// never drops the approval email. Shared by Sender and SMTPSender — see -// issue #287 / PR #298 dedup follow-up. -func sendPurchaseApprovalRequestVia(ctx context.Context, s SenderInterface, recipient, subject string, data NotificationData) error { - textBody, err := RenderPurchaseApprovalRequestEmail(data) - if err != nil { - return fmt.Errorf("failed to render purchase approval request email (text): %w", err) - } - // HTML render failure is non-fatal: degrade to single-part text. - // SendToEmailWithCCMultipart already handles htmlBody=="" by delegating - // to the single-part path on each transport. - htmlBody, htmlErr := RenderPurchaseApprovalRequestEmailHTML(data) - if htmlErr != nil { - // Surface the render failure for production diagnosis. We deliberately - // don't return — text-only delivery is the safer fallback than dropping - // the approval email entirely. - logging.Warnf("email: HTML approval-request render failed, falling back to text-only: %v", htmlErr) - htmlBody = "" - } - return s.SendToEmailWithCCMultipart(ctx, recipient, data.CCEmails, subject, textBody, htmlBody) -} - // sendMultipartVia is the generic dual-render send helper used by the // invite / password-reset / welcome flows. The two render closures are // invoked back-to-back; an HTML render failure is non-fatal and degrades From aef1850b1a8fa49656d12e9d15d8a84dcec23ded Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 20 Jul 2026 01:31:07 +0200 Subject: [PATCH 08/10] fix(notifications): harden one-click unsubscribe delivery - accept and validate RFC 8058 POST requests using signed query tuples - apply RI exchange mute scopes and unsubscribe headers in SES and SMTP - require an explicit notification mute secret in every environment --- .env.example | 3 + internal/api/handler_notifications.go | 52 ++++++++++----- internal/api/handler_notifications_test.go | 75 +++++++++++++++++++--- internal/api/router.go | 1 + internal/email/mute.go | 16 ++++- internal/email/mute_test.go | 61 ++++++++++++++++++ internal/email/sender.go | 6 +- internal/email/smtp_mute_test.go | 73 +++++++++++++++++++++ internal/email/smtp_sender.go | 34 +++++----- internal/email/templates.go | 50 +++++++-------- internal/server/app_mute_wiring_test.go | 1 + pkg/common/tokens.go | 31 +++------ pkg/common/tokens_test.go | 23 +++---- 13 files changed, 321 insertions(+), 105 deletions(-) diff --git a/.env.example b/.env.example index 8cea9a216..196f90174 100644 --- a/.env.example +++ b/.env.example @@ -69,6 +69,9 @@ ADMIN_PASSWORD_SECRET=ADMIN_PASSWORD_DEV ADMIN_PASSWORD_DEV=LocalDev!Pass123 API_KEY_SECRET_ARN=ADMIN_API_KEY_DEV ADMIN_API_KEY_DEV=cudly-local-dev-api-key-not-for-prod +# Required for signed one-click notification unsubscribe links. Generate a +# unique value for every environment (for example: openssl rand -hex 32). +NOTIFICATION_MUTE_SECRET= # Production examples (override SECRET_PROVIDER and these): # ADMIN_PASSWORD_SECRET=arn:aws:secretsmanager:us-east-1:000000000000:secret:cudly-admin-password-PLACEHOLDER # API_KEY_SECRET_ARN=arn:aws:secretsmanager:us-east-1:000000000000:secret:cudly-api-key-PLACEHOLDER diff --git a/internal/api/handler_notifications.go b/internal/api/handler_notifications.go index 5f489f447..1352d3f5e 100644 --- a/internal/api/handler_notifications.go +++ b/internal/api/handler_notifications.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "html/template" + "net/url" "strings" "github.com/LeanerCloud/CUDly/pkg/common" @@ -51,30 +52,49 @@ func scopeLabel(scope string) string { } } -// unsubscribeHandler handles GET /api/notifications/unsubscribe. +func validateOneClickUnsubscribeBody(req *events.LambdaFunctionURLRequest) error { + if req.RequestContext.HTTP.Method != "POST" { + return nil + } + values, err := url.ParseQuery(req.Body) + if err != nil || len(values) != 1 || len(values["List-Unsubscribe"]) != 1 || + values.Get("List-Unsubscribe") != "One-Click" { + return NewClientError(400, "invalid one-click unsubscribe body") + } + return nil +} + +func unsubscribeRequestParams(req *events.LambdaFunctionURLRequest) (token, email, scope string, err error) { + token = req.QueryStringParameters["token"] + email = req.QueryStringParameters["email"] + scope = req.QueryStringParameters["scope"] + if token == "" || email == "" || scope == "" { + return "", "", "", NewClientError(400, "token, email and scope are required") + } + if scope != string(common.ScopePurchaseApprovals) && scope != string(common.ScopeRIExchangeApprovals) { + return "", "", "", NewClientError(400, fmt.Sprintf("unknown notification scope: %s", scope)) + } + return token, email, scope, nil +} + +// unsubscribeHandler handles GET and RFC 8058 POST requests to +// /api/notifications/unsubscribe. // The URL carries a signed token that encodes (email, scope); the handler // verifies the HMAC, upserts the mute row, and returns a confirmation page. // // Auth: AuthPublic (token-based, no login required — mirrors approve/cancel). func (h *Handler) unsubscribeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { - token := req.QueryStringParameters["token"] - email := req.QueryStringParameters["email"] - scope := req.QueryStringParameters["scope"] - - if token == "" || email == "" || scope == "" { - return nil, NewClientError(400, "token, email and scope are required") + if err := validateOneClickUnsubscribeBody(req); err != nil { + return nil, err } - - // Reject unknown scopes early so we never create phantom rows. - validScope := scope == string(common.ScopePurchaseApprovals) || - scope == string(common.ScopeRIExchangeApprovals) - if !validScope { - return nil, NewClientError(400, fmt.Sprintf("unknown notification scope: %s", scope)) + token, email, scope, err := unsubscribeRequestParams(req) + if err != nil { + return nil, err } - // Resolve the HMAC key with the fail-closed production policy: a missing - // NOTIFICATION_MUTE_SECRET in production is a server misconfiguration, not a - // client error, and must never silently verify against a well-known key. + // Resolve the HMAC key with the fail-closed policy: a missing + // NOTIFICATION_MUTE_SECRET is a server misconfiguration, not a client error, + // and must never silently verify against a well-known key. key, err := common.ResolveMuteSecret() if err != nil { logging.Errorf("notifications/unsubscribe: %v", err) diff --git a/internal/api/handler_notifications_test.go b/internal/api/handler_notifications_test.go index 57f299182..aaa524fa1 100644 --- a/internal/api/handler_notifications_test.go +++ b/internal/api/handler_notifications_test.go @@ -18,8 +18,8 @@ import ( func validUnsubToken(t *testing.T, email, scope string) string { t.Helper() - // Resolve the key the same way the handler does so the generated token - // verifies. With ENVIRONMENT unset this yields the deterministic dev key. + t.Setenv("NOTIFICATION_MUTE_SECRET", "handler-notifications-test-secret") + // Resolve the key the same way the handler does so the generated token verifies. key, err := common.ResolveMuteSecret() require.NoError(t, err) return common.DeriveMuteToken(key, email, scope) @@ -54,7 +54,36 @@ func TestUnsubscribeHandler_Success(t *testing.T) { assert.Contains(t, raw.body, "purchase approval request") } +func TestUnsubscribeHandler_POSTRejectsInvalidOneClickBody(t *testing.T) { + ctx := context.Background() + email := "user@example.com" + scope := string(common.ScopePurchaseApprovals) + token := validUnsubToken(t, email, scope) + + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertNotCalled(t, "UpsertNotificationMute") }) + h := &Handler{config: mockStore} + + req := &events.LambdaFunctionURLRequest{ + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, + }, + QueryStringParameters: map[string]string{ + "token": token, + "email": email, + "scope": scope, + }, + Body: "List-Unsubscribe=Not-One-Click", + } + _, err := h.unsubscribeHandler(ctx, req, nil) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) +} + func TestUnsubscribeHandler_ForgedToken_Returns401(t *testing.T) { + t.Setenv("NOTIFICATION_MUTE_SECRET", "handler-forged-token-test-secret") ctx := context.Background() req := &events.LambdaFunctionURLRequest{ QueryStringParameters: map[string]string{ @@ -73,12 +102,10 @@ func TestUnsubscribeHandler_ForgedToken_Returns401(t *testing.T) { assert.Equal(t, 401, ce.code) } -func TestUnsubscribeHandler_ProductionMissingSecret_FailsClosed(t *testing.T) { - // With ENVIRONMENT=production and no NOTIFICATION_MUTE_SECRET, the handler - // must NOT verify against a well-known dev key (which would accept forged - // tokens). It returns a server-side error (500), never a 401/200, and never - // reaches the store. - t.Setenv("ENVIRONMENT", "production") +func TestUnsubscribeHandler_MissingSecret_FailsClosed(t *testing.T) { + // A missing NOTIFICATION_MUTE_SECRET must fail closed in every environment. + // It returns a server-side error (500), never a 401/200, and never reaches + // the store. t.Setenv("NOTIFICATION_MUTE_SECRET", "") ctx := context.Background() @@ -214,6 +241,7 @@ func TestIsPublicEndpoint_UnsubscribePath(t *testing.T) { // --------------------------------------------------------------------------- func TestRouter_UnsubscribeRoute_Registered(t *testing.T) { + t.Setenv("NOTIFICATION_MUTE_SECRET", "router-unsubscribe-test-secret") // Verify the route is wired: an unsigned token returns 401, which can only // happen if the router dispatched to the correct handler. ctx := context.Background() @@ -239,3 +267,34 @@ func TestRouter_UnsubscribeRoute_Registered(t *testing.T) { require.True(t, ok) assert.Equal(t, 401, ce.code) } + +func TestRouter_UnsubscribePOSTRoute_MutesSignedRecipientAndScope(t *testing.T) { + ctx := context.Background() + email := "ri-approver@example.com" + scope := string(common.ScopeRIExchangeApprovals) + token := validUnsubToken(t, email, scope) + + mockStore := new(MockConfigStore) + mockStore.On("UpsertNotificationMute", ctx, email, scope, token).Return(nil).Once() + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + r := NewRouter(&Handler{config: mockStore}) + + req := &events.LambdaFunctionURLRequest{ + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "POST", + Path: "/api/notifications/unsubscribe", + }, + }, + QueryStringParameters: map[string]string{ + "token": token, + "email": email, + "scope": scope, + }, + Body: "List-Unsubscribe=One-Click", + } + result, err := r.Route(ctx, "POST", "/api/notifications/unsubscribe", req) + require.NoError(t, err) + _, ok := result.(*rawResponse) + require.True(t, ok) +} diff --git a/internal/api/router.go b/internal/api/router.go index 47f538b76..22455cd61 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -323,6 +323,7 @@ func (r *Router) registerRoutes() { // Notification one-click unsubscribe (RFC 8058). AuthPublic: the signed // token in the query string is the credential (mirrors approve/cancel). {ExactPath: "/api/notifications/unsubscribe", Method: "GET", Handler: r.unsubscribeHandler, Auth: AuthPublic}, + {ExactPath: "/api/notifications/unsubscribe", Method: "POST", Handler: r.unsubscribeHandler, Auth: AuthPublic}, // Account self-registration (public, called by Terraform during federation IaC apply) {ExactPath: "/api/register", Method: "POST", Handler: r.submitRegistrationHandler, Auth: AuthPublic}, diff --git a/internal/email/mute.go b/internal/email/mute.go index 04efe71a7..34ca20343 100644 --- a/internal/email/mute.go +++ b/internal/email/mute.go @@ -45,9 +45,23 @@ func filterMutedRecipients(ctx context.Context, mc MuteChecker, addrs []string, return out } +// prepareMuteAwareDelivery applies the shared approval-email mute policy and +// returns the filtered CC list plus RFC 8058 header values. Headers are omitted +// for shared envelopes because their token is bound to the primary recipient. +func prepareMuteAwareDelivery(ctx context.Context, mc MuteChecker, baseURL, recipient string, cc []string, scope string) (filteredCC []string, headerValue, postValue string, muted bool) { + if isRecipientMuted(ctx, mc, recipient, scope) { + return nil, "", "", true + } + filteredCC = filterMutedRecipients(ctx, mc, cc, scope) + if len(filteredCC) == 0 { + headerValue, postValue = unsubscribeHeaderValuesFor(baseURL, recipient, scope) + } + return filteredCC, headerValue, postValue, false +} + // unsubscribeURLFor constructs the one-click unsubscribe URL for the given // (email, scope) pair. Returns "" when baseURL is empty or when no signing key -// is available (e.g. NOTIFICATION_MUTE_SECRET unset in production), so a +// is available (e.g. NOTIFICATION_MUTE_SECRET is unset), so a // tokenless, non-functional unsubscribe link is never emitted. func unsubscribeURLFor(baseURL, email, scope string) string { if baseURL == "" { diff --git a/internal/email/mute_test.go b/internal/email/mute_test.go index 965d561a0..3eaf9937d 100644 --- a/internal/email/mute_test.go +++ b/internal/email/mute_test.go @@ -20,6 +20,11 @@ type mockMuteChecker struct { mock.Mock } +func setMuteTestSecret(t *testing.T) { + t.Helper() + t.Setenv("NOTIFICATION_MUTE_SECRET", "email-mute-test-secret") +} + func (m *mockMuteChecker) IsNotificationMuted(ctx context.Context, email, scope string) (bool, error) { args := m.Called(ctx, email, scope) return args.Bool(0), args.Error(1) @@ -133,6 +138,60 @@ func TestSendPurchaseApprovalRequest_MuteCheckError_FailOpen(t *testing.T) { require.NoError(t, err) } +func TestSendRIExchangePendingApproval_MutedRecipient_NoSESCall(t *testing.T) { + ctx := context.Background() + ses := new(MockSESClient) + mc := new(mockMuteChecker) + mc.On("IsNotificationMuted", mock.Anything, "ri-approver@example.com", string(common.ScopeRIExchangeApprovals)). + Return(true, nil).Once() + t.Cleanup(func() { + mc.AssertExpectations(t) + ses.AssertNotCalled(t, "SendEmail") + }) + + s := newSenderWithMute(ses, mc) + err := s.SendRIExchangePendingApproval(ctx, RIExchangeNotificationData{ + RecipientEmail: "ri-approver@example.com", + DashboardURL: "https://dash.example.com", + Exchanges: []RIExchangeItem{{RecordID: "rec-1", ApprovalToken: "tok"}}, + }) + require.NoError(t, err) +} + +func TestSendRIExchangePendingApproval_EmitsScopedUnsubscribeHeaders(t *testing.T) { + setMuteTestSecret(t) + ctx := context.Background() + ses := new(MockSESClient) + mc := new(mockMuteChecker) + mc.On("IsNotificationMuted", mock.Anything, "ri-approver@example.com", string(common.ScopeRIExchangeApprovals)). + Return(false, nil).Once() + + var captured *sesv2.SendEmailInput + ses.On("SendEmail", mock.Anything, mock.MatchedBy(func(in *sesv2.SendEmailInput) bool { + captured = in + return true + })).Return(&sesv2.SendEmailOutput{}, nil).Once() + t.Cleanup(func() { + mc.AssertExpectations(t) + ses.AssertExpectations(t) + }) + + s := newSenderWithMute(ses, mc).WithUnsubscribeBaseURL("https://dash.example.com") + err := s.SendRIExchangePendingApproval(ctx, RIExchangeNotificationData{ + RecipientEmail: "ri-approver@example.com", + DashboardURL: "https://dash.example.com", + Exchanges: []RIExchangeItem{{RecordID: "rec-1", ApprovalToken: "tok"}}, + }) + require.NoError(t, err) + require.NotNil(t, captured) + require.NotNil(t, captured.Content.Simple) + require.Len(t, captured.Content.Simple.Headers, 2) + assert.Equal(t, "List-Unsubscribe", *captured.Content.Simple.Headers[0].Name) + assert.Contains(t, *captured.Content.Simple.Headers[0].Value, "scope="+string(common.ScopeRIExchangeApprovals)) + assert.NotContains(t, *captured.Content.Simple.Headers[0].Value, "scope="+string(common.ScopePurchaseApprovals)) + assert.Equal(t, "List-Unsubscribe=One-Click", *captured.Content.Simple.Headers[1].Value) +} + // TestSendPurchaseApprovalRequest_WithCC_SuppressesListUnsubscribe verifies the // List-Unsubscribe header (whose token is bound to the primary recipient) is NOT // emitted when the message also goes to CC recipients. A shared-envelope CC @@ -186,6 +245,7 @@ func TestBuildUnsubscribeURL_EmptyBaseURL_ReturnsEmpty(t *testing.T) { } func TestBuildUnsubscribeURL_WithBaseURL_ContainsParams(t *testing.T) { + setMuteTestSecret(t) s := &Sender{unsubscribeBaseURL: "https://dash.example.com"} u := s.buildUnsubscribeURL("user@example.com", "purchase_approvals") assert.Contains(t, u, "email=user%40example.com") @@ -201,6 +261,7 @@ func TestListUnsubscribeHeaders_EmptyBase_ReturnsEmpty(t *testing.T) { } func TestListUnsubscribeHeaders_WithBase(t *testing.T) { + setMuteTestSecret(t) s := &Sender{unsubscribeBaseURL: "https://dash.example.com"} hdr, post := s.listUnsubscribeHeaders("u@e.com", "purchase_approvals") assert.Contains(t, hdr, " Date: Tue, 21 Jul 2026 17:55:20 +0200 Subject: [PATCH 09/10] fix(email): remove unused isMuted/filterMutedAddresses wrappers; name unnamedResult golangci v2.10.1 flagged two unused methods on *Sender (isMuted, filterMutedAddresses) and an unnamedResult in renderRIExchangePendingApproval. The two methods were thin wrappers around package-level functions that are already used directly; removing them avoids the dead code. The return tuple is now named (textBody, htmlBody string, err error) and the first assignment changes from := to = accordingly. --- internal/email/sender.go | 15 --------------- internal/email/templates.go | 4 ++-- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/internal/email/sender.go b/internal/email/sender.go index 54cdbc4b9..bff8d0462 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -154,21 +154,6 @@ func (s *Sender) listUnsubscribeHeaders(email, scope string) (headerValue, postV return unsubscribeHeaderValuesFor(s.unsubscribeBaseURL, email, scope) } -// isMuted returns true when the given address is muted for this scope. When the -// mute checker is nil or returns an error the address is treated as not muted so -// a transient DB outage doesn't silently block approval emails. -func (s *Sender) isMuted(ctx context.Context, email, scope string) bool { - return isRecipientMuted(ctx, s.muteChecker, email, scope) -} - -// filterMutedAddresses returns a copy of addrs with any muted (for scope) -// entries removed. The original slice is not modified. Errors from the mute -// store are treated as "not muted" (fail-open) so a DB hiccup does not -// silently suppress approval emails. -func (s *Sender) filterMutedAddresses(ctx context.Context, addrs []string, scope string) []string { - return filterMutedRecipients(ctx, s.muteChecker, addrs, scope) -} - // snsMaxSubjectLen is the maximum byte length SNS accepts for a Subject. // Subjects longer than 100 bytes are rejected with InvalidParameter at runtime. const snsMaxSubjectLen = 100 diff --git a/internal/email/templates.go b/internal/email/templates.go index 765f5bf8d..96aaa4a8f 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -525,8 +525,8 @@ func (s *Sender) SendUserInviteEmail(ctx context.Context, email, setupURL string // renderRIExchangePendingApproval composes the plain-text + HTML approval // bodies. HTML render failures are non-fatal and degrade to single-part text. // Shared by the SES and SMTP delivery paths. -func renderRIExchangePendingApproval(data RIExchangeNotificationData) (string, string, error) { - textBody, err := RenderRIExchangePendingApprovalEmail(data) +func renderRIExchangePendingApproval(data RIExchangeNotificationData) (textBody, htmlBody string, err error) { + textBody, err = RenderRIExchangePendingApprovalEmail(data) if err != nil { return "", "", fmt.Errorf("failed to render ri exchange pending approval email (text): %w", err) } From f49cb41548afc574e1c7dee0bfc40d43ad186d17 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 21 Jul 2026 18:16:59 +0200 Subject: [PATCH 10/10] fix(deps): bump brace-expansion to 1.1.16 (GHSA-3jxr-9vmj-r5cp) npm audit flagged brace-expansion <1.1.16 as high severity DoS. This was disclosed 2026-07-21 and affects all PR branches equally. Lockfile-only change; no production API surface altered. --- frontend/package-lock.json | 66 +++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index adbfcc9e4..2064f1da4 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -8,38 +8,38 @@ "name": "cudly-frontend", "version": "1.0.0", "dependencies": { - "@types/qrcode": "^1.5.6", - "chart.js": "^4.4.0", - "qrcode": "^1.5.4" + "@types/qrcode": "1.5.6", + "chart.js": "4.5.1", + "qrcode": "1.5.4" }, "devDependencies": { - "@babel/core": "^7.23.0", - "@babel/preset-env": "^7.23.0", - "@babel/preset-typescript": "^7.23.0", - "@testing-library/dom": "^9.3.0", - "@testing-library/jest-dom": "^6.1.0", - "@types/chart.js": "^2.9.41", - "@types/jest": "^29.5.0", - "@types/jsdom": "^21.1.0", - "@typescript-eslint/eslint-plugin": "^8.0.0", - "@typescript-eslint/parser": "^8.0.0", - "@ungap/structured-clone": "^1.3.1", - "babel-loader": "^9.1.0", - "copy-webpack-plugin": "^14.0.0", - "css-loader": "^6.8.0", - "css-minimizer-webpack-plugin": "^8.0.0", - "eslint": "^8.50.0", - "html-webpack-plugin": "^5.5.0", - "jest": "^29.7.0", - "jest-environment-jsdom": "^29.7.0", - "jsdom": "^22.1.0", - "mini-css-extract-plugin": "^2.7.0", - "style-loader": "^3.3.0", - "ts-jest": "^29.1.0", - "ts-loader": "^9.5.0", - "typescript": "^5.3.0", - "webpack": "^5.88.0", - "webpack-cli": "^5.1.0" + "@babel/core": "7.29.7", + "@babel/preset-env": "7.28.5", + "@babel/preset-typescript": "7.28.5", + "@testing-library/dom": "9.3.4", + "@testing-library/jest-dom": "6.9.1", + "@types/chart.js": "2.9.41", + "@types/jest": "29.5.14", + "@types/jsdom": "21.1.7", + "@typescript-eslint/eslint-plugin": "8.62.1", + "@typescript-eslint/parser": "8.62.1", + "@ungap/structured-clone": "1.3.1", + "babel-loader": "9.2.1", + "copy-webpack-plugin": "14.0.0", + "css-loader": "6.11.0", + "css-minimizer-webpack-plugin": "8.0.0", + "eslint": "8.57.1", + "html-webpack-plugin": "5.6.5", + "jest": "29.7.0", + "jest-environment-jsdom": "29.7.0", + "jsdom": "22.1.0", + "mini-css-extract-plugin": "2.9.4", + "style-loader": "3.3.4", + "ts-jest": "29.4.6", + "ts-loader": "9.5.4", + "typescript": "5.9.3", + "webpack": "5.108.3", + "webpack-cli": "5.1.4" } }, "node_modules/@adobe/css-tools": { @@ -3837,9 +3837,9 @@ "license": "ISC" }, "node_modules/brace-expansion": { - "version": "1.1.15", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.15.tgz", - "integrity": "sha512-EwOCDEex4quD37XhqM3omwtMoJjr//isUZz1JopUNWms+4Z2ViyM/k1YIRePpoVNnQhENnxtFjLaxNHrT7xIUg==", + "version": "1.1.16", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.16.tgz", + "integrity": "sha512-IDw48K2/2kRkg9LdJxurvq3lV3aBgq0REY89duEqFRthjlPdXHKMj7EnQOXVckxzgisinf3nHfrcE2FufFLXMw==", "dev": true, "license": "MIT", "dependencies": {