Repository navigation
fix: repair the Windows and macOS test lane failures #9728
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 all commits
9ba0c81
13de53d
228fe12
d470f63
c13770e
e7f138a
b599554
7b8ffce
dcbc846
4c49041
680aae0
eaaedb7
82aabb9
ef3b51f
a8f137a
b888a42
7928543
1d59f56
3055f75
2328579
bc304fd
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 |
|---|---|---|
|
|
@@ -3182,8 +3182,15 @@ interface ActivePromptCall { | |
| } | ||
|
|
||
| function isOwnerOnlyDirectory(stats: Stats): boolean { | ||
| if (process.platform === 'win32') return false; | ||
| if (stats.isSymbolicLink() || !stats.isDirectory()) return false; | ||
| if (process.platform === 'win32') { | ||
|
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. [Suggestion] Follow-up on the accepted trade-off: the residual guarantee this decision rests on is itself void on the volumes this same PR calls unverifiable. The maintainer decision above keeps the win32 relaxation and names the remaining containment as "symlink/junction rejection via This PR adds On a Windows host whose Live conversations root sits on exFAT/FAT32 removable media or an SMB/mapped drive, every Cheapest fix consistent with the rest of the PR: guard those three comparisons with
Collaborator
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. Deferred (was deferred under the round-10 cap). Applying 中文说明延后(第 10 轮已按上限延后)。对 acpAgent 三处 dev/ino 比较( |
||
| // Node's fs.Stats exposes no ownership or permission bits on Windows, so | ||
|
Comment on lines
+3186
to
+3187
Collaborator
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. Re-verified at dcbc846: the trace stands — the win32 branch of
Recommendation on record: option 2 (fail-closed) unless the residual risk is explicitly accepted — it preserves the guard family's deny-what-cannot-be-verified invariant at the cost of the managed-relocation convenience on Windows only. No code change this round; awaiting the decision. 中文说明已在 dcbc846 上重新核实:追踪成立——
记录在案的建议:除非维护者明确接受残余风险,否则选择方案 2(失败即关闭)——它以仅失去 Windows 上的托管重定位便利为代价,保持该 guard 家族"无法验证即拒绝"的不变量。本轮未改动代码;等待决策。
Collaborator
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. Still escalated — no code change this round. Re-verified at HEAD eaaedb7: the win32 branch of 中文说明仍为升级状态——本轮无代码变更。已在 HEAD eaaedb7 复验:
Comment on lines
+3186
to
+3187
Collaborator
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. Disposition: escalated for maintainer decision — no code change this round; thread stays open. Code trace re-verified at this HEAD ( Two facts frame the decision: this PR introduced the flip — The options, as the escalation frames them:
Recommendation: (2) is the security-conservative default — it removes the bypass with zero regression against main; if Windows managed relocation is needed before an ACL mechanism exists, (3) with the ACL check as a fast follow-up, and (1) only with explicit maintainer sign-off on the residual risk. Which of the three should this PR land? 中文说明处理:升级给维护者决策——本轮不改代码,线程保持打开。 已在本 HEAD( 两个事实框定了决策:是本 PR 引入了这一翻转—— 升级所列的三个选项:
建议:(2) 是安全保守的默认项——它消除绕过且相对 main 零回归;若在 ACL 机制出现之前就需要 Windows 托管重定位,则选 (3) 并把 ACL 检查作为快速跟进;(1) 仅在维护者对残余风险明确签字时采用。本 PR 应落地三者中的哪一个?
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. Maintainer decision: accepted trade-off — no code change, closing this thread. The finding describes the weakening accurately; the disposition weighs the three available options:
The residual guarantee on win32 is narrower than POSIX but not empty: the relocation still requires a trusted private ACP parent, exactly one absolute allowed root, a direct-child target, symlink/junction rejection via This is also the trade-off
Collaborator
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. Escalating for an explicit maintainer call — this is a decision among three options, not a unilateral patch. The win32 branch returns 中文说明升级,等待维护者明确拍板——这是在三个选项之间做决定,而不是单方面的补丁。win32 分支对每个普通目录都返回 |
||
| // the POSIX mode/uid check has no equivalent here. Containment then rests | ||
| // on the structural checks around this predicate — symlink rejection and | ||
| // dev/ino identity across the realpath round trip — the same trade-off | ||
| // serve/live/discovery.ts already makes on this platform. | ||
| return true; | ||
| } | ||
| if (typeof process.getuid === 'function' && stats.uid !== process.getuid()) { | ||
| return false; | ||
| } | ||
|
|
||
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.
[Critical] Windows managed relocation bypasses folder trust without checking ACL ownership.
Failure scenario: On a multi-user Windows machine, a Live conversation root or direct child can grant another account write access through its ACL.
isOwnerOnlyDirectoryreturnstruefor every ordinary Windows directory, somanagedTrustAllowedbypasses trusted-folder approval and relocates the session into attacker-writable project content. The surrounding realpath and dev/ino checks establish identity, not exclusive write access.Witness: Not run — the decisive ACL condition requires a multi-account Windows environment unavailable on this Darwin host; the code trace confirms the unconditional
truereachesmanagedTrustAllowedand bypasses folder trust.Suggested fix: Keep the managed-trust bypass disabled on Windows unless ownership and write ACLs can be verified with a Windows-specific mechanism, or require the normal folder-trust path on Windows.
中文说明
失败场景: 在多用户 Windows 机器上,Live 会话根目录或其直接子目录可能通过 ACL 允许其他账户写入。这里对所有普通 Windows 目录都返回
true,导致managedTrustAllowed跳过可信文件夹确认,并把会话迁移到攻击者可写的项目内容中。周围的 realpath 与 dev/ino 检查只能确认目录身份,不能证明只有当前用户可写。验证: 未运行——关键 ACL 条件需要当前 Darwin 环境不具备的多账户 Windows;代码路径确认这个无条件
true会进入managedTrustAllowed并绕过 folder trust。建议修复: 除非使用 Windows 专用机制验证所有权和写入 ACL,否则不要在 Windows 上启用 managed-trust 绕过;或者在 Windows 上始终走正常的 folder-trust 流程。
— gpt-5.6-sol via Qwen Code /review (v0.21.14)
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.
Escalating for a maintainer decision — not implementing either suggested fix this round; thread left open.
Verified by code trace: on Windows
isOwnerOnlyDirectorynow returnstruefor any non-symlink directory (Node'sfs.Statsexposes no ownership/ACL bits on win32), and atrueresult feedsmanagedTrustAllowed, which skips the folder-trust check for Live managed relocation. Containment on Windows rests on the symlink-rejection arm (the relocation path usesfs.lstat) plus dev/ino identity across the TOCTOU windows. On a multi-user Windows host whose ACLs let another account write into the live-session root, these checks do not prove exclusive control — the failure scenario as described is traceable.Why neither suggested fix is actionable from this runner:
isOwnerOnlyDirectorygates the relocation itself (all five call sites reject onfalse), so this disables Live managed relocation on Windows entirely, undoing the behavior this PR restores and which the author's Windows verification run validated.icacls/Win32-based check cannot be developed or verified from this Linux runner (no Windows lane is available to this round).Context: the same platform trade-off is documented and made elsewhere in this codebase for daemon-managed paths —
serve/live/discovery.tsskips the POSIX owner/mode checks on win32, andmanaged-scratch-workspace.ts'svalidateRootStatsdoes the same.Question for the maintainer: accept and document the residual Windows ACL risk (status quo — relocation additionally requires a trusted private ACP parent, one absolute allowed root, and the lstat/dev-ino checks), disable managed relocation on Windows until a real ACL mechanism exists, or schedule a Windows ACL check (icacls/Win32 binding) as follow-up work on a Windows runner? Our recommendation is status quo + follow-up ACL work, but this is a risk-acceptance call we won't make unilaterally.
中文说明
升级给维护者决策——本轮不实施所建议的两个修复方向;线程保持打开。
代码追踪确认: 在 Windows 上
isOwnerOnlyDirectory现在对任何非符号链接目录返回true(Node 的fs.Stats在 win32 上不暴露属主/ACL 位),而true的结果会进入managedTrustAllowed,从而跳过 Live 托管重定位的 folder-trust 检查。Windows 上的容器性依赖符号链接拒绝分支(重定位路径使用fs.lstat)与 TOCTOU 窗口间的 dev/ino 同一性。在多用户 Windows 主机上,如果 ACL 允许其他账户写入 live 会话根目录,这些检查无法证明独占控制——所述失败场景可以追踪成立。为什么两个建议修复在本运行环境都不可执行:
isOwnerOnlyDirectory是重定位本身的门槛(五个调用点在返回false时都会拒绝),这等于在 Windows 上完全禁用 Live 托管重定位,撤销本 PR 恢复的、且作者已在 Windows 验证运行中确认过的行为。icacls/Win32 的检查无法在本 Linux 运行环境上开发或验证(本轮没有可用的 Windows lane)。背景: 代码库中其他守护进程托管路径已经记录并做出了同样的平台取舍——
serve/live/discovery.ts在 win32 上跳过 POSIX 属主/权限位检查,managed-scratch-workspace.ts的validateRootStats也是如此。给维护者的问题: 接受并在文档中记录 Windows ACL 残余风险(维持现状——重定位还额外要求受信任的私有 ACP 父进程、唯一绝对 allowed root 以及 lstat/dev-ino 检查),还是在真正的 ACL 机制出现之前在 Windows 上禁用托管重定位,或者把 Windows ACL 检查(icacls/Win32 绑定)安排为后续工作在 Windows 运行环境完成?我们的建议是维持现状 + 后续 ACL 工作,但这是风险接受决策,我们不会单方面做出。