From 4c2030ce4d9fc233f747943b7b11d4af211e011f Mon Sep 17 00:00:00 2001 From: arpan Date: Mon, 14 Sep 2026 19:24:50 +0530 Subject: [PATCH 1/2] A row that does not parse is one row too An independent review found json.loads running outside _read_receipt's guard. Signed-off-by: arpan --- src/ctrlrun/postgres.py | 4 +- src/ctrlrun/receipt.py | 29 ++++++-- src/ctrlrun/state.py | 6 +- tests/test_unreadable_receipt.py | 117 +++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 10 deletions(-) diff --git a/src/ctrlrun/postgres.py b/src/ctrlrun/postgres.py index 15a431be..aea5e467 100644 --- a/src/ctrlrun/postgres.py +++ b/src/ctrlrun/postgres.py @@ -2193,7 +2193,9 @@ def receipts(self) -> tuple[Receipt | UnreadableReceipt, ...]: # `json` here is `json.dumps(..., sort_keys=True)` and SQLite's is `to_json()`, which are # different byte strings -- and the chain does not care, because `chain_hash` recomputes # the canonical form from the parsed document rather than hashing whatever was stored. - return tuple(_read_receipt(json.loads(str(row[1])), row[2], row[0]) for row in rows) + # The stored **text**, not a parsed document, for `state.py`'s reason: parsing is one of + # the ways a row refuses, and a `json.loads` out here would raise through every caller. + return tuple(_read_receipt(str(row[1]), row[2], row[0]) for row in rows) def chain_head(self) -> tuple[int, str] | None: with self._connection().cursor() as cursor: diff --git a/src/ctrlrun/receipt.py b/src/ctrlrun/receipt.py index d2356ffd..903722bc 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -870,20 +870,37 @@ def _readable(rows: Iterable[Receipt | UnreadableReceipt]) -> tuple[Receipt, ... def _read_receipt( - document: Mapping[str, Any], stored_hash: str | None, stored_seq: int | None + stored_json: str, stored_hash: str | None, stored_seq: int | None ) -> Receipt | UnreadableReceipt: """One stored row, as a receipt or as a named refusal (SPEC-v0.11 §5.2). The one place a store turns a row into something a reader holds, so the two backends cannot come to disagree about what a row it cannot construct becomes. - `CTRLRunError` and nothing wider: `from_dict` raises `InvalidArgument` through the parsers it - calls, and a `KeyError` or a `TypeError` from a row that is not an object at all is the case - `from_dict`'s own docstring says raises as it did at 0.6.1. Both are caught, because a row a - tamperer truncated to `{}` is exactly as much "one bad row" as a float among the controls, and - a reader that recovered from one and not the other would still be blindable by one `UPDATE`. + **It takes the stored text and not a parsed document, because parsing is one of the ways a + row refuses.** The first version of this took a `Mapping` and both stores called + `json.loads(row["json"])` in the generator expression that fed it, so `json` set to anything + that is not JSON at all raised `JSONDecodeError` *outside* this guard and blinded every + reader exactly as before -- and worse than before, because `JSONDecodeError` is not a + `CTRLRunError`, so `cli/main.py`'s handler did not catch it either and `ctrlrun receipts` + printed a traceback. One `UPDATE receipts SET json = 'not json'` was enough. The rule this + broke is rule 3 itself, and the reason the first version's tests missed it is that they + tampered with a row's *content*: `{}` and a float among the controls are both valid JSON. + + `CTRLRunError` and the four builtins `from_dict` can raise: `InvalidArgument` through the + parsers it calls, `KeyError` or `TypeError` from a row that is not an object at all, which + `from_dict`'s own docstring says raises as it did at 0.6.1, and `ValueError`, which + `JSONDecodeError` subclasses. A reader that recovered from one and not another would still be + blindable by one `UPDATE`. """ + document: object = None try: + document = json.loads(stored_json) + # Narrowed here rather than trusted: `json.loads("3")` is an `int`, and `_stored_receipt` + # would raise `TypeError` on it, which this catches -- but naming the refusal at the + # parse says what is wrong with the row rather than what the next line tripped over. + if not isinstance(document, Mapping): + raise TypeError(f"a stored receipt must be an object, got {type(document).__name__}") return _stored_receipt(document, stored_hash, stored_seq) except (CTRLRunError, KeyError, TypeError, ValueError, AttributeError) as refused: identifier = document.get("receipt_id") if isinstance(document, Mapping) else None diff --git a/src/ctrlrun/state.py b/src/ctrlrun/state.py index d01824a9..c051633b 100644 --- a/src/ctrlrun/state.py +++ b/src/ctrlrun/state.py @@ -1712,9 +1712,9 @@ def receipts(self) -> tuple[Receipt | UnreadableReceipt, ...]: # # `_read_receipt` and not `_stored_receipt`: a row this binary cannot construct comes # back named at its `seq` rather than raising through every caller at once (§5.2). - return tuple( - _read_receipt(json.loads(row["json"]), row["hash"], row["seq"]) for row in rows - ) + # The stored **text**, not a parsed document: parsing is one of the ways a row refuses, + # and a `json.loads` out here would raise through every caller (§5.2). + return tuple(_read_receipt(row["json"], row["hash"], row["seq"]) for row in rows) # --- delegations (SPEC-v0.3 §5.2) ------------------------------------------------- diff --git a/tests/test_unreadable_receipt.py b/tests/test_unreadable_receipt.py index 1a889509..dd067728 100644 --- a/tests/test_unreadable_receipt.py +++ b/tests/test_unreadable_receipt.py @@ -648,3 +648,120 @@ def reopen(self): # that fails everything. clean = evidence_receipt.body(SQLiteBackend(tmp_path), 1) assert clean.status is SuiteStatus.PASS, clean + + +# --- T520: a row that does not parse at all ---------------------------------------------------- + + +#: Every tamper that stops a row parsing **before** `Receipt.from_dict` is reached. `T510` to +#: `T519` all tampered with a row's *content*, and `{}` and a float among the controls are both +#: valid JSON, so the parse was never on trial. An independent review found the gap: `json.loads` +#: ran in the generator expression that fed `_read_receipt`, outside its guard, so one `UPDATE` +#: setting `json` to anything unparseable raised through every reader exactly as before v0.11. +#: Worse than before, because `JSONDecodeError` is not a `CTRLRunError`, so `cli/main.py`'s +#: handler did not catch it either and `ctrlrun receipts` printed a **traceback**. +UNPARSEABLE = ( + ("not JSON at all", "not json at all", "JSONDecodeError"), + ("empty", "", "JSONDecodeError"), + ("truncated mid-object", '{"receipt_id": "ctr_1', "JSONDecodeError"), + ("a JSON array, not an object", "[1, 2, 3]", "TypeError"), + ("a bare JSON number", "3", "TypeError"), + ("a bare JSON string", '"a receipt"', "TypeError"), + ("JSON null", "null", "TypeError"), +) + + +@pytest.mark.parametrize( + ("label", "stored", "refusal"), UNPARSEABLE, ids=[t[0] for t in UNPARSEABLE] +) +def test_T520_a_row_that_does_not_parse_still_costs_one_row(workspace, label, stored, refusal): + """SPEC-v0.11 §5.2 and rule 3, through the door the first implementation left open. + + Rule 3 is *a malformed row names itself and blinds nothing else*, and it says **row**, not + "row whose content is wrong". A tamperer writing `not json` is doing less work than one + writing a well-formed document with a float in it, so a reader that survives the second and + not the first has not paid the debt. + + `TypeError` for the four that parse to something that is not an object: `json.loads("3")` is + an `int`, and the refusal names what is wrong with the row rather than what the next line + tripped over. + """ + database = workspace / "state.db" + store = SQLiteStateStore(database, clock=lambda: T0) + written = a_chain(store, 4) + ids = [receipt.receipt_id for receipt in written] + untouched = written[0].action_id + store.close() + + connection = sqlite3.connect(database) + connection.execute("UPDATE receipts SET json = ? WHERE seq = 2", (stored,)) + connection.commit() + connection.close() + + reopened = SQLiteStateStore(database, clock=lambda: T0) + rows = reopened.receipts() + report = verify_chain(reopened) + reopened.close() + + assert len(rows) == 4, f"{label}: one bad row cost {4 - len(rows)} extra rows" + assert isinstance(rows[1], UnreadableReceipt), rows + assert rows[1].seq == 2, "the refusal does not know where it is" + assert rows[1].refusal == refusal, f"{label}: refused as {rows[1].refusal}" + assert rows[1].receipt_id is None, "a receipt_id was invented for a row that has none" + assert isinstance(rows[0], Receipt) and isinstance(rows[2], Receipt) + assert ("content_altered", 2) in [(item.name, item.seq) for item in report.breaks] + + # And through the CLI, which is where this failed worst: `JSONDecodeError` is not a + # `CTRLRunError`, so the handler did not catch it and the command printed a traceback. + listed = _cli(workspace, database, "receipts") + assert listed.exit_code == 0, listed.output + assert "Traceback" not in listed.output, ( + f"{label}: ctrlrun receipts printed a traceback:\n{listed.output}" + ) + for receipt_id in [ids[0], ids[2], ids[3]]: + assert receipt_id in listed.output, f"{label}: an intact row is missing from the listing" + + inspected = _cli(workspace, database, "inspect", untouched) + assert inspected.exit_code == 0, inspected.output + assert "Traceback" not in inspected.output, inspected.output + + counted = _cli(workspace, database, "stats") + assert counted.exit_code == 0, counted.output + assert "Traceback" not in counted.output, counted.output + assert "unreadable receipts 1" in counted.output, counted.output + + +@postgres +def test_T520b_postgres_refuses_an_unparseable_row_the_same_way(workspace) -> None: + """The amendment is to `StateStore`, so a backend that raised here would blind every reader + in a deployment that uses it. `json` is a `text` column on both sides, so both can hold this. + """ + import psycopg + + from ctrlrun.postgres import PostgresStateStore + + schema = f"unparseable_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(POSTGRES_URL, schema) + try: + store = PostgresStateStore(POSTGRES_URL, schema=schema, clock=lambda: T0) + a_chain(store, 4) + store.close() + + with psycopg.connect(POSTGRES_URL) as connection: + with connection.cursor() as cursor: + cursor.execute( + f'UPDATE "{schema}".receipts SET json = %s WHERE seq = 2', ("not json at all",) + ) + connection.commit() + + reopened = PostgresStateStore(POSTGRES_URL, schema=schema, clock=lambda: T0) + rows = reopened.receipts() + report = verify_chain(reopened) + reopened.close() + + assert len(rows) == 4, "one bad row cost the Postgres reader more than one row" + assert isinstance(rows[1], UnreadableReceipt) + assert rows[1].seq == 2 and rows[1].refusal == "JSONDecodeError" + assert ("content_altered", 2) in [(item.name, item.seq) for item in report.breaks] + finally: + PostgresStateStore.drop_schema(POSTGRES_URL, schema) From be678374c1c9effdfd75990df414173c548f2b77 Mon Sep 17 00:00:00 2001 From: arpan Date: Mon, 14 Sep 2026 19:30:21 +0530 Subject: [PATCH 2/2] CHANGELOG: the unparseable row Signed-off-by: arpan --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3df981c..7580e518 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,16 @@ any change to one appears here. it, because `content_altered` already names a document that cannot be canonicalized and a second name for one fact would be two names for one break. +- **A row that does not parse is one row too.** The first implementation of the reader above + called `json.loads` in the generator expression that fed it, **outside** the guard, so a row + whose stored `json` is not JSON at all raised through every reader exactly as before v0.11, and + worse: `JSONDecodeError` is not a `CTRLRunError`, so the CLI's handler did not catch it either + and `ctrlrun receipts` printed a traceback. One `UPDATE receipts SET json = 'not json'` was + enough. Parsing now happens inside the refusal's own guard, and a row that parses to something + that is not an object (`3`, `"a receipt"`, `[1,2,3]`, `null`) is refused as one row rather than + trusted. Found by review; the tests that missed it all tampered with a row's *content*, and + `{}` and a float among the controls are both valid JSON. + ### Changed - **`StateStore.receipts()` returns `tuple[Receipt | UnreadableReceipt, ...]`**, amending