diff --git a/internal/auth/grant_ceiling_cover.go b/internal/auth/grant_ceiling_cover.go new file mode 100644 index 000000000..ba45edcbb --- /dev/null +++ b/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 +} diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go index 77f385fff..665078897 100644 --- a/internal/auth/group_ceiling.go +++ b/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 -} diff --git a/internal/auth/negative_purchase_cap_test.go b/internal/auth/negative_purchase_cap_test.go index b49a8ef7c..c9cda9d3a 100644 --- a/internal/auth/negative_purchase_cap_test.go +++ b/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. diff --git a/internal/auth/purchase_amount_cap.go b/internal/auth/purchase_amount_cap.go index 4b99b8da0..fab977f10 100644 --- a/internal/auth/purchase_amount_cap.go +++ b/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