refactor: migrate file/generate path containment onto declared policies (3b)#8
refactor: migrate file/generate path containment onto declared policies (3b)#8stevehansen wants to merge 1 commit into
Conversation
Move all 14 FileCommands.ValidatePath call sites and the GenerateCommands.RunHashFile inline copy onto declared Policy chains evaluated centrally at the dispatch seam. Adds a PathArg selector (Positional/FlagValue), RequirePathWithinProjectRule(PathArg), and RequireWithinSafeDeleteDirRule; delete-pattern's safe-dir ancestor walk becomes a rule. copy/move chain two positional checks; find/delete-pattern target the --in value. Closes defect #3 (the --json blocked-envelope fork) for path-containment blocks: blocked render now goes through the json-aware ports.Render.Blocked instead of OutputFormatter.WriteBlocked. STRIDE E1/I2 mitigations updated to the centrally-evaluated, boundary-tested rules (v3 review row dated 2026-05-30). hash-file's path selector is defined once (GenerateCommands.HashFilePath) and shared by BOTH the policy and the handler, so the validated token is always the hashed token -- closing a decoy-positional containment bypass found in review (B1). Non-path domain blocks (overwrite/tracked/uncommitted/dest-exists) are intentionally left on the handlers' legacy path until issue #2. Tests 198 -> 200. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request refactors path containment and sandboxing checks by migrating them from inline validation inside command handlers to centrally evaluated declarative policies (RequirePathWithinProjectRule and RequireWithinSafeDeleteDirRule) at the CommandDispatcher seam. It also introduces a PathArg abstraction to robustly extract path arguments from positional and flag values, preventing bypasses. Feedback is provided regarding a potential case-insensitivity bug in RequireWithinSafeDeleteDirRule where checking SafeDirs.Contains(seg.ToLowerInvariant()) will fail if SafeDirs contains mixed-case or uppercase strings, with a suggestion to use StringComparer.OrdinalIgnoreCase instead.
| foreach (var seg in rel.Split('/', '\\', StringSplitOptions.RemoveEmptyEntries)) | ||
| if (SafeDirs.Contains(seg.ToLowerInvariant())) return new PolicyResult.Allow(); |
There was a problem hiding this comment.
The current implementation of RequireWithinSafeDeleteDirRule performs a case-insensitive check by lowercasing the segment (seg.ToLowerInvariant()) and checking if SafeDirs contains it. However, if SafeDirs contains any mixed-case or uppercase strings (such as "TestResults" in SafeDeleteDirs), the Contains check will fail because SafeDirs is queried with a fully lowercased string.\n\nTo make this rule robust and fix the latent bug with "TestResults" without relying on the caller to pre-lowercase the collection, use StringComparer.OrdinalIgnoreCase with Enumerable.Contains.
foreach (var seg in rel.Split('/', '\\', StringSplitOptions.RemoveEmptyEntries))\n if (SafeDirs.Contains(seg, StringComparer.OrdinalIgnoreCase)) return new PolicyResult.Allow();
Step 3b — file/generate path containment → declared Policy
Third in the deep Safety Policy stack: foundation (#6) → 3a (#7) → 3b (this). Base is
feat/safety-policy-step3a.What this does
Moves every path-containment check from inline handler code onto declared
Policychains evaluated once atCommandDispatcher.Execute:FileCommands.ValidatePathcall sites + the private method, plusGenerateCommands.RunHashFile's inline copy, removed.PathArgselector (Positional(index, valueFlags)/FlagValue(flag)),RequirePathWithinProjectRule(PathArg), andRequireWithinSafeDeleteDirRule(delete-pattern's ancestor walk).RequirePathWithinProject();find/delete-pattern→FlagValue("--in");copy/move→ two chained positional checks;delete-pattern→ within + safe-dir chain;hash-file→Positional(0, ["--algorithm"]).Why
ValidatePath).--jsonblocked-envelope fork) for path blocks — blocked render now flows through the json-awareports.Render.Blockedinstead ofOutputFormatter.WriteBlocked.Review caught one blocking regression (B1), fixed in this PR
The first cut wired
hash-file's policy and handler to different path extractors, which diverged on a decoy input (hash-file sha256 <outside> --algorithm sha256) — a reproduced containment bypass. Fixed by defining the selector once (GenerateCommands.HashFilePath) and sharing it between policy and handler, so the validated token is always the hashed token. A fake-workspaceEvaluatecan't surface this (identityResolveinverts the production in/out decision), so the regression guard is at the shared-selector level.Scope / deferrals
SafeDeleteDirsentry"TestResults"never matches (candidate is lowercased before an ordinalContains). Not introduced here; faithfully preserved. Worth a one-line fix separately.AllowedFlags/ defect RFC: Introduce ports-and-adapters seam for handlers (IExecutor, IRenderer, IGitRepo) with a thin Run.* sugar layer #2) still deferred — needs theProgram.csproxy dispatch-bypass untangled first.Tests
198 → 200. Build clean (0 warnings under
-warnaserror); suite green across 3 consecutive runs. implement-issue pipeline verdict: verify PASS (8/8 conformance), review PASS after the B1 fix.🤖 Generated with Claude Code