sec(exchange): scheduled auto-exchange bypasses the execute:ri-exchange carve-out via update:config

[cristim]

Summary

execute:ri-exchange is now carved out of admin:* and only reachable via a group membership check (issue #1644, PR #1758). That closes the direct routed-handler path: POST /api/ri-exchange/execute and POST /api/ri-exchange/azure-instances/exchange now refuse a plain admin:* principal.

The scheduled automation path is a separate route to the same money-moving action and is NOT covered by the carve-out.

Update (follow-up investigation): there are two independent routes to this bypass, not one -- see "Two routes, not one" below. A fix on the dedicated RI-exchange config endpoint alone would look complete and would not be.

Call chain

1. PUT /api/ri-exchange/config (internal/api/router.go:316 -> internal/api/handler_ri_exchange.go:1942 updateRIExchangeConfig) is gated on update:config, which is intentionally not carved out -- PR sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group #1758's own control test (TestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs, internal/api/ri_exchange_carveout_test.go:84) asserts admin:* must keep update:config. The request body (RIExchangeConfigUpdateRequest) lets the caller set mode ("manual"/"auto"), auto_exchange_enabled, and both spend caps (max_payment_per_exchange_usd, max_payment_daily_usd) in one write.
2. The scheduled task TaskRIExchangeReshape (internal/server/handler.go:194) calls handleRIExchangeReshape (internal/server/handler_ri_exchange.go:35), which checks cfg.RIExchangeEnabled and, if enabled, calls executeRIExchangeReshape -> exchange.RunAutoExchange (pkg/exchange/auto.go:156).
3. Inside RunAutoExchange, processRecommendation branches on params.Config.Mode (pkg/exchange/auto.go:248): mode == "manual" routes to processManualExchange, which generates an approval token and leaves the exchange Pending for a human to approve/reject. Any other mode (i.e. "auto") routes to processAutoExchange (pkg/exchange/auto.go:477), which executes the exchange against the provider directly.

There is no execute:ri-exchange check, and no RI Exchanger group membership check, anywhere on this path.

Two routes, not one -- this is the headline

GlobalConfig.RIExchangeEnabled / RIExchangeMode (internal/config/types.go:36-37) are plain fields on the same struct both of these endpoints write:

• The dedicated endpoint: PUT /api/ri-exchange/config -> updateRIExchangeConfig (internal/api/handler_ri_exchange.go:1942), gated on update:config only (line 1943).
• The generic settings endpoint: PUT /api/config -> updateConfig (internal/api/handler_config.go:61), also gated on update:config only, and json.Unmarshals the request body directly onto the entire GlobalConfig struct (internal/api/handler_config.go:91) -- so a body of {"ri_exchange_enabled":true,"ri_exchange_mode":"auto"} reaches PUT /api/config exactly as effectively as it reaches the dedicated endpoint.

A fix on updateRIExchangeConfig alone would look complete and would not be -- this is the recurring failure mode in this repo: a guard on the primary writer defeated by an unguarded sibling writing the same field (the same shape as #1737's first AllowedAccounts enumeration gap, and why #1752 needed the adapter fixed as well as the handler). Any fix MUST land as one shared helper called from both updateConfig and updateRIExchangeConfig -- two call sites, one helper, or the next person fixes only one of them.

Why this is a genuine hole, not intended design

PR #1758's carve-out rationale (internal/auth/types.go:110-125, mirrored in the PR description) is explicit: "a compromised admin account alone cannot drain commitments." That guarantee does not hold here. An admin:* principal who is deliberately NOT a member of the RI Exchanger group -- and who therefore gets a 403 from the direct execute handler -- can still cause exchanges to execute automatically:

1. PUT /api/ri-exchange/config (or PUT /api/config) with mode: "auto", auto_exchange_enabled: true, and generous caps (all writable in the same request, gated only on update:config, which admin:* retains).
2. Wait for the next scheduled TaskRIExchangeReshape run.
3. processAutoExchange executes the exchange with no approval step and no execute:ri-exchange/RI-Exchanger check.

This reproduces the exact threat #1644 set out to close (a single compromised or malicious admin draining commitments via RI exchange), just via a different entry point that remains reachable by admin:* alone.

Are the spend caps a real bound? Real mechanism, wrong question.

Enforcement is genuinely live, not just a recommendation-time estimate:

• getValidatedQuote (pkg/exchange/auto.go:294) checks the per-exchange cap against a live AWS quote fetched at execution time.
• processAutoExchange (pkg/exchange/auto.go:477) checks the daily cap against a live GetRIExchangeDailySpend query immediately before executing.
• (H2, pkg/exchange/auto.go:537-540) the actual Execute call's MaxPaymentDueUSD is bounded to min(perExchangeCap, dailyCap-dailySpent), so a fresh re-quote inside Execute cannot slip past the daily ceiling either.

This is careful, real, live-checked enforcement -- not a rubber stamp, and the issue should not imply otherwise.

But it is not a bound against this threat model: RIExchangeMaxPerExchangeUSD / RIExchangeMaxDailyUSD are plain fields on the same GlobalConfig the attacker is already writing to, in the exact same request that flips mode/enabled. The same actor sets the switch and the ceiling together -- the cap bounds each individual exchange's cost, but it is not an independent party's constraint on the attacker.

There is a sharper, categorical gap underneath that: the scheduled path never consults auth.PermissionConstraints at all (grepped pkg/exchange/auto.go and the reshape handler -- zero references). The direct executeExchange handler enforces AccountIDs / Providers / Services / Regions / MaxPurchaseAmount scoping via requirePermissionConstraints (internal/api/handler_ri_exchange.go:1753) on every call. The scheduled path has none of that -- its only bound is a single coarse global USD ceiling, attacker-controlled, with no account/region/provider scoping whatsoever. That is a categorical gap in the auto path's enforcement model, not merely a weaker version of the direct path's control.

Recommended fix direction

Gate the config write that enables auto-mode, narrowly. Do not require approval-tokens for auto mode.

Requiring approval for every auto-mode exchange defeats the point of the mode: processManualExchange vs processAutoExchange (pkg/exchange/auto.go:248-264) shows "auto" exists specifically to execute without a human touch. Routing it through the same pending-approval flow manual mode already uses doesn't harden auto mode, it deletes it -- auto becomes indistinguishable from manual. It's also structurally awkward: the scheduled task runs as a service principal with no session, so there's no natural "who approves" inside a cron invocation.

Gating the write is the structurally coherent option: both updateConfig and updateRIExchangeConfig already resolve a session via requirePermission before touching the DB (in updateRIExchangeConfig it's currently discarded with _), so there IS a principal to check execute:ri-exchange against at write time -- unlike inside the cron job itself.

Concretely: require the caller to also hold execute:ri-exchange (checked via h.auth.HasPermissionAPI(ctx, session.UserID, "execute", "ri-exchange"), the same primitive requireSessionPermission already uses) only for the specific transition into the risky state -- writing RIExchangeMode == "auto" AND RIExchangeEnabled == true (or raising the caps while already in that state). Do not gate the whole update:config-on-ri-exchange-fields surface.

Cost, stated plainly: an update:config-only operator loses the ability to flip the system into unattended-auto-execute. Everything else on this settings surface stays available to them: view:config, setting mode: "manual" (fully functional -- manual just requires a human to click approve on each candidate), tuning UtilizationThreshold / LookbackDays, and even RIExchangeEnabled = true while mode stays "manual" (harmless -- that just means the scheduler identifies candidates and creates Pending approvals, no money moves). So the "configures but doesn't execute" role is preserved for everything except the one write that is functionally equivalent to pre-authorising every future exchange -- the same separation-of-duties boundary #1644 already drew for the direct handlers, extended to the one config write that has the same effect.

Files a fix would touch

• internal/api/handler_ri_exchange.go -- updateRIExchangeConfig (line 1942), needs the supplemental check.
• internal/api/handler_config.go -- updateConfig (line 61), needs the SAME check (see "Two routes, not one" above).
• A new shared helper (natural home near requirePermissionConstraints in internal/api/handler.go), e.g. requireRIExchangeAutoModeGrant(ctx, session, existing, proposed *config.GlobalConfig) error, called from both write paths before/inside the UpdateGlobalConfigAtomic closure.
• Tests mirroring ri_exchange_carveout_test.go's both-direction shape, against BOTH handlers.
• Frontend (UX-honesty, not the security fix -- backend enforcement is what matters): frontend/src/riexchange.ts's renderAutomationSettings (line 1937) renders the "Enable Automated Exchange" toggle and "Mode" select with zero canAccess gating today -- the only friction is a client-side confirm dialog ("Enable Fully Automated mode?", line 2082), which is UX, not authorization. Once the backend gate lands, disable/hide the auto-mode controls for non-RI-Exchanger users using isRIExchanger() (added in the sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group #1758 F2 round, frontend/src/permissions.ts).

Checked negative: the purchase-scheduling tasks are NOT the same shape

TaskProcessScheduledPurchases / TaskFireScheduledPurchases (internal/server/handler.go:242,334) only fire purchase_executions that already went through the direct approve path (gated on the carved-out verb at approval time); no config flag controls whether they're allowed to fire. Confirmed by reading both handlers -- neither reads a Mode/*Enabled config field to decide whether to act.

Related but separate: the ladder-automation feature has the identical pattern, currently inert

Filed separately as LeanerCloud/cloud-commitments-platform#178 rather than buried in this thread, so it surfaces when someone works on the ladder engine: GlobalConfig.LadderingEnabled + LadderExecutionEnabled are the identical config-gates-a-scheduled-write-side pattern, but the planning engine never actually calls PurchaseLayer/ReshapeBuffer yet (zero non-test callers), so it isn't exploitable today. It will reintroduce this issue's shape the moment that wiring ships.

Refs #1644, #1758, LeanerCloud/cloud-commitments-platform#178.

[cristim]

Follow-up investigation (no code changes -- gates: which fix direction, whether the pattern exists elsewhere, which files, whether the caps are a real bound). Issue body above has been rewritten to incorporate all four; summarized here for the record:

1. Fix direction: gate the config write into mode:"auto" + enabled:true, not approval-tokens for auto mode. Approval-for-auto would make auto indistinguishable from manual (deletes the feature) and is structurally awkward (the scheduled task has no session/principal to approve against). Gating the write is coherent because both write paths already resolve a session. Recommendation is scoped to the specific risky transition, not the whole update:config surface -- see "Recommended fix direction" above for the full reasoning and the cost this imposes on config-only operators.

2. Does the same shape exist elsewhere? Yes, in the ladder-automation feature (LadderingEnabled/LadderExecutionEnabled) -- identical config-gates-a-scheduled-write-side pattern. Currently inert: zero non-test callers of .PurchaseLayer(/.ReshapeBuffer( on LadderCapability, confirmed by grep and by handleLadderRun's own "plan-only" docstring. Filed separately as LeanerCloud/cloud-commitments-platform#178 (referencing this issue and LeanerCloud/cloud-commitments-go#27) rather than buried here, since whoever wires that engine needs to find it by searching the ladder code. Checked negative: TaskProcessScheduledPurchases/TaskFireScheduledPurchases are NOT the same shape -- they only fire executions already approved through the carve-out-gated direct path; no config flag gates whether they're allowed to act.

3. Files a fix would touch: internal/api/handler_ri_exchange.go (updateRIExchangeConfig), internal/api/handler_config.go (updateConfig -- this is the headline finding, see "Two routes, not one" above), a new shared helper near requirePermissionConstraints in internal/api/handler.go, tests mirroring ri_exchange_carveout_test.go's both-direction shape against both handlers, and (UX-honesty, not the security fix) frontend/src/riexchange.ts's renderAutomationSettings, which has zero canAccess gating today -- isRIExchanger() from the #1758 F2 round is the helper for it.

4. Are the caps a real bound? The enforcement mechanism is real and live (live quote at execution, live daily-spend query, min(perExchange, daily-spent) bound on the actual Execute call) -- that should be stated plainly, not undersold. But it is not an independent bound against this threat model: the same actor writes the switch and the ceiling in the same request. Sharper and more categorical: the scheduled path never consults auth.PermissionConstraints at all (zero references), so it has none of the account/region/provider/amount scoping the direct handler enforces via requirePermissionConstraints -- not a weaker version of that control, a different (absent) one.

No implementation here per plan -- holding for sequencing behind #1757 (CSRF work in handler_ri_exchange.go) and the #1758 review.