Repository navigation
Conversation
Glob patterns are validated lexically (`assertGlobPatternInScope` and the
worker's `assertContainedGlobPattern`): a pattern is rejected only when its
literal text carries an absolute path or a `..` segment. Brace expansion
happens later, inside the walker, so a lexically clean pattern like
`{a,../*}` walks above its search root and glob results listed entries
outside the session cwd with no downstream filter (apache#6397).
Filter at the shared reader instead: `globFiles` gains an optional
`containmentRoot` and drops matched paths that resolve outside it using the
existing `isPathInside` predicate (literal resolve-space containment — the
walk already refuses to follow symlinks, and results are names, not opened
content). Dropped escapes do not count toward the limit, so truncation
semantics still describe the returned prefix.
The workspace executor passes the canonicalised session cwd (same spelling
space as the walker's search root, or a symlinked root like /var ->
/private/var would reject every legitimate entry); host scope keeps raw
results and zero added work. The filesystem worker passes its search root,
matching its "stay inside its search root" guard contract. The Windows
sandbox preview's non-following walker (`windowsNonFollowingGlob`) is a
separate traversal implementation and is intentionally untouched.
Fixes apache#6397
Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed all five changed files at 864feb721a9da770f46736314f6df6616476a531. There is one P2 remaining containment gap, described inline at glob-search.ts:86.
The brace-expansion problem is reproducible, and this patch fixes the tested outward brace branches without counting excluded matches against the returned-result limit. However, the assumption that the walker never follows directory links does not hold for explicit link-prefixed patterns. With workspace/link-out pointing to an outside directory containing secret.txt, link-out/**/*.txt still returns link-out/secret.txt through the shared reader, the production local filesystem executor, and the production filesystem worker. The lexical result check accepts that spelling even though its ancestor resolves outside the workspace. This is also reproducible on current main; it is a pre-existing gap left open by this fix, not a newly introduced regression. The demonstrated exposure is an outside filename, not a successful file-content read.
I ran the repository glob-search and filesystem-worker suites: 35 tests passed. In 18 additional real-filesystem cases, 15 passed and the three link-traversal cases failed, one for each production entry point. Current main fails nine of those cases; the patch removes six brace-related failures and retains the three link failures. The actual clean merge with main 85a1031c10d104456127f35aac1523b9ffb1a384 produces the same results. Host scope intentionally retains outside matches; a symlinked Session root still keeps legitimate in-root results.
The head's hosted CI test is successful (run 40581707421). Local execution used the actual production modules, Node's TypeScript transformation, and glob 13.0.6, without filesystem adapters. I did not run the complete builtin-tools suite, a full repository build, an OS sandbox, native Windows, or macOS validation.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Directory symlinks escape the glob containment check: `resolve()` only normalizes the spelling, so an explicit pattern like `link-out/**/*.txt` with `link-out -> ../outside` returns files whose real location is outside the containment root while staying lexically inside it. Decide containment in real-path space instead: realpath the containment root once, realpath each candidate match, and run the pure predicate on those (the same idiom skills-discovery already uses). A root that cannot be realpath'd propagates, because the walk cannot claim containment it cannot place; a match that cannot be realpath'd (raced deletion) is dropped and marks the walk incomplete rather than silently complete. Fixes apache#6397 Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Rechecked the complete five-file patch at e7fb6e1920168413bfd9b6a75bd6ec76bf30a3ad, including the two-file delta from my previous review. The remaining directory-link containment P2 is fixed: the root and each accepted candidate are now compared in real-path space. The exact link-out/**/*.txt reproduction no longer returns outside filenames through the shared reader, local filesystem executor or filesystem worker. In-root directory aliases still return legitimate matches. I found no confirmed new P0–P2 issue.
On both the head and its actual clean merge with main 85a1031c10d104456127f35aac1523b9ffb1a384, 35 repository tests and 25 additional real-filesystem checks passed. The checks cover the previous 18-case matrix plus in-root directory links, outside file links and an unavailable containment root. Replacing only the shared reader with the previously reviewed implementation makes the three original directory-link cases fail again. Excluded matches still do not consume the returned-result cap; unrestricted host scope and a symlinked Session root retain their intended behavior.
The current test CI and both Windows checks are successful. Local tests execute the actual production modules with Node's TypeScript transformation and glob 13.0.6, using real temporary files and links. I did not run the complete builtin-tools suite, a full build or typecheck, an OS sandbox, or native Windows/macOS validation. The raced-candidate-deletion branch was inspected in source, not forced during these tests.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
github-actions
Bot
added
effort/M
|
Thank you for the recheck at e7fb6e1 — the 18-case matrix extended with in-root directory links, outside file links and an unavailable containment root, plus the swap-back ablation re-failing the three original cases, is exactly the verification loop this fix needed. No changes needed at this head. |
Summary
Glob patterns are validated lexically, but brace expansion happens inside the walker: a lexically clean pattern like
{a,../*}walks above the session cwd and its results listed outside entries with no downstream filter. This fixes it at the shared reader —globFilesgains an optionalcontainmentRootand drops matched paths that resolve outside it via the existingisPathInsidepredicate (literal containment; the walk already refuses to follow symlinks and results are names, not opened content). The workspace executor passes the canonicalised session cwd (same spelling space as the walker's root — a symlinked root like/var→/private/varwould otherwise reject every legitimate entry); the filesystem worker passes its search root, matching its guard's own contract. Dropped escapes do not count toward the limit. The Windows sandbox preview's non-following walker is a separate traversal and is intentionally untouched.Fixes #6397
Verification
Glob brace expansion cannot list entries outside the session cwdon unfixed base../*.txtlexical-guard pin stays green)tsc -p packages/runtime, biome per changed file,check:asf-headersAI use
Implemented with GLM-5.3-Flash (ZCode); the lexical guard, the brace-expansion semantics of the pinned glob version, and the missing downstream filter were re-verified line by line before filing #6397.
Checklist
/var→/private/var) was hit and fixed in testing