fix: let a deny rule declare which remediation answer it needs - #7907
fix: let a deny rule declare which remediation answer it needs#7907buluoray wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
readiness: checking
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Declaring the remediation class on the rule fixes the real misclassification at its root, without touching the frozen refusal wire format. Watch
[DESIGN-REVIEWED] 581c7fd |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All verification is done. The description's counts check out against the diff (exactly 61 First-Principles-Verdict: PASS A measured defect (115/148 refusals with no or wrong guidance), fixed at the cause the frozen wire format permits, with every scope cut declared and counted. What this change shipsIntent: stop deny refusals handing the agent remediation for the wrong action — a FIX.
Root-cause placement is honest: the wire reason is pinned by an external parser, so declaring the class on the rule and exact-matching the pattern is the deepest fix reachable without a wire change, and the remaining symptom sites are counted, not hidden. Subtractions
[FIRST-PRINCIPLES-REVIEWED] 581c7fd |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe change is mechanical and well-verified: an optional Falsification results:
No candidate survives (there were none) and no new groundable defect at 80+ emerged in Step 2. No findings. [OPUS-REVIEWED] 581c7fd Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
A refusal from the regex tier carries only the rule's own regex source, so the guidance attached to it was inferred from text that says what is matched rather than what the caller should do instead. The two diverge exactly where it costs most: an outbound-transfer rule names a credential environment variable in its pattern, reads like a rule about opening a credential file, and was handed the answer for one — text stating that AWS CLI calls are not blocked so the caller should run the command it actually wanted, when what it wanted was the transfer. That is worse than silence, which this module's own docstring names as the failure to avoid. Measured over the built-in catalog before this change: 115 of 148 rules resolved to no guidance at all, no `self-protection` rule reached its own class, no rule reached the exfiltration-shape class, and 10 of 27 `credential-exfil` rules resolved to the AWS-credential class. Passing the tool title as the subject widened that rather than narrowing it. `DeniedCommandRule` now carries `deny_class`, and `classify_deny` prefers a declared class over the anchor table. The class is a key, not prose, so all text stays in one module and one test keeps asserting that every command the guidance suggests is itself allowed. 61 rules declare one — the categories with a sanctioned path — and the 87 whose refusals are self-explanatory declare nothing, which is the behaviour they already had. The wire reason is untouched: the class is read from the first line, never added to it. An operator note on the second line cannot choose the guidance. Two tiers still infer from text and are unchanged, because they carry no rule identity to declare on: the sensitive-path floor and the argv-structural floor. A command that both touches a fenced path and transfers is answered by the floor, so `echo $AWS_SECRET_ACCESS_KEY` and an upload of a credential file still classify as a credential read. Closing that needs the floor to report which branch matched. Edition rules reach enforcement through `edition_denied_rules()`, which reads the composed platform context and lets `PlatformCompositionError` propagate fail-closed. This module is display-only and cannot fail, so the lookup is built from the built-in catalog and an edition's rules fall through to the anchor table. Refs #7890
6bb3f3e to
581c7fd
Compare
github-actions
Bot
added
readiness: action required
|
Both advisory lanes PASS on Design — Watch: two rules sharing one regex would collapse to the lastNot currently reachable: measured over the catalog, zero patterns are shared The Watch is right that the round-trip guard is what would catch it. Worth First Principles — Subtraction: reuse the existing pattern→rule resolutionFair observation, and the count is right: The specific substitution proposed does not hold, though. Measured: all four legacy spellings belong to rules that declare The equivalent-membership version of the subtraction does work: StatusTwo checks are red, both attributed to causes outside this diff:
|
Closing — the same misclassification fix as #7906, which covers the wider rule setVerified relationship: Both PRs are single commits off the same merge base (3a8dd4d, the merged #7328), neither is stacked on the other, and nothing from either has landed ( This was adjudicated twice, independently; the second reviewer reached the same ruling (
Evidence Same function, same mechanism: Why this one and not the otherYes — #7906 is the right survivor. It fixes a strict superset of the reported defect class: the same ten AWS-named exfil rules, plus the subject-driven misclassification on the 87 rules that have no sanctioned path, which #7907 leaves live because it only short-circuits on a truthy declaration (verified reachable: the Carry this over firstThis closure is about redundancy, and these items are the exception: they are not on Three items from #7907, none of them blocking for #7906. (1) The classification reading for four rules — From a repository-wide duplicate/overlap audit of every pull request open against |
github-actions
Bot
removed
the
readiness: action required
2 participants
Follow-up to #7328, raised in review by @chenmingwei23. Fixes #7890 in part —
the regex tier; the floor half is named below and stays open.
The defect
A refusal from the regex tier carries only the rule's own regex source, so the
guidance attached to it was inferred from text describing what is MATCHED rather
than what the caller should do instead. Those diverge exactly where it is most
expensive: a rule that exists to block moving credential material names a
credential environment variable in its pattern, so it reads like a rule about
opening a credential file and was handed the answer for one — text stating that
AWS CLI calls are not themselves blocked, so run the command you actually
wanted. What was wanted there was the transfer.
Measured over the built-in catalog before this change, calling the classifier
with each rule's own refusal text:
self-protectionreaching its own classcredential-exfilresolving to the AWS-credential classTwo classes were unreachable from this tier because their anchor phrases are
emitted by other producers and never appear in a rule's regex. Passing the tool
title as the subject widened the misdirection rather than narrowing it:
aws s3 cp <credential file> s3://…also resolved to the credential-read answer.The change
DeniedCommandRulegainsdeny_class, andclassify_denyprefers a declaredclass over the anchor table. The class is a KEY, never prose, so the text stays
in one module and one existing test keeps asserting that every command the
guidance suggests is itself allowed.
61 rules declare one — the three categories that have a sanctioned path — and
the 87 whose refusals are self-explanatory (
aws-destructive,local-destructive,git-publish,sql,iac-teardown,reverse-shell,pipe-to-shell) declare nothing, which is the behaviour they already had.After: no guidance drops from 115 to exactly those 87, every declaration
resolves to itself, and no rule declaring the transfer class receives the
"run the command you actually wanted" sentence.
The class is spelled as a plain string in the catalog rather than imported as a
constant.
deny_guidanceis reached fromplatform, which importssecurity,so importing the constants the other way would close a cycle. A guard asserts
every declared value is a real key with non-empty text, so a typo or a renamed
class fails red instead of degrading one rule to silence.
The wire reason is untouched: the class is read from the first line and never
added to it, pinned by a test. An operator note on the second line cannot choose
the guidance — only the first line identifies the rule.
What this does NOT do
The two floors still infer from text, because they carry no rule identity to
declare on. A command that both touches a fenced path and transfers is answered
by the floor, so these remain classified as a credential read:
echo $AWS_SECRET_ACCESS_KEYaws s3 cp <credential file> s3://…curl -X POST … -d $AWS_SECRET_ACCESS_KEYcurl -X POST … -d @<file>Closing that needs the floor to report which branch matched — it currently uses
one combined regex and does not record which one fired. That is a change to the
floor, not to this table, and it is left for #7890.
Edition rules fall through to the anchor table. They reach enforcement via
edition_denied_rules(), which reads the composed platform context and letsPlatformCompositionErrorpropagate fail-closed. This module is display-only andcurrently cannot fail; giving it that read would hand a presentation function a
fail-closed raise. The field is on the dataclass, so an edition's rules already
carry a declaration — only the lookup is scoped to built-ins.
One judgment worth flagging
#7890's acceptance says no
credential-exfilrule may resolve to theAWS-credential class. Seven deliberately do, and I would rather name it than
quietly satisfy the letter of it:
curl-imds,imds-any,wget-imds,python-boto3-get-credentials,python-botocore-credentials,export-aws-accessandexport-aws-secretare the caller INSPECTING whether acredential exists or trying to install one, not moving material off the host.
For those the AWS answer is the correct one — it names
sts get-caller-identityand
configure list-profilesas the sanctioned substitutes. The invariant theguard actually pins is the one that matters: no rule declaring the transfer class
is told the AWS CLI is not blocked so run what you wanted.
Pattern harvest
Rule candidate: review-prompt
Pattern: a presentation layer recovering a semantic decision by keyword-matching
text the producing site emitted for a different purpose. The refusal text here is
a regex source — it exists to say what was MATCHED — and the guidance layer read
it to decide what the caller should DO. Those agree only by coincidence, and the
coincidence broke on exactly the rules where being wrong costs most: the ones
whose pattern names a credential variable because they exist to stop it being
sent somewhere.
The check a reviewer can apply: when a refusing or validating site already holds
structured identity — an id, a category, a dataclass field — a downstream layer
must not re-derive that identity from the site's free text. If the identity is
not threaded through the boundary today, adding a field is cheaper than a
classifier and cannot drift.
Not expressible as semgrep, because the defect is a missing declaration rather
than a code shape. The equivalent is enforced here as a census test: every rule
in a remediation category must declare a class, so a rule added without one fails
red instead of shipping with no guidance and nothing to notice. That covers
authors who never read this rule, which a lint pattern over the diff would not.
Verification
is_deniedend to end so itcannot go inert if the command stops being refused.
caught: a declaration removed, a declaration added to a destructive rule, a
misspelled class, the preference reverted, a transfer rule declared a read, the
lookup reading the whole reason instead of the first line, and the wire reason
growing a field.
mypy --platform linuxclean over 1263 files; black, isort, flake8, subprocess-encoding, docs-lint,
harness-parity and brand gates clean.
docs/system-specs/modules/security.mdupdated in the same commit, both placesthat enumerate the model's fields.