Repository navigation
fix(agt): validate Guardian decision responses - #249
mjatin2007 wants to merge 2 commits into
Conversation
|
Hi @shivang-sec and @President, may I check how you’d prefer to sequence this? Section 6.4 on |
|
Thanks for checking, @mjatin2007 . I’d lean toward opening a Discussion for the Section 6.4 clarification first, while keeping this draft open for implementation review in parallel. That should let us resolve the intended behavior for parseable but schema-invalid Guardian responses before merging a change that deliberately differs from the current wording. I agree with keeping protocol violations distinct from technical failures, with the exact fail-closed behavior and its relationship to I’ll defer to @President on the preferred sequencing before we treat the behavior as settled. |
|
@mjatin2007 @shivang-sec - I think no new Discussion needed: same question is already open in #32 and in Rock's #308. In #308 I proposed that a reply that breaks the spec, e.g. a missing required field, is DENY, not a decision failure. That is what this draft already does. Until the spec's §6.4 text changes, I'd note in the host adapter's README that this is a stricter reading of §6.4. #182, the issue this draft fixes, is already accepted. Once #229 adds response signatures, I'd check the signature first (a bad one is DENY), then the schema. |
|
@President @shivang-sec I added the requested README clarification that the adapter intentionally takes a stricter reading of the current §6.4. I left signature-before-schema validation for #229. The update is in 3f4debc. Thanks! |
|
Hi Jatin, OWASP requires a sign-off on every commit, and 3f4debc on this PR doesn't have one. The Please run these commands: They add a |
Signed-off-by: Jatin Mannepalli <mjatin2007@users.noreply.github.com>
Signed-off-by: Jatin Mannepalli <mjatin2007@users.noreply.github.com>
3f4debc to
625efb4
Compare
|
Thanks, Rock. I rebased the branch onto the current |
|
@mjatin2007 thanks, the README note is exactly what I meant. Looks good to me. |
drk-ram
left a comment
There was a problem hiding this comment.
Security peer review of commit 625efb48f91276239a7961e30a6cfb96b6be9df0.
No exploitable security vulnerability found in the changed code. Decision results are checked against both the canonical response envelope and the method-specific result contract. Schema-invalid replies and recognized refusals take the deny path regardless of the negotiated delivery-failure posture; local validator unavailability remains a separately audited host fault.
Validation at that exact commit:
- Directly changed decision-client and failure-posture suites: 91 passed, 0 failed.
- Broader host-adapter and affected host suites: 393 passed, 53 failed across 446 tests. The 91 focused tests are included in this broader count, not additional tests.
- Comparison against the exact pre-change base
bac52e1c63ba479e7193b9a33d6b40ca5c4e046a: 382 passed, 53 failed across 435 tests on the same Windows host. The broader run therefore is not a clean pass, and the same failure count is present without this change.
I am leaving a non-blocking comment rather than an approval because the broader affected suite is not green in this environment. I found no security issue in this change that warrants a request for changes.
What changed
Validates parsed, correlated Guardian decision responses against the canonical
response-envelope.jsonschema and its method-specificAcsResultbranch before the host adapter accepts them. A canonical-schema or method-result violation is audited and denied even underon_decision_failure: proceed; inability to run the local validator remains a host-side failure and follows the negotiated posture.This follows the accepted issue's fail-closed direction, but under
proceedit deliberately differs from the literal current §6.4 treatment of a malformed response. Validation covers the complete canonical response envelope, not onlydeny.reasoning. This draft should not be treated as conformant until §6.4 is clarified, and it should not merge ahead of that clarification unless maintainers direct otherwise. Response-signature verification is separate and remains pending in #229.Which issue does this implement
Part of #182
meaning, so it needs no issue
Base branch
integration, because this touches the specification, schemas, a referenceimplementation, an adapter, tests, or CI
main, because every changed path is on the documentation lane allowlist inCONTRIBUTING.md
Type of change
I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineThe AGT validation also passes on the repository's pinned Bun 1.3.11: TypeScript succeeds, and the complete Bun suite reports 1,123 passed, 1 skipped, 0 failed across 68 files.
Checklist
git commit -s(required by the DCO)Security
Security impact: schema-invalid or method-invalid Guardian responses are blocked and audited. Local validator unavailability remains posture-governed and is attributed to the host rather than the Guardian.