From ee0dbcc5018dd49d27de1ba326f9f52e35c32b60 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 13 Aug 2026 13:31:21 +0200 Subject: [PATCH] sec(api): bound plan and account scope on the plan-accounts endpoints PUT /api/plans/:id/accounts gated only on update:plans, and GET only on view:plans. Both verbs are default Standard User grants, so a stock user scoped to one account could re-point any plan at any account and read back the account roster of any plan by id (issue #1769). requirePlanAccountsAccess bounds both axes before the write: the plan being re-pointed (requirePlanAccess) and every account being attached (requireAccountAccess). It resolves the scope once and short-circuits for unrestricted principals before any store lookup, mirroring requireExecutionAccess, so admin paths are unchanged. listPlanAccounts gains requirePlanAccess plus a per-account filter, since a plan reachable through one of the caller's accounts may also carry accounts they cannot see. Empty and "*" allow-lists mean unrestricted at exactly one seam (getAccountScope -> AccountScope.AllowsAll), so the empty-means-all rule is not re-derived here. Refusals are the enumeration-safe errNotFound. The account_ids payload validation moves into validatePlanAccountIDs with its messages unchanged, which keeps setPlanAccounts inside the gocyclo budget. Coverage runs both directions (refusal plus an in-scope control that must still succeed) at the handler and through Router.Route. --- internal/api/handler_accounts.go | 81 +++++-- internal/api/plan_accounts_scope_test.go | 279 +++++++++++++++++++++++ internal/api/scoping.go | 42 ++++ 3 files changed, 387 insertions(+), 15 deletions(-) create mode 100644 internal/api/plan_accounts_scope_test.go diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 13eb07ac9..66d1bc1c8 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -1300,12 +1300,19 @@ func (h *Handler) getPlanForAccountProviderValidation(ctx context.Context, planI // providers (extracted from plan.Services keys). Mismatches return 400 // listing every offender; the assignment is rejected atomically (no // partial writes). +// +// update:plans is a default Standard User grant, so the verb gate alone does +// not bound WHICH plan or WHICH accounts the caller may write (issue #1769). +// requirePlanAccountsAccess supplies that bound on both axes and runs before +// the provider validation below, so a scoped caller cannot use the +// mismatch/not-found messages to learn about accounts outside their scope. func (h *Handler) setPlanAccounts(ctx context.Context, httpReq *events.LambdaFunctionURLRequest, id string) (any, error) { if err := validateUUID(id); err != nil { return nil, err } - if _, err := h.requirePermission(ctx, httpReq, "update", "plans"); err != nil { + session, err := h.requirePermission(ctx, httpReq, "update", "plans") + if err != nil { return nil, err } @@ -1316,18 +1323,12 @@ func (h *Handler) setPlanAccounts(ctx context.Context, httpReq *events.LambdaFun return nil, NewClientError(400, "invalid request body") } - // Reject empty account_ids: a plan must remain tied to at least one - // cloud_account row. Allowing the PUT to clear all rows would recreate - // the universal-plan bug class (purchase_plans row with no matching - // plan_accounts row) that createPlan now refuses at insert time. - if len(body.AccountIDs) == 0 { - return nil, NewClientError(400, "account_ids is required: a plan must be tied to at least one account") + if err := validatePlanAccountIDs(body.AccountIDs); err != nil { + return nil, err } - for _, aid := range body.AccountIDs { - if err := validateUUID(aid); err != nil { - return nil, NewClientError(400, fmt.Sprintf("invalid account_id %q: must be a valid UUID", aid)) - } + if err := h.requirePlanAccountsAccess(ctx, session, id, body.AccountIDs); err != nil { + return nil, err } // Provider-match validation (issue #209). Extracted to keep @@ -1346,13 +1347,52 @@ func (h *Handler) setPlanAccounts(ctx context.Context, httpReq *events.LambdaFun return nil, nil } +// validatePlanAccountIDs rejects a missing/empty account_ids payload and +// entries that are not valid UUIDs. +// +// Empty is refused because a plan must remain tied to at least one +// cloud_account row: letting the PUT clear all rows would recreate the +// universal-plan bug class (a purchase_plans row with no matching +// plan_accounts row) that createPlan refuses at insert time. It is also what +// keeps the account-axis scope check in requirePlanAccountsAccess meaningful, +// since an empty list has nothing to check. +// +// Same contract as validateTargetAccounts (handler_plans.go) with this +// endpoint's field names in the messages, so a payload that gets past +// createPlan also gets past here. +func validatePlanAccountIDs(ids []string) error { + if len(ids) == 0 { + return NewClientError(400, "account_ids is required: a plan must be tied to at least one account") + } + for _, aid := range ids { + if err := validateUUID(aid); err != nil { + return NewClientError(400, fmt.Sprintf("invalid account_id %q: must be a valid UUID", aid)) + } + } + return nil +} + // listPlanAccounts handles GET /api/plans/:id/accounts. +// +// The read sibling of setPlanAccounts, and it carries the mirror image of the +// same gap (issue #1769): view:plans is a default Standard User grant, so +// without requirePlanAccess a scoped caller could enumerate the accounts of +// any plan by id. requirePlanAccess bounds WHICH plans are visible; the +// per-account filter then bounds WHAT of a visible plan is disclosed, since a +// plan the caller legitimately reaches through one of their accounts may also +// carry accounts they have no entitlement to see. func (h *Handler) listPlanAccounts(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) { if err := validateUUID(id); err != nil { return nil, err } - if _, err := h.requirePermission(ctx, req, "view", "plans"); err != nil { + session, err := h.requirePermission(ctx, req, "view", "plans") + if err != nil { + return nil, err + } + + err = h.requirePlanAccess(ctx, session, id) + if err != nil { return nil, err } @@ -1361,11 +1401,22 @@ func (h *Handler) listPlanAccounts(ctx context.Context, req *events.LambdaFuncti return nil, fmt.Errorf("accounts: %w", err) } - if accts == nil { - accts = []config.CloudAccount{} + allowed, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) } - return accts, nil + // Allows() is true for every account of an unrestricted scope, so this is + // one loop rather than a branch on AllowsAll. make() also gives the + // endpoint its no-nil-slice contract for free. + scoped := make([]config.CloudAccount, 0, len(accts)) + for i := range accts { + if allowed.Allows(accts[i].ID, accts[i].Name) { + scoped = append(scoped, accts[i]) + } + } + + return scoped, nil } // DiscoverOrgRequest is the request body for POST /api/accounts/discover-org. diff --git a/internal/api/plan_accounts_scope_test.go b/internal/api/plan_accounts_scope_test.go new file mode 100644 index 000000000..a10aa3e23 --- /dev/null +++ b/internal/api/plan_accounts_scope_test.go @@ -0,0 +1,279 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Account-scope coverage for the plan↔account association endpoints +// (issue #1769). +// +// PUT /api/plans/:id/accounts gated only on update:plans, and GET on +// view:plans. Both verbs are DEFAULT Standard User grants, so the verb gate +// alone let any stock user re-point any plan at any account and read back the +// account roster of any plan by id. The two axes are independent: the plan +// being written and the accounts being attached. +// +// Every refusal below is paired with a control that must still be ALLOWED. +// A refusal-only suite passes just as well against a handler that refuses +// everyone, which is the failure mode these endpoints are one bad edit away +// from. + +const ( + // The plan under test. Its CURRENT association set is what the plan-axis + // guard reads, and is seeded per test. + scopePlanID = "33333333-3333-4333-8333-333333333333" + // A second account that is in neither the allow-list nor any fixture, used + // to prove the write is refused before it reaches the store. + scopeThirdAccount = "44444444-4444-4444-8444-444444444444" +) + +// planAccountsWrite records what setPlanAccounts handed to the store. +type planAccountsWrite struct { + called bool + ids []string +} + +// seedPlanAccountsStore wires the store so the request would SUCCEED +// end-to-end if the scope guard were removed: the plan exists and derives the +// "aws" provider, every account id resolves to an existing aws account (so the +// issue-#209 provider validation passes), and SetPlanAccounts accepts the +// write. +// +// The permissive SetPlanAccounts stub is the load-bearing part. Without it a +// removed guard would fail these tests by an unstubbed-call panic, which is +// not evidence about the guard: mutation runs have to fail by ASSERTION. +// +// planAccounts is the plan's CURRENT association set, which is what +// requirePlanAccess reads to decide whether the caller may touch the plan at +// all. +func seedPlanAccountsStore(store *MockConfigStore, planAccounts []config.CloudAccount) *planAccountsWrite { + write := &planAccountsWrite{} + store.GetPurchasePlanFn = func(_ context.Context, id string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ + ID: id, + Name: "scoped plan", + Services: map[string]config.ServiceConfig{"aws/ec2": {}}, + }, nil + } + store.GetPlanAccountsFn = func(_ context.Context, _ string) ([]config.CloudAccount, error) { + return planAccounts, nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: "acct-" + id, Provider: "aws"}, nil + } + store.SetPlanAccountsFn = func(_ context.Context, _ string, ids []string) error { + write.called = true + write.ids = ids + return nil + } + return write +} + +// inScopeAccount / outOfScopeAccount are the two association fixtures: the +// account the scoped principal holds, and one it does not. +func inScopeAccount() config.CloudAccount { + return config.CloudAccount{ID: scopedInAccount, Name: "acct-" + scopedInAccount, Provider: "aws"} +} + +func outOfScopeAccount() config.CloudAccount { + return config.CloudAccount{ID: scopedOutAccount, Name: "acct-" + scopedOutAccount, Provider: "aws"} +} + +// ── setPlanAccounts: account axis ─────────────────────────────────────────── + +// A scoped caller may hold the plan and still not hold the account it is +// trying to attach. The write must be refused, and refused with the +// enumeration-safe not-found rather than a 403 that would confirm the account +// exists. +func TestSetPlanAccounts_ScopedCallerCannotAttachOutOfScopeAccount(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + // The plan itself IS in scope, so only the account axis can refuse this. + write := seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount()}) + + body := `{"account_ids":["` + scopedOutAccount + `"]}` + _, err := h.setPlanAccounts(ctx, scopedRequest(body), scopePlanID) + + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected the enumeration-safe not-found refusal, got %v", err) + assert.False(t, write.called, "SetPlanAccounts must not run for an out-of-scope account") +} + +// The control for the test above: same handler, same code path, an account the +// caller DOES hold. Without this, a handler that refused every write would +// pass the refusal test. +func TestSetPlanAccounts_ScopedCallerCanAttachInScopeAccount(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write := seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount()}) + + body := `{"account_ids":["` + scopedInAccount + `"]}` + _, err := h.setPlanAccounts(ctx, scopedRequest(body), scopePlanID) + + require.NoError(t, err) + require.True(t, write.called, "an in-scope write must still reach the store") + assert.Equal(t, []string{scopedInAccount}, write.ids) +} + +// A batch is refused whole. One out-of-scope entry alongside an in-scope one +// must not write the in-scope half either, since SetPlanAccounts replaces the +// association set rather than adding to it. +func TestSetPlanAccounts_ScopedCallerMixedBatchRefusedWhole(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write := seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount()}) + + body := `{"account_ids":["` + scopedInAccount + `","` + scopeThirdAccount + `"]}` + _, err := h.setPlanAccounts(ctx, scopedRequest(body), scopePlanID) + + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected not-found, got %v", err) + assert.False(t, write.called, "a batch containing an out-of-scope account must not write at all") +} + +// ── setPlanAccounts: plan axis ────────────────────────────────────────────── + +// The other axis, isolated: every account in the BODY is one the caller holds, +// but the plan's current association set is entirely outside their scope. The +// account-axis check passes here, so only the plan-axis check can refuse it. +func TestSetPlanAccounts_ScopedCallerCannotRepointOutOfScopePlan(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write := seedPlanAccountsStore(store, []config.CloudAccount{outOfScopeAccount()}) + + body := `{"account_ids":["` + scopedInAccount + `"]}` + _, err := h.setPlanAccounts(ctx, scopedRequest(body), scopePlanID) + + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected not-found, got %v", err) + assert.False(t, write.called, "a plan outside the caller's scope must not be re-pointed") +} + +// ── setPlanAccounts: unrestricted caller unchanged ────────────────────────── + +// An unrestricted principal keeps the pre-fix behavior, and pays for no extra +// store round-trip: the guard resolves the scope once and returns before +// touching the store, which is what keeps admin-path fixtures unchanged. +func TestSetPlanAccounts_UnrestrictedCallerUnaffected(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t) // no accounts -> grantAdmin -> unrestricted + t.Cleanup(func() { store.AssertExpectations(t) }) + write := seedPlanAccountsStore(store, []config.CloudAccount{outOfScopeAccount()}) + + body := `{"account_ids":["` + scopedOutAccount + `"]}` + _, err := h.setPlanAccounts(ctx, scopedRequest(body), scopePlanID) + + require.NoError(t, err, "an unrestricted caller must still write any account to any plan") + require.True(t, write.called) + assert.Equal(t, []string{scopedOutAccount}, write.ids) + store.AssertNotCalled(t, "GetPlanAccounts") +} + +// ── listPlanAccounts ──────────────────────────────────────────────────────── + +// The read sibling. A plan the caller can reach through one of their accounts +// may carry accounts they hold no entitlement to; those must not come back in +// the response. +func TestListPlanAccounts_ScopedCallerSeesOnlyItsOwnAccounts(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}) + + result, err := h.listPlanAccounts(ctx, scopedRequest(""), scopePlanID) + + require.NoError(t, err) + got, ok := result.([]config.CloudAccount) + require.True(t, ok, "expected []config.CloudAccount, got %T", result) + require.Len(t, got, 1, "only the in-scope account may be disclosed") + assert.Equal(t, scopedInAccount, got[0].ID) +} + +// A plan with no in-scope account at all is not readable, and hides behind the +// same not-found the write path returns. +func TestListPlanAccounts_ScopedCallerCannotReadOutOfScopePlan(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + seedPlanAccountsStore(store, []config.CloudAccount{outOfScopeAccount()}) + + result, err := h.listPlanAccounts(ctx, scopedRequest(""), scopePlanID) + + require.Error(t, err) + assert.Nil(t, result) + assert.True(t, IsNotFoundError(err), "expected not-found, got %v", err) +} + +// The read-side control: an unrestricted caller still gets the full roster, +// so the filter above is a scope decision rather than a blanket narrowing. +func TestListPlanAccounts_UnrestrictedCallerSeesAllAccounts(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t) // unrestricted + t.Cleanup(func() { store.AssertExpectations(t) }) + seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}) + + result, err := h.listPlanAccounts(ctx, scopedRequest(""), scopePlanID) + + require.NoError(t, err) + got, ok := result.([]config.CloudAccount) + require.True(t, ok, "expected []config.CloudAccount, got %T", result) + assert.Len(t, got, 2, "an unrestricted caller must still see every account of the plan") +} + +// ── Real dispatch path ────────────────────────────────────────────────────── +// +// Handler-level tests have missed router-level bypasses on this repo before +// (#1757, #1773), so the two directions are re-run through Router.Route with +// the real route table and auth gate rather than by calling the handler. + +func TestRouterDispatch_SetPlanAccounts_ScopedOutOfScopeAccountRefused(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write := seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount()}) + + body := `{"account_ids":["` + scopedOutAccount + `"]}` + _, err := NewRouter(h).Route(ctx, "PUT", "/api/plans/"+scopePlanID+"/accounts", scopedRequest(body)) + + require.Error(t, err) + assert.True(t, IsNotFoundError(err), "expected not-found through the router, got %v", err) + assert.False(t, write.called, "the router path must not reach the store either") +} + +func TestRouterDispatch_SetPlanAccounts_ScopedInScopeAccountAllowed(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + write := seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount()}) + + body := `{"account_ids":["` + scopedInAccount + `"]}` + _, err := NewRouter(h).Route(ctx, "PUT", "/api/plans/"+scopePlanID+"/accounts", scopedRequest(body)) + + require.NoError(t, err, "an in-scope write must still succeed through the router") + require.True(t, write.called) + assert.Equal(t, []string{scopedInAccount}, write.ids) +} + +func TestRouterDispatch_ListPlanAccounts_ScopedCallerFiltered(t *testing.T) { + ctx := context.Background() + h, store := scopedHandler(t, scopedInAccount) + t.Cleanup(func() { store.AssertExpectations(t) }) + seedPlanAccountsStore(store, []config.CloudAccount{inScopeAccount(), outOfScopeAccount()}) + + result, err := NewRouter(h).Route(ctx, "GET", "/api/plans/"+scopePlanID+"/accounts", scopedRequest("")) + + require.NoError(t, err) + got, ok := result.([]config.CloudAccount) + require.True(t, ok, "expected []config.CloudAccount, got %T", result) + require.Len(t, got, 1) + assert.Equal(t, scopedInAccount, got[0].ID) +} diff --git a/internal/api/scoping.go b/internal/api/scoping.go index f8f4bdc13..e67269fce 100644 --- a/internal/api/scoping.go +++ b/internal/api/scoping.go @@ -74,6 +74,48 @@ func (h *Handler) requirePlanAccess(ctx context.Context, session *Session, planI return errNotFound } +// requirePlanAccountsAccess guards BOTH axes of a plan↔account association +// write (PUT /api/plans/:id/accounts, issue #1769): the plan being re-pointed +// and every account being attached to it. +// +// Checking only one axis leaves the other open. Without the plan check a +// scoped caller can re-point a plan whose accounts are all outside their +// scope; without the per-account check they can attach an account they have +// no entitlement to onto a plan they legitimately hold. A plan's account set +// decides which accounts that plan buys commitments for, so either half +// redirects purchasing. +// +// The scope is resolved once and unrestricted callers short-circuit 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 +// (getAccountScope → AccountScope.AllowsAll), so "empty means all accounts" +// is handled in one place rather than re-derived here. +// +// Refusals are the enumeration-safe errNotFound from requireAccountAccess / +// requirePlanAccess rather than a 403, so a scoped caller cannot use this +// endpoint to confirm that a plan or an account exists. +// +// requirePermission must fire first; pass it the session that returned. +func (h *Handler) requirePlanAccountsAccess(ctx context.Context, session *Session, planID string, accountIDs []string) error { + allowed, err := h.getAccountScope(ctx, session) + if err != nil { + return fmt.Errorf("failed to get allowed accounts: %w", err) + } + 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 + } + } + return nil +} + // validatePurchaseRecommendationScope returns a 400 client error when any // recommendation in the batch targets an account the session can't access. // Admin / unrestricted sessions pass through. Recommendations with a nil