diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index df5f03432..6859c1713 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -11,6 +11,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/credentials" "github.com/LeanerCloud/CUDly/internal/email" "github.com/LeanerCloud/CUDly/internal/oidc" + "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/pkg/provider" "github.com/aws/aws-sdk-go-v2/aws" @@ -494,8 +495,9 @@ func (m *Manager) RecoverStrandedApprovals(ctx context.Context) (int, error) { // an explicit human approval action (fail closed on money paths). // // Rules: -// - source="web" rows must wait for the token-link approval path; the -// scheduler and SQS paths must never bypass that gate. +// - web-submitted rows (Source == common.PurchaseSourceWeb, "cudly-web") must +// wait for the token-link approval path; the scheduler and SQS paths must +// never bypass that gate. // - All other pending/notified rows require the owning plan to have // AutoPurchase=true. A plan-fetch error is propagated so the caller can // fail closed rather than defaulting to "execute". @@ -503,7 +505,11 @@ func (m *Manager) RecoverStrandedApprovals(ctx context.Context) (int, error) { // "approved" rows are handled by the session/token approval paths and // RecoverStrandedApprovals; this helper is only called for pending/notified. func (m *Manager) executableByScheduler(ctx context.Context, exec *config.PurchaseExecution) (bool, error) { - if exec.Source == "web" { + // Compare against the typed source constant, not a bare "web" literal: the + // persisted value is "cudly-web" (common.PurchaseSourceWeb), so the old + // literal never matched and web rows could be auto-executed without the + // token-link approval when AutoPurchase=true (fail-open on a money path). + if exec.Source == common.PurchaseSourceWeb { return false, nil } plan, err := m.config.GetPurchasePlan(ctx, exec.PlanID) diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index edbc2a6f6..482a4f6fa 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -386,9 +386,12 @@ func TestManager_ProcessScheduledPurchases_PendingAutoFalseSkipped(t *testing.T) } // TestManager_ProcessScheduledPurchases_WebSourceSkipped verifies that a -// pending execution with Source="web" is never auto-executed by the cron -// sweep, even when the plan has AutoPurchase=true. Web-submitted rows must -// wait for the token-link approval path. +// pending execution with the canonical web source (common.PurchaseSourceWeb, +// "cudly-web") is never auto-executed by the cron sweep, even when the plan +// has AutoPurchase=true. Web-submitted rows must wait for the token-link +// approval path. Uses the real persisted value, not a bare "web" literal, so +// the test reproduces the production data shape (regression guard for the +// stringly-typed gate that let "cudly-web" rows through). func TestManager_ProcessScheduledPurchases_WebSourceSkipped(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -400,15 +403,24 @@ func TestManager_ProcessScheduledPurchases_WebSourceSkipped(t *testing.T) { ExecutionID: "exec-web", PlanID: "plan-web", Status: "pending", - Source: "web", + Source: common.PurchaseSourceWeb, ScheduledDate: pastDate, }, } mockStore.On("GetStaleApprovedExecutions", ctx, mock.Anything).Return([]config.PurchaseExecution{}, nil) mockStore.On("GetPendingExecutions", ctx).Return(executions, nil) - // executableByScheduler short-circuits on Source="web" — GetPurchasePlan must NOT be called. - // TransitionExecutionStatus must NOT be called. + + // Sentinel: web rows must short-circuit in executableByScheduler BEFORE the + // AutoPurchase plan fetch. GetPurchasePlanFn returns AutoPurchase=true (the + // dangerous case: if the gate is bypassed the row would execute), and flips + // planFetched. The mock's default GetPurchasePlan stub does not record the + // call, so AssertNotCalled alone cannot detect the bypass — hence this flag. + planFetched := false + mockStore.GetPurchasePlanFn = func(_ context.Context, planID string) (*config.PurchasePlan, error) { + planFetched = true + return &config.PurchasePlan{ID: planID, AutoPurchase: true}, nil + } manager := &Manager{ config: mockStore, @@ -421,9 +433,11 @@ func TestManager_ProcessScheduledPurchases_WebSourceSkipped(t *testing.T) { assert.Equal(t, 0, result.Processed, "web-sourced rows must not be auto-executed") assert.Equal(t, 0, result.Executed) assert.Equal(t, 0, result.Failed) + assert.False(t, planFetched, + "web rows must short-circuit before the AutoPurchase plan fetch; a bare \"web\" "+ + "literal gate lets the persisted \"cudly-web\" value through to execution") mockStore.AssertExpectations(t) - mockStore.AssertNotCalled(t, "GetPurchasePlan", mock.Anything, mock.Anything) mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } diff --git a/internal/purchase/messages_test.go b/internal/purchase/messages_test.go index 2e9a55867..d4f83591a 100644 --- a/internal/purchase/messages_test.go +++ b/internal/purchase/messages_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/pkg/common" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -118,16 +119,24 @@ func TestManager_ProcessMessage(t *testing.T) { ExecutionID: "exec-web", PlanID: "plan-1", Status: "pending", - Source: "web", + Source: common.PurchaseSourceWeb, } mockStore.On("GetExecutionByID", ctx, "exec-web").Return(execution, nil) - // GetPurchasePlan must NOT be called (Source=web short-circuits) - // TransitionExecutionStatus must NOT be called + // Sentinel: web rows must short-circuit BEFORE the AutoPurchase plan + // fetch. Return AutoPurchase=true (the dangerous case) and flag the + // fetch; the default GetPurchasePlan stub doesn't record the call, so + // AssertNotCalled alone can't catch a bypass of the "cudly-web" gate. + planFetched := false + mockStore.GetPurchasePlanFn = func(_ context.Context, planID string) (*config.PurchasePlan, error) { + planFetched = true + return &config.PurchasePlan{ID: planID, AutoPurchase: true}, nil + } err := manager.ProcessMessage(ctx, `{"type": "execute_purchase", "execution_id": "exec-web"}`) assert.Error(t, err) assert.Contains(t, err.Error(), "not eligible for auto-execution") - mockStore.AssertNotCalled(t, "GetPurchasePlan", mock.Anything, mock.Anything) + assert.False(t, planFetched, + "web rows must short-circuit before the AutoPurchase plan fetch on the SQS path") mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) })