fix(hooks): scope prod-ops-no-approval to mutating verbs, not resource names - #50
Conversation
…e names The rule blocked on the NAME of the target: any command carrying --project=<something>-prod was refused, including pure reads. A `<tool> services list --project=<name>-prod` could not run, so an inventory sweep returned that project as its only unverified cell while the non-prod cells filled in normally. The hook did exactly what its code said and produced a missing datum. That matters past the nuisance. A guard that blocks reads teaches people to route around it, and the habit of routing around it does not distinguish the read it over-blocked from the write it was there to stop. A guard too wide ends up protecting less than a precise one. The rule now matches the VERB. Three AND-chained conditions, each reading a different surface of the command: 1. Does this name a production target? -> raw command (it is a flag) 2. Is this a mutation? -> flag-stripped command 3. Is it a dry run? -> raw command (also a flag) Condition 2 is an explicit block-list, so an unrecognised verb is ALLOWED. Read verbs are open-ended; mutating verbs are a closed, reviewable set, and enumerating reads is the failure being fixed. Adds `normalize: strip-flags` to the match-condition schema to support this — per-condition, not per-rule, since one rule needs both surfaces. Without it, `<tool> --project=x <group> delete` and `<tool> <group> delete --project=x` read differently, and moving a flag becomes a bypass. Unknown normalize values fail open at runtime and are rejected at build time. Tests: 5 of the 12 fixtures fail against the pre-fix engine, including the literal read that was refused in the field. The 5 blocking fixtures return exit 2 both before and after — without that pair, a fix that opened the guard entirely would have looked green. Created by Claude Code on behalf of @kvttvrsis Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@dojo-code-reviewer review |
![dojo-code-reviewer[bot]](https://avatars.githubusercontent.com/in/3092887?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Changes Requested
Changes requested — 1 blocker, 1 P3. Confidence: 1.00/5.00.
Walkthrough
main branch directly, bypassing the standard GitFlow pathway of feature -> develop -> main. Please verify if this direct target is intended and authorized.
Summary of Changes
This pull request refactors the prod-ops-no-approval pre-commit rule from a coarse resource-name check to a precise, three-stage AND-chained check verifying production targets, mutating verbs (using a newly implemented strip-flags normalizer), and dry-runs. It also adds a build-time schema check to reject unrecognized normalizers.
Areas Reviewed
hooks/lib/eval-rule.sh: The implementation of thestrip-flagsnormalizer insed.hooks/rules/rules.yaml&rules.json: The rule definitions, patterns, and test cases.scripts/build-rules.mjs: The validation logic for thenormalizefield.
Safety Rationale
The proposed logic improves tool usability by allowing harmless read-only commands (e.g., list, describe), but it is not yet safe to merge due to a regex boundary bug that allows common hyphenated/underscored mutating commands (e.g., create-bucket, start-instances) to bypass the hook.
Changes requested — 1 blocker, 1 P3.
🔴 P1 — Blockers
hooks/rules/rules.yaml:147— 🔴 P1 (blocker) — The regex pattern boundary ([^[:alnum:]_-]) used as a suffix for mutating verbs prevents the hook from matching any hyphenated or underscored subcommands (such ascreate-bucket,delete-instance, orstart-instances), which are extremely common in AWS, Azure, and other cloud CLI tools.
Because characters like - and _ are explicitly forbidden immediately after the verb, a command like aws s3api create-bucket --project=prod will completely bypass this mutation protection.
Solution
Change the trailing pattern boundary from ([^[:alnum:]_-]|$) to ([^[:alnum:]]|$). This allows hyphens or underscores to follow the mutating verb (supporting hyphenated subcommands) while still preventing false matches on alphabetical extensions (like creates or deploying).
[pass 1]
🔵 P3 — Minor
hooks/lib/eval-rule.sh:199— 🔵 P3 (minor) — Multiple commands separated by semicolons within a singlesedstring is a GNU extension. On standard POSIX/BSDsed(such as the default on macOS), this pattern can fail or trigger syntax errors.
To ensure complete portability across GNU and BSD platforms (consistent with the portability note DOJ-4571 in other hooks), use multiple -e options to chain the substitutions instead.
Additionally, using [^[:space:]]* to strip flag values can lead to partial stripping when flag values contain spaces (e.g. --filter="name = delete" becomes = delete"), which may then trigger false positives. While the author noted that quoted arguments are not stripped, handling nested double quotes on flags can be improved by matching quoted or unquoted values in the first substitution step.
[pass 1]
Total findings: 1 security, 1 business context (2 total)
| ;; | ||
| strip-flags) | ||
| VALUE="$(printf '%s' "$VALUE" | sed -E 's/--[A-Za-z0-9_-]+=[^[:space:]]*//g; s/--[A-Za-z0-9_-]+//g; s/[[:space:]]+/ /g')" | ||
| ;; |
There was a problem hiding this comment.
🔵 P3 (minor) — Multiple commands separated by semicolons within a single sed string is a GNU extension. On standard POSIX/BSD sed (such as the default on macOS), this pattern can fail or trigger syntax errors.
To ensure complete portability across GNU and BSD platforms (consistent with the portability note DOJ-4571 in other hooks), use multiple -e options to chain the substitutions instead.
Additionally, using [^[:space:]]* to strip flag values can lead to partial stripping when flag values contain spaces (e.g. --filter="name = delete" becomes = delete"), which may then trigger false positives. While the author noted that quoted arguments are not stripped, handling nested double quotes on flags can be improved by matching quoted or unquoted values in the first substitution step.
[pass 1]
| # describe, get, read, logs, versions, ...) and enumerating them all is | ||
| # the failure this rule is being fixed for. Mutating verbs are a closed, | ||
| # reviewable set. | ||
| - field: command |
There was a problem hiding this comment.
🔴 P1 (blocker) — The regex pattern boundary ([^[:alnum:]_-]) used as a suffix for mutating verbs prevents the hook from matching any hyphenated or underscored subcommands (such as create-bucket, delete-instance, or start-instances), which are extremely common in AWS, Azure, and other cloud CLI tools.
Because characters like - and _ are explicitly forbidden immediately after the verb, a command like aws s3api create-bucket --project=prod will completely bypass this mutation protection.
Solution
Change the trailing pattern boundary from ([^[:alnum:]_-]|$) to ([^[:alnum:]]|$). This allows hyphens or underscores to follow the mutating verb (supporting hyphenated subcommands) while still preventing false matches on alphabetical extensions (like creates or deploying).
[pass 1]
There was a problem hiding this comment.
✅ Approved
Approved — 0 blockers, 1 P3. Confidence: 4.80/5.00.
Walkthrough
Summary of Changes
This pull request narrows the scope of the prod-ops-no-approval rule from checking all commands containing production-named targets to specifically targeting mutating verbs on production-named targets. To support this without being bypassed by changing flag positions, a new normalize: strip-flags condition transform is introduced, which strips double-dash flags before matching.
Files Reviewed
hooks/lib/eval-rule.sh: Normalization logic andstrip-flagssed expression.hooks/rules/rules.yaml/rules.json: Rule configurations, added conditions, updated messages, and comprehensive test fixtures.scripts/build-rules.mjs: Build-time validation for known normalization methods.hooks/rules/README.md&CHANGELOG.md: Documentation updates.
Safety Rationale
The implementation is highly safe to merge because it preserves the strict matching logic for target detection, uses robust multi-condition logical AND gates, and is backed by thorough unit tests confirming that both read-only commands (including dry runs) are permitted while actual mutations remain securely blocked.
Approved — 0 blockers, 1 P3.
🔵 P3 — Minor
hooks/rules/rules.yaml:147— 🔵 P3 (minor) — Common CLI tools (such as Docker, Helm, git, npm, yarn, or AWS CLI) often userm,remove,uninstall, orterminateas their core mutating/destructive verbs. Currently, the regex only capturesremovewhen followed by a hyphen (such asremove-iam-policy-binding), meaning standalone commands likehelm uninstallordocker rmtargeting production resources are not blocked.
Adding these four common verbs to the block-list pattern will make the protection more robust.
[pass 1]
Total findings: 1 business context (1 total)
| # describe, get, read, logs, versions, ...) and enumerating them all is | ||
| # the failure this rule is being fixed for. Mutating verbs are a closed, | ||
| # reviewable set. | ||
| - field: command |
There was a problem hiding this comment.
🔵 P3 (minor) — Common CLI tools (such as Docker, Helm, git, npm, yarn, or AWS CLI) often use rm, remove, uninstall, or terminate as their core mutating/destructive verbs. Currently, the regex only captures remove when followed by a hyphen (such as remove-iam-policy-binding), meaning standalone commands like helm uninstall or docker rm targeting production resources are not blocked.
Adding these four common verbs to the block-list pattern will make the protection more robust.
[pass 1]
1 participant
Narrows
prod-ops-no-approvalso it matches the verb, not the resource name.Linear issue:
[to-be-filed]The defect
The rule was one condition: does the command carry a production-named target (
--project=<something>-prod,--env=prod,ENV=prod,--target=prod). That is a test on the name of the target, not on what the command does to it — so every read against a production-named resource was refused.Observed in the field: an agent could not run a plain
services listagainst a production-named project. A pure read, no mutating verb anywhere in it. That project came back as the single unverified cell of an inventory that covered its three siblings normally. The hook did exactly what its code said and produced a missing datum.The reason this matters past the nuisance: a guard that blocks reads teaches people to route around it, and the habit of routing around it does not distinguish the read it over-blocked from the write it existed to stop. A guard that is too wide ends up protecting less than a precise one.
(Live confirmation: opening this very PR was blocked by the installed pre-fix hook, because the PR body quotes the test fixtures. The body had to be moved out of the command line.)
The fix
Three AND-chained conditions, each reading a different surface of the same command:
Condition 2 is an explicit block-list, so an unrecognised verb is allowed. That direction is deliberate: read verbs are open-ended (
list,describe,get,read,logs,versions, …) and trying to enumerate them is the failure being fixed here. Mutating verbs are a closed, reviewable set —create,update,delete,destroy,patch,replace,deploy,apply,start,stop,restart,scale,resize,enable,disable,grant,revoke,import,restore,rollback,migrate,drop,truncate,purge,promote,rotate, plus theset-*/add-*/remove-*/unset-*families.New:
normalize: strip-flagsSupporting the above required a schema addition — a per-condition transform applied before the pattern (drops
--flagand--flag=value, collapses whitespace).It is per-condition, not per-rule, because this rule genuinely needs both surfaces: condition 1 matches a flag, condition 2 must not see flags at all. Without normalization, a global flag placed ahead of the verb reads differently from the same flag placed after it, and moving a flag becomes a bypass — which is not a guard. Same technique, same reason, as the ad-hoc-ops hook that motivated this shape.
Two deliberate limits, both documented in
hooks/rules/README.md:bash -c "…"payload. A false negative in a guard is silent; a false positive is visible and has a documented bypass. Residual cost: a read whose quoted argument contains a mutating word can still trip the rule.normalizefails open at runtime rather than matching un-normalized input, which would silently restore the broad behavior.build-rules.mjsrejects unknown values at build time, so the fail-open path is reachable only from a hand-editedrules.json.Tests
12 fixtures on the rule. Replayed against the pre-fix engine from
main:The two rows that carry the argument:
allows-readonly-list-against-prod-projectis the literal shape that was refused. It fails before, passes after.blocks-mutation-against-same-prod-projectis its mandatory counterpart — a real mutation against the same production project. It returns exit 2 both before and after. Without that pair, a "fix" that simply opened the guard would satisfy the first test and certify itself green.All five blocking fixtures are unchanged pre/post, which is the evidence that the guard was narrowed rather than disabled.
Blocking is exit 2, never 1 — an exit 1 is reported as non-blocking and the command still runs, which reads as enforcement while providing none. The fixtures assert
2explicitly.Full suite:
bash hooks/test-hooks.sh→ 328/328.npm test→ 60/60.Notes for review
blocks-prod-project-flagis gone by design — it asserted that acompute instances listwith a production project flag blocks, and that command is a read. It is replaced byallows-read-with-flag-before-verb(same command, inverted expectation) plusblocks-delete-with-flag-before-verb(same flag position, mutating verb). This is the intended behavior change, called out here so it is not mistaken for a deleted test.rules.jsonis regenerated, not hand-edited (npm run build-rules); CI's no-drift check should be satisfied.CHANGELOG.mdunder## [Unreleased], so whichever merges second will want a one-line conflict resolution there.Created by Claude Code on behalf of @kvttvrsis