From d5e7b058e5a72a6c5b166fd05816f5f9c484898c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 21 May 2026 14:32:17 +0200 Subject: [PATCH 1/2] fix(purchases): align token cancel path with session path and guard in-flight rows The token/email cancel path (loadCancelableExecution) only rejected completed/cancelled, so an email-link holder could cancel an approved/running/paused/failed/expired execution that the dashboard session path refuses. Cancelling an approved/running row is unsafe: the AWS commitment is being or has been created, so the cancel would leave the DB and the cloud out of sync. Restrict the token path to pending/notified only, matching cancelPurchaseViaSession. This restriction is itself the in-flight guard. Add table-driven tests on both the token (Manager.CancelExecution) and session (cancelPurchaseViaSession) paths covering every status: each non-cancelable status is rejected with no store write, and pending/notified still cancel successfully. Closes #645 --- internal/api/handler_purchases_test.go | 51 +++++++++++++++ internal/purchase/approvals.go | 10 ++- internal/purchase/approvals_test.go | 86 ++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 14e1f5262..4fc522c6a 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1721,6 +1721,57 @@ func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { mockAuth.AssertExpectations(t) } +// TestHandler_cancelPurchase_Session_RejectsEachNonCancelableStatus is the +// session-path companion to the token-path #645 regression guard: every +// status outside pending/notified must be rejected with a 409 and no write, +// for parity with purchase.Manager.CancelExecution. The admin session keeps +// the focus on the status guard (which fires before authorizeSessionCancel) +// rather than the RBAC matrix, already covered by the matrix tests above. +func TestHandler_cancelPurchase_Session_RejectsEachNonCancelableStatus(t *testing.T) { + rejected := []string{"approved", "running", "paused", "failed", "expired", "completed", "cancelled"} + for _, status := range rejected { + t.Run(status, func(t *testing.T) { + creator := cancelCallerID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: status, + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "admin"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot be cancelled") + assert.Contains(t, err.Error(), status) + mockConfig.AssertNotCalled(t, "WithTx") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) + }) + } +} + +// TestHandler_cancelPurchase_Session_AllowsEachCancelableStatus confirms the +// inverse: pending and notified rows remain cancelable on the session path, +// guarding against an over-restriction that would break the dashboard cancel +// of a row awaiting approval. +func TestHandler_cancelPurchase_Session_AllowsEachCancelableStatus(t *testing.T) { + allowed := []string{"pending", "notified"} + for _, status := range allowed { + t.Run(status, func(t *testing.T) { + creator := cancelCallerID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: status, + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} + runSessionCancelAllowed(t, exec, session, false, false) + }) + } +} + func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *testing.T) { // Pre-migration row: created_by_user_id is NULL. cancel-own can't // match a NULL creator, so a non-admin must be rejected. The email diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 998592629..097862412 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -188,7 +188,15 @@ func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, toke return nil, fmt.Errorf("approval token has expired") } - if execution.Status == "completed" || execution.Status == "cancelled" { + // Only pending/notified rows are cancelable — mirrors the session path + // in cancelPurchaseViaSession (issue #645). The previous predicate + // rejected only completed/cancelled, which let an email-link holder + // cancel an approved/running/paused/failed/expired execution that the + // dashboard user cannot. Restricting to the pre-purchase states is also + // the in-flight guard: approved/running rows are mid-execution (the AWS + // commitment is being or has been created), so cancelling them would + // leave the DB and the cloud out of sync. + if execution.Status != "pending" && execution.Status != "notified" { return nil, fmt.Errorf("execution cannot be cancelled, current status: %s", execution.Status) } return execution, nil diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index bc04bb308..44f20acc0 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -374,6 +374,92 @@ func TestManager_CancelExecution_AlreadyCompleted(t *testing.T) { mockStore.AssertExpectations(t) } +// TestManager_CancelExecution_RejectsNonCancelableStatus is the regression +// guard for issue #645: the token/email cancel path previously rejected only +// completed/cancelled, so an email-link holder could cancel an +// approved/running/paused/failed/expired execution that the dashboard +// (session) path refuses. Each non-pending/notified status must now be +// rejected with no write to the store — approved/running rows in particular +// are mid-execution and cancelling them would desync the DB from the cloud. +func TestManager_CancelExecution_RejectsNonCancelableStatus(t *testing.T) { + rejected := []string{"approved", "running", "paused", "failed", "expired", "completed", "cancelled"} + for _, status := range rejected { + t.Run(status, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + execution := &config.PurchaseExecution{ + ExecutionID: "exec-123", + PlanID: "plan-456", + Status: status, + ApprovalToken: "valid-token", + } + mockStore.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.CancelExecution(ctx, "exec-123", "valid-token", status) + require.Error(t, err) + assert.Contains(t, err.Error(), "execution cannot be cancelled") + assert.Contains(t, err.Error(), status) + // Status guard must fire before any persistence — a rejected + // cancel must not flip the row or drop suppressions. + // SavePurchaseExecutionTx forwards to SavePurchaseExecution in + // the mock, so this single assertion covers both the tx and + // non-tx write paths. + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockStore.AssertExpectations(t) + }) + } +} + +// TestManager_CancelExecution_AllowsCancelableStatus confirms the inverse of +// the #645 guard: genuinely-pending and notified rows are still cancelable on +// the token path, and the cancel commits (status flip + suppression cleanup) +// in a single tx. Without this the alignment fix could silently over-restrict +// and break the legitimate email-link cancel of a row awaiting approval. +func TestManager_CancelExecution_AllowsCancelableStatus(t *testing.T) { + allowed := []string{"pending", "notified"} + for _, status := range allowed { + t.Run(status, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + execution := &config.PurchaseExecution{ + ExecutionID: "exec-123", + PlanID: "plan-456", + Status: status, + ApprovalToken: "valid-token", + } + mockStore.On("GetExecutionByID", ctx, "exec-123").Return(execution, nil) + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + saved = args.Get(1).(*config.PurchaseExecution) + }). + Return(nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.CancelExecution(ctx, "exec-123", "valid-token", "") + require.NoError(t, err) + require.NotNil(t, saved, "cancel should persist the execution") + assert.Equal(t, "cancelled", saved.Status) + mockStore.AssertExpectations(t) + }) + } +} + func TestManager_CancelExecution_NotFound(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) From c78a60dd7351daf9d0505e8d5a5b17bb46dfcfbe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 12:04:34 +0200 Subject: [PATCH 2/2] refactor(purchase): centralize cancelable-status predicate Extract the pending/notified cancelability rule into a single PurchaseExecution.IsCancelable method and call it from both cancel paths (purchase.Manager.loadCancelableExecution and the session-authed cancelPurchaseViaSession) so the policy can never drift between the token and session flows. Behavior is unchanged: only pending/notified rows stay cancelable; approved/running and other in-flight states remain non-cancelable. Addresses CodeRabbit nitpick on PR #648 (issue #645). --- internal/api/handler_purchases.go | 2 +- internal/config/types.go | 12 ++++++++++++ internal/purchase/approvals.go | 16 +++++++++------- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 65626050b..a6a0379f8 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -509,7 +509,7 @@ func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.Lamb return nil, err } - if execution.Status != "pending" && execution.Status != "notified" { + if !execution.IsCancelable() { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled (status=%s)", execution.ExecutionID, execution.Status)) } diff --git a/internal/config/types.go b/internal/config/types.go index 66edb632d..b89603cc6 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -266,6 +266,18 @@ type PurchaseExecution struct { ApprovalTokenExpiresAt *time.Time `json:"approval_token_expires_at,omitempty" dynamodbav:"approval_token_expires_at,omitempty"` } +// IsCancelable reports whether an execution may still be cancelled. Only the +// pre-purchase states ("pending"/"notified") qualify: once a row reaches +// "approved" or "running" the AWS commitment is being or has been created, so +// cancelling would leave the DB and the cloud out of sync; "cancelled", +// "completed", "failed", "expired", and "paused" are likewise non-cancelable. +// Both cancel paths (purchase.Manager.CancelExecution on the email-token flow +// and the session-authed cancelPurchaseViaSession) call this single predicate +// so the policy can never drift between them (issue #645). +func (e *PurchaseExecution) IsCancelable() bool { + return e.Status == "pending" || e.Status == "notified" +} + // RecommendationRecord stores a recommendation with purchase status type RecommendationRecord struct { ID string `json:"id" dynamodbav:"id"` diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 097862412..2891993a5 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -188,15 +188,17 @@ func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, toke return nil, fmt.Errorf("approval token has expired") } - // Only pending/notified rows are cancelable — mirrors the session path - // in cancelPurchaseViaSession (issue #645). The previous predicate - // rejected only completed/cancelled, which let an email-link holder - // cancel an approved/running/paused/failed/expired execution that the - // dashboard user cannot. Restricting to the pre-purchase states is also - // the in-flight guard: approved/running rows are mid-execution (the AWS + // Only pending/notified rows are cancelable — shares the single + // PurchaseExecution.IsCancelable predicate with the session path in + // cancelPurchaseViaSession so the policy can never drift between the two + // flows (issue #645). The previous predicate rejected only + // completed/cancelled, which let an email-link holder cancel an + // approved/running/paused/failed/expired execution that the dashboard + // user cannot. Restricting to the pre-purchase states is also the + // in-flight guard: approved/running rows are mid-execution (the AWS // commitment is being or has been created), so cancelling them would // leave the DB and the cloud out of sync. - if execution.Status != "pending" && execution.Status != "notified" { + if !execution.IsCancelable() { return nil, fmt.Errorf("execution cannot be cancelled, current status: %s", execution.Status) } return execution, nil