From 8d5f264016f91913d2ebfdbb88353125deb1f3e7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 22:57:48 +0200 Subject: [PATCH] sec(api): refuse createPlan target_accounts outside the caller's allowed_accounts POST /api/plans checked only create:plans, UUID shape and provider match, so a scoped user could create an auto-purchase plan for an account they hold no entitlement to. Extract requireAccountsAccess from requirePlanAccountsAccess and run it in createPlan before the plan row is written. updatePlan/patchPlan cannot change target_accounts, and PUT /api/plans/:id/accounts already uses requirePlanAccountsAccess. Closes #95 --- internal/api/handler_plans.go | 9 ++- internal/api/plan_accounts_scope_test.go | 80 ++++++++++++++++++++++++ internal/api/scoping.go | 15 +++-- 3 files changed, 99 insertions(+), 5 deletions(-) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index c16aa7a0a..c0139bbc4 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -126,7 +126,8 @@ func mapCreatePlanStorageError(err error, notFoundMsg, genericMsg, logFmt string func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunctionURLRequest) (any, error) { // Require create:plans permission - if _, err := h.requirePermission(ctx, httpReq, "create", "plans"); err != nil { + session, err := h.requirePermission(ctx, httpReq, "create", "plans") + if err != nil { return nil, err } @@ -145,6 +146,12 @@ func (h *Handler) createPlan(ctx context.Context, httpReq *events.LambdaFunction return nil, err } + // Refuse before the plan row exists: a scoped caller must not create a + // plan that buys for an account outside their allowed_accounts. + if err := h.requireAccountsAccess(ctx, session, req.TargetAccounts); err != nil { + return nil, err + } + plan := req.toPurchasePlan() // Validate the plan diff --git a/internal/api/plan_accounts_scope_test.go b/internal/api/plan_accounts_scope_test.go index bfa6f0938..2d90ff611 100644 --- a/internal/api/plan_accounts_scope_test.go +++ b/internal/api/plan_accounts_scope_test.go @@ -2,10 +2,12 @@ package api import ( "context" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -277,3 +279,81 @@ func TestRouterDispatch_ListPlanAccounts_ScopedCallerFiltered(t *testing.T) { require.Len(t, got, 1) assert.Equal(t, scopedInAccount, got[0].ID) } + +// ── createPlan: target_accounts ───────────────────────────────────────────── + +// seedCreatePlanStore wires the store so POST /api/plans would SUCCEED if the +// scope guard were removed, and reports whether the plan row was written. +func seedCreatePlanStore(store *MockConfigStore) (*planAccountsWrite, *bool) { + write := seedPlanAccountsStore(store, nil) + created := false + store.On("CreatePurchasePlan", mock.Anything, mock.AnythingOfType("*config.PurchasePlan")). + Run(func(mock.Arguments) { created = true }).Return(nil).Maybe() + return write, &created +} + +func createPlanBody(accounts ...string) string { + return `{"name":"P","enabled":true,"auto_purchase":true,"provider":"aws","service":"ec2","target_accounts":["` + + strings.Join(accounts, `","`) + `"]}` +} + +// A scoped caller must not create a plan (auto-purchase or not) against an +// account outside their allowed_accounts. The refusal lands before the plan +// row is written, so there is no orphan to roll back. +func TestCreatePlan_ScopedCallerCannotTargetOutOfScopeAccount(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write, created := seedCreatePlanStore(store) + + _, err := h.createPlan(ctx, scopedRequest(createPlanBody(scopedOutAccount))) + + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected the enumeration-safe not-found refusal, got %v", err) + assert.False(t, *created, "CreatePurchasePlan must not run for an out-of-scope target account") + assert.False(t, write.called, "SetPlanAccounts must not run for an out-of-scope target account") +} + +func TestCreatePlan_ScopedCallerMixedTargetsRefusedWhole(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write, created := seedCreatePlanStore(store) + + _, err := h.createPlan(ctx, scopedRequest(createPlanBody(scopedInAccount, scopeThirdAccount))) + + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected not-found, got %v", err) + assert.False(t, *created) + assert.False(t, write.called) +} + +// Control: the same scoped caller may still create a plan for an account it holds. +func TestCreatePlan_ScopedCallerCanTargetInScopeAccount(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write, created := seedCreatePlanStore(store) + + _, err := h.createPlan(ctx, scopedRequest(createPlanBody(scopedInAccount))) + + require.NoError(t, err) + assert.True(t, *created) + require.True(t, write.called) + assert.Equal(t, []string{scopedInAccount}, write.ids) +} + +// Control: an unrestricted caller may target any account. +func TestCreatePlan_UnrestrictedCallerUnaffected(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t) + t.Cleanup(func() { store.AssertExpectations(t) }) + write, created := seedCreatePlanStore(store) + + _, err := h.createPlan(ctx, scopedRequest(createPlanBody(scopedOutAccount))) + + require.NoError(t, err) + assert.True(t, *created) + require.True(t, write.called) + assert.Equal(t, []string{scopedOutAccount}, write.ids) +} diff --git a/internal/api/scoping.go b/internal/api/scoping.go index 8d16cf25a..97a406881 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -86,7 +86,7 @@ func (h *Handler) requirePlanAccess(ctx context.Context, session *Session, planI // decides which accounts that plan buys commitments for, so either half // redirects purchasing. // -// The scope is resolved once and unrestricted callers short-circuit BEFORE +// Unrestricted callers short-circuit in both helpers BEFORE // any store lookup, mirroring requireExecutionAccess: an admin/API-key // session must not pay for (or need fixtures for) reads it cannot be refused // by. Empty and "*" allow-lists are unrestricted at exactly this seam @@ -99,6 +99,16 @@ func (h *Handler) requirePlanAccess(ctx context.Context, session *Session, planI // // requirePermission must fire first; pass it the session that returned. func (h *Handler) requirePlanAccountsAccess(ctx context.Context, session *Session, planID string, accountIDs []string) error { + if err := h.requirePlanAccess(ctx, session, planID); err != nil { + return err + } + return h.requireAccountsAccess(ctx, session, accountIDs) +} + +// requireAccountsAccess is requireAccountAccess over a batch: every account +// must be in the session's scope or the whole batch is refused with +// errNotFound. Unrestricted callers return before any store lookup. +func (h *Handler) requireAccountsAccess(ctx context.Context, session *Session, accountIDs []string) error { allowed, err := h.getAccountScope(ctx, session) if err != nil { return fmt.Errorf("failed to get allowed accounts: %w", err) @@ -106,9 +116,6 @@ func (h *Handler) requirePlanAccountsAccess(ctx context.Context, session *Sessio if allowed.AllowsAll() { return nil } - if err := h.requirePlanAccess(ctx, session, planID); err != nil { - return err - } for _, aid := range accountIDs { if _, err := h.requireAccountAccess(ctx, session, aid); err != nil { return err