From a47ce51d4c9c3389939c4dafbfeb5e5481f05276 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:03:31 -0300 Subject: [PATCH] fix(api): classify /api/acp/agents as loopback-only (#7948) (#7966) * fix(api): classify /api/acp/agents as loopback-only (#7948) /api/acp/agents is spawn-capable (POST registers a client-chosen `binary` via src/lib/acp/registry.ts; GET / POST {action:"refresh"} runs detectInstalledAgents() -> execFileSync(probe.command, probe.args, { shell }) transitively) but was never added to LOCAL_ONLY_API_PREFIXES in src/server/authz/routeGuard.ts, unlike every sibling spawn-capable route (Hard Rules #15/#17). A leaked JWT via tunnel could reach the version-probe execFileSync path. Adds the prefix to the loopback gate plus a direct regression test (tests/unit/route-guard-acp-agents-local-only.test.ts) and an assertion in tests/unit/check-route-guard-membership.test.ts documenting why the existing source-scan subcheck cannot catch this class of gap (the spawn call is transitive via registry.ts, not in the route file itself). * chore(quality): register route-guard-acp-agents-local-only test in stryker tap.testFiles --- .../fixes/7948-acp-agents-loopback-gate.md | 1 + src/server/authz/routeGuard.ts | 1 + stryker.conf.json | 29 ++++++++++------- .../unit/check-route-guard-membership.test.ts | 8 +++++ .../route-guard-acp-agents-local-only.test.ts | 31 +++++++++++++++++++ 5 files changed, 59 insertions(+), 11 deletions(-) create mode 100644 changelog.d/fixes/7948-acp-agents-loopback-gate.md create mode 100644 tests/unit/route-guard-acp-agents-local-only.test.ts diff --git a/changelog.d/fixes/7948-acp-agents-loopback-gate.md b/changelog.d/fixes/7948-acp-agents-loopback-gate.md new file mode 100644 index 0000000000..5473138a2b --- /dev/null +++ b/changelog.d/fixes/7948-acp-agents-loopback-gate.md @@ -0,0 +1 @@ +- fix(api): classify `/api/acp/agents` as loopback-only to close an RCE-via-tunnel gap in the ACP custom-agent registry (#7948) diff --git a/src/server/authz/routeGuard.ts b/src/server/authz/routeGuard.ts index 54b78d1289..a8166a52ef 100644 --- a/src/server/authz/routeGuard.ts +++ b/src/server/authz/routeGuard.ts @@ -52,6 +52,7 @@ export const LOCAL_ONLY_API_PREFIXES: ReadonlyArray = [ "/api/oauth/cursor/auto-import", // spawns `execFile("which", ["cursor"])` to verify a local Cursor install before importing creds — RCE-via-tunnel surface (Hard Rules #15 + #17, found by 6A.8 route-guard gate). Specific path only: the rest of /api/oauth/ (browser redirect/callback flows) must stay remote-reachable. "/api/skills/collect/", // Skill Collector CLI detection: GET .../detect probes getCliRuntimeStatus() per CLI_TOOL_IDS entry, which spawns a child process to check each tool — RCE-via-tunnel surface (Hard Rules #15 + #17, PR #6294 review). "/api/discovery/", // Discovery tool (opt-in provider scanner): the scan route makes outbound probes to provider endpoints (SSRF-adjacent) and the whole surface is an admin research tool — strict-loopback only, no manage-scope bypass (NOT in LOCAL_ONLY_MANAGE_SCOPE_BYPASS_PREFIXES). See _tasks/features-v3.8.42/gaps/DISCOVERY_TOOL_DESIGN.md. + "/api/acp/agents", // ACP custom-agent registry: POST registers a client-chosen `binary`; GET / POST {action:"refresh"} runs detectInstalledAgents() -> execFileSync(probe.command, probe.args, { shell }) transitively (src/lib/acp/registry.ts) — RCE-via-tunnel surface (Hard Rules #15 + #17, #7948) ]; /** diff --git a/stryker.conf.json b/stryker.conf.json index db9709543b..e93b99e852 100644 --- a/stryker.conf.json +++ b/stryker.conf.json @@ -39,7 +39,9 @@ "incremental": true, "incrementalFile": "reports/mutation/stryker-incremental.json", "testRunner": "tap", - "plugins": ["@stryker-mutator/tap-runner"], + "plugins": [ + "@stryker-mutator/tap-runner" + ], "tap": { "testFiles": [ "tests/unit/account-fallback-anthropic-quota.test.ts", @@ -103,20 +105,17 @@ "tests/unit/circuit-breaker-failure-kind.test.ts", "tests/unit/circuit-breaker-registry-cap.test.ts", "tests/unit/circuit-breaker-stream-controller-4602.test.ts", - "tests/unit/cliproxyapi-dedicated-credential-7645.test.ts", - "tests/unit/combo-least-used-account.test.ts", - "tests/unit/combo/recovery-hint.test.ts", - "tests/unit/ddg-circuit-breaker-null-content-6999-7000.test.ts", "tests/unit/claude-code-parity.test.ts", "tests/unit/claude-effort-suffix-strip.test.ts", "tests/unit/claude-oauth-provider.test.ts", "tests/unit/claude-passthrough-stream-boolean.test.ts", "tests/unit/claude-passthrough-thinking-2454.test.ts", "tests/unit/cli-simulate.test.ts", - "tests/unit/cliproxyapi-model-mapping-dispatch.test.ts", "tests/unit/cline-response-envelope.test.ts", - "tests/unit/cliproxyapi-model-mapping-dispatch.test.ts", "tests/unit/clinepass-provider.test.ts", + "tests/unit/cliproxyapi-dedicated-credential-7645.test.ts", + "tests/unit/cliproxyapi-model-mapping-dispatch.test.ts", + "tests/unit/cliproxyapi-model-mapping-dispatch.test.ts", "tests/unit/cliproxyapi-model-mapping-dispatch.test.ts", "tests/unit/codex-failover.test.ts", "tests/unit/codex-quota-selection-hydration.test.ts", @@ -135,6 +134,7 @@ "tests/unit/combo-health-dashboard.test.ts", "tests/unit/combo-health-route.test.ts", "tests/unit/combo-hedging.test.ts", + "tests/unit/combo-least-used-account.test.ts", "tests/unit/combo-lockout-quota-reset-6863.test.ts", "tests/unit/combo-max-depth-config.test.ts", "tests/unit/combo-model-lockout-honors-reset-1308.test.ts", @@ -168,6 +168,7 @@ "tests/unit/combo/auto-status-penalty-4540.test.ts", "tests/unit/combo/combo-exhausted-skip.test.ts", "tests/unit/combo/effective-max-concurrency.test.ts", + "tests/unit/combo/recovery-hint.test.ts", "tests/unit/complexity-aware-scoring-wiring.test.ts", "tests/unit/context-pinning-tool-calls.test.ts", "tests/unit/cooldown-epoch-string-3954.test.ts", @@ -175,6 +176,7 @@ "tests/unit/correctness/sanitizers.property.test.ts", "tests/unit/custom-model-target-format.test.ts", "tests/unit/db-reset-module-state.test.ts", + "tests/unit/ddg-circuit-breaker-null-content-6999-7000.test.ts", "tests/unit/domain-persistence.test.ts", "tests/unit/edgetts-provider.test.ts", "tests/unit/embeddings-auth.test.ts", @@ -239,25 +241,26 @@ "tests/unit/responses-handler.test.ts", "tests/unit/rotation-config-omniroute.test.ts", "tests/unit/route-explainability.test.ts", + "tests/unit/route-guard-acp-agents-local-only.test.ts", "tests/unit/route-guard-forge-jcode-settings-local-only.test.ts", "tests/unit/route-guard-grok-build-settings-local-only.test.ts", - "tests/unit/route-guard-qwen-settings-local-only.test.ts", "tests/unit/route-guard-middleware-local-only.test.ts", "tests/unit/route-guard-plugins-local-only.test.ts", "tests/unit/route-guard-private-lan.test.ts", "tests/unit/route-guard-provider-login-local-only.test.ts", + "tests/unit/route-guard-qwen-settings-local-only.test.ts", "tests/unit/router-strategies.test.ts", "tests/unit/rule12-error-sanitization-sweep.test.ts", "tests/unit/serial/combo-health-autopilot.test.ts", "tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts", "tests/unit/serial/combo-strategy-fallbacks-half-open-timing.test.ts", "tests/unit/serial/provider-health-autopilot.test.ts", - "tests/unit/service-provider-plugin-registry.test.ts", - "tests/unit/session-affinity-generic-7274.test.ts", "tests/unit/service-combo-metrics.test.ts", + "tests/unit/service-provider-plugin-registry.test.ts", "tests/unit/services-branch-hardening.test.ts", "tests/unit/services/bifrost-route-guard.test.ts", "tests/unit/services/combo-metrics-memory.test.ts", + "tests/unit/session-affinity-generic-7274.test.ts", "tests/unit/settings/authz-bypass.test.ts", "tests/unit/skip-provider-breaker-consumer-2743.test.ts", "tests/unit/sse-auth.test.ts", @@ -385,7 +388,11 @@ ".worktrees", ".stryker-tmp" ], - "reporters": ["progress", "html", "json"], + "reporters": [ + "progress", + "html", + "json" + ], "htmlReporter": { "fileName": "reports/mutation/mutation.html" }, diff --git a/tests/unit/check-route-guard-membership.test.ts b/tests/unit/check-route-guard-membership.test.ts index 2d6bafb6da..146772528b 100644 --- a/tests/unit/check-route-guard-membership.test.ts +++ b/tests/unit/check-route-guard-membership.test.ts @@ -145,6 +145,14 @@ test("6A.8 P1 RESOLVED: spawn-capable system/db-backups routes are classified lo assert.equal(isLocalOnlyPath("/api/db-backups/exportAll"), true); }); +test("#7948: /api/acp/agents (transitive execFileSync via registry) is classified local-only", () => { + // /api/acp/agents is spawn-capable only transitively (route.ts imports + // src/lib/acp/registry.ts, which calls execFileSync) — the source-scan + // subcheck above only greps the route file itself, so it cannot catch this + // class of gap. This assertion is the direct regression guard for #7948. + assert.equal(isLocalOnlyPath("/api/acp/agents"), true); +}); + test("6A.8: spawn-capable routes in SPAWN_CAPABLE_ROUTE_ROOTS are still all classified local-only", async () => { // The original subcheck (SPAWN_CAPABLE_ROUTE_ROOTS) must still pass. // This test is a regression guard — the new source-scan does not break the old check. diff --git a/tests/unit/route-guard-acp-agents-local-only.test.ts b/tests/unit/route-guard-acp-agents-local-only.test.ts new file mode 100644 index 0000000000..5f19722e14 --- /dev/null +++ b/tests/unit/route-guard-acp-agents-local-only.test.ts @@ -0,0 +1,31 @@ +/** + * Security regression (#7948): /api/acp/agents must be classified as LOCAL_ONLY + * so loopback enforcement runs unconditionally before any auth check. + * + * POST registers a CustomAgentDef with a client-chosen `binary` + * (src/lib/acp/registry.ts:40-48); `resolveVersionProbe()` only checks the + * tokenized `versionCommand`'s command matches the supplied `binary` — no + * known-safe allowlist. A later GET (or POST {action:"refresh"}) runs + * `detectInstalledAgents()` -> `detectAgent()` -> `execFileSync(probe.command, + * probe.args, { shell: shouldUseShellForVersionProbe(probe.command) })` + * (registry.ts:320-325), with `shell:true` on Windows for `.cmd`/`.bat`/ + * extensionless commands — the same spawn-capable class already gated for + * every sibling route (Hard Rules #15 + #17). Classifying it LOCAL_ONLY closes + * the remote-RCE vector: a leaked JWT over a Cloudflared/Ngrok tunnel cannot + * trigger process spawning. See docs/security/ROUTE_GUARD_TIERS.md. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import { isLocalOnlyPath } from "../../src/server/authz/routeGuard.ts"; + +test("#7948: /api/acp/agents is LOCAL_ONLY (spawns via registry execFileSync)", () => { + assert.equal(isLocalOnlyPath("/api/acp/agents"), true); +}); + +test("#7948: /api/acp/agents with trailing slash is LOCAL_ONLY", () => { + assert.equal(isLocalOnlyPath("/api/acp/agents/"), true); +}); + +test("#7948: nested acp agent paths stay LOCAL_ONLY under the same prefix", () => { + assert.equal(isLocalOnlyPath("/api/acp/agents/sub"), true); +});