fix(interceptor): GENERAL/DATA_QUERY passthrough returns UNVERIFIABLE, not FORWARDED (closes #6)#25
Conversation
closes #6 Root cause: _route_to_engine() returned verified=True for GENERAL and DATA_QUERY payloads with no verification engine. _build_verdict() then issued a signed JWT attestation for content that was never checked. Changes: - schema.py: add VerdictStatus.UNVERIFIABLE - interceptor.py: passthrough returns verified=False + status=unverifiable - interceptor.py: intercept() dispatches UNVERIFIABLE before FORWARDED check - interceptor.py: _build_verdict() skips JWT signing when UNVERIFIABLE (issuing a JWT for unverified content is a false cryptographic claim) Tests: - test_unverifiable_passthrough.py: 7 regression tests covering GENERAL and DATA_QUERY passthrough behavior, no-JWT guarantee, engine name, reason presence, and sanity check on verified path - test_interceptor.py: updated 5 existing tests to reflect new semantics (signing failure tests moved to FINANCIAL — GENERAL now skips signing)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new VerdictStatus.UNVERIFIABLE; interceptor routes passthrough/no-engine payloads (GENERAL, DATA_QUERY) as unverifiable, skips JWT attestation for those verdicts, increments a new telemetry counter, and updates/extends tests to assert UNVERIFIABLE behavior and attestations are omitted. ChangesUNVERIFIABLE Verdict Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Interceptor as A2AVerificationInterceptor.intercept
participant Router as _route_to_engine
participant Builder as _build_verdict
participant Crypto as CryptoService
Client->>Interceptor: send AgentMessage (e.g., GENERAL/DATA_QUERY)
Interceptor->>Router: determine engine for payload
Router-->>Interceptor: {engine:"passthrough", verified:false, status:"unverifiable", reason:"no engine"}
Interceptor->>Builder: build UNVERIFIABLE verdict with details
Builder->>Crypto: (skips) sign_verdict --x
Builder-->>Client: VerificationVerdict(status=UNVERIFIABLE, attestation_jwt=None)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 298d3f8f84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| if engine_result.get("status") == "unverifiable": | ||
| verdict = self._build_verdict( | ||
| trace_id=trace_id, | ||
| status=VerdictStatus.UNVERIFIABLE, |
There was a problem hiding this comment.
Emitting VerdictStatus.UNVERIFIABLE here introduces a new steady-state outcome, but record_intercept only counts forwarded and blocked explicitly and sends every other status to total_errors; in practice, normal GENERAL/DATA_QUERY traffic will now be misclassified as errors, skewing dashboards and any alerting keyed off error rate. Please add explicit handling for unverifiable (ideally a separate counter) before routing production traffic through this branch.
Useful? React with 👍 / 👎.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_unverifiable_passthrough.py (1)
63-73: ⚡ Quick winAssert DATA_QUERY reason presence to match passthrough contract.
You already enforce a non-empty
reasonfor GENERAL; mirroring that for DATA_QUERY will keep regression coverage symmetric and catch contract drift.✅ Suggested test addition
async def test_data_query_returns_unverifiable_not_forwarded( self, open_interceptor, data_query_message ): """DATA_QUERY payload must produce UNVERIFIABLE verdict, not FORWARDED.""" verdict = await open_interceptor.intercept( data_query_message, trace_id="t_data_query_unverifiable" ) assert verdict.status == VerdictStatus.UNVERIFIABLE, ( f"Expected UNVERIFIABLE, got {verdict.status}. " "DATA_QUERY payloads must not be falsely endorsed as verified." ) + assert verdict.reason is not None + assert len(verdict.reason) > 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_unverifiable_passthrough.py` around lines 63 - 73, The DATA_QUERY passthrough test currently only checks the UNVERIFIABLE status, so extend the existing test in test_data_query_returns_unverifiable_not_forwarded to also assert that verdict.reason is present and non-empty, matching the GENERAL passthrough contract. Use the same open_interceptor.intercept flow and keep the assertion alongside the existing VerdictStatus.UNVERIFIABLE check so regressions in reason propagation are caught for DATA_QUERY as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_unverifiable_passthrough.py`:
- Around line 63-73: The DATA_QUERY passthrough test currently only checks the
UNVERIFIABLE status, so extend the existing test in
test_data_query_returns_unverifiable_not_forwarded to also assert that
verdict.reason is present and non-empty, matching the GENERAL passthrough
contract. Use the same open_interceptor.intercept flow and keep the assertion
alongside the existing VerdictStatus.UNVERIFIABLE check so regressions in reason
propagation are caught for DATA_QUERY as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f18337b2-3ca8-4e44-a375-5c288d714728
📒 Files selected for processing (7)
src/qwed_a2a/interceptor.pysrc/qwed_a2a/protocol/schema.pytests/conftest.pytests/test_crypto_signing.pytests/test_endpoints.pytests/test_interceptor.pytests/test_unverifiable_passthrough.py
💤 Files with no reviewable changes (1)
- tests/test_crypto_signing.py
…A_QUERY Codex P2: record_intercept() was routing UNVERIFIABLE status to total_errors, misclassifying normal GENERAL/DATA_QUERY traffic as errors and skewing dashboards. Added explicit total_unverifiable counter with its own branch in record_intercept() and to_dict(). CodeRabbit: Extended test_data_query_returns_unverifiable_not_forwarded to assert verdict.reason is present and non-empty, matching the GENERAL passthrough contract for symmetric regression coverage.
|
The Sentry finding is not a valid bug in this context. The scenario described FINANCIAL_TRANSACTION with If a downstream consumer needs to distinguish "engine disabled by config" from Sentry suggestion rejected. |
1 participant
Problem
_route_to_engine()returnedverified: TrueforGENERALandDATA_QUERYpayloads — the two types with no verification engine behind them. This result
fed directly into
_build_verdict(), which then issued a signed JWT attestationwith
verdict: forwardedfor content that was never actually checked.An attacker only needed to label a payload
payload_type: generalto receivecryptographic endorsement from QWED-A2A.
Changes
schema.pyVerdictStatus.UNVERIFIABLE— a distinct status for payloads with noapplicable verification engine
interceptor.py(3 changes)_route_to_engine(): passthrough now returnsverified: False,status: unverifiableintercept(): new dispatch branch handlesUNVERIFIABLEbeforeFORWARDED_build_verdict(): skips JWT signing entirely when status isUNVERIFIABLE— issuing a signed JWT for unverified content is a false cryptographic claim
Tests
test_unverifiable_passthrough.py— 7 new regression tests:GENERAL→UNVERIFIABLE, notFORWARDEDDATA_QUERY→UNVERIFIABLE, notFORWARDEDattestation_jwton eitherengine_used == "passthrough"reasonis present and non-emptyFINANCIALpayloads still receive JWT attestationtest_interceptor.py— 5 existing tests updated to reflect new semanticsAcceptance criteria
GENERAL→VerdictStatus.UNVERIFIABLEDATA_QUERY→VerdictStatus.UNVERIFIABLEattestation_jwtisNonefor bothverified: Truenever returned from passthrough pathSummary by CodeRabbit
New Features
Behavior Changes
Tests
Telemetry