fix(cose): bound payload nesting explicitly, not via RecursionError - #290
Merged
Conversation
main has been red since #274 merged. Two deeply-nested-payload tests fail on ubuntu 3.12 and 3.13 while passing on 3.11 and on Windows, and the cause is not a flaky test: the DOS-006 guard did not exist on Linux. _parse_payload relied on json.loads raising RecursionError to refuse a deeply nested payload. CPython on Linux parses thousands of levels without raising, so the 5000-level payload simply parsed, _check_version then raised CoseVersionError("unsupported manifest version None"), and the test expecting CoseStructureError failed. Windows tripped its own recursion limit and therefore looked protected. A control whose behaviour depends on which platform it runs on is not a control. Replaced with an explicit bound: _MAX_PAYLOAD_NESTING = 64, checked by a string-aware scan of the decoded text before json.loads rather than after. Before, because refusing the work after paying for it defeats the purpose; string-aware, because a brace inside a member value must not inflate the count and make a legitimate manifest look like an attack. The RecursionError catch stays as a second line. The new test proves the bound rather than the accident: it refuses a 65-level payload, which is far below any interpreter's recursion limit and therefore cannot be failing for the old reason. 64 is generous - the deepest path this spec defines is about six levels. Found while cutting python-v0.11.0: the release PR's CI failed, and the failures predated it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
main has been red since #274 merged. Two deeply-nested-payload tests fail on ubuntu 3.12 and 3.13 while passing on 3.11 and on Windows.
It is not a flaky test. The DOS-006 guard did not exist on Linux.
What was happening
_parse_payloadrelied onjson.loadsraisingRecursionError:CPython on Linux parses thousands of levels without raising. So the 5000-level payload simply parsed,
_check_versionthen raised on the resulting{"a": {"a": ...}}:Windows tripped its own recursion limit and therefore looked protected. A control whose behaviour depends on which platform it runs on is not a control — on Linux, arbitrarily deep untrusted input was accepted.
The fix
An explicit bound,
_MAX_PAYLOAD_NESTING = 64, checked by a string-aware scan of the decoded text beforejson.loads:"\\"does not swallow the closing quote.RecursionErrorcatch stays as a second line.64 is generous: the deepest path this spec defines is roughly
artifacts.tool_manifest.tools[].approved_scope, about six levels.The test proves the bound, not the accident
The new test refuses a 65-level payload. That is far below any interpreter's recursion limit, so it cannot be passing for the old reason — if the bound were removed,
json.loadswould parse it happily on every platform and the test would fail. Plus a string-awareness test and an at-the-limit case that must still parse.How it was found
Cutting
python-v0.11.0(#289): that PR's CI failed, and the failures predated it. Worth stating plainly — this is the cost of #274 landing without its workflows ever running. Its logic was sound, which my review said and still holds; what was missing was the matrix.Tests
846 passed, 6 skipped.
ruff,mypyclean.Merge this before #289 so the release does not ship from a red main.
🤖 Generated with Claude Code