fix(review): resolve included filters by origin - #10474
Conversation
|
Thanks for the PR! Template looks good ✓ — every section present and filled in. Problem: observed and well-evidenced. Issue #10441 documents the gap with a live-verified reproduction: Direction: aligned — this is the exact fix #10441 spells out, and the next step in a maintainer-driven chain (#9558 → #10421 → round-1 review finding R1-1 → this issue). Size: no core-module paths touched. 95 production lines ( Approach: minimal, and it matches the issue spec point-for-point — enter with Risk: no elevated-risk path matches. One residual include variant worth maintainer eyes — detailed in the code review comment. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 各部分齐全且内容充实。 问题：已观测到且证据充分。Issue #10441 记录了该漏洞并附有实测复现： 方向：对齐 —— 这正是 #10441 明确指定的修复方式，也是维护者主导的链条（#9558 → #10421 → 第一轮 review 发现 R1-1 → 本 issue）的下一步。 规模：未触及核心模块路径。生产代码 95 行（ 方案：最小化，且与 issue 规格逐点对应 —— 以 风险：无高风险路径命中。有一种残留的 include 变体值得关注维护者注意 —— 详见代码审查评论。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRead the problem statement first and proposed my own fix before touching the diff: expand includes at read time, resolve each hit's origin with I verified the load-bearing assumptions against real git rather than taking the diff's word for them:
The tests are the two arms #10441 demanded, at both protected sites: a repo-local included smudge filter is refused with the key named and the canary never created (the canary pins order — refusal before checkout, not just instead of a verdict), and the same include shape resolving to the global config stays allowed — the git-lfs false-positive direction. Host git config is isolated per test. No critical blockers. One question for the maintainer (not a blocker — outside #10441's stated scope). The screen evaluates sequenceDiagram
participant C as Checkout caller (scratch reset, probe restore)
participant S as localFilterCommands
participant G as git config
C->>S: screen before checkout
S->>G: --file, --includes, --show-origin, --null, get-regexp filter commands
G-->>S: origin, key, value pairs (includes followed)
S->>S: realpath each origin, containment vs common dir and worktree gitdir
alt origin is repo-local
S-->>C: refuse and name the key
else origin is global or outside
S-->>C: pass, checkout proceeds
else origin unresolvable or read incomplete
S-->>C: refuse via unreadable path (fail closed)
end
TestingThis run carries CI evidence via API only (unattended run — PR code is never executed here), and the honest state is: the PR's own CI has not run yet. No
Sandboxed verification would settle the behavioural claim: 中文说明代码审查先读问题描述、在看 diff 之前提出自己的方案：读取时展开 include、用 关键假设我用真实 git 验证过，而不是只信 diff：
测试覆盖了 #10441 要求的两个方向、两个受保护调用点：仓库本地 include 中的 smudge 过滤器被拒绝、报出键名、canary 从未被创建（canary 钉住了顺序 —— 在 checkout 之前拒绝，而不只是代替结论）；同样的 include 结构指向全局配置时继续放行 —— 即 git-lfs 误报方向。每个测试都隔离了宿主 git 配置。 无关键阻塞项。 一个留给维护者的问题（非阻塞 —— 超出 #10441 的既定范围）。 筛查在 review worktree 的 gitdir 语境下评估 测试本次运行仅携带通过 API 获取的 CI 证据（无人值守运行 —— 不在此执行 PR 代码），而真实状态是：本 PR 自己的 CI 尚未运行。该 head 上没有任何 沙盒验证可以落实行为性结论： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — the code review is genuinely clean and the diff is exactly the fix #10441 specifies, but this run cannot supply what an approval would attest to: the fork's CI has not run at all, the PR is stacked on unmerged #10421 targeting its branch, and there is an open Going back to my independent proposal: the PR matches it point for point, and I did not find a simpler path it missed. The fail-closed discipline is the right instinct for a security screen, every degenerate parse lands on refusal, and the tests are precisely the two arms the issue demanded — with a canary that pins refusal before the checkout rather than merely instead of a verdict. If I were maintaining this in six months I would thank the author, not curse them. The problem it solves is real and was live-verified by the maintainer's own issue. So the hesitation is not about the code. It is about what approving would claim: that this is ready to merge, when (1) not one CI job of the PR's own suite has executed — the contribution is waiting for fork workflow approval, so the 184-test claim and the three-OS matrix are still the author's word from macOS; (2) the PR targets ⏸️ Deferring to @wenshao — you own #10441 and the parent #10421: this implements the issue's spec faithfully and the static review found no blockers, but it needs fork CI approved and run, the stacked-branch retargeting call, and your ruling on the residual 中文说明置信度：3/5 —— 代码审查确实干净，diff 也正是 #10441 指定的修复，但本次运行无法提供批准所应背书的东西：fork 的 CI 完全没有运行，本 PR 基于未合并的 #10421 并以其分支为目标，还有一个留给这条工作链负责人的 回到我的独立方案：PR 与之逐点对应，我也没找到它遗漏的更简路径。对安全筛查来说，fail-closed 的纪律是正确的本能，每一个退化解析都落入拒绝，测试也恰好是 issue 要求的两个方向 —— 并用 canary 钉住了在 checkout 之前拒绝、而不只是代替结论。如果六个月后由我来维护，我会感谢作者而不是抱怨。它解决的问题是真实的，且已由维护者自己的 issue 实测确认。 因此迟疑不在于代码，而在于批准所意味着的内容：（1）本 PR 自己的套件没有任何一个 CI 任务执行过 —— 贡献正等待 fork workflow 批准，184 个测试的结论与三平台矩阵目前仍是作者在 macOS 上的自述；（2）本 PR 的目标分支 ⏸️ 转交 @wenshao —— 您负责 #10441 与父 PR #10421：本 PR 忠实地实现了 issue 规格，静态审查未发现阻塞项，但需要先批准并运行 fork CI、决定堆叠分支的重定向，以及对残留的 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/commands/review/test-efficacy.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：src/commands/review/test-efficacy.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| '--null', | ||
| '--show-origin', | ||
| '--includes', |
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [new-surface] includeIf conditions are evaluated in the screening tree's context, not the authorized checkout's, so a conditionally included filter stays invisible to this screen and executes during the checkout the screen authorizes.
The screen spawns git config --file <common>/config --includes with cwd in the REVIEW worktree, but the checkout it authorizes (runScratchTree) runs in the SCRATCH worktree (<common>/worktrees/<basename>). Git evaluates includeIf.gitdir: conditions against the screening cwd's gitdir, so an include conditioned on the scratch tree's gitdir is invisible at screening time and expanded at checkout — the exact hole this PR closes for plain include.path survives for conditional includes. A probe plants the conditional include into the common dir with the same single write the screen's own doc names as the planting surface, and the scratch labels are constructed in-code (scratchWorktreePrefix) and enumerable/globbable (gitdir:<common>/worktrees/*-scratch-*). The probe-tree call sites are unaffected — screen and checkout share one tree there. A probe run against the unmodified PR certified the tree clean and handed back a path while the planted smudge executed during the authorized worktree add.
Witness:
F1: {"available":true, ..., "pwned":true} // includeIf keyed on the exact scratch gitdir
F1-GLOB: {"available":true, ..., "pwned":true} // includeIf "gitdir:<common>/worktrees/*-scratch-*"
CONTROL-INSIDE: {"available":false, "note":"...defines content filter(s) filter.included.smudge ..."}
Suggested fix — fail closed on repo-local conditional includes the screen cannot evaluate for the checkout's tree: additionally query each candidate file for ^includeif\..*\.path$ (without --includes) and route any hit into the existing unreadable refusal (flip-verified in the probe):
const conditional = spawnSync(
'git',
['config', '--file', file, '--get-regexp', '^includeif\\..*\\.path$'],
{ cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
);
if (conditional.status === 0) {
unreadable ??= file;
continue;
}Alternative: evaluate the include graph in each authorized worktree's context (untested). The refusal must keep allows a repo-local include whose filter is defined by global config (packages/cli/src/commands/review/scratch-tree.test.ts, added in this diff) green, and keep LocalFilterScreen.unreadable a single string (its doc: "One file, not a list"). Please add a regression test beside refuses a content filter reached through a repo-local include that plants includeIf.gitdir:<scratch tree gitdir> pointing at an .inc with a canary smudge plus the attributes line and asserts available === false with the canary untouched — then remove the new includeIf check and confirm that test reds.
中文说明
筛查命令以 REVIEW worktree 为 cwd 执行 git config --file <common>/config --includes，但它授权的 checkout（runScratchTree）运行在 SCRATCH worktree（<common>/worktrees/<basename>）中。Git 按筛查时 cwd 的 gitdir 来求值 includeIf.gitdir: 条件，因此以 scratch tree 的 gitdir 为条件的 include 在筛查时不可见、在 checkout 时却会展开——本 PR 为普通 include.path 堵上的漏洞，在条件式 include 上仍然存在。探针可以用 screen 文档自己指出的植入面（对 common dir 的一次写入）植入该条件式 include；scratch 标签由代码构造（scratchWorktreePrefix），可枚举、可通配（gitdir:<common>/worktrees/*-scratch-*）。probe-tree 的两个调用点不受影响——那里筛查与 checkout 共用同一棵树。针对未修改 PR 的探针运行显示：树被判定为干净并返回了路径，而植入的 smudge 在被授权的 worktree add 期间执行了。
建议修复：对筛查无法在 checkout 树的上下文中求值的仓库本地条件式 include 采取 fail-closed——对每个候选文件额外查询 ^includeif\..*\.path$（不需要 --includes），命中即走现有 unreadable 拒绝路径（已在探针中翻转验证）。备选方案：在每个被授权 worktree 的上下文中分别求值 include 图（未验证）。该拒绝必须保持 allows a repo-local include whose filter is defined by global config（packages/cli/src/commands/review/scratch-tree.test.ts，本 PR 新增）为绿色，并保持 LocalFilterScreen.unreadable 为单个字符串（其文档注明："One file, not a list"）。请在 refuses a content filter reached through a repo-local include 旁新增回归测试：植入指向含 canary smudge 的 .inc 文件的 includeIf.gitdir:<scratch tree gitdir> 及 attributes 行，断言 available === false 且 canary 未被创建——然后移除新增的 includeIf 检查，确认该测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Addressed in 5f1c17b using the alternative you outlined: evaluate the graph in the authorized worktree's real context. A fresh scratch tree is now registered with worktree add --no-checkout, screened via localFilterCommands(tree), and only then checked out, so no tracked file (and therefore no smudge input) exists before the screen. Reuse screens after the scratch identity gate and before its forced checkout. The new exact-scratch-gitdir includeIf regression asserts refusal, names filter.conditional.smudge, keeps the canary absent, and confirms the no-checkout registration is removed.
| if ( | ||
| !localRoots.some((root) => isWithinDirectory(root, canonicalOrigin)) | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-2: [certifies-falsely] [new-surface] The origin gate exempts ANY outside-root origin as "the user's contract", but the include directive routing there is attacker-writable — the screen fail-opens on the channel this PR exists to close.
The containment branch skips every hit whose canonical origin resolves outside the common dir and the worktree gitdir. The diff's rationale names the user's global config (git-lfs is the common case), but the include DIRECTIVE that routes to an outside origin sits in the repo-local config a probe can write, so the exemption authorizes arbitrary outside files — including the PR's own committed worktree content — not just the user's global/system config graph. One git config include.path <outside>/evil.inc write into the common dir routes the screen to an origin outside both roots; the key is silently dropped, keys stays empty, the checkout is authorized, and the planted filter executes. The identical plant with the target inside .git/ IS refused — the plant simply picks an outside target. Measured at the merge base: git config --file <common>/config --get-regexp '^filter…' without --includes exits 1 on the same plant, so the trigger bypassed the base too — a remaining gap in the new defense, not a regression.
Witness:
F2A: {"available":true, ..., "pwned":true} // include.path -> outside file, one write into the common dir
CONTROL: identical plant with an in-root target refused (available:false, key named)
Suggested fix — restrict the exemption to files reachable through the user's own config graph: before allowing an outside-root origin, confirm it is also reported by git config --global --list --show-origin --includes (same sanitized env); otherwise refuse it or treat it as unreadable. Equivalently, invert the gate: refuse any origin reached through a repo-local include directive unless it is in the user's global/system graph. The exemption must still permit a repo-local include of the user's actual global config — pinned by expect(run().available).toBe(true) in allows a repo-local include whose filter is defined by global config (packages/cli/src/commands/review/scratch-tree.test.ts) and by the git-lfs allowance in packages/cli/src/commands/review/test-efficacy.test.ts (expect(afterDetail).not.toContain('filter.lfs.clean')). Please add a test mirroring refuses a content filter reached through a repo-local include but with the include target OUTSIDE the gitdir (e.g. a mkdtemp dir or a committed worktree file), asserting available === false, the note names the key, and the canary never fires — then restore the blanket skip and confirm the test reds.
中文说明
包含关系分支会跳过所有规范化来源落在 common dir 与 worktree gitdir 之外的命中。diff 的理由说明指的是用户的全局配置（最常见的是 git-lfs），但把来源路由到外部的 include 指令本身位于探针可写的仓库本地配置中，因此该豁免实际上授权了任意外部文件——包括 PR 自己提交到工作树中的内容——而不仅仅是用户的全局/系统配置图。向 common dir 写入一次 git config include.path <outside>/evil.inc 即可把筛查路由到两个根之外的来源；该键被静默丢弃，keys 保持为空，checkout 被授权，植入的过滤器随之执行。目标在 .git/ 内部的相同植入会被正确拒绝——植入方只需选择一个外部目标即可。在 merge base 上实测：不带 --includes 的 git config --file <common>/config --get-regexp '^filter…' 对同一植入 exit 1，因此该触发在基线上同样能绕过——这是新防御上残留的缺口，而非回归。
建议修复：把豁免限制在用户自己的配置图可达的文件——在允许根外来源之前，确认该来源同样会被 git config --global --list --show-origin --includes（相同的净化环境）报告出来；否则拒绝或视为 unreadable。等价做法是反转判定门：凡是经仓库本地 include 指令到达的来源，若不在用户的全局/系统配置图中则拒绝。豁免仍须允许对用户真实全局配置的仓库本地 include——由 allows a repo-local include whose filter is defined by global config（packages/cli/src/commands/review/scratch-tree.test.ts）中的 expect(run().available).toBe(true) 与 packages/cli/src/commands/review/test-efficacy.test.ts 中的 git-lfs 豁免（expect(afterDetail).not.toContain('filter.lfs.clean')）钉住。请仿照 refuses a content filter reached through a repo-local include 新增测试，但 include 目标在 gitdir 之外（如 mkdtemp 目录或提交到工作树中的文件），断言 available === false、note 点名该键、canary 从未执行——然后恢复无差别跳过分支，确认测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Addressed in 5f1c17b. Outside-root no longer means user-owned: userFilterOrigins independently queries Git's --global and --system graphs with includes and reported origins, canonicalizes those origins, and exempts an outside hit only when that exact canonical file appears there. Arbitrary outside includes are now reported as filter keys. Regressions at both protected callers assert refusal and an untouched canary; the existing global/git-lfs controls remain green.
| if ( | ||
| !localRoots.some((root) => isWithinDirectory(root, canonicalOrigin)) | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-3: [certifies-falsely] [regression] A symlinked candidate config file screens clean — the pre-change parse loop refused this exact shape.
The origin gate also judges the candidate file ITSELF. When <common>/config (or a screened config.worktree) is a symlink pointing outside the local roots, git reports the --file path as the origin, realpathSync resolves it to the outside target, containment fails, and the filter keys are silently dropped — the screen returns {keys: [], unreadable: null} and the authorized checkout executes the filter. The readability pre-check does not catch this (statSync follows symlinks, matching git). The deleted parse loop (line.split(/\s+/)[0]) flagged every key git emitted regardless of origin, and the base invocation reads through the symlink — so this shape was refused before this diff. The symlink lives in the never-wiped common dir and persists into every later review.
Witness:
F2B: {"available":true, ..., "pwned":true} // <common>/config symlinked to an outside filter file
BASE: git config --file <symlinked .git/config> --get-regexp '^filter\..*\.(smudge|clean|process)$'
-> "filter.sym.smudge touch /tmp/PWNED-SYM", exit 0 // the deleted loop flagged this key
Suggested fix — flag hits whose reported origin is the candidate file itself (the path handed to --file) unconditionally and apply the containment judgement only to include-derived origins; or judge each candidate's own canonical path before trusting its keys, fail-closed:
let canonicalFile: string;
try {
canonicalFile = realpathSync(file);
} catch {
unreadable ??= file;
continue;
}
if (!localRoots.some((root) => isWithinDirectory(root, canonicalFile))) {
unreadable ??= file;
continue;
}The check must apply to candidate files' own origins, not to include-derived ones — it must not refuse include targets outside the local roots, the allowance pinned by allows a repo-local include whose filter is defined by global config (packages/cli/src/commands/review/scratch-tree.test.ts, added in this diff). Please add a scratch-tree.test.ts case that moves <common>/config outside the gitdir, symlinks it back, defines a smudge filter in the target, and asserts run().available === false with the canary never created — then revert the guard and confirm the test reds.
中文说明
来源判定门同样会评判候选文件本身。当 <common>/config（或被筛查的 config.worktree）是指向本地根之外的符号链接时，git 把 --file 路径作为来源上报，realpathSync 将其解析到外部目标，包含关系检查失败，过滤器键被静默丢弃——筛查返回 {keys: [], unreadable: null}，被授权的 checkout 随即执行该过滤器。可读性预检查拦不住这种情况（statSync 与 git 一致，会跟随符号链接）。被删除的解析循环（line.split(/\s+/)[0]）会标记 git 输出的每一个键、不判来源，且基线的调用会透过符号链接读取——因此该形态在本 diff 之前是被拒绝的。符号链接位于永不删除的 common dir 中，会持续影响之后每一次审查。
建议修复：对报告来源即候选文件本身（传给 --file 的路径）的命中无条件标记，仅对 include 派生的来源应用包含关系判定；或在信任候选文件的键之前先判定该文件自身的规范化路径，失败即走 fail-closed（见上方代码）。该检查只应用于候选文件自身的来源，不应用于 include 派生的来源——不得拒绝本地根之外的 include 目标，该豁免由 allows a repo-local include whose filter is defined by global config（packages/cli/src/commands/review/scratch-tree.test.ts，本 PR 新增）钉住。请在 scratch-tree.test.ts 中新增用例：将 <common>/config 移出 gitdir 并以符号链接指回，在目标文件中定义 smudge 过滤器，断言 run().available === false 且 canary 从未被创建——然后还原该守卫，确认测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Addressed in 5f1c17b. Each candidate is now canonicalized before its include graph is queried and must remain within the common/worktree gitdir roots; an outside-target candidate symlink enters the existing fail-closed unreadable path. The new real-Git regression moves <common>/config outside, symlinks it back, defines a smudge command through it, and verifies refusal before the canary can execute.
| function isWithinDirectory(parent: string, child: string): boolean { | ||
| const rel = relative(parent, child); |
There was a problem hiding this comment.
[Suggestion] R1-4: isWithinDirectory is the third copy of the package's containment helper.
It is a byte-for-body duplicate of isWithin in packages/cli/src/commands/review/save-artifact.ts:140, and both re-implement the exported isWithinRoot in packages/cli/src/config/path-comparison.ts:10 (imported at ~50 sites across the package). The codebase now carries three copies of a security-load-bearing containment test — the gate the two origin findings above depend on. When the boundary semantics need a change (e.g. the Windows case-insensitivity that core's isSubpath handles via its win32 branch but these copies do not), the fix must be applied to every copy, and a copy missed in that fix silently diverges: here a false "not within" verdict classifies a planted repo-local filter origin as outside and skips it, so the screen reports clean while the checkout executes the filter.
Witness:
witness: not run — the claim is a structural fact settled by direct comparison of the three function bodies; no behavioural claim to execute.
Suggested fix — delete isWithinDirectory (and the now-unneeded relative import) and use the canonical helper:
import { isWithinRoot } from '../../../config/path-comparison.js';
// ...
if (!localRoots.some((root) => isWithinRoot(canonicalOrigin, root))) {
continue;
}Core's isSubpath(root, canonicalOrigin) is an equally valid drop-in with the same (parent, child) order. Note the shared helper takes the child first: export function isWithinRoot(childPath: string, parentPath: string) — packages/cli/src/config/path-comparison.ts:10; keeping the old (parent, child) ordering computes relative(child, parent), which returns .. for a nested origin and would misclassify a local filter as outside and skip it.
中文说明
它与 packages/cli/src/commands/review/save-artifact.ts:140 的 isWithin 函数体逐字节相同，而两者都重新实现了已导出的 packages/cli/src/config/path-comparison.ts:10 中的 isWithinRoot（包内约有 50 处导入）。代码库现在有三份承担安全职责的包含关系测试副本——上面两条来源相关发现所依赖的判定门正是其一。当边界语义需要变更时（例如 core 的 isSubpath 通过其 win32 分支处理的 Windows 大小写不敏感问题，这些副本都没有处理），修复必须应用到每一份副本，而漏改的副本会静默偏离：在这里，一次错误的"不在其中"判定会把植入的仓库本地过滤器来源归为外部并跳过，于是筛查报告干净、checkout 却执行了过滤器。
建议修复：删除 isWithinDirectory（以及随之不再需要的 relative 导入），改用规范助手函数（见上方代码）。core 的 isSubpath(root, canonicalOrigin) 也是等价的直接替换，参数顺序同为 (parent, child)。注意共享助手以子路径为第一参数：export function isWithinRoot(childPath: string, parentPath: string)——packages/cli/src/config/path-comparison.ts:10；若沿用旧的 (parent, child) 顺序，实际计算的是 relative(child, parent)，对嵌套来源会返回 ..，把本地过滤器误判为外部并跳过。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Addressed in 5f1c17b. The duplicate helper and relative import are gone; both candidate and origin checks use the shared isWithinRoot(canonicalChild, root) with the child-first argument order.
| writeFileSync(included, `[filter "included"]\n\tsmudge = touch ${pwned}\n`); | ||
| git(worktree, 'config', 'include.path', included); |
There was a problem hiding this comment.
[Suggestion] R1-5: No test pins includeIf expansion — every plant in the new tests is unconditional include.path.
The fix is described as following both include.path and includeIf, but nothing in packages/cli/src/commands/review pins includeIf expansion (grep for includeIf across the tree → 0 matches). The expansion happens entirely inside git behind --includes, so a future regression that loses it — a refactor re-implementing include traversal, or a git behaviour change across versions — keeps every test in this diff green while a filter planted as [includeIf "gitdir:<common>/**"] path = evil.inc screens clean and executes during the checkout the screen authorizes: the exact hole this PR closes, reopened. Git does expand it today — measured live with the screen's exact spawn shape.
Witness:
grep -r 'includeIf' packages/cli/src/commands/review -> 0 matches
live git 2.47.3, screen spawn shape: [includeIf "gitdir:<repo>/**"] expands today
-> keyed worktree invocation prints the filter key (exit 0); screening-context invocation exits 1
Suggested fix — add a refuse-variant at both protected paths that plants an includeIf section (append directly to the config file, or git config 'includeIf.gitdir:.../.path' — measured to succeed), asserting available === false / verdict === 'inconclusive', the note names filter.<name>.smudge, and the canary never fires. The test must use a verified-matching gitdir pattern: gitdir:<repo>/** matched the gitdir in the measurements on git 2.47.3, but trailing-slash forms (gitdir:<repo>/.git/) did NOT match the gitdir itself. The new test itself is the witness — it must go red (available true / canary created) if --includes stops expanding includeIf.
中文说明
该修复被描述为同时跟随 include.path 与 includeIf，但 packages/cli/src/commands/review 中没有任何测试钉住 includeIf 展开（全树 grep includeIf → 0 处匹配）。展开完全发生在 git 内部、由 --includes 触发，因此未来任何失去该能力的回归——重新实现 include 遍历的重构，或 git 跨版本的行为变化——都会让本 diff 的所有测试保持绿色，而以 [includeIf "gitdir:<common>/**"] path = evil.inc 形式植入的过滤器却能通过筛查、并在筛查所授权的 checkout 期间执行：本 PR 堵上的漏洞被原样重新打开。当前 git 是会展开它的——已用筛查命令的原始形态实测确认。
建议修复：在两个受保护路径上各增加一个拒绝变体，植入 includeIf 段（直接向配置文件追加，或使用 git config 'includeIf.gitdir:.../.path'——实测可行），断言 available === false / verdict === 'inconclusive'、note 点名 filter.<name>.smudge、canary 从未执行。测试必须使用经过验证可匹配的 gitdir 模式：在 git 2.47.3 上实测 gitdir:<repo>/** 能匹配 gitdir，而带尾斜杠的形式（gitdir:<repo>/.git/）不能匹配 gitdir 本身。新测试本身就是见证——一旦 --includes 不再展开 includeIf，它必须变红（available 为 true / canary 被创建）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Addressed in 5f1c17b at both protected paths. scratch-tree.test.ts uses an includeIf.gitdir: keyed to the exact scratch admin entry and proves it is caught in the scratch context before checkout. test-efficacy.test.ts uses a matching current-gitdir conditional include and proves restore is refused before smudge. The affected real-Git suites now pass 189/189, with all canaries absent.
CanReader
left a comment
CanReader
left a comment
There was a problem hiding this comment.
Reviewed this as a security change and reproduced the hole it closes. The parser's assumptions about git's output format are correct — I checked them against real git rather than reading them off the comment.
Built a repo with a filter planted in an included file and ran the exact invocation this PR adds:
$ git config --null --show-origin --includes --file <common>/config \
--get-regexp '^filter\..*\.(smudge|clean|process)$'
fields: 4 even? true
origin: "file:/…/repo/extra.inc" key: "filter.included.smudge" value: "touch /tmp/PWNED"
origin: "file:/…/repo/.git/config" key: "filter.direct.smudge" value: "cat"
So the format really is <origin>\0<key>\n<value>\0, the field count really is even, origins really are file:-prefixed for --file reads, and — the point of the change — --includes does surface a filter that lives in an included file, with the included file as its origin. Without --includes that entry is invisible, which is the bypass. The "only the first newline separates key from value" note is right too and matters here, since a filter command is attacker-controlled and can contain newlines.
The parts I checked most carefully, all of which fail closed:
- odd field count, missing
file:prefix, or no newline in the key/value field all setunreadablerather than dropping the entry; realpathSyncfailing on a reported origin setsunreadableinstead of skipping, so a swapped-out origin cannot launder a hit into a clean screen;- the early
realpathSyncof the two roots returnsunreadablerather than falling back to a lexical prefix, which the comment correctly calls out as unsafe.
isWithinDirectory is also right to canonicalise both sides first: because localRoots and canonicalOrigin both go through realpathSync, the relative() comparison is not exposed to symlink or case differences on macOS, which is where a prefix check like this usually leaks.
The include-points-at-global-config case is the genuinely hard call and I think you have it right — an include resolving to the user's own ~/.gitconfig is the user's contract, the same as any git command they run, while an include resolving back inside the repo is attacker-planted. Having both tests, the refusal and the allow, is what makes that distinction reviewable.
No blocking findings from me.
|
Addressed all five review findings in
Local verification: affected real-Git suites 189/189, @qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 3b": run the new scratch-tree.test.ts / test-efficacy.test.ts suites as evidence — npm ci in the review worktree timed out at 10 minutes with no node_modules prese…; "agent 1a": run scratch-tree.test.ts / test-efficacy.test.ts to confirm green — vitest's globalSetup guard requires workspace dist/ output, and building the missing acp-….
Test Plan (not a blocker): src/commands/review/test-efficacy.test.ts — no such file or directory; src/commands/review/lib/worktree.ts — no such file or directory; src/commands/review/scratch-tree.ts — no such file or directory; src/commands/review/scratch-tree.test.ts — no such file or directory.
中文说明
未探索到全部深度（达到工具调用预算）："agent 3b"：run the new scratch-tree.test.ts / test-efficacy.test.ts suites as evidence — npm ci in the review worktree timed out at 10 minutes with no node_modules prese…；"agent 1a"：run scratch-tree.test.ts / test-efficacy.test.ts to confirm green — vitest's globalSetup guard requires workspace dist/ output, and building the missing acp-…。
Test Plan（非阻断）：src/commands/review/test-efficacy.test.ts — no such file or directory; src/commands/review/lib/worktree.ts — no such file or directory; src/commands/review/scratch-tree.ts — no such file or directory; src/commands/review/scratch-tree.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| 'yours; until then no scratch tree is safe to create or reset.', | ||
| ); | ||
| } | ||
| const tree = scratchWorktreePath(worktree, label); |
There was a problem hiding this comment.
[Critical] R2-1: [certifies-falsely] [regression] This round deletes the pre-residue filter screen from runScratchTree, so the shared-worktree residue measurement (git status --porcelain) now runs before any filter screening — and git status executes a planted clean filter when it re-reads a stale tracked file. The screens this PR adds live only in the reset and rebuild paths, both after the residue measurement, so pre-screen command execution is reintroduced in the very command this PR hardens. At the parent commit the screen preceded the residue measurement.
A probe makes the two writes this file's own docstring names as the planting surface — filter.evil.clean = <payload> in the common-dir config and *.ts filter=evil in info/attributes — and touches (mtime only) any tracked matching file in the shared review worktree. The next runScratchTree call then measures residue first: git forces a content read of the stale file through the clean filter and executes the payload as the user, before the screen refuses. The existing tests miss this because they plant only smudge filters, which status never executes.
Witness:
PR: F1-B: available=false canaryExists=true note=…defines content filter(s) filter.evil.clean…
FIX (pre-residue screen restored): F1-B: available=false canaryExists=false
Restore the early refusal before the residue measurement: call filterRefusal(localFilterCommands(worktree)) before worktreeResidue(worktree, …) and return the refusal when non-null (the review-worktree context is the right one — worktreeResidue measures that tree), keeping the per-scratch-tree screens for the includeIf.gitdir: contexts they were added for. The residue spawn only empties core.fsmonitor (lib/worktree.ts:1203 — "The measurement must not itself become the execution"); it does not and cannot neutralize config-driven filters, so the screen must run before it.
Please add a scratch-tree.test.ts case that plants filter.evil.clean = touch <canary> in the common config plus *.ts filter=evil in info/attributes, touches a tracked .ts file in the shared worktree fixture, runs run(), and asserts the canary does not exist and available is false — then remove the restored pre-residue screen and confirm the test reds.
中文说明
本轮删除了 runScratchTree 中 residue 测量之前的过滤器筛查，因此共享工作树的 residue 测量（git status --porcelain）现在先于任何过滤器筛查运行——而 git status 在重新读取过期的已跟踪文件时会执行植入的 clean 过滤器。本 PR 新增的筛查只存在于 reset 与 rebuild 路径，且都在 residue 测量之后，于是筛查前的命令执行在本 PR 要加固的命令中被重新引入。在父提交上，筛查先于 residue 测量。
探针只需本文件 docstring 自己指明的两个植入写入——common dir 配置中的 filter.evil.clean = <payload> 与 info/attributes 中的 *.ts filter=evil——再对共享 review worktree 中任一匹配的已跟踪文件做 mtime touch。下一次 runScratchTree 调用会先测量 residue：git 强制通过 clean 过滤器读取该过期文件，并以用户身份执行 payload，筛查来不及拒绝。现有测试没有覆盖这一点，因为它们只植入 smudge 过滤器，而 status 从不执行 smudge。
证据：PR 侧探针 available=false canaryExists=true（note 点名 filter.evil.clean）；恢复 residue 前筛查后 available=false canaryExists=false。
修复：在 residue 测量前恢复提前拒绝——先调用 filterRefusal(localFilterCommands(worktree))，非空即返回拒绝（review worktree 语境是正确的——worktreeResidue 测量的就是该树），保留各 scratch 树上的筛查以覆盖其为之添加的 includeIf.gitdir: 语境。residue 的 spawn 只清空 core.fsmonitor（lib/worktree.ts:1203——"测量本身不得成为执行"），无法中和配置驱动的过滤器，因此筛查必须先于它运行。
请新增 scratch-tree.test.ts 用例：植入 filter.evil.clean = touch <canary> 与 *.ts filter=evil，touch 共享工作树中一个已跟踪的 .ts 文件，运行 run()，断言 canary 不存在且 available 为 false——然后移除恢复的筛查并确认测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const refusal = filterRefusal(localFilterCommands(tree)); | ||
| if (refusal !== null) return { reset: false, filterRefusal: refusal }; |
There was a problem hiding this comment.
[Critical] R2-2: [certifies-falsely] The screen this diff inserts into resetScratchTree widens the identity-gate→checkout window, and the pre-checkout re-check only re-reads the leaf symlink — a rewrite of <tree>/.git (gitfile) content inside that window is never re-validated, so the authorized checkout --force --detach can be redirected onto another repository while the report certifies a successful reset.
A concurrent same-user probe shard rewrites a sibling scratch tree's .git file to gitdir: <common> during the multi-spawn screen window. The leaf lstatSync below still passes (the path is still a directory), and the subsequent checkout detaches the user's MAIN HEAD onto the PR sha and rewrites the main index while the report says available: true, reused: true — the shape the gate's own comment says was measured on a real repo. The window existed pre-diff; this diff widens it (a distinct mechanism from the rebuild-path leaf-symlink window reported separately).
Witness:
RACE HIT { iteration: 7, branch: "HEAD" (left 'main'), mainHeadNow == headSha,
reportAvailable: true, reportReused: true } // 1 hit / 7 iterations against the real runScratchTree
aim probe: mainHeadRef "HEAD" (was 'main'); main index mtime advanced 38s past the worktree indexes
Beside the leaf re-check immediately before the checkout, re-validate the gitfile target: re-read <tree>/.git, resolve the named admin entry, and repeat the gate's backpointer round-trip against tree, so a gitfile swapped during the screen window is caught the same way a leaf symlink is. The re-check must reuse the comparison shape the gate already establishes, realpathSync(dirname(resolve(gitdir, backpointer))) !== realpathSync(tree) (scratch-tree.ts:297), and like the existing re-check it must lstat rather than follow links (scratch-tree.ts:319).
中文说明
本 diff 在 resetScratchTree 中插入的筛查加宽了身份门→checkout 的窗口，而 checkout 前的复查只重新读取叶子符号链接——在该窗口内对 <tree>/.git（gitfile）内容的改写不会被重新验证，因此被授权的 checkout --force --detach 可以被重定向到另一个仓库，而报告仍证明 reset 成功。
同用户的并发探针分片在多 spawn 的筛查窗口内，把兄弟 scratch 树的 .git 文件改写为 gitdir: <common>。下方的叶子 lstatSync 仍然通过（路径仍是目录），随后的 checkout 会把用户的 MAIN HEAD 分离到 PR sha 并重写主索引，而报告却显示 available: true, reused: true——这正是门的注释声称在真实仓库上实测过的形态。该窗口在 diff 之前已存在；本 diff 将其加宽（与另行报告的 rebuild 路径叶子符号链接窗口是不同机制）。
证据：竞争探针 7 轮中命中 1 次（branch: "HEAD"（离开 'main'）、mainHeadNow == headSha、reportAvailable: true, reportReused: true）；定向探针显示 mainHeadRef "HEAD"（原为 'main'），主索引 mtime 比 worktree 索引晚 38 秒。
修复：在紧邻 checkout 前的叶子复查旁重新验证 gitfile 目标——重读 <tree>/.git、解析其指明的 admin entry，并对 tree 重复门中的 backpointer 往返检查，使筛查窗口内被调换的 gitfile 像叶子符号链接一样被捕获。该复查必须复用门已建立的比较形态 realpathSync(dirname(resolve(gitdir, backpointer))) !== realpathSync(tree)（scratch-tree.ts:297），且与现有复查一样必须使用 lstat 而不是跟随链接（scratch-tree.ts:319）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // include directive that routed here is repo-local and writable by a | ||
| // probe. Exempt only origins Git also reaches through the actual | ||
| // global/system filter graph; every other outside hit is a plant. | ||
| if (userOrigins.has(canonicalOrigin)) continue; |
There was a problem hiding this comment.
[Critical] R1-2: (fix-induced) [certifies-falsely] [new-surface] The round-1 fix closed the reported input — an arbitrary outside-root origin is no longer exempted — but the exemption it introduced judges an outside hit by the screen-time canonical target of the reported include path. When that path is a probe-controlled symlink, the screen-time target is exempted, and re-pointing the symlink after the screen changes what the authorized checkout executes.
A user whose global graph reaches an include file defining a filter (the git-lfs-with-include layout the exemption exists for) reviews a malicious PR: a concurrent same-user probe plants include.path = <attacker>/swap in the common-dir config with swap -> <the user's filter file> plus an attributes line selecting the filter name. The screen realpaths the reported origin to the user file, finds it in userFilterOrigins and exempts it; the probe then re-points swap to a payload file, and the sanctioned checkout --force --detach follows the include through the symlink and executes the payload as the user. A symlinked CANDIDATE config is refused by the new containment guard; a symlinked include TARGET is not. Pre-diff this plant executed unconditionally (the merge base had no --includes; at the round-1 anchor outside origins were skipped), so the exemption makes the plant strictly harder — the bypass lives in the new surface.
Witness:
RACE HIT at round 5 (trial 1) and round 2 (trial 2): canary created, report says "available": true
deterministic endpoints: swap -> user file screens clean ({keys:[],unreadable:null});
swap -> payload at screen time refuses
FIX (lstatSync fail-closed on symlinked outside origins): CANARY-EXISTS=false, direct-global exemption stays green
In the !isLocal branch, before consulting userOrigins, lstatSync the resolved reported path and fail closed (unreadable ??= …; continue) when it isSymbolicLink(): a user-owned origin is a real file in the user's graph, and an outside origin that reaches the graph only through a probe-repointable symlink is not a state the exemption can hold. The guard must key on the reported path being a symlink, not on the origin being outside the roots — the exemption for DIRECT outside origins must stay intact, as pinned by expect(run().available).toBe(true) in allows a repo-local include whose filter is defined by global config (scratch-tree.test.ts).
Please add a scratch-tree.test.ts case where the include directive points through a stable symlink at the global-graph filter file and asserts refusal (available false, canary untouched) — then remove the lstatSync check and confirm the test reds.
中文说明
第 1 轮的修复关闭了原报告的输入——任意的仓库外来源不再被豁免——但它引入的豁免按筛查时刻报告 include 路径的规范化目标来判定仓库外命中。当该路径是探针可控的符号链接时，筛查时刻的目标被豁免，而在筛查之后重新指向该符号链接，就会改变被授权 checkout 实际执行的内容。
全局配置图经由 include 文件到达过滤器定义的用户（正是该豁免为之存在的 git-lfs-with-include 布局）在审查一个恶意 PR：同用户并发探针对 common dir 配置植入 include.path = <attacker>/swap，其中 swap -> <用户的过滤器文件>，再加上选择该过滤器名的 attributes 行。筛查把报告的来源 realpath 到用户文件，发现其在 userFilterOrigins 中并予以豁免；随后探针把 swap 重新指向 payload 文件，被授权的 checkout --force --detach 便顺着 include 透过符号链接执行该 payload。符号链接的候选配置文件会被新的包含关系守卫拒绝；符号链接的 include 目标则不会。在本 diff 之前该植入会无条件执行（merge base 没有 --includes；第 1 轮锚点处仓库外来源被直接跳过），因此豁免实际上让植入变得更难——绕过存在于新表面之上。
证据：两轮竞争实验分别在第 5 轮与第 2 轮命中：canary 被创建，而报告显示 "available": true。确定性端点：swap -> 用户文件 时筛查干净（{keys:[],unreadable:null}）；筛查时刻 swap -> payload 则被拒绝。修复（对符号链接的仓库外来源以 lstatSync fail-closed）后 CANARY-EXISTS=false，且直接全局豁免保持绿色。
修复：在 !isLocal 分支中、查询 userOrigins 之前，对解析后的报告路径做 lstatSync，若其为符号链接则 fail-closed（unreadable ??= …; continue）：用户自有的来源是用户配置图中的真实文件，而一个仅经由探针可重指的符号链接才能到达配置图的仓库外来源，不是该豁免可以维持的状态。守卫必须以报告路径是否为符号链接为键，而不是以来源是否在仓库根之外为键——直接仓库外来源的豁免必须保持原样，由 allows a repo-local include whose filter is defined by global config（scratch-tree.test.ts）中的 expect(run().available).toBe(true) 钉住。
请新增用例：include 指令经由稳定符号链接指向全局配置图中的过滤器文件，断言拒绝（available 为 false、canary 未被触碰）——然后移除 lstatSync 检查并确认测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const reset = existsSync(tree) | ||
| ? resetScratchTree(tree, headSha, worktree) | ||
| : { reset: false as const }; | ||
| if (!reset.reset && reset.filterRefusal !== undefined) { |
There was a problem hiding this comment.
[Suggestion] R2-4: The post-residue refusal report literal (7 fields: available/reused/dependencies/sharedTreeResidue/sharedTreeResidueTotal/sharedTreeUnmeasured/note) is now pasted three times inside runScratchTree — this diff adds two copies (the reset-refusal here and the rebuild-refusal at scratch-tree.ts:571) that match the pre-existing catch-block copy at scratch-tree.ts:592 verbatim except for the note string.
ScratchTreeReport is a documented interface verifier scripts consume; a field added to it (optional fields are not forced into every object literal by the type checker) must be propagated to three identical sites by hand, and this PR shows the pattern grows — each new refusal path copies the block again, while the function already contains the unavailable(note) closure for pre-residue refusals with no post-residue counterpart.
Witness:
witness: not run — grep count over scratch-tree.ts enumerates the three identical 7-field refusal
literals (:505, :571, :592; two added by this diff); a maintainability-duplication claim has no
executable behaviour to probe or flip.
Add a closure beside unavailable and call it from all three sites:
const refusedWithResidue = (note: string): ScratchTreeReport => ({
available: false,
reused: false,
dependencies: null,
sharedTreeResidue,
sharedTreeResidueTotal: residue.total,
sharedTreeUnmeasured: residue.unmeasured,
note: note + residueNote,
});Do not merge it with unavailable — the contract at scratch-tree.ts:363-374 requires refusals BEFORE residue measurement to say so (sharedTreeResidue: [] + sharedTreeUnmeasured), and merging would make post-residue refusals report the residue as unmeasured.
中文说明
residue 测量之后的拒绝报告字面量（7 个字段）现在在 runScratchTree 内部被粘贴了三次——本 diff 新增两份（此处的 reset 拒绝与 scratch-tree.ts:571 的 rebuild 拒绝），与 scratch-tree.ts:592 既有的 catch 块副本除 note 字符串外逐字相同。
ScratchTreeReport 是验证脚本消费的文档化接口；向它新增字段（可选字段不会被类型检查器强制进入每个对象字面量）必须手工传播到三个相同的位点，而本 PR 表明该模式还在增长——每条新的拒绝路径都会再复制一遍该块，而函数中已经存在针对 residue 之前拒绝的 unavailable(note) 闭包，却没有 residue 之后的对应物。
修复：在 unavailable 旁新增闭包 refusedWithResidue（见上代码），并从三个位点统一调用。不要将其与 unavailable 合并——scratch-tree.ts:363-374 的契约要求 residue 测量之前的拒绝必须如实说明（sharedTreeResidue: [] + sharedTreeUnmeasured），合并会让 residue 之后的拒绝把 residue 报告为未测量。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| expect(existsSync(pwned)).toBe(false); | ||
| }); | ||
|
|
||
| it('screens includeIf in the scratch gitdir context before checkout', () => { |
There was a problem hiding this comment.
[Suggestion] R2-5: The reset path's screen running in the SCRATCH tree's gitdir context — the stated core of this commit — is unpinned: every conditional-include test added here runs once (rebuild path), and the only two-run (reset-path) test plants its filter in config.worktree, which the candidate scan catches in ANY context.
Mutating localFilterCommands(tree) → localFilterCommands(worktree) inside resetScratchTree (scratch-tree.ts:310) leaves all 51 tests green; at runtime a planted includeIf.gitdir:<scratchAdmin> plus a selecting attributes line then fails to match in the review-worktree context on the second run(), the reset screen reports clean, and the reset's checkout --force executes the smudge. Pre-diff the reset path had no screen at all, so the regression this commit fixes on the reset path ships unwitnessed.
Witness:
BASELINE (unmodified): 51 passed | UNDER MUTANT: 51 passed
mutant runtime probe: {"available":true,"reused":true,"canaryCreated":true}
proposed two-run test: red under the mutant, green on the PR
Add a two-run variant of the includeIf test: first run() creates the tree; then write includeIf.gitdir:<scratchAdmin>.path into the common config, the included filter file, and a selecting attributes line; second run() must return available: false with the key named in note and the canary file absent. Removing the screen call or swapping its context argument in resetScratchTree must make the new test red — that is the mutation the current suite cannot see.
中文说明
reset 路径的筛查运行在 SCRATCH 树的 gitdir 语境中——这是本提交声明的核心——但没有任何测试钉住它：此处新增的所有条件式 include 测试都只运行一次（走 rebuild 路径），而唯一的双跑（reset 路径）测试把过滤器植入 config.worktree，候选扫描在任何语境下都能捕获它。
把 resetScratchTree（scratch-tree.ts:310）中的 localFilterCommands(tree) 突变为 localFilterCommands(worktree)，全部 51 个测试仍然为绿；而在运行时，植入的 includeIf.gitdir:<scratchAdmin> 加选择的 attributes 行在第二次 run() 时于 review worktree 语境中不匹配，reset 筛查报告干净，reset 的 checkout --force 随即执行 smudge。本 diff 之前 reset 路径根本没有筛查，因此本提交在 reset 路径上修复的回归在没有见证的情况下交付。
证据：基线（未修改）51 通过；突变体下同样 51 通过。突变体运行时探针：{"available":true,"reused":true,"canaryCreated":true}。建议的双跑测试在突变体下变红、在本 PR 上为绿。
修复：新增 includeIf 测试的双跑变体：第一次 run() 创建树；随后向 common 配置写入 includeIf.gitdir:<scratchAdmin>.path、被 include 的过滤器文件以及选择的 attributes 行；第二次 run() 必须返回 available: false，note 点名该键，且 canary 文件不存在。移除筛查调用或调换其语境参数必须使新测试变红——这正是当前测试套件看不到的突变。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| '--show-origin', | ||
| '--includes', | ||
| scope, |
There was a problem hiding this comment.
[Suggestion] R2-6: The --includes flag in the new userFilterOrigins spawn is load-bearing but no test pins it: every exemption test defines its filter directly inside the global config file, so deleting the flag leaves the entire suite green.
A user whose ~/.gitconfig reaches its filter definitions through its own include.path (a realistic git-lfs/XDG-style layout), plus a repo under review carrying a repo-local include.path reaching the same included file: without --includes, userFilterOrigins misses the second-level file, the exemption fails, and every scratch-tree creation is refused with defines content filter(s) filter.lfs.clean — the permanent false refusal the exemption was built to prevent — with no test going red. Fail-closed, so a usability regression, not an execution hole.
Witness:
git --global read: exit=1 without --includes / exit=0 with --includes (file:…/real-filters.inc filter.lfs.clean)
PR: F4: available=true | MUTANT (--includes removed): F4: available=false note=…filter.included.smudge…
both existing exemption tests stay green under the mutant
Add an exemption test where the global config reaches the filter through its own include: write a filters.inc defining filter.lfs.clean under the isolated home, set include.path in the isolated .gitconfig, point a repo-local include.path at the same filters.inc, and assert run().available is true — removing --includes from this spawn must make it red.
中文说明
新的 userFilterOrigins spawn 中的 --includes 标志是承重的，但没有任何测试钉住它：所有豁免测试都把过滤器直接定义在全局配置文件内，因此删除该标志整个测试套件仍然为绿。
设用户的 ~/.gitconfig 经由自己的 include.path 到达过滤器定义（现实的 git-lfs/XDG 式布局），而被审查的仓库带有指向同一被 include 文件的仓库本地 include.path：没有 --includes 时，userFilterOrigins 会漏掉第二层文件，豁免失败，每次创建 scratch 树都会被以 defines content filter(s) filter.lfs.clean 拒绝——正是该豁免本要防止的永久误拒——而没有任何测试变红。由于失败即关闭，这是可用性回归而非执行漏洞。
证据：git --global 读取：不带 --includes 时 exit=1，带时 exit=0（报告 file:…/real-filters.inc filter.lfs.clean）。PR 侧探针 available=true；突变体（移除 --includes）available=false，note 点名 filter.included.smudge；两个既有豁免测试在该突变下仍为绿。
修复：新增豁免测试，让全局配置经由自己的 include 到达过滤器：在隔离的 home 下写入定义 filter.lfs.clean 的 filters.inc，在隔离的 .gitconfig 中设置 include.path，让仓库本地 include.path 指向同一 filters.inc，断言 run().available 为 true——从该 spawn 中移除 --includes 必须使测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| } | ||
|
|
||
| /** Filter origins that Git itself reaches through the user or system graph. */ | ||
| function userFilterOrigins(worktree: string): Set<string> { |
There was a problem hiding this comment.
[Suggestion] R2-7: userFilterOrigins under-approximates the graph its docstring promises: git config --global reads only ONE of git's global-level files, while the checkouts the screen authorizes load BOTH ~/.gitconfig and $XDG_CONFIG_HOME/git/config — so a filter defined only in the XDG file can never be exempted.
With both files present (a realistic layout), a probe writing include.path = <absolute XDG path> into the common-dir config causes permanent refusal of every scratch-tree create/reset — and the test-efficacy restore — with defines content filter(s) filter.lfs.clean, pointing the refusal message at the user's own legitimate global-level config. Fail-closed — refusal, not execution.
Witness:
F7 global-view: status=1 stdout="" — merged view: exit 0, reports file:<xdg>/git/config filter.lfs.clean
F7 RESULT: refused-with-include=true (control clean=true)
fix flip (reading the XDG file into userFilterOrigins): refused-with-include=false
Also query git's second global-level file: when $XDG_CONFIG_HOME/git/config (fallback $HOME/.config/git/config) exists and its canonical path is not the file --global selected, run the same --null --show-origin --includes --file <that file> read and add its origins. The widened read must stay limited to files git loads at the global/system level, and unresolved origins must keep failing closed per the existing catch in this function — the exemption's own rule at worktree.ts:847: "Exempt only origins Git also reaches through the actual global/system filter graph; every other outside hit is a plant."
Please add a variant of allows a repo-local include whose filter is defined by global config that, with a non-empty ~/.gitconfig present, defines the filter ONLY in $XDG_CONFIG_HOME/git/config and includes that file from the repo-local config, asserting run().available is true — removing the XDG read must make it red.
中文说明
userFilterOrigins 低估了其 docstring 所承诺的配置图：git config --global 只读取 git 全局级文件中的一个，而筛查所授权的 checkout 会同时加载 ~/.gitconfig 与 $XDG_CONFIG_HOME/git/config——因此只定义在 XDG 文件中的过滤器永远无法被豁免。
当两个文件同时存在（现实布局）时，探针对 common dir 配置写入 include.path = <绝对 XDG 路径> 就会导致每一次创建/重置 scratch 树——以及 test-efficacy 的 restore——被永久拒绝，理由为 defines content filter(s) filter.lfs.clean，拒绝信息把用户指向其自身合法的全局级配置。失败即关闭——是拒绝而非执行。
证据：F7 global-view: status=1 stdout=""，而合并视图 exit 0 并报告 file:<xdg>/git/config filter.lfs.clean；F7 RESULT: refused-with-include=true（对照 clean=true）；修复翻转（把 XDG 文件读入 userFilterOrigins）后 refused-with-include=false。
修复：同时查询 git 的第二个全局级文件：当 $XDG_CONFIG_HOME/git/config（回退 $HOME/.config/git/config）存在且其规范化路径不是 --global 选中的文件时，执行相同的 --null --show-origin --includes --file <该文件> 读取并加入其来源。扩展的读取必须限于 git 在全局/系统级加载的文件，未解析的来源必须继续按本函数既有的 catch 走 fail-closed——即 worktree.ts:847 处豁免自身的规则："只豁免 Git 经由实际的全局/系统过滤器配置图也能到达的来源；其余任何仓库外命中都是植入。"
请新增 allows a repo-local include whose filter is defined by global config 的变体：在存在非空 ~/.gitconfig 的情况下，仅在 $XDG_CONFIG_HOME/git/config 中定义过滤器，并从仓库本地配置 include 该文件，断言 run().available 为 true——移除 XDG 读取必须使测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
3 participants
What this PR does
Follows repo-local Git includes while screening executable content filters, parses Git-reported origins, and canonicalizes every candidate and origin before deciding whether checkout is safe.
The follow-up for review feedback also:
--no-checkout, screens it in its real gitdir context, and only then materializes tracked files, so scratch-onlyincludeIf.gitdir:conditions cannot hide a smudge command;isWithinRootpath helper.Real-Git regressions cover both protected checkout paths. They verify plain includes, matching
includeIf, untrusted outside origins, candidate-config symlinks, and the global-config false-positive control. Canary files prove refusal happens before a configured command executes.This PR is stacked on #10421 and intentionally targets
fix/review-filter-screen-v3. It should be retargeted tomainafter the parent merges.Why it is needed
The current
git config --filescreen does not expandinclude.pathorincludeIf, while the checkout it authorizes does. A command hidden in an included file can therefore execute during reset or restore.Origin location alone is not a trust boundary: repository config can include an arbitrary outside file, including writable worktree content. Conversely, blanket rejection of all outside origins would break legitimate user-level filter configuration such as git-lfs. The screen now permits an outside origin only when Git independently reaches that exact file through the user's global/system graph.
The scratch checkout also has a different gitdir from the review worktree. Screening only from the review worktree misses a conditional include that matches the predictable scratch admin entry. Registering with
--no-checkoutmakes the real context available before any smudge filter can run.Reviewer Test Plan
How to verify
cd packages/cli.npm test -- src/commands/review/scratch-tree.test.ts src/commands/review/test-efficacy.test.ts; expect 189 tests to pass.npm run typecheck.npx eslint src/commands/review/lib/worktree.ts src/commands/review/scratch-tree.ts src/commands/review/scratch-tree.test.ts src/commands/review/test-efficacy.test.ts.--includestemporarily; the plain-include regressions should fail.worktree add --no-checkout; the scratch-gitdirincludeIfregression should fail and its canary demonstrates the unsafe checkout.Local result on this head: 189/189 affected tests passed; TypeScript, targeted ESLint, Prettier, and
git diff --checkpassed.Evidence (Before & After)
N/A, internal review infrastructure with no user-visible UI.
Tested on
Environment (optional)
macOS 26.3.2, Node.js v25.9.0, npm 11.12.1, Git 2.50.1.
Risk & Scope
Linked Issues
Fixes #10441
中文说明
本 PR 做了什么
筛查可执行内容过滤器时展开仓库本地 Git include，解析 Git 返回的来源，并在决定 checkout 是否安全前对候选配置和来源做规范化路径判断。
根据 review 反馈，本轮还完成了以下加固：
--no-checkout注册，在其真实 gitdir 语境中完成筛查后才写出 tracked 文件，因此只匹配 scratch 的includeIf.gitdir:无法隐藏 smudge 命令；isWithinRoot路径判断工具。两个受保护的 checkout 路径都有真实 Git 回归测试，覆盖普通 include、匹配的
includeIf、不可信仓库外来源、候选配置符号链接，以及 global 配置误报对照。Canary 文件同时证明拒绝发生在危险命令执行之前。本 PR 基于 #10421，当前有意以
fix/review-filter-screen-v3为目标分支。父 PR 合并后应将目标分支改为main。为什么需要
当前
git config --file筛查不会展开include.path或includeIf，但它授权的 checkout 会展开，因此藏在 include 文件中的命令可能在 reset 或 restore 时执行。来源位于仓库外并不等于可信，因为仓库配置可以 include 任意外部文件，包括可写的 worktree 内容；但一律拒绝仓库外来源又会误伤 git-lfs 等合法用户配置。现在只有当 Git 从用户 global/system 配置图中也能到达同一个规范化文件时，才允许该来源。
此外，scratch checkout 的 gitdir 与 review worktree 不同。只在 review worktree 语境筛查，会漏掉专门匹配可预测 scratch admin entry 的条件 include。通过
--no-checkout先注册，可以在任何 smudge filter 执行前取得并筛查真实语境。Reviewer 测试计划
如何验证
cd packages/cli。npm test -- src/commands/review/scratch-tree.test.ts src/commands/review/test-efficacy.test.ts，应有 189 个测试通过。npm run typecheck。--includes，普通 include 回归测试应失败。worktree add --no-checkout之前，scratch-gitdirincludeIf回归测试应失败，canary 会显示危险 checkout。本地结果：受影响测试 189/189 通过；TypeScript、定向 ESLint、Prettier 和
git diff --check均通过。前后证据
N/A，内部 review 基础设施改动，没有用户可见 UI。
测试平台
风险与范围
关联 Issue
Fixes #10441