fix(serve): Allow approved same-host text reads outside workspace (#8620)
Some checks failed
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 1/2 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 2/2 (push) Waiting to run
E2E Tests / channel-plugin E2E (nightly) (push) Waiting to run
E2E Tests / cron-interactive E2E (nightly) (push) Waiting to run
E2E Tests / web-shell Browser Regression (push) Waiting to run
SDK Java / ubuntu-latest / Java 11 (push) Waiting to run
SDK Java / ubuntu-latest / Java 17 (push) Waiting to run
SDK Java / macos-latest / Java 21 (push) Waiting to run
SDK Java / ubuntu-latest / Java 21 (push) Waiting to run
SDK Java / windows-latest / Java 21 (push) Waiting to run
SDK Java / Real daemon E2E / Java 11 (push) Waiting to run
npm cache producer / Save npm cache (push) Has been cancelled

* fix(serve): allow same-host daemon text reads

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
jinye 2026-08-07 15:33:16 +08:00 committed by GitHub
parent 5fdcdb28e2
commit bf3abdee81
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
17 changed files with 533 additions and 32 deletions

View file

@ -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. |

View file

@ -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):