sec(api): upsertLadderConfig writes commitment-laddering config for ANY cloud account #1539

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p1Next up; this sprintNext up; this sprint

   severity/criticalMajor harm when it happensMajor harm when it happens

   urgency/nowDrop other thingsDrop other things

   impact/manyAffects most usersAffects most users

   effort/sHoursHours

   type/securitySecurity findingSecurity finding

   on Jul 28, 2026
2. cristim commented on Aug 8, 2026

   

   cristim

   on Aug 8, 2026

   MemberAuthor

   More actions

   Investigation before implementing — premise CONFIRMED by execution, severity likely one notch high

   Checked against origin/main @ 9bfd6822c (issue was filed against be11bdcb5; handler_ladder.go is materially unchanged since — the line numbers in the issue still land).

   1. Is the defect still live? Yes — demonstrated, not inferred

   Drove the real routed handler (PUT /api/ladder/configs → upsertLadderConfigHandler → upsertLadderConfig, router.go:326, Auth: AuthUser) with a principal scoped to one account via the grantScoped seam, targeting a different account:

   PROBE upsert out-of-scope: err=<nil> result=&{... CloudAccountID:22222222-2222-4222-8222-222222222222 ID:written-row ...}
   PROBE RESULT: BYPASS CONFIRMED -- out-of-scope ladder config write reached the store
   

   The companion probe pins the asymmetry the issue describes — same principal, same two rows, read side:

   PROBE read-side returned 1 config(s):   id=row-in account=1111...
   PROBE RESULT: read filtered, write unfiltered
   

   upsertLadderConfig:78 discards the session (if _, err := h.requirePermission(...)), and nothing between the decode and h.config.UpsertLadderConfig at :116 consults allowed_accounts. validateLadderAccountProvider:129 checks existence and provider match only, exactly as described. The fix direction is sound: requireAccountAccess exists at scoping.go:26 and already returns the enumeration-safe errNotFound for both the missing and the out-of-scope case.

   2. Blast radius — narrower than the issue states, in two independent ways

   (a) No seeded group grants update:config to a scoped user. Migration 000088 says so explicitly in its own header: "Non-admin groups are NOT granted update:config here." Grepping the defaults confirms it — ResourceConfig appears in DefaultUserPermissions and DefaultReadOnlyPermissions only as {ActionView, ResourceConfig} (types.go:611,623). So the failure scenario's "Standard User U … holds update:config" is not reachable out of the box; it needs an operator-created custom group that grants update:config and carries a restricted allowed_accounts. That is a legitimate and supported configuration, but it is not the default.

   A plain admin cannot exploit this: admin:* comes with allowed_accounts = ARRAY['*'], so IsUnrestrictedAccess is true and the scope check would pass anyway. There is no privilege escalation for the principals that hold update:config today.

   (b) The auto-purchase consequence additionally requires the target to be the deployment's own AWS account. The issue states the engine "now auto-purchases uncapped commitments on account B". That is only true when B is the account the deployment runs in. processOneLadderConfig (internal/server/handler_ladder.go:234) applies a single-account gate:

   if cloudAcct.ExternalID != ownAccountID {
       // ... skipped (multi_account_unsupported)
       return outcomeSkippedMultiAccount
   }

   where ownAccountID is resolved from the deployment's own STS identity. Execution also requires laddering_enabled and ladder_execution_enabled to be true globally (handler_ladder.go:89,126).

   So the two reachable outcomes are:

   target account	consequence
   the deployment's own AWS account	money path is real — mode:auto_approve and a nil max_hourly_commit_per_run both reach the engine via ladderConfigToEngine:516,525. Because the store upsert is UNIQUE(cloud_account_id, provider), a scoped user can also overwrite an admin's existing ladder config for that account: raise/remove the cap, flip the mode, or set enabled:false to silently stop a running ladder.
   any other registered account	write succeeds, row is invisible to the writer afterwards (GET filters it), never executes

   Classification: same-tenant, wrong-scope — not cross-tenant and not cross-deployment.

   Suggested re-triage: severity/critical → severity/high, keeping priority/p1 / urgency/now. Reasoning: the bypass is real and the fix is trivial, but reaching the money consequence needs a non-default custom group and two global flags and the target to be the deployment's own account. Against that, the tamper case is cheap once laddering is in use at all (both flags are on by definition then), so this is a defensible high, not a medium.

   I have deliberately not changed the label — it affects queue position, which the coordinator owns. Happy to apply it on a word.

   3. Same shape elsewhere — one real sibling found, in a different file

   Swept all 321 *Handler methods in internal/api, attributing 74/74 requirePermission call sites (my first pass matched only the two-arg (ctx, req) signature and silently covered 13 of 74 — the corrected sweep is the one reported here). Of 33 mutating handlers: 13 apply a scope helper, 3 are account-bearing without one.

   Of those 3:

   • handler_purchases.go:2127 validateExecutePurchaseRequest — false positive, it calls validatePurchaseRecommendationScope at :2152.
   • handler_ladder.go:77 upsertLadderConfig — this issue.
   • handler_accounts.go:1304 setPlanAccounts — a real second instance, and arguably worse.

   setPlanAccounts takes account_ids straight from the body and calls h.config.SetPlanAccounts at :1340 with neither requirePlanAccess nor requireAccountAccess. Meanwhile getPlan / updatePlan / deletePlan all call requirePlanAccess (handler_plans.go:208,248,306,332,530). The guarded handlers live in handler_plans.go; the unguarded sibling lives in handler_accounts.go, which is plausibly why it was missed — the same file-boundary shape as #1737's AllowedAccounts miss and #1752's adapter.

   It matters more than the ladder case because update:plans IS a default Standard User grant (DefaultUserPermissions), so no custom group is required. Demonstrated:

   PROBE setPlanAccounts out-of-scope account: err=<nil>
   PROBE RESULT: BYPASS CONFIRMED -- out-of-scope account attached to a plan
   PROBE control requireAccountAccess: err=not found
   PROBE RESULT: seam refuses it; the handler simply never calls the seam
   

   The control matters: it proves the seam this handler skips does refuse that exact account, so the probe is not passing against a handler that refuses everything.

   Filed separately rather than folded in here, to keep this issue's scope to the ladder path.

   4. Size and files

   Small, and splittable so it does not collide with the #1757 CSRF work.

   	files	shape
   this issue	internal/api/handler_ladder.go, internal/api/handler_ladder_test.go	capture the session at :78; call requireAccountAccess before the upsert. requireAccountAccess already does the GetCloudAccount + nil check that validateLadderAccountProvider repeats, so the two should be folded into one lookup rather than fetching twice.
   sibling	internal/api/handler_accounts.go, tests	requirePlanAccess on the plan id + requireAccountAccess per account id

   One behaviour change to flag on this issue: requireAccountAccess returns errNotFound (404) where the current code returns 400 "cloud account %q does not exist". That is the right direction — a scoped caller must not be able to distinguish "does not exist" from "exists but is not yours" — but it will move existing assertions, so it is a deliberate change rather than a silent one.

   Tests will assert both directions (out-of-scope refused, in-scope and unrestricted still allowed) and be mutation-verified individually, since a refusal-only test passes just as well against a handler that refuses everyone.
3. cristim commented on Aug 8, 2026

   

   cristim

   on Aug 8, 2026

   MemberAuthor

   More actions

   Sibling from §3 filed as #1769 (setPlanAccounts, handler_accounts.go:1304) — labelled p1/severity/high/urgency/now. Kept out of this issue's scope; it needs handler_accounts.go, whereas this one is confined to handler_ladder.go, so the two can land independently.
4. cristim

   added

   severity/highSignificant harmSignificant harm

   and removed

   severity/criticalMajor harm when it happensMajor harm when it happens

   on Aug 8, 2026
5. cristim

   added a commit that references this issue on Aug 8, 2026

   sec(api): scope ladder config writes to the caller's allowed_accounts

   ...

   This issue will close once commit ec92067 is merged into the 'main' branch.

   ec92067
6. cristim

   closed this as completedin #1770on Aug 8, 2026