From bf3abdee81186f87c0d65119e0754ceede30cf0f Mon Sep 17 00:00:00 2001 From: jinye Date: Fri, 7 Aug 2026 15:33:16 +0800 Subject: [PATCH] fix(serve): Allow approved same-host text reads outside workspace (#8620) * fix(serve): allow same-host daemon text reads Co-authored-by: Qwen-Coder * docs(serve): address review on same-host text reads Record what the read capability does not fix: #8618 still reproduces for the write and edit family, whose delegated writes are refused after the user has already approved the diff. Give the daemon's pre-approval SSE fan-out its own bullet in the user-facing security section, restore the sentence stating that environment isolation is not an OS security boundary, and make the design doc the single owner of the tradeoff list so tuning a limit cannot leave stale copies behind. Test fixtures no longer land in the developer's real home directory, the assertion pinned to localized rejection copy is dropped, and the combined capability case is split so deleting the write half cannot silently remove read coverage. * fix(test): declare REPO_ROOT and bind the external-read session to the daemon's workspace The external-read regression test referenced REPO_ROOT twice without declaring it, which made it unrunnable everywhere: - On a developer box the ReferenceError was swallowed by the bare catch in findExternalReadBase(), every candidate was discarded, and the test reported a green skip -- exactly the silently-disabled security test the CI loud-fail added last round was meant to prevent. The guard was defeated three lines above itself. - On CI that loud-fail branch threw at module scope, so the file failed to collect and took the four pre-existing tests down with it. Declare REPO_ROOT the way every other daemon integration test does. The session also asked for `workspaceCwd: REPO_ROOT` while beforeAll binds the daemon with `--workspace workspaceDir`, so the create returned 400 Workspace mismatch even once the constant existed. The read under test is external because externalReadDir sits outside the bound workspace, not because the session claims a wider one. Finally, collect each candidate's rejection reason instead of dropping it, and fold it into both branches: the CI throw names why every candidate failed and the developer-box skip warns with the same text. A bare catch cannot tell "no /var/tmp on this image" from a bug in the function, and the second reads as a green skip. Reported by @wenshao, who reproduced all three consequences against a real qwen serve daemon on Linux and supplied the repair. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Qwen-Coder Co-authored-by: Claude Opus 5 --- docs/design/daemon-local-text-reads.md | 88 ++++++ .../serve-large-text-range-consistency.md | 9 +- docs/developers/daemon/03-acp-bridge.md | 3 +- .../daemon/07-workspace-filesystem.md | 15 +- docs/users/qwen-serve.md | 1 + .../cli/qwen-serve-streaming.test.ts | 265 +++++++++++++++++- packages/acp-bridge/README.md | 10 +- .../src/bridge-file-capabilities.test.ts | 47 ++++ packages/acp-bridge/src/bridge.ts | 10 +- packages/acp-bridge/src/bridgeOptions.ts | 28 +- .../service/filesystem.test.ts | 49 +++- .../src/acp-integration/service/filesystem.ts | 4 + .../src/serve/bridge-file-system-adapter.ts | 13 +- packages/cli/src/serve/run-qwen-serve.test.ts | 10 + packages/cli/src/serve/run-qwen-serve.ts | 3 + .../server-default-bridge-wiring.test.ts | 5 + packages/cli/src/serve/server.ts | 5 +- 17 files changed, 533 insertions(+), 32 deletions(-) create mode 100644 docs/design/daemon-local-text-reads.md create mode 100644 packages/acp-bridge/src/bridge-file-capabilities.test.ts diff --git a/docs/design/daemon-local-text-reads.md b/docs/design/daemon-local-text-reads.md new file mode 100644 index 0000000000..4ce78edf45 --- /dev/null +++ b/docs/design/daemon-local-text-reads.md @@ -0,0 +1,88 @@ +# Daemon local text reads + +## Decision + +`BridgeOptions.delegateReadTextFileToClient` defaults to `true`, preserving +generic ACP, IDE, remote, and virtual-filesystem behavior. Same-host `qwen +serve` runtimes set it to `false`, so the ACP initialize capability is +`{ readTextFile: false, writeTextFile: true }` and the child uses its regular +CLI filesystem service for text reads. Caller-injected bridges remain under +the caller's control. + +## Behavior + +Direct external text `read_file` calls use the normal CLI permission flow: +their default is `ask`, approval allows the read, and rejection prevents tool +execution. Allow rules and automatic approval modes behave as in the CLI. +Non-text `read_file` paths were already read locally by the child and are +unchanged. + +Because the capability applies to `FileSystemService.readTextFile`, shared +text pre-reads used by write, edit, notebook, sed, and artifact operations also +move to the regular CLI filesystem service. This intentionally accepts the +CLI's read-side limits and behavior instead of WFS's 256 KiB returned-output +and full-snapshot cap, 8 MiB large-text scan cap, read audit, symlink rejection, +and read-side TOCTOU protections. Direct `read_file` still applies the core line +and output limits, subject to their existing configuration. + +This document is the single owner of that tradeoff list. Other documents +reference it rather than restating the limits, so tuning one of them does not +leave stale copies behind. + +### What this does not fix + +Reads become child-local; final ACP text writes stay delegated. The reported +failure in #8618 therefore still reproduces for the `write_file`, `replace`, +and `notebook_edit` family, only later in the sequence: the pre-read now +succeeds locally, the diff renders, the user approves, and the delegated write +is then refused by the workspace filesystem because the target is outside the +workspace. The model can still fall back to shell at that point. Moving writes +child-local as well would give up the trust gate, symlink rejection, TOCTOU +protection, atomic temp-and-rename with mode preservation, and the write audit, +which is a materially larger concession than the read change; it is deliberately +out of scope here and tracked separately. + +### Pre-approval exposure in the daemon + +A confirmation payload is built by reading the file, so an edit or write +confirmation for an out-of-workspace path now carries that file's content in +its diff. The daemon fans that payload out to every attached SSE subscriber +before the approval decision exists. In the interactive CLI the same diff is +seen only by the person at the terminal. This follows from treating +authenticated daemon clients as one security principal, and is called out here +because that framing is easy to read past. + +HTTP filesystem routes such as `/glob` and `/list` remain workspace-scoped. +Agent `glob`, `ls`, `grep`, and other discovery-tool behavior is unchanged by +this capability. Final ACP `writeTextFile` content writes stay delegated through +`WorkspaceFileSystem`, retaining workspace, trust, symlink, atomic-write, and +audit enforcement. This does not imply that every agent write or helper +operation goes through WFS. + +## Resource and audit boundaries + +A child-local text read does not emit WFS `fs.access`; direct external +`read_file` retains its permission audit and core file-operation telemetry. +Same-host reads run under the daemon user's OS identity. `qwen serve` assumes +one machine, one UID, and one security principal; it is not an OS sandbox. + +## Compatibility + +Only the default embedded daemon bridge and primary, static-secondary, and +dynamic `qwen serve` workspace runtimes disable read delegation. The WFS +adapter keeps its read implementation so an unexpected or +capability-violating delegated read still reaches the workspace boundary and +fails closed for external paths. + +That "fails closed" is bounded, not absolute. `AcpFileSystemService` has a +second, pre-existing bypass: when a delegated read is refused with +`path_outside_workspace` or `symlink_escape`, it retries the read locally if +the path's realpath sits under one of its managed read roots. Those roots +include `/tmp` unconditionally on POSIX, plus anything named by +`QWEN_ACP_LOCAL_READ_ROOTS`. So the boundary is fail-closed only for paths +outside those roots. The daemon neutralizes the env-supplied half by setting +`QWEN_ACP_LOCAL_READ_ROOTS` empty for the child. + +With the capability off, that retry path is unreachable in the daemon anyway — +the capability check returns before the delegated call is attempted — so it +now guards only generic ACP hosts that keep delegation enabled. diff --git a/docs/design/serve-large-text-range-consistency.md b/docs/design/serve-large-text-range-consistency.md index 0d2323d5c0..325852c594 100644 --- a/docs/design/serve-large-text-range-consistency.md +++ b/docs/design/serve-large-text-range-consistency.md @@ -82,7 +82,14 @@ reaching this boundary: - `GET /file` - ACP HTTP `_qwen/file/read` -- the injected ACP `readTextFile` adapter + +The injected ACP `readTextFile` adapter is no longer a production consumer: +same-host daemon runtimes advertise `readTextFile: false`, so agent text reads +are served by the child's regular CLI filesystem service and never reach this +boundary. The adapter's read path is kept as a fail-closed guard for an +unexpected or capability-violating delegated read, but the concurrent-append, +truncation, and symlink-replacement guarantees verified below no longer apply +to any agent read. See [daemon local text reads](./daemon-local-text-reads.md). Windowless reads used by workspace setup retain the existing 256 KiB full-snapshot refusal. diff --git a/docs/developers/daemon/03-acp-bridge.md b/docs/developers/daemon/03-acp-bridge.md index b33decd574..ea2270fa58 100644 --- a/docs/developers/daemon/03-acp-bridge.md +++ b/docs/developers/daemon/03-acp-bridge.md @@ -15,7 +15,7 @@ Each active `WorkspaceRuntime` owns one `HttpAcpBridge` instance. Production att - Per-session FIFO for `setSessionModel` calls so concurrent attaches with different models do not race the agent. - Per-session `EventBus` that drives `GET /session/:id/events` (see [`10-event-bus.md`](./10-event-bus.md)). - Permission flow: `BridgeClient.requestPermission` → `MultiClientPermissionMediator.request` → fan-out → vote collection → ACP response (see [`04-permission-mediation.md`](./04-permission-mediation.md)). -- File I/O: `BridgeFileSystem` adapter for ACP `readTextFile` / `writeTextFile` calls (see [`07-workspace-filesystem.md`](./07-workspace-filesystem.md)). +- File I/O: `BridgeFileSystem` adapter for ACP reads and writes; same-host daemon runtimes advertise `readTextFile: false` so normal text reads stay in the child while final text writes remain delegated (see [`07-workspace-filesystem.md`](./07-workspace-filesystem.md)). - extMethod RPCs for workspace-level status (`/workspace/mcp`, `/workspace/skills`, `/workspace/providers`), MCP restart, and the optional private managed Tool Guard callback. - Lifecycle: graceful `shutdown()` with `KILL_HARD_DEADLINE_MS` (10s) per channel; synchronous `killAllSync()` for second-signal force-exit. @@ -209,6 +209,7 @@ sequenceDiagram | `persistApprovalMode`, `persistDisabledTools` | — | Settings-write hooks for the Wave 4 mutation routes. | | `contextFilename` | from `settings.json`'s `context.fileName` | Overrides `getCurrentGeminiMdFilename`. | | `statusProvider` | (none) | Daemon-host preflight cells (`DaemonStatusProvider`). | +| `delegateReadTextFileToClient` | `true` | Set `false` only for same-host runtimes so every child `FileSystemService.readTextFile` consumer uses the regular CLI filesystem service. | | `fileSystem` | (none) | `BridgeFileSystem` adapter for ACP `readTextFile` / `writeTextFile`. | | `permissionPolicy` | from `settings.json`'s `policy.permissionStrategy` | One of `first-responder` / `designated` / `consensus` / `local-only`. | | `permissionConsensusQuorum` | from `settings.json` | N for consensus policy. | diff --git a/docs/developers/daemon/07-workspace-filesystem.md b/docs/developers/daemon/07-workspace-filesystem.md index a0f6f3a270..dadefb216f 100644 --- a/docs/developers/daemon/07-workspace-filesystem.md +++ b/docs/developers/daemon/07-workspace-filesystem.md @@ -2,7 +2,7 @@ ## Overview -The daemon never lets HTTP routes or ACP-side agent calls touch the host filesystem directly. Every read, write, list, glob, and stat goes through the `WorkspaceFileSystem` boundary (`packages/cli/src/serve/fs/`), which provides: +Daemon HTTP file routes and delegated ACP `readTextFile` / `writeTextFile` calls go through the `WorkspaceFileSystem` boundary (`packages/cli/src/serve/fs/`), which provides: - **Path resolution** — canonicalize paths and reject anything escaping the bound workspace, including via symlinks. - **Trust gating** — refuse writes when the workspace is not trusted (`untrusted_workspace`). @@ -11,7 +11,16 @@ The daemon never lets HTTP routes or ACP-side agent calls touch the host filesys - **Audit** — every access / denial emits a structured event for `PermissionAuditRing` / monitoring. - **Typed errors** — closed `FsErrorKind` union mapped to HTTP statuses. -The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST /file/edit`, `GET /list`, `GET /glob`, `GET /stat`) and the ACP-side `BridgeFileSystem` adapter (so agent-driven `readTextFile` / `writeTextFile` calls get the same gates) both go through this boundary. +The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST /file/edit`, `GET /list`, `GET /glob`, `GET /stat`) use this boundary. In the production daemon, ACP calls that remain delegated reach WFS through the injected bridge adapter; generic bridge callers use WFS only when they inject such an adapter. Production same-host `qwen serve` runtimes advertise `readTextFile: false`, so all child `FileSystemService.readTextFile` consumers use the regular CLI filesystem service; final ACP `writeTextFile` content writes remain delegated through WFS. + +That text-read capability slice covers direct `read_file` plus the shared pre-reads used by write, edit, notebook, sed, and artifact operations: + +- It intentionally accepts regular CLI read behavior rather than the WFS read-side guarantees. [The design doc](../../design/daemon-local-text-reads.md) owns the exact list of what is given up. +- The same doc records why #8618 still reproduces for the write and edit family even after this change, and the bounded sense in which the retained adapter read path "fails closed". +- Direct external `read_file` keeps the normal CLI permission rules and core file-operation telemetry. +- HTTP filesystem routes remain workspace-scoped, and agent discovery-tool behavior is unchanged by this capability. +- Auxiliary actions such as parent-directory creation and shell commands are separate existing paths, not covered by this boundary. +- `qwen serve` assumes a same-machine, same-UID security principal and is not an OS sandbox. ## Responsibilities @@ -66,7 +75,7 @@ interface BridgeFileSystem { } ``` -This is the injection point for ACP `readTextFile` / `writeTextFile`. Bridge tests and Mode A embedded callers can omit it on `BridgeOptions`; `BridgeClient` falls back to its inline `fs.readFile` / `fs.writeFile` proxy (preserves pre-F1 behavior). Production `qwen serve` wires `BridgeFileSystem` through `createBridgeFileSystemAdapter(fsFactory)` (`packages/cli/src/serve/bridge-file-system-adapter.ts`) so agent-side ACP writes pick up the same TOCTOU, symlink, trust-gate, and audit gates the HTTP routes use. +This is the injection point for ACP `readTextFile` / `writeTextFile`. Bridge tests and Mode A embedded callers can omit it on `BridgeOptions`; `BridgeClient` falls back to its inline `fs.readFile` / `fs.writeFile` proxy (preserves pre-F1 behavior). Production `qwen serve` wires `BridgeFileSystem` through `createBridgeFileSystemAdapter(fsFactory)` (`packages/cli/src/serve/bridge-file-system-adapter.ts`) and sets `delegateReadTextFileToClient: false`. Capability-compliant children therefore read text locally and delegate final ACP text writes. The adapter retains its read implementation so unexpected or capability-violating delegated reads still encounter WFS's workspace boundary. Two defensive properties the adapter MUST preserve (because the inline proxy is fully bypassed when the adapter is injected): diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 54bd53ff23..d45b5de96a 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -557,6 +557,7 @@ provider decision with their normal tool policy and isolation boundary. - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin `** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` and `/demo` remain pre-auth on loopback by default) or a canonical URL origin (`://[:]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: `, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the `/demo` page) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. - **Chrome extension browser automation is separate from framing.** `qwen serve --allow-origin chrome-extension://` lets the extension frame the Web Shell and connect to the daemon. Console/network/screenshot/click tools require an external CDP MCP adapter command: `QWEN_CDP_MCP_COMMAND=/path/to/cdp-mcp-adapter qwen serve --allow-origin chrome-extension://`. The main CLI package does not bundle a browser automation adapter; clients can check `caps.features.includes('browser_automation_mcp')` before presenting those tools as available. - **A spawned `qwen --acp` child receives its owning runtime's effective environment.** The daemon freezes a process-env base, applies that workspace's settings/env-file overlay to a runtime-local snapshot, and never writes the overlay back to `process.env`; same-named keys in another runtime do not cross over. `QWEN_SERVER_TOKEN` is scrubbed before spawn because the agent does not need the daemon bearer. Base credentials such as `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, `QWEN_*`, and `DASHSCOPE_API_KEY` otherwise pass through unless the runtime overlay changes them. **This is intentional, not a sandbox.** The agent runs as the same UID with shell-tool access, so anything in `~/.bashrc`, `~/.aws/credentials`, or `~/.npmrc` is reachable by prompt injection regardless. Environment isolation between runtimes is not an operating-system security boundary; do not run `qwen serve` under an identity that has credentials you would not trust the agent with. +- **Agent text reads are child-local and follow the regular CLI permission rules, not the workspace filesystem boundary.** Direct `read_file` can reach host text paths outside every registered workspace: external paths default to confirmation, and allow rules or approval modes may approve them automatically. Approved reads use the configurable CLI output limits rather than the workspace filesystem's returned-output, full-snapshot, and large-text scan caps. This applies to every shared text-read consumer, so the pre-reads performed by write, edit, notebook, sed, and artifact operations lose those caps together with the workspace filesystem's read audit, symlink rejection, and read-side TOCTOU protections — see [the design doc](../design/daemon-local-text-reads.md) for the exact list. Because a confirmation payload is built by reading the file, an out-of-workspace diff is fanned out to **every** attached SSE subscriber before anyone approves it — in the interactive CLI that content is seen only by the person at the terminal. Treat authenticated daemon clients as the same security principal. HTTP filesystem routes remain workspace-scoped and still refuse these paths, agent discovery-tool behavior is unchanged, and final ACP `writeTextFile` content writes continue through the workspace filesystem. - **Per-subscriber bounded SSE queues** — a slow client that overflows its queue gets a `client_evicted` terminal frame and is closed; one stuck consumer can't pin the daemon. - **Per-session prompt admission cap** — defaults to 5 accepted-but-unsettled prompts per session. A buggy client cannot enqueue unbounded prompt promises or temporary SSE waits for one session. - **Graceful shutdown** — SIGINT/SIGTERM drain the agent children before closing the listener (10s deadline per child). diff --git a/integration-tests/cli/qwen-serve-streaming.test.ts b/integration-tests/cli/qwen-serve-streaming.test.ts index 4592b0e6f5..6e0c68a997 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -9,7 +9,7 @@ * * These tests fire real daemon prompts and observe the resulting SSE stream, * but the model side is backed by a local OpenAI-compatible fake server so - * the suite can run without API keys. They cover three flows that unit tests + * the suite can run without API keys. They cover five flows that unit tests * can't fully exercise: * * 1. Real `qwen --acp` child crash → daemon publishes `session_died`, @@ -24,14 +24,26 @@ * 4. An admitted prompt keeps running with no SSE subscriber while the Todo * Stop Guard performs its bounded continuations; a later subscriber * replays each discrete status event. + * 5. A same-host ACP child reads text outside the workspace only after the + * daemon permission request is approved, and never returns the content + * after rejection. * */ import { spawn, execSync, type ChildProcess } from 'node:child_process'; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { + accessSync, + constants, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { isPathWithinRoot } from '@qwen-code/qwen-code-core'; import { DaemonClient, parseSseStream } from '@qwen-code/sdk'; import type { DaemonEvent, DaemonSessionSummary } from '@qwen-code/sdk'; import { @@ -41,6 +53,7 @@ import { } from '../fake-openai-server.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(__dirname, '../..'); // Match the rest of the integration suite: prefer `TEST_CLI_PATH` // from `globalSetup.ts` (root `dist/cli.js` bundle), fall back to // the per-package output for direct vitest invocations. See the same @@ -71,14 +84,69 @@ const SKIP = ); const describePOSIX = SKIP ? describe.skip : describe; +// The base only has to sit outside both the workspace and the `/tmp` local-read +// root, so the test reads a genuinely external path. The real `$HOME` is +// excluded deliberately: cleanup lives in `afterAll`, so a Ctrl-C, `--bail`, or +// CI timeout leaks the fixture dir. `/var/tmp` leaks the same way — the leak is +// relocated somewhere harmless, not eliminated. +function findExternalReadBase(): string | undefined { + if (SKIP) return undefined; + const candidates = [ + // Escape hatch for images where /var/tmp is absent or read-only. + process.env['QWEN_TEST_EXTERNAL_READ_BASE'], + '/var/tmp', + ].filter((value): value is string => Boolean(value)); + // Carry each rejection reason into the diagnostics below. A bare `catch {}` + // here cannot tell "no /var/tmp on this image" (expected) from a bug in this + // function (not expected), and the latter reads as a green skip. + const rejections: string[] = []; + for (const candidate of candidates) { + try { + const resolved = realpathSync(candidate); + accessSync(resolved, constants.W_OK); + if ( + isPathWithinRoot(resolved, realpathSync('/tmp')) || + isPathWithinRoot(resolved, realpathSync(REPO_ROOT)) + ) { + rejections.push(`${candidate}: inside the /tmp read root or the repo`); + continue; + } + return resolved; + } catch (error) { + rejections.push(`${candidate}: ${error}`); + } + } + // Skipping is acceptable on a developer box, but on CI a silently disabled + // security regression test is indistinguishable from a passing one. Fail + // loudly instead and let the operator point QWEN_TEST_EXTERNAL_READ_BASE at + // a writable directory outside both the workspace and the /tmp read root. + const diagnostics = `no usable external-read fixture base (${rejections.join('; ')})`; + if (process.env['CI']) { + throw new Error( + `${diagnostics}. Set QWEN_TEST_EXTERNAL_READ_BASE to a writable ` + + 'directory outside the repo and outside /tmp.', + ); + } + console.warn( + `[qwen-serve-streaming] skipping external read tests: ${diagnostics}`, + ); + return undefined; +} + +const externalReadBase = findExternalReadBase(); + let daemon: ChildProcess; let port = 0; let base = ''; let client: DaemonClient; let fakeServer: FakeOpenAIServer; let homeDir = ''; +let externalReadDir = ''; let workspaceDir = ''; let pendingWritePath = ''; +let pendingReadPath = ''; +let pendingReadMarker = ''; +let pendingReadSentinel = ''; beforeAll(async () => { if (SKIP) return; @@ -119,9 +187,45 @@ beforeAll(async () => { }; } + if ( + pendingReadPath && + pendingReadMarker && + messages.includes(pendingReadMarker) + ) { + if (!hasToolResult) { + return { + toolCalls: [ + fakeToolCall('read_file', { + file_path: pendingReadPath, + }), + ], + }; + } + + return { + content: messages.includes(pendingReadSentinel) + ? `external read observed: ${pendingReadSentinel}` + : 'external read content not observed', + }; + } + return { content: 'fake response complete' }; }); homeDir = mkdtempSync(path.join(tmpdir(), 'qwen-serve-streaming-home-')); + if (externalReadBase) { + let candidateDir = ''; + try { + candidateDir = mkdtempSync( + path.join(externalReadBase, '.qwen-serve-external-read-'), + ); + externalReadDir = realpathSync(candidateDir); + } catch { + if (candidateDir) { + rmSync(candidateDir, { recursive: true, force: true }); + } + externalReadDir = ''; + } + } const qwenHome = path.join(homeDir, '.qwen'); mkdirSync(qwenHome, { recursive: true }); writeFileSync( @@ -168,6 +272,7 @@ beforeAll(async () => { ), HOME: homeDir, QWEN_HOME: path.join(homeDir, '.qwen'), + QWEN_ACP_LOCAL_READ_ROOTS: '', NO_PROXY: '127.0.0.1,localhost', no_proxy: '127.0.0.1,localhost', OPENAI_API_KEY: 'fake-key', @@ -215,6 +320,9 @@ afterAll(async () => { if (homeDir) { rmSync(homeDir, { recursive: true, force: true }); } + if (externalReadDir) { + rmSync(externalReadDir, { recursive: true, force: true }); + } if (workspaceDir) { rmSync(workspaceDir, { recursive: true, force: true }); } @@ -452,6 +560,159 @@ describePOSIX('qwen serve — multi-client first-responder permission', () => { }, 90_000); }); +describePOSIX('qwen serve — same-host external text reads', () => { + async function runExternalRead( + decision: 'allow_once' | 'reject_once', + ): Promise { + const suffix = `${decision}-${Date.now()}`; + const marker = `external-read-${suffix}`; + const sentinel = `external-read-sentinel-${suffix}`; + const externalPath = path.join(externalReadDir, 'outside-workspace.txt'); + writeFileSync(externalPath, sentinel); + pendingReadPath = externalPath; + pendingReadMarker = marker; + pendingReadSentinel = sentinel; + + const session = await client.createOrAttachSession({ + // The daemon is bound to `workspaceDir` by `beforeAll`, so any other + // value is rejected with 400 Workspace mismatch. The read under test is + // external because `externalReadDir` sits outside this workspace, not + // because the session claims a wider one. + workspaceCwd: workspaceDir, + sessionScope: 'thread', + }); + await client.setSessionApprovalMode(session.sessionId, 'default'); + + const events: DaemonEvent[] = []; + const ac = new AbortController(); + let promptId: string | undefined; + const subscriber = (async () => { + try { + for await (const event of sseFrames(session.sessionId, { + signal: ac.signal, + })) { + events.push(event); + const data = event.data as { promptId?: string } | undefined; + if (event.type === 'turn_complete' && data?.promptId === promptId) { + break; + } + } + } catch { + /* aborted */ + } + })(); + const findReadPermission = () => + events.find((event) => { + if (event.type !== 'permission_request') return false; + const data = event.data as { + toolCall?: { + rawInput?: { file_path?: string }; + _meta?: { toolName?: string }; + }; + }; + return ( + data.toolCall?._meta?.toolName === 'read_file' && + data.toolCall.rawInput?.file_path === externalPath + ); + }); + + const requestStart = fakeServer.requests.length; + try { + await new Promise((resolve) => setTimeout(resolve, 200)); + const accepted = await client.promptNonBlocking(session.sessionId, { + prompt: [{ type: 'text', text: marker }], + }); + expect('promptId' in accepted).toBe(true); + if (!('promptId' in accepted)) return; + promptId = accepted.promptId; + + await expect.poll(findReadPermission, { timeout: 30_000 }).toBeDefined(); + const permission = findReadPermission(); + const permissionData = permission!.data as { + requestId: string; + options: Array<{ optionId: string; kind: string }>; + }; + const optionId = permissionData.options.find( + (option) => option.kind === decision, + )?.optionId; + expect(optionId).toBeDefined(); + expect( + await client.respondToPermission(permissionData.requestId, { + outcome: { outcome: 'selected', optionId: optionId! }, + }), + ).toBe(true); + + await expect + .poll( + () => + events.some((event) => { + const data = event.data as { promptId?: string } | undefined; + return ( + event.type === 'turn_complete' && data?.promptId === promptId + ); + }), + { timeout: 30_000 }, + ) + .toBe(true); + + const modelRequests = fakeServer.requests + .slice(requestStart) + .map((request) => JSON.stringify(request.body['messages'] ?? [])) + .filter((messages) => messages.includes(marker)); + + const serializedEvents = JSON.stringify(events); + if (decision === 'allow_once') { + expect(modelRequests.length).toBeGreaterThanOrEqual(2); + expect( + modelRequests.some((messages) => messages.includes(sentinel)), + ).toBe(true); + expect(serializedEvents).toContain( + `external read observed: ${sentinel}`, + ); + } else { + expect(modelRequests).toHaveLength(1); + expect( + modelRequests.every((messages) => !messages.includes(sentinel)), + ).toBe(true); + expect( + events.some((event) => { + if (event.type !== 'session_update') return false; + const data = event.data as { + update?: { sessionUpdate?: string; status?: string }; + }; + return ( + data.update?.sessionUpdate === 'tool_call_update' && + data.update.status === 'failed' + ); + }), + ).toBe(true); + // The failed `tool_call_update` above and the sentinel absence below + // carry the whole meaning. Asserting the user-facing rejection copy + // would fail on a wording change or a non-English locale for reasons + // unrelated to the capability under test. + expect(serializedEvents).not.toContain(sentinel); + } + } finally { + await client.cancel(session.sessionId).catch(() => undefined); + ac.abort(); + await subscriber; + await client.closeSession(session.sessionId).catch(() => undefined); + pendingReadPath = ''; + pendingReadMarker = ''; + pendingReadSentinel = ''; + rmSync(externalPath, { force: true }); + } + } + + it('returns approved content and withholds rejected content', async (ctx) => { + if (!externalReadDir) { + ctx.skip('no writable fixture root outside the workspace and /tmp'); + } + await runExternalRead('allow_once'); + await runExternalRead('reject_once'); + }, 150_000); +}); + describePOSIX('qwen serve — Last-Event-ID resume', () => { it('reconnect with Last-Event-ID:N yields events with id > N', async () => { const session = await client.createOrAttachSession({ diff --git a/packages/acp-bridge/README.md b/packages/acp-bridge/README.md index c278e7aa41..9b090df529 100644 --- a/packages/acp-bridge/README.md +++ b/packages/acp-bridge/README.md @@ -62,6 +62,9 @@ Lift history (#4175 Mode B daemon roadmap): injection seam for daemon-host env / preflight cells (production impl in `cli/src/serve/daemon-status-provider.ts`) and the F1 `BridgeFileSystem` injection seam for the ACP fs proxy. + `delegateReadTextFileToClient` defaults to `true`; same-host daemon callers + may set it to `false` so child text reads use the regular CLI filesystem + service while final ACP text writes remain delegated. - `spawnChannel` (F1) — `defaultSpawnChannelFactory` + `killChild` + `SCRUBBED_CHILD_ENV_KEYS` denylist + `scrubChildEnv` pure env-policy helper (exported for adapter reuse + unit-test access; isolates the @@ -90,9 +93,10 @@ Lift history (#4175 Mode B daemon roadmap): ACP fs proxy. When wired through `BridgeOptions.fileSystem`, `BridgeClient.readTextFile` / `BridgeClient.writeTextFile` delegate to it instead of the inline `fs.realpath` / - `fs.writeFile` / `fs.readFile` proxy. Production `qwen serve` - follow-up wraps PR 18's `WorkspaceFileSystem` here so writes - get TOCTOU + symlink + trust-gate + audit guarantees. + `fs.writeFile` / `fs.readFile` proxy. Production `qwen serve` injects + `WorkspaceFileSystem` for final ACP `writeTextFile` content writes and for + defensive handling of unexpected or capability-violating delegated reads; + normal same-host text reads stay in the child. ## Imports — root vs subpaths diff --git a/packages/acp-bridge/src/bridge-file-capabilities.test.ts b/packages/acp-bridge/src/bridge-file-capabilities.test.ts new file mode 100644 index 0000000000..a3dbc336eb --- /dev/null +++ b/packages/acp-bridge/src/bridge-file-capabilities.test.ts @@ -0,0 +1,47 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, describe, expect, it } from 'vitest'; +import { makeBridge, makeChannel } from './internal/testUtils.js'; +import type { AcpSessionBridge } from './bridgeTypes.js'; + +describe('ACP bridge file capabilities', () => { + let bridge: AcpSessionBridge | undefined; + + afterEach(async () => { + await bridge?.shutdown(); + bridge = undefined; + }); + + it('delegates reads to the ACP client by default', async () => { + const handle = makeChannel(); + bridge = makeBridge({ channelFactory: async () => handle.channel }); + + await bridge.preheat(); + + expect(handle.agent.initializeCalls).toHaveLength(1); + expect(handle.agent.initializeCalls[0]!.clientCapabilities!.fs).toEqual({ + readTextFile: true, + writeTextFile: true, + }); + }); + + it('can keep text reads in a same-host ACP child', async () => { + const handle = makeChannel(); + bridge = makeBridge({ + channelFactory: async () => handle.channel, + delegateReadTextFileToClient: false, + }); + + await bridge.preheat(); + + expect(handle.agent.initializeCalls).toHaveLength(1); + expect(handle.agent.initializeCalls[0]!.clientCapabilities!.fs).toEqual({ + readTextFile: false, + writeTextFile: true, + }); + }); +}); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 63bf942936..14e84d2f53 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1394,6 +1394,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { let liveTaskToolRequestHandler: LiveTaskToolRequestHandler | undefined; let liveSpeakToUserHandler: LiveSpeakToUserHandler | undefined; const defaultSessionScope = opts.sessionScope ?? 'single'; + // Resolved once beside the other option defaults: this default is + // load-bearing for every non-daemon consumer, and reading `?? true` inline + // would let a second `initialize` site drift away from it. + const delegateReadTextFileToClient = + opts.delegateReadTextFileToClient ?? true; // `undefined` → default 32 (intentionally tight to avoid resource cliffs). // `0` → explicitly unlimited (operator opt-out). // `Infinity` → unlimited (programmatic opt-out — accepted as a @@ -2542,7 +2547,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { [PRIVATE_PARENT_CAPABILITY_META_KEY]: privateParentCapability, }, clientCapabilities: { - fs: { readTextFile: true, writeTextFile: true }, + fs: { + readTextFile: delegateReadTextFileToClient, + writeTextFile: true, + }, }, clientInfo: { name: 'qwen-serve-bridge', version: '0' }, }), diff --git a/packages/acp-bridge/src/bridgeOptions.ts b/packages/acp-bridge/src/bridgeOptions.ts index 0d1eee25ef..99e67f7d49 100644 --- a/packages/acp-bridge/src/bridgeOptions.ts +++ b/packages/acp-bridge/src/bridgeOptions.ts @@ -373,19 +373,23 @@ export interface BridgeOptions { telemetry?: BridgeTelemetry; /** - * Optional fs injection seam. When provided, `BridgeClient.readTextFile` and - * `BridgeClient.writeTextFile` delegate every ACP fs call to this - * implementation instead of using BridgeClient's inline - * `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy. + * Whether ACP text reads are delegated to the client filesystem service. + * Defaults to true for generic ACP, IDE, remote, and virtual-filesystem + * compatibility. Same-host runtimes may set false so the child uses its + * regular CLI filesystem service for every `FileSystemService.readTextFile` + * consumer. Final ACP text writes remain delegated independently. + */ + delegateReadTextFileToClient?: boolean; + + /** + * Optional fs injection seam. When provided, the enabled + * `BridgeClient.readTextFile` / `BridgeClient.writeTextFile` callbacks + * delegate ACP fs calls to this implementation instead of using + * BridgeClient's inline `fs.realpath` / `fs.writeFile` / `fs.readFile` + * proxy. * - * The immediate F1 follow-up will land a serve-side adapter that - * wraps its `WorkspaceFileSystem` and a `runQwenServe` wiring - * patch so production `qwen serve` writes pick up its TOCTOU + - * symlink-substitution + trust-gate + `.gitignore` + audit - * machinery — closing the follow-up thread about - * `BridgeClient`'s inline fs proxy bypassing `WorkspaceFileSystem` - * (originally raised in code review). Until that lands, BridgeClient's inline - * proxy continues to handle writes (current behavior preserved). + * Production `qwen serve` injects a `WorkspaceFileSystem` adapter for final + * text writes and for defensive handling of unexpected delegated reads. * * When omitted (tests, Mode A in-process consumers, channels / * IDE companion using the bridge directly), BridgeClient's inline diff --git a/packages/cli/src/acp-integration/service/filesystem.test.ts b/packages/cli/src/acp-integration/service/filesystem.test.ts index ba31dc6de5..5aa630cf52 100644 --- a/packages/cli/src/acp-integration/service/filesystem.test.ts +++ b/packages/cli/src/acp-integration/service/filesystem.test.ts @@ -26,6 +26,7 @@ import type { FileSystemService } from '@qwen-code/qwen-code-core'; import { AcpFileSystemService } from './filesystem.js'; import type { AgentSideConnection } from '@agentclientprotocol/sdk'; import { promises as fs } from 'node:fs'; +import type { Stats } from 'node:fs'; import { realpath as fsRealpath } from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; @@ -965,9 +966,13 @@ describe('AcpFileSystemService', () => { }); }); - it('uses fallback when readTextFile capability is disabled', async () => { + // Split from the write case below on purpose: this half is the one that + // protects the capability's core behavior, so deleting "the write test" + // later must not silently drop read coverage with it. + it('routes reads to the local fallback when readTextFile capability is disabled', async () => { const client = { readTextFile: vi.fn(), + writeTextFile: vi.fn().mockResolvedValue(undefined), } as unknown as AgentSideConnection; const fallback = createFallback(); @@ -987,22 +992,64 @@ describe('AcpFileSystemService', () => { ); const signal = new AbortController().signal; + const stats = {} as Stats; const result = await svc.readTextFile({ path: '/some/file.txt', line: 0, + limit: 7, maxOutputBytes: 2048, signal, + stats, + _meta: { request: 'same-host' }, }); expect(result).toEqual(fallbackResponse); expect(fallback.readTextFile).toHaveBeenCalledWith({ path: '/some/file.txt', line: 0, + limit: 7, maxOutputBytes: 2048, signal, + stats, + _meta: { request: 'same-host' }, }); expect(client.readTextFile).not.toHaveBeenCalled(); }); + + it('keeps writes delegated when readTextFile capability is disabled', async () => { + const client = { + readTextFile: vi.fn(), + writeTextFile: vi.fn().mockResolvedValue(undefined), + } as unknown as AgentSideConnection; + + const fallback = createFallback(); + const svc = new AcpFileSystemService( + client, + 'session-3', + { readTextFile: false, writeTextFile: true }, + fallback, + ); + + // A defined `_meta` round trip: `toEqual` ignores undefined-valued + // properties, so asserting `{ _meta: undefined }` also passes for `{}` + // and would not catch the field being dropped. `bom: true` additionally + // pins the BOM prepend on the delegated content. + const meta = { bom: true }; + const writeResult = await svc.writeTextFile({ + path: '/some/file.txt', + content: 'updated content', + _meta: meta, + }); + + expect(writeResult).toEqual({ _meta: meta }); + expect(client.writeTextFile).toHaveBeenCalledWith({ + path: '/some/file.txt', + content: '\uFEFFupdated content', + sessionId: 'session-3', + _meta: meta, + }); + expect(fallback.writeTextFile).not.toHaveBeenCalled(); + }); }); describe('writeTextFile', () => { diff --git a/packages/cli/src/acp-integration/service/filesystem.ts b/packages/cli/src/acp-integration/service/filesystem.ts index bcc010f716..2744131835 100644 --- a/packages/cli/src/acp-integration/service/filesystem.ts +++ b/packages/cli/src/acp-integration/service/filesystem.ts @@ -145,6 +145,10 @@ export class AcpFileSystemService implements FileSystemService { return this.fallback.readTextFile(params); } + // Everything below — including the localReadRoots retry in the catch — is + // unreachable under `qwen serve`, which advertises this capability as + // false. It guards only generic ACP hosts that keep delegation on. Do not + // read the retry as a live backstop for daemon reads. let response: ReadTextFileResponse; try { response = await this.connection.readTextFile( diff --git a/packages/cli/src/serve/bridge-file-system-adapter.ts b/packages/cli/src/serve/bridge-file-system-adapter.ts index 584826b042..0bc48cba03 100644 --- a/packages/cli/src/serve/bridge-file-system-adapter.ts +++ b/packages/cli/src/serve/bridge-file-system-adapter.ts @@ -6,10 +6,11 @@ /** * Serve-side adapter that satisfies `@qwen-code/acp-bridge`'s - * `BridgeFileSystem` interface by routing ACP `writeTextFile` / - * `readTextFile` requests through the `WorkspaceFileSystem`. Agent-side - * ACP fs calls pick up the same defensive guarantees the HTTP file - * routes already enforce. + * `BridgeFileSystem` interface by routing delegated ACP `writeTextFile` / + * `readTextFile` requests through the `WorkspaceFileSystem`. Production + * `qwen serve` keeps text reads in the same-host child and delegates final ACP + * `writeTextFile` content writes through this adapter. The read path remains a + * fail-closed boundary for unexpected or capability-violating delegated reads. * * The adapter is a thin translation layer: * - ACP request → `WorkspaceFileSystem.resolve(path, intent)` to @@ -87,8 +88,8 @@ function buildAuditContext( /** * Adapter factory. Pass the existing `WorkspaceFileSystemFactory` * (the same instance `createServeApp` / `runQwenServe` build for - * HTTP fs routes) — both paths share the same `fsAuditEmit` channel - * + trust gate snapshot so an operator gets a unified audit stream. + * HTTP fs routes) — delegated operations share the same `fsAuditEmit` channel + * + trust gate snapshot. */ export function createBridgeFileSystemAdapter( factory: WorkspaceFileSystemFactory, diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 47986a5fff..41ac3c204a 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -1257,6 +1257,11 @@ describe('runQwenServe telemetry validation', () => { await closing; } expect(createBridge).toHaveBeenCalledTimes(2); + for (const [options] of createBridge.mock.calls) { + expect(options).toMatchObject({ + delegateReadTextFileToClient: false, + }); + } for (const result of createBridge.mock.results) { expect(result.value.shutdown).toHaveBeenCalledWith({ reason: 'daemon_shutdown', @@ -1485,6 +1490,11 @@ describe('runQwenServe telemetry validation', () => { }); expect(readded.status).toBe(201); expect(createBridge).toHaveBeenCalledTimes(3); + for (const [options] of createBridge.mock.calls) { + expect(options).toMatchObject({ + delegateReadTextFileToClient: false, + }); + } let releaseRemoval!: (count: number) => void; removeByIds.mockImplementationOnce( () => diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index ae3c849719..de36145482 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -3934,6 +3934,7 @@ async function runQwenServeImpl( : {}), permissionAudit: permissionAuditPublisher, statusProvider, + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(fsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { @@ -4334,6 +4335,7 @@ async function runQwenServeImpl( : {}), permissionAudit: permissionAuditPublisher, statusProvider: secondaryStatusProvider, + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(secondaryBridgeFsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { @@ -4884,6 +4886,7 @@ async function runQwenServeImpl( statusProvider: runtime.createDaemonStatusProvider({ env: wsEnv.effectiveEnv, }), + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(wsFsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { diff --git a/packages/cli/src/serve/server-default-bridge-wiring.test.ts b/packages/cli/src/serve/server-default-bridge-wiring.test.ts index cbd464df86..c5eac70377 100644 --- a/packages/cli/src/serve/server-default-bridge-wiring.test.ts +++ b/packages/cli/src/serve/server-default-bridge-wiring.test.ts @@ -53,6 +53,7 @@ describe('createServeApp default bridge wiring', () => { it('wires the internally-created bridge lifecycle into the workspace registry', async () => { let sessionLifecycle: BridgeOptions['sessionLifecycle']; + let bridgeOptions: BridgeOptions | undefined; const liveSessionIds = new Set(); const bridge = makeBridge(0, liveSessionIds); vi.doMock('./acp-session-bridge.js', async () => { @@ -62,6 +63,7 @@ describe('createServeApp default bridge wiring', () => { return { ...actual, createAcpSessionBridge: vi.fn((opts: BridgeOptions) => { + bridgeOptions = opts; sessionLifecycle = opts.sessionLifecycle; return bridge; }), @@ -80,6 +82,9 @@ describe('createServeApp default bridge wiring', () => { const locals = app.locals as { workspaceRegistry?: WorkspaceRegistry }; expect(sessionLifecycle).toBeDefined(); + expect(bridgeOptions).toMatchObject({ + delegateReadTextFileToClient: false, + }); liveSessionIds.add('session-indexed'); sessionLifecycle!({ type: 'registered', diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index c817a8461e..6f403aac5d 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -977,8 +977,9 @@ export function createServeApp( // Wire the production status provider so direct embeds / tests // that don't inject `deps.bridge` get daemon env + preflight cells. statusProvider, - // Wire the WorkspaceFileSystem adapter so ACP writeTextFile / - // readTextFile pick up trust / TOCTOU / audit. + delegateReadTextFileToClient: false, + // Final ACP text writes remain delegated through WorkspaceFileSystem. + // Unexpected delegated reads still fail closed at the WFS boundary. fileSystem: createBridgeFileSystemAdapter(fsFactory), // Reverse tool channel: answer the child's `client_mcp/message` // ext-method by reaching the WS connection that hosts the named server.