From b93b1d882fa5897a9f9f4a429f0b0547d23ac49b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 21:36:14 +0200 Subject: [PATCH 01/20] feat(email): post-execution notification with one-click revoke (closes #291) After a purchase execution completes (via both session-RBAC and token approval paths), send a post-execution notification email to the global notification address, per-account contact email(s), and the requester's email (deduplicated, first contact is To, rest are Cc). The email includes a one-click "Revoke this purchase" link backed by the existing approval_token. A new AuthPublic route GET/POST /api/purchases/revoke/{id}?token=... validates the token via crypto/subtle.ConstantTimeCompare (timing-safe), then records revocation_requested status. Session-authed users with cancel-any/own permission may also revoke without a token. New helpers: sendPurchaseExecutedEmail, resolveExecutedNotificationRecipients, revokePurchase, revokeViaSession. PII-safe: log recipient count, never raw addresses. Mock stubs added for scheduler, server, and purchase test packages. --- internal/api/coverage_gaps_test.go | 3 + internal/api/handler_purchases.go | 271 ++++++++++++++++++++ internal/api/handler_purchases_test.go | 208 +++++++++++++++ internal/api/middleware.go | 1 + internal/api/router.go | 12 +- internal/email/interfaces.go | 7 + internal/email/nop_sender.go | 5 + internal/email/sender.go | 15 ++ internal/email/smtp_sender.go | 15 ++ internal/email/templates.go | 168 ++++++++++++ internal/mocks/email.go | 7 + internal/purchase/mocks_test.go | 3 + internal/scheduler/scheduler_test.go | 3 + internal/server/app_test.go | 3 + internal/server/handler_ri_exchange_test.go | 4 + 15 files changed, 724 insertions(+), 1 deletion(-) diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index b2ebf6206..984c6e886 100644 --- a/internal/api/coverage_gaps_test.go +++ b/internal/api/coverage_gaps_test.go @@ -929,6 +929,9 @@ func (s *stubEmailNotifier) SendPurchaseApprovalRequest(_ context.Context, _ ema func (s *stubEmailNotifier) SendPurchaseScheduledNotification(_ context.Context, _ email.NotificationData) error { return nil } +func (s *stubEmailNotifier) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { + return nil +} func (s *stubEmailNotifier) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d09f16e09..1f7db0409 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -4,6 +4,7 @@ package api import ( "context" "crypto/sha256" + "crypto/subtle" "encoding/hex" "encoding/json" "errors" @@ -587,6 +588,11 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam logging.Infof("purchase[%s]: approvePurchaseViaSession completed in %s (auth=session)", execution.ExecutionID, time.Since(t0)) + // Best-effort post-execution notification email (issue #291). Fires + // after the synchronous purchase completes so the email carries the + // final committed state. Errors are logged inside sendPurchaseExecutedEmail + // and never propagate — the purchase is already done at this point. + h.sendPurchaseExecutedEmail(ctx, req, execution, session.Email) return map[string]string{"status": "completed"}, nil } @@ -971,6 +977,113 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return map[string]string{"status": "canceled"}, nil } +// revokePurchase is the one-click revocation handler embedded in the +// post-execution notification email (issue #291). It accepts the same +// three-mode dispatch shape as approvePurchase / cancelPurchase: +// +// 1. Session present AND RBAC-authorized (admin / cancel-any / +// cancel-own) → session-authed path. +// 2. token != "" → token-authed path via the email one-click link. +// 3. No token, no qualifying session → 401. +// +// The revocation window check is intentionally limited in this +// iteration: because the sibling "AWS RI/SP revocation via support case" +// issue has not yet landed its dedicated revocation_window_closes_at +// column, revokePurchase reuses the ApprovalToken. Once the sibling lands +// and adds a RevocationToken + RevocationWindowClosesAt column, this +// handler should validate RevocationWindowClosesAt here before +// attempting the cancellation. +// +// At this scope the revoke action is equivalent to a cancel on a +// completed/completed execution — the underlying cloud revocation +// (AWS support-case path) is out of scope for this issue and is handled +// by the sibling "AWS RI/SP revocation" issue. +// +// Present-day behaviour: calling this route within the AWS revocation +// window requests the cancellation and returns {"status":"revocation_requested"}. +// Past the window it returns a friendly 409 with a plain-language message +// rather than a stack trace. +func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execID, token string) (any, error) { + if err := validateUUID(execID); err != nil { + return nil, err + } + execution, err := h.config.GetExecutionByID(ctx, execID) + if err != nil { + return nil, fmt.Errorf("failed to get execution: %w", err) + } + if execution == nil { + return nil, NewClientError(404, "execution not found") + } + + // Only completed/partially_completed purchases have anything to revoke. + // A pending/notified purchase should be cancelled instead. + switch execution.Status { + case "completed", "partially_completed": + // valid + case "pending", "notified": + return nil, NewClientError(409, fmt.Sprintf( + "execution %s is still pending — use the Cancel link instead of Revoke", execID)) + default: + return nil, NewClientError(409, fmt.Sprintf( + "execution %s cannot be revoked (status=%s); the revocation window may have closed or the purchase was not completed", + execID, execution.Status)) + } + + // Three-mode dispatch — same shape as cancelPurchase. + if session := h.tryGetSession(ctx, req); session != nil { + switch sessErr := h.authorizeSessionCancel(ctx, session, execution); { + case sessErr == nil: + return h.revokeViaSession(ctx, execution, session.Email) + case isPermissionDenied(sessErr): + // Fall through to the token branch. + default: + return nil, sessErr + } + } + + if token == "" { + return nil, NewClientError(401, "sign in or use the revocation link from the notification email") + } + + // Token-authed path: validate the token (reusing the approval token for + // this iteration) and confirm the caller is the authorised contact email. + actor, err := h.authorizeApprovalAction(ctx, req, execution) + if err != nil { + return nil, err + } + // Validate token against the execution's ApprovalToken using constant-time + // comparison to prevent timing attacks (same guard as ApproveExecution in + // internal/purchase/approvals.go). + if execution.ApprovalToken == "" { + return nil, NewClientError(403, "invalid revocation token") + } + if subtle.ConstantTimeCompare([]byte(execution.ApprovalToken), []byte(token)) != 1 { + return nil, NewClientError(403, "invalid revocation token") + } + return h.revokeViaSession(ctx, execution, actor) +} + +// revokeViaSession performs the post-execution revocation action by recording +// a "revocation_requested" status on the execution. Actual cloud-side +// revocation (AWS support-case) is out of scope for issue #291 — it is +// handled by the sibling "AWS RI/SP revocation via support case" issue. +// This call records the intent so the History UI can surface it. +func (h *Handler) revokeViaSession(ctx context.Context, execution *config.PurchaseExecution, revokedBy string) (any, error) { + execution.Status = "revocation_requested" + if revokedBy != "" { + rb := revokedBy + execution.CancelledBy = &rb + } + if err := h.config.SavePurchaseExecution(ctx, execution); err != nil { + return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err) + } + logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, revokedBy) + return map[string]string{ + "status": "revocation_requested", + "message": "Revocation request recorded. Contact AWS Support to complete the cancellation within the allowed window.", + }, nil +} + // authorizeSessionCancel returns nil when the session is permitted to cancel // the given execution under the cancel-any / cancel-own RBAC rules added in // issue #46. Returns a 403 ClientError otherwise. @@ -2165,6 +2278,164 @@ func (h *Handler) sendPurchaseApprovalEmail(ctx context.Context, req *events.Lam return true, "", responseRecipient } +// sendPurchaseExecutedEmail sends a post-execution notification to the +// configured recipients after a purchase completes successfully. It follows +// the same best-effort contract as sendPurchaseApprovalEmail: errors are +// logged but never propagate to the caller — the purchase is already done. +// +// Recipients (deduped): +// - global notification_email (Settings → General) +// - per-account contact_email for each recommendation's account +// - email of the user who originally submitted the execution +// (looked up by CreatedByUserID via h.auth.GetUser) +// +// The revocation link uses the execution's ApprovalToken (reusing the same +// token infrastructure). When the sibling "AWS RI/SP revocation via support +// case" issue lands and adds a dedicated revocation token + window field, +// this method should be updated to use those fields instead. +func (h *Handler) sendPurchaseExecutedEmail(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, executedByEmail string) { + if h.emailNotifier == nil { + logging.Debug("sendPurchaseExecutedEmail: no email notifier configured, skipping") + return + } + if execution.ExecutionID == "" { + logging.Warn("sendPurchaseExecutedEmail: empty execution ID, skipping") + return + } + + globalCfg, err := h.config.GetGlobalConfig(ctx) + if err != nil { + logging.Errorf("sendPurchaseExecutedEmail: failed to load global config: %v", err) + return + } + globalNotify := "" + if globalCfg != nil && globalCfg.NotificationEmail != nil { + globalNotify = strings.TrimSpace(*globalCfg.NotificationEmail) + } + + // Gather per-account contact emails for the recommendations. + contactEmails, err := h.gatherAccountContactEmails(ctx, execution.Recommendations) + if err != nil { + logging.Errorf("sendPurchaseExecutedEmail: failed to gather contact emails: %v", err) + contactEmails = nil + } + + // Look up the requester's email via their user ID (if available). + requesterEmail := "" + requesterName := "" + if h.auth != nil && execution.CreatedByUserID != nil && *execution.CreatedByUserID != "" { + if u, lookupErr := h.auth.GetUser(ctx, *execution.CreatedByUserID); lookupErr == nil && u != nil { + requesterEmail = u.Email + } + // Error is non-fatal — we just omit the field from the email body. + } + + // Build the deduplicated To / Cc list. + // Priority: contact emails are To (first one) + Cc (rest); global notify + // and requester email are Cc. When there are no contact emails, the global + // notify becomes To (matching the approval-email fallback in resolveApprovalRecipients). + to, cc := resolveExecutedNotificationRecipients(contactEmails, globalNotify, requesterEmail) + if to == "" { + logging.Warn("sendPurchaseExecutedEmail: no recipients resolved, skipping") + return + } + + summaries := make([]email.RecommendationSummary, 0, len(execution.Recommendations)) + for _, rec := range execution.Recommendations { + summaries = append(summaries, email.RecommendationSummary{ + Service: rec.Service, + ResourceType: rec.ResourceType, + Engine: rec.Engine, + Region: rec.Region, + Count: rec.Count, + MonthlySavings: rec.Savings, + Term: rec.Term, + Payment: rec.Payment, + UpfrontCost: rec.UpfrontCost, + }) + } + + dashboardBase := h.resolveDashboardURL(req) + data := email.NotificationData{ + DashboardURL: dashboardBase, + ExecutionID: execution.ExecutionID, + TotalSavings: execution.EstimatedSavings, + TotalUpfrontCost: execution.TotalUpfrontCost, + Recommendations: summaries, + RecipientEmail: to, + CCEmails: cc, + // Reuse the approval token as the revocation token so the recipient can + // trigger a post-execution cancel via the /revoke route. A dedicated + // revocation token will be added when the sibling "AWS RI/SP revocation" + // issue lands its own DB column. + RevocationToken: execution.ApprovalToken, + RequestedByEmail: requesterEmail, + RequestedByName: requesterName, + RequestedAt: executionTimestamp(execution), + ExecutedBy: executedByEmail, + ExecutedAt: time.Now().UTC().Format(time.RFC3339), + } + if dashboardBase != "" { + data.ArcheraEducationURL = dashboardBase + "/archera-insurance" + } + + if sendErr := h.emailNotifier.SendPurchaseExecutedNotification(ctx, data); sendErr != nil { + logging.Errorf("sendPurchaseExecutedEmail: send failed for execution %s: %v", execution.ExecutionID, sendErr) + } +} + +// resolveExecutedNotificationRecipients builds the To / Cc pair for the +// post-execution notification from the three input email sets. The logic +// mirrors resolveApprovalRecipients: first contact email is To; remaining +// contact emails, globalNotify, and requesterEmail are Cc (deduped). When +// no contact emails are present, globalNotify becomes To and requesterEmail +// becomes Cc. +func resolveExecutedNotificationRecipients(contactEmails []string, globalNotify, requesterEmail string) (to string, cc []string) { + seen := map[string]bool{} + addCc := func(addr string) { + norm := strings.ToLower(strings.TrimSpace(addr)) + if norm == "" || seen[norm] { + return + } + seen[norm] = true + cc = append(cc, addr) + } + + if len(contactEmails) > 0 { + to = contactEmails[0] + seen[strings.ToLower(strings.TrimSpace(to))] = true + for _, addr := range contactEmails[1:] { + addCc(addr) + } + addCc(globalNotify) + addCc(requesterEmail) + return to, cc + } + + // No contact emails: fall back to globalNotify as To. + if globalNotify != "" { + to = globalNotify + seen[strings.ToLower(strings.TrimSpace(to))] = true + addCc(requesterEmail) + return to, cc + } + + // Last resort: requester email only. + to = strings.TrimSpace(requesterEmail) + return to, nil +} + +// executionTimestamp returns a human-readable timestamp for when the execution +// was submitted (ScheduledDate for web-submitted rows). Returns empty string +// when the execution has no timestamp (shouldn't happen in practice but +// guards against zero-value time panics in templates). +func executionTimestamp(exec *config.PurchaseExecution) string { + if exec.ScheduledDate.IsZero() { + return "" + } + return exec.ScheduledDate.UTC().Format(time.RFC3339) +} + // resolveDashboardURL returns the absolute base URL to embed in email // approval/cancel links. Preference order matches the OIDC issuer helper's // strategy for the same underlying problem (Lambda's Function URL can't be diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 8efd3c374..61b0bf6c9 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4004,3 +4004,211 @@ func TestApproveWithDelay_CASLostMaps409(t *testing.T) { assert.Equal(t, 409, ce.code, "concurrent cancel CAS race must map to 409, not 500") mockConfig.AssertExpectations(t) } + +// --------------------------------------------------------------------------- +// revokePurchase handler tests (issue #291) +// --------------------------------------------------------------------------- + +// buildCompletedExec returns a completed execution suitable for revoke tests. +func buildCompletedExec(execID, approvalToken string) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: approvalToken, + Status: "completed", + } +} + +// TestHandler_revokePurchase_ValidToken verifies that a valid token on a +// completed execution records a revocation_requested status. +func TestHandler_revokePurchase_ValidToken(t *testing.T) { + ctx := context.Background() + execID := "11111111-1111-1111-1111-111111111111" + token := "abc123validtoken" + revokerEmail := "contact@acct.example.com" + accountID := "acct-1" + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: token, + Status: "completed", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + // authorizeApprovalAction: GetGlobalConfig for global notify + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + // GetCloudAccount returns the contact email so authorizeApprovalAction can + // resolve the approver and verify the session's email matches. + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: revokerEmail}, nil + } + // SavePurchaseExecution for the revocation_requested update. + mockStore.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { + return e.ExecutionID == execID && e.Status == "revocation_requested" + })).Return(nil) + + mockAuth := new(MockAuthService) + // Provide a session for the revoker so authorizeApprovalAction resolves actor. + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) + // RBAC: revoker has no cancel-any or cancel-own, so falls through to token path. + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + QueryStringParameters: map[string]string{"token": token}, + } + result, err := handler.revokePurchase(ctx, req, execID, token) + require.NoError(t, err, "valid token on completed execution must not error") + resultMap := result.(map[string]string) + assert.Equal(t, "revocation_requested", resultMap["status"]) + mockStore.AssertExpectations(t) +} + +// TestHandler_revokePurchase_InvalidToken verifies that a wrong token returns 403. +// The session provides an email that matches the contact email (so +// authorizeApprovalAction passes), but the token itself is wrong. +func TestHandler_revokePurchase_InvalidToken(t *testing.T) { + ctx := context.Background() + execID := "22222222-2222-2222-2222-222222222222" + contactEmail := "contact@acct.example.com" + accountID := "acct-1" + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "the-real-token", + Status: "completed", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + QueryStringParameters: map[string]string{"token": "wrong-token"}, + } + _, err := handler.revokePurchase(ctx, req, execID, "wrong-token") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 403, ce.code) +} + +// TestHandler_revokePurchase_PendingExecution verifies that a pending execution +// returns 409 with a friendly message directing the user to Cancel instead. +func TestHandler_revokePurchase_PendingExecution(t *testing.T) { + ctx := context.Background() + execID := "33333333-3333-3333-3333-333333333333" + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "tok", + Status: "pending", + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + + handler := &Handler{config: mockStore} + req := &events.LambdaFunctionURLRequest{ + QueryStringParameters: map[string]string{"token": "tok"}, + } + _, err := handler.revokePurchase(ctx, req, execID, "tok") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "Cancel") +} + +// TestHandler_revokePurchase_NotFound verifies 404 when the execution does not exist. +func TestHandler_revokePurchase_NotFound(t *testing.T) { + ctx := context.Background() + execID := "44444444-4444-4444-4444-444444444444" + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(nil, nil) + + handler := &Handler{config: mockStore} + req := &events.LambdaFunctionURLRequest{} + _, err := handler.revokePurchase(ctx, req, execID, "some-token") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 404, ce.code) +} + +// --------------------------------------------------------------------------- +// resolveExecutedNotificationRecipients unit tests (issue #291) +// --------------------------------------------------------------------------- + +// TestResolveExecutedNotificationRecipients_ContactEmailAsTo verifies the +// first contact email becomes To and the rest + global notify + requester +// are added as Cc (deduplicated). +func TestResolveExecutedNotificationRecipients_ContactEmailAsTo(t *testing.T) { + to, cc := resolveExecutedNotificationRecipients( + []string{"contact@a.example.com", "contact@b.example.com"}, + "notify@example.com", + "requester@example.com", + ) + assert.Equal(t, "contact@a.example.com", to) + assert.Contains(t, cc, "contact@b.example.com") + assert.Contains(t, cc, "notify@example.com") + assert.Contains(t, cc, "requester@example.com") + // No duplicates. + assert.Equal(t, 3, len(cc)) +} + +// TestResolveExecutedNotificationRecipients_GlobalNotifyFallback verifies that +// when no contact emails are available, the global notification email becomes To. +func TestResolveExecutedNotificationRecipients_GlobalNotifyFallback(t *testing.T) { + to, cc := resolveExecutedNotificationRecipients( + nil, + "notify@example.com", + "requester@example.com", + ) + assert.Equal(t, "notify@example.com", to) + assert.Contains(t, cc, "requester@example.com") + assert.Equal(t, 1, len(cc)) +} + +// TestResolveExecutedNotificationRecipients_RequesterOnlyFallback verifies that +// when neither contact emails nor global notify are set, the requester email +// becomes To (last resort). +func TestResolveExecutedNotificationRecipients_RequesterOnlyFallback(t *testing.T) { + to, cc := resolveExecutedNotificationRecipients(nil, "", "requester@example.com") + assert.Equal(t, "requester@example.com", to) + assert.Empty(t, cc) +} + +// TestResolveExecutedNotificationRecipients_Deduplication verifies that the +// same email in multiple lists is not repeated. +func TestResolveExecutedNotificationRecipients_Deduplication(t *testing.T) { + // Same email in all three slots. + to, cc := resolveExecutedNotificationRecipients( + []string{"same@example.com"}, + "SAME@example.com", // case-insensitive dedup + "same@example.com", + ) + assert.Equal(t, "same@example.com", to) + assert.Empty(t, cc, "duplicate emails must be deduplicated") +} diff --git a/internal/api/middleware.go b/internal/api/middleware.go index c5867b2d5..96ecbab68 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -20,6 +20,7 @@ func (h *Handler) isPublicEndpoint(path string) bool { "/version", // Public build-version endpoint (version / git SHA / build time) "/api/purchases/approve/", "/api/purchases/cancel/", + "/api/purchases/revoke/", "/api/ri-exchange/approve/", "/api/ri-exchange/reject/", "/api/auth/login", diff --git a/internal/api/router.go b/internal/api/router.go index fd33af446..b5051219b 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -163,6 +163,12 @@ func (r *Router) registerRoutes() { {PathPrefix: "/api/purchases/approve/", Method: "POST", Handler: r.approvePurchaseHandler, Auth: AuthPublic}, {PathPrefix: "/api/purchases/cancel/", Method: "GET", Handler: r.cancelPurchaseHandler, Auth: AuthPublic}, {PathPrefix: "/api/purchases/cancel/", Method: "POST", Handler: r.cancelPurchaseHandler, Auth: AuthPublic}, + // Revoke a completed purchase (issue #291). Same AuthPublic + token-based + // auth pattern as approve/cancel — the token is embedded in the + // post-execution notification email's one-click link. Session-authed + // users with revoke:purchases (or admin) may also use the route. + {PathPrefix: "/api/purchases/revoke/", Method: "GET", Handler: r.revokePurchaseHandler, Auth: AuthPublic}, + {PathPrefix: "/api/purchases/revoke/", Method: "POST", Handler: r.revokePurchaseHandler, Auth: AuthPublic}, // Retry a failed purchase execution (issue #47). Session-authed // only — the original failed row's email-token has already been // consumed/expired, so there is no token-mode dispatch here. @@ -555,7 +561,11 @@ func (r *Router) retryPurchaseHandler(ctx context.Context, req *events.LambdaFun } func (r *Router) revokePurchaseHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { - return r.h.revokePurchase(ctx, req, params["id"]) + if err := r.h.checkRateLimit(ctx, req, "approve_cancel_public"); err != nil { + return nil, err + } + token := resolveApprovalToken(req) + return r.h.revokePurchase(ctx, req, params["id"], token) } func (r *Router) calculateRevokeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { diff --git a/internal/email/interfaces.go b/internal/email/interfaces.go index 72c242df3..2fb1eae3e 100644 --- a/internal/email/interfaces.go +++ b/internal/email/interfaces.go @@ -27,6 +27,13 @@ type SenderInterface interface { // (issue #291 wave-2). Notifies the user that the purchase will execute at // RevocationWindowClosesAt and includes a one-click revoke link. SendPurchaseScheduledNotification(ctx context.Context, data NotificationData) error + // SendPurchaseExecutedNotification fires after a purchase executes + // (regardless of whether it came from the approval-email path or the + // direct-execute path). Recipients: global notification_email, per-account + // contact emails, and the requester. The data must carry RevocationToken + // and RevocationWindowClosesAt so the email embeds a one-click revoke link + // valid for the AWS cancel window. + SendPurchaseExecutedNotification(ctx context.Context, data NotificationData) error SendRegistrationReceivedNotification(ctx context.Context, data RegistrationNotificationData) error SendRegistrationDecisionNotification(ctx context.Context, toEmail string, data RegistrationDecisionData) error } diff --git a/internal/email/nop_sender.go b/internal/email/nop_sender.go index 8fc3423f2..2db3b71de 100644 --- a/internal/email/nop_sender.go +++ b/internal/email/nop_sender.go @@ -95,6 +95,11 @@ func (n *NopSender) SendPurchaseScheduledNotification(_ context.Context, _ Notif return nil } +func (n *NopSender) SendPurchaseExecutedNotification(_ context.Context, _ NotificationData) error { + logging.Debugf("email/nop: SendPurchaseExecutedNotification suppressed") + return nil +} + func (n *NopSender) SendRegistrationReceivedNotification(_ context.Context, _ RegistrationNotificationData) error { logging.Debugf("email/nop: SendRegistrationReceivedNotification suppressed") return nil diff --git a/internal/email/sender.go b/internal/email/sender.go index 2f78f62c3..c0cdf5526 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -433,6 +433,21 @@ type NotificationData struct { DaysUntilPurchase int TotalUpfrontCost float64 TotalSavings float64 + // RevocationToken is the one-time token embedded in the revocation link + // of a post-execution notification email. When non-empty, the template + // renders a "Revoke this purchase" CTA that hits + // /api/purchases/revoke/{ExecutionID}?token=. + // Empty silently omits the revocation panel so other email flows are + // unaffected. + RevocationToken string + // ExecutedAt is the ISO-8601 / RFC-3339 timestamp the purchase was + // executed at. Used in the post-execution notification body. + // Empty omits the timestamp from the body. + ExecutedAt string + // ExecutedBy is the email of the user who triggered execution (approved + // the purchase). Used in the post-execution notification body. + // Empty omits the field. + ExecutedBy string } // RecommendationSummary is a simplified recommendation for email display. diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index 66bc5886b..3968934c6 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -457,6 +457,21 @@ func (s *SMTPSender) SendPurchaseScheduledNotification(ctx context.Context, data return s.SendToEmailWithCC(ctx, recipient, data.CCEmails, subject, body) } +// SendPurchaseExecutedNotification sends the post-execution notification email +// via SMTP. Mirrors SendPurchaseApprovalRequest: prefers data.RecipientEmail +// over the static s.notifyEmail. Issue #291. +func (s *SMTPSender) SendPurchaseExecutedNotification(ctx context.Context, data NotificationData) error { + recipient := data.RecipientEmail + if recipient == "" { + recipient = s.notifyEmail + } + if recipient == "" { + return ErrNoRecipient + } + subject := buildExecutedNotificationSubject(data) + return sendPurchaseExecutedNotificationVia(ctx, s, recipient, subject, data) +} + // SendRegistrationReceivedNotification sends an email to CUDly administrators // for a new registration via SMTP. Prefers the caller-resolved // data.RecipientEmail + CCEmails (admin emails + global notify) so the To / diff --git a/internal/email/templates.go b/internal/email/templates.go index 39da7e6b9..1328fbc62 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -800,6 +800,174 @@ func (s *Sender) SendPurchaseScheduledNotification(ctx context.Context, data Not return s.SendToEmailWithCCMultipart(ctx, data.RecipientEmail, data.CCEmails, subject, body, "") } +// --------------------------------------------------------------------------- +// Post-execution notification templates (issue #291) +// --------------------------------------------------------------------------- + +// purchaseExecutedNotificationTemplate is the plain-text half of the +// post-execution notification email. Rendered alongside +// purchaseExecutedNotificationHTMLTemplate for multipart/alternative delivery. +const purchaseExecutedNotificationTemplate = `[CUDly] Purchase executed{{if .Recommendations}} ({{len .Recommendations}} commitment(s)){{end}} +============================================================================= +{{if .RequestedByEmail}} +Requested by: {{if .RequestedByName}}{{.RequestedByName}} <{{.RequestedByEmail}}>{{else}}{{.RequestedByEmail}}{{end}}{{if .RequestedAt}} at {{.RequestedAt}}{{end}} +{{end}}{{if .ExecutedBy}} +Executed by: {{.ExecutedBy}}{{if .ExecutedAt}} at {{.ExecutedAt}}{{end}} +{{end}} +Summary: +-------- +Total Upfront Cost: ${{printf "%.2f" .TotalUpfrontCost}} +Estimated Monthly Savings: ${{printf "%.2f" .TotalSavings}} + +Commitments: +{{range .Recommendations}} +- {{.Count}}x {{.ResourceType}}{{if .Engine}} ({{.Engine}}){{end}} in {{.Region}} + Service: {{.Service}}{{if .AccountLabel}} | Account: {{.AccountLabel}}{{end}}{{if .Term}} | Term: {{.Term}}yr{{end}}{{if .Payment}} | Payment: {{.Payment}}{{end}} + Upfront: ${{printf "%.2f" .UpfrontCost}} | Est. Savings: ${{printf "%.2f" .MonthlySavings}}/month +{{end}} +{{if .RevocationToken}} +------------------------------------------------------------ +REVOCATION WINDOW +{{if .RevocationWindowClosesAt}}You can revoke this purchase until {{.RevocationWindowClosesAt}}. +After that, contact AWS Support. +{{end}} +One-click revoke: +{{.DashboardURL}}/api/purchases/revoke/{{.ExecutionID}}?token={{urlquery .RevocationToken}} + +Plain-text URL (copy + paste if the link above is broken): +{{.DashboardURL}}/api/purchases/revoke/{{.ExecutionID}}?token={{urlquery .RevocationToken}} +{{end}} +View in dashboard: +{{.DashboardURL}}/purchases#history?execution={{.ExecutionID}} + +This is an automated message from CUDly. +` + +// purchaseExecutedNotificationHTMLTemplate is the HTML half of the +// post-execution notification email. Inline-styled per email-client constraints +// (Outlook, mobile Gmail ignore class-based CSS). Issue #291. +const purchaseExecutedNotificationHTMLTemplate = ` +[CUDly] Purchase executed + + +
+ + + + + + + +{{if .RevocationToken}} + +{{end}} + + + +
+

Purchase Executed

+

{{len .Recommendations}} commitment(s) were purchased successfully.

+
+ + + +{{if .RequestedByEmail}}{{end}} +{{if .ExecutedBy}}{{end}} +
Total Upfront Cost${{printf "%.2f" .TotalUpfrontCost}}
Estimated Monthly Savings${{printf "%.2f" .TotalSavings}}
Requested by{{if .RequestedByName}}{{.RequestedByName}} <{{.RequestedByEmail}}>{{else}}{{.RequestedByEmail}}{{end}}{{if .RequestedAt}} at {{.RequestedAt}}{{end}}
Executed by{{.ExecutedBy}}{{if .ExecutedAt}} at {{.ExecutedAt}}{{end}}
+
+

Commitments

+ + + + + + + + + +{{range .Recommendations}} + + + + + + +{{end}}
Service / SKURegionTerm · PaymentUpfrontSavings/mo
{{.Count}}× {{.ResourceType}}{{if .Engine}} ({{.Engine}}){{end}}
{{.Service}}{{if .AccountLabel}} · {{.AccountLabel}}{{end}}
{{.Region}}{{if .Term}}{{.Term}}yr{{end}}{{if .Payment}} · {{.Payment}}{{end}}${{printf "%.2f" .UpfrontCost}}${{printf "%.2f" .MonthlySavings}}
+
+

Revocation Window

+{{if .RevocationWindowClosesAt}}

You can revoke this purchase until {{.RevocationWindowClosesAt}}. After that, contact AWS Support.

{{end}} + + +
Revoke this purchase
+

If the button does not work, copy and paste this URL: {{.DashboardURL}}/api/purchases/revoke/{{.ExecutionID}}?token={{urlquery .RevocationToken}}

+
+

This is an automated message from CUDly.

+
+
+` + +// RenderPurchaseExecutedNotificationEmail renders the plain-text half of +// the post-execution notification email (issue #291). +func RenderPurchaseExecutedNotificationEmail(data NotificationData) (string, error) { + return renderTemplate("purchase-executed-notification", purchaseExecutedNotificationTemplate, data) +} + +// RenderPurchaseExecutedNotificationEmailHTML renders the HTML half of the +// post-execution notification email. Pair with +// RenderPurchaseExecutedNotificationEmail for multipart/alternative delivery. +func RenderPurchaseExecutedNotificationEmailHTML(data NotificationData) (string, error) { + return renderTemplate("purchase-executed-notification-html", purchaseExecutedNotificationHTMLTemplate, data) +} + +// sendPurchaseExecutedNotificationVia composes the plain-text + HTML bodies +// and ships them through s.SendToEmailWithCCMultipart. HTML render failures +// are non-fatal and degrade to single-part text. Shared by Sender and +// SMTPSender so the two transports stay in sync (same pattern as +// sendPurchaseApprovalRequestVia). Issue #291. +func sendPurchaseExecutedNotificationVia(ctx context.Context, s SenderInterface, recipient, subject string, data NotificationData) error { + textBody, err := RenderPurchaseExecutedNotificationEmail(data) + if err != nil { + return fmt.Errorf("failed to render purchase executed notification (text): %w", err) + } + // HTML render failure is non-fatal: degrade to single-part text. + htmlBody, htmlErr := RenderPurchaseExecutedNotificationEmailHTML(data) + if htmlErr != nil { + logging.Warnf("email: HTML executed-notification render failed, falling back to text-only: %v", htmlErr) + htmlBody = "" + } + return s.SendToEmailWithCCMultipart(ctx, recipient, data.CCEmails, subject, textBody, htmlBody) +} + +// SendPurchaseExecutedNotification sends the post-execution notification email +// to the configured recipients (global notification_email, per-account contact +// emails, and the requester). data.RecipientEmail must be set to the primary To +// address; data.CCEmails carries additional recipients. data.RevocationToken +// and data.RevocationWindowClosesAt control the revocation-link panel in the +// template. Issue #291. +func (s *Sender) SendPurchaseExecutedNotification(ctx context.Context, data NotificationData) error { + if data.RecipientEmail == "" { + return ErrNoRecipient + } + if !isValidFromEmail(s.fromEmail) { + return ErrNoFromEmail + } + subject := buildExecutedNotificationSubject(data) + return sendPurchaseExecutedNotificationVia(ctx, s, data.RecipientEmail, subject, data) +} + +// buildExecutedNotificationSubject constructs the subject line for the +// post-execution notification, including a brief SKU summary when the +// recommendation list is small enough to fit. Extracted so both Sender +// and SMTPSender use the same subject format. +func buildExecutedNotificationSubject(data NotificationData) string { + if len(data.Recommendations) == 1 { + r := data.Recommendations[0] + return fmt.Sprintf("[CUDly] Purchase executed: %s %s in %s", + r.Service, r.ResourceType, r.Region) + } + return fmt.Sprintf("[CUDly] Purchase executed (%d commitment(s))", len(data.Recommendations)) +} + // --------------------------------------------------------------------------- // Account registration email templates // --------------------------------------------------------------------------- diff --git a/internal/mocks/email.go b/internal/mocks/email.go index 7642fb1d3..0f26f3d92 100644 --- a/internal/mocks/email.go +++ b/internal/mocks/email.go @@ -66,6 +66,12 @@ func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data return args.Error(0) } +// SendPurchaseExecutedNotification mocks the post-execution notification operation (issue #291). +func (m *MockEmailSender) SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error { + args := m.Called(ctx, data) + return args.Error(0) +} + func (m *MockEmailSender) SendRegistrationReceivedNotification(ctx context.Context, data email.RegistrationNotificationData) error { args := m.Called(ctx, data) return args.Error(0) @@ -87,6 +93,7 @@ type EmailSenderAPI interface { SendPasswordResetEmail(ctx context.Context, email, resetURL string) error SendWelcomeEmail(ctx context.Context, email, dashboardURL, role string) error SendPurchaseApprovalRequest(ctx context.Context, data email.NotificationData) error + SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error } // Ensure MockEmailSender implements EmailSenderAPI. diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 4da862771..8326b50dd 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -228,6 +228,9 @@ func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data func (m *MockEmailSender) SendPurchaseScheduledNotification(_ context.Context, _ email.NotificationData) error { return nil } +func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { + return nil +} func (m *MockEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 67230a806..4045a2da5 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -109,6 +109,9 @@ func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data func (m *MockEmailSender) SendPurchaseScheduledNotification(_ context.Context, _ email.NotificationData) error { return nil } +func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { + return nil +} func (m *MockEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil } diff --git a/internal/server/app_test.go b/internal/server/app_test.go index e72b2dd0d..700e6f43a 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -313,6 +313,9 @@ func (n *noopEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data func (n *noopEmailSender) SendPurchaseScheduledNotification(_ context.Context, _ email.NotificationData) error { return nil } +func (n *noopEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { + return nil +} func (n *noopEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil } diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index 2525f94ef..7221d4dbd 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -892,6 +892,10 @@ func (m *mockEmailSender) SendRIExchangeCompleted(ctx context.Context, data emai return nil } +func (m *mockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { + return nil +} + func (m *mockEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil } From 2f1b0f51ae57fb3ad1b0cf097b0e4e40c8494224 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 18:58:50 +0200 Subject: [PATCH 02/20] refactor(api/purchases): extract sendPurchaseExecutedEmail + revokePurchase helpers to fit gocyclo budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sendPurchaseExecutedEmail (16 → 9): extract globalNotifyEmail and lookupRequesterInfo to pull the nullable-field branches and auth lookup out of the main flow. revokePurchase (13 → 9): extract checkRevokableStatus, tryRevokeViaSession, and validateRevokeToken, mirroring the pattern used by cancelPurchase and its siblings. No behaviour changes; validation order, error messages, and email sender call shape are identical. --- internal/api/handler_purchases.go | 125 +++++++++++++++++++++--------- 1 file changed, 88 insertions(+), 37 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 1f7db0409..bfa5d8c99 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1017,28 +1017,13 @@ func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunction // Only completed/partially_completed purchases have anything to revoke. // A pending/notified purchase should be cancelled instead. - switch execution.Status { - case "completed", "partially_completed": - // valid - case "pending", "notified": - return nil, NewClientError(409, fmt.Sprintf( - "execution %s is still pending — use the Cancel link instead of Revoke", execID)) - default: - return nil, NewClientError(409, fmt.Sprintf( - "execution %s cannot be revoked (status=%s); the revocation window may have closed or the purchase was not completed", - execID, execution.Status)) + if err := checkRevokableStatus(execution); err != nil { + return nil, err } // Three-mode dispatch — same shape as cancelPurchase. - if session := h.tryGetSession(ctx, req); session != nil { - switch sessErr := h.authorizeSessionCancel(ctx, session, execution); { - case sessErr == nil: - return h.revokeViaSession(ctx, execution, session.Email) - case isPermissionDenied(sessErr): - // Fall through to the token branch. - default: - return nil, sessErr - } + if result, handled, err := h.tryRevokeViaSession(ctx, req, execution); handled { + return result, err } if token == "" { @@ -1051,16 +1036,66 @@ func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunction if err != nil { return nil, err } - // Validate token against the execution's ApprovalToken using constant-time - // comparison to prevent timing attacks (same guard as ApproveExecution in - // internal/purchase/approvals.go). + if err := validateRevokeToken(execution, token); err != nil { + return nil, err + } + return h.revokeViaSession(ctx, execution, actor) +} + +// tryRevokeViaSession attempts the session-authenticated branch of the +// revokePurchase three-mode dispatch (same shape as the session branch of +// cancelPurchase). Returns (result, true, err) when the session was present +// and either completed the revocation or encountered a hard error; returns +// (nil, false, nil) when the session was absent or returned a permission-denied +// error so the caller can fall through to the token branch. Extracted from +// revokePurchase to keep that function under the cyclomatic limit. +func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (any, bool, error) { + session := h.tryGetSession(ctx, req) + if session == nil { + return nil, false, nil + } + switch sessErr := h.authorizeSessionCancel(ctx, session, execution); { + case sessErr == nil: + result, err := h.revokeViaSession(ctx, execution, session.Email) + return result, true, err + case isPermissionDenied(sessErr): + // Fall through to the token branch. + return nil, false, nil + default: + return nil, true, sessErr + } +} + +// checkRevokableStatus returns nil when the execution is in a state that allows +// revocation (completed or partially_completed), or a 409 ClientError when the +// status makes revocation impossible. Extracted from revokePurchase to keep +// that function under the cyclomatic limit. +func checkRevokableStatus(execution *config.PurchaseExecution) error { + switch execution.Status { + case "completed", "partially_completed": + return nil + case "pending", "notified": + return NewClientError(409, fmt.Sprintf( + "execution %s is still pending — use the Cancel link instead of Revoke", execution.ExecutionID)) + default: + return NewClientError(409, fmt.Sprintf( + "execution %s cannot be revoked (status=%s); the revocation window may have closed or the purchase was not completed", + execution.ExecutionID, execution.Status)) + } +} + +// validateRevokeToken checks that the execution carries a non-empty +// ApprovalToken and that it matches the supplied token using constant-time +// comparison (same guard as ApproveExecution in internal/purchase/approvals.go). +// Extracted from revokePurchase to keep that function under the cyclomatic limit. +func validateRevokeToken(execution *config.PurchaseExecution, token string) error { if execution.ApprovalToken == "" { - return nil, NewClientError(403, "invalid revocation token") + return NewClientError(403, "invalid revocation token") } if subtle.ConstantTimeCompare([]byte(execution.ApprovalToken), []byte(token)) != 1 { - return nil, NewClientError(403, "invalid revocation token") + return NewClientError(403, "invalid revocation token") } - return h.revokeViaSession(ctx, execution, actor) + return nil } // revokeViaSession performs the post-execution revocation action by recording @@ -2308,10 +2343,7 @@ func (h *Handler) sendPurchaseExecutedEmail(ctx context.Context, req *events.Lam logging.Errorf("sendPurchaseExecutedEmail: failed to load global config: %v", err) return } - globalNotify := "" - if globalCfg != nil && globalCfg.NotificationEmail != nil { - globalNotify = strings.TrimSpace(*globalCfg.NotificationEmail) - } + globalNotify := globalNotifyEmail(globalCfg) // Gather per-account contact emails for the recommendations. contactEmails, err := h.gatherAccountContactEmails(ctx, execution.Recommendations) @@ -2321,14 +2353,7 @@ func (h *Handler) sendPurchaseExecutedEmail(ctx context.Context, req *events.Lam } // Look up the requester's email via their user ID (if available). - requesterEmail := "" - requesterName := "" - if h.auth != nil && execution.CreatedByUserID != nil && *execution.CreatedByUserID != "" { - if u, lookupErr := h.auth.GetUser(ctx, *execution.CreatedByUserID); lookupErr == nil && u != nil { - requesterEmail = u.Email - } - // Error is non-fatal — we just omit the field from the email body. - } + requesterEmail, requesterName := h.lookupRequesterInfo(ctx, execution) // Build the deduplicated To / Cc list. // Priority: contact emails are To (first one) + Cc (rest); global notify @@ -2384,6 +2409,32 @@ func (h *Handler) sendPurchaseExecutedEmail(ctx context.Context, req *events.Lam } } +// globalNotifyEmail returns the trimmed notification email from a GlobalConfig, +// or "" when the config is nil or the field is unset. Extracted from +// sendPurchaseExecutedEmail to keep that function under the cyclomatic limit. +func globalNotifyEmail(globalCfg *config.GlobalConfig) string { + if globalCfg != nil && globalCfg.NotificationEmail != nil { + return strings.TrimSpace(*globalCfg.NotificationEmail) + } + return "" +} + +// lookupRequesterInfo resolves the email (and name, currently always "") for +// the user who originally submitted the execution. The lookup is non-fatal: +// when auth is unavailable or the user cannot be found, both fields are +// returned empty and the notification is sent without them. Extracted from +// sendPurchaseExecutedEmail to keep that function under the cyclomatic limit. +func (h *Handler) lookupRequesterInfo(ctx context.Context, execution *config.PurchaseExecution) (email, name string) { + if h.auth == nil || execution.CreatedByUserID == nil || *execution.CreatedByUserID == "" { + return "", "" + } + u, err := h.auth.GetUser(ctx, *execution.CreatedByUserID) + if err == nil && u != nil { + return u.Email, "" + } + return "", "" +} + // resolveExecutedNotificationRecipients builds the To / Cc pair for the // post-execution notification from the three input email sets. The logic // mirrors resolveApprovalRecipients: first contact email is To; remaining From f5cc6190f2de04989d337182ed2f555eae22d00f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 13:50:54 +0200 Subject: [PATCH 03/20] fix(api/purchases): enforce revoke-token expiry, atomic status, PII-safe log Address CodeRabbit pass-1 findings on the one-click revoke path (#291): - validateRevokeToken now rejects an expired ApprovalToken (409) before the constant-time match, mirroring ApproveExecution's expiry guard in internal/purchase/approvals.go. Legacy rows without ApprovalTokenExpiresAt (pre-migration 000051) still pass through. - revokeViaSession replaces the read-modify-write SavePurchaseExecution with the existing atomic TransitionExecutionStatus conditional UPDATE (completed|partially_completed -> revocation_requested), closing a lost-update race; CAS rejection maps to 409 and a vanished row to 404. CancelledBy is stamped in a follow-up save on the freshly-returned row. - The revocation log line now redacts the actor email via redactEmail instead of printing it raw, removing PII from logs. Tests: - Add session-auth revoke branch coverage (tryRevokeViaSession -> revokeViaSession): admin cancel-any, owner cancel-own, and a no-permission/no-token denial. - Add regression tests for the expired-token rejection and the concurrent-transition 409. - Switch MockEmailSender.SendPurchaseExecutedNotification to testify's m.Called recorder in internal/purchase and internal/scheduler so the method participates in expectation/assertion coverage. --- internal/api/handler_purchases.go | 42 +++++- internal/api/handler_purchases_test.go | 194 ++++++++++++++++++++++++- internal/purchase/mocks_test.go | 5 +- internal/scheduler/scheduler_test.go | 5 +- 4 files changed, 232 insertions(+), 14 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index bfa5d8c99..465ac68cd 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1085,13 +1085,20 @@ func checkRevokableStatus(execution *config.PurchaseExecution) error { } // validateRevokeToken checks that the execution carries a non-empty -// ApprovalToken and that it matches the supplied token using constant-time -// comparison (same guard as ApproveExecution in internal/purchase/approvals.go). +// ApprovalToken, that the token has not expired, and that it matches the +// supplied token using constant-time comparison (same guard as +// ApproveExecution in internal/purchase/approvals.go). Pre-migration rows +// without an ApprovalTokenExpiresAt (legacy executions created before +// migration 000051) pass the expiry check, mirroring ApproveExecution. // Extracted from revokePurchase to keep that function under the cyclomatic limit. func validateRevokeToken(execution *config.PurchaseExecution, token string) error { if execution.ApprovalToken == "" { return NewClientError(403, "invalid revocation token") } + if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) { + return NewClientError(409, fmt.Sprintf( + "execution %s cannot be revoked: the revocation link has expired", execution.ExecutionID)) + } if subtle.ConstantTimeCompare([]byte(execution.ApprovalToken), []byte(token)) != 1 { return NewClientError(403, "invalid revocation token") } @@ -1103,16 +1110,35 @@ func validateRevokeToken(execution *config.PurchaseExecution, token string) erro // revocation (AWS support-case) is out of scope for issue #291 — it is // handled by the sibling "AWS RI/SP revocation via support case" issue. // This call records the intent so the History UI can surface it. +// +// The status flip uses TransitionExecutionStatus, which issues a conditional +// UPDATE WHERE status IN ('completed','partially_completed'). This guards +// against lost-update races: if the row changed between the GetExecutionByID +// read and this write (e.g. a concurrent transition), zero rows are affected +// and we return a 409 rather than blindly overwriting. CancelledBy is stamped +// in a follow-up SavePurchaseExecution on the freshly-returned row. func (h *Handler) revokeViaSession(ctx context.Context, execution *config.PurchaseExecution, revokedBy string) (any, error) { - execution.Status = "revocation_requested" + updated, err := h.config.TransitionExecutionStatus( + ctx, execution.ExecutionID, + []string{"completed", "partially_completed"}, "revocation_requested") + if err != nil { + if errors.Is(err, config.ErrExecutionNotInExpectedStatus) { + return nil, NewClientError(409, fmt.Sprintf( + "execution %s cannot be revoked: a concurrent operation changed its status", execution.ExecutionID)) + } + if errors.Is(err, config.ErrNotFound) { + return nil, NewClientError(404, "execution not found") + } + return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err) + } if revokedBy != "" { rb := revokedBy - execution.CancelledBy = &rb - } - if err := h.config.SavePurchaseExecution(ctx, execution); err != nil { - return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err) + updated.CancelledBy = &rb + if err := h.config.SavePurchaseExecution(ctx, updated); err != nil { + return nil, fmt.Errorf("failed to record revocation requester for execution %s: %w", execution.ExecutionID, err) + } } - logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, revokedBy) + logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, redactEmail(revokedBy)) return map[string]string{ "status": "revocation_requested", "message": "Revocation request recorded. Contact AWS Support to complete the cancellation within the allowed window.", diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 61b0bf6c9..9bbddf7fa 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4045,9 +4045,15 @@ func TestHandler_revokePurchase_ValidToken(t *testing.T) { mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { return &config.CloudAccount{ID: id, ContactEmail: revokerEmail}, nil } - // SavePurchaseExecution for the revocation_requested update. + // Atomic conditional transition completed/partially_completed -> + // revocation_requested, returning the updated row. + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested"). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil) + // SavePurchaseExecution stamps CancelledBy on the returned row. mockStore.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { - return e.ExecutionID == execID && e.Status == "revocation_requested" + return e.ExecutionID == execID && e.Status == "revocation_requested" && + e.CancelledBy != nil && *e.CancelledBy == revokerEmail })).Return(nil) mockAuth := new(MockAuthService) @@ -4157,6 +4163,190 @@ func TestHandler_revokePurchase_NotFound(t *testing.T) { assert.Equal(t, 404, ce.code) } +// TestHandler_revokePurchase_ExpiredToken verifies that a matching token whose +// ApprovalTokenExpiresAt deadline has passed is rejected with 409, so a stale +// completed execution cannot be marked revocation_requested via an old link. +// Regression test for the revoke-token expiry CR finding (PR #889). +func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { + ctx := context.Background() + execID := "55555555-5555-5555-5555-555555555555" + token := "expiredbutmatching" + contactEmail := "contact@acct.example.com" + accountID := "acct-1" + past := time.Now().Add(-1 * time.Hour) + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: token, + ApprovalTokenExpiresAt: &past, + Status: "completed", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + QueryStringParameters: map[string]string{"token": token}, + } + _, err := handler.revokePurchase(ctx, req, execID, token) + require.Error(t, err, "expired revocation token must be rejected") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "expired") +} + +// TestHandler_revokePurchase_SessionAdminCancelAny exercises the tokenless +// session-authorized revoke branch (tryRevokeViaSession -> revokeViaSession) +// for an admin holding cancel-any on purchases. It asserts the atomic +// TransitionExecutionStatus call and the revocation_requested result. +// Covers the session-auth revoke branch CR finding (PR #889). +func TestHandler_revokePurchase_SessionAdminCancelAny(t *testing.T) { + ctx := context.Background() + execID := "66666666-6666-6666-6666-666666666666" + adminEmail := "admin@example.com" + adminUserID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + + exec := buildCompletedExec(execID, "unused-token") + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested"). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil) + mockStore.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { + return e.ExecutionID == execID && e.CancelledBy != nil && *e.CancelledBy == adminEmail + })).Return(nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: adminUserID, Email: adminEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, adminUserID, "cancel-any", "purchases").Return(true, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + // No token query param: forces the tokenless session-auth path. + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer admin-token"}, + } + result, err := handler.revokePurchase(ctx, req, execID, "") + require.NoError(t, err, "session admin with cancel-any must succeed") + resultMap := result.(map[string]string) + assert.Equal(t, "revocation_requested", resultMap["status"]) + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) +} + +// TestHandler_revokePurchase_SessionOwnerCancelOwn exercises the tokenless +// session-auth branch for a non-admin owner holding cancel-own on a purchase +// they created. Covers the session-auth revoke branch CR finding (PR #889). +func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) { + ctx := context.Background() + execID := "77777777-7777-7777-7777-777777777777" + ownerEmail := "owner@example.com" + ownerUserID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + + exec := buildCompletedExec(execID, "unused-token") + exec.CreatedByUserID = &ownerUserID + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested"). + Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil) + mockStore.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool { + return e.ExecutionID == execID && e.CancelledBy != nil && *e.CancelledBy == ownerEmail + })).Return(nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "owner-token"). + Return(&Session{UserID: ownerUserID, Email: ownerEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-own", "purchases").Return(true, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer owner-token"}, + } + result, err := handler.revokePurchase(ctx, req, execID, "") + require.NoError(t, err, "session owner with cancel-own must succeed") + resultMap := result.(map[string]string) + assert.Equal(t, "revocation_requested", resultMap["status"]) + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) +} + +// TestHandler_revokePurchase_SessionNoPermissionNoToken verifies that a session +// lacking both cancel-any and cancel-own, with no token supplied, is denied: +// the session branch returns permission-denied and falls through to the +// tokenless 401. Covers the session-auth revoke branch CR finding (PR #889). +func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { + ctx := context.Background() + execID := "88888888-8888-8888-8888-888888888888" + userEmail := "nobody@example.com" + userID := "cccccccc-cccc-cccc-cccc-cccccccccccc" + + exec := buildCompletedExec(execID, "unused-token") + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "user-token"). + Return(&Session{UserID: userID, Email: userEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(false, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer user-token"}, + } + _, err := handler.revokePurchase(ctx, req, execID, "") + require.Error(t, err, "no permission and no token must be denied") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 401, ce.code) + mockStore.AssertExpectations(t) + mockAuth.AssertExpectations(t) +} + +// TestHandler_revokeViaSession_ConcurrentTransition verifies that a lost-update +// race (the row's status changed between read and the conditional UPDATE) is +// surfaced as a 409 rather than silently overwriting. Regression test for the +// atomic-status-transition CR finding (PR #889). +func TestHandler_revokeViaSession_ConcurrentTransition(t *testing.T) { + ctx := context.Background() + execID := "99999999-9999-9999-9999-999999999999" + + exec := buildCompletedExec(execID, "tok") + + mockStore := new(MockConfigStore) + mockStore.On("TransitionExecutionStatus", ctx, execID, + []string{"completed", "partially_completed"}, "revocation_requested"). + Return(nil, fmt.Errorf("%w: execution %s", config.ErrExecutionNotInExpectedStatus, execID)) + + handler := &Handler{config: mockStore} + _, err := handler.revokeViaSession(ctx, exec, "someone@example.com") + require.Error(t, err, "a concurrent transition must surface an error") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error") + assert.Equal(t, 409, ce.code) + mockStore.AssertExpectations(t) +} + // --------------------------------------------------------------------------- // resolveExecutedNotificationRecipients unit tests (issue #291) // --------------------------------------------------------------------------- diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 8326b50dd..aeb1047f7 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -228,8 +228,9 @@ func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data func (m *MockEmailSender) SendPurchaseScheduledNotification(_ context.Context, _ email.NotificationData) error { return nil } -func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { - return nil +func (m *MockEmailSender) SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error { + args := m.Called(ctx, data) + return args.Error(0) } func (m *MockEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 4045a2da5..1e037df0c 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -109,8 +109,9 @@ func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data func (m *MockEmailSender) SendPurchaseScheduledNotification(_ context.Context, _ email.NotificationData) error { return nil } -func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { - return nil +func (m *MockEmailSender) SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error { + args := m.Called(ctx, data) + return args.Error(0) } func (m *MockEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error { return nil From 90d29d7c4abef86c5054b9681fd6c6256467f8c5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 15:58:50 +0200 Subject: [PATCH 04/20] fix(api/middleware): exact-match /version to prevent auth bypass via prefix /version was in the prefix-checked list, which allowed any path starting with "/version" (e.g. "/version-admin") to bypass validateSecurity. Move it to an exact-match switch alongside /api/register. Add regression tests for /version (public), /version-evil and /versionXYZ (both private), and /api/registrations (private, must not match /api/register/ prefix). --- internal/api/middleware.go | 16 +++++++++++----- internal/api/middleware_test.go | 6 ++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 96ecbab68..fec28c4cf 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -13,11 +13,13 @@ import ( // isPublicEndpoint returns true for endpoints that don't require authentication. func (h *Handler) isPublicEndpoint(path string) bool { - publicEndpoints := []string{ + // Prefix-matched public endpoints: dynamic routes where the path carries + // a resource ID (approval/cancel/revoke tokens, register tokens, docs + // sub-paths) and static prefixes that are always fully public. + publicPrefixEndpoints := []string{ "/health", // Root health endpoint (no /api prefix) "/api/health", // API health endpoint "/api/info", - "/version", // Public build-version endpoint (version / git SHA / build time) "/api/purchases/approve/", "/api/purchases/cancel/", "/api/purchases/revoke/", @@ -32,13 +34,17 @@ func (h *Handler) isPublicEndpoint(path string) bool { "/docs", "/api/docs", } - for _, ep := range publicEndpoints { + for _, ep := range publicPrefixEndpoints { if strings.HasPrefix(path, ep) { return true } } - // Exact match for POST /api/register (no trailing slash). - if path == "/api/register" { + // Exact-match singleton public endpoints. These must not be prefix-matched + // to prevent "/version-anything" or "/api/register-anything" from + // bypassing auth via accidental prefix overlap. + switch path { + case "/version", // Public build-version endpoint (version / git SHA / build time) + "/api/register": // POST /api/register (no trailing slash) return true } return false diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index bb44a75ce..bdad7c049 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -25,6 +25,12 @@ func TestHandler_isPublicEndpoint(t *testing.T) { {"/api/recommendations", false}, {"/api/plans", false}, {"/api/history", false}, + // /version is a public endpoint but must be exact-matched only. + {"/version", true}, + {"/version-evil", false}, // prefix overlap must not bypass auth + {"/versionXYZ", false}, // prefix overlap must not bypass auth + {"/api/register", true}, // POST /api/register (exact) + {"/api/registrations", false}, // must not match via prefix } for _, tt := range tests { From 27f1b770e9813fcb53820118a680e901b8e3117a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 11:11:00 +0200 Subject: [PATCH 05/20] fix(api/purchases): send executed notification from direct-execute path (#291) The direct-execute path (#289) committed the purchase synchronously but sent no email at all -- approval-email was skipped by design, and the post-execution notification (#291) was never wired in. Recipients of a direct-executed purchase therefore got zero notice and no revoke link, the exact scenario where the executed-notification matters most. Call sendPurchaseExecutedEmail after ApproveAndExecute succeeds in directExecutePurchase, mirroring the token-approve and session-approve call sites. Recipient resolution and the nil-notifier guard live inside the shared helper, so the contract matches the approve paths exactly. Also document the logged-out one-click 401 limitation in revokePurchase: authorizeApprovalAction derives the actor from the session, not the token's bound contact email, so the email link requires a logged-in recipient with the matching contact email. This matches the existing approve/cancel email-link behaviour; tokenless one-click is deferred to the sibling AWS RI/SP revocation work. The authz model is unchanged. --- internal/api/handler_purchases.go | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 465ac68cd..80a69e56d 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1030,6 +1030,16 @@ func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunction return nil, NewClientError(401, "sign in or use the revocation link from the notification email") } + // One-click limitation (issue #291): the email link carries a token, but + // authorizeApprovalAction derives the actor from the *session* + // (tryResolveActorEmail), not from the token's bound contact email. A + // recipient who clicks the link while logged out therefore gets a 401 and + // must sign in with the matching contact email first. This intentionally + // matches the existing approve/cancel email-link behaviour -- we do not + // widen the authz model here. True tokenless one-click (deriving the actor + // from a single-use, contact-bound token) is deferred to the sibling + // "AWS RI/SP revocation" work, which adds a dedicated revocation token. + // // Token-authed path: validate the token (reusing the approval token for // this iteration) and confirm the caller is the authorised contact email. actor, err := h.authorizeApprovalAction(ctx, req, execution) @@ -2120,7 +2130,7 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio if err := h.authorizeSessionExecuteDirect(ctx, session, creatorID); err != nil { return nil, err } - return h.directExecutePurchase(ctx, execution, session) + return h.directExecutePurchase(ctx, req, execution, session) } // Send approval email synchronously so the response can surface the @@ -2182,7 +2192,11 @@ func buildApprovalPendingResponse( // synchronously. ApproveAndExecute already stamps ApprovedBy; we pass // the session email as the actor so the approved_by column also records // who direct-executed. -// 3. Return a "completed" status to the caller. +// 3. Send the best-effort post-execution notification email (issue #291). +// This is the only email the direct-execute flow emits -- no approval +// email precedes it -- so it is the path where the executed-notification +// matters most. +// 4. Return a "completed" status to the caller. // // The audit fields are best-effort if ApproveAndExecute's SavePurchaseExecution // races with our pre-call stamp -- but in practice ApproveAndExecute calls @@ -2191,7 +2205,7 @@ func buildApprovalPendingResponse( // TransitionExecutionStatus. The critical audit invariant is that a non-nil // executed_by_user_id always co-occurs with a non-nil pre_approval_skip_reason, // and both are set atomically in the same SavePurchaseExecution call here. -func (h *Handler) directExecutePurchase(ctx context.Context, execution *config.PurchaseExecution, session *Session) (any, error) { +func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, session *Session) (any, error) { t0 := time.Now() executionID := execution.ExecutionID logging.Infof("purchase[%s]: directExecutePurchase entry (auth=session)", executionID) @@ -2222,6 +2236,15 @@ func (h *Handler) directExecutePurchase(ctx context.Context, execution *config.P } logging.Infof("purchase[%s]: directExecutePurchase completed in %s", executionID, time.Since(t0)) + // Best-effort post-execution notification email (issue #291). The + // direct-execute path sends no approval email (the whole point is to skip + // the approval round-trip), so this is the only notification the recipients + // receive -- making the executed-notification the most valuable here. + // Mirrors the approve-path call sites (see approvePurchase and + // approvePurchaseViaSession); recipient resolution and the nil-notifier + // guard live inside sendPurchaseExecutedEmail. session.Email is the actor + // who direct-executed, matching the actor passed to ApproveAndExecute above. + h.sendPurchaseExecutedEmail(ctx, req, execution, session.Email) return map[string]any{ "execution_id": executionID, "status": "completed", From f402c0433915c83d91d7f3d7e3c7977e66c84826 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 11:11:19 +0200 Subject: [PATCH 06/20] test(api,email): cover executed-notification send/render for all paths (#291) The post-execution notification (#291) had no send-path coverage: every existing approve test built the handler with emailNotifier=nil, so the send was silently skipped, and the direct-execute path had no test at all. Add execution-flow tests with a recording emailNotifier asserting SendPurchaseExecutedNotification fires exactly once with the expected To (per-account contact), executor, and revocation token for all three paths: token-approve, session-approve, and direct-execute. Include a nil-notifier guard test confirming direct-execute still completes the purchase without panicking when no notifier is configured. Add internal/email render tests for RenderPurchaseExecutedNotification Email / ...HTML and buildExecutedNotificationSubject: revoke URL present with the execution route, token url-escaped (raw special chars never leak), revoke panel omitted when the token is absent, the expiry note gated on RevocationWindowClosesAt (deferred -> omitted; populated -> rendered), and the single-vs-multi subject format. --- .../api/executed_notification_flow_test.go | 229 ++++++++++++++++++ internal/email/executed_notification_test.go | 154 ++++++++++++ 2 files changed, 383 insertions(+) create mode 100644 internal/api/executed_notification_flow_test.go create mode 100644 internal/email/executed_notification_test.go diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go new file mode 100644 index 000000000..f7b863bce --- /dev/null +++ b/internal/api/executed_notification_flow_test.go @@ -0,0 +1,229 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/email" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordingExecutedNotifier captures the NotificationData passed to +// SendPurchaseExecutedNotification so the execution-flow tests can assert the +// post-execution notification (issue #291) fires with the expected recipients +// and body fingerprints. All other SenderInterface methods are no-ops via the +// embedded stubEmailNotifier. +type recordingExecutedNotifier struct { + stubEmailNotifier + calls int + captured email.NotificationData +} + +func (r *recordingExecutedNotifier) SendPurchaseExecutedNotification(_ context.Context, data email.NotificationData) error { + r.calls++ + r.captured = data + return nil +} + +// assertExecutedNotificationFingerprints asserts the shared invariants of the +// post-execution notification across all three execution paths: it fired +// exactly once, resolved the per-account contact email as the primary To, and +// carried the execution's approval token as the revocation token + the executor +// in the body. +func assertExecutedNotificationFingerprints(t *testing.T, n *recordingExecutedNotifier, contact, executedBy, token string) { + t.Helper() + require.Equal(t, 1, n.calls, "SendPurchaseExecutedNotification must fire exactly once") + assert.Equal(t, contact, n.captured.RecipientEmail, + "primary To must be the per-account contact email") + assert.Equal(t, executedBy, n.captured.ExecutedBy, + "body must record the executing actor") + assert.Equal(t, token, n.captured.RevocationToken, + "revocation token reuses the execution approval token (issue #291)") + assert.NotEmpty(t, n.captured.ExecutedAt, "executed-at timestamp must be set") +} + +// TestExecutedNotification_TokenApprovePath covers the email one-click +// (token-authed) approve branch of approvePurchase: after ApproveExecution +// succeeds, sendPurchaseExecutedEmail must fire with the recording notifier. +func TestExecutedNotification_TokenApprovePath(t *testing.T) { + ctx := context.Background() + execID := "12345678-1234-1234-1234-123456789abc" + contact := "contact@example.com" + + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contact, + }, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(nil) + + notifier := &recordingExecutedNotifier{} + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + auth: mockAuth, + emailNotifier: notifier, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + result, err := handler.approvePurchase(ctx, req, execID, "valid-token") + require.NoError(t, err) + assert.Equal(t, "completed", result.(map[string]string)["status"]) + + assertExecutedNotificationFingerprints(t, notifier, contact, contact, "valid-token") + mockPurchase.AssertExpectations(t) +} + +// TestExecutedNotification_SessionApprovePath covers the dashboard +// (session-authed) approve branch via approvePurchaseViaSession: after +// ApproveAndExecute succeeds, the notification must fire. +func TestExecutedNotification_SessionApprovePath(t *testing.T) { + ctx := context.Background() + execID := "23456789-2345-2345-2345-23456789abcd" + adminEmail := "admin@example.com" + contact := "contact@example.com" + + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contact, + }, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) + mockAuth.grantAdmin() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil) + + notifier := &recordingExecutedNotifier{} + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + auth: mockAuth, + emailNotifier: notifier, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + // Empty token forces the dashboard (session) branch. + result, err := handler.approvePurchase(ctx, req, execID, "") + require.NoError(t, err) + assert.Equal(t, "completed", result.(map[string]string)["status"]) + + // Admin approved, so the executor recorded in the body is the admin. + assertExecutedNotificationFingerprints(t, notifier, contact, adminEmail, "valid-token") + mockPurchase.AssertExpectations(t) + mockPurchase.AssertNotCalled(t, "ApproveExecution") +} + +// TestExecutedNotification_DirectExecutePath is the regression test for the +// adversarial-verification blocker: the direct-execute path (issue #289) sent +// NO notification at all before the #291 wiring. After ApproveAndExecute +// succeeds, directExecutePurchase must fire the post-execution notification -- +// this is the only email the direct-execute flow emits. +func TestExecutedNotification_DirectExecutePath(t *testing.T) { + ctx := context.Background() + execID := "34567890-3456-3456-3456-34567890abcd" + adminEmail := "admin@example.com" + contact := "contact@example.com" + accountID := "acct-1" + + mockConfig := new(MockConfigStore) + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "valid-token", + Status: "pending", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contact}, nil + } + // directExecutePurchase stamps audit fields, then sendPurchaseExecutedEmail + // reads the global config. + mockConfig.On("SavePurchaseExecution", ctx, exec).Return(nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contact, + }, nil) + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil) + + notifier := &recordingExecutedNotifier{} + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + emailNotifier: notifier, + // auth left nil: lookupRequesterInfo tolerates a nil auth and the + // execution has no CreatedByUserID, so the requester lookup is skipped. + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + session := &Session{Email: adminEmail, UserID: "admin-uid"} + + result, err := handler.directExecutePurchase(ctx, req, exec, session) + require.NoError(t, err) + resultMap := result.(map[string]any) + assert.Equal(t, "completed", resultMap["status"]) + assert.Equal(t, true, resultMap["direct_execute"]) + + assertExecutedNotificationFingerprints(t, notifier, contact, adminEmail, "valid-token") + mockPurchase.AssertExpectations(t) +} + +// TestExecutedNotification_DirectExecute_NilNotifierNoPanic guards the +// best-effort contract: a direct-execute with no email notifier configured +// must still complete the purchase without panicking. +func TestExecutedNotification_DirectExecute_NilNotifierNoPanic(t *testing.T) { + ctx := context.Background() + execID := "45678901-4567-4567-4567-45678901abcd" + adminEmail := "admin@example.com" + + mockConfig := new(MockConfigStore) + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "valid-token", + Status: "pending", + Recommendations: []config.RecommendationRecord{ + {ID: "r1"}, + }, + } + mockConfig.On("SavePurchaseExecution", ctx, exec).Return(nil) + + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil) + + handler := &Handler{ + purchase: mockPurchase, + config: mockConfig, + emailNotifier: nil, // best-effort send is skipped + } + + req := &events.LambdaFunctionURLRequest{} + session := &Session{Email: adminEmail, UserID: "admin-uid"} + + result, err := handler.directExecutePurchase(ctx, req, exec, session) + require.NoError(t, err) + assert.Equal(t, "completed", result.(map[string]any)["status"]) + mockPurchase.AssertExpectations(t) +} diff --git a/internal/email/executed_notification_test.go b/internal/email/executed_notification_test.go new file mode 100644 index 000000000..7b6a895b2 --- /dev/null +++ b/internal/email/executed_notification_test.go @@ -0,0 +1,154 @@ +package email + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// executedNotificationData returns a representative NotificationData for the +// post-execution notification render tests (issue #291). The revocation token +// deliberately contains characters that MUST be percent-encoded by the +// template's {{urlquery}} pipeline so the tests can assert escaping. +func executedNotificationData() NotificationData { + return NotificationData{ + DashboardURL: "https://dashboard.example.com", + ExecutionID: "exec-12345", + TotalSavings: 420.50, + TotalUpfrontCost: 1200.00, + RecipientEmail: "contact@example.com", + CCEmails: []string{"notify@example.com", "requester@example.com"}, + RevocationToken: "tok en/with+special=chars", + RequestedByEmail: "requester@example.com", + RequestedByName: "Requester R", + RequestedAt: "2026-06-01T10:00:00Z", + ExecutedBy: "admin@example.com", + ExecutedAt: "2026-06-01T10:05:00Z", + Recommendations: []RecommendationSummary{ + { + Service: "ec2", + ResourceType: "m5.large", + Region: "us-east-1", + Count: 4, + Term: 3, + Payment: "all-upfront", + UpfrontCost: 1200.00, + MonthlySavings: 420.50, + AccountLabel: "AWS 540659244915", + }, + }, + } +} + +// escapedToken is the exact rendered form of the test token after +// url.QueryEscape (the {{urlquery}} func) followed by html/template's +// contextual auto-escaping: space -> '+' -> '+', '/' -> %2F, '+' -> %2B, +// '=' -> %3D. Both the text and HTML halves use html/template, so both render +// the token identically. +const escapedToken = "tok+en%2Fwith%2Bspecial%3Dchars" + +// rawTokenFragment is a substring of the un-escaped token that MUST NOT appear +// verbatim in a correctly-escaped render (it contains a raw '/'). +const rawTokenFragment = "tok en/with+special=chars" + +func TestRenderPurchaseExecutedNotificationEmail_RevokeURLPresent(t *testing.T) { + data := executedNotificationData() + + body, err := RenderPurchaseExecutedNotificationEmail(data) + require.NoError(t, err) + + // The one-click revoke route is rendered with the execution ID and the + // url-escaped token. + assert.Contains(t, body, "/api/purchases/revoke/exec-12345?token=") + // Token is url-escaped: percent-encoded fragments present... + assert.Contains(t, body, "%2F", "'/' must be percent-encoded") + assert.Contains(t, body, "%2B", "'+' must be percent-encoded") + assert.Contains(t, body, "%3D", "'=' must be percent-encoded") + assert.Contains(t, body, escapedToken) + // ...and the raw token never leaks verbatim into the URL. + assert.NotContains(t, body, rawTokenFragment) + + // Recipient summary (To + Cc) is surfaced for the executed-by / requested-by + // context block. + assert.Contains(t, body, "admin@example.com") // executed by + assert.Contains(t, body, "requester@example.com") // requested by + assert.Contains(t, body, "Requester R") +} + +func TestRenderPurchaseExecutedNotificationEmailHTML_RevokeURLPresent(t *testing.T) { + data := executedNotificationData() + + body, err := RenderPurchaseExecutedNotificationEmailHTML(data) + require.NoError(t, err) + + // HTML half renders the revoke CTA anchor with the escaped token. + assert.Contains(t, body, "/api/purchases/revoke/exec-12345?token="+escapedToken) + assert.Contains(t, body, "Revoke this purchase") + assert.Contains(t, body, "%2F") + assert.Contains(t, body, "%2B") + assert.Contains(t, body, "%3D") + assert.NotContains(t, body, rawTokenFragment) +} + +func TestRenderPurchaseExecutedNotificationEmail_NoRevokeWhenTokenAbsent(t *testing.T) { + data := executedNotificationData() + data.RevocationToken = "" + + body, err := RenderPurchaseExecutedNotificationEmail(data) + require.NoError(t, err) + htmlBody, err := RenderPurchaseExecutedNotificationEmailHTML(data) + require.NoError(t, err) + + // With no token the revocation panel is omitted entirely in both halves. + assert.NotContains(t, body, "/api/purchases/revoke/") + assert.NotContains(t, body, "REVOCATION WINDOW") + assert.NotContains(t, htmlBody, "/api/purchases/revoke/") + assert.NotContains(t, htmlBody, "Revoke this purchase") +} + +func TestRenderPurchaseExecutedNotificationEmail_ExpiryNoteDeferred(t *testing.T) { + data := executedNotificationData() + // RevocationWindowClosesAt is deferred to the sibling AWS-revert work; when + // empty (the present default) the templates omit the expiry sentence. + data.RevocationWindowClosesAt = "" + + body, err := RenderPurchaseExecutedNotificationEmail(data) + require.NoError(t, err) + htmlBody, err := RenderPurchaseExecutedNotificationEmailHTML(data) + require.NoError(t, err) + + assert.NotContains(t, body, "You can revoke this purchase until") + assert.NotContains(t, htmlBody, "You can revoke this purchase until") + // The revoke link itself is still present even without the expiry note. + assert.Contains(t, body, "/api/purchases/revoke/exec-12345") +} + +func TestRenderPurchaseExecutedNotificationEmail_ExpiryNoteWhenPopulated(t *testing.T) { + data := executedNotificationData() + // If/when #804 populates the window, the expiry sentence renders. + data.RevocationWindowClosesAt = "2026-06-08 10:05 UTC" + + body, err := RenderPurchaseExecutedNotificationEmail(data) + require.NoError(t, err) + htmlBody, err := RenderPurchaseExecutedNotificationEmailHTML(data) + require.NoError(t, err) + + assert.Contains(t, body, "You can revoke this purchase until 2026-06-08 10:05 UTC") + assert.Contains(t, htmlBody, "2026-06-08 10:05 UTC") +} + +func TestBuildExecutedNotificationSubject_SingleAndMulti(t *testing.T) { + single := executedNotificationData() + subject := buildExecutedNotificationSubject(single) + assert.Equal(t, "[CUDly] Purchase executed: ec2 m5.large in us-east-1", subject) + assert.True(t, strings.HasPrefix(subject, "[CUDly] Purchase executed:")) + + multi := executedNotificationData() + multi.Recommendations = append(multi.Recommendations, RecommendationSummary{ + Service: "rds", ResourceType: "db.r5.large", Region: "eu-west-1", + }) + multiSubject := buildExecutedNotificationSubject(multi) + assert.Equal(t, "[CUDly] Purchase executed (2 commitment(s))", multiSubject) +} From 23fd6ce78e3ef8898a7c4a21f2ba8f28d9c8eeff Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 11:11:34 +0200 Subject: [PATCH 07/20] fix(api/test): repair broken Session.Role literal and gofmt drift internal/api test package failed to compile: TestHandler_getPurchaseDetails_ NotFound_IsClientError set a nonexistent Session.Role field. The Session struct carries only UserID/Email; admin is resolved via HasPermissionAPI. Drop the bogus field and confer admin through mockAuth.grantAdmin() so the test reaches the GetExecutionByID not-found branch it asserts on (#431). Also apply gofmt to handler_purchases_guards_test.go and middleware_test.go, which carried multi-statement-per-line drift that failed the gofmt hook. --- internal/api/middleware_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index bdad7c049..6d0262f95 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -27,9 +27,9 @@ func TestHandler_isPublicEndpoint(t *testing.T) { {"/api/history", false}, // /version is a public endpoint but must be exact-matched only. {"/version", true}, - {"/version-evil", false}, // prefix overlap must not bypass auth - {"/versionXYZ", false}, // prefix overlap must not bypass auth - {"/api/register", true}, // POST /api/register (exact) + {"/version-evil", false}, // prefix overlap must not bypass auth + {"/versionXYZ", false}, // prefix overlap must not bypass auth + {"/api/register", true}, // POST /api/register (exact) {"/api/registrations", false}, // must not match via prefix } From 18a5f5cd9c555dfc5a9aaf6d5ef6929dad921879 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 11:35:06 +0200 Subject: [PATCH 08/20] fix(auth): align requiresCSRFValidation with exact-match + gofmt middleware_test Replace strings.HasPrefix loop in requiresCSRFValidation() with a switch statement using exact-match, mirroring the approach already in place for isPublicEndpoint(). Previously a path like /api/register-malicious would be silently exempted from CSRF validation because it shares the /api/register prefix with the legitimate registration endpoint. Add TestHandler_requiresCSRFValidation_ExactMatch to regression-guard the prefix-overlap gap (e.g. POST /api/register-malicious must require CSRF). --- internal/api/middleware.go | 34 ++++++++++++++------------ internal/api/middleware_test.go | 43 +++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/internal/api/middleware.go b/internal/api/middleware.go index fec28c4cf..a8a747593 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -139,24 +139,28 @@ func (h *Handler) requiresCSRFValidation(method, path string, req *events.Lambda return false } - // Auth endpoints that don't have a session yet are exempt unconditionally. - // Prefix matching is safe for these /api/auth/* paths because no admin - // sub-paths share those prefixes. - csrfExemptAlwaysPrefix := []string{ - "/api/auth/login", + // Auth endpoints that don't have a session yet are exempt. + // + // Note: /api/purchases/approve/ and /api/purchases/cancel/ are NOT listed + // here. Those routes are AuthPublic and listed in isPublicEndpoint(), so + // validateSecurity() short-circuits before requiresCSRFValidation() is + // reached on the token-only (email-link) path. For the session-authed path + // (approvePurchaseViaSession / cancelPurchaseViaSession), CSRF is enforced + // directly inside those functions -- a blanket middleware exemption would + // leave session-authenticated POSTs to these endpoints unprotected (CSRF). + // + // Similarly, /api/ri-exchange/approve/ and /api/ri-exchange/reject/ are + // AuthPublic and therefore exempted by isPublicEndpoint(), not here. + // + // All exemptions use exact-match to prevent a path like + // /api/register-malicious from bypassing CSRF via accidental prefix overlap. + // This mirrors the exact-match approach in isPublicEndpoint(). + switch path { + case "/api/auth/login", "/api/auth/setup-admin", "/api/auth/forgot-password", "/api/auth/reset-password", - } - for _, exempt := range csrfExemptAlwaysPrefix { - if strings.HasPrefix(path, exempt) { - return false - } - } - // Exact match for the public self-registration endpoint (issue #1017). - // HasPrefix would also exempt /api/registrations//approve|reject|delete, - // which are AuthAdmin state-changing routes that MUST be CSRF-protected. - if path == "/api/register" { + "/api/register": // POST /api/register (public registration, no session) return false } diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 6d0262f95..3acd189f9 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -247,6 +247,49 @@ func TestHandler_authenticate_BearerTokenWithAPIKey(t *testing.T) { assert.False(t, handler.authenticate(ctx, req)) } +// TestHandler_requiresCSRFValidation_ExactMatch asserts that paths sharing a +// prefix with a CSRF-exempt route are NOT themselves exempt. The fix uses +// exact-match (switch) instead of strings.HasPrefix so that a path like +// /api/register-malicious is not silently exempted because it starts with +// /api/register. +func TestHandler_requiresCSRFValidation_ExactMatch(t *testing.T) { + handler := &Handler{} + + tests := []struct { + method string + path string + requiresCSRF bool + }{ + // Exempt routes must still pass (exact-match still covers them). + {"POST", "/api/auth/login", false}, + {"POST", "/api/auth/setup-admin", false}, + {"POST", "/api/auth/forgot-password", false}, + {"POST", "/api/auth/reset-password", false}, + {"POST", "/api/register", false}, + // Paths sharing only a PREFIX with an exempt route must require CSRF. + {"POST", "/api/register-malicious", true}, + {"POST", "/api/register-anything", true}, + {"POST", "/api/auth/login-evil", true}, + {"POST", "/api/auth/forgot-password-extra", true}, + // Non-exempt POST paths + {"POST", "/api/config", true}, + {"POST", "/api/recommendations", true}, + // GET never requires CSRF regardless of path + {"GET", "/api/register", false}, + {"GET", "/api/config", false}, + // DELETE on a protected endpoint requires CSRF + {"DELETE", "/api/plans", true}, + } + + for _, tt := range tests { + name := tt.method + " " + tt.path + t.Run(name, func(t *testing.T) { + result := handler.requiresCSRFValidation(tt.method, tt.path) + assert.Equal(t, tt.requiresCSRF, result) + }) + } +} + // --------------------------------------------------------------------------- // CSRF boundary tests for session-authed approve/cancel (issue #404) // --------------------------------------------------------------------------- From 01872c3f1df7110775254aa23087aa32d4094b34 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:34:40 +0200 Subject: [PATCH 09/20] fix(api/test): update requiresCSRFValidation call to 3-arg signature The base branch added a req parameter to requiresCSRFValidation; the ExactMatch test in this PR still called the 2-arg form. Pass nil for tests that only exercise the path-matching branches (which return before inspecting the request). --- internal/api/middleware_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 3acd189f9..f83a598d7 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -284,7 +284,7 @@ func TestHandler_requiresCSRFValidation_ExactMatch(t *testing.T) { for _, tt := range tests { name := tt.method + " " + tt.path t.Run(name, func(t *testing.T) { - result := handler.requiresCSRFValidation(tt.method, tt.path) + result := handler.requiresCSRFValidation(tt.method, tt.path, nil) assert.Equal(t, tt.requiresCSRF, result) }) } From a31938db679c521e350b853462f153d0455ab4d5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 13:17:12 -0700 Subject: [PATCH 10/20] fix(api/purchases): GET /revoke renders confirmation form; only POST mutates (CRITICAL: email prefetchers) GET /api/purchases/revoke/:id now returns a minimal HTML confirmation page containing a POST form with the token in a hidden input. No DB queries are made on GET, so email prefetchers and link scanners cannot auto-trigger the revocation. The POST form encoding (application/x-www-form-urlencoded) is now also supported by resolveApprovalToken alongside the existing JSON body and query-string paths, so the confirmation form's submission is correctly handled on the POST path. Tests: TestRevokePurchase_GETRendersConfirmationPage_NoMutation, TestRevokePurchase_POSTPerformsRevoke --- internal/api/handler_purchases.go | 56 ++++++++++++++++ internal/api/handler_purchases_test.go | 89 ++++++++++++++++++++++++++ internal/api/router.go | 29 ++++++--- 3 files changed, 166 insertions(+), 8 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 80a69e56d..ad3db4f7e 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1003,10 +1003,43 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb // window requests the cancellation and returns {"status":"revocation_requested"}. // Past the window it returns a friendly 409 with a plain-language message // rather than a stack trace. +// revokeConfirmPageTmpl is the minimal HTML form rendered on GET /revoke. +// The form POSTs the token from a hidden input so the actual mutation never +// fires via a GET request (prevents email prefetchers from accidentally +// triggering the revocation). +const revokeConfirmPageTmpl = ` + +Confirm Revocation + + + +

Confirm Revocation

+

You are about to request revocation for purchase execution {{.ExecutionID}}.

+

This will record a revocation request. Contact AWS Support to complete the +cancellation within the allowed window.

+
+ + +
+

If you did not request this, ignore this page. No action has been taken.

+ +` + func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunctionURLRequest, execID, token string) (any, error) { if err := validateUUID(execID); err != nil { return nil, err } + + // GET: render a confirmation page so email prefetchers cannot auto-trigger + // the revocation. The token travels in the URL query string (unavoidable for + // the email one-click link), but no mutation occurs on GET. + if req.RequestContext.HTTP.Method == "GET" { + return renderRevokeConfirmPage(execID, token), nil + } + execution, err := h.config.GetExecutionByID(ctx, execID) if err != nil { return nil, fmt.Errorf("failed to get execution: %w", err) @@ -1052,6 +1085,29 @@ func (h *Handler) revokePurchase(ctx context.Context, req *events.LambdaFunction return h.revokeViaSession(ctx, execution, actor) } +// renderRevokeConfirmPage returns a rawResponse containing the HTML +// confirmation form for GET /revoke. The form POSTs the token so the actual +// mutation only happens when the user explicitly clicks "Confirm Revoke". +func renderRevokeConfirmPage(execID, token string) *rawResponse { + // Manual substitution to avoid importing html/template just for this one + // small page. The values substituted here (execID and token) are both + // treated as opaque strings — execID is a UUID (hex+hyphens only), + // token is a generated random string. HTML-escape them defensively anyway. + escaped := func(s string) string { + s = strings.ReplaceAll(s, "&", "&") + s = strings.ReplaceAll(s, "<", "<") + s = strings.ReplaceAll(s, ">", ">") + s = strings.ReplaceAll(s, "\"", """) + return s + } + page := strings.ReplaceAll(revokeConfirmPageTmpl, "{{.ExecutionID}}", escaped(execID)) + page = strings.ReplaceAll(page, "{{.Token}}", escaped(token)) + return &rawResponse{ + contentType: "text/html; charset=utf-8", + body: page, + } +} + // tryRevokeViaSession attempts the session-authenticated branch of the // revokePurchase three-mode dispatch (same shape as the session branch of // cancelPurchase). Returns (result, true, err) when the session was present diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 9bbddf7fa..a54cfe817 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -4402,3 +4402,92 @@ func TestResolveExecutedNotificationRecipients_Deduplication(t *testing.T) { assert.Equal(t, "same@example.com", to) assert.Empty(t, cc, "duplicate emails must be deduplicated") } + +// --------------------------------------------------------------------------- +// Finding #2: GET /revoke must render confirmation form, not mutate state +// --------------------------------------------------------------------------- + +// TestRevokePurchase_GETRendersConfirmationPage_NoMutation verifies that a GET +// to the revoke handler returns 200 HTML containing a confirmation form and +// does NOT call TransitionExecutionStatus (no mutation). +func TestRevokePurchase_GETRendersConfirmationPage_NoMutation(t *testing.T) { + ctx := context.Background() + execID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + token := "revoke-token-abc" + + // Store must NOT be called at all: the GET path short-circuits before any DB ops. + mockStore := new(MockConfigStore) + + handler := &Handler{config: mockStore} + req := &events.LambdaFunctionURLRequest{ + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "GET"}, + }, + QueryStringParameters: map[string]string{"token": token}, + } + result, err := handler.revokePurchase(ctx, req, execID, token) + require.NoError(t, err) + + raw, ok := result.(*rawResponse) + require.True(t, ok, "GET /revoke must return a *rawResponse (HTML confirmation page)") + assert.Contains(t, raw.body, `