A document PyYAML cannot convert is refused in words, not raised as OverflowError - #142
Merged
Merged
Conversation
…verflowError `Policy.from_yaml` promises `PolicyError` for anything malformed, without qualification, and `strict_load` is the one loader every CTRLRun document goes through. PyYAML converts a scalar before it has decided the document is well formed, so three conversions came back as the interpreter's own exception and went straight past `except yaml.YAMLError`: - an over-long `\U` escape overflows converting the codepoint to a C int, which is `OverflowError`. The fuzzer found this after 174,380 executions; - `"\U00110000"` is above the Unicode maximum and `chr()` says `ValueError`; - `x: 2026-99-99` is a `ValueError` from `datetime`, and is the one that matters: a mistyped month is a thing a person writes in a real policy. Nothing unsafe was admitted -- the document is refused either way -- so what leaked is the exception type, and the type is the contract: a caller catching `PolicyError` around a policy load caught none of these. `ValueError` and `OverflowError` are refusals too now. Deliberately not `except Exception`: this function calls one thing, so a `MemoryError` here is not the document's fault. `RecursionError` stays uncaught for the same reason `fuzz/properties.py` excludes it. Regression tests cover all three, the first seeded from the fuzzer's own crash unit decoded as the harness decodes it, so the test fails if either the decode path or the loader changes. The crash unit now passes `check_policy`.
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
Member
Author
|
@coderabbitai review |
|
arpanghoshal
added a commit
to CTRLRun/ctrlrun-docs
that referenced
this pull request
Sep 11, 2026
CTRLRun/ctrlrun#142 documents why `strict_load` treats `ValueError` and `OverflowError` as refusals, and the docstring is rendered: `Policy.mdx` and `parse_conditions.mdx` follow it. The three regression tests move the collected count to 4,513, counted under the docs job's install line rather than this laptop's, and eleven CLAIMS citations move with the lines the docstring pushed down. Generated throughout: render_api, render_readiness, repoint-claims. Nothing here is hand-written.
arpanghoshal
added a commit
that referenced
this pull request
Sep 11, 2026
Integrated by merge rather than by rebase, and the reason is written down: this branch's reviewed commit is already on the remote, so a rebase could only reach it by force, and that is forbidden here. The tree is the one a rebase onto origin/main produces. The conflicts were the catalogue and the counts it feeds. G13 and G16 both land in ctrlrun.guarantees/v3, in id order. On SQLite G13 is N/A, because SQLite has no clock of its own, and G16 is graded, so examples/authority/payments.yaml is 12/12 with one not applicable and examples/policies/payments.yaml is 7/7 with six; ci.yml, the verify tests and the report tests carry those numbers.
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
1 participant
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.
Found by
fuzzon #140, which is how a non-required check earns its place: the failure had nothing to do with that PR and everything to do withmain.What escapes
strict_loadPolicy.from_yamlpromisesPolicyErrorfor anything malformed, without qualification, andstrict_loadis the one loader every CTRLRun document goes through. PyYAML converts a scalar before it has decided the document is well formed, so these come back as the interpreter's own exception and go straight pastexcept yaml.YAMLError:"\Ueeeeeeeeeeeeeeeee…OverflowError: Python int too large to convert to C int"\U00110000"ValueError: chr() arg not in range(0x110000)x: 2026-99-99ValueError: month must be in 1..12The third is the one that matters. The first two need a fuzzer or a bad day; a wrong month is an ordinary typo somebody writes by hand.
What this does and does not fix
Nothing unsafe was ever admitted: the document is refused either way. What leaked is the exception type, and the type is the contract. A caller wrapping a policy load in
except PolicyErrorcaught none of these.ValueErrorandOverflowErrorare refusals too now. Deliberately notexcept Exception:strict_loadcalls one thing, so aMemoryErrororKeyboardInterruptthere is not the document's fault and must not be reported as one.RecursionErrorstays uncaught for the same reasonfuzz/properties.pyalready excludes it.Tests
All three cases, with the first seeded from the fuzzer's own crash unit decoded exactly as
policy_text_from_bytesdecodes it, so the test fails if either the decode path or the loader changes. Replaying the crash unit throughproperties.check_policynow passes.Local: 2,948 passed, 65 skipped.
ruff format --check,ruff checkandmypy --strict srcclean.🤖 Generated with Claude Code