fix(isolation): enforce copy path containment - #947
Conversation
Greptile SummaryThe PR introduces a shared pinned-root boundary for isolation copies and applies it across traversal, directory creation, synchronous copies, and worker copies.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and both previously reported documentation omissions are fixed at the current head.
|
| Filename | Overview |
|---|---|
| src/copy-containment.ts | Adds centralized relative-path validation, root identity pinning, canonical containment checks, and typed worker-error reconstruction. |
| src/copy-worker.ts | Routes worker copies through the shared boundary and reports the first unexpected or containment failure to the parent. |
| src/isolation-manager.js | Applies containment checks throughout collection, directory creation, synchronous copying, and worker orchestration. |
| tests/isolation-copy-containment.test.js | Covers traversal, symlink escapes and cycles, root replacement, in-root symlinks, and worker failure propagation. |
| CLAUDE.md | Records the shared pinned-root and immediate revalidation invariant, resolving the previous Claude-guidance finding. |
| AGENTS.md | Directs automated contributors to preserve the containment boundary across every isolation-copy phase, resolving the previous agent-guidance finding. |
Sequence Diagram
sequenceDiagram
participant IM as IsolationManager
participant CB as CopyBoundary
participant FS as Filesystem
participant W as Copy Worker
IM->>CB: Pin source and destination roots
CB->>FS: Resolve canonical paths and identities
IM->>CB: Validate traversal entries
CB->>FS: Revalidate roots and resolve symlinks
IM->>FS: Create validated destination directories
alt Small copy
IM->>CB: Resolve source and destination
IM->>FS: copyFileSync
else Worker copy
IM->>W: Send files and expected boundary
W->>CB: Re-pin and compare expected roots
W->>CB: Resolve each source and destination
W->>FS: copyFileSync
W-->>IM: Result or typed containment error
end
Reviews (6): Last reviewed commit: "refactor(isolation): share worker error ..." | Re-trigger Greptile
a13645f to
d709225
Compare
|
Updated the branch and rebased it onto current
Local validation on head
The new CI run is now starting. Once it settles, could a maintainer please take a human review? |
|
CI follow-up: the only underlying failure is Both my CLI rerun and the connected GitHub app were denied upstream Actions write permission. @mkceichelbeck, could you please rerun the failed jobs for run 31306614628 and take a human review when convenient? |
20f230d to
97e3cf0
Compare
|
Rebased this PR onto current Validated on the current head:
The branch is mergeable again and fresh CI is running. |
a15df4a to
52f3130
Compare
tomdps
left a comment
There was a problem hiding this comment.
Reviewed against #939 and current main. The shared pinned-root boundary is enforced across traversal, directory creation, synchronous copies, and worker copies; containment violations fail closed while ordinary unreadable/broken entries retain existing skip behavior. Source/destination symlink escapes, cycles, raw traversal, root replacement, worker error propagation, and in-root compatibility are covered. I also validated a conflict-free synthetic merge with current main: 22 focused containment tests, 12 existing isolation-manager integration tests, full TypeScript build/typecheck, and package smoke all pass. The documented same-user path-based TOCTOU/hard-link/mount-alias limitation is acceptable for this defense-in-depth scope.
|
🎉 This PR is included in version 6.39.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Related issue
Closes #939
Symlink and scope policy
Source symlinks and existing destination symlinks remain allowed when their canonical targets stay inside the pinned source or destination root. Escaping source links, escaping or broken destination links, directory cycles, absolute paths, and traversal components are rejected. The supported producer remains local
readdirSync(..., { withFileTypes: true })traversal; archive or external member ingestion is out of scope.The boundary pins root device/inode identity and revalidates canonical paths immediately before each filesystem effect. Node's path-based filesystem APIs cannot make that check/use sequence atomic against a same-user concurrent tree replacement, hard-link alias, or mount alias. Worker failure cancellation is also non-transactional: already completed safe in-root copies are not rolled back.
Testing
npm run typecheckpassesnpm run lintpasses with no errors (existing warnings remain)Broader local validation note: the default parallel
npm testrun exhausted this host's process-spawn capacity. A corrected single-job retry progressed until the unchangedtests/unit/attach-stdin.test.jsfailed to start its PTY withposix_spawnp failed; the isolated test fails at the same spawn boundary before reaching changed code. The focused, related, static, and scale gates above pass.