Repository navigation
fix(acp-bridge): map Windows-shaped workspace paths to their sandbox mount #7228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
1dd5393
4ab9f67
3046e3b
59f11f0
b646a41
60705e1
e9c3b00
3675d96
88a5fc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2025 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
| import { makeBridge, makeChannel } from './internal/testUtils.js'; | ||
| import { _setSandboxMountExistsForTest } from './workspacePaths.js'; | ||
|
|
||
| // #7139 wiring for the bridge ingestion site: `resolveWorkspaceKey` guards | ||
| // with `path.isAbsolute` before canonicalizing, so a Windows-shaped | ||
| // `workspaceCwd` arriving via spawnOrAttach (clients, persisted | ||
| // registrations) must be translated to its bind mount first — the sibling | ||
| // of the dispatch/request-helpers/route/boot-validator wiring tests. | ||
| describe('bridge spawnOrAttach inside a POSIX container sandbox (#7139)', () => { | ||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); | ||
| _setSandboxMountExistsForTest(undefined); | ||
| }); | ||
|
|
||
| it.skipIf(process.platform === 'win32')( | ||
| 'accepts a Windows-shaped workspaceCwd bound to its mount location', | ||
| async () => { | ||
| vi.stubEnv('SANDBOX', 'qwen-code-sandbox-0'); | ||
| _setSandboxMountExistsForTest((p) => p === '/c/qwen-repro'); | ||
| const handle = makeChannel(); | ||
| const bridge = makeBridge({ | ||
| boundWorkspace: '/c/qwen-repro', | ||
| channelFactory: vi.fn().mockResolvedValue(handle.channel), | ||
| }); | ||
| try { | ||
| const session = await bridge.spawnOrAttach({ | ||
| workspaceCwd: 'C:\\qwen-repro', | ||
| }); | ||
| expect(session.sessionId).toBeTruthy(); | ||
| } finally { | ||
| await bridge.shutdown(); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| it.skipIf(process.platform === 'win32')( | ||
| 'still rejects a Windows-shaped workspaceCwd outside a sandbox', | ||
| async () => { | ||
| vi.stubEnv('SANDBOX', ''); | ||
| const handle = makeChannel(); | ||
| const bridge = makeBridge({ | ||
| boundWorkspace: '/c/qwen-repro', | ||
| channelFactory: vi.fn().mockResolvedValue(handle.channel), | ||
| }); | ||
| try { | ||
| await expect( | ||
| bridge.spawnOrAttach({ workspaceCwd: 'C:\\qwen-repro' }), | ||
| ).rejects.toThrow('workspaceCwd must be an absolute path'); | ||
| } finally { | ||
| await bridge.shutdown(); | ||
| } | ||
| }, | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,7 +82,10 @@ import { | |
| SessionBusyError, | ||
| InvalidRewindTargetError, | ||
| } from './bridgeErrors.js'; | ||
| import { canonicalizeWorkspace } from './workspacePaths.js'; | ||
| import { | ||
| canonicalizeWorkspace, | ||
| translateWindowsWorkspaceForPosixSandbox, | ||
| } from './workspacePaths.js'; | ||
| import { parseSessionSource } from './session-source.js'; | ||
| import { | ||
| CHANNEL_STARTUP_PROFILE_META_KEY, | ||
|
|
@@ -2845,7 +2848,12 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { | |
| return entry.transportClosedReject; | ||
| }; | ||
|
|
||
| const resolveWorkspaceKey = (workspaceCwd: string): string => { | ||
| const resolveWorkspaceKey = (rawWorkspaceCwd: string): string => { | ||
| // #7139: host-shaped Windows paths reach the in-container bridge via | ||
| // clients and persisted registrations; map to the bind mount before | ||
| // the absolute-path guard (no-op outside a POSIX container sandbox). | ||
| const workspaceCwd = | ||
| translateWindowsWorkspaceForPosixSandbox(rawWorkspaceCwd); | ||
| if (!path.isAbsolute(workspaceCwd)) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in 3675d96 — the ordering now has a single enforcement point: 中文:已在 3675d96 落地——顺序收敛到唯一强制点 |
||
| throw new Error( | ||
| `workspaceCwd must be an absolute path; got "${workspaceCwd}"`, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2025 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
| import { canonicalizeWorkspace } from './workspacePaths.js'; | ||
|
|
||
| // Isolated from workspacePaths.test.ts because it mocks node:fs — the other | ||
| // file drives real filesystem fixtures. | ||
| vi.mock('node:fs', async (importOriginal) => { | ||
| const actual = (await importOriginal()) as typeof import('node:fs'); | ||
| return { | ||
| ...actual, | ||
| existsSync: (p: unknown) => | ||
| p === '/c/qwen-repro' ? true : actual.existsSync(p as never), | ||
| }; | ||
| }); | ||
|
|
||
| describe('canonicalizeWorkspace inside a POSIX container sandbox (#7139)', () => { | ||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); | ||
| }); | ||
|
|
||
| it.skipIf(process.platform === 'win32')( | ||
| 'resolves a Windows-shaped workspace to its bind-mount location', | ||
| () => { | ||
| vi.stubEnv('SANDBOX', 'qwen-code-sandbox-0'); | ||
| // The mount exists (mocked), realpath on it ENOENTs on this host, so | ||
| // the fallback returns the translated absolute path — NOT the cwd | ||
| // concatenation `path.resolve` alone would produce. | ||
| expect(canonicalizeWorkspace('C:\\qwen-repro')).toBe('/c/qwen-repro'); | ||
| }, | ||
| ); | ||
|
|
||
| it.skipIf(process.platform === 'win32')( | ||
| 'keeps Windows-shaped input untouched outside a sandbox', | ||
| () => { | ||
| // Explicitly clear SANDBOX: when this suite itself runs inside the | ||
| // project's Docker sandbox the launcher sets it, and unstubAllEnvs | ||
| // would not remove a genuinely inherited value. | ||
| vi.stubEnv('SANDBOX', ''); | ||
| const result = canonicalizeWorkspace('C:\\qwen-repro'); | ||
|
Comment on lines
+37
to
+44
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 60705e1 — the outside-a-sandbox tests (both this file and the REST-helper sibling) now stub |
||
| // Unsandboxed POSIX behavior is unchanged: the string resolves | ||
| // relative to the cwd (and stays broken — which is what the | ||
| // pre-#7139 sandbox path produced too). | ||
| expect(result.endsWith('C:\\qwen-repro')).toBe(true); | ||
| expect(result.startsWith('/')).toBe(true); | ||
| }, | ||
| ); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,78 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { realpathSync } from 'node:fs'; | ||
| import { existsSync, realpathSync } from 'node:fs'; | ||
| import * as path from 'node:path'; | ||
|
|
||
| const WINDOWS_ABSOLUTE_PATH_RE = /^([A-Za-z]):[\\/](.*)$/; | ||
|
|
||
| /** | ||
| * Maps a Windows-shaped absolute path to the container mount produced by the | ||
| * host-side sandbox launcher (`C:\work\proj` → `/c/work/proj`, mirroring | ||
| * `getContainerPath` in `cli/src/utils/sandbox.ts`). | ||
| * | ||
| * A Windows host relaunching `qwen serve` into a Linux Docker/Podman sandbox | ||
| * translates the bind mount and `--workdir`, but path-valued CLI arguments | ||
| * (`--workspace C:\…`), client-registered workspaces, and persisted | ||
| * registrations reach the in-container daemon in host shape. Left alone, | ||
| * `path.resolve` on Linux mangles them further (prepends the cwd) and every | ||
| * ACP child spawn fails with `chdir(2) ENOENT` before running anything | ||
| * (#7139). | ||
| * | ||
| * Deliberately conservative — the input is returned unchanged unless ALL of: | ||
| * - the daemon is running on POSIX inside a container sandbox (`SANDBOX` env | ||
| * set by the launcher; macOS `sandbox-exec` does not remap paths and is | ||
| * excluded), | ||
| * - the path is Windows-absolute (`<drive>:\…` or `<drive>:/…`), | ||
| * - the translated candidate actually exists (i.e. the drive really is | ||
| * mounted the way the launcher mounts workspaces). | ||
| * | ||
| * The `opts` seams exist for tests; production callers use the defaults. | ||
| */ | ||
| // Test-only override for the mount-existence probe: the translated target | ||
| // (`/c/…`) sits at the filesystem root, which tests cannot create, and | ||
| // cross-package node:fs mocks don't reach this module's binding. Follows | ||
| // the `_reset*ForTest` convention. | ||
| let sandboxMountExistsOverrideForTest: ((p: string) => boolean) | undefined; | ||
| export function _setSandboxMountExistsForTest( | ||
| fn?: (p: string) => boolean, | ||
| ): void { | ||
| sandboxMountExistsOverrideForTest = fn; | ||
| } | ||
|
|
||
| export function translateWindowsWorkspaceForPosixSandbox( | ||
| p: string, | ||
| opts: { | ||
| platform?: NodeJS.Platform; | ||
| sandboxEnv?: string | undefined; | ||
| exists?: (candidate: string) => boolean; | ||
| } = {}, | ||
| ): string { | ||
| const platform = opts.platform ?? process.platform; | ||
| const sandboxEnv = | ||
| 'sandboxEnv' in opts ? opts.sandboxEnv : process.env['SANDBOX']; | ||
| const exists = opts.exists ?? sandboxMountExistsOverrideForTest ?? existsSync; | ||
| if (platform === 'win32' || !sandboxEnv || sandboxEnv === 'sandbox-exec') { | ||
| return p; | ||
| } | ||
| const match = WINDOWS_ABSOLUTE_PATH_RE.exec(p); | ||
| if (!match) return p; | ||
| const translated = `/${match[1]!.toLowerCase()}/${match[2]!.replace(/\\/g, '/')}`; | ||
| // `..` segments would let the existence probe resolve outside the drive | ||
| // mount (`C:\..\..\etc` -> existsSync('/c/../../etc') === true via /etc), | ||
| // making the "translated candidate actually exists" contract a lie. Not a | ||
| // privilege escalation (downstream workspace binding still rejects it), | ||
| // but refuse to translate anything that escapes the drive prefix. | ||
| const resolvedTranslated = path.resolve(translated); | ||
| if ( | ||
| resolvedTranslated !== `/${match[1]!.toLowerCase()}` && | ||
| !resolvedTranslated.startsWith(`/${match[1]!.toLowerCase()}/`) | ||
| ) { | ||
| return p; | ||
| } | ||
| return exists(resolvedTranslated) ? translated : p; | ||
| } | ||
|
|
||
| /** | ||
| * Canonicalize a workspace path so the boot-time bound path and every | ||
| * request's `workspaceCwd` collapse to the same key. `path.resolve` | ||
|
|
@@ -40,7 +109,11 @@ import * as path from 'node:path'; | |
| * at the original location. | ||
| */ | ||
| export function canonicalizeWorkspace(p: string): string { | ||
| const resolved = path.resolve(p); | ||
| // #7139: inside a Linux container sandbox, host-shaped Windows workspace | ||
| // paths must be mapped to their bind-mount location BEFORE resolution — | ||
| // `path.resolve('C:\\x')` on POSIX treats the whole string as relative | ||
| // and prepends the cwd. | ||
| const resolved = path.resolve(translateWindowsWorkspaceForPosixSandbox(p)); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in 3046e3b — the finding was exactly right: I verified
Also added the wiring test you asked for: 中文:发现完全正确——已核实五处调用点都在 |
||
| try { | ||
| // FIXME(stage-2): switch to `fs.promises.realpath` once the | ||
| // bridge call sites become async-friendly. This sync syscall | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2025 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
| import { _setSandboxMountExistsForTest } from '@qwen-code/acp-bridge/workspacePaths'; | ||
| import { parseOptionalWorkspaceCwd } from './dispatch.js'; | ||
|
|
||
| // #7139 wiring: the ACP JSON-RPC `cwd` entry point must translate a | ||
| // Windows-shaped path to its bind mount BEFORE its absolute-path guard — | ||
| // this is the dispatch-side sibling of request-helpers.sandbox.test.ts. | ||
| describe('ACP dispatch parseOptionalWorkspaceCwd inside a POSIX container sandbox (#7139)', () => { | ||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); | ||
| _setSandboxMountExistsForTest(undefined); | ||
| }); | ||
|
|
||
| it.skipIf(process.platform === 'win32')( | ||
| 'accepts a Windows-shaped cwd and returns its bind-mount location', | ||
| () => { | ||
| vi.stubEnv('SANDBOX', 'qwen-code-sandbox-0'); | ||
| _setSandboxMountExistsForTest((p) => p === '/c/qwen-repro'); | ||
| expect( | ||
| parseOptionalWorkspaceCwd({ cwd: 'C:\\qwen-repro' }, '/c/qwen-repro'), | ||
| ).toBe('/c/qwen-repro'); | ||
| }, | ||
| ); | ||
|
|
||
| it.skipIf(process.platform === 'win32')( | ||
| 'still rejects a Windows-shaped cwd outside a sandbox', | ||
| () => { | ||
| vi.stubEnv('SANDBOX', ''); | ||
| expect(() => | ||
| parseOptionalWorkspaceCwd({ cwd: 'C:\\qwen-repro' }, '/tmp'), | ||
| ).toThrow('`cwd` must be an absolute path when provided'); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in e9c3b00 —
bridge.sandbox.test.tsdrives the realspawnOrAttach(fake channel harness) withworkspaceCwd: 'C:\\qwen-repro'andboundWorkspace: '/c/qwen-repro': inside a sandbox (stubbedSANDBOX+ mount seam) the session resolves; outside one the exactworkspaceCwd must be an absolute pathrejection is pinned. All five ingestion sites now have wiring coverage. 中文:已补第五个入口的接线测试——真实 spawnOrAttach 经 resolveWorkspaceKey 守卫,沙箱内解析成功、沙箱外钉住绝对路径拒绝。五个入口全部覆盖。