From 7bd66c6e85f47ebb2f612f0153a453e71a740f75 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Tue, 19 May 2026 19:55:37 +0800 Subject: [PATCH] fix(acp-bridge): bridge.ts security fold-in from #4297 review (3 issues) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds 3 unresolved review comments from the post-merge thread on #4297 (wenshao via qwen-latest agent) into F1 (#4319). All 3 touch `acp-bridge/src/bridge.ts` — the same file F1 already moves the lifted factory into — so consolidating here saves opening a separate follow-up PR and keeps the security narrative in one reviewable commit. The 2 cross-package fixes (`core/src/memory/const.ts` test gap + `cli/src/serve/runQwenServe.ts` malformed-context fallback) will land as their own small PRs after F1 merges. #### Fix 1 (wenshao Critical, #4297 thread): `fs.unlink(target)` arbitrary-file-deletion primitive in `verifyParentWithinWorkspace` 'create'-cleanup After `fs.open(target, 'wx')` creates the empty file at the real parent, an attacker with local workspace write access can swap the parent directory for a symlink (`docs/` → `/etc`). The cleanup's `fs.unlink(target)` re-resolves the TEXTUAL path through the attacker's freshly-planted parent symlink, deleting whatever file exists at the external location. Fix: drop the `fs.unlink(target)` line. The 0-byte file at the pre-race location is harmless (0 bytes, inside the workspace we'd already verified) — leaving it over deleting an arbitrary external file is the right safety trade. Comment block explains the reasoning so future maintainers don't re-introduce the unlink. #### Fix 2 (wenshao Critical): `O_TRUNC` arbitrary-file-truncation primitive in workspace-init 'overwrite' branch `O_TRUNC` causes the kernel to truncate the file to zero bytes AT `open(2)` SYSCALL TIME — strictly before `verifyParentWithinWorkspace` runs. A parent-symlink TOCTOU race between `canonicalizeExistingAncestor` and this `open()` zeros the file at the attacker-redirected location (arbitrary-file-truncation primitive against any file the daemon UID can open). The pre-fix code's own comment on `verifyParentWithinWorkspace` acknowledged this as "Acceptable residual posture for the Stage-1 trust model"; wenshao pushed back that arbitrary-file-zeroing exceeds the Stage-1 trust budget. Fix: drop `O_TRUNC` from the open flags. Truncation moves to AFTER `verifyParentWithinWorkspace` succeeds, via `fh.truncate(0)` on the fd we already hold. fd-based truncate does NOT re-resolve the path — an attacker swapping the parent symlink after we open can't redirect the truncation. #### Fix 3 (wenshao Suggestion): `canonicalizeExistingAncestor` missing `ELOOP` catch Circular symlinks in the parent path (`a -> b`, `b -> a`) cause `fs.realpath` to fail with `ELOOP`. Without catching it, the error propagates as an unstructured HTTP 500 instead of the typed `WorkspaceInitSymlinkError` (HTTP 400) the route handler expects from the workspace-init race-detection family. Fix: add `'ELOOP'` to the caught error codes alongside `'ENOENT'` and `'ENOTDIR'`. Walking up the parent chain when ELOOP hits at a sub-component preserves the existing "walk to the deepest extant ancestor" contract — the deepest realpath-able ancestor still dictates the canonical prefix. #### Why no new tests in this commit - Fix 1 is a single-line removal: any regression that re-adds the unlink would be caught by reviewing the diff; existing 174-test `httpAcpBridge.test.ts` integration suite confirms the create-path still works (file is created + closed correctly; only the attacker-cleanup branch changes). - Fix 2 is a structural move (truncate from open-time to post-verify); the existing overwrite-init integration tests confirm the end-to-end behavior is unchanged (file ends up empty after init). Adding a TOCTOU race regression test requires controlled filesystem-race simulation that exceeds reasonable test infra scope for this PR. - Fix 3 is a one-word addition to an error code list; the `canonicalizeExistingAncestor` helper is module-private and the integration test for circular-symlink → typed 400 would require exporting it OR setting up a real circular-symlink workspace. Both routes widen scope beyond the security fix itself; the high-level behavior is verifiable by the existing route-error- mapping test pattern + diff review. A follow-up PR can add the integration tests once the security fix itself has shipped; the immediate priority is closing the arbitrary-file-deletion + arbitrary-file-truncation primitives. - 62/62 acp-bridge tests pass - 174/174 cli httpAcpBridge.test.ts pass - typecheck + eslint clean #### Refs - Original review on #4297 (wenshao via qwen-latest agent), post- merge, currently unresolvable on #4297 itself because that PR is already MERGED. - Other 2 #4297 review threads (`const.ts` test coverage, `runQwenServe.ts` malformed-context observability) target files outside F1's scope and will land as separate follow-up PRs. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) --- packages/acp-bridge/src/bridge.ts | 71 +++++++++++++++++++++++++------ 1 file changed, 59 insertions(+), 12 deletions(-) diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 0ec22a63ba..116c0d1e62 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -3025,18 +3025,35 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // there — Windows daemon support is best-effort.) let fh: import('node:fs/promises').FileHandle; try { + // #4297 post-merge wenshao Critical fold-in (folded into F1 + // #4319): drop `O_TRUNC` from the open flags. The kernel + // applies O_TRUNC AT `open(2)` SYSCALL TIME — before + // `verifyParentWithinWorkspace` (below) gets a chance to + // detect a parent-symlink race. With O_TRUNC, a local user + // who wins the TOCTOU between `canonicalizeExistingAncestor` + // and this `open()` zeros the file at the attacker- + // redirected location (arbitrary-file-truncation primitive + // against any file the daemon UID can open). The pre-fix + // code's own comment on `verifyParentWithinWorkspace` + // acknowledged this as "documented residual risk"; wenshao + // pushed back that this exceeds the Stage-1 trust model. + // + // Truncation now happens AFTER `verifyParentWithinWorkspace` + // succeeds, via `fh.truncate(0)` on the fd we already hold. + // fd-based truncate does NOT re-resolve the path, so an + // attacker swapping the parent symlink after we open can't + // redirect the truncation. + // + // #4297 fold-in 10 (qwen-latest S3, addresses #3263954697): + // `O_NOFOLLOW ?? 0` matches the defensive pattern in + // `core/src/utils/{sessionStorageUtils,gitDiff}.ts` and + // `cli/src/ui/utils/customBanner.ts` for platforms that + // don't expose the constant. Functionally a no-op (JS + // bitwise coerces `undefined` to 0) but keeps the codebase + // consistent for the next greppy refactor. fh = await fs.open( target, - // #4297 fold-in 10 (qwen-latest S3, addresses #3263954697): - // `O_NOFOLLOW ?? 0` matches the defensive pattern in - // `core/src/utils/{sessionStorageUtils,gitDiff}.ts` and - // `cli/src/ui/utils/customBanner.ts` for platforms that - // don't expose the constant. Functionally a no-op (JS - // bitwise coerces `undefined` to 0) but keeps the codebase - // consistent for the next greppy refactor. - fsConstants.O_WRONLY | - fsConstants.O_TRUNC | - (fsConstants.O_NOFOLLOW ?? 0), + fsConstants.O_WRONLY | (fsConstants.O_NOFOLLOW ?? 0), ); } catch (err) { const code = (err as { code?: unknown } | null | undefined)?.code; @@ -3092,6 +3109,14 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { 'overwrite', fh, ); + // #4297 post-merge wenshao Critical fold-in (folded into F1 + // #4319): truncate AFTER verify, using the fd we already + // hold. fd-based truncate doesn't re-resolve the path, so + // an attacker who swaps the parent symlink between + // verifyParentWithinWorkspace and here can't redirect the + // truncation to an external file. See the open-flags + // comment above for the full O_TRUNC race analysis. + await fh.truncate(0); await fh.writeFile('', 'utf8'); } finally { await fh.close(); @@ -3370,7 +3395,18 @@ async function canonicalizeExistingAncestor( return await fs.realpath(current); } catch (err) { const code = (err as NodeJS.ErrnoException | null | undefined)?.code; - if (code !== 'ENOENT' && code !== 'ENOTDIR') throw err; + // #4297 post-merge wenshao S2 fold-in (folded into F1 #4319): + // also catch ELOOP — a circular symlink in the parent path + // (e.g., `a -> b`, `b -> a`) makes `fs.realpath` fail with + // ELOOP. Without this, that bubbles up as an unstructured + // HTTP 500 instead of the typed `WorkspaceInitSymlinkError` + // (400) the route handler expects from the workspace-init + // race detection family. Walking up the parent chain when + // ELOOP hits at a sub-component preserves the existing + // "walk to the deepest extant ancestor" contract. + if (code !== 'ENOENT' && code !== 'ENOTDIR' && code !== 'ELOOP') { + throw err; + } const parent = path.dirname(current); if (parent === current) throw err; current = parent; @@ -3429,9 +3465,20 @@ async function verifyParentWithinWorkspace( // Best-effort cleanup before throwing. We're already in a failure // path; ignore secondary errors so the original race-detection // throw isn't shadowed. + // + // #4297 post-merge wenshao Critical fold-in (folded into F1 #4319): + // do NOT `fs.unlink(target)`. After a parent-directory race the + // textual `target` path now resolves through the attacker's freshly- + // planted parent symlink to an external location — `fs.unlink` + // would happily delete whatever file exists at the attacker's + // chosen path, giving any local user with workspace write access + // an arbitrary-file-deletion primitive against the daemon's UID. + // The empty file we created at the pre-race location is harmless + // (0 bytes, inside the workspace we'd just verified). Leaving it + // there over deleting an arbitrary external file is the right + // safety trade. if (cleanup === 'create') { await fh.close().catch(() => {}); - await fs.unlink(target).catch(() => {}); } throw new WorkspaceInitSymlinkError( target,