Repository navigation
fix(runtime): enforce canonical base64url in agent_manifest and reject duplicate JSON keys across server, catalog, and inspection (#798) - #800
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
dinakarjs
left a comment
There was a problem hiding this comment.
Bounded review at 043be57. I fetched the source and the five named test modules at this SHA and independently ran them on Linux/Python 3.12: 213 passed (one Starlette deprecation warning), not the 300 stated in the description. The documented Ruff selection passes. Please reconcile the count or identify the additional executable selection and revision behind 300.
The inspected paths reject duplicate members recursively, retain JSON/COSE loader distinction, reject noncanonical key/signature encodings, and retain sensitivity scanning when schema parsing denies a response. The new proxy numeric checks and gated refusal tests pass in this selection. No additional blocker found in this bounded source/test review.
This run used the pinned repository source with agent-manifest 0.15.0 and published dependencies; it was not a hash-locked full-suite run. Current-head checks exposed here contain contributor/approval gates but do not establish the full runtime CI matrix. Full pytest, mypy, Bandit and required independent maintainer approvals remain necessary before merge. No hardware or production validation claimed. Reviewed with OpenAI Codex.
dinakarjs
left a comment
There was a problem hiding this comment.
Expanded validation at 043be57: all 722 tracked source blobs matched the pinned GitHub tree. In a hash-locked development environment with the package installed editable, tests/unit completed with 2,586 passed and 6 skipped. Ruff (src/tests), mypy (76 source files), and Bandit passed locally. This supplements my earlier 213-test focused run; it does not establish the PR body's separate 300-test command/count. Hosted CI and independent maintainer approval remain separate gates.
|
Thank you @dinakarjs! Updated the PR description to state 213 passed for the 5 focused test modules (2,586 passed, 6 skipped across ests/unit). |
carloshvp
left a comment
There was a problem hiding this comment.
Reviewed exact head 043be57a3eee9b0bd9f5d755c5bd0291031eeab1 in an isolated checkout.
The changed runtime boundaries reject ambiguous JSON, non-canonical Agent Manifest encodings and private public-key material, while retaining PII classification on denied responses and controlled TRACE finalization for invalid numbers. I found no blocking defect in the changed paths. The two duplicate-key/schema/PII regressions pass here and both fail with the actual merge-base pipeline restored in process.
In a fresh hash-locked Python 3.12.10 environment with the local package installed, the full unit suite passed: 2,572 passed, 20 skipped. pip check, Ruff, mypy and configured Bandit passed. The commit sign-offs are present. git diff --check reports only new blank lines at test-file EOFs; please trim those as cleanup. This is a source-review approval; the hosted approval/policy checks and applicable CI still need to complete.
Reviewed with OpenAI Codex.
|
Hosted CI follow-up at head The older approval gate is also inconsistent with the current review API: rerun job 120848778091 reports zero independent approvals, while REST review 5637901229 is APPROVED by carloshvp at this exact head. I re-evaluated the checked-in filter using the current base-policy file and live reviews and obtained one qualifying approval. I have not established why that job sees zero, and repeated reruns are not evidence of a source defect. The local source approval stands; hosted audit and gate completion remain separate. |
|
Maintainer-policy readback for current head GitHub's raw review API currently returns my approval 5637901229 as APPROVED, type User, on that exact commit. Both the PR-base and head maintainer rosters include carloshvp. The ordinary eligibility calculation therefore finds one approval, while the executed gate finds none. This is an unresolved policy/check-state discrepancy. The required gate should remain blocking until a legitimate current policy run succeeds; no code change or approval bypass was made for this retry. The existing technical approval remains attached to its reviewed commit. Prepared with OpenAI Codex. |
…t duplicate JSON keys across server, catalog, and inspection - Reject len % 4 == 1 and non-canonical trailing bits in cmcp_runtime.agent_manifest._b64url_decode and verify v0.1 signature_value canonical encoding - Reject private JWK key parameters in Agent Manifest trust anchors and catalog publisher_jwk entries - Reject duplicate JSON object member names and non-finite constants across mcp/server.py, catalog/loader.py, agent_manifest.py, and inspection/pipeline.py - Reject non-finite floats and integers outside the RFC 8785 safe-integer range in mcp/proxy.py (_cedar_safe, trace_action, _trace_begin, and _call_tool_impl) Closes agentrust-io#798 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com> Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
|
@carloshvp @dinakarjs Rebased onto |
043be57 to
31fdb56
Compare
dinakarjs
left a comment
There was a problem hiding this comment.
Follow-up at exact head 31fdb56, isolated checkout on Linux/Python 3.12.14.
Verified the rebased head incorporates the merged dependency-audit baseline and preserves the JSON/canonical-encoding and schema/PII follow-up paths inspected here. Full unit selection passes 2,595 tests with 6 skips after removing this environment's inherited SOCKS proxy variables; the first run's nine failures were missing SOCKS transport support in the environment. Ruff, mypy over 63 source files, configured Bandit and installed dependency consistency pass. This closes my requested rebase/test follow-up as a technical COMMENT; it does not resolve or bypass the previously reported maintainer-gate discrepancy, grant approval for an unreviewed commit, or establish hardware validation. Independent current-head maintainer review and a legitimate successful policy run remain required.
Reviewed with OpenAI Codex.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Summary
src/cmcp_runtime/agent_manifest.py: Enforce canonical unpadded base64url (len(value) % 4 != 1and exact round-trip encoding), reject RFC 7517/7518/8037 private or symmetric JWK parameters (d,p,q,dp,dq,qi,oth,k), and reject duplicate JSON object keys andNaN/Infinityconstants when loading Agent Manifest documents, trust policies, and revocation lists.src/cmcp_runtime/catalog/loader.py: Reject duplicate JSON object keys,NaN/Infinityconstants, and private JWK parameters inpublisher_jwkwhen loading catalogs and entry schemas.src/cmcp_runtime/inspection/pipeline.py&src/cmcp_runtime/mcp/server.py: Reject duplicate JSON object keys in MCP JSON-RPC envelopes, admin endpoints, pattern files, Stage 2 schema validation, and sensitivity classification.src/cmcp_runtime/mcp/proxy.py: Enforce IEEE-754 / RFC 8785 safe-integer bounds (abs(v) <= 2**53 - 1) and reject non-finite floats in_cedar_safeandtrace_action, failing closed asPolicyDeny/trace:refused.Closes #798
Validation
python -m pytest tests/unit/test_agent_manifest.py tests/unit/test_catalog.py tests/unit/test_mcp_proxy.py tests/unit/test_stage2_schema.py tests/unit/test_trace_gate_hook.py -q(213 passedin the 5 focused modules;2,586 passed, 6 skippedacrosstests/unit)python -m ruff check src/cmcp_runtime/agent_manifest.py src/cmcp_runtime/catalog/loader.py src/cmcp_runtime/inspection/pipeline.py src/cmcp_runtime/mcp/proxy.py src/cmcp_runtime/mcp/server.py(All checks passed!)