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

[cristim]

Closes #1539

Established before fixing

The issue reads as a plausible-but-unverified claim, so I checked it by execution rather than by reading. It is real, and two of its stated premises are narrower than written — both recorded on the issue.

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

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

Companion probe, 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

What was actually wrong

upsertLadderConfig discarded the session (if _, err := h.requirePermission(...)) and nothing between the decode and h.config.UpsertLadderConfig consulted allowed_accounts. validateLadderAccountProvider checked existence and provider match only.

getLadderConfigs has filtered on allowed_accounts since migration 000088 opened view:config to scoped users — its doc comment records exactly that reasoning. It was applied to the read and not to the write, so the row was also filtered back out of the writer's own GET afterwards.

Severity, stated honestly

Filed as severity/critical; re-triaged to severity/high during investigation, p1/urgency/now unchanged. Two independent narrowings:

No seeded group grants update:config to a scoped user. Migration 000088 says so in its own header — "Non-admin groups are NOT granted update:config here." ResourceConfig appears in the defaults only as {ActionView, ResourceConfig}. So this needs an operator-created custom group granting update:config and carrying a restricted allowed_accounts. A plain admin is unaffected either way: admin:* carries allowed_accounts ARRAY['*'], so the new check passes for it. There is no privilege escalation for the principals that hold the verb today.

The auto-purchase consequence needs the target to be the deployment's own AWS account. The issue states the engine "auto-purchases uncapped commitments on account B". That holds only when B is the account the deployment runs in — processOneLadderConfig (internal/server/handler_ladder.go:234) skips any config whose ExternalID != ownAccountID as multi_account_unsupported, and execution also needs laddering_enabled and ladder_execution_enabled globally.

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. And because the store upserts on UNIQUE(cloud_account_id, provider), an out-of-scope caller can overwrite an admin's existing row: raise the cap, flip the mode, or set enabled:false to silently stop a running ladder.
any other registered account	write succeeds, row invisible to its author afterwards, never executes

Same-tenant wrong-scope, not cross-tenant. high, not critical — but not lower either, because once laddering is in use at all both global flags are on by definition, so the tamper case is cheap.

What landed

validateLadderAccountProvider becomes requireLadderAccountAccess and delegates the lookup to requireAccountAccess, which already fetches and nil-checks the account — so the account is fetched once rather than twice. The rename is deliberate: the function now enforces authorization, not just validation, and a reviewer seeing validate… would not expect an authz check inside it.

Ordering inside the helper matters and is not incidental: requireAccountAccess runs before the provider comparison, so the 400 "provider %q does not match cloud account provider %q" — which names the account's provider — is unreachable for an out-of-scope caller.

Behaviour change. A cloud_account_id that does not resolve now returns errNotFound (404) instead of 400 "cloud account %q does not exist". requireAccountAccess answers "no such account" and "not yours" identically on purpose: a distinguishable 400 would let a scoped caller enumerate the account table by probing ids. One existing test asserted the 400 and was updated with that reasoning recorded on it.

Deliberately NOT added: the requirePermissionConstraints call the issue's fix direction suggests. requirePermissionConstraintsAction is hardcoded to "execute" (handler.go:472), with a comment saying a genuinely new action should add a real parameter back rather than resurrect an unused one. Calling it here would evaluate the caller's execute permission on config — not the verb being exercised — and would fail closed for anyone holding only update:config. The allowed_accounts check is what closes the reported bypass; adding a second, mis-aimed authorization layer would be its own bug surface.

Verification

Coverage runs both directions, since a refusal-only test passes equally well against a handler that refuses everyone. Mutation-verified per test, each run alone, on the committed diff:

mutation	expected	observed
M1 scope check → plain GetCloudAccount (the fix removed)	OutOfScopeAccountRefused fails	FAIL — and In-scope / Unrestricted / Nonexistent all still pass, so M1 is precisely targeted
M2 refuse everyone	In-scope + Unrestricted fail	both FAIL — the two positive controls are not vacuous
M3 provider check dropped	ProviderMismatchRejected fails	FAIL — with scope tests still passing

TestUpsertLadderConfig_UnrestrictedSessionUnaffected writes the same account the scoped principal was refused, which pins that the refusal is driven by the caller's scope rather than by anything about the account.

Gates at this head, as actually observed at the time of opening: go build ./... 0 · go vet ./... 0 · go vet -tags=integration ./... 0 · internal/api green (29.9s). The full ./internal/... ./cmd/... run, golangci-lint v2.10.1 (the CI-pinned version) and gocyclo -over 10 -ignore "_test\.go" were still running locally when this was opened — I will post the exit codes as a follow-up comment rather than assert them here. CI covers the same ground independently.

Known-red main: TestGrantCeiling_ConstraintContainment in internal/auth fails 3 subtests on main itself — #1737's test uses execute:ri-exchange as a generic fixture verb and #1758 then added that verb to adminCarvedOuts, so checkGrantCeiling refuses it before the containment logic under test is reached. This PR's diff touches 0 files under internal/auth, so that failure cannot originate here; it clears when the test-only fix lands.

Sibling found and filed separately

The completeness sweep over all 321 *Handler methods (attributing 74/74 requirePermission call sites — a first pass matched only the two-arg signature and silently covered 13 of 74) found one more real instance of this shape: #1769, setPlanAccounts at handler_accounts.go:1304, which writes plan→account associations with neither requirePlanAccess nor requireAccountAccess. It is higher-reachability than this one — update:plans is a default Standard User grant — and is filed separately because it needs a different file and can land independently.

validateExecutePurchaseRequest also surfaced as a suspect and is a false positive: it calls validatePurchaseRecommendationScope.

Summary by CodeRabbit

• Bug Fixes

  • Restricted ladder configuration updates to authorized accounts and matching providers.
  • Prevented unauthorized access from revealing whether an account exists.
  • Preserved configuration updates for permitted accounts and unrestricted sessions.

• Tests

  • Added coverage for out-of-scope account rejection, allowed account updates, and unrestricted access.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a65c5ddc-d4c7-411d-8a09-bf82c3f24c18

📥 Commits

Reviewing files that changed from the base of the PR and between 6df07bb and 6064849.

📒 Files selected for processing (2)

• internal/api/handler_ladder.go
• internal/api/handler_ladder_test.go

---

📝 Walkthrough

Walkthrough

The ladder configuration upsert path now checks the caller’s account scope and provider before writing. Inaccessible and nonexistent accounts return the shared not-found error. Tests cover scoped, in-scope, and unrestricted sessions.

Changes

Ladder configuration authorization

Layer / File(s)	Summary
Enforce scoped ladder access internal/api/handler_ladder.go	upsertLadderConfig retains the authorized session and calls requireLadderAccountAccess before the upsert. Account scope, account existence, and provider matching are validated with indistinguishable not-found errors.
Validate scoped write behavior internal/api/handler_ladder_test.go	Tests verify that nonexistent and out-of-scope accounts do not write, in-scope accounts write successfully, and unrestricted sessions remain unaffected.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• sec(api): setPlanAccounts writes plan->account associations with no allowed_accounts or plan scope check #1769 — Uses the same account-scope authorization pattern for another write path.

Possibly related PRs

• LeanerCloud/CUDly#1355 — Extends the same ladder configuration handler and tests with account-scoped authorization.
• LeanerCloud/CUDly#1489 — Modifies ladder configuration authorization for a related read operation.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant upsertLadderConfig
  participant requireLadderAccountAccess
  participant LadderConfigStore
  Client->>upsertLadderConfig: Submit ladder configuration
  upsertLadderConfig->>requireLadderAccountAccess: Check session account access
  requireLadderAccountAccess-->>upsertLadderConfig: Return access result
  alt Account is accessible
    upsertLadderConfig->>LadderConfigStore: Upsert configuration
  else Account is inaccessible or nonexistent
    upsertLadderConfig-->>Client: Return errNotFound
  end

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the primary change: scoping ladder configuration writes to the caller's allowed accounts.
Linked Issues check	✅ Passed	The PR satisfies #1539 by enforcing account-scope access before ladder configuration writes and covering scoped, unrestricted, and nonexistent accounts.
Out of Scope Changes check	✅ Passed	The changes are limited to ladder write authorization and related regression tests, with no unrelated code changes identified.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sec/1539-ladder-account-scope

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review at c57b40081 — 1 blocking, everything else verified clean

Fresh worktree at the PR head plus one rebased onto green main (6df07bb7f), so the inherited internal/auth failure could not mask anything. Every claim re-derived.

The security work here is genuinely well done — the guard is correct, it refuses at the routed endpoint, and the enumeration is complete. There is one mechanical problem that turns CI red.

---

F1 (blocking) — the PR introduces 4 new govet shadow findings and fails the Lint job

golangci-lint v2.10.1 on the root module (the module CI lints), comm set diff, base vs the same commit rebased onto 6df07bb7f:

side	result
base 6df07bb7f	exit 0, genuine 0 issues.
head	exit 1, 4 issues: govet: 4

All four are new, all in the changed file, nothing removed:

internal/api/handler_ladder.go:100:5: shadow: declaration of "err" shadows declaration at line 89 (govet)
internal/api/handler_ladder.go:115:5: shadow: declaration of "err" shadows declaration at line 89 (govet)
internal/api/handler_ladder.go:120:5: shadow: declaration of "err" shadows declaration at line 89 (govet)
internal/api/handler_ladder.go:124:5: shadow: declaration of "err" shadows declaration at line 89 (govet)

Cause is the one-line change that captures the session:

session, err := h.requirePermission(ctx, req, "update", "config")   // line 89 — err now function-scoped

err was previously confined to if _, err := .... Hoisting it means the four pre-existing if err := ... statements below (dec.Decode, json.Unmarshal, cfg.Validate, requireLadderAccountAccess) now shadow it.

Confirmed live, not just local: gh pr checks 1770 shows Lint Code fail (4m59s) and the aggregate CI Success fail. This is separate from the inherited internal/auth test failure — that one is in Unit Tests, still pending on this run.

Smallest fix that keeps the diff shape:

session, permErr := h.requirePermission(ctx, req, "update", "config")
if permErr != nil {
	return nil, permErr
}

---

Verified clean

The guard refuses at the routed endpoint, not just in the helper. Drove NewRouter(h), located the registered PUT /api/ladder/configs route rather than assuming it exists, and called its handler:

ROUTED out-of-scope account   auth=AuthUser  err=not found   store write reached = false
ROUTED in-scope account       auth=AuthUser  err=<nil>       store write reached = true

There is exactly one route reaching this write (router.go:326 → upsertLadderConfigHandler → h.upsertLadderConfig, a bare pass-through), so the tests calling the handler method are not bypassing a gate.

The refusal is produced by this guard and nothing else. Mutated away only the allowed_accounts filtering, keeping the existence lookup and the nil→errNotFound handling intact:

• TestUpsertLadderConfig_OutOfScopeAccountRefused becomes the sole failure in the package.
• The routed probe then shows store write reached = true for the out-of-scope account — the pre-fix vulnerability reproduced end to end at the route.
• The in-scope case is unaffected by the mutation, so that test is not accidentally coupled to the guard.

(Method note: my first mutation replaced requireAccountAccess with a bare GetCloudAccount, which dropped the nil check too and nil-dereferenced — the panic aborted the binary before the scoping tests ran. A mutation that removes more than the property under test proves nothing; worth stating because the corrected one is what the result above rests on.)

Both directions are genuinely covered. _OutOfScopeAccountRefused / _InScopeAccountStillAllowed, plus _UnrestrictedSessionUnaffected as a control that the refusal tracks the caller's scope rather than the account. The in-scope test fails against a refuse-everyone handler, which is the property a refusal-only suite lacks.

The wildcard predicate is right, because the PR did not write one. It reuses the shared requireAccountAccess, which delegates to auth.IsUnrestrictedAccess — that handles len(allowed) == 0 and a "*" entry, then auth.MatchesAccount. No bespoke comparison was introduced, which is exactly how the #1752 predicate error is avoided. The _UnrestrictedSessionUnaffected test pins it.

The reachability claim that moved severity checks out. Verified with validated negatives (each negative grep sanity-checked against a pattern known to match):

• No migration seeds update:config. The JSON-form negative returns nothing, while the same pattern shape matches {"action":"view","resource":"config"} at 000088:20,26 — so the negative is real, not a silent no-match.
• No default permission set contains {Action: ActionUpdate, Resource: ResourceConfig} in production code. The only three hits are test fixtures (service_apikeys_test.go:962,1180, service_group_test.go:416); the ActionView form matches at types.go:611,623 as sanity.
• Migration 000088 says so in its own comment: "Non-admin groups are NOT granted update:config here."

So it needs an operator-created custom group granting update:config and carrying a restricted allowed_accounts. Supported and legitimate, but not a default. severity/high is correct.

The enumeration is complete. Rather than grepping for the pattern, I derived the 36 mutating store methods from internal/config/interfaces.go, then parsed every func (h *Handler) body in internal/api for one that both writes and takes an account identifier — 10 candidates, resolved individually:

handler	verdict
upsertLadderConfig	fixed here (my per-function heuristic flagged it as unscoped only because the check lives in requireLadderAccountAccess)
setPlanAccounts	genuinely unscoped — PUT /api/plans/{id}/accounts, Auth: AuthUser, update:plans, then SetPlanAccounts(ctx, id, body.AccountIDs) with no scoping on any supplied ID. Confirms #1769.
persistDiscoveredMembers, approveRegistration	AuthAdmin routes — outside the scoped-caller class
persistRetryExecution, persistExecutionAndSuppressions	account id derived from the execution/recs, not caller-supplied
revokeScheduledExecution	gated by authorizeSessionRevokeExecution
saveAccountServiceOverride, deleteAccountServiceOverride, marketplaceCancel	already scoped

So the class has exactly two members: this one and #1769. Ladder-specific: only two ladder store calls exist in the API — GetLadderConfigs (already filtered) and UpsertLadderConfig (now guarded); there is no delete endpoint. No unguarded sibling remains.

The name-only AssertCalled(t, "UpsertLadderConfig") calls are legitimate. MockConfigStore shadows the helpers with callLog-backed versions (internal/mocks/assertions.go:134,154) where name-only deliberately means "called at all", and #1750's TestNoUnfailableMockAssertions passes on the rebased head — so these are not the unfailable form.

---

Two observations — neither blocks this PR

#1769 is more reachable than #1539, which may warrant re-triage. Plan Authors (00000000-...-000000000003) is a seeded group holding update:plans (migration 000024). Its seeded allowed_accounts is ['*'], so out of the box it is unrestricted and the gap is moot — but narrowing an existing seeded group's allowed_accounts is a one-field operator change, whereas #1539 requires creating a custom group and granting a verb no seeded group carries. Worth reflecting in #1769's labels.

handler_purchases_revoke.go:398 has the wildcard-predicate error in the over-refusal direction (pre-existing, unrelated to this diff):

if len(allowed) > 0 && !stringInSlice(*record.CloudAccountID, allowed) {

stringInSlice is exact-match and ResolveAllowedAccounts returns AllowedAccounts verbatim, so a scope of ["*"] satisfies len > 0 and fails the membership test — a 403 on every revoke-own. Latent rather than live: admins short-circuit on revoke-any at :361, and no migration seeds revoke-own (validated — no "action":"revoke grant appears in any migration, and DefaultUserPermissions() is consumed only by cmd/gen-permissions). It needs an operator-created group granting revoke-own with ['*'], which is the shape copying a seeded group would produce. Denial of a feature, not a security hole. Worth its own issue.

---

Gates at c57b40081

gate	result
gofmt -l .	clean (0 files)
go build ./... / go vet ./...	exit 0 / exit 0
gocyclo -over 10 -ignore "_test\.go" .	exit 0, empty
go test -race -count=1 ./... at PR head	31 ok, 1 FAIL — TestGrantCeiling_ConstraintContainment, the known inherited failure (branch predates #1772)
same, rebased onto 6df07bb7f — internal/auth, internal/api, internal/mocks	all ok, so the inherited failure is the only test failure and it is not this PR's
golangci-lint v2.10.1, root module, comm set diff	4 new, 0 removed — F1

Verdict: not mergeable as-is on F1 alone. The security change itself is correct, well-tested in both directions, verified at the routed endpoint, and the enumeration is complete — fix the shadowing and this is ready.

[cristim]

F1 fixed — head 606484940

The four govet shadow findings were mine, and a direct side effect of this branch. Capturing the session changed if _, err := h.requirePermission(...) (which bound err inside the if-statement) into session, err := ..., creating a function-scoped err that the four subsequent if err := ... statements in the same function then shadowed:

internal/api/handler_ladder.go:100:5: shadow: declaration of "err" shadows declaration at line 89 (govet)
internal/api/handler_ladder.go:115:5: shadow: ...
internal/api/handler_ladder.go:120:5: shadow: ...
internal/api/handler_ladder.go:124:5: shadow: ...
4 issues:
* govet: 4

Fixed by binding the permission error as permErr rather than err. That leaves the four body-parsing and validation statements untouched — they are unrelated to this change, and rewriting them to err = ... would enlarge the diff for no behavioural gain — and matches the naming the package already uses for exactly this situation (planErr, apiKeyErr, awsIDErr).

Rebased onto 6df07bb7f at the same time, so the internal/auth fixture fix from #1772 is genuinely in this branch rather than only in a merge ref. internal/auth now passes here: ok github.com/LeanerCloud/CUDly/internal/auth 11.135s.

Why the earlier gate run did not catch this

My first golangci-lint run in this worktree came back exit 3 with zero findings and the message parallel golangci-lint is running — lock contention from a concurrent agent, not a clean result. I treated it as unrun and re-ran serially, which reproduced all four findings. Worth recording because exit 3 with an empty finding list reads exactly like a pass; the verdict needs exit 0 AND a genuine 0 issues. line, and this run had neither.

Gates at 606484940

gate	exit	verdict
golangci-lint v2.10.1 (CI-pinned)	0	0 issues. line present — genuinely clean
go build ./internal/api/	0	pass
go test ./internal/api/	0	ok, 34.9s
go test ./internal/auth/	0	ok, 11.1s — the inherited failure is gone
ladder handler tests	0	10/10

Guard re-verified after the rebase, not carried forward

A rebase plus an edit inside the same function is where a previously-verified guard can quietly stop being load-bearing, so the mutations were re-run on the new head rather than assumed to still hold. Each test run alone, after committing:

mutation	expected	observed
M1 scope check → plain GetCloudAccount (the fix removed)	out-of-scope test fails	FAIL — in-scope and unrestricted still pass, so M1 stays precisely targeted
M2 refuse everyone	both positive controls fail	both FAIL — they are still not vacuous

git diff clean after each inverse-edit undo.

Thanks for the enumeration detail on the review — deriving the 36 mutating store methods from interfaces.go and resolving all 10 candidates individually is a stronger result than my grep-based sweep, and knowing the class has exactly two members bounds #1769 usefully. Picking up #1769 next.

[cristim]

Merging. Closes #1539.

Merging on independent adversarial review — CodeRabbit has no verdict at this head.

upsertLadderConfig wrote commitment-laddering config for accounts outside the callers allowed_accounts, demonstrated by probe pre-fix with the out-of-scope write reaching the store.

Re-triaged severity/critical -> high during investigation, before any code was written. No seeded group grants update:config to a scoped user — migration 000088 says so explicitly, and ResourceConfig appears in the default permission sets only as {ActionView, ResourceConfig}. The failure scenario therefore needs an operator-created custom group granting update:config and carrying a restricted allowed_accounts: legitimate and supported, but not the default. It stayed high rather than dropping further because once laddering is in use at all, both global flags are on by definition, so the tamper case is cheap from there.

The enumeration is worth recording for how it was done. Rather than grepping for the pattern, the reviewer derived the 36 mutating store methods from internal/config/interfaces.go, then parsed every func (h *Handler) body in internal/api for one that both writes and takes an account identifier — 10 candidates, each resolved individually.

Result: the class has exactly two members, this one and #1769. Only two ladder store calls exist in the API — GetLadderConfigs (already filtered) and UpsertLadderConfig (now guarded) — and there is no delete endpoint. No unguarded sibling remains.

A grep tells you what matched a pattern; deriving the candidate set from the interface and resolving each supports the stronger claim that the class is closed. That also makes #1769 the last member rather than the start of an open-ended sweep. This is the recurring failure shape in this repo — a guard on the primary writer defeated by an unguarded sibling writing the same field. It is why #1737s first enumeration missed AllowedAccounts, why #1752 needed the adapter as well as the handler, and why #1765 turned out to have two routes rather than one.

Verified in review: the guard refuses at the routed endpoint, not merely in the scoping helper — the distinction that let #1757 ship a test asserting "CSRF MUST be required" which passed while never touching the enforced path. Both directions covered: out-of-scope refused, in-scope still allowed.

One blocking finding, now fixed: the first revision introduced 4 new govet shadow findings and failed the Lint job. The delta since review is that fix alone (avoid shadowing err in upsertLadderConfig, +9/-3 in handler_ladder.go) plus the red-main fix pulled in by rebase — mechanical, so the security review carries forward unchanged.

Gates at 606484940: 20/20 checks pass, zero unresolved threads.

Sibling still open: #1769 (setPlanAccounts, handler_accounts.go:1304) — same shape, same investigation, filed separately because it needs a different file and lands independently.