Skip to content

fix(authz): gate the devin CLI-spawn fallback on peer locality (#15159 S-01) - #15766

Merged
diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.52from
jonlwheat2-gif:fix/s01-devin-spawn-local-only
Oct 8, 2026
Merged

diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.52from
jonlwheat2-gif:fix/s01-devin-spawn-local-only

Conversation

@jonlwheat2-gif

Copy link
Copy Markdown
Contributor

Closes the residual S-01 gap. Separate from #15242, which fixed the two spawn hops the audit reported — one item per PR, per the owner's guidance.

The gap

validateDevinCloudAgentProvider falls back to validateDevinCliKeyFallback() on an upstream 401/403, and that fallback is:

spawn(bin, ["acp", "--agent-type", "summarizer"], …)   // webProvidersB.ts:535

It is reached from three routes, none of which is in LOCAL_ONLY_API_PATTERNS:

Route Path to the spawn
POST /api/providers/{id}/test testSingleConnection() → testApiKeyConnection()
POST /api/providers testSingleConnection() (fire-and-forget auto-test, route.ts:412)
POST /api/providers/test-batch testSingleConnection() (test-batch/route.ts:173)

All three are remote-reachable management routes, so a leaked JWT via tunnel could still trigger process spawning — what Hard Rules #15/#17 forbid. PR #15242 gated /validate, /import, /bulk, /{id}/login and /{id}/refresh-cursor but not these.

The binary and argv are fixed (CLI_DEVIN_BIN or "devin"), so this is not caller-controlled RCE — but the rule is about the spawn, not the argv. The owner agreed it should be fixed on exactly this reasoning.

Why not a path pattern

Adding /api/providers/{id}/test to LOCAL_ONLY_API_PATTERNS would lock remote dashboards out of testing connections entirely. That is the same over-broadening the /login precedent in routeGuard.ts exists to avoid — and the second S-01 hop already took the call-site-gate route for this reason (models/route.ts:1452). This PR reuses the shape already established for allowLocalRuntimeProbe in these same three routes.

A regression test asserts the route is not blanket-locked, so this cannot later be "fixed" the wrong way.

The fix

allowLocalSpawn threads from the route (derived from getRequestPeerLocality(request) !== "remote") through validateProviderApiKey to the devin validator. When false, the 401/403 branch returns the HTTP verdict without the CLI re-probe.

Two details worth naming:

  • The dispatcher entry became a wrapper. devin: validateDevinCloudAgentProvider as a bare reference would silently drop the flag and re-open the spawn.
  • The default stays permissive. Direct callers (credential-health scheduler, VNC harvest) are internal, and VNC is already loopback-gated. A non-loopback caller still gets the HTTP verdict unchanged — a rejected key is still invalid, just not re-probed on the host on its behalf.

TDD

tests/unit/devin-cli-fallback-spawn-local-only-15159.test.ts, written first. It detects a spawn with a marker file written by the child process — no module mocking, so a read-time-only check could not pass. The fake CLI is node itself: deliberately cross-platform, because a #!/bin/sh fixture cannot be spawned on Windows and a .cmd one needs shell:true.

Watched fail 2/5 first (the two open behaviours), then pass 5/5. The two that passed at RED were the "already correct" guards, so they were not vacuous. A SANITY case proves the fixture does spawn when allowed — without it, the negative assertion would prove nothing.

One existing test inverted

tests/unit/provider-test-local-runtime-probe.test.ts:102 pinned the exact old call shape testSingleConnection(conn.id, undefined, { allowLocalRuntimeProbe }). This change grows that call, so the assertion was updated rather than worked around, with the reasoning left inline. It now pins both flags — restoring the old regex would re-open the spawn.

Verification

  • New suite 5/5; related suites 165/165 (provider-test-local-runtime-probe, route-guard-providers-spawn-local-only, cursor-models-route-spawn-gate-15159, provider-validation-specialty, provider-validation-branches)
  • check:error-helper OK (1216 files, 21 frozen) · check-route-guard-membership OK, 0 new gaps
  • check:any-budget:t11 PASS · ESLint clean as CI runs it · all pre-commit hooks green

Base-red inherited

  • typecheck:core fails on open-sse/services/apiKeyRotator.ts:511 (TS2484 KeyHealth). Confirmed present on the base commit a2ed070433 via git show origin/release/v3.8.52:… — not caused by these files; it is the only error reported.
  • tests/unit/devin-cloud-agent-validator-6142.test.ts has one failing test ("a CLI-format key … validates via the real CLI probe"). Its #!/bin/sh fixture cannot execute on Windows. This predates this change and is left as-is — fixing it is a separate concern from S-01, and was verified failing on the base.

…er-inference#15159 S-01)

PR cheaper-inference#15242 gated the two spawn hops this audit reported, but left a third
route on the same call chain ungated.

validateDevinCloudAgentProvider falls back to
validateDevinCliKeyFallback() on an upstream 401/403, and that fallback is
`spawn(bin, ["acp", "--agent-type", "summarizer"])`. It is reached from:

  POST /api/providers/{id}/test   -> testSingleConnection() -> testApiKeyConnection()
  POST /api/providers             -> testSingleConnection() (fire-and-forget auto-test)
  POST /api/providers/test-batch  -> testSingleConnection()

None of the three is in LOCAL_ONLY_API_PATTERNS, and all three are
remote-reachable management routes. A leaked JWT via tunnel could therefore
still trigger process spawning, which Hard Rules #15/#17 forbid. The binary
and argv are fixed (CLI_DEVIN_BIN or "devin"), so this is not
caller-controlled RCE — but the rule is about the spawn, not the argv.

The fix threads `allowLocalSpawn` from the route down through
validateProviderApiKey to the devin validator, derived from the trusted
peer-locality header. It reuses the shape already established for
`allowLocalRuntimeProbe` in these same three routes rather than adding a
path pattern: blanket-locking /api/providers/{id}/test would break remote
dashboards that legitimately test connections, which is the exact
over-broadening the /login precedent in routeGuard.ts exists to avoid.

The dispatcher entry is now a wrapper rather than a bare reference — a bare
`devin: validateDevinCloudAgentProvider` would silently drop the flag and
re-open the spawn.

The default stays permissive because the direct callers (credential-health
scheduler, VNC harvest) are internal; VNC is already loopback-gated. A
non-loopback caller still gets the HTTP verdict unchanged — a rejected key is
still invalid, just not re-probed on the host on its behalf.

TDD: tests/unit/devin-cli-fallback-spawn-local-only-15159.test.ts, written
first. It detects a spawn with a marker file written by the child process —
no module mocking, so a read-time-only check could not pass. The fake CLI is
node itself, which is deliberately cross-platform: a `#!/bin/sh` fixture
cannot be spawned on Windows, and the repo's existing devin validator test
depends on a POSIX shell and cannot pass there. A SANITY case proves the
fixture does spawn when allowed, so the negative assertion is not vacuous.
Watched fail 2/5 first, then pass 5/5.

The pre-existing devin-cloud-agent-validator-6142 test was left failing: its
`#!/bin/sh` fixture is Windows-incompatible, which predates this change.

Base-red inherited: the typecheck:core failure in
open-sse/services/apiKeyRotator.ts:511 (TS2484 KeyHealth) is present on the
base commit and unrelated to these files.
…per-inference#15766)

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Added a changelog.d fragment so merge-integrity can pass (870c518ea8). The other reds on this run are inherited from release/v3.8.52 (#15306 docs-count + the pre-#15769 API typecheck). Not merging the red base into this branch.

@diegosouzapw
diegosouzapw merged commit ec90315 into cheaper-inference:release/v3.8.52 Oct 8, 2026
29 of 51 checks passed
woodsonl added a commit to woodsonl/OmniRoute that referenced this pull request Oct 11, 2026
…oute tier

A reverse-proxied dashboard (docker + nginx) got 403 LOCAL_ONLY on
/api/providers/validate for every provider key — rendered by the Add API
Key modal as a bare 'Invalid' badge with no reason, because the modal only
rendered string-shaped error bodies. cheaper-inference#15242 added the blanket pattern for
the devin CLI-spawn fallback; cheaper-inference#15766 already established the precise fix
(peer-locality gate at the spawn call site) but only wired the
test/create/test-batch routes. Drop the blunt pattern, thread allowLocalSpawn
into validate/bulk/import (cheaper-inference#15766 shape), and surface {code,message} error
envelopes in the modals via validationErrorMessage().
fouadSalkini pushed a commit to fouadSalkini/OmniRoute that referenced this pull request Oct 19, 2026
…er-inference#15159 S-01) (cheaper-inference#15766)

Validated in a combined board with the other PRs of this wave on release/v3.8.52: the PR's own tests pass; typecheck:core, open-sse typecheck, api-typecheck, complexity, changelog-integrity, env-doc-sync, test-discovery, vitest-exclusions, error-helper, test-masking, lockfile, deps, gate-manifest, cli-i18n and i18n coverage green. Any file-size ceiling or stryker registration this wave needs lands in the wave's follow-up PR. Thank you for the contribution!

(cherry picked from commit ec90315)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants