fix(auth): #940 review follow-ups — split group_ceiling.go, deny malformed requested amount under a cap

internal/auth/grant_ceiling_cover.go

@@ -0,0 +1,77 @@
+package auth
+
+// Constraint-coverage comparators for the grant ceiling (group_ceiling.go):
+// they decide whether a held permission's constraints are broad enough to
+// cover a requested grant's, mirroring enforcement's semantics in
+// matchConstraints.
+
+// constraintsCover reports whether a permission constrained by held is broad
+// enough to cover a grant constrained by req.
+//
+// Mirrors the enforcement-time semantics in matchConstraints: an empty list
+// or a zero MaxPurchaseAmount means "no restriction on this dimension". So an
+// unconstrained holder covers anything, while a constrained one requires the
+// grant to name a subset of the same values and a cap no higher than its own.
+// A nil req against a constrained held is a widening and is refused.
+func constraintsCover(held, req *PermissionConstraints) bool {
+	if held == nil {
+		return true
+	}
+	if req == nil {
+		req = &PermissionConstraints{}
+	}
+	return listCovers(held.AccountIDs, req.AccountIDs) &&
+		listCovers(held.Providers, req.Providers) &&
+		listCovers(held.Services, req.Services) &&
+		listCovers(held.Regions, req.Regions) &&
+		amountCovers(held.MaxPurchaseAmount, req.MaxPurchaseAmount)
+}
+
+// listCovers reports whether every value in req is permitted by held. An
+// empty held list is "no restriction on this dimension" and covers anything;
+// a non-empty held list requires req to be a NON-EMPTY subset, so a grant can
+// never drop the restriction.
+//
+// Comparison is EXACT for every dimension. An earlier version normalised
+// (trim + lower-case) and justified it as "matching matchAllRegionsConstraint"
+// -- true for Regions only. AccountIDs, Providers and Services are enforced by
+// containsAny, an exact case-sensitive lookup, so normalising made the ceiling
+// LOOSER than enforcement there: a holder constrained to ["ACCT-1"] could
+// grant ["acct-1"], a value they cannot themselves use.
+//
+// Being exact on Regions too is stricter than its enforcement, not looser, so
+// it fails closed. A ceiling may exceed enforcement in strictness; it must
+// never fall short.
+func listCovers(held, req []string) bool {
+	if len(held) == 0 {
+		return true
+	}
+	if len(req) == 0 {
+		return false
+	}
+	permitted := make(map[string]bool, len(held))
+	for _, v := range held {
+		permitted[v] = true
+	}
+	for _, v := range req {
+		if !permitted[v] {
+			return false
+		}
+	}
+	return true
+}
+
+// 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. 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 invalidPurchaseAmountCap(heldMax) {
+		return false
+	}
+	if heldMax == 0 {
+		return true
+	}
+	return reqMax > 0 && reqMax <= heldMax
+}

internal/auth/group_ceiling.go

@@ -446,74 +446,3 @@ func permissionCoveredBy(set []Permission, req Permission) bool {
 	}
 	return false
 }
-
-// constraintsCover reports whether a permission constrained by held is broad
-// enough to cover a grant constrained by req.
-//
-// Mirrors the enforcement-time semantics in matchConstraints: an empty list
-// or a zero MaxPurchaseAmount means "no restriction on this dimension". So an
-// unconstrained holder covers anything, while a constrained one requires the
-// grant to name a subset of the same values and a cap no higher than its own.
-// A nil req against a constrained held is a widening and is refused.
-func constraintsCover(held, req *PermissionConstraints) bool {
-	if held == nil {
-		return true
-	}
-	if req == nil {
-		req = &PermissionConstraints{}
-	}
-	return listCovers(held.AccountIDs, req.AccountIDs) &&
-		listCovers(held.Providers, req.Providers) &&
-		listCovers(held.Services, req.Services) &&
-		listCovers(held.Regions, req.Regions) &&
-		amountCovers(held.MaxPurchaseAmount, req.MaxPurchaseAmount)
-}
-
-// listCovers reports whether every value in req is permitted by held. An
-// empty held list is "no restriction on this dimension" and covers anything;
-// a non-empty held list requires req to be a NON-EMPTY subset, so a grant can
-// never drop the restriction.
-//
-// Comparison is EXACT for every dimension. An earlier version normalised
-// (trim + lower-case) and justified it as "matching matchAllRegionsConstraint"
-// -- true for Regions only. AccountIDs, Providers and Services are enforced by
-// containsAny, an exact case-sensitive lookup, so normalising made the ceiling
-// LOOSER than enforcement there: a holder constrained to ["ACCT-1"] could
-// grant ["acct-1"], a value they cannot themselves use.
-//
-// Being exact on Regions too is stricter than its enforcement, not looser, so
-// it fails closed. A ceiling may exceed enforcement in strictness; it must
-// never fall short.
-func listCovers(held, req []string) bool {
-	if len(held) == 0 {
-		return true
-	}
-	if len(req) == 0 {
-		return false
-	}
-	permitted := make(map[string]bool, len(held))
-	for _, v := range held {
-		permitted[v] = true
-	}
-	for _, v := range req {
-		if !permitted[v] {
-			return false
-		}
-	}
-	return true
-}
-
-// 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. 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 invalidPurchaseAmountCap(heldMax) {
-		return false
-	}
-	if heldMax == 0 {
-		return true
-	}
-	return reqMax > 0 && reqMax <= heldMax
-}

internal/auth/negative_purchase_cap_test.go

@@ -49,6 +49,24 @@ func TestMatchPurchaseAmountConstraint_ValidCapsUnchanged(t *testing.T) {
 	}
 }
 
+// A malformed REQUESTED amount (negative, NaN or +/-Inf) is denied whenever a
+// cap applies. reqMax is computed server-side, so this is defense in depth: a
+// NaN in particular compares false against the cap and used to slip under it.
+// With no cap (permMax == 0) the amount dimension is unrestricted, so the
+// request is allowed, consistent with a permission that carries no
+// constraints at all (checkPermissionConstraints never reaches the matcher).
+func TestMatchPurchaseAmountConstraint_MalformedRequestedAmount(t *testing.T) {
+	malformed := []float64{-1, -0.01, -math.MaxFloat64, math.Inf(-1), math.Inf(1), math.NaN()}
+	for _, reqMax := range malformed {
+		assert.False(t, matchPurchaseAmountConstraint(1000, reqMax),
+			"capped permission must deny malformed request %v", reqMax)
+		assert.False(t, matchPurchaseAmountConstraint(math.MaxFloat64, reqMax),
+			"capped permission must deny malformed request %v even at the largest cap", reqMax)
+		assert.True(t, matchPurchaseAmountConstraint(0, reqMax),
+			"uncapped permission leaves the amount dimension unrestricted for %v", reqMax)
+	}
+}
+
 // Through the real service enforcement path a handler calls at purchase and
 // approval time: a group permission stored with a negative cap refuses even a
 // $1 purchase, while a sibling positive cap is enforced exactly as before.

internal/auth/purchase_amount_cap.go

@@ -17,14 +17,29 @@ import (
 // rather than falling through to "no cap" (issue #83). Write paths refuse such
 // a value (validateMaxPurchaseAmount), but rows already stored that way must
 // still fail closed here.
+//
+// When a cap applies, a malformed REQUESTED amount (negative, NaN or +/-Inf,
+// see invalidRequestedAmount) is denied too: NaN compares false against the
+// cap and a negative amount sits below it, so either would otherwise pass.
+// reqMax is computed server-side, so this is defense in depth. With no cap the
+// amount dimension is unrestricted and the request is allowed whatever its
+// amount, matching a permission with no constraints at all
+// (checkPermissionConstraints never calls the matcher for one).
 func matchPurchaseAmountConstraint(permMax, reqMax float64) bool {
 	if invalidPurchaseAmountCap(permMax) {
 		return false
 	}
-	if permMax > 0 && reqMax > permMax {
-		return false
+	if permMax == 0 {
+		return true
 	}
-	return true
+	return !invalidRequestedAmount(reqMax) && reqMax <= permMax
+}
+
+// invalidRequestedAmount reports whether a requested purchase amount cannot be
+// a real amount: negative, NaN or infinite. Zero is valid (a request that
+// carries no amount, e.g. a non-money permission check).
+func invalidRequestedAmount(v float64) bool {
+	return v < 0 || math.IsNaN(v) || math.IsInf(v, 0)
 }
 
 // invalidPurchaseAmountCap reports whether a stored MaxPurchaseAmount is