From 4259c537468e8d2b749bd3ca0b954f9cf9be4d8a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 16:03:03 +0100 Subject: [PATCH 1/2] fix(api): treat re-enabling a disabled entry on an armed plan as an escalation planArmEscalates compared providers and services, which ignore Enabled, so a PUT that turned a disabled entry back on skipped the execute:purchases arming grant and the armed plan started buying for it again. A disabled-to-enabled flip on an armed plan now needs the grant, like a new service does. The stored plan fixture now matches real data (entries created through the API are enabled). Closes #1777 Co-Authored-By: Claude Sonnet 5.5 --- .../api/auto_purchase_bound_binding_test.go | 6 ++--- internal/api/auto_purchase_grant.go | 16 ++++++++++++- internal/api/auto_purchase_grant_test.go | 12 +++++++++- internal/api/auto_purchase_scope_test.go | 24 +++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/internal/api/auto_purchase_bound_binding_test.go b/internal/api/auto_purchase_bound_binding_test.go index 6758f50f3..558452659 100644 --- a/internal/api/auto_purchase_bound_binding_test.go +++ b/internal/api/auto_purchase_bound_binding_test.go @@ -55,8 +55,8 @@ func TestPlanDisarm_AllowsStrippedBoundPlan(t *testing.T) { } } -// Re-enabling a stripped entry widens an armed plan even though -// planArmEscalates cannot see it (its service sets ignore Enabled). +// Re-enabling a stripped entry widens an armed plan: the arming gate sees it +// (#1777), and the binding refusal runs before the gate, so the answer is 409. func TestUpdatePlanGated_RefusesReenablingStrippedEntry(t *testing.T) { before := strippedBoundPlan(true) before.Services = map[string]config.ServiceConfig{ @@ -68,7 +68,7 @@ func TestUpdatePlanGated_RefusesReenablingStrippedEntry(t *testing.T) { g2 := after.Services["aws/rds#g2"] g2.Enabled = true after.Services["aws/rds#g2"] = g2 - require.False(t, planArmEscalates(before, &after), "the gap: the arming gate alone misses this write") + require.True(t, planArmEscalates(before, &after), "a re-enabled entry is an arming escalation") store := new(MockConfigStore) h, req := apgCaller{perms: apsGrant(nil)}.handlerAndReq(store, `{}`) diff --git a/internal/api/auto_purchase_grant.go b/internal/api/auto_purchase_grant.go index 598abc271..6faed99ab 100644 --- a/internal/api/auto_purchase_grant.go +++ b/internal/api/auto_purchase_grant.go @@ -73,7 +73,21 @@ func planArmEscalates(before, after *config.PurchasePlan) bool { } return !subsetOf(afterProviders, beforeProviders) || !subsetOf(config.DerivePlanServices(after), config.DerivePlanServices(before)) || - boundGroupsEscalate(before, after) + boundGroupsEscalate(before, after) || + reEnablesEntry(before, after) +} + +// reEnablesEntry reports whether an entry that was disabled is enabled again. +// The armed plan then buys for it, which the provider and service sets above +// cannot see because they ignore Enabled (#1777). Unlike enablesEntry, an +// absent entry is not counted: a new service is already caught by those sets. +func reEnablesEntry(before, after *config.PurchasePlan) bool { + for key, svc := range after.Services { + if prev, ok := before.Services[key]; ok && svc.Enabled && !prev.Enabled { + return true + } + } + return false } func subsetOf(sub, super []string) bool { diff --git a/internal/api/auto_purchase_grant_test.go b/internal/api/auto_purchase_grant_test.go index 78eaff56e..ac202f58c 100644 --- a/internal/api/auto_purchase_grant_test.go +++ b/internal/api/auto_purchase_grant_test.go @@ -40,7 +40,7 @@ func apgStoredPlan(armed bool, capUSD *float64) *config.PurchasePlan { return &config.PurchasePlan{ ID: apgPlanID, Name: "p", Enabled: true, AutoPurchase: armed, MaxStepCommitment: capUSD, NotificationDaysBefore: 3, CreatedAt: time.Now().AddDate(0, -1, 0), - Services: map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds"}}, + Services: map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Enabled: true}}, } } @@ -114,6 +114,16 @@ func TestAutoPurchaseGate_PlanWrites(t *testing.T) { {"put 100.10 (binary-float trap) over 100.10 is the same cap: admin allowed", admin, "put", apgStoredPlan(true, apgCap(100.10)), `{"name":"p","enabled":true,"auto_purchase":true,"max_step_commitment":100.10}`, false}, {"put one cent more is a raise: admin refused", admin, "put", apgStoredPlan(true, apgCap(1000)), `{"name":"p","enabled":true,"auto_purchase":true,"max_step_commitment":1000.01}`, true}, {"put omitting the cap keeps it: admin allowed", admin, "put", apgStoredPlan(true, apgCap(1000)), `{"name":"p","enabled":true,"auto_purchase":true}`, false}, + {"put re-enables a disabled entry on an armed plan: admin refused", admin, "put", func() *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.Services = map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Enabled: false}} + return p + }(), `{"name":"p","enabled":true,"auto_purchase":true,"provider":"aws","service":"rds"}`, true}, + {"put re-enables a disabled entry on an armed plan: execute:purchases allowed", granted, "put", func() *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.Services = map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Enabled: false}} + return p + }(), `{"name":"p","enabled":true,"auto_purchase":true,"provider":"aws","service":"rds"}`, false}, {"put disarms with admin API key allowed", key, "put", apgStoredPlan(true, apgCap(1000)), `{"name":"p","enabled":true,"auto_purchase":false}`, false}, } for _, tc := range tests { diff --git a/internal/api/auto_purchase_scope_test.go b/internal/api/auto_purchase_scope_test.go index e3dd1040f..e958a5763 100644 --- a/internal/api/auto_purchase_scope_test.go +++ b/internal/api/auto_purchase_scope_test.go @@ -278,6 +278,30 @@ func TestPlanArmEscalates_ProviderAndServiceWidening(t *testing.T) { }) } +// A disabled entry turned back on starts buying under the existing arming, so +// it needs the grant like a new service does (#1777). +func TestPlanArmEscalates_ReEnabledEntry(t *testing.T) { + plan := func(enabled bool) *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.Services = map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Enabled: enabled}} + return p + } + for _, tc := range []struct { + name string + before, after bool + want bool + }{ + {"disabled to enabled", false, true, true}, + {"enabled stays enabled", true, true, false}, + {"disabled stays disabled", false, false, false}, + {"enabled to disabled is a narrowing", true, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, planArmEscalates(plan(tc.before), plan(tc.after))) + }) + } +} + // A provider mismatch is a malformed request, so it is a 400 whatever the // plan's armed state; the 403 arming gate must not mask it. func TestAutoPurchaseScope_RetargetProviderMismatchBeforeArmingGate(t *testing.T) { From c07148df77a4e84c6811a197f89c5d624f47e80d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 18:56:38 +0100 Subject: [PATCH 2/2] style(api): range over the services map by key in reEnablesEntry golangci-lint's gocritic rangeValCopy flagged the 312-byte ServiceConfig copy per iteration. Index the map instead. No behavior change. Refs #1777 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/auto_purchase_grant.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/auto_purchase_grant.go b/internal/api/auto_purchase_grant.go index 6faed99ab..e480580d8 100644 --- a/internal/api/auto_purchase_grant.go +++ b/internal/api/auto_purchase_grant.go @@ -82,8 +82,8 @@ func planArmEscalates(before, after *config.PurchasePlan) bool { // cannot see because they ignore Enabled (#1777). Unlike enablesEntry, an // absent entry is not counted: a new service is already caught by those sets. func reEnablesEntry(before, after *config.PurchasePlan) bool { - for key, svc := range after.Services { - if prev, ok := before.Services[key]; ok && svc.Enabled && !prev.Enabled { + for key := range after.Services { + if prev, ok := before.Services[key]; ok && after.Services[key].Enabled && !prev.Enabled { return true } }