fix(auth): fail closed on a negative MaxPurchaseAmount spend cap

internal/api/handler_apikeys.go

@@ -90,6 +90,8 @@ func (h *Handler) createAPIKey(ctx context.Context, req *events.LambdaFunctionUR
 //     500 (issue #61).
 //   - ErrAPIKeyExpiresAtRequired / ErrAPIKeyExpiresAtTooFar / ErrAPIKeyExpiresAtInPast
 //     -> 400: caller-correctable request validation.
+//   - ErrInvalidPermission -> 400: a malformed permission entry, e.g. a
+//     negative MaxPurchaseAmount (issue #83).
 //   - All other errors pass through unchanged for handleRequestError to
 //     render as 500.
 func mapCreateAPIKeyError(err error) error {
@@ -99,7 +101,8 @@ func mapCreateAPIKeyError(err error) error {
 	case errors.Is(err, auth.ErrEmptyAPIKeyPermissions),
 		errors.Is(err, auth.ErrAPIKeyExpiresAtRequired),
 		errors.Is(err, auth.ErrAPIKeyExpiresAtTooFar),
-		errors.Is(err, auth.ErrAPIKeyExpiresAtInPast):
+		errors.Is(err, auth.ErrAPIKeyExpiresAtInPast),
+		errors.Is(err, auth.ErrInvalidPermission):
 		return NewClientError(400, err.Error())
 	}
 	return fmt.Errorf("failed to create API key: %w", err)

internal/api/handler_ri_exchange.go

@@ -1069,7 +1069,8 @@ func (h *Handler) resolveAzureExchangeAccountID(ctx context.Context, subscriptio
 //
 // When the sentinel check fails, a second call with the amount dimension
 // neutralized (MaxPurchaseAmount: 0, which matchPurchaseAmountConstraint
-// always treats as satisfied) disambiguates the cause: if that second call
+// treats as satisfied by any well-formed cap; a malformed negative cap still
+// denies it, issue #83) disambiguates the cause: if that second call
 // still fails, some other dimension (account/provider/service/region) is
 // the real reason and its error is returned unchanged; otherwise the amount
 // constraint was specifically the blocker and a currency-specific 403 is

internal/api/negative_purchase_cap_test.go

@@ -0,0 +1,103 @@
+package api
+
+// Negative MaxPurchaseAmount fails closed at every money-path enforcement
+// point (issue #83). The mock auth service answers constraint checks with the
+// real auth.PermissionsAllowForConstraintSets, so these exercise the
+// production matcher. Nothing here reaches a purchase manager or provider.
+
+import (
+	"context"
+	"math/big"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func capRec(upfront float64) config.RecommendationRecord {
+	acct := "123456789012"
+	return config.RecommendationRecord{
+		ID: "rec-1", Provider: "aws", Service: "ec2", Region: "us-east-1",
+		CloudAccountID: &acct, Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: upfront,
+	}
+}
+
+func handlerWithCap(action string, maxAmount float64) *Handler {
+	mockAuth := new(MockAuthService)
+	mockAuth.grantPermissions([]auth.Permission{{
+		Action: action, Resource: auth.ResourcePurchases,
+		Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: maxAmount},
+	}})
+	return &Handler{auth: mockAuth}
+}
+
+func TestEnforcePurchaseConstraints_NegativeCapRefuses(t *testing.T) {
+	ctx := context.Background()
+	session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"}
+	// The execute, direct-execute and approval verbs all share this check.
+	for _, action := range []string{auth.ActionExecute, auth.ActionExecuteAny, auth.ActionApproveAny} {
+		t.Run(action+"/negative refused", func(t *testing.T) {
+			h := handlerWithCap(action, -1)
+			err := h.enforcePurchaseConstraints(ctx, session, action, []config.RecommendationRecord{capRec(1)})
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected a clientError, got %v", err)
+			assert.Equal(t, 403, ce.code)
+		})
+		t.Run(action+"/positive cap enforced as before", func(t *testing.T) {
+			h := handlerWithCap(action, 1000)
+			require.NoError(t, h.enforcePurchaseConstraints(ctx, session, action, []config.RecommendationRecord{capRec(500)}))
+			err := h.enforcePurchaseConstraints(ctx, session, action, []config.RecommendationRecord{capRec(5000)})
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok)
+			assert.Equal(t, 403, ce.code)
+		})
+		t.Run(action+"/zero is no cap", func(t *testing.T) {
+			h := handlerWithCap(action, 0)
+			require.NoError(t, h.enforcePurchaseConstraints(ctx, session, action, []config.RecommendationRecord{capRec(1_000_000)}))
+		})
+	}
+}
+
+func TestCheckAzureExecuteConstraints_NegativeCapRefuses(t *testing.T) {
+	ctx := context.Background()
+	session := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd"}
+	grant := func(maxAmount float64) *Handler {
+		mockAuth := new(MockAuthService)
+		mockAuth.grantPermissions([]auth.Permission{{
+			Action: auth.ActionExecute, Resource: auth.ResourceRIExchange,
+			Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: maxAmount},
+		}})
+		return &Handler{auth: mockAuth}
+	}
+	regions := []string{"eastus"}
+	small := big.NewRat(1, 1)
+
+	for _, currency := range []string{"USD", "EUR"} {
+		t.Run(currency+"/negative refused", func(t *testing.T) {
+			err := grant(-1).checkAzureExecuteConstraints(ctx, session,
+				AzureExecuteExchangeRequestBody{Currency: currency}, "sub-1", small, regions)
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected a clientError, got %v", err)
+			assert.Equal(t, 403, ce.code)
+		})
+	}
+	t.Run("USD/positive cap enforced as before", func(t *testing.T) {
+		h := grant(1000)
+		require.NoError(t, h.checkAzureExecuteConstraints(ctx, session,
+			AzureExecuteExchangeRequestBody{Currency: "USD"}, "sub-1", big.NewRat(500, 1), regions))
+		require.Error(t, h.checkAzureExecuteConstraints(ctx, session,
+			AzureExecuteExchangeRequestBody{Currency: "USD"}, "sub-1", big.NewRat(5000, 1), regions))
+	})
+}
+
+func TestMapCreateAPIKeyError_InvalidPermissionIs400(t *testing.T) {
+	err := mapCreateAPIKeyError(auth.ErrInvalidPermission)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a clientError, got %v", err)
+	assert.Equal(t, 400, ce.code)
+}

internal/auth/errors.go

@@ -53,8 +53,9 @@ var (
 	// malformed input, NOT a request for the "*" wildcard: the group-edit
 	// form renders an empty stored resource as the selected "All (*)" option
 	// and saves it back as view:*, and the same widening is reachable
-	// directly through the API because nothing validated the list. Mapped to
-	// 400 (issues #1730, #1550).
+	// directly through the API because nothing validated the list. Also
+	// returned for a negative or NaN MaxPurchaseAmount on a group or API-key
+	// write (issue #83). Mapped to 400 (issues #1730, #1550).
 	ErrInvalidPermission = errors.New("invalid permission")
 
 	// ErrSystemManagedGroup is returned when a write targets one of the

internal/auth/group_ceiling.go

@@ -231,11 +231,15 @@ func validateRequestedPermissions(requested []Permission) error {
 // would break edits of data this endpoint already accepted.
 //
 // MaxPurchaseAmount has no blank form: zero already means "no cap" on both
-// sides of the comparison, so there is nothing here to disambiguate.
+// sides of the comparison. A negative or NaN cap is refused instead
+// (validateMaxPurchaseAmount, issue #83).
 func validateConstraintEntries(permIndex int, p Permission) error {
 	if p.Constraints == nil {
 		return nil
 	}
+	if err := validateMaxPurchaseAmount(permIndex, p); err != nil {
+		return err
+	}
 	dimensions := []struct {
 		name   string
 		values []string
@@ -502,9 +506,13 @@ func listCovers(held, req []string) bool {
 // amountCovers reports whether a grant capped at reqMax stays within a holder
 // capped at heldMax. Zero means "no cap" on both sides (mirroring
 // matchPurchaseAmountConstraint), so a capped holder cannot hand out an
-// uncapped grant.
+// uncapped grant. A malformed (negative/NaN) held cap denies everything at
+// enforcement, so it covers nothing here either (issue #83).
 func amountCovers(heldMax, reqMax float64) bool {
-	if heldMax <= 0 {
+	if invalidPurchaseAmountCap(heldMax) {
+		return false
+	}
+	if heldMax == 0 {
 		return true
 	}
 	return reqMax > 0 && reqMax <= heldMax