fix(provenance): verify_record fails closed on malformed identity/tool_catalog

[rajnisht7]

What this changes

Two functions in provenance.py verify_record() and check_tool_catalog() they are both documented to raise ProvenanceError (or its subclass ToolCatalogMismatch) when a record is bad. That's the whole point of these functions: they're the gate that untrusted, possibly malicious records have to pass through, and callers are told it's safe to just catch ProvenanceError.

The bug: in four places, the code read a nested field like this:

identity = record.get("identity") or {}

This only handles the case where the field is missing. If someone sends a record where identity is something other than a dict a plain string, a number, a list, True that value is still "truthy," so or {} never kicks in. The very next line then calls .get() on it, and Python throws a plain AttributeError instead of the ProvenanceError the function promises. A caller doing exactly what the docs tell them to do (except ProvenanceError) would not catch this and their program would crash.

Type of change

• Editorial (typo, link fix, clarification — no normative effect)
• Non-breaking spec change (new optional field, new platform profile, informative addition)
• Breaking spec change (requires 14-day comment period and Project Lead sign-off)
• Schema change
• Example addition

Spec section

None

Checklist

• DCO sign-off on all commits (git commit -s)
• CHANGELOG.md updated (for any normative change)
• Breaking changes marked with <!-- CHANGED: #NNN — description --> in spec text
• Backward compatibility statement included (for breaking changes)

[lywinged]

Approving. Two things in here are better than what usually shows up in a hardening PR, and they are worth naming.

The first is the pair of control tests — test_null_identity_and_tool_catalog_still_behave_like_absent and its check_tool_catalog twin. Pinning that None still produces "neither an artifact nor an endpoint" and "not a sha256", rather than a type error, is what stops a fix like this from quietly over-tightening into rejecting records that were always legitimate. Most hardening patches ship the positive cases and stop.

The second is that you found the second site yourself. check_tool_catalog() is callable on its own, without verify_record() having seen the record first, so it needed the same contract independently — that is a question about what the function is for, not about what the traceback said, and it is the question that finds the rest of a defect class rather than the one instance reported.

Verified on your branch: 812 pass, ruff and mypy clean. I fuzzed every field of a valid provenance record against six wrong types (12 × 6 = 72 probes); nothing escapes as AttributeError any more. The four guards plus check_tool_catalog cover every instance of the or {} pattern that was reachable.

The match= on each pytest.raises is doing real work too. Building probe records for this review, three of mine failed an earlier check and reported a false pass before I caught it — a test asserting only the exception type would have done the same. Yours can't.

Why this class of bug is here and not in sign.verify_record: the same fuzz against Trust Records (156 probes) comes back clean, because sign.verify_record runs validate_json against the JSON Schema before touching anything. Provenance records have no schema, so verify_record does structural validation by hand, and every hand-written check is a place to miss one. _as_object is the right tactical fix; the durable one is probably a schema for agentrust-io/mcp-server-provenance/1.

Two things left over, neither a blocker, both one-liners in the function you are already in.

1. cnf is the fifth instance of the same pattern — provenance.py:349, forty lines below your last edit:

embedded = (record.get("cnf") or {}).get("jwk")

An AST sweep of the file says it is the only one left: 27 .get() calls, one still on an unproven receiver. On your branch:

cnf="not-a-dict"  → AttributeError @ provenance.py:349
cnf=None          → ProvenanceError ✓

2. attestation has the same defect in a different shape, and this one fails open. _check_structure declares attestation: dict[str, Any] | None and then only tests it for truthiness:

kind="tee-attested", attestation="not-a-dict"  → verifies clean
kind="tee-attested", attestation=42            → verifies clean
kind="tee-attested", attestation=True          → verifies clean

Pre-existing — main behaves the same, so this is not something your PR introduced — but tee-attested is the strongest claim the format makes, and a record carrying attestation: "yes" currently passes verification rather than crashing on it. mypy misses it because record.get("attestation") is Any, so the declared parameter type is never enforced at the call boundary. The same isinstance you added for artifact and endpoint closes it.

One process note: DCO is signed on 2 of the 5 commits (19939aa, 5d61c1b); 7c1e9f1 and 69d9d00 will still fail the check. git rebase --signoff over the branch rather than amending the tip.

---

Generated by Claude Code

[imran-siddique]

Correct, and the reasoning in the description is the part that makes it easy to review.

record.get(field) or {} reads like it defaults a missing block, and it does, but only that. A present non-dict value is truthy, survives the or, and the next .get() raises AttributeError. A caller doing exactly what the docstring says, wrapping in except ProvenanceError, does not catch it. In a function whose entire job is to reject adversarial records, crashing the caller instead of rejecting the input is the wrong failure.

Checked:

• All four sites covered, including tool_catalog read independently in check_tool_catalog(). That one is the easy miss, since it is callable without verify_record() having seen the record.
• _check_structure guards artifact and endpoint before touching them rather than relying on the caller.
• _as_object keeps absent as {}, so no existing accept path changes. Only crashes become refusals.
• Tests cover each shape and assert the message, not just the type.
• CI green; the earlier gate failure was the approval check, which this satisfies.

Docstring updated to state the contract check_tool_catalog now actually keeps. Thanks.