Repository navigation
sec(exchange): gate the config write that arms unattended RI exchange - #1773
Conversation
📝 WalkthroughWalkthroughThe change adds authorization checks for RI exchange auto-mode configuration. Arming auto mode or increasing spending caps requires ChangesRI exchange authorization
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigurationHandler
participant AuthorizationGate
participant ConfigStore
Client->>ConfigurationHandler: submit RI exchange configuration
ConfigurationHandler->>ConfigStore: snapshot current state under lock
ConfigurationHandler->>AuthorizationGate: validate proposed state and credential
AuthorizationGate-->>ConfigurationHandler: allow or return 403
ConfigurationHandler->>ConfigStore: persist allowed configuration
ConfigurationHandler-->>Client: return update result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cristim
added
triaged
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/api/ri_exchange_automode_gate_test.go (2)
103-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the comment subject.
The comment starts with "armed is the stored state". The function is
armedConfig.✏️ Proposed wording fix
-// armed is the stored state an attacker would be escalating FROM or operating -// within: unattended execution already switched on. +// armedConfig is the stored state an attacker would be escalating FROM or +// operating within: unattended execution already switched on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/ri_exchange_automode_gate_test.go` around lines 103 - 104, Update the comment immediately above armedConfig to refer to armedConfig as the stored state, replacing the incorrect “armed” subject while preserving the rest of the explanation.
192-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive control for the cap-raise transition.
The matrix gates cap raises with
autoGateConfigOnlyonly. No case raises a cap while holdingexecute:ri-exchange. Without that case, a gate that refuses every cap raise regardless of permission still passes. The arming transition already has its positive control at line 166.♻️ Proposed additional case
{ name: "raising the daily cap while already armed is refused", perms: autoGateConfigOnly, stored: armedConfig(), mode: "auto", enabled: true, perExchange: 100, daily: 100_000, wantRefused: true, }, + { + name: "raising a cap while armed with execute:ri-exchange is allowed", + perms: autoGateConfigPlusExecute, + stored: armedConfig(), + mode: "auto", + enabled: true, + perExchange: 100_000, daily: 100_000, + wantRefused: false, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/ri_exchange_automode_gate_test.go` around lines 192 - 218, Add a positive test case to the cap-raise scenarios in the automode gate matrix using permissions that include execute:ri-exchange, with an armed stored configuration and a raised per-exchange or daily cap; assert the transition is allowed. Keep the existing autoGateConfigOnly refusal cases unchanged.internal/api/handler.go (1)
572-586: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the tail of the gate.
The final
ifonly forwards the error. Return the call result directly.♻️ Proposed simplification
- if _, err := h.requireSessionPermission(ctx, session, auth.ActionExecute, auth.ResourceRIExchange); err != nil { - return err - } - return nil + _, err := h.requireSessionPermission(ctx, session, auth.ActionExecute, auth.ResourceRIExchange) + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler.go` around lines 572 - 586, In requireRIExchangeAutoModeGrant, replace the final error-checking if block with a direct return of h.requireSessionPermission(ctx, session, auth.ActionExecute, auth.ResourceRIExchange), preserving the existing early returns and arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/handler.go`:
- Around line 572-586: In requireRIExchangeAutoModeGrant, replace the final
error-checking if block with a direct return of h.requireSessionPermission(ctx,
session, auth.ActionExecute, auth.ResourceRIExchange), preserving the existing
early returns and arguments.
In `@internal/api/ri_exchange_automode_gate_test.go`:
- Around line 103-104: Update the comment immediately above armedConfig to refer
to armedConfig as the stored state, replacing the incorrect “armed” subject
while preserving the rest of the explanation.
- Around line 192-218: Add a positive test case to the cap-raise scenarios in
the automode gate matrix using permissions that include execute:ri-exchange,
with an armed stored configuration and a raised per-exchange or daily cap;
assert the transition is allowed. Keep the existing autoGateConfigOnly refusal
cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1623f06b-2e17-49db-a14d-098573d40ceb
📒 Files selected for processing (4)
internal/api/handler.gointernal/api/handler_config.gointernal/api/handler_ri_exchange.gointernal/api/ri_exchange_automode_gate_test.go
22de327 to
cb7898b
Compare
Independent adversarial review — head
|
| route | response |
|---|---|
PUT /api/config |
permission check failed: user not found — IsClientError=false (500) |
PUT /api/ri-exchange/config |
failed to save config: permission check failed: user not found — IsClientError=false (500) |
The second is literally the outcome the PR body says it removed ("a refusal is returned as its own 403 rather than … blaming the store for an authorization decision"). The IsClientError unwrap in updateRIExchangeConfig doesn't catch it because the error isn't a ClientError in the first place.
Direction is fail-closed, so this is not exploitable — but it blocks nothing either: the same admin API key still reaches execute:ri-exchange directly at handler_ri_exchange.go:1172 and :1713 via the same short-circuit, and requirePermissionConstraints explicitly bypasses the sentinel (handler.go:498-500). Net effect is a documented full-access infrastructure credential losing the ability to save global config whenever the result is armed, with an opaque 500. Please decide the sentinel's treatment explicitly and pin it with a test.
F3 — Low: before := *existing is a shallow snapshot
Correct today (every gated field is a scalar). But GlobalConfig carries GracePeriodDays map[string]int and EnabledProviders []string, and json.Unmarshal into a non-nil slice reuses the backing array. If a future axis of the gate reads a slice or map field — account scoping is the obvious candidate — the "snapshot" silently stops being one. Worth a word in the comment, which currently just says "Snapshot before the wholesale unmarshal".
F4 — Low, test fidelity: the suite drives handlers, not routes
Every case calls h.updateConfig / h.updateRIExchangeConfig directly with a principal holding exactly [update:config]. On the real route, PUT /api/config is AuthAdmin (router.go:110), so that principal never reaches the handler — the production principal there is admin:*. I confirmed the gate is still correct for admin:* on both routes through Router.Route (403, write blocked, both). But the two credential shapes that genuinely diverge from a bearer session — user API key and admin API key — are exactly the two the suite never exercises, and both turned out to carry defects (F1, F2).
What I verified and found correct
Priority 1 — two write paths, plus the search for a third. Enumerated producers, not consumers. Only two production writers of GlobalConfig exist, both via UpdateGlobalConfigAtomic (handler_config.go:95, handler_ri_exchange.go:1965), and both call the helper. SaveGlobalConfig has no production caller outside the store implementation and the test mock. No third writer: migration 000010_ri_exchange_global_config.up.sql defaults to false / 'manual'; the ri_exchange.* keys in internal/config/defaults.go belong to DefaultSettings, which has zero non-test consumers — an inert registry, not a write path. (Method: git grep with [[:space:]] character classes, never \s; each negative sanity-checked against a line known to match — the assignment sweep returned the four real writers plus two == 0 comparisons, proving the pattern was live. Both the string literals "execute", "ri-exchange" and the constants auth.ActionExecute / auth.ResourceRIExchange were searched; the codebase uses both forms and both were found.)
The predicate mirrors the consumer. pkg/exchange/auto.go:248 routes to processManualExchange on params.Config.Mode == "manual" only. RIExchangeEnabled is the master switch at internal/server/handler_ri_exchange.go:42. GlobalConfig.Validate does not constrain the mode. Enabled && Mode != "manual" is right, and the dedicated endpoint's validate() genuinely restricts mode to manual|auto, so limiting _NonManualModeIsArmed to the generic route is correct.
The deliberate hole is justified. processManualExchange generates an approval token and persists a Pending record — no execution. So "enabled while mode stays manual is allowed" really does move no money.
Cap gating is not theatre. Both caps are live-enforced in processAutoExchange (pkg/exchange/auto.go:497-540, chooseEffectiveCap), so refusing a cap raise while armed protects a real bound.
No two-step bypass. enabled=true, mode="manual" then mode="auto" → second write has wasArmed=false → refused. Reverse order likewise. Raising a cap while disarmed is allowed but arming afterwards still requires the verb.
Priority 2 — mutation-verified independently, each mutation applied alone against a committed tree, restored by re-applying the inverse from a pristine snapshot (never git checkout --), worktree confirmed clean before and after every measurement. All six killed:
| mutation | result | killed by |
|---|---|---|
M1 — gate removed from PUT /api/config only |
KILLED | only the generic PUT /api/config subtests + all 5 NonManualModeIsArmed |
M2 — gate removed from PUT /api/ri-exchange/config only |
KILLED | only the dedicated subtests |
M3 — predicate narrowed to Mode == "auto" |
KILLED | all 5 NonManualModeIsArmed |
| M4 — cap raise while armed ungated | KILLED | both cap-raise quadrants, both endpoints |
| M5 — refuse-everyone | KILLED | all 4 "still allowed" quadrants |
M6 — gate checks update:config |
KILLED | all refusal quadrants + NonManualModeIsArmed |
M1 and M2 killing disjoint subtest sets is genuine evidence that the two-route coverage is real rather than incidental. The PR body's mutation table reproduces.
Scope. The PR body's "Out of scope, flagged not fixed" section names the frontend renderAutomationSettings gap and LeanerCloud/cloud-commitments-platform#178 explicitly. It does not imply broader coverage.
Gate results (my runs, at cb7898b10)
| gate | result |
|---|---|
gofmt -l . |
clean, exit 0 |
go vet ./... |
exit 0 |
gocyclo -over 10 -ignore "_test\.go" . |
empty, exit 0 |
go test -race -count=1 ./... |
exit 0 — 6928 passed, 43 packages |
golangci-lint v2.10.1 (CI pin, golangci-lint has version 2.10.1) |
head: exit 0 and 0 issues.; base eca603a36: exit 0 and 0 issues. |
comm set diff, base vs head |
empty in both directions — zero new, zero removed |
Branch is 3 commits behind main (main = eca603a36; PR parent = 726389b48) — worth a rebase before merge, though nothing in the delta touches these files.
Live CI at review time: 20 passed / 0 failed. Review state: only coderabbitai (COMMENTED, 2026-08-08); no human review.
cb7898b to
f833ab0
Compare
F1–F4 addressed at
|
| mutation | killed by |
|---|---|
| M0 — verb resolved against the owning user instead of the credential | only the two UserAPIKeyUsesKeyPermissions cases |
M1 — gate removed from PUT /api/config only |
the generic subtests + all 5 NonManualModeIsArmed |
M2 — gate removed from PUT /api/ri-exchange/config only |
the dedicated subtests + the key test |
M3 — predicate narrowed to mode == "auto" |
all 5 NonManualModeIsArmed |
| M4 — cap raises while armed no longer gated | both cap quadrants, both routes |
| M5 — gate applied to every config write (refuse-everyone) | all 4 "still allowed" quadrants, both routes |
M0 killing only the key tests is the useful signal: a session principal resolves identically either way, so the mutation breaks exactly one axis and exactly one test catches it. That is the axis that had no coverage before.
Gates at f833ab067
| gate | result |
|---|---|
gofmt -l . |
clean (0 files) |
go build ./... / go vet ./... |
exit 0 / exit 0 |
gocyclo -over 10 -ignore "_test\.go" . |
exit 0, empty |
internal/api + internal/auth + internal/mocks, -race |
all ok, 0 FAIL |
golangci-lint v2.10.1, root module |
exit 0 and a genuine 0 issues. line |
Rebased onto eca603a36 before this work; main has since moved to ea0578cfc, one commit ahead, no conflict.
Nothing touched in the areas the review confirmed correct — the two-writer enumeration, the four quadrants, and the NonManualModeIsArmed predicate coverage are unchanged apart from being re-pointed at routes.
Delta review —
|
scenario (all via Router.Route, PUT /api/ri-exchange/config) |
result |
|---|---|
key scoped to update:config, owner holds execute:ri-exchange |
403, message names execute on ri-exchange, SaveGlobalConfig not called |
key that legitimately holds execute:ri-exchange |
allowed, write lands |
scoped key writing mode:"manual" |
allowed, write lands |
The middle row is the one that matters for not breaking real CI credentials, and the third confirms the configure-but-do-not-execute role survives on the key axis too. I asserted the refusal's provenance (the 403 text names the verb and resource) so it is provably the gate refusing rather than some other check on the path.
Mutation. Reverting the gate to the pre-fix authorization — resolve the session from the request, then requireSessionPermission on the session's user — makes TestRIExchangeAutoModeGate_UserAPIKeyUsesKeyPermissions/key_without_execute:ri-exchange_is_refused fail. The coverage is real. Control mutation (gate authorizes update:config through requirePermission instead) is killed cleanly by all three refusal quadrants on both routes plus the API-key test.
Routing requirePermission rather than adding a second key-aware branch was the right call. Router.Route establishes a principal before dispatch (router.go:394-416; validateSecurityContext at handler.go:766-770 on the real request path), so inside the gate requirePermission reads the principal and does not re-validate the credential from headers — no second ValidateSession inside the advisory-locked transaction.
F5 — NEW: the key test's decision function has the wrong signature, so it kills by panic instead of by its own assertion
autoGateKeyRouter registers the owning user's permissions as a 4-argument function:
mockAuth.On("HasPermissionAPI", …).
Return(func(_ context.Context, _, action, resource string) bool { … }, nil).Maybe()permissionDecision (internal/api/mocks_test.go:289) type-asserts to a 2-argument func(action, resource string) bool. The assertion fails, control falls through to args.Bool(0), and that panics:
panic: assert: arguments: Bool(0) failed because object wasn't correct type
Two consequences:
- The premise the test documents is never actually modeled. The comment says "The owning user's groups are BROADER than the key… A gate that resolved the verb against the USER would pass here" — but the owner's decision function can never be consulted, so that scenario is asserted rather than exercised.
- The regression kills by panic, not by assertion. A panic aborts the whole test binary, so a future regression here takes the rest of
internal/api's results down with it and reports as a stack trace rather than as "arming was not refused".
Proof it is exactly the signature: change that one function to func(action, resource string) bool and re-run the same mutation — it then fails cleanly at ri_exchange_automode_gate_test.go:336 with require.Error / "An error is expected but got nil", which is precisely the intended assertion and precisely the F1 bug. One-line fix; worth taking before merge.
On the #1744 shadowing class specifically: the ordering here is correct. The two specific HasAPIKeyPermissionAPI registrations precede the mock.Anything catch-all, and testify's findExpectedCall returns the first matching registration with Repeatability > -1, so the specific ones serve the calls. I confirmed independently by re-running the same scenarios with no catch-all at all — identical results. The catch-all still earns deletion (it can only mask a change in which verbs the path queries, and it returns true), but it is not shadowing anything today.
F2 — resolved, and the resulting semantic should be said plainly
The stateless admin API key now succeeds on both routes: err=<nil>, write lands. The failed to save config: … user not found 500 is gone. Inside the gate, requirePermission reads PrincipalAdminAPIKey and returns the session with no permission check at all — consistent with the same key on the direct execute handlers (handler_ri_exchange.go:1172, :1713) and with requirePermissionConstraints' explicit sentinel bypass, exactly as the new comment claims.
So the semantic that ships is: the stateless admin API key can arm unattended RI exchange with no execute:ri-exchange check. That is defensible — it is an infrastructure credential, not a user account, and #1758's carve-out is about compromised admin accounts — but nothing pins it. If someone later hardens requirePrincipalPermission, no test catches the change on a money path. Two lines would fix that.
F3 — fixed, better than what was asked
riExchangeArmState is an explicit value snapshot of exactly the four compared scalars. That removes the aliasing question structurally instead of commenting around it, and makes the gate's signature say what it reads.
F4 — fixed
Every case now runs through Router.Route, and the generic route's principal is admin:* — the principal that can actually reach an AuthAdmin route, and the #1765 threat model. Small residual: with the matrix on admin:*, a plain non-admin session holding only update:config on the dedicated AuthUser route is no longer covered by any quadrant. The API-key tests do cover a non-admin credential, so the shape is not wholly untested. Low.
Re-verified at this head (no regression from the delta)
- The predicate still mirrors the consumer:
riExchangeArmState.armed()isenabled && mode != "manual", matchingpkg/exchange/auto.go'sparams.Config.Mode == "manual"routing. - Both spend caps are still live-enforced in
processAutoExchange/chooseEffectiveCap, so gating cap raises still protects a real bound. - Still exactly two production writers of
GlobalConfig, both routed through the gate. - All four quadrants intact on both routes.
Gates at f833ab067
| gate | result |
|---|---|
gofmt -l . |
clean, exit 0 |
go vet ./... |
exit 0 |
gocyclo -over 10 -ignore "_test\.go" . |
empty, exit 0 |
go test -race -count=1 ./... |
exit 0 — 6931 passed, 43 packages |
| golangci-lint v2.10.1 | head exit 0 and 0 issues.; merge-base eca603a36 exit 0 and 0 issues. |
comm set diff vs merge-base |
empty in both directions |
| live CI | 20 passed / 0 failed |
Two housekeeping notes: the branch is 1 behind main (ea0578cfc), so a rebase is needed — trivial, nothing in #1795 touches these files. And the CodeRabbit check reports pass with the reason "Review rate limited", which is not evidence that a review ran.
execute:ri-exchange is carved out of admin:* (#1644, PR #1758) so a compromised admin account alone cannot drain commitments. update:config is deliberately NOT carved out -- but a single update:config write could set ri_exchange_mode "auto" plus ri_exchange_enabled, after which the scheduled TaskRIExchangeReshape -> RunAutoExchange -> processAutoExchange executed exchanges against the provider with no execute:ri-exchange check anywhere on that path. The carve-out's guarantee did not hold via that route. Arming auto mode is functionally equivalent to pre-authorizing every future exchange, so the transition into that state now requires the caller to also hold execute:ri-exchange. The verb is reused, not reinvented. Authorization goes through requirePermission with the request the caller was authenticated from, so the verb is evaluated against THE CREDENTIAL PRESENTED rather than the owning user's group permissions. For a user API key that routes to authorizeAPIKey and checks the KEY's effective permissions. Resolving it against the session's user id instead let a CI key that is correctly denied execute:ri-exchange arm the scheduler to execute every exchange -- the same permission-inheritance bug requirePermissionConstraints already guards against one function above, reintroduced one function later. It also gives the stateless admin API key the same answer here as on the direct execute handlers, rather than failing a group lookup on a sentinel user id and surfacing a 500 that blamed the store for an authorization outcome. Both write paths are gated through one helper. PUT /api/config unmarshals the body onto the whole GlobalConfig, so it reaches ri_exchange_mode and ri_exchange_enabled exactly as effectively as the dedicated PUT /api/ri-exchange/config. The two differ in reach, though: /api/config is AuthAdmin and unreachable to a user API key, so the dedicated route carries the credential axis alone -- covering both handlers was necessary and not sufficient. "Armed" is defined as enabled AND mode != "manual", mirroring the consumer rather than the field's nominal domain. processRecommendation routes to processManualExchange only on the literal "manual" and sends every other value to processAutoExchange, and GlobalConfig.Validate never constrains RIExchangeMode -- so via PUT /api/config a mode of "Auto", "automatic" or "" arms unattended execution just as effectively as "auto". A gate written as mode == "auto" would have been open on precisely the route that matters. The compared state is a four-field value snapshot rather than a copy of the whole struct: GlobalConfig carries slices and maps that a shallow copy would alias, so narrowing to the fields the decision reads removes the question. Only the escalation is gated. An update:config-only operator keeps view:config, mode "manual", the utilization and lookback tunables, and ri_exchange_enabled while mode stays manual -- that only has the scheduler raise Pending approvals, which move no money. Raising a spend cap while already armed is also gated, since the caps are plain fields on the same struct and the same actor would otherwise set both the switch and its bound. Lowering a cap, disarming, and an idempotent re-save of an already-armed config are de-escalations and stay available. The check runs inside the UpdateGlobalConfigAtomic closure so it compares against the same advisory-locked row the write lands on; checking beforehand would race a concurrent writer between the check and the write. Tests drive Router.Route rather than the handlers: the credential axis is invisible below the router, which is why a handler-level suite passed while the key bypass was live. Refs #1765
f833ab0 to
7c398af
Compare
F5 fixed at
|
| gate | result |
|---|---|
gofmt -l . |
clean (0 files) |
go build ./... / go vet ./... |
exit 0 / exit 0 |
gocyclo -over 10 -ignore "_test\.go" . |
exit 0, empty |
internal/api + internal/auth + internal/mocks, -race |
all ok, 0 FAIL |
golangci-lint v2.10.1, root module |
exit 0 and a genuine 0 issues. line |
Force-pushed with --force-with-lease pinned to f833ab06775e48fe3c758a8920f3eeb8d72fef80.
Nothing else touched — the two-writer enumeration, the four quadrants, NonManualModeIsArmed, and the F1/F2/F3 fixes are unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/ri_exchange_automode_gate_test.go (1)
326-358: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd stateless admin API-key route coverage.
The test matrix covers bearer sessions and user API keys only. It does not cover the documented stateless admin API-key bypass. Add allowed arming cases for
dedicatedPathandgenericPath. This prevents a synthetic-user lookup failure or an incorrect denial from returning without regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/ri_exchange_automode_gate_test.go` around lines 326 - 358, Extend TestRIExchangeAutoModeGate_UserAPIKeyUsesKeyPermissions with stateless admin API-key cases that arm auto mode successfully through both dedicatedPath and genericPath. Configure each request with the documented stateless admin key, assert no routing error, and verify the store write occurs, covering both route variants without relying on synthetic-user lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/ri_exchange_automode_gate_test.go`:
- Around line 326-358: Extend
TestRIExchangeAutoModeGate_UserAPIKeyUsesKeyPermissions with stateless admin
API-key cases that arm auto mode successfully through both dedicatedPath and
genericPath. Configure each request with the documented stateless admin key,
assert no routing error, and verify the store write occurs, covering both route
variants without relying on synthetic-user lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec58eb1a-f345-4373-a8fe-bf8781476dec
📒 Files selected for processing (4)
internal/api/handler.gointernal/api/handler_config.gointernal/api/handler_ri_exchange.gointernal/api/ri_exchange_automode_gate_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/handler_ri_exchange.go
- internal/api/handler.go
|
Merging. Closes #1765. Merging on independent adversarial review — CodeRabbit produced no verdict at any head here. The defect
Review found a live bypass in the fix, and it is the sharpest finding of this seriesThe first version authorized via
A CI key that cannot execute a single exchange could arm the scheduler to execute all of them. It reintroduced the exact class #1758s F2 round fixed, twenty lines from the comment explaining why: "This prevents a CI key with MaxPurchaseAmount=$100 from spending up to the owning users full group limit." Two things made it invisible. The authors suite drove handlers rather than routes, and the bypass only appears through Fixed by routing through Then the verifying test was itself killing by panicF5: the decision function wired into the key test had the wrong signature, so under mutation it died via a testify panic rather than via Three layers, each visible only once the one above it was fixed. Verified, by execution
Scope, stated rather than impliedGates the transition, not the surface. An The frontend gap ( One residual, recorded rather than fixed: with the F4 matrix on Gates at |
1 participant
Closes #1765
The defect
execute:ri-exchangeis carved out ofadmin:*(#1644, PR #1758) so "a compromised admin account alone cannot drain commitments."update:configis deliberately not carved out — PR #1758's own control test assertsadmin:*must keep it.But a single
update:configwrite could setri_exchange_mode: "auto"plusri_exchange_enabled: true, and the scheduledTaskRIExchangeReshape→RunAutoExchange→processAutoExchangethen executed against the provider with noexecute:ri-exchangecheck anywhere on that path. The carve-out's guarantee did not hold via that route.The fix
Arming auto mode is functionally equivalent to pre-authorizing every future exchange, so the transition into that state now requires the caller to also hold
execute:ri-exchange. The existing verb is reused, not reinvented.Both write paths, one helper
PUT /api/config(updateConfig) unmarshals the body onto the wholeGlobalConfig, so it reachesri_exchange_mode/ri_exchange_enabledexactly as effectively as the dedicatedPUT /api/ri-exchange/config(updateRIExchangeConfig). A gate on the dedicated endpoint alone would have looked complete and would not have been. One helper —requireRIExchangeAutoModeGrant— is called from both. Both already resolved a session; one merely discarded it with_."Armed" mirrors the consumer, not the field's nominal domain
This is the part that would have shipped a hole.
processRecommendation(pkg/exchange/auto.go:248) routes toprocessManualExchangeonly on the literal"manual"and sends every other value toprocessAutoExchange. AndGlobalConfig.Validatenever constrainsRIExchangeMode— verified: the field appears only in the struct, the Postgres scan, and the DB default.So via
PUT /api/config, a mode of"Auto","automatic"or""arms unattended execution just as effectively as"auto", while a gate written asMode == "auto"would wave it straight through — on precisely the route the issue calls the headline. The predicate is thereforeEnabled && Mode != "manual", andTestRIExchangeAutoModeGate_NonManualModeIsArmedpins it against five such strings.The dedicated endpoint's own
validate()restricts mode tomanual|auto, so that shape cannot reach it. That asymmetry is exactly why the gate reads the field the scheduler reads rather than the one the stricter endpoint validates.Only the escalation is gated
An
update:config-only operator keeps everything except the one write:update:configalonemode: "manual"ri_exchange_enabled: truewhile mode stays manualenabled+ non-manual mode)Cap raises are gated because the caps are plain fields on the same struct — the same actor would otherwise set both the switch and its own bound in one request.
Two smaller correctness points
UpdateGlobalConfigAtomicclosure, so it compares against the same advisory-locked row the write lands on. Checking beforehand would race a concurrent writer between the check and the write.fmt.Errorf("failed to save config: %w", …), which would have blamed the store for an authorization decision. (IsClientErroruseserrors.As, so the status survived wrapping — but the message did not.)Tests — four quadrants × both endpoints
Every case in
TestRIExchangeAutoModeGateruns against both handlers as subtests, and each asserts the error and whether the write reached the store — asserting only the error would pass against a handler that returned one after already saving.A refusal-only suite passes just as well against a gate that refuses every config write, which would silently strip the "configures but does not execute" operator role this fix is careful to preserve. Hence the three "still allowed" quadrants, plus
_UnrelatedConfigWriteUnaffectedand_ArmedIdempotentRewriteAllowedas scope controls.Mutation-tested — six mutations, all killed by named tests
PUT /api/configonly (the "looks complete" fix)generic PUT /api/configsubtests + all 5NonManualModeIsArmedcasesPUT /api/ri-exchange/configonlydedicatedsubtestsMode == "auto"NonManualModeIsArmedcasesupdate:configinstead ofexecute:ri-exchangeNonManualModeIsArmedM1 and M2 are the pair that matters: each kills only on its own endpoint's subtests, which is the evidence that the two-route coverage is real rather than incidental.
Out of scope, flagged not fixed
frontend/src/riexchange.ts'srenderAutomationSettingsrenders the "Enable Automated Exchange" toggle and Mode select with zerocanAccessgating; the only friction is a client-side confirm dialog, which is UX and not authorization. Backend enforcement is what matters and is what this PR delivers — a non-RI-Exchanger will now get a 403 rather than a silent success. Disabling those controls for non-RI-Exchanger users is a follow-up;isRIExchanger()(added in sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group #1758's F2 round,frontend/src/permissions.ts) is the helper for it.auth.PermissionConstraints, so it has none of the account/provider/region/amount scopingrequirePermissionConstraintsenforces on the direct handler. That is a separate categorical gap noted in sec(exchange): scheduled auto-exchange bypasses the execute:ri-exchange carve-out via update:config #1765's analysis; this PR closes the arming route, not that.Known coverage residual
The matrix drives both routes with an
admin:*principal, which is the #1765 threat model and what/api/config'sAuthAdmingate requires. One shape is therefore not covered by any quadrant: a plain non-admin session holding onlyupdate:configagainst the dedicatedAuthUserroute. The API-key tests do exercise a non-admin credential on that route, so the shape is not wholly untested. Stated here rather than papered over; the matrix is deliberately not expanded for it.Gates
gofmt -l .go build ./.../go vet ./...gocyclo -over 10 -ignore "_test\.go" .go test -race -count=1 ./...(root)golangci-lintv2.10.1, root module,commset diff vsorigin/main0 issues., head exit 0 /0 issues.— zero new, zero removedThe lint gate caught three findings on the first pass and they were fixed before commit: a
govet shadowfrom hoistingerrto function scope inupdateConfig(the same class currently blocking #1770), anerrcheckon a discarded*clientError, and amisspell.Summary by CodeRabbit