From 63a6cc4a86775f8a4121f54c418d4e59cb274943 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Thu, 6 Aug 2026 10:47:13 +0800 Subject: [PATCH 1/3] fix(serve): allow same-host daemon text reads Co-authored-by: Qwen-Coder --- docs/design/daemon-local-text-reads.md | 48 ++++ docs/developers/daemon/03-acp-bridge.md | 3 +- .../daemon/07-workspace-filesystem.md | 8 +- docs/users/qwen-serve.md | 2 +- .../cli/qwen-serve-streaming.test.ts | 238 +++++++++++++++++- packages/acp-bridge/README.md | 10 +- .../src/bridge-file-capabilities.test.ts | 47 ++++ packages/acp-bridge/src/bridge.ts | 5 +- packages/acp-bridge/src/bridgeOptions.ts | 28 ++- .../service/filesystem.test.ts | 24 +- .../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 +- 15 files changed, 416 insertions(+), 33 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 00000000000..9619e39609f --- /dev/null +++ b/docs/design/daemon-local-text-reads.md @@ -0,0 +1,48 @@ +# 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. + +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. diff --git a/docs/developers/daemon/03-acp-bridge.md b/docs/developers/daemon/03-acp-bridge.md index b33decd574b..ea2270fa588 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 a0f6f3a270c..9882a18095a 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,9 @@ 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 includes direct `read_file` and shared pre-reads used by write, edit, notebook, sed, and artifact operations. It intentionally accepts regular CLI read behavior rather than 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 external `read_file` keeps the normal CLI permission rules and core file-operation telemetry. HTTP filesystem routes remain workspace-scoped; agent discovery-tool behavior is unchanged by this capability. Auxiliary actions such as parent-directory creation and shell commands are separate existing paths and are not covered by this boundary. `qwen serve` assumes a same-machine, same-UID security principal and is not an OS sandbox. ## Responsibilities @@ -66,7 +68,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 d76a8253457..c3b28ef0bdc 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -554,7 +554,7 @@ provider decision with their normal tool policy and isolation boundary. - **Host header allowlist** — on **loopback** binds the daemon checks `Host:` matches `localhost:port` / `127.0.0.1:port` / `[::1]:port` / `host.docker.internal:port` (case-insensitive per RFC 7230 §5.4) to defend against DNS rebinding. **Non-loopback binds (`--hostname 0.0.0.0`) intentionally bypass the Host allowlist** — the operator has chosen the surface area, so the bearer-token gate is the sole authentication layer; reverse proxies / SNI / client cert pinning are the operator's responsibility, not the daemon's. If you need Host-based isolation on a non-loopback bind, terminate TLS + check Host at a front proxy. - **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. +- **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, and direct `read_file` can read host text paths outside registered workspaces under the normal CLI permission rules (external paths default to confirmation; allow rules and approval modes may approve them automatically). Approved external reads use configurable CLI output limits rather than WFS's 256 KiB returned-output and full-snapshot cap or 8 MiB large-text scan cap. All shared `FileSystemService.readTextFile` consumers—including pre-reads for write, edit, notebook, sed, and artifact operations—also use the child's regular CLI filesystem service and therefore do not receive those WFS limits, read audit, symlink rejection, or read-side TOCTOU protections. HTTP filesystem routes remain workspace-scoped, agent discovery-tool behavior is unchanged, and final ACP `writeTextFile` content writes continue through WFS. Anything in `~/.bashrc`, `~/.aws/credentials`, or `~/.npmrc` is therefore reachable by an approved direct read or shell access; treat authenticated daemon clients as the same security principal and do not run `qwen serve` under an identity that has credentials you would not trust the agent with. - **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 a9ab2b2ce8f..0b20fa8f3bb 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,11 +24,22 @@ * 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 { tmpdir } from 'node:os'; +import { + accessSync, + constants, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { homedir, tmpdir } from 'node:os'; import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; @@ -51,6 +62,16 @@ const CLI_BIN = const TOKEN = 'streaming-integ-secret'; const REPO_ROOT = path.resolve(__dirname, '../..'); +function isPathAtOrWithin(root: string, candidate: string): boolean { + const relative = path.relative(root, candidate); + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); +} + // Windows: this suite shells out to `pgrep` / `kill -KILL` to simulate // child-process crashes for the SIGKILL → `session_died` test, and those // binaries are POSIX-only. A Windows-equivalent (`taskkill`) would need @@ -72,13 +93,38 @@ const SKIP = ); const describePOSIX = SKIP ? describe.skip : describe; +function findExternalReadBase(): string | undefined { + if (SKIP) return undefined; + for (const candidate of [homedir(), '/var/tmp']) { + try { + const resolved = realpathSync(candidate); + accessSync(resolved, constants.W_OK); + if ( + !isPathAtOrWithin(realpathSync('/tmp'), resolved) && + !isPathAtOrWithin(realpathSync(REPO_ROOT), resolved) + ) { + return resolved; + } + } catch { + // Try the next candidate. + } + } + return undefined; +} + +const externalReadBase = findExternalReadBase(); + let daemon: ChildProcess; let port = 0; let base = ''; let client: DaemonClient; let fakeServer: FakeOpenAIServer; let homeDir = ''; +let externalReadDir = ''; let pendingWritePath = ''; +let pendingReadPath = ''; +let pendingReadMarker = ''; +let pendingReadSentinel = ''; beforeAll(async () => { if (SKIP) return; @@ -119,9 +165,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( @@ -164,6 +246,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', @@ -211,6 +294,9 @@ afterAll(async () => { if (homeDir) { rmSync(homeDir, { recursive: true, force: true }); } + if (externalReadDir) { + rmSync(externalReadDir, { recursive: true, force: true }); + } }, 15_000); /** Open an authenticated SSE stream and yield parsed frames. */ @@ -445,6 +531,152 @@ 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({ + workspaceCwd: REPO_ROOT, + 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); + expect(serializedEvents).toContain('was canceled by the user.'); + 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 c278e7aa41d..9b090df529b 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 00000000000..a3dbc336eba --- /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 b7b8b4dbdf6..5effaec93a2 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -2542,7 +2542,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { [PRIVATE_PARENT_CAPABILITY_META_KEY]: privateParentCapability, }, clientCapabilities: { - fs: { readTextFile: true, writeTextFile: true }, + fs: { + readTextFile: opts.delegateReadTextFileToClient ?? true, + 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 0d1eee25efc..0e2af10b9f5 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, enabled + * `BridgeClient.readTextFile` and `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 ba31dc6de56..891279f7edc 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,10 @@ describe('AcpFileSystemService', () => { }); }); - it('uses fallback when readTextFile capability is disabled', async () => { + 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(); @@ -987,21 +989,41 @@ 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(); + + const writeResult = await svc.writeTextFile({ + path: '/some/file.txt', + content: 'updated content', + }); + + expect(writeResult).toEqual({ _meta: undefined }); + expect(client.writeTextFile).toHaveBeenCalledWith({ + path: '/some/file.txt', + content: 'updated content', + sessionId: 'session-3', + }); + expect(fallback.writeTextFile).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/serve/bridge-file-system-adapter.ts b/packages/cli/src/serve/bridge-file-system-adapter.ts index 584826b0427..0bc48cba03b 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 54b32cc2ae6..98e267d0c3e 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -1210,6 +1210,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', @@ -1438,6 +1443,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 f5435259e5c..bcd2aebfca5 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -3891,6 +3891,7 @@ async function runQwenServeImpl( : {}), permissionAudit: permissionAuditPublisher, statusProvider, + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(fsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { @@ -4290,6 +4291,7 @@ async function runQwenServeImpl( : {}), permissionAudit: permissionAuditPublisher, statusProvider: secondaryStatusProvider, + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(secondaryBridgeFsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { @@ -4814,6 +4816,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 cbd464df86f..c5eac703770 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 f06b738ff1e..9c8c2b043ed 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -975,8 +975,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. From 2aa6748df65ebc79f16e77757cfea5f930baf42a Mon Sep 17 00:00:00 2001 From: jinye Date: Thu, 6 Aug 2026 19:12:34 +0800 Subject: [PATCH 2/3] 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. --- docs/design/daemon-local-text-reads.md | 27 ++++++++++++++++ .../serve-large-text-range-consistency.md | 9 +++++- .../daemon/07-workspace-filesystem.md | 2 +- docs/users/qwen-serve.md | 3 +- .../cli/qwen-serve-streaming.test.ts | 29 +++++++++-------- .../service/filesystem.test.ts | 31 +++++++++++++++++-- 6 files changed, 80 insertions(+), 21 deletions(-) diff --git a/docs/design/daemon-local-text-reads.md b/docs/design/daemon-local-text-reads.md index 9619e39609f..ec554be263f 100644 --- a/docs/design/daemon-local-text-reads.md +++ b/docs/design/daemon-local-text-reads.md @@ -25,6 +25,33 @@ 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 diff --git a/docs/design/serve-large-text-range-consistency.md b/docs/design/serve-large-text-range-consistency.md index 0d2323d5c0b..325852c5949 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/07-workspace-filesystem.md b/docs/developers/daemon/07-workspace-filesystem.md index 9882a18095a..7038536fca7 100644 --- a/docs/developers/daemon/07-workspace-filesystem.md +++ b/docs/developers/daemon/07-workspace-filesystem.md @@ -13,7 +13,7 @@ Daemon HTTP file routes and delegated ACP `readTextFile` / `writeTextFile` calls 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 includes direct `read_file` and shared pre-reads used by write, edit, notebook, sed, and artifact operations. It intentionally accepts regular CLI read behavior rather than 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 external `read_file` keeps the normal CLI permission rules and core file-operation telemetry. HTTP filesystem routes remain workspace-scoped; agent discovery-tool behavior is unchanged by this capability. Auxiliary actions such as parent-directory creation and shell commands are separate existing paths and are not covered by this boundary. `qwen serve` assumes a same-machine, same-UID security principal and is not an OS sandbox. +That text-read capability slice includes direct `read_file` and 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, and also records why #8618 still reproduces for the write and edit family even after this change. Direct external `read_file` keeps the normal CLI permission rules and core file-operation telemetry. HTTP filesystem routes remain workspace-scoped; agent discovery-tool behavior is unchanged by this capability. Auxiliary actions such as parent-directory creation and shell commands are separate existing paths and are not covered by this boundary. `qwen serve` assumes a same-machine, same-UID security principal and is not an OS sandbox. ## Responsibilities diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index c3b28ef0bdc..47b241bde9e 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -554,7 +554,8 @@ provider decision with their normal tool policy and isolation boundary. - **Host header allowlist** — on **loopback** binds the daemon checks `Host:` matches `localhost:port` / `127.0.0.1:port` / `[::1]:port` / `host.docker.internal:port` (case-insensitive per RFC 7230 §5.4) to defend against DNS rebinding. **Non-loopback binds (`--hostname 0.0.0.0`) intentionally bypass the Host allowlist** — the operator has chosen the surface area, so the bearer-token gate is the sole authentication layer; reverse proxies / SNI / client cert pinning are the operator's responsibility, not the daemon's. If you need Host-based isolation on a non-loopback bind, terminate TLS + check Host at a front proxy. - **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, and direct `read_file` can read host text paths outside registered workspaces under the normal CLI permission rules (external paths default to confirmation; allow rules and approval modes may approve them automatically). Approved external reads use configurable CLI output limits rather than WFS's 256 KiB returned-output and full-snapshot cap or 8 MiB large-text scan cap. All shared `FileSystemService.readTextFile` consumers—including pre-reads for write, edit, notebook, sed, and artifact operations—also use the child's regular CLI filesystem service and therefore do not receive those WFS limits, read audit, symlink rejection, or read-side TOCTOU protections. HTTP filesystem routes remain workspace-scoped, agent discovery-tool behavior is unchanged, and final ACP `writeTextFile` content writes continue through WFS. Anything in `~/.bashrc`, `~/.aws/credentials`, or `~/.npmrc` is therefore reachable by an approved direct read or shell access; treat authenticated daemon clients as the same security principal and do not run `qwen serve` under an identity that has credentials you would not trust the agent with. +- **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 0b20fa8f3bb..2e2043f1d8a 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -39,10 +39,11 @@ import { rmSync, writeFileSync, } from 'node:fs'; -import { homedir, tmpdir } from 'node:os'; +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 { @@ -62,16 +63,6 @@ const CLI_BIN = const TOKEN = 'streaming-integ-secret'; const REPO_ROOT = path.resolve(__dirname, '../..'); -function isPathAtOrWithin(root: string, candidate: string): boolean { - const relative = path.relative(root, candidate); - return ( - relative === '' || - (relative !== '..' && - !relative.startsWith(`..${path.sep}`) && - !path.isAbsolute(relative)) - ); -} - // Windows: this suite shells out to `pgrep` / `kill -KILL` to simulate // child-process crashes for the SIGKILL → `session_died` test, and those // binaries are POSIX-only. A Windows-equivalent (`taskkill`) would need @@ -93,15 +84,20 @@ const SKIP = ); const describePOSIX = SKIP ? describe.skip : describe; +// Deliberately excludes the real `$HOME`: fixtures are created with +// `mkdtempSync` and only removed in `afterAll`, so a Ctrl-C, `--bail`, or CI +// timeout would leave a hidden directory behind in the developer's home on +// every interrupted run. `/var/tmp` is outside both the workspace and the +// `/tmp` local-read root, which is all this fixture base needs to be. function findExternalReadBase(): string | undefined { if (SKIP) return undefined; - for (const candidate of [homedir(), '/var/tmp']) { + for (const candidate of ['/var/tmp']) { try { const resolved = realpathSync(candidate); accessSync(resolved, constants.W_OK); if ( - !isPathAtOrWithin(realpathSync('/tmp'), resolved) && - !isPathAtOrWithin(realpathSync(REPO_ROOT), resolved) + !isPathWithinRoot(resolved, realpathSync('/tmp')) && + !isPathWithinRoot(resolved, realpathSync(REPO_ROOT)) ) { return resolved; } @@ -653,7 +649,10 @@ describePOSIX('qwen serve — same-host external text reads', () => { ); }), ).toBe(true); - expect(serializedEvents).toContain('was canceled by the user.'); + // 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 { diff --git a/packages/cli/src/acp-integration/service/filesystem.test.ts b/packages/cli/src/acp-integration/service/filesystem.test.ts index 891279f7edc..5aa630cf523 100644 --- a/packages/cli/src/acp-integration/service/filesystem.test.ts +++ b/packages/cli/src/acp-integration/service/filesystem.test.ts @@ -966,7 +966,10 @@ describe('AcpFileSystemService', () => { }); }); - it('keeps writes delegated 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), @@ -1011,17 +1014,39 @@ describe('AcpFileSystemService', () => { _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: undefined }); + expect(writeResult).toEqual({ _meta: meta }); expect(client.writeTextFile).toHaveBeenCalledWith({ path: '/some/file.txt', - content: 'updated content', + content: '\uFEFFupdated content', sessionId: 'session-3', + _meta: meta, }); expect(fallback.writeTextFile).not.toHaveBeenCalled(); }); From 297dea9fde67020c37d016aa8c1c3968de7ca475 Mon Sep 17 00:00:00 2001 From: jinye Date: Fri, 7 Aug 2026 12:56:56 +0800 Subject: [PATCH 3/3] 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 --- .../cli/qwen-serve-streaming.test.ts | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/integration-tests/cli/qwen-serve-streaming.test.ts b/integration-tests/cli/qwen-serve-streaming.test.ts index 3ef822feedf..6e0c68a9972 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -53,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 @@ -95,31 +96,40 @@ function findExternalReadBase(): string | undefined { 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)) + isPathWithinRoot(resolved, realpathSync('/tmp')) || + isPathWithinRoot(resolved, realpathSync(REPO_ROOT)) ) { - return resolved; + rejections.push(`${candidate}: inside the /tmp read root or the repo`); + continue; } - } catch { - // Try the next candidate. + 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( - `No usable external-read fixture base (tried: ${candidates.join(', ')}). ` + - 'Set QWEN_TEST_EXTERNAL_READ_BASE to a writable directory outside the ' + - 'repo and outside /tmp.', + `${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; } @@ -564,7 +574,11 @@ describePOSIX('qwen serve — same-host external text reads', () => { pendingReadSentinel = sentinel; const session = await client.createOrAttachSession({ - workspaceCwd: REPO_ROOT, + // 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');