feat(infra): implement filesystem bootstrap with domain path-containment primitives#20
feat(infra): implement filesystem bootstrap with domain path-containment primitives#20
Conversation
Boot-time bootstrap that creates Forgent's 12-directory layout under the two allowed roots: <project>/.claude/forgent/ and ~/.forgent/. Path containment is enforced before any filesystem syscall via a lexical check that rejects '..' traversal components and refuses any candidate dir that does not start with one of the two allowed roots derived from AppConfig. 5 tokio tests in tempfile sandboxes covering full creation, idempotency, traversal refusal, escape-root refusal, and enumeration of created entries. Coverage 99.28% lines. PRD §0.3, ARCHI §5.5 / §9.2.
…urity /simplify follow-up on T-104. Two changes: - Populate the previously-empty domain/security/path_containment.rs stub with pure helpers has_traversal(path) and is_within(path, root). 7 unit tests, 100% coverage (security target). Pure std, no I/O, no canonicalize — TOCTOU-immune by construction. Available now for prompts_fs, worktrees and other adapters that will need containment checks. - Refactor infrastructure/filesystem/paths.rs::ensure_dirs to delegate to the new domain helpers, hoist project_forgent + global_root derivation out of the per-iteration helper (eliminates ~24 redundant PathBuf allocations per boot), inline a PROJECT_FORGENT_SUBDIR constant, and drop the needless borrow on starts_with. Behavior unchanged. cargo test --workspace 27 → 34, paths.rs coverage 99.28 → 99.29%, no clippy warnings. Skipped: phase enum (T-109 stub), try_join_all (boot-only path), AppConfig::global_root field (out of T-103 scope).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracts pure path-containment primitives ( ChangesPath Security & Directory Bootstrap
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant ENS as ensure_dirs
participant PC as path_containment
participant FS as Filesystem
Caller->>ENS: ensure_dirs(cfg)
ENS->>ENS: derive project_root & global_root
ENS->>ENS: build required_dirs list
loop each target
ENS->>PC: has_traversal(path)?
PC-->>ENS: bool
alt traversal found
ENS-->>Caller: AppError::Security
else
ENS->>PC: is_within(path, project_root/global_root)?
PC-->>ENS: bool
alt within allowed root
ENS->>FS: tokio::fs::create_dir_all(path)
FS-->>ENS: Result
else outside roots
ENS-->>Caller: AppError::Security
end
end
end
ENS-->>Caller: Result<(), AppError>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/infrastructure/filesystem/paths.rs`:
- Around line 30-33: The loop currently interleaves validation and creation
causing partial side effects; first collect all directories from
required_dirs(cfg, &project_forgent), run ensure_under_allowed_root for each
(using the same project_forgent and global_root) and fail early if any
validation errors occur, and only after all validations succeed iterate and call
tokio::fs::create_dir_all for each dir; update the code around the
required_dirs/ensure_under_allowed_root/create_dir_all usage to implement this
two‑phase validate-then-create flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c29238ab-f8dd-4689-99c5-e1d4e662b3da
📒 Files selected for processing (4)
CHANGELOG.mdsrc-tauri/src/domain/security/path_containment.rssrc-tauri/src/infrastructure/filesystem/mod.rssrc-tauri/src/infrastructure/filesystem/paths.rs
Addresses CodeRabbit review on PR #20. The previous loop interleaved ensure_under_allowed_root with tokio::fs::create_dir_all, so a malformed cfg.prompts_overrides_dir (outside both allowed roots) could create the project's .claude/forgent subtree before failing on the prompts loop. First iteration runs containment validation over all 12 candidates; second iteration calls create_dir_all only after every path passes. Strengthened ensure_dirs_refuses_dir_outside_two_allowed_roots with negative assertions on <project>/.claude and <home>/.forgent non-existence after a validation failure.
Summary
Implements task T-104: filesystem bootstrap with path-containment isolation.
ensure_dirs()creates Forgent's 12-directory layout under two allowed roots (<project>/.claude/forgent/and~/.forgent/). Path containment is enforced before any syscall via lexical check that rejects..traversal and validates against allowed roots—TOCTOU-immune by construction.Secondary refactor extracts reusable path primitives into pure domain helpers (
has_traversal,is_within) for use by other adapters (prompts_fs, worktrees, etc.).Closes T-104. PRD §0.3, ARCHI §5.5 / §9.2.
Why
Forgent's path-isolation guarantee (PRD §0.3) requires the app to never write outside the two allowed roots. Boot must create the full directory structure idempotently before any I/O. Extracting primitives to domain layer makes containment checks reusable and encourages composition over duplication across future filesystem consumers.
Changes
pub async fn ensure_dirs(cfg: &AppConfig)ininfrastructure/filesystem/paths.rsthat creates 12 directories across two allowed roots, enforcing path containment before eachcreate_dir_allsyscall.domain/security/path_containment.rsstub withhas_traversal()(lexical rejection ofParentDircomponents) andis_within()(component-aware prefix matching).ensure_dirsto delegate to domain helpers, hoist root derivation (eliminates ~24 redundant allocations per boot), and inlinePROJECT_FORGENT_SUBDIRconstant.Testing
All tests pass, linting green. tempfile-based integration tests verify behavior on temp filesystem; lexical checks immune to TOCTOU races.
Related Issues
Closes #T-104
Checklist
Summary by CodeRabbit
Security Improvements
New Features
Refactor
Tests