vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-07-09 17:19:02 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 10

fix(acp-bridge): bridge.ts security fold-in from #4297 review (3 issues)

Browse source 
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)
This commit is contained in:
doudouOUC 2026-05-19 19:55:37 +08:00
parent c4bcd6da1f
commit 7bd66c6e85
1 changed files with 59 additions and 12 deletions
Download patch file Download diff file Expand all files Collapse all files

71
packages/acp-bridge/src/bridge.ts
View file

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