From 5355179d81bdbd5d2c02c9e57f20f63b3658b8fb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 21 Jul 2026 21:56:17 +0200 Subject: [PATCH] fix(auth): bind MaxPurchaseAmount to total commitment, enforce on retry purchaseConstraintSets capped MaxPurchaseAmount against a batch's total upfront cost only. A no-upfront or partial-upfront commitment's real cost is the recurring monthly charge over the term, not the (possibly zero) upfront alone, so an honest no-upfront purchase could evade an otherwise-binding cap. Fix the basis to the batch's total commitment (upfront plus monthly_cost * term_months, recTotalCommitment) and reject a batch whose computed total is exactly zero rather than silently reading MaxPurchaseAmount==0 as "unconstrained" (requireNonZeroCommitment). Also reject a negative monthly_cost at the request boundary, since it would otherwise offset the recurring leg and reopen the same evasion. retryPurchase never consulted the execute:purchases permission Constraints at all, so a retry-any/retry-own session could replay another user's over-cap failed purchase, and a permission tightened after the original submission never applied to a replay. Route both the direct-execute and retry paths through a new shared enforcePurchaseConstraints helper so they check identical Constraints. Adversarial review follow-up to #1210 (SEC-01, issue #1141). --- internal/api/handler_purchases.go | 101 +++++++++++-- internal/api/handler_purchases_guards_test.go | 9 ++ internal/api/handler_purchases_test.go | 140 ++++++++++++++++-- internal/api/validation.go | 8 + 4 files changed, 239 insertions(+), 19 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d3a184c6f..6063a328b 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1523,6 +1523,15 @@ func resolveOpsHint(failureReason string) string { // → may retry their own failed row. // - else → 403. // +// Constraint gate (SEC-01, issue #1141, adversarial review follow-up to +// #1210): the retried recommendations must ALSO satisfy the retrying +// session's execute:purchases permission Constraints (MaxPurchaseAmount, +// Providers, Services, Regions, AccountIDs), exactly as a fresh +// executePurchase call would enforce. Without this gate a retry-any +// session could replay another user's over-cap failed purchase, and a +// permission tightened after the original submission would never apply +// to a replay. +// // State gate: // - failedExec.Status must be "failed" → 409 otherwise. // - failedExec.Error must NOT match the persistent-failure map → @@ -1548,6 +1557,18 @@ func (h *Handler) retryPurchase(ctx context.Context, req *events.LambdaFunctionU return nil, err } + // Enforce the per-permission Constraints (MaxPurchaseAmount, Providers, + // Services, Regions, AccountIDs) configured on the retrying session's + // execute:purchases permission (SEC-01, issue #1141), mirroring the gate + // in validateExecutePurchaseRequest. Without this, retry-any could + // 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). + if constraintErr := h.enforcePurchaseConstraints(ctx, session, failedExec.Recommendations); constraintErr != nil { + return nil, constraintErr + } + newExecution, err := h.persistRetryExecution(ctx, failedExec, session, totalUpfront, totalSavings) if err != nil { return nil, err @@ -1995,32 +2016,54 @@ func (h *Handler) validateExecutePurchaseRequest(ctx context.Context, req *event // 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 upfront cost so it - // cannot be evaded by splitting a large purchase across recs. - if err := h.requirePermissionConstraints(ctx, session, "execute", "purchases", purchaseConstraintSets(execReq.Recommendations)); err != nil { + // 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 { return ExecutePurchaseRequest{}, nil, err } return execReq, session, nil } +// enforcePurchaseConstraints builds the per-recommendation +// auth.PermissionConstraints sets (purchaseConstraintSets) and enforces them +// against session's execute:purchases permission, first rejecting a batch +// whose total commitment computes to zero (requireNonZeroCommitment). +// 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). +func (h *Handler) enforcePurchaseConstraints(ctx context.Context, session *Session, recs []config.RecommendationRecord) error { + constraintSets := purchaseConstraintSets(recs) + if err := requireNonZeroCommitment(constraintSets); err != nil { + return err + } + return h.requirePermissionConstraints(ctx, session, "execute", "purchases", constraintSets) +} + // purchaseConstraintSets builds one auth.PermissionConstraints per // recommendation in a web execute request, for the SEC-01 constraint // enforcement in validateExecutePurchaseRequest. Single-value Provider/ // Service/Region/AccountIDs lists make the auth service's any-overlap // matcher equivalent to strict containment, so a batch cannot pass on the // strength of one in-scope rec while another rec is out of scope. -// MaxPurchaseAmount carries the batch's total upfront cost (the same basis -// as the global $10M sanity cap in validateAndTotalRecommendations) on -// every set. AccountIDs is ALWAYS populated: a rec without a CloudAccountID +// MaxPurchaseAmount carries the batch's TOTAL commitment (upfront cost plus +// the full recurring obligation over the term, see recTotalCommitment) on +// every set, not just the upfront cost. A no-upfront or partial-upfront +// commitment's UpfrontCost is legitimately $0 or small, so capping on +// upfront alone let a batch with an arbitrarily large recurring commitment +// evade the cap using entirely honest data (adversarial review follow-up +// to #1210). AccountIDs is ALWAYS populated: a rec without a CloudAccountID // carries unattributedAccountConstraint so an AccountIDs-constrained // permission denies it (the auth matcher treats an empty request-side list // as satisfied, so omitting the dimension would fail open). Session-level // allowed_accounts scoping is independently enforced by // validatePurchaseRecommendationScope. func purchaseConstraintSets(recs []config.RecommendationRecord) []auth.PermissionConstraints { - var totalUpfront float64 + var totalCommitment float64 for i := range recs { - totalUpfront += recs[i].UpfrontCost + totalCommitment += recTotalCommitment(&recs[i]) } sets := make([]auth.PermissionConstraints, 0, len(recs)) for i := range recs { @@ -2034,12 +2077,52 @@ func purchaseConstraintSets(recs []config.RecommendationRecord) []auth.Permissio Services: []string{rec.Service}, Regions: []string{rec.Region}, AccountIDs: []string{accountID}, - MaxPurchaseAmount: totalUpfront, + MaxPurchaseAmount: totalCommitment, }) } return sets } +// recTotalCommitment returns a single recommendation's full committed +// spend: the upfront cost plus the recurring monthly cost multiplied by +// the term length in months. UpfrontCost and MonthlyCost are both totals +// for the rec's Count (AWS Cost Explorer reports them per-batch, not +// per-instance -- see recommendationToOffering), so they sum directly +// with no per-instance scaling needed. rec.Term is in years (AWS/Azure/GCP +// RI/SP standard; see exchange_lookup.go), converted to months with the +// 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. +func recTotalCommitment(rec *config.RecommendationRecord) float64 { + total := rec.UpfrontCost + if rec.Term > 0 && rec.MonthlyCost != nil { + total += *rec.MonthlyCost * float64(rec.Term*12) + } + return total +} + +// requireNonZeroCommitment guards against a purchase batch whose computed +// total commitment (see recTotalCommitment) is exactly zero even though it +// targets real, count>0 resources. A genuine RI/Savings Plan/commitment +// purchase always carries SOME committed spend -- either an upfront cost +// (all/partial-upfront) or a recurring monthly charge (no-upfront); a batch +// that totals to exactly $0 is not "unconstrained", it is malformed or +// adversarial data (e.g. a no-upfront rec submitted with monthly_cost +// omitted or zeroed out to slip under a MaxPurchaseAmount cap -- the same +// evasion shape recTotalCommitment was introduced to close). Per this +// repo's no-silent-fallback-on-money-paths convention, a zero total must +// fail loud rather than be silently treated as "no constraint applies" +// (MaxPurchaseAmount==0 reads as uncapped to matchPurchaseAmountConstraint). +// sets is assumed non-empty whenever recs is (callers already reject empty +// recommendation batches before reaching this check). +func requireNonZeroCommitment(sets []auth.PermissionConstraints) error { + if len(sets) > 0 && sets[0].MaxPurchaseAmount == 0 { + return NewClientError(400, "recommendation batch has zero total commitment (no upfront or recurring cost); refusing to evaluate purchase constraints against it") + } + return nil +} + // normalizeCapacityPercent defaults an absent/zero capacity_percent to 100 // and rejects anything outside [1, 100]. capacity_percent is audit-only but // still bounded: a value outside the range is a client bug worth surfacing diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index eb8c09ecf..400af8ce5 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -94,6 +94,15 @@ func TestValidatePurchaseRecommendation(t *testing.T) { {"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true}, {"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true}, {"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true}, + {"negative monthly cost rejected", mutate(func(r *config.RecommendationRecord) { + m := -1.0 + r.MonthlyCost = &m + }), true}, + {"nil monthly cost accepted", mutate(func(r *config.RecommendationRecord) { r.MonthlyCost = nil }), false}, + {"zero monthly cost accepted", mutate(func(r *config.RecommendationRecord) { + m := 0.0 + r.MonthlyCost = &m + }), false}, {"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true}, {"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true}, {"empty provider rejected", mutate(func(r *config.RecommendationRecord) { r.Provider = "" }), true}, diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 32afd4461..99014ee67 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3052,6 +3052,13 @@ func buildSessionRetryHandler(failed *config.PurchaseExecution, session *Session mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(hasAny, nil).Maybe() mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-own", "purchases").Return(hasOwn, nil).Maybe() } + // SEC-01 constraint check (adversarial review follow-up to #1210): the + // retry path now re-evaluates the retrying session's execute:purchases + // Constraints before persisting a successor. .Maybe() so RBAC/state-gate + // tests that reject before reaching the constraint check aren't required + // to hit it; success-path tests get an unconstrained "allow" by default, + // same as setupDirectExecMocks does for the direct-execute path. + mockAuth.allowConstraintChecks() return &Handler{config: mockConfig, auth: mockAuth}, mockConfig, mockAuth } @@ -3113,7 +3120,7 @@ func TestHandler_retryPurchase_Admin_AllowsAny(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} // Admin (Administrators-group member) modeled as a retry-any holder; the @@ -3133,7 +3140,7 @@ func TestHandler_retryPurchase_RetryAny_AllowsAny(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "ops@example.com"} runSessionRetryAllowed(t, failed, session, true, false, sessionRetryReq()) @@ -3147,7 +3154,7 @@ func TestHandler_retryPurchase_RetryOwn_AllowsCreator(t *testing.T) { Error: "send failed: SES recipient mailbox full", CreatedByUserID: &creator, RetryAttemptN: 2, // already retried twice - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "u1@example.com"} newExec, updated := runSessionRetryAllowed(t, failed, session, false, true, sessionRetryReq()) @@ -3257,7 +3264,7 @@ func TestHandler_retryPurchase_PersistentFailure_NoMatch_AllowsRetry(t *testing. Status: "failed", Error: "send failed: SES throttle exceeded, please retry", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3271,7 +3278,7 @@ func TestHandler_retryPurchase_Threshold_BlocksAtFive_NoForce(t *testing.T) { Status: "failed", RetryAttemptN: 5, // already at the threshold CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3294,7 +3301,7 @@ func TestHandler_retryPurchase_Threshold_AllowsWithForce(t *testing.T) { Status: "failed", RetryAttemptN: 5, CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3309,7 +3316,7 @@ func TestHandler_retryPurchase_JustUnderThreshold_AllowsNoForce(t *testing.T) { Status: "failed", RetryAttemptN: 4, // n=4 < threshold=5 → allowed CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3366,7 +3373,7 @@ func TestHandler_retryPurchase_PreservesPlanMetadata(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3408,6 +3415,64 @@ func TestHandler_retryPurchase_AlreadyRetried_RBACBeforeLeak(t *testing.T) { } } +// TestHandler_retryPurchase_PermissionConstraintsDenied is the +// adversarial-review follow-up to #1210 (bug #3): retryPurchase must +// re-evaluate the retrying session's execute:purchases Constraints before +// persisting a successor execution, exactly as executePurchase does. Pre-fix, +// retryPurchase never called requirePermissionConstraints at all, so a +// retry-own session could relaunch its OWN over-cap failed purchase even +// though the SAME constraints would deny it as a fresh executePurchase call +// -- and a permission tightened after the original submission would never +// apply to a replay. +// +// Fails pre-fix: no HasPermissionForConstraintsAPI expectation is ever +// consulted and the handler proceeds straight to persisting the retry, so a +// pre-fix run panics on the unexpected SavePurchaseExecution call (no store +// expectation is registered for it here) instead of the request succeeding +// silently past the cap. +// Passes post-fix: the retry is denied with 403 before any execution is +// persisted. +func TestHandler_retryPurchase_PermissionConstraintsDenied(t *testing.T) { + creator := retryCallerID + failed := &config.PurchaseExecution{ + ExecutionID: retryExecID, + Status: "failed", + Error: "send failed: transient SES throttle", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "us-east-1", Term: 1, UpfrontCost: 5000}, + }, + } + session := &Session{UserID: retryCallerID, Email: "capped@example.com"} + + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, failed.ExecutionID).Return(failed, nil) + t.Cleanup(func() { mockConfig.AssertExpectations(t) }) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil) + // Caller owns the row; retry-own authorizes it (issue #907). + mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "retry-own", "purchases").Return(true, nil) + // The retrying session's execute:purchases permission is capped below + // the failed batch's $5,000 total commitment. + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, session.UserID, "execute", "purchases", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && sets[0].MaxPurchaseAmount == 5000.0 + })).Return(false, nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + handler := &Handler{config: mockConfig, auth: mockAuth} + _, err := handler.retryPurchase(context.Background(), sessionRetryReq(), retryExecID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "constraints") + mockConfig.AssertNotCalled(t, "WithTx") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") +} + // --- Regression tests for issue #408 (crypto/rand token in retry path) --- // TestPersistRetryExecution_ApprovalTokenNotUUID is the regression guard for @@ -3423,7 +3488,7 @@ func TestPersistRetryExecution_ApprovalTokenNotUUID(t *testing.T) { Status: "failed", Error: "ses throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3452,7 +3517,7 @@ func TestPersistRetryExecution_ApprovalTokenExpiresAtSet(t *testing.T) { Status: "failed", Error: "ses throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} @@ -3707,6 +3772,61 @@ func TestHandler_executePurchase_PermissionConstraintsDenied(t *testing.T) { assert.Contains(t, ce.Error(), "constraints") } +// TestHandler_executePurchase_NoUpfrontBatch_TotalCommitmentEnforced is the +// adversarial-review follow-up to #1210: a MaxPurchaseAmount cap must bind +// the batch's TOTAL commitment (upfront plus the recurring charge over the +// term), not the upfront cost alone. This rec is entirely honest, +// no-upfront data ($0 upfront, $600/mo recurring, 3-year term = $21,600 +// real commitment) -- exactly the shape that evaded the cap pre-fix, +// because a no-upfront recurring commitment's UpfrontCost is legitimately +// $0 regardless of how large the real committed spend is. +// +// Fails pre-fix: purchaseConstraintSets summed UpfrontCost only, so the +// constraint set built here would carry MaxPurchaseAmount=0. The +// mock.MatchedBy predicate below requires 21600.0, so a pre-fix call +// wouldn't match any registered expectation and testify would panic on an +// unexpected call, failing the test loudly instead of silently passing an +// under-counted amount through to a "true" auth decision. +// Passes post-fix: the constraint set carries the real $21,600 total, and +// the mocked auth denial (simulating a permission capped below that) is +// surfaced as a 403 before any execution is persisted. +func TestHandler_executePurchase_NoUpfrontBatch_TotalCommitmentEnforced(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + // No store expectations registered: the request must be rejected before + // SavePurchaseExecution / GetPendingExecutions are reached. + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + userSession := &Session{ + UserID: "ffffffff-ffff-ffff-ffff-ffffffffffff", + Email: "capped-noupfront@example.com", + } + const wantTotalCommitment = 21600.0 // $0 upfront + $600/mo * 36 months + mockAuth.On("ValidateSession", ctx, "noupfront-token").Return(userSession, nil) + mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "execute", "purchases").Return(true, nil) + mockAuth.On("GetAllowedAccountsAPI", ctx, userSession.UserID).Return([]string{}, nil) + mockAuth.On("HasPermissionForConstraintsAPI", ctx, userSession.UserID, "execute", "purchases", + mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && sets[0].MaxPurchaseAmount == wantTotalCommitment + })).Return(false, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer noupfront-token"}, + Body: `{"recommendations": [ + {"id": "rec-1", "provider": "aws", "service": "ec2", "region": "us-east-1", "count": 1, "term": 3, "payment": "no-upfront", "upfront_cost": 0.0, "monthly_cost": 600.0, "savings": 50.0} + ]}`, + } + _, err := handler.executePurchase(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got: %v", err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "constraints") +} + // TestHandler_executePurchase_UserAPIKeyConstraintsDenied is the // adversarial-review F2 regression test: a user API key with a // MaxPurchaseAmount cap must be denied when the request exceeds that cap, even diff --git a/internal/api/validation.go b/internal/api/validation.go index 66593eb6a..9b5fdf34b 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -553,6 +553,14 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e if !purchaseTermWhitelist[provider][rec.Term] { return NewClientError(400, fmt.Sprintf("recommendation %d has invalid term %d for provider %s: must be 1 or 3", idx, rec.Term, provider)) } + // A negative MonthlyCost would let recTotalCommitment's recurring leg + // subtract from the batch's total commitment, offsetting or masking a + // real upfront cost and evading a MaxPurchaseAmount cap (adversarial + // review follow-up to #1210). Nil is fine (no recurring charge); only a + // present-and-negative value is rejected. + if rec.MonthlyCost != nil && *rec.MonthlyCost < 0 { + return NewClientError(400, fmt.Sprintf("recommendation %d has negative monthly cost: %.2f", idx, *rec.MonthlyCost)) + } payment := strings.ToLower(strings.TrimSpace(rec.Payment)) // Coerce any legacy/cross-provider alias before the whitelist check so // that callers using old AWS-style tokens are transparently redirected to