Repository navigation
[Bug][Windows] 36 of 48 pnpm test failures are suites creating symlinks without the Windows privilege
#5573
Replies: 2 comments
|
Excellent write-up — the isolation is rigorous and the The containment tests exercise filesystem identity, not just lexical paths
// containment.ts:42-44
function sameIdentity(left, right) {
return left.dev === right.dev && left.ino === right.ino
}The alias-equivalence test ( const realRoot = join(base, 'real')
const aliasRoot = join(base, 'alias')
await mkdir(realRoot)
await symlink(realRoot, aliasRoot) // → junction on win32
expect(await isPathUnder(join(await realpath(realRoot), 'missing', 'file.txt'), aliasRoot)).toBe(true)This works with The security-relevant edge case to budget a test for
function sameIdentity(left, right) {
return left.dev === right.dev && left.ino !== 0n && left.ino === right.ino
}This is orthogonal to your symlink fix (it's an identity-check gap, not a link-type gap), but it's the one place where For the rest — your split of — argszero |
|
Thanks — the 1. The logic gap is real
vi.mock('node:fs/promises', () => ({ stat: vi.fn() }))
vi.mocked(stat).mockImplementation(async () => ({ dev: 42n, ino: 0n }) as never)
await isPathUnder('/mnt/share/other-users/secret.txt', '/mnt/share/workspace', true)The same spec with per-entry inode numbers denies it correctly, and the root and its descendants stay admitted on the ino-less mount (they take the lexical fast path). So it degenerates exactly where you said: the ancestor walk's first existing entry compares equal to the root. Worth noting that 2. Your fix, verified, no regressions function sameIdentity(left: BigIntStats, right: BigIntStats): boolean {
- return left.dev === right.dev && left.ino === right.ino
+ return left.dev === right.dev && left.ino !== 0n && left.ino === right.ino
}
The alias-equivalence test keeps passing, which is the case that mattered: I also wrote it up as an Agent Note ( 3. Correction to the premise: I could not produce an
|
| measured on Windows 11 / NTFS | result |
|---|---|
| junction → non-existent target | created; lstat().isSymbolicLink() === true; readlink returns the absolute target |
| junction → itself | created; stat() then fails with ELOOP |
| junction → a file | ENOENT (matches the Node docs: "Junction points on NTFS volumes can only point to directories") |
| junction → relative target | silently normalized to an absolute path |
So the dangling-link cases (fsio.spec.ts:179, filesystem.spec.ts:316 and :408, discovery.spec.ts:330, executor.spec.ts:137) and both ELOOP cases (fsio.spec.ts:245, :253) take 'junction' and keep running on Windows — my table had them as skipIf. Only a genuine file target, a relative link target, or a POSIX-only permission fixture has to be skipped. Measured on the full suite:
master |
with the suite fixes | |
|---|---|---|
| Tests | 48 failed / 16142 passed / 82 skipped | 20 failed / 16155 passed / 97 skipped |
EPERM symlink failures |
36 | 0 |
The 15 added skips are exactly the tests that cannot express their fixture on Windows; the other 21 now run and pass. None of the 20 remaining failures is in any file this change touches — they are the pwsh-resolution and contention classes, and that set moves by a few between runs (a noisier run of the same tree showed 27).
The precedent is also much stronger than the two call sites I cited: the repository already uses process.platform === 'win32' ? 'junction' : 'dir' in 31 places and it.skipIf(process.platform === 'win32') in over 20, and several of the affected files already carry the correct idiom on a different line — filesystem.spec.ts:385, spill-local.spec.ts:463, agent-instructions.spec.ts:443. That makes these oversights rather than deliberate choices, which is worth knowing before anyone reviews the batch.
7. Two things I would not fold together
You suggested folding the ino guard into the same change as the suite fixes. I would keep them apart, for two reasons that are the repository's own:
- AGENTS.md requires an Agent Note for a non-trivial change and exempts "purely mechanical or local" edits. The suite batch is mechanical;
sameIdentityalters a contract. Landing them together puts the security-semantics change under the mechanical batch's review bar and makes it hard to bisect later. - The guard needs its own coverage. The new conjunct is a branch, and
test:coverageis a per-file 100% gate onpackages/*/*/src. The suite fixes need no new test; the guard cannot land without one.
So: test(win32): create test symlinks the way Windows allows (16 files, 37 lines, test-only) and fix(fs-sandbox): deny containment when filesystem identity is unknown (src + spec + Agent Note), as two independent commits off master. Happy to paste either diff here, or the whole thing, in whatever form is easiest for you to consume — I know external PRs are closed.
The diffs
The guard itself, plus the contract comment that keeps it from being deleted as a redundant conjunct:
--- a/packages/fs/fs-sandbox/src/containment.ts
+++ b/packages/fs/fs-sandbox/src/containment.ts
@@ -39,8 +39,16 @@ async function statIfPresent(path: string): Promise<BigIntStats | undefined> {
}
}
+/**
+ * Whether two stats name the same filesystem entry. `ino` 0 is not an identity
+ * but the placeholder a mount reports when it supplies none, so it answers
+ * "unknown" and refuses the match — every entry on such a volume would
+ * otherwise compare equal to the root and admit a path outside it. File IDs are
+ * documented as file-system-specific, reusable over time, and not unique on
+ * ReFS, so equality here is a match, never a proof.
+ */
function sameIdentity(left: BigIntStats, right: BigIntStats): boolean {
- return left.dev === right.dev && left.ino === right.ino
+ return left.dev === right.dev && left.ino !== 0n && left.ino === right.ino
}
/**
@@ -53,7 +61,10 @@ function sameIdentity(left: BigIntStats, right: BigIntStats): boolean {
* @param root - canonical writable root.
* @param caseSensitive - whether lexical comparison preserves case; defaults
* to the host filesystem convention used by supported platforms.
- * @returns whether the target is the root or a descendant of it.
+ * @returns whether the target is the root or a descendant of it. A mount that
+ * supplies no inode number makes identity unknown rather than equal, and an
+ * unknown identity denies: an alias-equivalent root is not recognized there,
+ * which is the direction a containment check must fail.
*/
export async function isPathUnder(
path: string,And the spec that pins it. It is mocked rather than filesystem-driven because the behavior needs a mount no CI host has, and it is what supplies the new conjunct's branch coverage:
// packages/fs/fs-sandbox/tests/containment-ino.spec.ts
import { describe, expect, it, vi } from 'vitest'
import { join, sep } from 'node:path'
vi.mock('node:fs/promises', () => ({ stat: vi.fn() }))
const { stat } = await import('node:fs/promises')
const { isPathUnder } = await import('../src/containment.ts')
/** Every path exists, one volume, and the filesystem supplies no inode number. */
function inoZeroMount(): void {
vi.mocked(stat).mockImplementation(async () => ({ dev: 42n, ino: 0n }) as never)
}
/** Same mount, but with real per-entry inode numbers (the NTFS/ext4 case). */
function realInoMount(): void {
let next = 1n
const assigned = new Map<string, bigint>()
vi.mocked(stat).mockImplementation(async (path: unknown) => {
const key = String(path)
if (!assigned.has(key)) assigned.set(key, next++)
return { dev: 42n, ino: assigned.get(key) } as never
})
}
const root = `${sep}mnt${sep}share${sep}workspace`
const outside = join(`${sep}mnt${sep}share`, 'other-users', 'secret.txt')
describe('isPathUnder identity fallback on an ino-less filesystem', () => {
it('denies an outside path when the filesystem supplies inode numbers', async () => {
realInoMount()
expect(await isPathUnder(outside, root, true)).toBe(false)
})
it('denies an outside path when every entry reports ino 0', async () => {
inoZeroMount()
expect(await isPathUnder(outside, root, true)).toBe(false)
})
it('still admits the root itself and its descendants on such a mount', async () => {
inoZeroMount()
expect(await isPathUnder(root, root, true)).toBe(true)
expect(await isPathUnder(join(root, 'file.txt'), root, true)).toBe(true)
})
})The suite batch is 37 lines of the same two shapes repeated, so it is not worth pasting whole; every hunk is either
- await symlink(realRoot, linkRoot)
+ await symlink(realRoot, linkRoot, process.platform === 'win32' ? 'junction' : 'dir')or
- it('reports symlinks without following them', async () => {
+ it.skipIf(process.platform === 'win32')('reports symlinks without following them', async () => {Both commits sit on master as git format-patch output; say the word and I will paste either one in full, or just the file list with the per-hunk classification.
One caveat on what I verified: I ran the unit suite, the fs coverage gate for the touched package, oxlint over the touched packages, and the doc gates listed above. I did not run the full typecheck / lint gates, which need the host build first — so treat those two as unverified on my side.
One ask
You mentioned the ino issue was "previously reported as a separate finding" — I searched Discussions for sameIdentity, ino, and containment-escape wording and only found this thread. If you have the link, could you drop it here? I would rather add these measurements to that thread than split the topic, and it would also tell me whether the POSIX-side carrier is already pinned down there.
中文摘要
sameIdentity 那条是这个帖子里更值钱的一半。我复现了、验证了修法，并顺带得到三个结果：一处前提纠正、一处对我自己表格的纠正，和一个我认为该先回答的问题。
- 逻辑缺陷是真的：mock
node:fs/promises模拟「路径都存在、同一卷、ino: 0n」的挂载 →isPathUnder(沙箱外路径, root)返回true。 - 修法已验证零回归：加
left.ino !== 0n后 containment 两个 spec 8/8（不加则 1 挂 7 过）；fs-sandbox+fs-local+tool-fs全跑，非 EPERM 失败为零。另按 AGENTS.md 补了implemented/bug-fix/的双语 Agent Note，六个文档门禁全绿。 - 前提要纠正：Windows 11 / NTFS 上我造不出
ino=0。Windows 主机提供的 SMB 共享返回与本地完全相同的 dev/ino；FAT 的 file ID 按文档由「目录首簇 + 条目字节偏移」推导，会变但非零。可信载体更可能在 POSIX 侧（st_ino0 占位、不合成 inode 的 FUSE）。这不削弱守卫，但改变了「谁能复现」。 - 同一页文档还说了守卫盖不住的事：「File IDs are not guaranteed to be unique over time」+「The 64-bit identifier ... is not guaranteed to be unique on ReFS」。
ino就是那个 64 位标识，而 Windows 11 Dev Drive 默认就是 ReFS —— 恰好是开发者会放工作区的地方。ino !== 0n关掉全零退化，关不掉 64 位 ID 碰撞。我没有 ReFS 卷，这条按文档报，不声称有可复现的逃逸。 - 身份回落在生产里到底还承重吗：
writableRoots让根过realpathSync.native（roots.ts:36），checkedTarget让目标过resolve（index.ts:115）—— 而realpath.native本身就解 8.3 短名、junction 和大小写，正是该回落注释自称的三个理由。若生产中无人走到它，那删掉这个与安全相关的相等判定，严格优于加固它，还顺带消掉 ReFS 那个问题。我没删，因为「我找不到调用方」不等于「没有」。 - 我自己的表格错了三行，而且是朝「多跳过」的方向错：实测 junction 可指向不存在的目标（
isSymbolicLink=true）、可自指（stat()抛 ELOOP）、不能指向文件（ENOENT）、相对目标会被规范成绝对路径。所以悬空链接和两个 ELOOP 用例都能用 junction、继续在 Windows 上跑。另外仓内既有 31 处 junction、20+ 处skipIf，且filesystem.spec.ts:385、spill-local.spec.ts:463、agent-instructions.spec.ts:443就在受影响文件里的另一行已经写对了 —— 这说明是疏漏而非有意。 - 两件不建议合在一起做：AGENTS.md 要求非平凡改动带 Agent Note、而「纯机械/局部」改动豁免 —— 套件批是机械的，
sameIdentity改的是契约；且新合取项是一个分支，test:coverage是逐文件 100% 门禁，守卫不带测试落不了地。已拆成两个基于master的独立提交。
Uh oh!
There was an error while loading. Please reload this page.
Summary
On a clean Windows 11 checkout of
master,pnpm testreports 48 failed / 16142 passed (21 failed files). 36 of those 48 failures, across 16 suites, are one single cause — and it is not a product defect and not flakiness.Those suites call
fs.symlink()/symlinkSync()with notypeargument. On Windows that requests a real symbolic link, which needsSeCreateSymbolicLinkPrivilege(Developer Mode, or an elevated shell). Without it Node fails withEPERMin the arrange step, before the test body ever reaches product code.The repository already contains both correct patterns — they are simply not used in these sixteen suites:
packages/test-support/session-snapshot/src/launcher.ts:405—symlinkSync(packageDir, link, process.platform === 'win32' ? 'junction' : 'dir')packages/test-support/session-snapshot/tests/harness.spec.ts:242— same junction formpackages/sandbox/sandbox-policy/tests/policy.spec.ts:90—it.skipIf(process.platform === 'win32')for a test that genuinely needs POSIX symlink semanticsSo this is an inconsistency inside the test suite, not an open design question.
Environment
76fda7297(master)pnpm install→npx vitest runRoot cause, isolated
A standalone probe on the same host, no repository code involved:
Two consequences:
type: 'junction'— no privilege required, and it resolves throughrealpaththe way the POSIX symlink does.win32— exactly whatsandbox-policy/tests/policy.spec.ts:90already does.type: 'dir'does not help: it requests a real symbolic link too.Affected suites (36 failures / 16 files)
packages/fs/fs-local/tests/fsio.spec.ts:64 :83 :157 :179 :202 :206 :245 :253 :263chmod 0o000caseskipIffor the POSIX-semantics onespackages/fs/fs-local/tests/filesystem.spec.ts:172 :316 :408 :757 :769skipIf(win32)packages/spill/spill-local/tests/spill-local.spec.ts:303 :322 :558'junction'packages/lsp/lsp-stdio/tests/host.spec.ts:52 :105 :113'junction'packages/context/agent-instructions/tests/agent-instructions.spec.ts:471 :492 :3436AGENTS.md)skipIf(win32)packages/workspace/workspace/tests/workspace.spec.ts:211 :363'junction'— ✅ verified belowpackages/skill/skill-filesystem/tests/skill-filesystem.spec.ts:415–:418,:785.md,/dev/null)skipIffor the restpackages/fs/fs-sandbox/tests/fs-sandbox.spec.ts:122 :129'junction'packages/fs/fs-sandbox/tests/containment.spec.ts:37'junction'scripts/dev-web.spec.ts:94(node_modules)'junction'scripts/project-doc-site.spec.ts:160'junction'packages/util/atomic-write/tests/atomic-write.spec.ts:96skipIf(win32)packages/test-support/session-snapshot/tests/workspace.spec.ts:29'a.txt')skipIf(win32)— a junction must be absolutepackages/shell/pwsh-local/tests/executor.spec.ts:137.exe)skipIf(win32)packages/settings/settings-file/tests/local.spec.ts:205skipIf(win32)packages/preset/agent-presets/tests/discovery.spec.ts:330'junction'orskipIfVerified fix (one representative suite)
packages/workspace/workspace/tests/workspace.spec.ts, two lines, in the form already used bysession-snapshot/src/launcher.ts:That suite alone:
And re-running the whole suite with only those two lines changed moves the totals by exactly two, confirming nothing else shifted:
The alias-identity assertions are unchanged because the junction resolves through
realpaththe same way.typeis ignored on non-Windows, so POSIX behaviour is untouched.Why this is worth fixing
docs/development.md→ Prerequisites lists Node, Corepack pnpm and Git, and closes with "Setup is complete whenpnpm run typecheckexits successfully." A Windows contributor who follows that and then runspnpm testgets 21 red files and no way to separate an undocumented environment prerequisite from a regression they just introduced. Either resolution works — fix the call sites, or state the privilege requirement in the prerequisites — but silently requiring Developer Mode is the option that costs the most contributor time.The remaining 12 failures, for completeness
Separate causes; happy to split them into their own posts if that is more useful.
packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts(6) spawns a literalpwshand fails withwindows-acl-run: CreateProcessAsUserW failed (Win32 2)→ exit 127. Related:packages/shell/pwsh-sandbox/tests/sandbox.spec.ts:180assertsargv[0]matches/pwsh(\.exe)?$/, but on a host without PowerShell 7 the product correctly resolvesC:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe— the documented last-resort fallback inpwsh-local/src/resolve.ts:34-35. That assertion contradicts the product's own supported fallback rather than catching a defect.packages/subagent/subagent-claude-code/tests/real-product.spec.ts. Under the parallel suiteafterEachhitsHook timed out in 10000msand the max-turns case observes['success', 'error_max_turns']instead of['error_max_turns']. Run alone the file passes 8/8 in 40s, so this is contention, not a product defect.scripts/oxlint-contract.spec.ts,scripts/run-gates.spec.ts,scripts/client-build-environment.client.spec.ts, andpackages/llm/llm-pi-ai/tests/adapter.spec.ts. Together with (2) these are the class already reported in Windows: full test suite fails with default 5s timeouts under parallel execution #3947 / No testTimeout is configured, so Windows inherits a POSIX-calibrated 5s default that measures contention #4151 (Windows inherits a POSIX-calibrated defaulttestTimeoutunder parallel execution).None of the 36 symlink failures above are timeouts, so this report is distinct from #3947 / #4151.
中文摘要
Windows 11 干净检出跑
pnpm test：48 失败 / 16142 通过（21 个文件红），其中 36 个失败、16 个套件是同一个原因 —— 环境问题而非产品缺陷。这些用例调用
fs.symlink()时不传type，Windows 上这是申请真正的符号链接，需要SeCreateSymbolicLinkPrivilege（开发者模式或管理员）；普通用户直接EPERM，测试体根本没跑到产品代码。仓库里两种正确写法都已存在，只是这 16 个套件没用：目录别名传
type: 'junction'（见session-snapshot/src/launcher.ts:405），真正依赖 POSIX 语义的用it.skipIf(process.platform === 'win32')（见sandbox-policy/tests/policy.spec.ts:90）。注意type: 'dir'无效 —— 它同样申请真符号链接。已验证：只给
workspace/workspace/tests/workspace.spec.ts两行加 junction，该文件 2 失败 → 43 全绿，全量总数 48 → 46，无其它偏移。docs/development.md的先决条件只写了 Node / pnpm / Git，并声明「pnpm run typecheck通过即完成安装」。Windows 贡献者照做后跑测试拿到 21 个红文件，无法区分是环境缺配置还是自己改坏的。All reactions