Skip to content

fix(cli): reject symlinked screenshot paths on win32 in capture_screen_context #9847

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.

Sign up for GitHub

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

Merged
wenshao merged 5 commits into from
Aug 29, 2026
Merged

fix(cli): reject symlinked screenshot paths on win32 in capture_screen_context #9847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d44f873
fix(cli): reject symlinked screenshot paths on win32 in capture_scree…
zhou2024NAU Aug 24, 2026
4c890fd
test(cli): pin the dedicated symlink error message in capture_screen_…
zhou2024NAU Aug 24, 2026
bb8c46d
chore: re-trigger automatic review
zhou2024NAU Aug 24, 2026
4d44548
Merge branch 'main' into fix/windows-lane-posix-test-guards
zhou2024NAU Aug 24, 2026
f684fdb
Merge branch 'main' into fix/windows-lane-posix-test-guards
wenshao Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions packages/cli/src/acp-integration/live/capture-screen-context.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,29 @@ describe('CaptureScreenContextTool', () => {
},
);

it('reports the dedicated symlink error on every platform', async () => {
const target = await captureFile();
const link = join(target.directory, 'linked.png');
Comment on lines +101 to +103

@qwen-code-ci-bot qwen-code-ci-bot Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The sibling test above — rejects a symlink and deletes only the Host-provided link — is still gated it.skipIf(process.platform === 'win32') with the comment "The rejection relies on O_NOFOLLOW, which libuv ignores on win32". That rationale no longer holds: rejection now goes through the platform-agnostic lstat guard this PR adds, and this new test beside it runs the identical setup on every platform. The skip entered this branch through the merge with main (#9728, which silenced the then-failing test); with the guard in place the sibling test passes on win32 as written.

This matters because the sibling test is the only test asserting the cleanup semantics — the symlink's target survives and only the Host-provided link is unlinked. While it stays win32-skipped, nothing verifies those assertions on the very platform this PR hardens: a future change that regresses the finally-block unlink on win32 (resolving/realpath-ing the path before unlink and deleting the target instead of the link, or failing to remove the link) ships green on the Windows lane. The stale comment also invites a future maintainer to conclude win32 symlink rejection is untestable and re-introduce a skip or remove the guard.

Suggested fix: drop .skipIf(process.platform === 'win32') from the sibling test and rewrite its comment — e.g. "Rejection goes through the explicit lstat guard on every platform; O_NOFOLLOW is the POSIX TOCTOU backstop." Symlink creation is smoke-validated on this repo's self-hosted Windows pool (windows-runner-smoke.yml, "Verify symbolic links" step), and this PR's own new test already calls symlink() ungated. Alternatively, fold the sibling's two filesystem assertions into this test and delete the duplicate.

中文说明

上方相邻的用例 rejects a symlink and deletes only the Host-provided link 仍然被 it.skipIf(process.platform === 'win32') 跳过，且注释写着 "The rejection relies on O_NOFOLLOW, which libuv ignores on win32"。这个理由已经不再成立：拒绝逻辑现在走的是本 PR 新增的、与平台无关的 lstat 守卫，而旁边这个新用例已经在所有平台上运行完全相同的准备步骤。这个跳过是经由与 main 的合并（#9728，当时为了 silenced 失败的测试）进入本分支的；在守卫就位后，相邻用例在 win32 上按原样即可通过。

这一点很重要，因为相邻用例是唯一断言清理语义的测试——符号链接的目标文件保持完好、只有 Host 提供的链接本身被删除。只要它在 win32 上仍被跳过，在本 PR 所加固的这个平台上就没有任何测试验证这些断言：未来某个改动若在 win32 上破坏了 finally 块中的 unlink（例如先 resolve/realpath 再删除、从而删掉了目标文件而不是链接，或根本没有删掉链接），会在 Windows 流水线上绿灯通过。过时的注释还会误导后来的维护者，使其以为 win32 上无法测试符号链接拒绝，从而重新引入跳过或删除守卫。

建议修复：去掉相邻用例上的 .skipIf(process.platform === 'win32')，并改写其注释——例如 "Rejection goes through the explicit lstat guard on every platform; O_NOFOLLOW is the POSIX TOCTOU backstop."。本仓库自托管 Windows 资源池已通过冒烟流程验证可以创建符号链接（windows-runner-smoke.yml 的 "Verify symbolic links" 步骤），且本 PR 自己的新用例已经在不加门控的情况下调用 symlink()。也可以把相邻用例的两个文件系统断言合并进本用例，然后删除重复的用例。

— qwen3.8-max via Qwen Code /review (v0.22.0)

await symlink(target.path, link);
const tool = new CaptureScreenContextTool(
async () => ({
appName: 'Finder',
accessibilityText: '',
screenshotPath: link,
}),
target.directory,
);

const result = await tool.build({}).execute(new AbortController().signal);

// The exact message pins the explicit lstat guard: without it, win32
// reads through the link (no error at all) while POSIX falls back to
// O_NOFOLLOW's generic ELOOP message.
expect(result.error?.message).toBe(
'Host returned a symbolic link screenshot path.',
);
});

it('rejects a screenshot outside the Host private directory', async () => {
const outside = await captureFile();
const allowed = await mkdtemp(join(tmpdir(), 'capture-screen-allowed-'));
Expand Down
7 changes: 6 additions & 1 deletion packages/cli/src/acp-integration/live/capture-screen-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { constants } from 'node:fs';
import { open, unlink } from 'node:fs/promises';
import { lstat, open, unlink } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { dirname, join, resolve } from 'node:path';
import {
Expand Down Expand Up @@ -52,6 +52,11 @@ function resolvePrivatePngPath(path: string, captureDirectory: string): string {
}

async function readPrivatePng(path: string): Promise<Buffer> {
// Windows silently ignores O_NOFOLLOW, so a symlinked screenshot path
// would be followed and read on win32. Probe the link itself first.
if ((await lstat(path)).isSymbolicLink()) {
throw new Error('Host returned a symbolic link screenshot path.');

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The new win32 symlink guard is not gated by any test: the pre-existing symlink test asserts only expect(result.error?.message).toBeTruthy(), and on the Linux PR-gate runners open(..., O_NOFOLLOW) throws ELOOP for a symlinked path anyway, so deleting the four added probe lines keeps the whole suite green. A future change that removes or breaks the win32 probe would pass PR CI silently and surface only in the post-approval Windows merge-queue job, re-opening the issue 9481 failure cluster this PR fixes.

Probe evidence (Linux scratch tree, this review):

  • probe deleted → symlink test still green via the ELOOP fallback (Test Files 3 passed (3)), error='ELOOP: too many symbolic links encountered', truthy assertion passes
  • strengthened assertion → red on the mutant: expected 'ELOOP: too many symbolic links encoun…' to be 'Host returned a symbolic link screens…'

Strengthen the assertion in capture-screen-context.test.ts so removing the probe fails on every platform:

expect(result.error?.message).toBe(
  'Host returned a symbolic link screenshot path.',
);
中文说明

新的 win32 符号链接守卫没有任何测试把关：现有的符号链接测试只断言 expect(result.error?.message).toBeTruthy()，而在 Linux PR 门禁的 runner 上，无论是否有新增的探测代码，open(..., O_NOFOLLOW) 都会对符号链接路径抛出 ELOOP，因此删除新增的 4 行探测代码后整个测试套件依然是绿的。未来若有改动移除或破坏了 win32 探测，PR CI 会静默放行，只有批准后的 Windows 合并队列任务才会发现，等于重新打开本 PR 修复的 issue 9481 失败簇。

探测证据（Linux 草稿树，本次审查）：

  • 删除探测代码 → 符号链接测试经 ELOOP 兜底仍然通过（Test Files 3 passed (3)），truthy 断言通过；
  • 加强断言后 → 突变体变红：expected 'ELOOP: too many symbolic links encoun…' to be 'Host returned a symbolic link screens…'

在 capture-screen-context.test.ts 中加强断言，使删除探测代码在所有平台上都会让测试失败：

expect(result.error?.message).toBe(
  'Host returned a symbolic link screenshot path.',
);

— qwen3.8-max via Qwen Code /review (v0.22.0)

}
const handle = await open(path, constants.O_RDONLY | constants.O_NOFOLLOW);
try {
const stat = await handle.stat();
Expand Down
Loading