fix(intent-bridge): validate signed decision enum - #322
Conversation
|
🔴 Contributor Check: HIGH
Automated check by AgenTrust Contributor Check. |
lywinged
left a comment
There was a problem hiding this comment.
head daeec5c: Approve
Verified on daeec5c. Each of the five values from #319, true, 1, null, the empty string and "reject", re-signed with sign_bridge and verified at a fixed time inside the window, now raises IntentBridgeError with the message naming the field, and none of them is an AuthorizationDenied. The literal "deny" still is, "allow" still verifies, and a capitalised Allow, which I added, is refused as malformed rather than read as a denial.
The check sits with the module's other typed field checks, before the signature is verified, which is where #319 asked for it. So a malformed decision under a bad signature reports the decision, and a signed "deny" under a bad signature reports the signature. That is the order the other fields already had. The three things that went with the change are all here: the AuthorizationDenied docstring names the literal deny, the regression tests re-sign each value so the enum check and not the signature is what fails, and the CHANGELOG.md entry is under Unreleased, Fixed.
The four CI steps with the hash-pinned install, on 3.11 and on 3.12: green, 1407 passed, 1 skipped. With src/ reverted to a247244 and the new tests kept, exactly the five parametrized cases fail and nothing else, so the tests hold the change and no more.
For sequencing: the intent_bridge.py hunk here is not the one #325 touches, and CHANGELOG.md is the only overlap between the two.
|
Conflicted by my own merge, and it is mechanical. Apologies for the round trip. I merged #315 a few minutes ago, which added a Rebase onto I trial-merged each of the three against current main before writing this. In every case the single conflicted file is On #322 itself: approved by @lywinged, all five checks green, and I was merging it when the conflict appeared. It goes in as soon as it applies. The changelog collision is our problem rather than yours. Every open PR in this repo appends to one block, so each merge conflicts the rest. I am looking at whether a fragment directory fixes it properly. |
Rebased onto upstream 760cc3f after the CHANGELOG-only conflict identified by maintainers. Signed-off-by: Altru.dev <altrudevelop@gmail.com>
daeec5c to
202b7b7
Compare
imran-siddique
left a comment
There was a problem hiding this comment.
Approving and merging. Thank you for the fast rebase.
On the review state: @lywinged approved this at daeec5c and that approval was dismissed automatically when you rebased, not withdrawn. The rebase existed because I merged #315 into the same CHANGELOG.md block, which was my sequencing error rather than anything about this PR.
Verified the rebase preserved what that approval covered rather than assuming it. At 202b7b71: _decision still refuses anything that is not the literal "allow" or "deny", AuthorizationDenied still carries the "literal valid deny" wording, and the parametrised test still covers True, 1, None, "" and "reject". Same +33/-1 across the same three files.
The property this protects is worth restating since it is the reason it merges rather than the diff size: a verifier that reports a malformed value as AuthorizationDenied is telling a relying party that the producer said no, when what actually happened is that the producer said something unreadable. Those are different facts and only one of them is a policy decision.
Closes #319.
What
verify_bridge()now validatesauthorization.decisionagainst the bridge's closedallow|denycontract before applying authorization semantics.Previously the runtime used
decision != "allow"as the denial branch, so a correctly re-signed bridge carrying malformed values such astrue,1,null,"", or"reject"was reported asAuthorizationDenied. That conflated malformed producer output with a legitimate signed denial.The fix adds an explicit decision validator alongside the module's existing typed field checks. Only the literal valid
"deny"reachesAuthorizationDenied; malformed values raiseIntentBridgeError. TheAuthorizationDenieddocstring now names that literal-deny meaning.Regression coverage
The tests re-sign each malformed authorization so the enum check, rather than signature failure, is load-bearing. They cover
true,1,null,"", and"reject", and assert that the resulting exception isIntentBridgeErrorbut notAuthorizationDenied. The existing valid-denytest remains unchanged and continues to hold the policy-denial path.Scope
No wire-format, signature, trust-key, scope, digest, transcript, or authorization-policy change. This aligns the runtime verifier with the published bridge schema and documented error boundary.
CHANGELOG.mdis updated under Unreleased / Fixed.Branch base: upstream
a247244fb0bb97aea4e60d655f36d445cca18cd5.DCO sign-off is present. AI-assistance disclosure: ChatGPT assisted with source review, duplicate search, implementation drafting, and regression construction.
altrudevreviewed the bounded change and remains responsible for the contribution.