Fix: Bound POST /verify (JSON) body size and field cardinality - #407
Fix: Bound POST /verify (JSON) body size and field cardinality#407rajnisht7 wants to merge 1 commit into
Conversation
|
The failing test (
Opened a PR for the same #408 |
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
f360ff4 to
6e2e6d2
Compare
imran-siddique
left a comment
There was a problem hiding this comment.
Approving. Three of the things in this PR were not asked for and each of them is the reason to take it rather than a bonus on top.
The seventh field. The report named six. verified_attestation_manifest_hashes has the identical unbounded set[str] shape, sits directly below two fields that were named, and reaches VerificationContext by the same path. Capping six and leaving the seventh open would have moved the attack rather than closed it, and your comment says exactly that in the code where the next reader needs it. Fixing the instances the reporter missed is the right instinct on a bounds defect, because the bound is only worth what its weakest field is.
The regression you caught in your own change. Swapping a VerifyRequest parameter for a raw Request removes FastAPI's automatic Content-Type gate, so the route would have parsed a JSON body submitted as text/plain, as a bogus subtype, or with no header at all, contradicting the application/json contract its own OpenAPI schema advertises. That is invisible in a diff review: the removed behaviour has no line in the patch. Restoring it explicitly with FastAPI's own rule (application/json, a +json vendor subtype, or absent) rather than a stricter homemade one is the right call, because a stricter gate would have been a silent behaviour change for existing callers.
The pre-existing bug you found looking for the first one. Both routes checked the accumulated buffer against the cap after appending each chunk, so the buffer could be grown a full chunk past the limit before rejection. Checking before appending is correct and it fixes verify_cose, which was not in scope. A size cap enforced after the allocation is not a size cap.
The 1 MiB byte cap and the 10,000-entry collection cap answer different attacks and both are needed: the byte cap alone misses a small body carrying a million short keys, and the entry cap alone misses one enormous value. Renaming MAX_COSE_ENVELOPE_BYTES to the endpoint-neutral MAX_VERIFY_BODY_BYTES and sharing it across both routes keeps them from drifting apart again, which is how this gap opened in the first place.
On CI: every one of the ten failing lines across the five platform jobs is test_docs_first_manifest.py::test_followup_tutorial[operations/monitoring.md] failing on ModuleNotFoundError: No module named 'prometheus_client'. That is #408, your other PR, now merged, and it was failing on main and on every open PR in this repository. Nothing in your diff caused a failure. I am closing and reopening this so CI recomputes against a main that has the lockfile fix, since a re-run would replay the original merge commit and keep the stale base.
What
Adds the same body-size and collection-cardinality limits to
POST /verify(JSON) thatPOST /verify/cosealready has.Why
POST /verifytook aVerifyRequestPydantic model directly, so FastAPI would fully buffer and validate an arbitrarily large JSON body e.g. atrusted_keysdict with millions of entries before any verification logic ran.POST /verify/cosealready guards against this; this endpoint didn't.Closes #383.
Spec impact
None
Test plan
pytest -vpassesmypy src/agent_manifestpassesruff check src/ tests/passesCHANGELOG.mdupdatedDCO
All commits in this PR are signed off (
git commit -s). By submitting this PR I certify the Developer Certificate of Origin.