diff --git a/CHANGELOG.md b/CHANGELOG.md index ff7d84c..174169b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ Format follows [Keep a Changelog](https://keepachangelog.com/). - **Five files left the tree without leaving the disk (2026-10-08).** `AUDIT_REPORT_20260629.md` audits 18 of what are now 731 files; `ROADMAP.md` opens by declaring itself legacy and frozen against reality; `.ai-memory.toml` and `.codegraph/` are local tool state; `.compose/context/gates.md` is a working note. Each was checked for live references first — none had any, beyond `.compose` in docstrings that turned out to mean the operator's `~/.compose`, not this one. They stay locally, named in `.gitignore`. `.codegraph/` is ignored from the root file now instead of by its own nested `.codegraph/.gitignore`, because git honours a nested ignore file and build backends do not — the same asymmetry that put a 5.6 GB venv into a source distribution in the previous commit. ### Fixed +- **`is_encrypted_blob` answered a security question by sniffing one byte, and was wrong 1.68% of the time (2026-10-08).** The envelope is `nonce(24) || ciphertext`, so its first byte is the first byte of a random nonce — which is `{`, `[`, a space or a newline in **4 cases out of 256**. The old test (`head not in (b"{", b"[", b" ", b"\n")`) therefore called real, readable ciphertext "plain JSON"; measured over 4096 draws: **69 misclassifications, 1.68%**. Detection now attempts decryption: a blob is encrypted when it decrypts under the given key, and nothing else counts. Truncated blobs, filler bytes, plain JSON, tampered ciphertext and blobs encrypted under a foreign key all correctly report `False`. + + **Breaking change**, `shared/crypto.py`: `is_encrypted_blob(blob_head: bytes)` → **`is_encrypted_blob(blob: bytes, master_key: bytes)`**. A one-byte prefix cannot be authenticated, so the old signature was incapable of a correct answer — pass the whole blob. The path-based helpers `shared.master_key.is_encrypted_blob(path)` and `shared.saga.impl.crypto.is_encrypted_blob(path)` keep their signatures and are now decrypt-based too. Nothing inside the package used any of these to decide anything (the saga read path already decrypted first); they are exported utilities, which is why the wrong answer mattered for callers rather than for us. + + The workaround this bug had accumulated is gone: `tests/test_shared/test_crypto_smoke.py` used to redraw up to 32 blobs until one *looked* encrypted, because a single draw flaked ~1.6% of runs. It now asserts the opposite — it waits for the `{`-first nonce and requires detection to survive it. `tests/test_secrets.py` had been proving encryption with filler bytes; it now writes a real envelope. - **Two assertions were a 1-in-256 flake, and they were asserting the wrong thing (2026-10-08).** `test_saga_crypto_coverage` checked `not data.startswith(b"{")` to prove a state file had been encrypted. The blob is `nonce(24) || ciphertext`, so its first byte is the first byte of a random nonce — `{` in 1 write out of 256. Measured over 4096 `encrypt_json` calls: **14 began with `{`** (0.34%), and one of them failed this suite's own pre-commit gate. Fixed by asserting behaviour instead of a byte: the encrypted file must fail a plain-JSON decode, `read_state` must round-trip it, and a second `read_state_legacy_or_encrypted` must return the same value *without* a rotation warning. Verified over 2048 fresh blobs, including the 5 that begin with `{` — zero failures. Recorded separately, not fixed: the exported helper `is_encrypted_blob` still sniffs a single byte and so calls 4 of 256 real ciphertexts plain JSON (measured **69/4096 = 1.68%**); the saga read path no longer uses it, so this is an API footgun rather than a live defect. - **One persona's base was collecting five other personas (2026-10-04).** Found preparing her history import: the source database is **multi-agent** — `agent_id` is `''` for the default agent and names every other agent sharing the base — and the agent config filtered only on `role`, so the daemon tailed every agent's chat into one persona's memory. The `role` column is the owner/persona axis *within* an agent; it does not name the agent, which is precisely the mistake. Measured: **168** foreign rows in her `l0_journal`, **53** in her dispatch log, **19** mentions of other personas' names in `core_memory` and `episodes`. diff --git a/shared/crypto.py b/shared/crypto.py index 5902501..a30666c 100644 --- a/shared/crypto.py +++ b/shared/crypto.py @@ -47,12 +47,22 @@ def decrypt_json(blob: bytes, master_key: bytes) -> Any: return json.loads(box.decrypt(ct, nonce).decode("utf-8")) -def is_encrypted_blob(blob_head: bytes) -> bool: - """Check if data starts like an encrypted blob (heuristic). +def is_encrypted_blob(blob: bytes, master_key: bytes) -> bool: + """Return True when `blob` is a readable SecretBox envelope. - JSON starts with { or [. + Decided by attempting decryption, never by inspecting a byte. The envelope + is `nonce(24) || ciphertext`, so its first byte is the first byte of a + random nonce — `{` in roughly one draw out of 256. The previous first-byte + test consequently reported real ciphertext as "plain JSON" in 4 cases out + of 256 (measured: 69 of 4096 = 1.68%), which is the wrong way for a + predicate to fail: it turns a readable secret into a "missing" one. + + Pass the whole blob. A truncated one cannot authenticate and reports False. """ - if not blob_head: + try: + decrypt_json(blob, master_key) + except Exception: + # Too short, bad MAC, wrong key, or a plaintext that is not JSON: in + # every one of those cases this is not a readable envelope. return False - head = blob_head[:1] - return head not in (b"{", b"[", b" ", b"\n") + return True diff --git a/shared/master_key.py b/shared/master_key.py index ed8c7e1..5e0fc55 100644 --- a/shared/master_key.py +++ b/shared/master_key.py @@ -177,14 +177,15 @@ def decrypt_json(blob: bytes) -> Any: def is_encrypted_blob(path: Path) -> bool: - """Check if file is encrypted (not plain JSON). + """Return True when the file is a readable encrypted envelope. - Heuristic: encrypted blobs start with random 24 bytes (nonce), - JSON starts with { or [. + Reads the file and attempts decryption with this process's master key. The + previous version sniffed the first byte; because the envelope begins with a + random nonce, one byte in 256 looks like JSON (`{`, `[`, space, newline) and + real ciphertext was reported as plain JSON in 4 cases out of 256 (measured + 69/4096 = 1.68%). """ if not path.exists(): return False # Path is verified to be within app data dir by caller, safe. - with path.open("rb") as f: - head = f.read(1) - return bool(_is_encrypted_blob(head)) + return bool(_is_encrypted_blob(path.read_bytes(), _get_master_key())) diff --git a/shared/saga/impl/crypto.py b/shared/saga/impl/crypto.py index c3f5997..dab3b69 100644 --- a/shared/saga/impl/crypto.py +++ b/shared/saga/impl/crypto.py @@ -14,7 +14,6 @@ from typing import TYPE_CHECKING, Any from shared.master_key import decrypt_json, encrypt_json -from shared.crypto import is_encrypted_blob as _is_crypto_encrypted_blob logger = logging.getLogger(__name__) @@ -29,12 +28,19 @@ def is_encrypted_blob(path: Path) -> bool: - """Check if file is encrypted (not plain JSON).""" + """Return True when the file is a readable encrypted envelope. + + Decrypt-based, matching shared.master_key.is_encrypted_blob. The previous + first-byte sniff called real ciphertext plain JSON whenever the nonce began + with `{`, `[`, a space or a newline — 4 bytes in 256 (measured 1.68%). + """ if not path.exists(): return False - with path.open("rb") as f: - head = f.read(1) - return bool(_is_crypto_encrypted_blob(head)) + try: + decrypt_json(path.read_bytes()) + except Exception: + return False + return True if TYPE_CHECKING: diff --git a/tests/test_secrets.py b/tests/test_secrets.py index 67f94ba..5059aab 100644 --- a/tests/test_secrets.py +++ b/tests/test_secrets.py @@ -27,12 +27,16 @@ def test_tampered_ciphertext_rejected(): def test_is_encrypted_blob(tmp_path: Path): - from features.secrets import is_encrypted_blob + from features.secrets import encrypt_json, is_encrypted_blob plain = tmp_path / "plain.json" enc = tmp_path / "enc.json" plain.write_text('{"a": 1}') - enc.write_bytes(b"\xab\xcd" * 30) + # A real envelope. This used to be filler bytes (b"\xab\xcd" * 30), which + # only ever passed because the old check looked at a single byte and + # anything not `{`/`[`/space/newline counted as encrypted. Detection now + # decrypts, so the file has to be genuinely encrypted to count. + enc.write_bytes(encrypt_json({"a": 1})) assert not is_encrypted_blob(plain) assert is_encrypted_blob(enc) diff --git a/tests/test_shared/test_crypto_smoke.py b/tests/test_shared/test_crypto_smoke.py index 6bf851c..f2108c3 100644 --- a/tests/test_shared/test_crypto_smoke.py +++ b/tests/test_shared/test_crypto_smoke.py @@ -19,18 +19,46 @@ def key() -> bytes: def test_roundtrip_dict(key): payload = {"k": "v", "n": 42, "nested": {"a": [1, 2, 3]}} - # 19.09: is_encrypted_blob is a first-byte heuristic, and the nonce is - # random — P(first byte looks like JSON) ≈ 4/256 per draw. A single draw - # flakes ~1.6% of gate runs. Redraw boundedly; the heuristic intent stays. + blob = crypto.encrypt_json(payload, key) + assert crypto.is_encrypted_blob(blob, key) + assert crypto.decrypt_json(blob, key) == payload + + +def test_detection_survives_a_nonce_that_looks_like_json(key): + """Regression: detection must not depend on the nonce's first byte. + + The envelope is nonce(24) || ciphertext and the nonce is random, so its + first byte is `{` in about 1 draw out of 256. The old first-byte test + pronounced such real ciphertext "plain JSON" — measured 69 of 4096 draws + (1.68%) — which made a readable secret look missing. This test waits for + that exact draw instead of avoiding it: the loop needs ~256 iterations, and + (255/256)**16384 ≈ 0, so it is deterministic in practice. + """ + payload = {"secret": "value"} blob = b"" - for _ in range(32): - blob = crypto.encrypt_json(payload, key) - if crypto.is_encrypted_blob(blob[:8]): + for _ in range(16384): + candidate = crypto.encrypt_json(payload, key) + if candidate[:1] == b"{": + blob = candidate break - assert blob != b"" and crypto.is_encrypted_blob(blob[:8]) + assert blob, "could not obtain a blob starting with '{' — test would be vacuous" + assert blob[:1] == b"{" # the very draw that used to break detection + assert crypto.is_encrypted_blob(blob, key) assert crypto.decrypt_json(blob, key) == payload +def test_detection_rejects_anything_that_is_not_a_readable_envelope(key): + assert not crypto.is_encrypted_blob(b'{"plain": true}', key) # plain JSON + assert not crypto.is_encrypted_blob(b"", key) # empty + assert not crypto.is_encrypted_blob(b"\x00" * 10, key) # shorter than nonce+MAC + assert not crypto.is_encrypted_blob(b"\xab\xcd" * 30, key) # filler, not an envelope + foreign = crypto.encrypt_json({"a": 1}, os.urandom(32)) + assert not crypto.is_encrypted_blob(foreign, key) # encrypted, but under another key + tampered = bytearray(crypto.encrypt_json({"a": 1}, key)) + tampered[-1] ^= 0xFF + assert not crypto.is_encrypted_blob(bytes(tampered), key) # MAC no longer matches + + def test_roundtrip_list_and_unicode(key): payload = ["строка", "второй", 3] blob = crypto.encrypt_json(payload, key)