fix: key deny remediation by rule identity in the regex tier - #7906
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Root-cause fix at the right seam: the tier that knows which rule fired now answers from identity, and text scanning is confined to producers that genuinely have only text. The one structural risk — the identity lookup re-parsing the refusal's first line — is a contract RecoveryCard already depends on and [DESIGN-REVIEWED] a6eea32 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/deny_guidance.py:313 -- [GPT-REVIEWED] a6eea32 False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified: the fix has 3 production consumers ( First-Principles-Verdict: PASS One root cause — the rule-naming tier inferred class from regex wording — fixed at that cause, with every sibling route (argv floor, git-publish id, subject leak) covered. What this change shipsIntent: stop a blocked credential-exfil command from receiving guidance that invites rerunning it — a FIX.
Watch
[FIRST-PRINCIPLES-REVIEWED] a6eea32 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo findings. The sole candidate is a cache-staleness hypothesis on [OPUS-REVIEWED] a6eea32 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
22ca2dd to
72866a9
Compare
Audit note — #7907 is being closed in favour of this PRTwo independent adjudicators both ruled this the surviving implementation. What the two sharedSame function, same mechanism: Why this oneYes — #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 What #7907 had that this PR does notPlease pick these up (or say they are not wanted): 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 |
72866a9 to
923a6b4
Compare
|
Thanks for the audit note. All three items are answered below; two are now in the (1) The four rules -- taken, and your reading is the right one
That makes the exception list to #7890's acceptance criterion 1 seven rules, not (2) The
|
923a6b4 to
d10b575
Compare
Ten credential-exfil rules were answered with AWS credential-READ guidance
telling the caller that AWS CLI calls are not blocked and to rerun the command
it wanted. The class was recovered from the refusal text, so for a refusal that
names a rule it was read out of that rule's own regex source -- and those rules
name the AWS credential environment variables because that is what they exist
to catch.
A producer that names a catalog rule now takes the class from that rule's
identity and nothing else: _CATEGORY_CLASSES for the three categories that have
a sanctioned path, _RULE_CLASSES for the 20 rules whose answer is not their
category's. That holds when the rule has NO guidance too -- a rule's silence is
an answer, so `aws ec2 terminate-instances --profile sso` no longer draws
enterprise-SSO login prose from a word in its own arguments.
The index covers the EFFECTIVE catalog, built-ins plus whatever the edition
contributes through the DeniedRuleProvider seam, composed the way hooks.py
composes the enforced set. Indexing built-ins alone would leave exactly the
rules an enterprise adds on the scan. It is keyed by both the pattern and the
rule id, because a refusal leads with whichever its producer chose: the regex
tier and the argv floor report the pattern, the git-publish gated floor reports
the id. Keying patterns alone left that floor scanned, so `git push origin main
# rotate the sso session first` was answered with live-SSO-credential prose.
The anchor scan now answers only for producers that refuse generically -- the
fnmatch overlay, the sensitive-path floor, the exfiltration audit -- and it is
the only path that reads the subject: a command refused for moving a credential
necessarily contains that credential's name, so `aws s3 cp ./report.aws
s3://bucket` would otherwise be answered as a credential read.
Seven credential-exfil rules keep the credential-read answer because they move
nothing off the host: three fetch from the metadata endpoint, two resolve and
print through an SDK, two inject an attacker-chosen credential into the
environment. The outbound-transfer prose ("name the file and the destination")
answers a question those never ask.
All 61 rules in the three categories with a sanctioned path resolve; the other
87 answer silence. The wire reason is untouched -- no producer changed. A
catalog census fails when a rule in a remediation category resolves to no
guidance, when any rule's class comes from the anchor scan rather than its
identity, when two identities collide with different classes, and when a
_RULE_CLASSES row merely repeats its category's answer.
Closes #7890
d10b575 to
a6eea32
Compare
|
Rebutting the GPT 5.6 finding on The finding's premise is correct. The prescribed remedy is wrong. "Do not cache an index built from an empty The finding does not reach a defect, for a structural reason. The scenario needs Residual, recorded rather than hidden. If an edition ever ships a provider that I have not pushed for this. The head currently has zero blocking findings with |
iamwhatever
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: deny remediation prose keyed by rule identity instead of inferred from the rule's regex source, which mis-keyed ten credential-exfil rules onto credential-read guidance. Spec files changed as a ride-along (a minority of the diff on both file count and changed lines), not reviewed as a design decision: docs/system-specs/modules/learn-cron-dashboard.md.
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Problem / Motivation
A blocked tool call is handed remediation guidance chosen by the CLASS of thing
the gate refused, and the class was recovered from the refusal text. For the
built-in rule tier that reads the class out of the rule's own REGEX SOURCE, which
is accidental. Ten
credential-exfilrules -- the ones that block moving AWScredentials OUT -- name the AWS credential environment variables in their
patterns, because that is exactly what they exist to catch, so they landed in the
AWS credential-READ class. Its prose tells the caller that AWS CLI calls are not
themselves blocked and to go ahead and run the command it actually wanted:
Measured on this base before the change, over the whole built-in catalog: 115 of
148 rules resolved to no guidance at all,
self-protectionrules reaching theself-protection class 0 of 7, any rule reaching the exfiltration-shape class 0.
Why it matters
This is fail-wrong rather than fail-silent, which the module's own docstring
names as the worse of the two. The guidance exists because an agent that is
refused and told nothing retries the same shape under a different reader and then
reports the capability missing; guidance that instead invites it to rerun the
refused command spends a turn and teaches it that the advice is untrustworthy.
The affected class is outbound credential transfer, where "try it again" is the
last thing the refusal should say.
Separately, adding a rule to the catalog was a fully green change that shipped
with no remediation and nothing turning red -- and that is the normal way this
catalog grows.
What changed (motivation -> approach -> change)
Symptom: an outbound-transfer refusal receives credential-read prose. Root
cause: the class is inferred from refusal TEXT for every tier, including the one
tier that already knows which rule fired. Fix: key that tier by rule identity.
The refusal's first line is
DENY_REASON_PREFIXplus the pattern of the rulethat fired, so
classify_denyreads that line, resolves the rule, and answersfrom two tables:
_CATEGORY_CLASSES-- the three categories that have a sanctioned path at all(
sensitive-file-read,credential-exfil,self-protection). Mappingcredential-exfilto the outbound-transfer answer is what fixes the tenheadline rules; no per-rule row is needed for them and none is shipped, since a
row repeating its category's answer would have no effect on the lookup (a
census now fails on one).
_RULE_CLASSES-- the 20 rules whose answer is NOT their category's. The threeinstance-metadata rules become credential-read, which is what they are (they
ACQUIRE a credential, and the sensitive-path floor already answers that way for
the same address, so the two enforcement routes agree). The two product-token
rules become self-protection, matching the argv-structural floor. Two legacy
secret-READ rules filed under the exfiltration category become the
credential-material answer. The nine AWS-profile rules in
sensitive-file-readare named explicitly rather than left to the
.awsanchor in their pattern.A refusal that names a rule is answered by that rule and nothing else, and the
anchor scan runs only for producers that name none -- the fnmatch overlay, the
sensitive-path floor, the exfiltration audit -- which is where a classifier is the
right tool. Two consequences, each pinned by a test:
answer; letting it fall through meant
aws ec2 terminate-instances --instance-ids ... --profile ssowas answered with enterprise-SSO login prose,matched from a word in its own arguments. (Reachable before this change and on
the previous head of this PR; found by GPT 5.6 review.)
credential necessarily contains that credential's name, so weighing the subject
against a rule's own verdict re-created the defect by another route:
aws s3 cp ./report.aws s3://bucketdrew credential-read prose. (Also found byGPT 5.6 review, on the first head.)
Measured after: all 61 rules in the three remediation categories resolve, zero
non-metadata
credential-exfilrule reaches the credential-read class, and the87 rules in the other seven categories answer silence authoritatively (a destructive
rmexplains itself).No producer in
security.pychanged, so the wire reason is byte-identical.The
securityimport is at module scope, per the repo'stop-level-importsruleand because there is no cycle to avoid:
securitydoes not import this module,and
platform/defaults.py:29-- which this module already imports at module scope-- imports
securityitself, so that edge existed before this change and nothingnow loads eagerly that did not before. Verified by importing
deny_guidancefirst in a fresh interpreter and via
security,cli_doctor,dashboard.state,hooksandplatform.defaults; all six orders are clean.What is deferred is the index BUILD, and only for one reason: an edition's rules
arrive through the
DeniedRuleProviderseam and are not knowable until it iscomposed, so an index built at import time would omit them permanently. A
successful lookup is cached -- including the empty list an ungoverned host returns
-- while a raise is not, so one transient composition fault at a process's first
refusal cannot pin a built-ins-only index and put exactly the enterprise-added
rules back on the anchor scan.
Two deliberate deviations from the issue's "Agreed shape", both called out for
the reviewer:
alternativefield onDeniedCommandRuledelivered throughis_denied(reason_notes=...), plusDeniedRuleProvider.denied_rules()returning full rule objects so a composed edition can carry its own per-rule
remediation. This PR does neither. Routing to the existing class prose meets
all five acceptance criteria with one table instead of ~61 copies of that
prose on rule rows, and without changing the wire reason for 61 rules or the
provider interface. The edition-extensibility seam is a real want but it is a
separate concern from the ten wrong pairings and is left open.
credential-exfilrule resolves to theAWS-credential class. Three of the 27 still do, on purpose: the
metadata-endpoint rules refuse a credential ACQUISITION, not an outbound
transfer, so the read answer is the actionable one there and diverging from
the floor would make the two routes contradict each other. The criterion's
operative clause -- an outbound-transfer refusal never receives text stating
that AWS CLI calls are not blocked -- is asserted over the whole category with
those three named as the exception.
Tests
All in
test/test_deny_guidance.py. 32 of them fail on this base without thesource change; 116 pass with it.
TestRuleIdentityRoutesTheRegexTier::test_each_pairing-- one case percorrected pairing (the ten defects, the three metadata, the two token, the two
legacy, plus category-default and anchors-still-win cases).
test_an_outbound_transfer_never_hears_that_aws_calls_are_allowed-- thedecisive case end to end through the real producer chain, first asserting that
neither always-on floor answers it, then asserting the OUTPUT prose lacks the
credential-read sentence.
test_the_command_title_cannot_pull_a_rule_off_its_own_class-- the fix wouldhold only for a reason inspected without its command if the subject were read
with equal authority.
test_a_rule_tier_self_protection_refusal_matches_what_the_floor_says-- thetwo enforcement routes to the same rule must say the same thing.
test_an_operator_note_is_not_read_as_a_rule_identity,test_the_index_survives_a_reset.TestCatalogCensus-- every rule in the catalog resolves from its identity rather than the anchor scan; every rule in a remediation category resolves to prose; no two rules share a pattern with different classes; no_RULE_CLASSESrow merely repeats its category; nooutbound-transfer rule gets the read answer; the other seven categories still
get nothing (so a future table entry cannot spray prose over destructive
rules); every table key names a rule id / category that exists (a rename must
fail here rather than silently lose its correction); every routed class has
prose.
TestWireReasonFirstLineIsUnchanged-- the first line of every one of the 148rules' refusals is exactly the prefix plus the pattern, and an operator note
never reaches it. This is the contract
RecoveryCard.tsxparses and the onethe identity lookup now also depends on.
Run:
pytest test/test_deny_guidance.py -n 2-- 116 passed.black --check,flake8,isort --check-onlyandmypyclean on the touched files.Manual verification
N/A -- unit coverage is sufficient: the assertions drive the real producers in
security.pyand the real classifier output, which is the whole surface thischange touches. The before/after census quoted above was produced by the issue's
own repro against this branch.
Related Issues
Closes #7890
Pattern harvest
Rule candidate: review-prompt
Pattern: a value recovered by re-parsing text that a caller already held
structurally -- here the deny class inferred from a rule's regex source when the
rule's own id and category were in hand, so the classification tracked the
author's incidental word choice instead of the rule's purpose.