fix(api): derive the purchase spend cap from the stored recommendation

internal/api/handler_per_account_perms_test.go

@@ -711,6 +711,11 @@ func TestPerAccountPerms_ExecutePurchase_AllowedAccountAccepted(t *testing.T) {
 	mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) {
 		return permsAccountList(), nil
 	}
+	// #1905: the rec is now priced from the stored recommendation set.
+	accA := permsAccA
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "aws", Service: "ec2", CloudAccountID: &accA, Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 10,
+	})
 
 	handler := &Handler{
 		auth:   scopedAuthMock(ctx),

internal/api/handler_purchases.go

@@ -1626,7 +1626,10 @@ func (h *Handler) retryPurchase(ctx context.Context, req *events.LambdaFunctionU
 	// relaunch another user's over-cap failed purchase that the retrying
 	// session's OWN constraints would deny, and constraints tightened after
 	// the original submission would never apply on replay (adversarial
-	// review follow-up to #1210).
+	// review follow-up to #1210). failedExec.Recommendations carry the
+	// store-derived prices stamped at submit time
+	// (priceRecommendationsFromStore), so this re-check never sees
+	// client-supplied numbers.
 	if constraintErr := h.enforcePurchaseConstraints(ctx, session, failedExec.Recommendations); constraintErr != nil {
 		return nil, constraintErr
 	}
@@ -2100,6 +2103,11 @@ func (h *Handler) getPurchaseDetails(ctx context.Context, req *events.LambdaFunc
 
 // ExecutePurchaseRequest represents the request to execute purchases.
 type ExecutePurchaseRequest struct {
+	// Recommendations are matched against the stored recommendation set on
+	// (provider, cloud_account_id, service, region, resource_type, engine,
+	// term, payment). Only count, recommended_count and selected are taken
+	// from the request; id, details and every cost field are replaced from
+	// the stored row scaled by count (priceRecommendationsFromStore, #1905).
 	Recommendations []config.RecommendationRecord `json:"recommendations"`
 	// CapacityPercent is what fraction (1..100) of the originally-
 	// recommended counts the user chose in the bulk Purchase flow.
@@ -2169,16 +2177,17 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event
 	if err := validateCapacityConsistency(execReq.Recommendations, execReq.CapacityPercent); err != nil {
 		return ExecutePurchaseRequest{}, nil, nil, err
 	}
-	// Enforce the per-permission Constraints (MaxPurchaseAmount, Providers,
-	// Services, Regions, AccountIDs) configured on the granting
-	// execute:purchases permission (SEC-01, issue #1141). Runs after the
-	// per-rec validation above so provider tokens are already normalized.
-	// Each recommendation must individually be granted by a permission; the
-	// amount cap is checked against the batch's total commitment (upfront
-	// plus recurring, see recTotalCommitment) so it cannot be evaded by
-	// splitting a large purchase across recs or by a no-upfront commitment
-	// whose real cost is entirely recurring.
-	if err := h.enforcePurchaseConstraints(ctx, session, execReq.Recommendations); err != nil {
+	// Price every rec from the stored recommendation set, then enforce the
+	// per-permission Constraints (MaxPurchaseAmount, Providers, Services,
+	// Regions, AccountIDs) on the granting execute:purchases permission
+	// (SEC-01, issue #1141) against those store-derived values. Runs after
+	// the per-rec validation above so provider and payment tokens are
+	// canonical before the identity match, and after the scope check so an
+	// out-of-scope account is refused as 403 before any pricing lookup. The
+	// client's own upfront_cost / monthly_cost / savings / details are never
+	// used (issue #1905): a purchaser under a MaxPurchaseAmount cap could
+	// otherwise state any number and buy at list price.
+	if err := h.priceAndEnforcePurchaseConstraints(ctx, session, execReq.Recommendations); err != nil {
 		return ExecutePurchaseRequest{}, nil, nil, err
 	}
 	return execReq, session, adjustments, nil
@@ -2191,7 +2200,10 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event
 // Shared by the direct-execute request validation and the retry path
 // (retryPurchase) so both re-derive and check the exact same Constraints
 // (SEC-01, issue #1141; adversarial review follow-up to #1210 -- the retry
-// path previously skipped this check entirely).
+// path previously skipped this check entirely). The web execute path
+// reaches this through priceAndEnforcePurchaseConstraints, so recs carry
+// store-derived costs; the retry path enforces against the persisted row,
+// which was priced when it was written.
 func (h *Handler) enforcePurchaseConstraints(ctx context.Context, session *Session, recs []config.RecommendationRecord) error {
 	constraintSets := purchaseConstraintSets(recs)
 	if err := requireNonZeroCommitment(constraintSets); err != nil {
@@ -2251,7 +2263,9 @@ func purchaseConstraintSets(recs []config.RecommendationRecord) []auth.Permissio
 // same *12 convention used throughout this package. A nil MonthlyCost
 // (all-upfront commitments have no recurring charge) or a non-positive
 // Term (can't be converted to months) contribute nothing to the recurring
-// leg, matching the Term<=0 handling elsewhere in this package.
+// leg, matching the Term<=0 handling elsewhere in this package. On the web
+// execute path the two cost fields are the store-derived values written by
+// priceRecommendationsFromStore.
 func recTotalCommitment(rec *config.RecommendationRecord) float64 {
 	total := rec.UpfrontCost
 	if rec.Term > 0 && rec.MonthlyCost != nil {
@@ -2341,7 +2355,10 @@ func (h *Handler) finalizePurchaseStatus(ctx context.Context, execution *config.
 	return "failed"
 }
 
-// validateAndTotalRecommendations validates each recommendation and returns totals.
+// validateAndTotalRecommendations validates each recommendation and returns
+// totals. Runs on store-derived recs for the web path and on the persisted
+// row for retry; the negative and $10M guards stay as the sanity check for
+// both.
 func validateAndTotalRecommendations(recs []config.RecommendationRecord) (upfront, savings float64, err error) {
 	const maxAmount = 10_000_000 // $10M sanity cap
 	for i := range recs {

internal/api/handler_purchases_guards_test.go

@@ -363,6 +363,12 @@ func TestHandler_executePurchase_SurfacesPaymentAdjustments(t *testing.T) {
 	mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil)
 	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil)
 	mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil)
+	// #1905: both recs canonicalise to azure/vm/monthly before pricing, so
+	// they both match this one stored row; the second rec's client
+	// upfront_cost is ignored.
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "azure", Service: "vm", Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 100, Savings: 50,
+	})
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 
@@ -413,6 +419,9 @@ func TestHandler_executePurchase_NoAdjustmentsWhenCanonical(t *testing.T) {
 	mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil)
 	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil)
 	mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil)
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "azure", Service: "vm", Count: 1, Term: 1, Payment: "monthly", UpfrontCost: 100, Savings: 50,
+	})
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 

internal/api/handler_purchases_test.go

@@ -2202,14 +2202,23 @@ func TestHandler_executePurchase_Success(t *testing.T) {
 	// The #644 idempotency lookup queries pending executions before creating.
 	// No prior pending row → not a duplicate → proceeds to create.
 	mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil)
+	// #1905: recs are now priced from the stored recommendation set. The two
+	// rows need distinct identity tuples (ResourceType here) — the real store
+	// can never hold two rows under the same tuple (migration 000043's unique
+	// index), and an identical-tuple fixture would trip the loadStoredRecommendationIndex
+	// duplicate-key guard.
+	expectStoredRecs(mockStore,
+		config.RecommendationRecord{Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50},
+		config.RecommendationRecord{Provider: "aws", Service: "ec2", ResourceType: "m5.xlarge", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 200, Savings: 100},
+	)
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 
 	req := &events.LambdaFunctionURLRequest{
 		Headers: map[string]string{
 			"Authorization": "Bearer admin-token",
 		},
-		Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}, {"id": "rec-2", "provider": "aws", "service": "ec2", "count": 2, "term": 1, "payment": "all-upfront", "upfront_cost": 200.0, "savings": 100.0}]}`,
+		Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "resource_type": "m5.large", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}, {"id": "rec-2", "provider": "aws", "service": "ec2", "resource_type": "m5.xlarge", "count": 2, "term": 1, "payment": "all-upfront", "upfront_cost": 200.0, "savings": 100.0}]}`,
 	}
 	result, err := handler.executePurchase(ctx, req)
 	require.NoError(t, err)
@@ -2283,6 +2292,7 @@ func TestHandler_executePurchase_EmptyRecommendations(t *testing.T) {
 
 func TestHandler_executePurchase_NegativeUpfrontCost(t *testing.T) {
 	ctx := context.Background()
+	mockStore := new(MockConfigStore)
 	mockAuth := new(MockAuthService)
 
 	adminSession := &Session{
@@ -2292,8 +2302,14 @@ func TestHandler_executePurchase_NegativeUpfrontCost(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdminPurchaser()
+	// #1905: the client's upfront_cost is now ignored; a negative cost on
+	// the STORED row is what must be refused (the client value can no
+	// longer poison the check).
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: -100,
+	})
 
-	handler := &Handler{auth: mockAuth}
+	handler := &Handler{config: mockStore, auth: mockAuth}
 
 	req := &events.LambdaFunctionURLRequest{
 		Headers: map[string]string{
@@ -2304,11 +2320,12 @@ func TestHandler_executePurchase_NegativeUpfrontCost(t *testing.T) {
 	result, err := handler.executePurchase(ctx, req)
 	assert.Error(t, err)
 	assert.Nil(t, result)
-	assert.Contains(t, err.Error(), "negative upfront cost")
+	assert.Contains(t, err.Error(), "no usable price")
 }
 
 func TestHandler_executePurchase_NegativeSavings(t *testing.T) {
 	ctx := context.Background()
+	mockStore := new(MockConfigStore)
 	mockAuth := new(MockAuthService)
 
 	adminSession := &Session{
@@ -2318,14 +2335,23 @@ func TestHandler_executePurchase_NegativeSavings(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdminPurchaser()
+	// #1905: pricing passes (stored upfront/count are usable); the negative
+	// savings on the stored row still trips the validateAndTotalRecommendations
+	// guard the same as before. The request itself carries a VALID savings
+	// value (50.0) so this only fails because the stored row's savings (-50)
+	// governs, not because the client's own number happens to be negative
+	// too (CodeRabbit finding on #2073).
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: -50,
+	})
 
-	handler := &Handler{auth: mockAuth}
+	handler := &Handler{config: mockStore, auth: mockAuth}
 
 	req := &events.LambdaFunctionURLRequest{
 		Headers: map[string]string{
 			"Authorization": "Bearer admin-token",
 		},
-		Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": -50.0}]}`,
+		Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}]}`,
 	}
 	result, err := handler.executePurchase(ctx, req)
 	assert.Error(t, err)
@@ -2372,6 +2398,7 @@ func TestHandler_executePurchase_TooManyRecommendations(t *testing.T) {
 
 func TestHandler_executePurchase_ExceedsMaxAmount(t *testing.T) {
 	ctx := context.Background()
+	mockStore := new(MockConfigStore)
 	mockAuth := new(MockAuthService)
 
 	adminSession := &Session{
@@ -2381,14 +2408,22 @@ func TestHandler_executePurchase_ExceedsMaxAmount(t *testing.T) {
 
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdminPurchaser()
+	// #1905: the $10M sanity guard now fires against the stored cost. The
+	// request itself carries a VALID upfront_cost (100.0, well under the
+	// sanity limit) so this only fails because the stored row's cost
+	// ($15M) governs, not because the client's own number also exceeds
+	// the limit (CodeRabbit finding on #2073).
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 15_000_000, Savings: 50,
+	})
 
-	handler := &Handler{auth: mockAuth}
+	handler := &Handler{config: mockStore, auth: mockAuth}
 
 	req := &events.LambdaFunctionURLRequest{
 		Headers: map[string]string{
 			"Authorization": "Bearer admin-token",
 		},
-		Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 15000000.0, "savings": 50.0}]}`,
+		Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}]}`,
 	}
 	result, err := handler.executePurchase(ctx, req)
 	assert.Error(t, err)
@@ -2411,6 +2446,9 @@ func TestHandler_executePurchase_SaveError(t *testing.T) {
 	mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(errors.New("database error"))
 	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil)
 	mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil)
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, Savings: 50,
+	})
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 
@@ -3674,6 +3712,10 @@ func setupDirectExecMocks(ctx context.Context, store *MockConfigStore) {
 	store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil)
 	store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil)
 	store.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil)
+	// #1905: directExecRecBody's rec-1 is now priced from the stored set.
+	expectStoredRecs(store, config.RecommendationRecord{
+		ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100,
+	})
 }
 
 // TestHandler_executePurchase_DirectExec_NoPermission verifies the fail-closed
@@ -3767,6 +3809,11 @@ func TestHandler_executePurchase_PermissionConstraintsDenied(t *testing.T) {
 				assert.ObjectsAreEqual([]string{"ec2"}, sets[1].Services) &&
 				assert.ObjectsAreEqual([]string{"eu-west-1"}, sets[1].Regions)
 		})).Return(false, nil)
+	// #1905: constraint sets are now built from the stored costs.
+	expectStoredRecs(mockStore,
+		config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 3000, Savings: 50},
+		config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "eu-west-1", Count: 2, Term: 1, Payment: "all-upfront", UpfrontCost: 2500, Savings: 100},
+	)
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 	req := &events.LambdaFunctionURLRequest{
@@ -3823,6 +3870,12 @@ func TestHandler_executePurchase_NoUpfrontBatch_TotalCommitmentEnforced(t *testi
 		mock.MatchedBy(func(sets []auth.PermissionConstraints) bool {
 			return len(sets) == 1 && sets[0].MaxPurchaseAmount == wantTotalCommitment
 		})).Return(false, nil)
+	// #1905: the total-commitment constraint is now built from the stored
+	// upfront/monthly cost.
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 3, Payment: "no-upfront",
+		UpfrontCost: 0, MonthlyCost: float64Ptr(600), Savings: 50,
+	})
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 	req := &events.LambdaFunctionURLRequest{
@@ -3881,6 +3934,10 @@ func TestHandler_executePurchase_UserAPIKeyConstraintsDenied(t *testing.T) {
 			}
 			return sets[0].MaxPurchaseAmount == 500.0
 		})).Return(false, nil)
+	// #1905: the constraint set is now built from the stored cost.
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 50,
+	})
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
 	req := &events.LambdaFunctionURLRequest{
@@ -4107,6 +4164,10 @@ func TestHandler_executePurchase_DirectExec_FourEyesOn_DeniesSelfExecute(t *test
 	mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil)
 	mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOn(), nil)
 	mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil)
+	// #1905: directExecRecBody's rec-1 is now priced from the stored set.
+	expectStoredRecs(mockStore, config.RecommendationRecord{
+		ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100,
+	})
 	// enforceFourEyesPolicy re-loads the (freshly-created, randomly-ID'd)
 	// execution to read CreatedByUserID; the real one always carries the
 	// direct-executor's own UUID (see comment above), which is what makes