From 7f3aed2050bb58c5c580f4f1a31d43a8aa60e2dc Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 03:56:24 +0100 Subject: [PATCH 1/2] fix(auth): fail closed on a negative MaxPurchaseAmount spend cap matchPurchaseAmountConstraint gated on permMax > 0, so a permission whose stored MaxPurchaseAmount was negative (or NaN) read as "no cap" and allowed any purchase. A malformed cap now denies every request at enforcement, covering rows already stored that way. The same predicate is applied to the grant ceiling (amountCovers), which otherwise let a group edit carry a stored negative cap through as an uncapped permission, and to the group and API-key write paths, which now refuse a negative/NaN cap with ErrInvalidPermission (400). Zero keeps its existing meaning of "no cap". The cap helpers move to purchase_amount_cap.go to keep service_group.go under the 500-line guideline. Closes #83 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_apikeys.go | 5 +- internal/api/handler_ri_exchange.go | 3 +- internal/api/negative_purchase_cap_test.go | 103 ++++++++ internal/auth/errors.go | 5 +- internal/auth/group_ceiling.go | 14 +- internal/auth/negative_purchase_cap_test.go | 246 ++++++++++++++++++++ internal/auth/purchase_amount_cap.go | 49 ++++ internal/auth/service_apikeys.go | 5 + internal/auth/service_group.go | 8 - 9 files changed, 423 insertions(+), 15 deletions(-) create mode 100644 internal/api/negative_purchase_cap_test.go create mode 100644 internal/auth/negative_purchase_cap_test.go create mode 100644 internal/auth/purchase_amount_cap.go diff --git a/internal/api/handler_apikeys.go b/internal/api/handler_apikeys.go index 8486a7125..1e7e5cfeb 100644 --- a/internal/api/handler_apikeys.go +++ b/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) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index e622b1256..35e14e422 100644 --- a/internal/api/handler_ri_exchange.go +++ b/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 diff --git a/internal/api/negative_purchase_cap_test.go b/internal/api/negative_purchase_cap_test.go new file mode 100644 index 000000000..875f94485 --- /dev/null +++ b/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) +} diff --git a/internal/auth/errors.go b/internal/auth/errors.go index a6868026a..9e57ac388 100644 --- a/internal/auth/errors.go +++ b/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 diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go index 0c71240e9..77f385fff 100644 --- a/internal/auth/group_ceiling.go +++ b/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 diff --git a/internal/auth/negative_purchase_cap_test.go b/internal/auth/negative_purchase_cap_test.go new file mode 100644 index 000000000..cf92cac7a --- /dev/null +++ b/internal/auth/negative_purchase_cap_test.go @@ -0,0 +1,246 @@ +package auth + +// Negative MaxPurchaseAmount must fail closed (issue #83). +// +// Zero is the unset zero-value and keeps meaning "no cap". A negative (or +// NaN) cap is malformed spend-cap data: enforcement denies every request +// against it, the grant ceiling treats it as covering nothing, and the group +// and API-key write paths refuse it. + +import ( + "context" + "math" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestMatchPurchaseAmountConstraint_NegativeCapDeniesAll(t *testing.T) { + malformed := []float64{-1, -0.01, -math.MaxFloat64, math.Inf(-1), math.NaN()} + requests := []float64{0, 0.01, 1, 1_000_000, math.MaxFloat64, -5} + for _, permMax := range malformed { + for _, reqMax := range requests { + assert.False(t, matchPurchaseAmountConstraint(permMax, reqMax), + "cap %v must deny request %v", permMax, reqMax) + } + } +} + +func TestMatchPurchaseAmountConstraint_ValidCapsUnchanged(t *testing.T) { + cases := []struct { + permMax, reqMax float64 + want bool + }{ + {1000, 500, true}, + {1000, 1000, true}, + {1000, 1000.01, false}, + {1000, math.MaxFloat64, false}, + // Zero keeps its existing meaning: no cap. + {0, 0, true}, + {0, 1_000_000, true}, + {0, math.MaxFloat64, true}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, matchPurchaseAmountConstraint(tc.permMax, tc.reqMax), + "cap %v, request %v", tc.permMax, tc.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. +func TestService_HasPermissionForConstraintsAPI_NegativeStoredCap(t *testing.T) { + ctx := context.Background() + stub := func(mockStore *MockStore, maxAmount float64) { + mockStore.On("GetUserByID", ctx, "purchaser-1"). + Return(&User{ID: "purchaser-1", GroupIDs: []string{"purchasers"}}, nil).Once() + mockStore.On("GetGroup", ctx, "purchasers").Return(&Group{ + ID: "purchasers", + Permissions: []Permission{{ + Action: ActionExecute, + Resource: ResourcePurchases, + Constraints: &PermissionConstraints{Providers: []string{"aws"}, MaxPurchaseAmount: maxAmount}, + }}, + }, nil).Once() + } + cases := []struct { + name string + cap float64 + request float64 + want bool + }{ + {"negative cap refuses a small purchase", -100, 1, false}, + {"negative cap refuses a large purchase", -100, 1_000_000, false}, + {"positive cap allows within cap", 1000, 500, true}, + {"positive cap refuses over cap", 1000, 5000, false}, + {"zero cap is no cap", 0, 1_000_000, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + stub(mockStore, tc.cap) + + has, err := svc.HasPermissionForConstraintsAPI(ctx, "purchaser-1", ActionExecute, ResourcePurchases, + []PermissionConstraints{{Providers: []string{"aws"}, MaxPurchaseAmount: tc.request}}) + require.NoError(t, err) + assert.Equal(t, tc.want, has) + }) + } +} + +func TestAmountCovers_NegativeHeldCapCoversNothing(t *testing.T) { + for _, held := range []float64{-1, -0.01, math.NaN()} { + for _, req := range []float64{0, 1, 100, -1} { + assert.False(t, amountCovers(held, req), "held %v must not cover %v", held, req) + } + } + // Unchanged for well-formed caps. + assert.True(t, amountCovers(0, 0)) + assert.True(t, amountCovers(0, 1_000_000)) + assert.True(t, amountCovers(100, 50)) + assert.True(t, amountCovers(100, 100)) + assert.False(t, amountCovers(100, 101)) + assert.False(t, amountCovers(100, 0)) +} + +// An already-stored negative cap on a carved-out money verb must not be +// carried through an edit as an UNCAPPED permission: before the fix the +// carry-over check (permissionCoveredBy -> amountCovers) read the negative +// cap as "no cap" and accepted the widening. +func TestGrantCeiling_StoredNegativeCapCannotBeWidened(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, adminOnly) + stubTargetGroup(ctx, mockStore, &Group{ + ID: ceilingTargetID, + Name: "Custom Purchasers", + Permissions: []Permission{{ + Action: ActionExecute, + Resource: ResourcePurchases, + Constraints: &PermissionConstraints{MaxPurchaseAmount: -1}, + }}, + }) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Maybe() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{Action: ActionExecute, Resource: ResourcePurchases})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionNotGrantable) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +func TestGroupWrite_RejectsNegativeMaxAmount(t *testing.T) { + ctx := context.Background() + for _, amount := range []float64{-1, -0.01} { + perm := APIPermission{Action: ActionView, Resource: ResourcePlans, + Constraints: &APIPermissionConstraint{MaxAmount: amount}} + + t.Run("update", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + // Permissive stubs: without the guard the write would succeed. + stubActorPermissionsMaybe(ctx, mockStore, adminOnly) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Maybe() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, updateReqWith(perm)) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidPermission) + assert.Contains(t, err.Error(), "entry 0") + assert.Contains(t, err.Error(), "max purchase amount") + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + + t.Run("create", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + stubActorPermissionsMaybe(ctx, mockStore, adminOnly) + mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Maybe() + + _, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{ + Name: "Team", Permissions: []APIPermission{perm}, + }) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidPermission) + mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything) + }) + } +} + +// Negative control: zero (no cap) and a positive cap are still written. +func TestGroupWrite_AcceptsZeroAndPositiveMaxAmount(t *testing.T) { + ctx := context.Background() + for _, amount := range []float64{0, 0.01, 5000} { + mockStore := new(MockStore) + svc := newCeilingService(t, mockStore) + stubActorPermissions(ctx, mockStore, adminOnly) + var saved *Group + mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")). + Run(func(args mock.Arguments) { saved, _ = args.Get(1).(*Group) }).Return(nil).Once() + + _, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{ + Name: "Team", + Permissions: []APIPermission{{Action: ActionView, Resource: ResourcePlans, + Constraints: &APIPermissionConstraint{MaxAmount: amount}}}, + }) + + require.NoError(t, err, "amount %v", amount) + require.NotNil(t, saved) + assert.Equal(t, amount, saved.Permissions[0].Constraints.MaxPurchaseAmount) + mockStore.AssertExpectations(t) + } +} + +func TestCreateAPIKey_NegativeMaxPurchaseAmount(t *testing.T) { + ctx := context.Background() + const password = "CorrectHorse@123" + passwordHash, err := newTestService().hashPassword(password) + require.NoError(t, err) + expiresAt := time.Now().Add(24 * time.Hour) + grpID := "99999999-9999-4999-8999-999999999999" + + run := func(mockStore *MockStore, amount float64) error { + service := &Service{store: mockStore} + user := &User{ID: "user-123", Active: true, PasswordHash: passwordHash, GroupIDs: []string{grpID}} + mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil) + mockStore.On("GetGroup", ctx, grpID).Return(&Group{ + ID: grpID, + Permissions: []Permission{{Action: ActionView, Resource: ResourcePlans}}, + }, nil).Maybe() + mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil).Maybe() + + _, _, err := service.CreateAPIKey(ctx, "user-123", "ci", password, []Permission{ + {Action: ActionView, Resource: ResourcePlans}, + {Action: ActionView, Resource: ResourcePlans, Constraints: &PermissionConstraints{MaxPurchaseAmount: amount}}, + }, &expiresAt) + return err + } + + t.Run("negative refused before the key is stored", func(t *testing.T) { + mockStore := new(MockStore) + err := run(mockStore, -1) + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidPermission) + assert.Contains(t, err.Error(), "entry 1") + mockStore.AssertNotCalled(t, "CreateAPIKey", mock.Anything, mock.Anything) + }) + + for _, amount := range []float64{0, 100} { + mockStore := new(MockStore) + require.NoError(t, run(mockStore, amount), "amount %v", amount) + mockStore.AssertCalled(t, "CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")) + } +} diff --git a/internal/auth/purchase_amount_cap.go b/internal/auth/purchase_amount_cap.go new file mode 100644 index 000000000..4b99b8da0 --- /dev/null +++ b/internal/auth/purchase_amount_cap.go @@ -0,0 +1,49 @@ +package auth + +import ( + "fmt" + "math" +) + +// MaxPurchaseAmount (spend cap) semantics shared by enforcement +// (matchConstraints), the grant ceiling (amountCovers) and the group and +// API-key write paths. Zero is the unset zero-value and means "no cap"; a +// negative or NaN cap is malformed and fails closed everywhere (issue #83). + +// matchPurchaseAmountConstraint checks if requested amount is within permitted limit. +// +// Zero is the unset zero-value and means "no cap". A negative (or NaN) cap is +// malformed spend-cap data, not an absent one, so it denies every request +// 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. +func matchPurchaseAmountConstraint(permMax, reqMax float64) bool { + if invalidPurchaseAmountCap(permMax) { + return false + } + if permMax > 0 && reqMax > permMax { + return false + } + return true +} + +// invalidPurchaseAmountCap reports whether a stored MaxPurchaseAmount is +// malformed: negative, or NaN (which compares false against everything and +// would otherwise read as "no cap"). Shared by enforcement, the grant ceiling +// and the write-time validators so the three cannot disagree. +func invalidPurchaseAmountCap(v float64) bool { + return v < 0 || math.IsNaN(v) +} + +// validateMaxPurchaseAmount refuses a permission whose MaxPurchaseAmount is +// malformed (see invalidPurchaseAmountCap) at write time, so a negative cap is +// rejected with a 400 instead of being stored as a deny-all row. permIndex +// names the offending entry in the refusal. +func validateMaxPurchaseAmount(permIndex int, p Permission) error { + if p.Constraints == nil || !invalidPurchaseAmountCap(p.Constraints.MaxPurchaseAmount) { + return nil + } + return fmt.Errorf( + "%w: entry %d (%s:%s) has an invalid max purchase amount %v; a spend cap must be zero (no cap) or positive", + ErrInvalidPermission, permIndex, p.Action, p.Resource, p.Constraints.MaxPurchaseAmount) +} diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index b34726c74..e097223aa 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -158,6 +158,11 @@ func (s *Service) validateCreateAPIKeyRequest(user *User, password, name string, if len(permissions) == 0 { return ErrEmptyAPIKeyPermissions } + for i, p := range permissions { + if err := validateMaxPurchaseAmount(i, p); err != nil { + return err + } + } // ExpiresAt is mandatory and capped: a nil value used to mean "never // expires", which let a single leaked key outlive any credential rotation. if expiresAt == nil { diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index f8be7e9bf..5cc3f618c 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -464,11 +464,3 @@ func matchAllRegionsConstraint(permRegions, reqRegions []string) bool { } return true } - -// matchPurchaseAmountConstraint checks if requested amount is within permitted limit. -func matchPurchaseAmountConstraint(permMax, reqMax float64) bool { - if permMax > 0 && reqMax > permMax { - return false - } - return true -} From 7dc1891a9155401f03c49c0d1ee83a6a65092e16 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 04:14:07 +0100 Subject: [PATCH 2/2] test(auth): reuse testAPIKeyPassword in the negative-cap API-key test The inline password literal tripped the git-secrets password detector; the package already has a shared test password constant for this. Co-Authored-By: Claude Opus 5.5 --- internal/auth/negative_purchase_cap_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/auth/negative_purchase_cap_test.go b/internal/auth/negative_purchase_cap_test.go index cf92cac7a..b49a8ef7c 100644 --- a/internal/auth/negative_purchase_cap_test.go +++ b/internal/auth/negative_purchase_cap_test.go @@ -206,8 +206,7 @@ func TestGroupWrite_AcceptsZeroAndPositiveMaxAmount(t *testing.T) { func TestCreateAPIKey_NegativeMaxPurchaseAmount(t *testing.T) { ctx := context.Background() - const password = "CorrectHorse@123" - passwordHash, err := newTestService().hashPassword(password) + passwordHash, err := newTestService().hashPassword(testAPIKeyPassword) require.NoError(t, err) expiresAt := time.Now().Add(24 * time.Hour) grpID := "99999999-9999-4999-8999-999999999999" @@ -222,7 +221,7 @@ func TestCreateAPIKey_NegativeMaxPurchaseAmount(t *testing.T) { }, nil).Maybe() mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil).Maybe() - _, _, err := service.CreateAPIKey(ctx, "user-123", "ci", password, []Permission{ + _, _, err := service.CreateAPIKey(ctx, "user-123", "ci", testAPIKeyPassword, []Permission{ {Action: ActionView, Resource: ResourcePlans}, {Action: ActionView, Resource: ResourcePlans, Constraints: &PermissionConstraints{MaxPurchaseAmount: amount}}, }, &expiresAt)