From 7d19c5bc9e5bd95f7706a993c85a3281e1471e1e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 27 Jul 2026 09:40:20 -0500 Subject: [PATCH] =?UTF-8?q?fix(tests):=20the=20at-rest=20plaintext=20probe?= =?UTF-8?q?=20was=20both=20flaky=20and=20blind=20=E2=80=94=20decode=20befo?= =?UTF-8?q?re=20matching?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tests/test_store_aad_binding.py asserted `"DOE" not in on_disk` against an mfenc cell. The cell's payload is base64 of a random nonce + AES-GCM ciphertext, so matching a 3-character needle against that TEXT is a coin flip: measured ~0.12% of runs at this payload size, about 1 in 850. It red-X'd PR #7, whose diff touched release.yml and the leak-gate docs and nothing within reach of the store. The flake is the smaller half. The same check is also BLIND to the thing it exists to catch. Handed a cell that base64-encodes the plaintext verbatim, `"DOE" not in on_disk` evaluates TRUE -- "no plaintext here" -- because base64 of "DOE^JANE" contains no literal "DOE". So it fired at random on safe data and would have passed a real PHI-at-rest leak. Demonstrated both directions in the commit's test run. And the obvious fix is a trap. Reaching for a longer, more distinctive needle -- "DOE^JANE" -- makes it VACUOUS: '^' is not in the base64 alphabet, so the assertion could never match, would pass unconditionally, and would look stronger than what it replaced. So: decode first, then match the FULL plaintext against the at-rest BYTES. A false positive now needs a collision across the whole message rather than three characters, and a cell carrying its plaintext fails loudly. `test_the_plaintext_probe_can_actually_fail` pins the non-vacuity directly: it feeds the probe a plaintext-bearing cell and asserts the probe says so. Without it the round-trip test would pass no matter what the store wrote at rest -- which is exactly the state this repo keeps rediscovering. NOT changed, because not every short needle is wrong: * tests/test_support_bundle.py:81 (`"2575" not in blob`) matches PLAINTEXT json.dumps output. It is a deterministic redaction check and a short needle is exactly right there. * tests/test_uploads.py:52 (`"MRN123" not in blob`) is the same ciphertext shape as this one, but a 6-character needle is ~400,000x safer (~3e-7 %). Left alone rather than churn it; the pattern to avoid is a SHORT needle against ciphertext, not the length itself. --- tests/test_store_aad_binding.py | 41 ++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/tests/test_store_aad_binding.py b/tests/test_store_aad_binding.py index 3283cf7..095a6eb 100644 --- a/tests/test_store_aad_binding.py +++ b/tests/test_store_aad_binding.py @@ -12,6 +12,7 @@ from __future__ import annotations +import base64 from pathlib import Path import pytest @@ -29,6 +30,28 @@ async def _open_bound(path: Path, key: str, retired: list[str] | None = None) -> return await MessageStore.open(path, cipher=make_cipher(key, retired or [], write_v2=True)) +def _plaintext_absent(cell: str, plaintext: str) -> bool: + """Is ``plaintext`` absent from the DECODED at-rest bytes of an ``mfenc`` cell? + + Decoding first is the whole point. Substring-matching the BASE64 TEXT is wrong in both directions, + and this assertion previously did it with a 3-character needle ("DOE"): + + * TOO SHORT -> FLAKY. The payload is base64 of a random nonce + AES-GCM ciphertext, so any short + needle appears by chance. Measured for a 3-char needle at this payload size: ~0.12% of runs, + i.e. roughly 1 in 850 -- which red-X'd a PR whose diff touched nothing near the store. + * TOO LONG -> VACUOUS. The obvious "fix" of a longer, more distinctive needle such as + "DOE^JANE" can NEVER match, because '^' is not in the base64 alphabet. The assertion would + pass unconditionally and prove nothing -- a worse outcome than the flake, because it looks + stronger. + + Against the decoded bytes the needle is the FULL plaintext, so a false positive needs a collision + across the whole message rather than three characters: deterministic in practice, and it still + fails loudly if a cell ever carries its plaintext. + """ + payload = cell.split(":", 4)[4] # mfenc:v2::: + return plaintext.encode() not in base64.b64decode(payload) + + @pytest.fixture async def aad_store(tmp_path: Path): store = await _open_bound(tmp_path / "aad.db", generate_key()) @@ -56,7 +79,23 @@ async def test_messages_cells_roundtrip_under_aad_bind(aad_store: MessageStore) async with aad_store._read() as db: cur = await db.execute("SELECT raw FROM messages WHERE id=?", (mid,)) on_disk = (await cur.fetchone())["raw"] - assert on_disk.startswith("mfenc:v2:") and "DOE" not in on_disk + assert on_disk.startswith("mfenc:v2:") + assert _plaintext_absent(on_disk, RAW) + + +def test_the_plaintext_probe_can_actually_fail() -> None: + """Guards the guard. + + The assertion ``_plaintext_absent`` replaced was flaky, and the obvious alternative (a longer + needle against the base64 text) would have been VACUOUS -- it could never match. A probe that + cannot fail is indistinguishable from a passing one, so pin that this probe still reports a cell + that really does carry its plaintext. Without this, the round-trip test above would pass no matter + what the store wrote at rest. + """ + leaked = "mfenc:v2:aes-gcm:k1:" + base64.b64encode(RAW.encode()).decode() + assert _plaintext_absent(leaked, RAW) is False, "the probe cannot see plaintext at rest" + opaque = "mfenc:v2:aes-gcm:k1:" + base64.b64encode(b"\x00" * 96).decode() + assert _plaintext_absent(opaque, RAW) is True async def test_queue_payload_roundtrip_and_claim(aad_store: MessageStore) -> None: