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

[cristim]

Reviewed commit: be11bdcb5 (origin/main), from the 2026-07-28 full-repo review.

Severity: CRITICAL. Class: the read side of a feature is scoped by allowed_accounts, the write side of the same feature is not.

Where

• internal/api/handler_ladder.go:77-122 (write path, upsertLadderConfig)
• internal/api/handler_ladder.go:27-66 (read path, which does call filterLadderConfigsByAllowedAccounts)
• internal/api/handler_ladder.go:129 (validateLadderAccountProvider)

What

PUT /api/ladder/configs gates only on requirePermission(ctx, req, "update", "config"). cfg.CloudAccountID comes straight from the request body; validateLadderAccountProvider only checks that the account exists and that its provider matches. Neither getAllowedAccounts / MatchesAccount nor requirePermissionConstraints is called before h.config.UpsertLadderConfig at :116.

The GET handler's own doc comment (:20-26) records that migration 000088 granted view:config to the Standard User and Read-Only User groups precisely so that scoped users reach this endpoint. That reasoning was applied to the read and not to the write.

Failure scenario

Standard User U has allowed_accounts = [acct-A] and holds update:config. U sends:

PUT /api/ladder/configs
{"cloud_account_id":"<acct-B-uuid>","provider":"aws","enabled":true,
 "mode":"auto_approve","cadence":"daily","max_hourly_commit_per_run":null, ...}

which returns 200. LadderConfigDB.MaxHourlyCommitPerRun is *float64 where nil means no cap (internal/config/types.go:1075-1077), and Mode accepts auto_approve, so the ladder engine now auto-purchases uncapped commitments on account B, which U has no entitlement to and cannot even see. The GET filters row B back out, so the change is invisible to U afterwards and to any other scoped operator.

Fix direction

Call h.requireAccountAccess(ctx, session, cfg.CloudAccountID) before the upsert, and add a requirePermissionConstraints("update", "config", {AccountIDs, Providers}) set, mirroring executeExchange's pattern.

Related

Same family as the other write-side scoping gaps filed from this review. Not covered by LeanerCloud/cloud-commitments-platform#70 / LeanerCloud/cloud-commitments-go#27 / LeanerCloud/cloud-commitments-go#36, which are ladder feature/algorithm issues.

[cristim]

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.

[cristim]

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.