Refuse a string UTF-8 cannot encode, in words rather than as a UnicodeEncodeError - #94
Merged
Conversation
…eEncodeError `fuzz/` found this on its first run, before Atheris was even wired up. `canonical_bytes` raised `UnicodeEncodeError` for a string holding an unpaired UTF-16 surrogate. It arrives the ordinary way -- `json.loads` produces one from a six-character escape -- so an MCP tool call carried it into the action path; `Action(...)` accepted it and `action_hash` was where it failed. Fail-closed **held**: the gateway and `Control` both wrap the action path in `except Exception`, so this was a contract violation and a crash, not an authorization bypass. What it broke is the closed error set in `errors.py`. A caller catching `CTRLRunError` did not catch this one. **Two checks, and neither is subsumed by the other.** `_encodable` refuses at construction, beside the `float` and non-string-key checks, so an `Action` cannot be built holding a value it could never hash -- a trap set at construction and sprung somewhere else. `canonical_bytes` refuses again at the encode, which is the only guard on the path that does not go through `Action` at all: the policy hash and the receipt chain call it directly on documents. Mutation-tested separately; removing the second turns four fuzz tests red. **A refusal, not a repair.** `errors="replace"` would map two distinct arguments onto one canonical form, which is the collision §2.3 exists to prevent. Checked at the encode rather than per string, so it costs nothing on the path that succeeds -- the encode already walks every character. A *paired* surrogate is unaffected, and a negative control asserts it: a check keyed on "contains a surrogate code point" rather than on encodability would reject an ordinary emoji and still pass every other test here. That mutation is in the table and is caught. §2.3's rule is satisfied without a schema bump: `test_T164b_every_recorded_hash_still_verifies` passes, because every input that changed behaviour is one that previously raised. Five mutations, five caught. `fuzz/KNOWN_FINDINGS` is empty again, and it emptied itself: the entry's own test asserted the finding *still reproduced*, so fixing the bug turned it red and forced the entry out. `fuzz/README.md` keeps it as a worked example of the mechanism rather than as a live defect, and the reproducer stays in the corpus as a regression input. Also regenerated: the API reference, whose docstrings changed, and three `docs/CLAIMS.md` anchors that pointed at `action.py:47` for the float refusal and had drifted to line 71.
arpanghoshal
force-pushed
the
fix/canonical-surrogate
branch
from
September 6, 2026 18:21
7dfa851 to
4557279
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…the style `py/empty-except` on the regression test's `try/except CTRLRunError: pass`. The finding reads as a style nit and is not one. `InvalidArgument` **is** a `CTRLRunError`, so the `pytest.raises(InvalidArgument)` on the line above already proved that branch, and the `except UnicodeEncodeError` sitting beside it was unreachable from any input. A subsumed guard and an orphaned handler in six lines, in a test whose whole subject is guards that do not fire. Replaced with two assertions on the raised exception: that it is in the closed set, so `except CTRLRunError` catches it, and that it is not the `UnicodeEncodeError` this used to raise, which no such handler would have caught. Both are reachable and both are asserted. Mutation-checked: removing `_encodable` from `_frozen_value` still turns this test red.
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
2 participants
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.
The finding
fuzz/found this on its first run, before Atheris was even wired up.canonical_bytesraisedUnicodeEncodeErrorfor a string holding an unpaired UTF-16 surrogate. It arrives the ordinary way —json.loadsproduces one from a six-character escape — so an MCP tool call carried it into the action path.Action(...)accepted it;action_hashwas where it failed.Fail-closed held. The gateway and
Controlboth wrap the action path inexcept Exception, so this was a contract violation and a crash, not an authorization bypass. What it broke is the closed error set inerrors.py: a caller catchingCTRLRunErrordid not catch this one.Two checks, neither subsumed by the other
_encodable, at construction — beside thefloatand non-string-key checks, so anActioncannot be built holding a value it could never hash. An object that constructs fine and explodes later is a trap set in one place and sprung in another.canonical_bytes, at the encode — the only guard on the path that never goes throughAction. The policy hash and the receipt chain callcanonical_bytesdirectly on documents.I expected the second to be redundant and mutation-testing said otherwise: removing it turns four fuzz tests red.
A refusal, not a repair
errors="replace"would map two distinct arguments onto one canonical form — precisely the collision §2.3 exists to prevent.Checked at the encode rather than per string, so it costs nothing on the path that succeeds: the encode already walks every character.
A paired surrogate is unaffected, and a negative control asserts it. A check keyed on "contains a surrogate code point" rather than on encodability would reject an ordinary emoji and still pass every other test here — that mutation is in the table below and is caught.
§2.3's rule, without a schema bump
test_T164b_every_recorded_hash_still_verifiespasses. Every input whose behaviour changed is one that previously raised, so no hash that ever succeeded is different.Mutation table
canonical_bytesremovederrors="replace")nested-surrogate)The KNOWN_FINDINGS mechanism emptied itself
I did not have to remember to remove the entry. Its own test asserted the finding still reproduced, so fixing the bug turned that test red and forced the entry out.
fuzz/README.mdnow keeps it as a worked example of the mechanism rather than as a live defect, and the reproducer stays in the corpus as a regression input.Two generated-docs updates
Regenerated the API reference (docstrings changed), and repointed three
docs/CLAIMS.mdanchors that namedaction.py:47for the float refusal and had drifted to line 71. The CLAIMS line-anchor guard caught that itself.Checks
ruff format --check,ruff check,mypy --strict srcclean; full suite 3946 passed, 45 skipped.One process note: an earlier full-suite run came back green and I discarded it — a timed-out mutation task had restored
action.pymid-run, so the tree under test was not the tree being committed. The clean re-run found the two docs failures above. A green suite on a tree you cannot vouch for is worth less than no run at all.