refactor: migrate 6 handler groups onto declared safety policies (step 3a)#7
refactor: migrate 6 handler groups onto declared safety policies (step 3a)#7stevehansen wants to merge 1 commit into
Conversation
…olicies Step 3 of the deep Safety Policy RFC (specs/RFC-safety-policy.md), building on the foundation (#6). Moves the hand-rolled safety checks in git, db, docker, process, npm, and pnpm onto declarative Policy chains attached at registration and enforced once at the dispatch site. Handlers keep only usage checks and the actual execution; the safety verdict is now data, not scattered control flow. Closes two latent defects for these groups: - #1 --flag=value bypass: BlockFlags normalizes via Flag.Base, so --force=true, --accept-data-loss=..., etc. are now caught (git push, db migrations, npm audit-fix), where the old exact-token blocklists let them through. - #3 --json blocked-envelope fork: blocked output now renders through the central dispatcher (ConsoleRenderer.Blocked, which has a JSON branch), so `safe git push --force --json` emits a structured envelope instead of markup. Also collapses the 3x-duplicated run-script allowlist (npm/pnpm/bun) into a single PackageScripts.Allowed, and makes CommandRegistry.Initialize() idempotent and thread-safe -- harmless under production's single startup call, but the new parallel test collections raced on the shared list. Out of scope (deferred to follow-up PRs): proxy AllowedFlags enforcement (defect #2, which also requires untangling the Program.cs dispatch bypass) and file/generate path-containment -- both still validate inline. Tests: 92 -> 157, green across repeated runs. 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 the validation logic across several command groups (Git, DB, Docker, Process, NPM, PNPM, Bun) by migrating from manual inline checks to a declarative, policy-based framework. It also consolidates the allowed package scripts into a shared class, makes the command registry initialization thread-safe, and adds a comprehensive test suite. The review feedback is highly constructive, pointing out opportunities to prevent security bypasses by blocking the git push '-d' short flag, ensuring consistency by blocking the '*' wildcard in git add, and improving usability by allowing multiple files to be restored in git checkout-file.
| private static readonly HashSet<string> LogAllowedFlags = ["-n", "--oneline", "--graph", "--format", "--pretty", "--author", "--since", "--until", "--all", "--stat", "--no-merges", "--first-parent", "--reverse", "--abbrev-commit", "--date"]; | ||
| private static readonly HashSet<string> DiffAllowedFlags = ["--staged", "--cached", "--name-only", "--name-status", "--stat", "--shortstat", "--numstat", "--diff-filter", "--no-color", "--color=never", "--unified", "-U"]; | ||
| private static readonly HashSet<string> GitValueFlags = ["-n", "--format", "--pretty", "--author", "--since", "--until", "--date", "--diff-filter", "--unified", "-U"]; | ||
| private static readonly HashSet<string> PushBlockedFlags = ["--force", "-f", "--delete", "--no-verify"]; |
There was a problem hiding this comment.
The PushBlockedFlags set is missing the short flag -d, which is the standard alias for --delete in git push. Without blocking -d, users could bypass the deletion block by running safe git push origin -d . Please add -d to the blocked flags set to prevent this bypass.
private static readonly HashSet<string> PushBlockedFlags = ["--force", "-f", "--delete", "-d", "--no-verify"];| private static readonly HashSet<string> GitValueFlags = ["-n", "--format", "--pretty", "--author", "--since", "--until", "--date", "--diff-filter", "--unified", "-U"]; | ||
| private static readonly HashSet<string> PushBlockedFlags = ["--force", "-f", "--delete", "--no-verify"]; | ||
| private static readonly HashSet<string> CommitBlockedFlags = ["--no-verify"]; | ||
| private static readonly HashSet<string> AddBlockedArgs = ["-A", "--all", "."]; |
There was a problem hiding this comment.
The AddBlockedArgs set blocks ., but does not block the wildcard *. While shell expansion often resolves * before it reaches the application, blocking * explicitly ensures consistency with checkout-file (which blocks both . and *) and guards against staging all files if the wildcard is passed literally. Consider adding * to AddBlockedArgs.
private static readonly HashSet<string> AddBlockedArgs = ["-A", "--all", ".", "*"];| return 1; | ||
| } | ||
|
|
||
| return RunGit(["checkout", "--", args[0]], json); |
There was a problem hiding this comment.
Currently, RunCheckoutFile only restores the first file (args[0]) and ignores any subsequent files passed to the command. Since the policy BlockFlags([., *]) evaluates all arguments in the vector, it is completely safe to pass all arguments to RunGit using the spread operator (..args). This will allow users to restore multiple files at once (e.g., safe git checkout-file file1.cs file2.cs).
return RunGit(["checkout", "--", ..args], json);
What & why
Step 3 of the deep Safety Policy RFC (
specs/RFC-safety-policy.md), stacked on the foundation in #6. The foundation introduced thePolicyvocabulary, the domain ports, and central enforcement at the dispatch site, but migrated onlybunas a proof of concept. This PR migrates the inline, hand-rolled safety validation in 6 of the remaining groups —git,db,docker,process,npm,pnpm— onto declarativePolicychains.Each handler's safety check (
OutputFormatter.WriteBlockedblocks, blocklistforeachloops,FilterFlags, the git clean-tree / not-a-repo / amend-already-pushed probes) becomes data attached at registration and evaluated once before the handler runs. Handlers keep only their usage checks and the actual execution.What landed
RequireGitRepo()on every command (the probe spawnsgitonce per run, not ~25×);push/commit/add/checkout/checkout-fileblock rules;commit-amend→RequireHeadNotPushed();pull/checkout/merge→RequireCleanTree();log/diffflag-allowlists →AllowOnlyFlagsrewrite.BlockFlags(DestructiveFlags);artisan-migrate→BlockSubstrings;django-migrate→BlockFlags(["zero"]).compose-down→BlockFlags;build/compose-upFilterFlags→AllowOnlyFlags.kill-name→AllowOnlyFirstArg.run→AllowOnlyFirstArg; npmaudit-fix --force→BlockFlags.PackageScripts.Allowed.Defects closed (for these groups)
--flag=valuebypass —BlockFlagsnormalizes viaFlag.Base, so--force=true,--accept-data-loss=…, etc. are now caught (git push, db migrations, npm audit-fix).--jsonblocked-envelope fork — blocked output now renders through the central dispatcher (ConsoleRenderer.Blocked, which has a JSON branch), sosafe git push --force --jsonemits a structured envelope instead of Spectre markup.Out of scope (follow-up PRs)
AllowedFlags— also needs theProgram.csproxy early-return untangled so proxy commands reach the dispatcher.--in-value cases) than the foundation shipped.Behavior deltas (intentional, RFC-sanctioned)
RequireGitRepo"Not a git repository" now renders as aBlocked:envelope rather than a plainError:; block messages show the whole arg vector; db block reasons are generic;kill-namelists 15 names instead of 10; dockerFilterFlagsunified onto the explicit value-flag algorithm;artisan/djangoblocks are now case-insensitive (strictly safer); safety runs before a handler's usage check.Tests
92 → 157 passing, green across repeated runs. New
MigratedCommandPolicyTests.csasserts each migrated command's wired policy directly (allowed/rewrite cases never dispatch, to avoid spawning real tools); blocked-path +--jsonenvelope are checked through the dispatcher.Review notes resolved
Reviewer flagged a flaky suite:
CommandRegistry.Initialize()was neither idempotent nor thread-safe, and the new parallel test collections raced on the shared list. Fixed at the root (double-checked guard); harmless to production's single startup call.🤖 Generated with Claude Code