sec(api): setPlanAccounts writes plan->account associations with no allowed_accounts or plan scope check

[cristim]

Found while investigating #1539 (upsertLadderConfig writes ladder config for any cloud account). Same class, different file, and reachable with default permissions rather than a custom group.

Verified against origin/main @ 9bfd6822c by execution, not by reading.

Where

• internal/api/handler_accounts.go:1304 — setPlanAccounts, PUT /api/plans/:id/accounts
• internal/api/handler_accounts.go:1351 — listPlanAccounts (read sibling, same gap)
• internal/api/handler_plans.go:208,248,306,332,530 — the guarded handlers, for contrast

What

setPlanAccounts gates only on requirePermission(ctx, httpReq, "update", "plans"). body.AccountIDs comes straight from the request body, and between the decode and h.config.SetPlanAccounts(ctx, id, body.AccountIDs) at :1340 the handler calls:

• validateUUID on the plan id and each account id — well-formedness only
• validatePlanAccountProviders — checks each account exists and its provider matches the plan's derived providers

Neither requirePlanAccess(ctx, session, id) nor requireAccountAccess(ctx, session, aid) is called. The session returned by requirePermission is discarded (if _, err := ...), which is the same tell as in #1539.

Two distinct gaps, on two axes:

1. Plan axis — the caller can target a plan whose accounts are entirely outside their allowed_accounts. getPlan / updatePlan / deletePlan all call requirePlanAccess; this writer does not. The guarded handlers live in handler_plans.go while this one lives in handler_accounts.go, which is plausibly why it was missed — the same file-boundary shape as sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737's AllowedAccounts miss and sec(auth): fail closed when a user's account scope cannot be established #1752's adapter.
2. Account axis — the caller can attach accounts outside their allowed_accounts to a plan.

listPlanAccounts (:1351) has the mirror-image read gap: it requires view:plans and returns GetPlanAccounts unfiltered, so a scoped user can enumerate the account names of any plan by id.

Why this outranks #1539 on reachability

update:plans and view:plans are default Standard User grants (DefaultUserPermissions, internal/auth/types.go). #1539 needs an operator-created custom group because migration 000088 deliberately withheld update:config from non-admin groups. Here no custom group is required — a stock Standard User scoped to one account can already do this.

Reproduction (executed on main)

Driving the real handler with a principal scoped to one account, targeting a different one:

PROBE setPlanAccounts out-of-scope account: err=<nil>
PROBE RESULT: BYPASS CONFIRMED -- out-of-scope account attached to a plan

Negative control against the seam the handler skips, same principal, same account:

PROBE control requireAccountAccess: err=not found
PROBE RESULT: seam refuses it; the handler simply never calls the seam

The control matters: without it the probe would pass equally against a handler that refuses everything.

Impact

A plan's account set determines which accounts a purchase plan buys commitments for. Re-pointing a plan at an account the caller has no entitlement to, or adding such an account to an existing plan, redirects that plan's purchasing. Provider-match validation still applies, so the injected account must share the plan's provider — it does not have to be one the caller can see.

Same-tenant, wrong-scope. Not cross-tenant.

Fix direction

In setPlanAccounts, capture the session and add both checks before the write:

• h.requirePlanAccess(ctx, session, id)
• h.requireAccountAccess(ctx, session, aid) for each aid in body.AccountIDs

requireAccountAccess (internal/api/scoping.go:26) already performs the GetCloudAccount + nil check that validatePlanAccountProviders repeats per account, so the two lookups should be folded rather than doubled.

For listPlanAccounts, add requirePlanAccess and filter the returned accounts through allowed_accounts.

Both refusals should be errNotFound rather than 403, matching requireAccountAccess's existing enumeration-safe contract.

Tests

Both directions, per the standing rule that a refusal-only test passes against a handler that refuses everyone:

• scoped caller + out-of-scope account → refused
• scoped caller + in-scope account → still allowed
• scoped caller + out-of-scope plan → refused
• unrestricted/admin caller → unchanged

Mutation-verified per test, run alone.

Related

• sec(api): upsertLadderConfig writes commitment-laddering config for ANY cloud account #1539 — same class, ladder config write path
• sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737, sec(auth): fail closed when a user's account scope cannot be established #1752 — prior instances of the guarded-primary / unguarded-sibling shape