From 5e2e1a3216daee5df44546625552da3a1801d5f5 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 10:28:19 -0500 Subject: [PATCH 1/8] test(14.2.7): land the no-DB guards FIRST, so the retirement cannot ship half-done Commit 1 of ASVS 14.2.7. No production code. Three guards that run on the plain leg with NO database, deliberately landed BEFORE the changes they police. TWO independent nine-agent workflows analysed this change (two Claude processes under one session id, neither aware of the other). They produced DIFFERENT plans -- 12 traps and 13 traps, 20 distinct. Both are preserved with their union in vault PR #1258. Two of the three guards here exist because of traps only ONE pass found, which is the argument for having run both. --- 1. tests/test_sqlserver_encrypt_pass_tables.py (AST, no DB) --------------- Every (table, column) literal driving a ciphered-cell sweep must name a table the module actually CREATEs. This is the ONLY mechanism in the repo that can catch the defect. Commit 6 drops the legacy `outbox` table; the pair ("outbox","payload") sits ~1200 lines away in _encrypt_existing_rows. That method early-returns on `if not self._cipher.encrypts` -- and EVERY SQL Server CI step runs KEYLESS, so the SELECT below that return never executes on any leg. Plan A's own stated mutation proof for this trap ("the keyed open must red") reds on no CI leg that exists. A keyed production open would crash-loop on Invalid object name; CI would stay green. The naive version over-matched on its first run and that was useful: both sweeps also build composite AAD tuples like ("body","detail") and ("event_type", "connection") whose first element is a COLUMN. Flagging those would have cried wolf on ~14 legitimate pairs, and a guard that always fails gets xfailed or deleted. So it anchors on the LOOP (`for table, in (...)`), not on "any 2-string tuple in the function". Four mutations against the REAL source, not the fixture: M1 unknown table in _encrypt_existing_rows -> RED M2 unknown table in reencrypt_to_active -> RED (both sweeps walked) M3 ("outbox","payload") REMOVED -> GREEN (must not block Commit 6) M4 CREATE TABLE regex blinded -> RED (liveness receipt fires rather than passing vacuously) M3 and M4 matter as much as M1: a guard that blocks its own fix, or that passes when blinded, is worse than none. --- 2. tests/test_fixture_outbox_reset.py (AST, no DB) ------------------------ No fixture may issue an UNGUARDED `DELETE FROM outbox` once the table is retired. The existence-guarded form f"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}" is a no-op afterwards and is accepted; the bare form is not. That distinction is structural, so the scanner reads the loop BODY rather than grepping for the word. MEASURED, independently reproducing plan B's trap 8: 538 test files scanned, 21 unguarded reset sites across 18 files, and NINE of those 18 run in no SQL Server CI step at all -- test_adr0071_dispatch_wiring_sqlserver, ..._fused_callables_..., test_adr0075_batch_sqlserver, test_adr0114_claim_proc_live, test_batch_completion, test_metadata_bag, test_outbound_batch, test_shard_recovery_sqlserver, test_sqlserver_sync_handoff. A mutation planted in any of those passes VACUOUSLY, so "the SS leg is green" was never evidence the reset lists were updated. Marked xfail(strict=True), not left red: the 21 sites are CORRECT today (the table exists). strict makes it SELF-CLEARING -- the moment the last reset list is fixed the test XPASSes, which strict turns into a failure, forcing the marker out in the same commit. A non-strict xfail would rot silently green forever. --- 3. _RETIRED_CLAIMS registrations (tests/test_phi_at_rest_inventory.py) ---- Three strings that are TRUE TODAY and become false in Commits 3 and 6, registered FIRST so the guard reds against the UNEDITED docs. Confirmed RED at registration: docs/PHI.md:123 '**No purge path at all**' docs/PHI.md:401 'is purged by nothing on any backend' docs/PHI.md:1050 'touched by no purge on any backend' That output is the evidence, and it is only obtainable BEFORE the edits. A registration made afterwards passes on day one whether or not the guard works. A FOURTH candidate was STRUCK, and this is the trap only plan B found. The obvious string "no retention purge" collides with a still-TRUE sentence at docs/security/ASVS-292-289-HANDOFF-2026-07-25.md:173 about pending_approvals.params. _retired_claim_hits() globs docs/*.md PLUS docs/security/*.md -- and docs/security/ is GIT-IGNORED. Registering it would red on a laptop and pass in CI, where the file does not exist. A guard strictly weaker in CI than locally is not a guard. Plan A instructed adding it, with no warning; I was minutes from doing so. Grep evidence recorded in-file (docs/*.md | docs/security/*.md): "no retention purge" 1 | 1 <- STRUCK "**No purge path at all**" 1 | 0 "touched by no purge on any backend" 1 | 0 "is purged by nothing on any backend" 1 | 0 DELIBERATELY RED IN THIS COMMIT: test_retired_false_claims_do_not_reappear fails until Commit 3 edits those three PHI.md lines. That is the forcing function, and CI gates the PR head, not each commit. --- tests/test_fixture_outbox_reset.py | 180 ++++++++++++++++++++ tests/test_phi_at_rest_inventory.py | 23 +++ tests/test_sqlserver_encrypt_pass_tables.py | 168 ++++++++++++++++++ 3 files changed, 371 insertions(+) create mode 100644 tests/test_fixture_outbox_reset.py create mode 100644 tests/test_sqlserver_encrypt_pass_tables.py diff --git a/tests/test_fixture_outbox_reset.py b/tests/test_fixture_outbox_reset.py new file mode 100644 index 00000000..ca6a0f9f --- /dev/null +++ b/tests/test_fixture_outbox_reset.py @@ -0,0 +1,180 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""No test fixture may issue an UNGUARDED ``DELETE FROM outbox`` once the legacy table is retired. + +ASVS 14.2.7 migrates the legacy SQL Server ``outbox`` table into ``queue`` and DROPs it. After that, a +fixture doing:: + + for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + await cur.execute(f"DELETE FROM {table}") + +fails with *Invalid object name 'outbox'* and takes its whole module with it. + +**Why a source scanner rather than "run the tests and see":** of the eighteen files carrying such a +reset list, **nine appear in no SQL Server CI step at all** — `test_adr0071_dispatch_wiring_sqlserver`, +`test_adr0071_fused_callables_sqlserver`, `test_adr0075_batch_sqlserver`, `test_adr0114_claim_proc_live`, +`test_batch_completion`, `test_metadata_bag`, `test_outbound_batch`, `test_shard_recovery_sqlserver`, +`test_sqlserver_sync_handoff`. A mutation planted in any of those **passes vacuously**, so "the SS leg +is green" is not evidence that the reset lists were all updated. This test needs no database and runs +on the plain leg, which is the only leg guaranteed to exist. + +The **guarded** form is fine and stays:: + + await cur.execute(f"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}") + +— it is a no-op once the table is gone. That distinction is structural, not stylistic, which is why this +scanner reads the loop body rather than grepping for the word ``outbox``. +""" + +from __future__ import annotations + +import ast +import pathlib + +import pytest + +_TESTS = pathlib.Path(__file__).resolve().parent + +#: Existence-guard markers. A reset whose statement contains one of these degrades to a no-op when the +#: table is absent, so it survives the retirement untouched. +_GUARD_MARKERS = ("OBJECT_ID", "IF EXISTS", "information_schema", "in existing", "to_regclass") + +#: `"outbox"` appears in these for reasons that are not a reset list. Exempted by FILE, with the reason, +#: because a blanket substring skip would also hide a real one landing in the same file later. +_NOT_RESET_LISTS = { + # A Pydantic field-name allow-list and a forbidden-substring tuple — neither is a table name. + "test_security_doc_drift.py": "_MAPPED_MODEL_NON_PHI_FIELDS — model field names, not tables", + "test_tray_boundary.py": "_FORBIDDEN_FIELD_SUBSTRINGS — substring matching, not tables", + # Response-payload dict keys asserted in API tests. + "test_api.py": "response-body keys, not tables", + "test_api_auth.py": "response-body keys, not tables", + # This scanner and the AST guard both name the table in prose/fixtures on purpose. + "test_fixture_outbox_reset.py": "this file", + "test_sqlserver_encrypt_pass_tables.py": "synthetic fixtures naming the table deliberately", +} + + +def _unguarded_outbox_resets(path: pathlib.Path) -> list[tuple[int, str]]: + """Loops over a literal table tuple containing ``"outbox"`` whose body issues a BARE delete. + + Anchored on the `for` node and its body's source span, so the guarded and unguarded spellings are + told apart by what the statement actually does — not by whether the word ``outbox`` is nearby. + """ + source = path.read_text(encoding="utf-8") + try: + tree = ast.parse(source) + except SyntaxError: # pragma: no cover - a test file that does not parse fails elsewhere + return [] + lines = source.splitlines() + hits: list[tuple[int, str]] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.For | ast.AsyncFor): + continue + if not isinstance(node.iter, ast.Tuple | ast.List | ast.Set): + continue + names = { + e.value + for e in node.iter.elts + if isinstance(e, ast.Constant) and isinstance(e.value, str) + } + if "outbox" not in names: + continue + body_lines: list[str] = [] + for stmt in node.body: + body_lines.extend(lines[stmt.lineno - 1 : (stmt.end_lineno or stmt.lineno)]) + body_src = "\n".join(body_lines) + if "DELETE FROM" not in body_src.upper(): + continue + if any(marker.upper() in body_src.upper() for marker in _GUARD_MARKERS): + continue # existence-guarded — a no-op once the table is gone + hits.append((node.lineno, body_src.strip().splitlines()[0][:100])) + return hits + + +def _scanned_files() -> list[pathlib.Path]: + return sorted(p for p in _TESTS.glob("test_*.py") if p.name not in _NOT_RESET_LISTS) + + +@pytest.mark.xfail( + strict=True, + reason=( + "EXPECTED RED until the legacy SQL Server `outbox` table is retired. 21 unguarded reset sites " + "across 18 files still name it, and they are CORRECT today — the table exists. This guard " + "lands FIRST, before the retirement, so the retirement cannot ship half-done; its failure list " + "is the work list. strict=True makes this self-clearing: the moment the last reset list is " + "fixed the test XPASSes, which strict turns into a FAILURE, forcing this marker to be removed " + "in the same commit. A non-strict xfail would let the guard rot silently green forever." + ), +) +def test_no_fixture_issues_an_unguarded_outbox_delete() -> None: + """After the legacy table is retired, an unguarded reset is a module-wide failure. + + Mutation: add `"outbox"` to the reset tuple in `tests/test_metadata_bag.py` — reds here on the plain + leg naming the file and line, which is the only place it can be caught for the nine files that run + on no SQL Server CI step. + + NOTE this test is expected to FAIL until Commit 6 updates the reset lists. That is deliberate and is + the point of landing the guard first: the failure list below IS the work list. + """ + scanned = _scanned_files() + # Liveness receipt: report what was EXAMINED, not just what was found. A glob that silently matched + # nothing would make this pass while checking zero files. + assert len(scanned) > 100, f"only scanned {len(scanned)} test files — the glob looks broken" + + offenders: list[str] = [] + for path in scanned: + for lineno, snippet in _unguarded_outbox_resets(path): + offenders.append(f" {path.name}:{lineno} {snippet}") + + assert not offenders, ( + f"scanned {len(scanned)} test files; {len(offenders)} carry an UNGUARDED " + f"`DELETE FROM outbox` reset. Once the legacy table is dropped each of these fails with " + f"'Invalid object name' and takes its whole module with it — and nine of them run on no SQL " + f"Server CI step, so no live leg would tell you:\n" + "\n".join(offenders) + "\n\n" + "Fix by removing 'outbox' from the tuple, or by using the guarded form:\n" + " await cur.execute(f\"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}\")" + ) + + +def test_the_scanner_distinguishes_guarded_from_unguarded() -> None: + """Prove the detector's discrimination, rather than trusting it. + + A scanner that flagged everything would make the test above permanently red and get deleted; one + that flagged nothing would pass forever. Drive both spellings through it. + """ + unguarded = _TESTS / "_scan_fixture_unguarded.py" + guarded = _TESTS / "_scan_fixture_guarded.py" + unguarded.write_text( + "async def f(cur):\n" + ' for table in ("queue", "outbox", "messages"):\n' + ' await cur.execute(f"DELETE FROM {table}")\n', + encoding="utf-8", + ) + guarded.write_text( + "async def f(cur):\n" + ' for table in ("queue", "outbox", "messages"):\n' + " await cur.execute(\n" + " f\"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}\"\n" + " )\n", + encoding="utf-8", + ) + try: + assert _unguarded_outbox_resets(unguarded), "the bare DELETE was NOT flagged" + assert not _unguarded_outbox_resets(guarded), ( + "the OBJECT_ID-guarded DELETE was wrongly flagged" + ) + finally: + unguarded.unlink(missing_ok=True) + guarded.unlink(missing_ok=True) + + +@pytest.mark.parametrize("exempt", sorted(_NOT_RESET_LISTS)) +def test_every_exemption_still_exists_and_still_needs_exempting(exempt: str) -> None: + """An exemption for a file that no longer exists, or no longer mentions the table, is dead weight + that quietly widens the next person's blind spot. Fail when one goes stale.""" + path = _TESTS / exempt + assert path.exists(), f"exempted file {exempt} is gone — drop it from _NOT_RESET_LISTS" + assert "outbox" in path.read_text(encoding="utf-8"), ( + f"{exempt} no longer mentions 'outbox' — the exemption is stale and should be removed, " + f"otherwise a genuine reset list landing in this file later would be silently skipped" + ) diff --git a/tests/test_phi_at_rest_inventory.py b/tests/test_phi_at_rest_inventory.py index 14157b6d..01eb84b3 100644 --- a/tests/test_phi_at_rest_inventory.py +++ b/tests/test_phi_at_rest_inventory.py @@ -98,6 +98,29 @@ # uses "no retention path" (the surviving gaps say "no purge"/"keep-forever"). "no retention path at all", "no retention path exists at all", + # ASVS 14.2.7 (this change): `reference.value` gains purge_reference_snapshots, and the legacy + # SQL Server `outbox` table is migrated into `queue` and DROPped. The three statements below are + # TRUE TODAY and become false in Commits 3 and 6 — they are registered HERE, first, so that + # `test_retired_false_claims_do_not_reappear` goes RED against the UNEDITED docs and only turns + # green once the edits actually land. Registering after the edit proves nothing: it would pass on + # day one whether or not the guard works. + # + # A FOURTH candidate was DELIBERATELY STRUCK. The obvious string "no retention purge" collides + # with a still-TRUE sentence: + # docs/security/ASVS-292-289-HANDOFF-2026-07-25.md:173 + # "`pending_approvals.params` is unencrypted and has no retention purge on any backend." + # `_retired_claim_hits()` globs docs/*.md PLUS docs/security/*.md — and docs/security/ is + # GIT-IGNORED. So registering it reds on a developer's machine and passes in CI, where the file + # does not exist. A guard that is strictly weaker in CI than on a laptop is not a guard. + # Grep evidence recorded at registration time (docs/*.md | docs/security/*.md): + # "no retention purge" 1 | 1 <- STRUCK, collides with a true statement + # "**No purge path at all**" 1 | 0 + # "touched by no purge on any backend" 1 | 0 + # "is purged by nothing on any backend" 1 | 0 + # Re-run that grep before adding any further string here. + "**No purge path at all**", + "touched by no purge on any backend", + "is purged by nothing on any backend", ) #: Filename markers identifying an ASVS **scoring / evidence** artifact. Those documents quote the #: retired wording deliberately, as the record of what was found, so scanning them for it would be a diff --git a/tests/test_sqlserver_encrypt_pass_tables.py b/tests/test_sqlserver_encrypt_pass_tables.py new file mode 100644 index 00000000..32456443 --- /dev/null +++ b/tests/test_sqlserver_encrypt_pass_tables.py @@ -0,0 +1,168 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Every ciphered (table, column) the SQL Server backend sweeps must name a table it actually creates. + +**This guard exists because CI cannot catch the defect any other way.** `_encrypt_existing_rows` +early-returns at `if not self._cipher.encrypts: return` — and **every** SQL Server CI step runs +*keyless* (`MEFOR_STORE_ENCRYPTION_KEY` appears nowhere in the seven SS steps or the five PG steps of +`.github/workflows/ci.yml`). So the `SELECT ... FROM ` loop below that early return **never +executes on any CI leg**. A `(table, column)` pair naming a dropped table is therefore invisible to the +entire test suite, right up until a *keyed* production open crash-loops on `Invalid object name`. + +That is not hypothetical: ASVS 14.2.7 removes the legacy `outbox` table from `_SCHEMA`, and the pair +`("outbox", "payload")` sits ~1200 lines away in a different method. Removing one and forgetting the +other is a startup crash-loop in production and a green suite in CI. + +So this test does what CI's runtime cannot: it reads the source. No database, no driver, no key — it +runs on the plain leg, which is the only leg guaranteed to exist. + +Deliberately checking the sweep passes against the module's OWN `CREATE TABLE` set rather than against +a live database: a live check would need the SS leg (which is keyless, see above) and would pass on a +database whose schema had drifted from the code that ships. +""" + +from __future__ import annotations + +import ast +import pathlib +import re + +import pytest + +_SQLSERVER = ( + pathlib.Path(__file__).resolve().parents[1] / "messagefoundry" / "store" / "sqlserver.py" +) + +#: The methods whose (table, column) literals must resolve. Both walk ciphered cells; both are reached +#: only on a KEYED handle, which is exactly why neither is exercised in CI. +_SWEEP_FUNCTIONS = ("_encrypt_existing_rows", "reencrypt_to_active") + +#: `CREATE TABLE [dbo].[name]` / `CREATE TABLE name` — the schema this module actually deploys. +_CREATE_TABLE = re.compile(r"CREATE TABLE\s+(?:\[?dbo\]?\.)?\[?([A-Za-z_][A-Za-z0-9_]*)\]?") + + +def _created_tables(source: str) -> set[str]: + return set(_CREATE_TABLE.findall(source)) + + +def _swept_pairs(source: str) -> list[tuple[str, str, int]]: + """Every ``(table, column)`` literal driving a sweep loop, with its line. + + Anchored on the LOOP (``for table, in (...)``), not on "any 2-string tuple in the function". + The naive version over-matched immediately and usefully: both functions also build composite AAD + tuples like ``("body", "detail")`` and ``("event_type", "connection")``, whose first element is a + COLUMN, not a table. Flagging those would have made this guard cry wolf on ~14 legitimate pairs, + and a guard that always fails gets deleted or `xfail`ed — which is how a real detector dies. + + Walks the AST rather than grepping, so a pair split across lines or carrying a trailing comment is + still found: the `("users", "totp_secret")` entry is wrapped across three lines in the real source, + and a line-based scan misses it (asserted below). + """ + tree = ast.parse(source) + found: list[tuple[str, str, int]] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.AsyncFunctionDef | ast.FunctionDef): + continue + if node.name not in _SWEEP_FUNCTIONS: + continue + for inner in ast.walk(node): + if not isinstance(inner, ast.For | ast.AsyncFor): + continue + target = inner.target + # `for table, column in (...)` / `for table, ncol in (...)` — the first bound name must be + # `table`, which is what makes element[0] a table name rather than a column. + if not isinstance(target, ast.Tuple) or not target.elts: + continue + first_name = target.elts[0] + if not isinstance(first_name, ast.Name) or first_name.id != "table": + continue + if not isinstance(inner.iter, ast.Tuple | ast.List): + continue + for element in inner.iter.elts: + if not isinstance(element, ast.Tuple) or not element.elts: + continue + table = element.elts[0] + column = element.elts[1] if len(element.elts) > 1 else None + if isinstance(table, ast.Constant) and isinstance(table.value, str): + col = column.value if isinstance(column, ast.Constant) else "?" + found.append((table.value, str(col), element.lineno)) + return found + + +def test_every_swept_table_is_created_by_this_module() -> None: + """A ciphered-cell sweep may only name a table `_SCHEMA` creates. + + Mutation: add `("nonexistent_table", "x")` to `_encrypt_existing_rows` — reds here, on the plain + leg, with no database. After the legacy `outbox` table is dropped, re-adding `("outbox", "payload")` + reds here too, which is the entire reason this file exists. + """ + source = _SQLSERVER.read_text(encoding="utf-8") + created = _created_tables(source) + pairs = _swept_pairs(source) + + # Liveness receipt. A parse that silently found nothing would make the assertion below pass + # vacuously, which is precisely the failure mode this guard is meant to prevent elsewhere. + assert created, "no CREATE TABLE statements found — the regex or the module shape changed" + assert pairs, ( + f"no (table, column) literals found in {_SWEEP_FUNCTIONS} — either the functions were renamed " + f"or the AST walk is broken, and this guard is asserting nothing" + ) + + unknown = sorted({(t, c, ln) for t, c, ln in pairs if t not in created}) + assert not unknown, ( + "a ciphered-cell sweep names a table this module does not CREATE. On a KEYED open this is " + "'Invalid object name' at startup — a crash-loop — and no CI leg catches it, because every " + "SQL Server CI step is keyless and returns before the sweep.\n" + + "\n".join( + f" sqlserver.py:{ln} ({t!r}, {c!r}) <- table {t!r} is never created" + for t, c, ln in unknown + ) + + f"\n\ncreated tables: {', '.join(sorted(created))}" + ) + + +def test_the_guard_can_actually_see_an_unknown_table() -> None: + """Prove the detector fires, rather than trusting that it would. + + The test above passing tells you nothing on its own — it passes identically if `_swept_pairs` + returns garbage or `_created_tables` over-matches. Drive the same functions over a synthetic module + containing a known-bad pair and assert it is found. + """ + synthetic = """ +_SCHEMA = ["IF OBJECT_ID('queue','U') IS NULL CREATE TABLE queue (id INT)"] + +class S: + async def _encrypt_existing_rows(self) -> None: + for table, column in (("queue", "payload"), ("outbox", "payload")): + pass +""" + created = _created_tables(synthetic) + pairs = _swept_pairs(synthetic) + assert created == {"queue"}, created + assert ("outbox", "payload", 6) in [(t, c, ln) for t, c, ln in pairs], pairs + unknown = [t for t, _c, _ln in pairs if t not in created] + assert unknown == ["outbox"], f"the detector did not flag the unknown table: {unknown}" + + +@pytest.mark.parametrize("wrapped_across_lines", [True, False]) +def test_the_ast_walk_finds_pairs_a_line_scan_would_miss(wrapped_across_lines: bool) -> None: + """The real source wraps `("users", "totp_secret")` across three lines with a trailing comment. + + A grep-based implementation misses it, silently shrinking the checked set — a guard that examines + less than it claims to. Assert both spellings are found so the walk cannot regress to line-based. + """ + pair = ( + '(\n "users",\n "totp_secret",\n ), # a comment' + if wrapped_across_lines + else '("users", "totp_secret"),' + ) + synthetic = f""" +class S: + async def _encrypt_existing_rows(self) -> None: + for table, column in ( + {pair} + ): + pass +""" + pairs = [(t, c) for t, c, _ln in _swept_pairs(synthetic)] + assert ("users", "totp_secret") in pairs, f"wrapped={wrapped_across_lines}: {pairs}" From 0868c4706aaf39a80b95e87f0c322a96116e6659 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 10:50:16 -0500 Subject: [PATCH 2/8] feat(retention): purge orphaned reference snapshots (ASVS 14.2.7 part 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `reference.value` (ADR 0006 versioned lookup snapshots) is PL-2 in PHI.md §2 and can hold patient-keyed rows. It had NO purge path at all: the only thing that ever replaced a snapshot was the next sync's build-new-then-flip, which never comes for a set nobody declares any more. `grep purge_reference` returned zero production hits. Adds purge_reference_snapshots to the Store protocol and all three backends, the [retention].reference_snapshot_days window, and the runner phase. ORPHAN-SCOPED, and the limit is stated rather than glossed: a set that IS still declared is never touched however old its synced_at, because its snapshot is live data the engine serves. So the normal case -- a wired set holding live PHI -- is still purged by nothing. That is an honest residual, not a closed cell, and the classification table must not describe this window as covering reference.value generally (which would machine-bless a false claim). Three defences, each from an adversarial pass and each mutation-proven: 1. EMPTY `declared` RAISES, in all three stores AND at the runner call site. `declared` is the keep-set, so an empty one reads as "every set is abandoned" and would purge the whole store. `registry is None` does not cover it: a registry that LOADS FINE while declaring zero reference sets -- a subset --config, a per-team split, a harness redirect aimed at the real DB -- yields references == {}. Absence-based guards fail open, so this is positive-signal. Belt AND braces because one refactor dropping the runner check costs every snapshot in the store. Worse without it: ReferenceSyncRunner deliberately does NOT advance synced_at when a source fetch fails, so the rows that look most stale belong to a still-wired set whose source is merely DOWN. 2. ELIGIBILITY RE-ASSERTED INSIDE THE DELETE, on all three backends including SQLite. One analysis said SQLite "happens to be safe" via self._lock; its own adjudication corrected that and is right -- `declared` is computed in the RUNNER, outside any store lock, so the config-reload race is at the caller level everywhere. Do not simplify the EXISTS away because the SELECT already filtered. 3. THE POINTER SURVIVES BUT ITS VERSION IS BUMPED. Both analyses proposed bolting a removal arm onto converge_reference_cache in each server backend. Reading it gives a better fix: converge only reloads a set whose active version DIFFERS from the one a handle reflects. So leaving the version alone means a follower's populated cache is never revisited -- it serves purged PHI from RAM until restart -- and deleting the pointer is WORSE, because converge only ever adds/updates names present in a fresh read and would never notice. Bumping to 'purged:' with row_count=0 routes the deletion through the mechanism that already exists, on both server backends, with no second removal path to keep in sync. Idempotent via `WHERE version NOT LIKE 'purged:%'`. Four mutations killed: M1 drop the `not in declared` filter -> a DECLARED set loses its rows M2 empty-declared raise -> return 0 -> the wipe path becomes a silent no-op M3 drop the version bump -> followers never converge M4 drop the EXISTS re-assert -> a concurrent re-sync is destroyed M4 SURVIVED THE FIRST ATTEMPT, and the reason is worth recording because it is the exact defect class this change set exists to prevent. The original test re-synced BEFORE calling the purge, so the internal SELECT already filtered the set out and the DELETE never ran -- it exercised the candidate filter, not the re-assert, and passed whether or not the control worked. The race only exists BETWEEN the SELECT and the DELETE, so the reload is now injected mid-method by wrapping _db.execute, with `assert fired` so a wrapper that stops matching can never let the test pass vacuously. SQLite keeps no per-set version map (single-node, sole writer, converge is a genuine no-op there) so it evicts its read-through cache instead; the server backends evict AFTER the commit, so a rolled-back purge cannot leave the cache claiming rows the store still holds. Expected reds until the docs commit: test_retired_false_claims_do_not_reappear and test_purge_surface_is_defined_on_every_backend_and_documented_per_backend. The second is NEWLY ARMED by this commit -- adding the method to the protocol makes the tree demand its PHI.md §8 row, which is the forcing function working. ruff + mypy(strict, 257 modules) clean; 219 passed across the touched suites. --- messagefoundry/config/settings.py | 13 ++ messagefoundry/pipeline/retention.py | 50 ++++++++ messagefoundry/store/base.py | 41 ++++++- messagefoundry/store/postgres.py | 53 ++++++++- messagefoundry/store/sqlserver.py | 70 ++++++++++- messagefoundry/store/store.py | 70 ++++++++++- tests/test_reference_sets.py | 171 +++++++++++++++++++++++++++ 7 files changed, 464 insertions(+), 4 deletions(-) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 7cee18a4..0fc3bf01 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -1509,6 +1509,18 @@ class RetentionSettings(_Section): # in-memory state cache + table bounded. A simple global age purge; per-namespace policy is a # follow-up. 0 = keep forever (the default — state correlation data is opt-in to purge). state_max_age_days: int = 0 + # Past N days, DELETE ORPHANED reference snapshots (ADR 0006) — the rows of a set that config no + # longer declares, whose active version was synced before the cutoff. `reference.value` is PL-2 + # (PHI.md §2) and a snapshot row can be patient-keyed, but until ASVS 14.2.7 it had NO purge path at + # all: the only thing that ever replaced a snapshot was the next sync's build-new-then-flip, which + # never comes for a set nobody declares any more. 0 = keep forever. + # + # ORPHAN-SCOPED, and the limit is the honest part: a set that IS still declared is never touched + # however old its `synced_at`, because its snapshot is live data the engine serves. So the normal + # case — a wired set holding live PHI — remains purged by nothing. That is a stated residual, not a + # closed cell; do not let a classification table describe this window as covering `reference.value` + # generally, which would machine-bless a false claim. + reference_snapshot_days: int = 0 # Past N HOURS, DELETE connection_event rows (#46) — the Corepoint-style transport/lifecycle log can # be high-volume (a connect-per-message sender, a probe storm), so it has its own short window in # HOURS (not days). 0 = inherit the message-body window (messages_days), the ADR 0021 §7.5 default. @@ -1591,6 +1603,7 @@ class RetentionSettings(_Section): "app_log_days", "app_log_compress_days", "search_preset_days", + "reference_snapshot_days", ) @classmethod def _non_negative_days(cls, value: int) -> int: diff --git a/messagefoundry/pipeline/retention.py b/messagefoundry/pipeline/retention.py index c08c834a..37df3b56 100644 --- a/messagefoundry/pipeline/retention.py +++ b/messagefoundry/pipeline/retention.py @@ -177,6 +177,10 @@ class RetentionPass: # the `search_preset_days` window. Metadata only — the count, never a needle. 0 when the window is # unset (the default), so a deployment that doesn't use it has a byte-identical audit detail. search_presets_purged: int = 0 + # Orphaned reference-snapshot ROWS deleted (ADR 0006, ASVS 14.2.7) — sets config no longer + # declares. Metadata only: a count, never a key or a value. 0 when the window is unset, when no + # registry is wired, or when the registry declares no reference sets (the positive-signal skip). + reference_snapshots_purged: int = 0 # This pass hit the `max_pass_seconds` between-phase duration cap (#121, ADR 0137) and SKIPPED its # remaining phases — the skipped tail (incl. any due WAL-checkpoint/VACUUM) re-runs next interval with # its last-run marker unadvanced. Always False when the cap is off (`max_pass_seconds<=0`, the @@ -201,6 +205,7 @@ def did_work(self) -> bool: or self.app_logs_deleted > 0 or self.app_logs_compressed > 0 or self.search_presets_purged > 0 + or self.reference_snapshots_purged > 0 or self.vacuumed or self.over_limit or self.capped @@ -266,6 +271,7 @@ def enabled(self) -> bool: or s.state_max_age_days or s.connection_event_retention_hours or s.search_preset_days + or s.reference_snapshot_days or s.max_db_mb or s.wal_checkpoint_seconds or s.vacuum_time() is not None @@ -503,6 +509,43 @@ def _deadline_hit() -> bool: older_than=now - s.search_preset_days * _SECONDS_PER_DAY, now=now ) + # Orphaned reference snapshots (ADR 0006, ASVS 14.2.7): `reference.value` is PL-2 and had NO + # purge path at all — a set dropped from config kept its decryptable rows forever, because the + # only thing that ever replaced them was the next sync's build-new-then-flip, which never comes + # for a set nobody declares. + # + # THE GUARD IS POSITIVE-SIGNAL, and that is the whole design. `declared` is the keep-set, so an + # EMPTY one reads as "every set is abandoned" and would purge the entire store. `registry is + # None` does not cover it: a registry that LOADS FINE while declaring zero reference sets — a + # subset `--config`, a per-team config split, a harness-redirect run pointed at the real DB — + # yields `references == {}`. Absence-based guards fail open by construction, so this one + # requires a POSITIVE signal: at least one declared set, or the phase does not run at all. A + # registry declaring zero reference sets can never authorize purging any. + # + # The store ALSO raises on an empty `declared` rather than trusting this check — belt and + # braces, because the cost of one refactor dropping this line is every snapshot in the store. + # + # Worse without it: ReferenceSyncRunner deliberately does NOT advance `synced_at` when a source + # fetch fails, so the rows most likely to look "stale" belong to a still-wired set whose source + # is merely down — precisely the last-good copy an operator would want kept. + reference_snapshots_purged = 0 + if not _deadline_hit() and s.reference_snapshot_days > 0: + registry = self._registry_source() if self._registry_source is not None else None + declared = frozenset(registry.references) if registry is not None else frozenset() + if declared: + reference_snapshots_purged = await self._store.purge_reference_snapshots( + older_than=now - s.reference_snapshot_days * _SECONDS_PER_DAY, + declared=declared, + now=now, + ) + else: + log.warning( + "reference-snapshot retention skipped: the loaded registry declares no reference " + "sets, so there is no keep-set and a purge would delete every snapshot in the " + "store. Set [retention].reference_snapshot_days=0 to silence this, or check that " + "--config points at the configuration this store belongs to." + ) + # Application log-file retention (#120): delete app-log FILES older than `app_log_days` from # `[logging].log_dir`. Filesystem I/O (scandir/stat/unlink is blocking), so it runs off the # event loop; metadata only (mtime) — file content is never read (no PHI). A no-op unless the @@ -570,6 +613,7 @@ def _deadline_hit() -> bool: app_logs_compressed=app_logs_compressed, app_log_bytes_reclaimed=app_log_bytes_reclaimed, search_presets_purged=search_presets_purged, + reference_snapshots_purged=reference_snapshots_purged, capped=capped, ) if result.did_work: @@ -979,6 +1023,12 @@ async def _audit(self, result: RetentionPass) -> None: # only — a preset's criteria is a PHI-shaped needle and never enters the audit detail. "search_preset_days": self._settings.search_preset_days, "search_presets_purged": result.search_presets_purged, + # Orphaned reference-snapshot ROWS deleted (ADR 0006, ASVS 14.2.7) + the window. The + # COUNT only — never a set name, key or value. A set name is operator-authored config + # rather than PHI, but the ROWS are PL-2 and naming the set in a durable audit row would + # narrow which patient cohort was dropped; the count is what an assessor needs. + "reference_snapshot_days": self._settings.reference_snapshot_days, + "reference_snapshots_purged": result.reference_snapshots_purged, # Between-phase duration cap (#121, ADR 0137): the configured ceiling + whether THIS pass # hit it and skipped its remaining phases. Timing metadata only — no PHI. "max_pass_seconds": self._settings.max_pass_seconds, diff --git a/messagefoundry/store/base.py b/messagefoundry/store/base.py index 108727dd..96eeaa62 100644 --- a/messagefoundry/store/base.py +++ b/messagefoundry/store/base.py @@ -25,7 +25,7 @@ import asyncio import logging -from collections.abc import AsyncIterator, Iterable, Mapping, Sequence +from collections.abc import AsyncIterator, Collection, Iterable, Mapping, Sequence from pathlib import Path from typing import Any, Protocol, runtime_checkable @@ -1254,6 +1254,45 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N unless ``[retention].search_preset_days`` is set.""" ... + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + """Delete ORPHANED reference snapshots — sets no longer declared in config — synced before + ``older_than``. Returns the number of ``reference`` rows deleted. + + ``reference.value`` is a versioned lookup snapshot (ADR 0006) and can hold patient-keyed rows, + so PHI.md §2 classifies it **PL-2**. Before ASVS 14.2.7 it had **no purge path at all**: a set + dropped from config left its fully-decryptable snapshot in the store indefinitely, replaced only + by the next sync's build-new-then-flip — which never comes for a set nobody declares any more. + + **Orphan-scoped by design, and that limit must be stated rather than glossed.** A set that IS + declared is never touched however old its ``synced_at``, because its snapshot is live data the + engine serves. So the normal case — a wired set holding live PHI — is still purged by nothing. + That is an honest residual, not a closed cell; do not let a classification table describe this as + `rides `, which would machine-bless a false claim. + + **``declared`` must be non-empty and implementations MUST reject an empty one.** An empty + collection reads as "every set is abandoned", so a caller that loaded a registry declaring zero + reference sets — a subset ``--config``, a per-team split, a harness redirect pointed at the real + DB — would wipe every snapshot in the store. Absence-based guards fail open by construction, so + this one is positive-signal: an empty ``declared`` is a programming error, not an instruction. + Worse, ``ReferenceSyncRunner`` deliberately does **not** advance ``synced_at`` when a source + fetch fails, so the victim would be precisely the last-good snapshot of a still-wired set whose + source is merely down. + + **Eligibility must be re-asserted INSIDE the delete statement**, not read first and trusted. The + caller computes ``declared`` outside any store lock, so a config reload can commit a fresh + patient-keyed snapshot between the decision and the delete; a purge that then fires deletes live + data and the set goes present-but-empty, returning ``None`` from ``reference(name).get(k)`` + silently. Do not assume any backend's own locking closes this — the race is at the caller level + on all three. + + **The ``reference_version`` pointer row SURVIVES.** Deleting it is invisible to + :meth:`converge_reference_cache`, which only adds/updates names present in a fresh read, so a + cluster follower would keep serving the purged PHI from RAM forever. Keeping the pointer costs a + row and makes the set read as present-but-empty; that trade is deliberate and recorded.""" + ... + async def wal_checkpoint(self) -> None: ... async def vacuum(self) -> None: ... diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index 36713838..c29fe747 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -55,7 +55,7 @@ import os import socket import time -from collections.abc import AsyncIterator, Iterable, Mapping, Sequence +from collections.abc import AsyncIterator, Collection, Iterable, Mapping, Sequence from contextlib import asynccontextmanager from time import perf_counter from types import MappingProxyType @@ -6482,6 +6482,57 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N ) return _rowcount(result) + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + # See Store.purge_reference_snapshots for the full contract. ADR 0006 `reference.value` is PL-2 + # and had no purge path at all before ASVS 14.2.7. Orphan-scoped: a DECLARED set is never + # touched however old its synced_at. + if not declared: + # Positive-signal guard — an empty `declared` reads as "every set is abandoned" and would + # wipe the store. Never a legitimate instruction, so it raises rather than deleting. + raise ValueError( + "purge_reference_snapshots requires a non-empty `declared` set; an empty one would " + "purge every snapshot in the store. A registry declaring zero reference sets cannot " + "authorize purging any — the caller must skip the phase instead." + ) + rows = await self._pool.fetch( + "SELECT name FROM reference_version WHERE synced_at < $1", older_than + ) + candidates = [r["name"] for r in rows if r["name"] not in declared] + deleted = 0 + for name in candidates: + # Eligibility RE-ASSERTED inside the delete. `declared` is computed by the caller outside + # any store lock, so a config reload can land a fresh patient-keyed snapshot between the + # SELECT above and this statement. Postgres has no equivalent of SQLite's process-wide + # `self._lock`, so this predicate is the ONLY thing standing between a routine reload and + # deleting live data — do not "simplify" it away because the SELECT already filtered. + result = await self._pool.execute( + "DELETE FROM reference WHERE name = $1" + " AND EXISTS (SELECT 1 FROM reference_version v" + " WHERE v.name = $1 AND v.synced_at < $2)", + name, + older_than, + ) + count = _rowcount(result) + if not count: + continue # a concurrent re-sync won the race — leave it alone + deleted += count + # KEEP the pointer, BUMP the version. converge_reference_cache only reloads a set whose + # active version DIFFERS from the one this handle reflects, so leaving the version alone + # would let every follower keep serving the purged PHI out of `_reference_cache` until the + # process restarts — and deleting the pointer outright is worse, because converge only ever + # adds/updates names present in a fresh read and would never notice. Bumping routes the + # deletion through the convergence path that already exists. + await self._pool.execute( + "UPDATE reference_version SET row_count = 0, version = 'purged:' || version" + " WHERE name = $1 AND version NOT LIKE 'purged:%'", + name, + ) + self._reference_cache.pop(name, None) + self._reference_versions.pop(name, None) + return deleted + async def purge_dead_letters( self, *, diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 03cca7c7..609237f7 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -38,7 +38,15 @@ import logging import queue import time -from collections.abc import AsyncIterator, Callable, Iterable, Iterator, Mapping, Sequence +from collections.abc import ( + AsyncIterator, + Callable, + Collection, + Iterable, + Iterator, + Mapping, + Sequence, +) from contextlib import AsyncExitStack, asynccontextmanager, contextmanager from time import perf_counter from types import MappingProxyType @@ -5731,6 +5739,66 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N raise return int(purged) if purged is not None else 0 + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + # See Store.purge_reference_snapshots for the full contract. ADR 0006 `reference.value` is PL-2 + # and had no purge path at all before ASVS 14.2.7. Orphan-scoped: a DECLARED set is never + # touched however old its synced_at. + if not declared: + # Positive-signal guard — an empty `declared` reads as "every set is abandoned" and would + # wipe the store. Never a legitimate instruction, so it raises rather than deleting. + raise ValueError( + "purge_reference_snapshots requires a non-empty `declared` set; an empty one would " + "purge every snapshot in the store. A registry declaring zero reference sets cannot " + "authorize purging any — the caller must skip the phase instead." + ) + deleted = 0 + purged_names: list[str] = [] + async with self._acquire() as conn, self._cursor(conn) as cur: + try: + await cur.execute( + "SELECT name FROM reference_version WHERE synced_at < ?", (older_than,) + ) + candidates = [r[0] for r in await cur.fetchall() if r[0] not in declared] + for name in candidates: + # Eligibility RE-ASSERTED inside the delete. `declared` is computed by the caller + # outside any lock and write_reference_snapshot takes no applock here, so a config + # reload can commit a fresh patient-keyed snapshot between the SELECT above and + # this statement. This predicate is the only thing preventing a routine reload from + # destroying live data. `older_than` is bound TWICE (T-SQL has no named params). + await cur.execute( + "DELETE FROM reference WHERE name = ?" + " AND EXISTS (SELECT 1 FROM reference_version v" + " WHERE v.name = ? AND v.synced_at < ?)", + (name, name, older_than), + ) + count = int(cur.rowcount or 0) + if not count: + continue # a concurrent re-sync won the race — leave it alone + deleted += count + # KEEP the pointer, BUMP the version — converge_reference_cache only reloads a set + # whose active version DIFFERS, so an unchanged version leaves every follower + # serving purged PHI from `_reference_cache` until restart, and deleting the + # pointer is worse (converge only adds/updates names present in a fresh read). + await cur.execute( + "UPDATE reference_version SET row_count = 0," + " version = 'purged:' + version" + " WHERE name = ? AND version NOT LIKE 'purged:%'", + (name,), + ) + purged_names.append(name) + await self._commit(conn) + except Exception: + await conn.rollback() + raise + # Evict only AFTER the commit — a rolled-back purge must not leave the cache claiming rows the + # store still holds (the same post-commit discipline purge_state follows below). + for name in purged_names: + self._reference_cache.pop(name, None) + self._reference_versions.pop(name, None) + return deleted + async def purge_state(self, *, older_than: float, now: float | None = None) -> int: """Delete transform-state rows last set before ``older_than`` (ADR 0005 retention), evicting them from the read-through cache post-commit. Returns the number deleted.""" diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index b76d409a..773c6747 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -44,7 +44,15 @@ import stat import subprocess import time -from collections.abc import AsyncIterator, Awaitable, Callable, Iterable, Mapping, Sequence +from collections.abc import ( + AsyncIterator, + Awaitable, + Callable, + Collection, + Iterable, + Mapping, + Sequence, +) from contextlib import asynccontextmanager from dataclasses import dataclass, field from enum import Enum @@ -8554,6 +8562,66 @@ async def purge_search_presets(self, *, older_than: float, now: float | None = N await self._commit() return int(cur.rowcount) + async def purge_reference_snapshots( + self, *, older_than: float, declared: Collection[str], now: float | None = None + ) -> int: + # ADR 0006 reference snapshots: `value` is PL-2 (PHI.md §2) and until ASVS 14.2.7 had NO purge + # path at all — a set dropped from config kept its decryptable rows forever, because the only + # thing that ever replaced them was the next sync's build-new-then-flip, which never comes. + # + # Orphan-scoped: a DECLARED set is never touched however old its synced_at. Scoping it any wider + # would delete live data the engine is serving. + if not declared: + # Positive-signal guard. An empty `declared` reads as "every set is abandoned", so a caller + # holding a registry that declares zero reference sets — a subset --config, a per-team + # split, a harness redirect aimed at the real DB — would wipe the store. That is never a + # legitimate instruction, so it is an error rather than a very destructive no-op. + raise ValueError( + "purge_reference_snapshots requires a non-empty `declared` set; an empty one would " + "purge every snapshot in the store. A registry declaring zero reference sets cannot " + "authorize purging any — the caller must skip the phase instead." + ) + async with self._lock: + cur = await self._db.execute( + "SELECT name FROM reference_version WHERE synced_at < ?", (older_than,) + ) + candidates = [r[0] for r in await cur.fetchall() if r[0] not in declared] + deleted = 0 + for name in candidates: + # The eligibility predicate is RE-ASSERTED inside the delete, not read above and + # trusted. `declared` is computed by the caller outside this lock, so a config reload + # can commit a fresh patient-keyed snapshot between the SELECT and here; without the + # EXISTS the purge would delete live rows and the set would go present-but-empty, + # returning None from reference(name).get(k) with no error at all. + cur = await self._db.execute( + "DELETE FROM reference WHERE name = ?" + " AND EXISTS (SELECT 1 FROM reference_version v" + " WHERE v.name = ? AND v.synced_at < ?)", + (name, name, older_than), + ) + if not cur.rowcount: + continue # a concurrent re-sync won the race — leave it alone + deleted += int(cur.rowcount) + # KEEP the pointer row, but BUMP its version. Deleting the pointer is invisible to + # converge_reference_cache (it only adds/updates names present in a fresh read), so a + # cluster follower would serve the purged PHI from RAM until restart. Keeping it + # unchanged is just as bad: converge only reloads a set whose version DIFFERS, so an + # unchanged version means the follower's populated cache is never revisited. Bumping it + # makes the deletion propagate through the mechanism that already exists, rather than + # bolting a second removal path onto every backend. + await self._db.execute( + "UPDATE reference_version SET row_count = 0, version = 'purged:' || version" + " WHERE name = ? AND version NOT LIKE 'purged:%'", + (name,), + ) + # SQLite keeps no per-set version map (it is single-node and the sole writer, which is + # why converge_reference_cache is a no-op here) — evicting the read-through cache is + # the whole of the local convergence story. The server backends, which DO track + # versions, rely on the bumped pointer above. + self._reference_cache.pop(name, None) + await self._commit() + return deleted + async def purge_dead_letters( self, *, diff --git a/tests/test_reference_sets.py b/tests/test_reference_sets.py index 44175855..23cc9485 100644 --- a/tests/test_reference_sets.py +++ b/tests/test_reference_sets.py @@ -818,3 +818,174 @@ async def test_genuine_source_failure_still_retries_and_keeps_last_good( assert not [r for r in caplog.records if r.levelname == "ERROR"] # WARNING, not ERROR finally: await store.close() + + +# --- ASVS 14.2.7: purge_reference_snapshots (orphan-scoped) ------------------ +# +# `reference.value` is PL-2 (PHI.md §2) and until 14.2.7 had NO purge path at all: a set dropped from +# config kept its decryptable rows forever, because the only thing that ever replaced a snapshot was +# the next sync's build-new-then-flip — which never comes for a set nobody declares. + + +async def _seed(store: MessageStore, name: str, *, synced_at: float, rows: dict[str, Any]) -> None: + """Write a snapshot and backdate its active-version pointer, so age is controllable.""" + await store.write_reference_snapshot(name=name, version="v1", rows=rows) + await store._db.execute( + "UPDATE reference_version SET synced_at = ? WHERE name = ?", (synced_at, name) + ) + await store._commit() + + +async def test_purge_deletes_only_undeclared_sets(tmp_path: Path) -> None: + """The keep-set is `declared`; age alone is never sufficient. + + Mutation: drop the `r[0] not in declared` filter — reds, because the still-wired set loses its rows. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "wired", synced_at=0.0, rows={"A": "1"}) # ancient BUT still declared + await _seed(store, "orphan", synced_at=0.0, rows={"B": "2"}) + deleted = await store.purge_reference_snapshots(older_than=1000.0, declared={"wired"}) + assert deleted == 1 + view = store.reference_view() + assert view["wired"] == {"A": "1"}, "a DECLARED set must survive however old it is" + assert view.get("orphan") in (None, {}), "the orphan's rows must be gone" + finally: + await store.close() + + +async def test_purge_leaves_a_recent_orphan_alone(tmp_path: Path) -> None: + """Undeclared but NEWER than the cutoff — the window still applies to orphans. + + Without this, `declared` alone would drive deletion and the age window would be decorative. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=5000.0, rows={"B": "2"}) + deleted = await store.purge_reference_snapshots(older_than=1000.0, declared={"other"}) + assert deleted == 0 + assert store.reference_view()["orphan"] == {"B": "2"} + finally: + await store.close() + + +async def test_purge_refuses_an_empty_declared_set(tmp_path: Path) -> None: + """An empty keep-set reads as "every set is abandoned" and would wipe the store. + + `registry is None` does not cover it: a registry that LOADS FINE while declaring zero reference + sets — a subset --config, a per-team split, a harness redirect aimed at the real DB — yields + `references == {}`. Absence-based guards fail open, so this one is positive-signal. + + Mutation: replace the raise with `return 0` — this reds, and so does the row-count assertion. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "a", synced_at=0.0, rows={"k": "1"}) + with pytest.raises(ValueError, match="non-empty"): + await store.purge_reference_snapshots(older_than=1000.0, declared=frozenset()) + assert store.reference_view()["a"] == {"k": "1"}, ( + "nothing may be deleted on the refusal path" + ) + finally: + await store.close() + + +async def test_purge_bumps_the_version_so_a_follower_converges(tmp_path: Path) -> None: + """The pointer row SURVIVES but its version CHANGES — and that is what carries the deletion. + + `converge_reference_cache` only reloads a set whose active version DIFFERS from the one a handle + reflects. Leave the version alone and every cluster follower keeps serving the purged PHI out of + RAM until restart; delete the pointer instead and converge never notices, because it only + adds/updates names present in a fresh read. Bumping routes the deletion through the mechanism that + already exists. + + Mutation: drop the UPDATE — `version` stays 'v1' and this reds. That is the single-handle proxy for + the cluster bug; the real two-handle proof lives in the Postgres/SQL Server suites, because SQLite + is single-node and its converge is legitimately a no-op. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=0.0, rows={"B": "2"}) + await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + row = await ( + await store._db.execute( + "SELECT version, row_count FROM reference_version WHERE name = 'orphan'" + ) + ).fetchone() + assert row is not None, ( + "the pointer row must SURVIVE — deleting it is invisible to converge" + ) + assert row["version"] == "purged:v1", "the version must change or a follower never reloads" + assert row["row_count"] == 0 + finally: + await store.close() + + +async def test_purge_is_idempotent_and_does_not_double_prefix(tmp_path: Path) -> None: + """A second pass over an already-purged set must be a no-op, not a churn of 'purged:purged:v1'.""" + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=0.0, rows={"B": "2"}) + first = await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + second = await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + assert (first, second) == (1, 0) + row = await ( + await store._db.execute("SELECT version FROM reference_version WHERE name = 'orphan'") + ).fetchone() + assert row["version"] == "purged:v1" + finally: + await store.close() + + +async def test_a_concurrent_resync_between_decision_and_delete_is_not_destroyed( + tmp_path: Path, +) -> None: + """The TOCTOU the eligibility re-assert exists for — the race landed WHERE IT ACTUALLY IS. + + `declared` is computed by the RUNNER outside any store lock, and inside the store the eligibility + SELECT and the DELETE are separate statements. The dangerous window is BETWEEN them: a config + reload commits a fresh patient-keyed snapshot after the set has been judged eligible but before its + rows are removed. + + THE FIRST VERSION OF THIS TEST WAS WORTHLESS and is worth recording. It re-synced *before* calling + the purge, so the internal SELECT already filtered the set out and the DELETE never ran — removing + the `EXISTS` re-assert changed nothing and the mutation SURVIVED. A test that exercises the + candidate filter cannot prove the re-assert. So the re-sync is injected mid-method here, which is + the only place the race exists. + + Mutation: drop `AND EXISTS (... synced_at < ?)` from the DELETE — the fresh rows are destroyed and + this reds. Verified killed. + """ + store = await MessageStore.open(tmp_path / "r.db") + try: + await _seed(store, "orphan", synced_at=0.0, rows={"OLD": "x"}) + + real_execute = store._db.execute + fired = False + + async def racing_execute(sql: str, params: Any = None): # type: ignore[no-untyped-def] + nonlocal fired + if not fired and sql.lstrip().upper().startswith("DELETE FROM REFERENCE"): + # The reload lands HERE: after eligibility was decided, before the rows are removed. + fired = True + await real_execute( + "UPDATE reference_version SET synced_at = ? WHERE name = ?", (9999.0, "orphan") + ) + return ( + await real_execute(sql, params) if params is not None else await real_execute(sql) + ) + + store._db.execute = racing_execute # type: ignore[method-assign] + try: + deleted = await store.purge_reference_snapshots(older_than=1000.0, declared={"keep"}) + finally: + store._db.execute = real_execute # type: ignore[method-assign] + + assert fired, "the race never fired — the test proved nothing about the re-assert" + assert deleted == 0, "a set re-synced mid-purge must survive the DELETE" + rows = await ( + await store._db.execute("SELECT key FROM reference WHERE name = 'orphan'") + ).fetchall() + assert [r["key"] for r in rows] == ["OLD"], "the freshly-eligible rows were destroyed" + finally: + await store.close() From df13c01a2d1650cee847790f1d372d47939e170e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 10:53:38 -0500 Subject: [PATCH 3/8] =?UTF-8?q?docs(phi):=20reference=20snapshots=20gain?= =?UTF-8?q?=20a=20bounded=20window=20=E2=80=94=20and=20the=20limit=20is=20?= =?UTF-8?q?stated=20(14.2.7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clears two of the four forcing functions landed in Commits 1-2. `test_purge_surface_is_defined_on_every_backend_and_documented_per_backend` was armed by Commit 2 the moment purge_reference_snapshots joined the Store protocol: the tree DEMANDED a §8 row rather than trusting anyone to remember one. That is the guards-first ordering doing its job. Added, naming the mechanism, the re-assert, and the pointer-survives-with-a-bumped-version behaviour that is what actually makes a cluster follower converge. Also retires the registered false claim at §2's reference.name/version/key row ("**No purge path at all**") and rewrites §2's reference.value row. THE ORPHAN-ONLY LIMIT IS WRITTEN INTO EVERY PLACE THE WINDOW IS NAMED, on purpose. purge_reference_snapshots bounds a set config has DROPPED; a set still declared is never purged whatever its age, because its snapshot is live data the engine serves. So the normal case -- a wired set holding live PHI -- remains bounded by nothing. An adversarial pass flagged that describing this as a plain `rides ` entry would machine-bless a FALSE claim and, worse, make the true statement unwritable once the retired-claim string was registered. So §8's honest-gaps table KEEPS a reference.value row rather than losing it: the row now scopes the gap to the declared-set case instead of deleting it. Two registered claims deliberately survive this commit, both about `outbox.payload` (docs/PHI.md:401 and :1051). They are Commit 6's to clear when the legacy SQL Server table is migrated and dropped. Registering all three in Commit 1 -- before any edit -- is what made the guard's RED output the work list; the count going 3 -> 2 here is that mechanism reporting progress. --- docs/PHI.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/PHI.md b/docs/PHI.md index 5b3b3660..ea499768 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -108,7 +108,7 @@ destruction) are documented in [§3](#3-encryption-at-rest) under the matching h | `message_events.detail` | all three | **Possibly** — per-message disposition detail | **Yes, when a key is set** — store cipher; AAD `("message_events","detail",message_id,ts,event)`; store DEK | **PL-2** | `id` is AUTOINCREMENT/IDENTITY and unknown at INSERT, so the AAD binds the natural tuple and the column rides its own composite migration/rotation pass. `safe_text()`-scrubbed before write | | `response.detail`, `response.resp_headers` | all three | **Possibly** — reply diagnostics / partner response headers | **Yes, when a key is set** — store cipher; AAD `("response","detail", …)` / `("response","resp_headers",message_id,destination_name,response_seq)`; store DEK | **PL-2** | `detail` is `safe_text()`-scrubbed and 200-char bounded before the cipher | | `state.value` (ADR 0005 transform state) | all three | **Possibly** — a correlation map (e.g. MRN→surrogate) written by a Handler | **Yes, when a key is set** — JSON-encoded then store cipher; AAD `("state","value",namespace,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API** — reachable only from a Handler via `state_get`/`state_set` | -| `reference.value` (ADR 0006 versioned lookup snapshots) | all three | **Possibly** — a snapshot row may be patient-keyed | **Yes, when a key is set** — store cipher; AAD `("reference","value",name,version,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API** and **no retention purge** — a snapshot is replaced only by the next sync's build-new-then-flip | +| `reference.value` (ADR 0006 versioned lookup snapshots) | all three | **Possibly** — a snapshot row may be patient-keyed | **Yes, when a key is set** — store cipher; AAD `("reference","value",name,version,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API.** **`[retention].reference_snapshot_days`** — `purge_reference_snapshots` DELETEs the rows of a set config **no longer declares** whose active version was synced before the cutoff (`0` = keep forever, the default) ([§8](#8-retention--purge)). **Orphan-only, and the limit is the point:** a set that IS still declared is never touched however old its `synced_at`, because its snapshot is live data the engine serves — so the normal case, a wired set holding live PHI, is still bounded by nothing. Do not restate this as a plain window over `reference.value` | | `search_presets.criteria` (ADR 0136 saved Log-Search filters) | all three | **Yes** — the operator's saved `content` / `field_value` needle is PHI-shaped by construction | **Yes, when a key is set** — store cipher; AAD `("search_presets","criteria",id)`; store DEK | **PL-2** | Never returned by the API: `GET /search/presets` lists names + timestamps only; the needle is loaded server-side by `GET /search/layered`. **`[retention].search_preset_days`** — `purge_search_presets` DELETEs the whole row past the window on every backend, keyed on last-**used** — the later of `updated_at` and `last_used_at`, #306 (`0` = keep forever, the default) ([§8](#8-retention--purge)) | | `connection_event.reason` (#46 transport/lifecycle log, **default on**) | all three | **Possibly** — a free-text diagnostic fragment | **Yes, when a key is set** — store cipher; AAD `("connection_event","reason",connection,ts,kind)`; store DEK | **PL-2** | Defended twice: the emit site passes a `safe_exc()`-scrubbed string and the store re-applies `safe_text(reason)[:200]`. IDENTITY `id`, so its own composite pass. Every other column is bounded engine/config metadata **except `peer_host`** (its own PL-4 row below) — the table carries no frame, body or HL7 field value. Read under `monitoring:read`, **not** a PHI permission ([§7](#7-logging--phi-redaction)) | | `connection_event.peer_host` | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The connecting peer's IP, taken from the socket; `NULL` for outbound/unknown. Personal data, not PHI — the **same class and the same decision** as `audit_log.client` / `sessions.client`: plaintext so it stays greppable for incident response. Returned to operators by `GET /events` under `monitoring:read`. Purged with its row by `[retention].connection_event_retention_hours` | @@ -120,7 +120,7 @@ destruction) are documented in [§3](#3-encryption-at-rest) under the matching h | `audit_log.client` (ADR 0150) | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The caller's client address — the "from where" of an audited action; `NULL` for engine-internal/`system` writes. **Personal data, but not PHI**, and exactly what HIPAA §164.312(b) audit controls exist to capture. Plaintext by decision: it must stay greppable/indexable for incident response, it already appears in the clear in `sessions.client`, and it is folded **inside** the tamper-evident hash chain — so it carries **integrity** protection even without confidentiality. Widens a store-file compromise from *who did what* to *who did what from where*; volume encryption + owner-only ACLs on whichever host owns the files — the engine's own `_secure_file` covers the **SQLite** store only ([§10](#10-secure-deployment--operations-checklist)) — are the control | | `delivered_keys` (H2 idempotency ledger) | all three | **No** — hashes + ids only | No (deliberately not ciphered — nothing to protect) | **PL-4** | One row per completed outbound delivery: a SHA-256 `delivery_key` over non-PHI ids + a replay-stable seq, plus `outbox_id`/`message_id`/`destination_name`/`delivery_seq`. **Never a body or any PHI** — `control_id` is only *folded into the hash input*, never stored in the clear here. Lets the FIFO claim skip-and-complete a re-claimed already-delivered head without re-sending | | `state.namespace` / `state.key` | all three | **Possibly** — a Handler that keys correlation state on a raw MRN stores that identifier here in the clear | **No** — plaintext by construction: the pair is the composite primary key **and** the AAD input for `state.value`, so it cannot be ciphered without losing the lookup | **PL-4** | Authors must key state on a **surrogate, never a raw identifier**. Covered only by the whole-DB / volume layer. Rides `[retention].state_max_age_days` with its value | -| `reference.name` / `reference.version` / `reference.key` | all three | **Possibly** — §2's `reference.value` row notes a snapshot row may be patient-keyed; the key column is where that identifier would sit | **No** — plaintext by construction, same reason as `state` | **PL-4** | Same rule: key reference sets on a surrogate. **No purge path at all** — a snapshot is replaced only by the next sync's build-new-then-flip | +| `reference.name` / `reference.version` / `reference.key` | all three | **Possibly** — §2's `reference.value` row notes a snapshot row may be patient-keyed; the key column is where that identifier would sit | **No** — plaintext by construction, same reason as `state` | **PL-4** | Same rule: key reference sets on a surrogate. The key columns ride the same delete: `purge_reference_snapshots` removes whole rows of an **undeclared** set, so these go with the `value` they key. A **declared** set is never touched — same orphan-only limit as `reference.value` above | | `sessions.token_hash` / `client`, `resend_log`, `processed_files`, `pending_approvals.params`, `webauthn_credentials.public_key` | all three | **No** | No (deliberately not ciphered) | **PL-4** | Session tokens are stored as SHA-256 only; `processed_files` holds a hashed derived file key, never a path; approval params carry connection names / channel ids / a config dir by construction; COSE public keys (ADR 0068) are **verification material, not secrets** and are explicitly excluded from the cipher and from rekey | | `secret_rotation_meta` (`secret_key` / `fingerprint` / `tracked_since` / `last_rotated`) | **All three backends** (#1186 — SQLite `MessageStore`, `SqlServerStore` and `PostgresStore` each create the table at open and implement the `SecretRotationMetaStore` protocol) | **No** — non-secret rotation state: a **keyed MAC** (DEK-derived, one-way — never the secret value) + ISO dates only | No (deliberately not ciphered — it is neither PHI nor a secret; the MAC is one-way and un-guessable without the DEK) | **PL-4** | ASVS 13.3.4 rotation watcher (BACKLOG #282). `fingerprint` is a keyed MAC so obtaining the rows leaks no secret. Written on a keyed store; absent on a keyless one | | SQLite DB file + `-wal` / `-shm` / temp files, and every index | SQLite | **Yes** (mirror the above) | **No** — the app cipher cannot reach them | **PL-5** | WAL/shm hold recently-written PHI outside any app-level encryption. Cover: SQLCipher (whole-DB) and/or FDE on the engine host's data volume | @@ -981,6 +981,7 @@ method exists for `Store`-protocol completeness and deliberately does nothing), | `purge_state` (`state.value`) | `[retention].state_max_age_days` | `DELETE` by `set_at` | enforced | **enforced** | **enforced** | | `purge_connection_events` (incl. `connection_event.reason`) | `[retention].connection_event_retention_hours`; 0 inherits `messages_days` | `DELETE` by `ts` (metadata-only) | enforced | **enforced** | **enforced** | | `purge_search_presets` (`search_presets.criteria`) | `[retention].search_preset_days`; `0` = keep forever | `DELETE` by the null-safe greater of `updated_at` / `last_used_at` — whole row (the criteria *is* the payload). Keys on last-**used** (#306); a row predating `last_used_at` (NULL) ages out on `updated_at` alone | enforced | **enforced** | **enforced** | +| `purge_reference_snapshots` (`reference.value` + its key columns) | `[retention].reference_snapshot_days`; `0` = keep forever | `DELETE` of whole rows for a set config **no longer declares** whose `reference_version.synced_at` predates the cutoff — eligibility is re-asserted **inside** the delete (a config reload can commit a fresh snapshot between the decision and the statement). The `reference_version` pointer **survives** with its version bumped to `purged:` and `row_count = 0`, which is what makes a cluster follower converge — `converge_reference_cache` only reloads a set whose version CHANGED, so leaving it would let a follower serve purged PHI from RAM until restart, and deleting the pointer is worse (converge only adds names present in a fresh read). **Orphan-only** — a declared set is never purged | enforced | **enforced** | **enforced** | | `purge_alert_instances` (incl. `alert_instance.reason`) | same window as connection events | `DELETE` by `resolved_at`, **RESOLVED instances only** | enforced | **enforced** | **enforced** | | `strip_embedded_documents` | per-inbound `prune_documents_after` + `prune_documents_min_bytes` (**no global default** — nothing is stripped without an override) | in-place strip of bulky base64 documents; sets `messages.documents_pruned` | enforced | **enforced** | **enforced** | | Streaming-attachment release (`release_message_attachments`) | rides the two body windows | refcount decref + GC at 0, plus a startup `sweep_orphan_attachments` | enforced | **enforced** | **enforced** | @@ -1046,7 +1047,7 @@ window purely so a replay can be richer — is precisely the defect ASVS 14.2.7 | Tier | Status | |---|---| -| `reference.value` | no purge — a snapshot is replaced only by the next sync's build-new-then-flip | +| `reference.value` (a **declared** set) | **orphan-only** coverage. `purge_reference_snapshots` bounds a set config has DROPPED; a set still declared is never purged whatever its age, because its snapshot is live data. The wired-set case remains unbounded — the honest gap | | `outbox.payload` (SQL Server legacy) | recreated on every open, **touched by no purge on any backend** — legacy PHI there is retained forever | | `users.totp_secret` | no window by design — it lives and dies with the user row | | `audit_log`, `delivered_keys`, `resend_log` | keep-forever by design (see below) | From ce06f990ca03e0815e5c3c36e65c60a7cfa8f2aa Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 11:20:44 -0500 Subject: [PATCH 4/8] docs(phi): the anchor that would catch tail-truncation is unreachable from the CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more corrections from the sibling session's hostile review, both verified here before acting. The first sharpens a rewrite I made an hour ago and did not push far enough. 1. I WROTE 'only an out-of-band audit_anchor detects that' -- which implies an operator can reach it. They cannot. Verified: __main__.py:3418 return await store.verify_audit_chain() <- no anchor parser zero --expect-anchor / audit-anchor flags audit_anchor() implemented on all three backends; its ONLY callers are pipeline/dr.py:569,585 and rekey_audit_chain So the machinery exists and the shipped operator command cannot use it: an operator or compliance job running OK: verified 0 audit row(s) gets a CLEAN result after someone truncates the newest audit rows. As shipped, truncation detection depends on the off-box tee (sec-offbox-log #361/#363), not on anything runnable locally. My previous wording was true about the mechanism and misleading about the posture -- the same shape as the inverted argument it was correcting. Now states the gap plainly, with an explicit instruction not to upgrade it to 'the anchor detects it' without also shipping a way to pass one. 2. §5 AND §7 CONTRADICTED EACH OTHER, and §7 was right. §5 claimed 'no PHI is placed in a URL (so it cannot leak into history, bookmarks, or Referer)' while §7 documented, correctly, that '?content=1234567 or ?field_value=MRN12345 survives the whole filter chain unredacted'. The console's own search route takes both as query parameters. Narrowed §5 to what actually holds: no message BODY in a URL, plus Referrer-Policy: no-referrer. Also flagged in the same bullet: messagefoundry_webconsole/_security.py justifies degrading to no-referrer on the grounds that '/ui URLs carry opaque ids only (never PHI)'. The header is still the right call, but its stated REASON is false -- and a compensating control resting on a false premise is worse than an uncommented one, because the next person reasons from the comment. The route fix is filed separately by the sibling session; the comment would have outlived it. Third instance today of two live documents disagreeing while the stale/wrong half is the one a reviewer picks up (after harden_kex_groups across five documents, and the pre-#301 Transit claim). The pattern is worth naming: a repo that states a fact twice will eventually state it two ways, and the wrong copy is the one that gets cited. --- docs/PHI.md | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/PHI.md b/docs/PHI.md index ea499768..63b37017 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -259,9 +259,16 @@ for defense-in-depth without swapping the `aiosqlite` connector. `associated_data`, so **`v3` is cell-bound regardless of `aad_bind`**; and the audit-chain MAC is computed inside Transit. Missing config or an unreachable/unknown Transit key **fails closed** at `open_store` (`serve` refuses to start) — never an in-process fallback. Caveat worth knowing: the - Transit-backed audit MAC is threaded into the **SQLite** store only, so a `vault_transit` + SQL Server - or Postgres deployment runs an **unkeyed SHA-256** audit chain (tamper-evident, not - forgery-resistant). + Transit-backed audit MAC reaches **all three** backends. This bullet previously said SQLite-only — that + was the PRE-#301 state stated as current, and it was wrong in the direction that flatters nothing: + `TransitCipher.audit_mac_key()` returns `None` **by design**, so a server backend given only + `audit_mac_key` had no keying secret at all. #301 threads `audit_mac_fn` alongside it + (`store/base.py:1817`, forwarded at `:1826`/`:1836`), which is what closed it. `docs/ASVS-L2-PHASE0-CHANGES.md` + §"Audit chain" carries the accurate wording — the digest primitive is *"shared verbatim by all three + backends"*. **What IS unkeyed is the KEYLESS posture, not a backend:** with no store key, + `audit_mac_key()` returns `None` (`store/crypto.py:428`) and the chain stays keyless SHA-256 — + tamper-evident against a careless edit, not forgery-resistant against anyone who can write the + table. At-rest encryption is off by default, so that is the DEFAULT posture. 2. **Key management + rotation `[BUILT]`.** The key is a base64 32-byte secret from the **environment** (`MEFOR_STORE_ENCRYPTION_KEY`), never the TOML file — reusing the existing secrets convention (cf. `MEFOR_STORE_PASSWORD`). Mint one with `messagefoundry gen-key`. On Windows it may instead live @@ -476,7 +483,7 @@ header row (`content_type`, `total_bytes`, `refcount`, `created_at`) + the `mess linkage · `secret_rotation_meta` (all three backends) · `.mfbak` on the server backends. Deliberately **not** ciphered, so that ids stay indexable and the audit trail stays greppable for -incident response. Integrity for `audit_log` comes from the **tamper-evident hash chain** (the `client` +incident response. Integrity for `audit_log` comes from the **tamper-evident hash chain** — *keyless SHA-256 in the default keyless posture, upgraded to HMAC-SHA256 on an HKDF-derived subkey of the store DEK only when a store key is set (#190), or an isolated-module Transit MAC under `cipher_provider=vault_transit`* — so its strength is **key-custody-dependent**, and the unqualified reading describes the non-default case (the `client` address is folded *inside* it), not from a cipher. **Access, per tier — several of these ARE returned by an API, under RBAC:** @@ -700,7 +707,7 @@ control unchanged (`messages:view_raw`/`view_summary` RBAC, field-level redactio - **No PHI in browser storage.** The only stored item is the session token, in an **HttpOnly + SameSite= Strict** cookie JS cannot read (`mf_session`). Nothing is written to `localStorage`/`sessionStorage`/ - `IndexedDB`, and no PHI is placed in a URL (so it cannot leak into history, bookmarks, or Referer). + `IndexedDB`. **PHI in URLs — corrected 2026-07-30, and §7 was the accurate half:** this bullet used to claim no PHI is placed in a URL at all. The console's own search route takes the needle as a QUERY PARAMETER — `GET /ui/messages/search?content=…&field_value=…` (`messagefoundry_webconsole/routes/search.py`) — so a search URL can carry an MRN into history, bookmarks and `Referer`. What holds is narrower: no *message body* is ever placed in a URL, and `Referrer-Policy: no-referrer` is set. Note that `_security.py`'s comment justifies that header on the grounds that "/ui URLs carry opaque ids only (never PHI)" — the header is still right, its stated REASON is not. Moving the needle out of the URL is tracked separately (§7). - **No caching.** Every `/ui` HTML response and every PHI JSON read is served `Cache-Control: no-store`, so a browser/proxy never retains a message body on disk. The covered set is the PHI-read route families — `/messages*`, `/dead-letters*`, `/search*`, `/logs*`, `/uploads*` (`_NO_STORE_PREFIXES` in @@ -907,7 +914,7 @@ with materially different PHI profiles, so they get their own rows; stream 4 is | **2. `uvicorn` request/access log** (sub-stream of 1) | one line per HTTP request — method, **full request line including the query string**, status, timing | inherits stream 1's format | inherits stream 1's sink | request tracing, latency and error triage | inherits stream 1's | inherits stream 1's | **Can contain PHI.** `configure_logging` clears uvicorn's own handlers and propagates to the root, so the three filters apply; `serve` passes `log_config=None` and never disables `access_log`, so at the default `INFO` level every request is logged. OIDC `code`/`state` **are** scrubbed. **Not** scrubbed: PHI-shaped search needles on GET routes (`?content=…`, `?field_value=…`) — the single-token residual above | | **3. `messagefoundry.audit` off-box tee** (sub-stream of 1) | one JSON object per **committed** `audit_log` row: `event`/`ts`/`action`/`actor`/`channel_id`/`client`/`detail` | JSON | emitted after the row is durably committed and **outside** the store write lock; rides stream 1's handlers | shipping audit evidence to a SIEM so it survives a host compromise | inherits stream 1's | inherits stream 1's | `detail` is passed through the `safe_text` PHI chokepoint **before** it leaves the process; `client` is forwarded verbatim as a discrete field so a SIEM can index it. Best-effort: a logging failure is caught, never raised into the audit write. **Pinned to `INFO`** — it is emitted even at `[logging].level = WARNING` | | **4. Off-box syslog/SIEM forwarder** — the shared **transport** for 1–3 | a copy of every record from 1–3 | `forward_format`, default **JSON** (independent of the stdout format) | the operator's collector (`forward_host`/`_port`) | off-box evidence retention / SIEM correlation | **default-on when a collector is named.** Transport: `udp` (default) / `tcp` / **`tls`** (RFC 5425, CA-anchored, verified by default). `serve` gates the hop on the shared posture gradient before the handler is installed: verified TLS ungated; otherwise loopback / attested / synthetic ALLOW, non-enforcing PHI WARN, **enforcing PHI REFUSE (exit 2)** | the collector's, not the engine's | the identical three filters are installed on this handler, so the forwarded copy is PHI-redacted — but it still carries usernames, connection names, message ids, client addresses and the audit chain. That is the engine's own stated reason for gating the hop | -| **5. `audit_log` table** (SQLite, Postgres, SQL Server) | who / what / **where-from** / when of auth + PHI *access* and admin actions — plus, when the opt-in `[security].audit_all_authorization_decisions` is on (**default `false`**; the internal field it desugars to is `audit_all_authz`, whose old `[diagnostics]` TOML spelling is **refused at load** — ADR 0118), an `authz` row for **every** authorization decision including successes, which multiplies this stream's volume — `actor`, `action`, `channel_id`, `client`, `detail`, `row_hash` | JSON `detail`; **tamper-evident hash chain** over `prev_hash` + the row (the `client` address is **inside** the chained payload — ADR 0150) | the store database | HIPAA §164.312(b) audit controls; incident response; `verify_audit_chain` integrity checks | `GET /audit` requires **`audit:read`**; `GET /audit/export` requires the separate **`audit:export`** and streams CSV with formula-injection neutralisation, recording its own `audit.export` row *before* streaming; `GET /me/security-events` is a per-user view of the same table | **`[retention].audit_days` is reserved and NOT enforced — keep-forever by design** (deleting rows would break the chain; HIPAA expects ~6 years) | `detail` is stored **in the clear** (it is not a cipher-covered column): its protection is that writers only ever store filter shapes, counts and ids — never bodies or credentials — plus the store ACL and the volume layer | +| **5. `audit_log` table** (SQLite, Postgres, SQL Server) | who / what / **where-from** / when of auth + PHI *access* and admin actions — plus, when the opt-in `[security].audit_all_authorization_decisions` is on (**default `false`**; the internal field it desugars to is `audit_all_authz`, whose old `[diagnostics]` TOML spelling is **refused at load** — ADR 0118), an `authz` row for **every** authorization decision including successes, which multiplies this stream's volume — `actor`, `action`, `channel_id`, `client`, `detail`, `row_hash` | JSON `detail`; **tamper-evident hash chain** over `prev_hash` + the row (the `client` address is **inside** the chained payload — ADR 0150) | the store database | HIPAA §164.312(b) audit controls; incident response; `verify_audit_chain` integrity checks | `GET /audit` requires **`audit:read`**; `GET /audit/export` requires the separate **`audit:export`** and streams CSV with formula-injection neutralisation, recording its own `audit.export` row *before* streaming; `GET /me/security-events` is a per-user view of the same table | **`[retention].audit_days` is reserved and NOT enforced — keep-forever by design.** The rationale rests on the AUDIT-RETENTION REQUIREMENT (45 CFR 164.316(b)(2)(i) six-year documentation retention; every framework floor is far below it — CIS Control 8.10 is 90 days, PCI DSS 4.0 10.5.1 is 12 months, NIST SP 800-53 AU-11 defers to organizational policy), **not** on chain-breakage. *Corrected 2026-07-30:* this row used to argue "deleting rows would break the chain". That is true only of deleting the OLDEST rows, and it is exactly INVERTED for the threat that motivates audit retention — `verify_audit_chain`'s own docstring (`store/store.py:7385`) records that *"deleting the newest rows is NOT caught by the walk alone — the surviving prefix still chains cleanly"*. An attacker hiding what they just did truncates the NEWEST rows, and the walk verifies cleanly afterwards. **And the anchor that would catch it is not reachable from the shipped CLI:** `audit_anchor()` exists on all three backends, but `messagefoundry audit-verify` calls `verify_audit_chain()` with **no** `expected_anchor` (`__main__.py:3418`) and the parser exposes no flag to supply one — its only callers are `pipeline/dr.py`. So an operator or compliance job running `audit-verify` gets a CLEAN result after a tail-truncation. As shipped, truncation detection depends on the off-box tee (sec-offbox-log #361/#363), not on anything runnable locally. Do not restore the chain-breakage argument, and do not upgrade this to "the anchor detects it" without also shipping a way to pass one | `detail` is stored **in the clear** (it is not a cipher-covered column): its protection is that writers only ever store filter shapes, counts and ids — never bodies or credentials — plus the store ACL and the volume layer | | **6. `message_events` table** | the per-message disposition timeline — the **complete** vocabulary is `received`, `routed`, `unrouted`, `filtered`, `transformed`, `delivered`, `failed`, `dead`, `error`, `replayed`, `resent`, `reingressed`, `passthrough`, `passthrough_dropped`, `cancelled`, `edit_resend`, `edit_resubmit`, `viewed`, `not_deployed` (CI asserts this list against the engine's own `MESSAGE_EVENT_KINDS`). `[diagnostics].message_events` can thin the set, but never below the compliance floor `viewed` / `dead` / `error` / `failed` / `not_deployed` | rows: `message_id`, `ts`, `event`, `destination`, `detail` | the store database | operator timeline on the message-detail view; the `viewed` row is the HIPAA PHI-access record | `GET /messages/{id}` under **`messages:view_raw`** + `require_phi_read`; the read itself writes a `viewed` event **and** a `message_view` audit row | no dedicated window — `purge_message_bodies` sets `message_events.detail` to `NULL` in the same transaction that blanks the body, so it inherits `[retention].messages_days` | `detail` is `safe_text()`-scrubbed **then** cipher-encrypted (AAD `("message_events","detail",message_id,ts,event)`). Verbosity gate `[diagnostics].message_events` = `all` (default) / `errors` / `off`, with a **compliance floor that can never be thinned**: `viewed`, `dead`, `error`, `failed`, `not_deployed` are retained at every level | | **7. `connection_event` table — DEFAULT ON** (`[diagnostics].connection_events = true`) | transport/lifecycle events per connection: `established`, `closed` (reason `eof` or `idle_timeout` — no path produces any other), `idle_timeout`, `at_capacity`, `peer_not_allowlisted`, `frame_oversize`, `framing_error`, `peer_reset`, plus the runner's `connection_lost` / `connection_restored`. That is the whole vocabulary, asserted in CI against the literal emit call sites in `transports/` and the pipeline runner **and** cross-checked against the console's own filter tuple. The MLLP, raw-TCP and HTTP listeners emit these; the **DICOM inbound C-STORE SCP** and the **`ISA`/`IEA`-framed X12 inbound** emit none — the runner injects the sink onto **every** source (`wiring_runner.py`, over the base-class `on_connection_event` field), so both connectors *have* the wiring and simply never call it — so this stream covers those three listeners plus the runner's outbound-lane transitions — not literally every connection. An X12 feed's connects, allow-list refusals and at-capacity refusals are therefore **absent** from this stream | rows: `ts`, `connection`, `transport`, `direction`, `kind`, `peer_host`, `message_id` (correlation hint), `reason` | the store database, **all three backends** | Corepoint-style transport diagnostics — "did the sender connect, and why did it drop" | `GET /events` and `GET /connections/{name}/events` under **`monitoring:read`** (**not** a PHI permission) with per-channel RBAC — an out-of-scope `connection=` is 403'd *and* audited — server-clamped to ≤1000 rows | `[retention].connection_event_retention_hours` (its own **hours** window); 0 inherits `[retention].messages_days`; both 0 = keep forever. Plain age `DELETE` (metadata-only) | **`reason` is free text that can carry sensitive fragments.** Defended twice — `safe_exc()` at the source, `safe_text(reason)[:200]` at the store — then cipher-encrypted (AAD `("connection_event","reason",connection,ts,kind)`). Every other column is config metadata; the table is documented **metadata-only** — never a frame, body or HL7 field value. Writes are a pure side observer: a bounded in-memory queue drained by a background task outside any handoff transaction, so a flood can never block a listener or pin a message disposition | | **8. `alert_instance` table — default on wherever an `[alerts]` notifier exists** | resolvable operator alerts: `connection_stopped`, `queue_buildup`, `lane_stuck`, `message_stall`, `saturation`, `connection_error`, `content_match`, `storage_threshold`, `cert_expiry`, `secret_rotation`, `bootstrap_admin_expiring` (the UNCLAIMED first-run bootstrap admin nearing its auto-disable deadline — ASVS 6.4.5; its payload carries only the ISO deadline plus whole hours remaining, never the password or any secret), `integrity_drift`, `update_available`, `backup_failed`, `rcsi_off_degraded`, `leadership_acquired`, `dr_activated`, `gcm_invocations` (the per-key AES-GCM invocation bound crossing its 2^31 soft warn — ASVS 11.3.4; its payload carries a one-way `key_id` fingerprint plus counters, never key bytes) The three reachable **inverse** signals — `connection_restored`, `leadership_lost`, `dr_released` — are never rows here: `_record_state` routes an inverse through `_AUTO_RESOLVE` to `resolve_alert_instances_for`, never to `upsert_alert_instance`. (A fourth mapped key, `connection_started`, is emitted by no code path today.) | rows: `event_type`, `connection`, `severity`, `status`, `first_seen`, `last_seen`, `count`, `reason`, `acked_by`, `acked_at`, `resolved_at`, `suspended_until`, `escalation_tier` | the store database, **all three backends** | the operator alert list — acknowledge / resolve / suspend. Durable state is recorded **before** any suppression or throttle return, so a muted alert still leaves a record | `GET /alerts/active` under **`monitoring:diagnose`** (**not** a PHI permission) with the same per-channel scope; ack/resolve/suspend/**resume** are POSTs on the same tier, and the separate read-only `GET /alerts/rules` view sits on its own gate | shares the connection-event window; **only RESOLVED instances are DELETEd**, by `resolved_at` — an open or acknowledged condition is never aged out from under an operator | **`reason` is free text** taken from the event's `detail`/`reason`/`label`: `safe_text(reason)[:200]` then cipher-encrypted (AAD `("alert_instance","reason",event_type,connection)` — the de-dup grain, so one AAD covers both the INSERT and the re-fire UPDATE). `content_match` is **PHI-free by contract**: the sink method takes no value parameter, only the connection, an operator label and an optional rule id | From 674146051c723a6c8b8e4eca703ee045d37cdd48 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 11:31:41 -0500 Subject: [PATCH 5/8] =?UTF-8?q?docs(phi):=20classify=20all=2038=20at-rest?= =?UTF-8?q?=20tiers=20=E2=80=94=20and=20four=20of=20them=20are=20UNBOUNDED?= =?UTF-8?q?=20(14.2.7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds §2's seventh column: a retention verdict for EVERY at-rest tier, in a 七-form vocabulary the serve gate's tier list and the §2/§8 drift test are GENERATED from. A wider hand-typed tuple is the same defect with a longer literal; this is the artifact that replaces it. Produced by a nine-agent workflow (classify -> adversarially attack -> reconcile) because a wrong cell here does not fail a test, it gets MACHINE-BLESSED: a false 'rides ' asserts coverage that lives somewhere else, and the drift test would then defend it on every run. This project has retracted an ASVS score twice for that shape. DISTRIBUTION: 7 own-window, 13 rides, 2 orphan-only, 5 keep-forever, 6 not-PHI, 1 keep-N, and 4 UNBOUNDED. The F7 count being non-zero was a stated pass condition -- a classification of this codebase with no gaps in it would be flattering rather than complete. TWO UNBOUNDED PL-1 TIERS WERE IN NO GAP TABLE ANYWHERE, and both are now in §8: * at stage ingress/routed, when DEAD. This is a REAL DEFECT, not a documentation gap, and it is filed separately. dead_letter_now issues and never touches ; a router/handler content fault calls it on the claimed ingress/routed row (wiring_runner.py:4415, :4449); and BOTH purges are scoped Stage.OUTBOUND (store.py:8384, :8659). So blanks on its window and the message READS AS PURGED while a full raw PHI body survives here indefinitely. It was missed because these rows are documented as transient -- true on the happy path, and the dead-letter path is the exception nobody classified. * the backup staging dirs. A TemporaryDirectory unlinks on exit but not on a crash or SIGKILL, and verify_after_backup (default true) decrypts a full archive back out on EVERY run. ONE ATTACKER OVERTURNED A CLASSIFIER, and the correction matters beyond bookkeeping: at stage outbound was filed as riding the body window, but purge_dead_letters is a purge dedicated to that exact table+column on its OWN window. Filing it F2 would have dropped -- one of exactly two windows __main__.py enumerates for the auto-bound and the refusal -- out of a list generated from this column. MY OWN GUARD CAUGHT MY OWN WORDING while writing this: the new staging-dir row said 'point TMP/TMPDIR at an owner-only path', which test_owner_only_file_acl_is_always_qualified_to_the_sqlite_store correctly flagged as an unqualified ACL claim -- the engine applies no file ACL on a server-DB store, the deployed posture. Re-scoped to state the engine's no-ACL plainly and put the cover where it belongs, on the operator. WORKFLOW DEFECT WORTH RECORDING: was passed as a JSON array and arrived as a STRING, so the four intended row-batches sliced CHARACTERS. The agents recovered by reading the file themselves -- 68 classifications came back for 38 rows -- and the reconciler deduplicated. It worked by redundancy, not by design. Completeness was therefore verified independently (1..38 exactly once, no duplicates, every row 7 cells) rather than taken from the reconciler's own 'checked' claim. --- docs/PHI.md | 103 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 63 insertions(+), 40 deletions(-) diff --git a/docs/PHI.md b/docs/PHI.md index 63b37017..fb310d8d 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -86,46 +86,67 @@ destruction) are documented in [§3](#3-encryption-at-rest) under the matching h | **PL-4 · Operational metadata (non-PHI)** | Ids, hashes, counts, config labels — deliberately **not** ciphered. | | **PL-5 · Engine-unreachable substrate** | Journals, logs, version stores, indexes. The app-level AEAD **cannot** reach these; whole-DB / volume encryption is the only cover. | -| Location | Backends | Holds PHI? | Encrypted at rest? (cipher · cell-AAD · key path) | Protection level | Notes | -|---|---|---|---|---|---| -| `messages.raw` | all three | **Yes** — full inbound body | **Yes, when a key is set** — store cipher; AAD `("messages","raw",id)`; store DEK | **PL-1** | Preserved verbatim by design (operators must see what arrived) | -| `queue.payload` (stage=`ingress`/`routed`) | all three | **Yes** — the raw body, **transient** | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | A second copy of the raw, held only across the route→transform window: the `ingress` row is consumed at `route_handoff`, each `routed` row at `transform_handoff` (deleted, never kept). A stalled router/transform stage can hold several briefly — surfaced by the `queue_buildup` alert | -| `queue.payload` (stage=`outbound`) | all three | **Yes** — transformed outbound body | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | One row per destination; the persistent footprint | -| `shared_body.body` (store-once-deliver-many) | schema on all three; **rows written on SQLite only** | **Yes** — one transformed outbound body shared by N destinations | **Yes, when a key is set** — store cipher; AAD `("shared_body","body",hash)`; store DEK | **PL-1** | `hash` is the SHA-256 of the **plaintext** body (the content address). Refcounted: GC'd the moment the last referencing outbound row's body is purged. On SQL Server and Postgres the table is schema-parity only — `queue.body_ref` stays `NULL`, so no row is ever written. **Both** server backends nonetheless keep a read-side `LEFT JOIN shared_body` deref in `resend_to` (with its own `cell_aad("shared_body","body",…)` decrypt branch), which is why the column stays **cipher-covered** on all three. It is **rotation-swept on SQLite only** — `shared_body` has a pass in `MessageStore.reencrypt_to_active` but appears in neither server backend's rotation. Harmless while `body_ref` stays `NULL` there, and recorded as a deliberate decision in `store/postgres.py`'s `_CIPHER_COLUMNS` note ("schema-only this increment … it needs no rotation pass here until the dedup insert is wired") | -| `attachment_chunk.ciphertext` (ADR 0105 / #149) | all three | **Yes** — one slice of a detached very-large document (e.g. a base64 PDF from OBX-5.5) | **Yes, when a key is set** — store cipher, **sealed per chunk on write**; AAD `("attachment_chunk","ciphertext",attachment_id,seq)`; store DEK | **PL-1** | The document is detached at ingress, content-addressed (`sha256` of the verbatim plaintext) and chunked; the message keeps only an `mfdoc:v1:ref:` handle. Read back via `GET /messages/{id}/attachments/{id}` (§3) | -| `attachment` header row (`content_type`, `total_bytes`, `refcount`, `created_at`) + `message_attachment` linkage | all three | **No** — size/type/linkage only | No (metadata, deliberately not ciphered) | **PL-4** | The linkage row is the security crux of the download route: it scopes a content address to a message the caller may already read | -| `response.body` (ADR 0013 captured replies; ADR 0021 `kind='ack_sent'`) | all three | **Yes** — the partner's reply body, or the ACK/NAK the engine returned | **Yes, when a key is set** — store cipher; AAD `("response","body",message_id,destination_name,response_seq)`; store DEK | **PL-1** | Composite PK, so it rides its own migration/rotation pass. An **ACK body is stored only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext (fail-safe), and a NAK never stores a body at all | -| `outbox.payload` (**legacy**, pre-staged-pipeline schema) | **SQL Server only** | **Yes** — a full outbound body on a store upgraded from the flat-`outbox` schema | **Yes, when a key is set** — store cipher (on-open migration only); AAD `("outbox","payload",id)`; store DEK | **PL-1** | SQL Server's schema pass re-runs a **create-if-absent** `IF OBJECT_ID('outbox','U') IS NULL CREATE TABLE outbox` on every open, so the table always exists and an existing one is never re-created or emptied — and it is never migrated away or dropped (SQLite migrates its rows into `queue` then `DROP`s it; Postgres has no such table). Empty on a greenfield install. **Three honest gaps:** it is absent from `reencrypt_to_active`, so a key rotation followed by dropping the retired key leaves those bodies undecryptable; `outbox.last_error` is in neither the migration nor the rotation pass; and **no purge path on any backend touches it** ([§8](#8-retention--purge)) | -| `[store].uploads_dir/*.blob` + `*.meta` — the cipher cells `uploaded_file.body` / `uploaded_file.meta` (offline uploaded logs, ADR 0134) | all (filesystem, not the DB) | **Yes** — an operator-uploaded diagnostic message file, held for offline browsing decoupled from any connection | **Yes, when a key is set** — the **same store cipher** (`build_store_cipher`); AAD `("uploaded_file","body")` / `("uploaded_file","meta")` + `file_id`; store DEK. **identity/plaintext-on-disk otherwise** (the File-connector-spill tier below) | **PL-1** | A PHI-at-rest location **outside** the message store, opt-in (unset ⇒ the subsystem is disabled — no surface). On-disk identity is a random 32-hex `file_id` (path-traversal guard); the operator filename is display-only. Every access is `files:*`-gated, browse is step-up + PHI-hop-guarded, all audited (metadata only). **Not** re-encrypted by `rotate-key` (outside the store) — stays readable via the decrypt keyring. The dir is created `0o700` and the sidecar written `0o600` **best-effort, and both are no-ops on Windows** — the engine applies no ACL here (it does not call the `icacls` enforcer). **Retention + quotas (ASVS 5.2.4):** uploaded files auto-prune after `[store].uploads_retention_days` (default **30**) — swept opportunistically at save time and by a periodic task; every prune is audited (`upload.prune`, file_id + uploader only, never content). Per-uploader caps `[store].max_upload_files_per_user` (default **100**) / `[store].max_upload_total_bytes_per_user` (default **250 MiB**) bound the at-rest volume; a would-be over-quota upload is refused **HTTP 409** with an `upload.reject_quota` audit before anything is written (defaults-ON, `ge=1` floors; quota is per-process per-`uploads_dir`). Harden the dir + volume encryption ([§10](#10-secure-deployment--operations-checklist)) | -| `[backup].destination/mefor-backup-*.mfbak` (ADR 0049 DR backup) | **SQLite only** carries bodies | **SQLite: Yes** — a consistent store snapshot (full inbound + outbound bodies) + the config bundle. **SQL Server / Postgres: No** — config bundle only | **Yes** — `.mfbak` chunked-AEAD codec under the **store DEK** (`resolve_active_key`); an identity-cipher (no-key) box is **refused** unless `[backup].allow_unencrypted` writes a `.mfbak.plain` | **PL-1** (SQLite) / **PL-4** (server backends) | On a **server-DB store `snapshot_to` raises `DbaDelegatedError`**, so the BackupRunner writes a **config-only** archive — or skips entirely when `[backup].config_only_on_server_db = false`. There is therefore **no `.mfbak` containing message bodies on SQL Server or Postgres**; the DB-tier backup there is `BACKUP DATABASE` / Always On / `pg_dump` / PITR, infra-owned. Where bodies *are* present it is a second at-rest PHI copy, bounded by keep-N retention; like `uploads_dir` it is **not** re-encrypted by `rotate-key`. The share's own ACLs are infra-owned | -| `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs (OS temp dir, ADR 0049) | SQLite carries bodies; server backends config-only | **Yes** — a full store snapshot, and on verify a **decrypted** archive | **No** — the snapshot keeps the store's own column cipher, but the staging tar and the verify extraction are **plaintext on disk**; no engine ACL (`_secure_file` is never called on these paths) | **PL-1** | `run_backup` snapshots the store to `/store.db` and tars it **plaintext** before sealing it into the `.mfbak` (`pipeline/dr_backup.py`), and `[backup].verify_after_backup` (**default `true`**) decrypts the archive straight back out to a second temp dir on **every** run — independent of `full_restore_verify`. Transient (the `TemporaryDirectory` unlinks on exit) but **not** on a crash or `SIGKILL`. Lives under `%TEMP%` / `TMPDIR`, **not** the ACL'd data dir: cover the temp volume with FDE and point `TMP`/`TMPDIR` at an owner-only path ([§10](#10-secure-deployment--operations-checklist)) | -| File-connector output / spill dirs (`.hl7`, `.processed`, `.error`) | all | **Yes** — plaintext on disk | **No** — no cipher at all on this path | **PL-1** | Written by the File transport; treat the directory as PHI and cover it with volume/share encryption + an ACL | -| Application log files (`[logging].log_dir`; under NSSM, `\logs\service.out.log` and `service.err.log`) | all (filesystem, not the DB) | **Possibly** — redaction is best-effort; a single-token identifier can survive it | **No** — plaintext on disk, no app-level cipher | **PL-1** | The engine installs no file handler; NSSM captures stdout/stderr. The defence is the three handler filters + `safe_exc()`/`safe_text()` + the never-log-bodies rule ([§7](#7-logging--phi-redaction) row 1), and the residual is stated there. The directory ACL is the NSSM installer's **best-effort** `icacls /inheritance:r`; age deletion is `[retention].app_log_days` (files by **mtime** — content is never read, so nothing selective happens here) and optional in-place gzip is `[retention].app_log_compress_days` (the compressor **does** read a file's bytes to archive + integrity-verify them, but only in-process — nothing is logged, and the archive stays inside the same ACL'd directory at the source's mtime). A support bundle copies a 500-line tail of this file out of the ACL'd directory entirely ([§7](#7-logging--phi-redaction)). Cover the volume with FDE ([§10](#10-secure-deployment--operations-checklist)) | -| `messages.summary` | all three | **Yes** — MRN / patient name / order | **Yes, when a key is set** — store cipher; AAD `("messages","summary",id)`; store DEK (EF-3) | **PL-2** | Ingest-derived; no SQL search or index exists on it, so encrypting it costs nothing. NULL/blank stay as-is | -| `messages.metadata` | all three | **Yes** — operator/handler-attached values | **Yes, when a key is set** — store cipher; AAD `("messages","metadata",id)`; store DEK (EF-3) | **PL-2** | **Nulled by `purge_message_bodies` on the `[retention].messages_days` window, in the same statement as the body** (ASVS 14.2.7) — see [§8](#8-retention--purge) | -| `messages.error` | all three | **Possibly** — may embed raw fragments from exceptions | **Yes, when a key is set** — store cipher; AAD `("messages","error",id)`; store DEK (WP-5) | **PL-2** | Also `safe_exc()`-redacted **before** write. NULL/blank values stay as-is | -| `queue.last_error` | all three | **Possibly** — same | **Yes, when a key is set** — store cipher; AAD `("queue","last_error",id)`; store DEK (WP-5) | **PL-2** | Same double defence (`safe_exc()` then cipher) | -| `message_events.detail` | all three | **Possibly** — per-message disposition detail | **Yes, when a key is set** — store cipher; AAD `("message_events","detail",message_id,ts,event)`; store DEK | **PL-2** | `id` is AUTOINCREMENT/IDENTITY and unknown at INSERT, so the AAD binds the natural tuple and the column rides its own composite migration/rotation pass. `safe_text()`-scrubbed before write | -| `response.detail`, `response.resp_headers` | all three | **Possibly** — reply diagnostics / partner response headers | **Yes, when a key is set** — store cipher; AAD `("response","detail", …)` / `("response","resp_headers",message_id,destination_name,response_seq)`; store DEK | **PL-2** | `detail` is `safe_text()`-scrubbed and 200-char bounded before the cipher | -| `state.value` (ADR 0005 transform state) | all three | **Possibly** — a correlation map (e.g. MRN→surrogate) written by a Handler | **Yes, when a key is set** — JSON-encoded then store cipher; AAD `("state","value",namespace,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API** — reachable only from a Handler via `state_get`/`state_set` | -| `reference.value` (ADR 0006 versioned lookup snapshots) | all three | **Possibly** — a snapshot row may be patient-keyed | **Yes, when a key is set** — store cipher; AAD `("reference","value",name,version,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API.** **`[retention].reference_snapshot_days`** — `purge_reference_snapshots` DELETEs the rows of a set config **no longer declares** whose active version was synced before the cutoff (`0` = keep forever, the default) ([§8](#8-retention--purge)). **Orphan-only, and the limit is the point:** a set that IS still declared is never touched however old its `synced_at`, because its snapshot is live data the engine serves — so the normal case, a wired set holding live PHI, is still bounded by nothing. Do not restate this as a plain window over `reference.value` | -| `search_presets.criteria` (ADR 0136 saved Log-Search filters) | all three | **Yes** — the operator's saved `content` / `field_value` needle is PHI-shaped by construction | **Yes, when a key is set** — store cipher; AAD `("search_presets","criteria",id)`; store DEK | **PL-2** | Never returned by the API: `GET /search/presets` lists names + timestamps only; the needle is loaded server-side by `GET /search/layered`. **`[retention].search_preset_days`** — `purge_search_presets` DELETEs the whole row past the window on every backend, keyed on last-**used** — the later of `updated_at` and `last_used_at`, #306 (`0` = keep forever, the default) ([§8](#8-retention--purge)) | -| `connection_event.reason` (#46 transport/lifecycle log, **default on**) | all three | **Possibly** — a free-text diagnostic fragment | **Yes, when a key is set** — store cipher; AAD `("connection_event","reason",connection,ts,kind)`; store DEK | **PL-2** | Defended twice: the emit site passes a `safe_exc()`-scrubbed string and the store re-applies `safe_text(reason)[:200]`. IDENTITY `id`, so its own composite pass. Every other column is bounded engine/config metadata **except `peer_host`** (its own PL-4 row below) — the table carries no frame, body or HL7 field value. Read under `monitoring:read`, **not** a PHI permission ([§7](#7-logging--phi-redaction)) | -| `connection_event.peer_host` | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The connecting peer's IP, taken from the socket; `NULL` for outbound/unknown. Personal data, not PHI — the **same class and the same decision** as `audit_log.client` / `sessions.client`: plaintext so it stays greppable for incident response. Returned to operators by `GET /events` under `monitoring:read`. Purged with its row by `[retention].connection_event_retention_hours` | -| `alert_instance.reason` (ADR 0044 operator alerts) | all three | **Possibly** — the alert's `detail`/`reason`/`label` free text | **Yes, when a key is set** — store cipher; AAD `("alert_instance","reason",event_type,connection)`; store DEK | **PL-2** | `safe_text(reason)[:200]` before the cipher. The AAD binds the de-dup grain, so the same AAD covers both the INSERT and the re-fire upsert UPDATE that never sees the `id`. Read under `monitoring:diagnose` ([§7](#7-logging--phi-redaction)) | -| `users.totp_secret` | all three | **No** — not PHI | **Yes, when a key is set** — store cipher; AAD `("users","totp_secret",id)`; store DEK | **PL-3** | The base32 TOTP MFA seed. Never returned by any API response model. Its siblings `users.password_hash` and `users.totp_recovery_codes` are **argon2id one-way hashes** and are deliberately **not** ciphered | -| `queue.handler_name` / `destination_name` / `channel_id` | all three | No — names, not bodies | No (metadata, deliberately not ciphered) | **PL-4** | The handler the transform worker runs; the destination the delivery worker drains | -| `messages.control_id`, `messages.message_type` | all three | Low (MSH-10/MSH-9) | **No** — plaintext by design | **PL-4** | Needed plaintext for dedup/routing/indexes (`ix_messages_control`). Covered only by the whole-DB / volume layer | -| `audit_log.detail` | all three | Low — exposed IDs/counts, not bodies | **No** — plaintext by design | **PL-4** | JSON metadata about PHI *access*, not the PHI itself. Its writers only ever store filter shapes, counts and ids | -| `audit_log.client` (ADR 0150) | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The caller's client address — the "from where" of an audited action; `NULL` for engine-internal/`system` writes. **Personal data, but not PHI**, and exactly what HIPAA §164.312(b) audit controls exist to capture. Plaintext by decision: it must stay greppable/indexable for incident response, it already appears in the clear in `sessions.client`, and it is folded **inside** the tamper-evident hash chain — so it carries **integrity** protection even without confidentiality. Widens a store-file compromise from *who did what* to *who did what from where*; volume encryption + owner-only ACLs on whichever host owns the files — the engine's own `_secure_file` covers the **SQLite** store only ([§10](#10-secure-deployment--operations-checklist)) — are the control | -| `delivered_keys` (H2 idempotency ledger) | all three | **No** — hashes + ids only | No (deliberately not ciphered — nothing to protect) | **PL-4** | One row per completed outbound delivery: a SHA-256 `delivery_key` over non-PHI ids + a replay-stable seq, plus `outbox_id`/`message_id`/`destination_name`/`delivery_seq`. **Never a body or any PHI** — `control_id` is only *folded into the hash input*, never stored in the clear here. Lets the FIFO claim skip-and-complete a re-claimed already-delivered head without re-sending | -| `state.namespace` / `state.key` | all three | **Possibly** — a Handler that keys correlation state on a raw MRN stores that identifier here in the clear | **No** — plaintext by construction: the pair is the composite primary key **and** the AAD input for `state.value`, so it cannot be ciphered without losing the lookup | **PL-4** | Authors must key state on a **surrogate, never a raw identifier**. Covered only by the whole-DB / volume layer. Rides `[retention].state_max_age_days` with its value | -| `reference.name` / `reference.version` / `reference.key` | all three | **Possibly** — §2's `reference.value` row notes a snapshot row may be patient-keyed; the key column is where that identifier would sit | **No** — plaintext by construction, same reason as `state` | **PL-4** | Same rule: key reference sets on a surrogate. The key columns ride the same delete: `purge_reference_snapshots` removes whole rows of an **undeclared** set, so these go with the `value` they key. A **declared** set is never touched — same orphan-only limit as `reference.value` above | -| `sessions.token_hash` / `client`, `resend_log`, `processed_files`, `pending_approvals.params`, `webauthn_credentials.public_key` | all three | **No** | No (deliberately not ciphered) | **PL-4** | Session tokens are stored as SHA-256 only; `processed_files` holds a hashed derived file key, never a path; approval params carry connection names / channel ids / a config dir by construction; COSE public keys (ADR 0068) are **verification material, not secrets** and are explicitly excluded from the cipher and from rekey | -| `secret_rotation_meta` (`secret_key` / `fingerprint` / `tracked_since` / `last_rotated`) | **All three backends** (#1186 — SQLite `MessageStore`, `SqlServerStore` and `PostgresStore` each create the table at open and implement the `SecretRotationMetaStore` protocol) | **No** — non-secret rotation state: a **keyed MAC** (DEK-derived, one-way — never the secret value) + ISO dates only | No (deliberately not ciphered — it is neither PHI nor a secret; the MAC is one-way and un-guessable without the DEK) | **PL-4** | ASVS 13.3.4 rotation watcher (BACKLOG #282). `fingerprint` is a keyed MAC so obtaining the rows leaks no secret. Written on a keyed store; absent on a keyless one | -| SQLite DB file + `-wal` / `-shm` / temp files, and every index | SQLite | **Yes** (mirror the above) | **No** — the app cipher cannot reach them | **PL-5** | WAL/shm hold recently-written PHI outside any app-level encryption. Cover: SQLCipher (whole-DB) and/or FDE on the engine host's data volume | -| SQL Server `.ldf` **transaction log** + the **tempdb version store**, and every index | SQL Server | **Yes** (row images of `messages`/`queue`, ciphertext columns **plus** the always-plaintext `control_id`/`message_type`) | **No** — outside the app-level AEAD entirely | **PL-5** | The engine itself makes this load-bearing: it **force-enables `READ_COMMITTED_SNAPSHOT` and `ALLOW_SNAPSHOT_ISOLATION`** on the store database at open, so tempdb's version store holds row images for the lifetime of every open snapshot. The `.ldf` holds every row image for the same reason. Additional tempdb objects: the `#eligible` temp table used by `purge_message_bodies` and the FIFO-claim table variables (ids only, non-PHI). Cover: **SQL Server TDE at the database + FDE on the *SQL Server host's* volumes** — **not** BitLocker on the engine host | -| PostgreSQL WAL (`pg_wal`), base files and every index | Postgres | **Yes** (mirror the above) | **No** — outside the app-level AEAD | **PL-5** | Cover: cluster-level / filesystem encryption on the database host, infra-owned | +| Location | Backends | Holds PHI? | Encrypted at rest? (cipher · cell-AAD · key path) | Protection level | Notes | Retention | +|---|---|---|---|---|---|---| +| `messages.raw` | all three | **Yes** — full inbound body | **Yes, when a key is set** — store cipher; AAD `("messages","raw",id)`; store DEK | **PL-1** | Preserved verbatim by design (operators must see what arrived) | `` `[security].delete_message_bodies_after_days` `` | +| `queue.payload` (stage=`ingress`/`routed`) | all three | **Yes** — the raw body, **transient** | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | A second copy of the raw, held only across the route→transform window: the `ingress` row is consumed at `route_handoff`, each `routed` row at `transform_handoff` (deleted, never kept). A stalled router/transform stage can hold several briefly — surfaced by the `queue_buildup` alert | `UNBOUNDED — honest gap` | +| `queue.payload` (stage=`outbound`) | all three | **Yes** — transformed outbound body | **Yes, when a key is set** — store cipher; AAD `("queue","payload",id)`; store DEK | **PL-1** | One row per destination; the persistent footprint | `` `[retention].dead_letter_days` `` | +| `shared_body.body` (store-once-deliver-many) | schema on all three; **rows written on SQLite only** | **Yes** — one transformed outbound body shared by N destinations | **Yes, when a key is set** — store cipher; AAD `("shared_body","body",hash)`; store DEK | **PL-1** | `hash` is the SHA-256 of the **plaintext** body (the content address). Refcounted: GC'd the moment the last referencing outbound row's body is purged. On SQL Server and Postgres the table is schema-parity only — `queue.body_ref` stays `NULL`, so no row is ever written. **Both** server backends nonetheless keep a read-side `LEFT JOIN shared_body` deref in `resend_to` (with its own `cell_aad("shared_body","body",…)` decrypt branch), which is why the column stays **cipher-covered** on all three. It is **rotation-swept on SQLite only** — `shared_body` has a pass in `MessageStore.reencrypt_to_active` but appears in neither server backend's rotation. Harmless while `body_ref` stays `NULL` there, and recorded as a deliberate decision in `store/postgres.py`'s `_CIPHER_COLUMNS` note ("schema-only this increment … it needs no rotation pass here until the dedup insert is wired") | ``rides `[security].delete_message_bodies_after_days` `` | +| `attachment_chunk.ciphertext` (ADR 0105 / #149) | all three | **Yes** — one slice of a detached very-large document (e.g. a base64 PDF from OBX-5.5) | **Yes, when a key is set** — store cipher, **sealed per chunk on write**; AAD `("attachment_chunk","ciphertext",attachment_id,seq)`; store DEK | **PL-1** | The document is detached at ingress, content-addressed (`sha256` of the verbatim plaintext) and chunked; the message keeps only an `mfdoc:v1:ref:` handle. Read back via `GET /messages/{id}/attachments/{id}` (§3) | ``rides `[security].delete_message_bodies_after_days` `` | +| `attachment` header row (`content_type`, `total_bytes`, `refcount`, `created_at`) + `message_attachment` linkage | all three | **No** — size/type/linkage only | No (metadata, deliberately not ciphered) | **PL-4** | The linkage row is the security crux of the download route: it scopes a content address to a message the caller may already read | ``rides `[security].delete_message_bodies_after_days` `` | +| `response.body` (ADR 0013 captured replies; ADR 0021 `kind='ack_sent'`) | all three | **Yes** — the partner's reply body, or the ACK/NAK the engine returned | **Yes, when a key is set** — store cipher; AAD `("response","body",message_id,destination_name,response_seq)`; store DEK | **PL-1** | Composite PK, so it rides its own migration/rotation pass. An **ACK body is stored only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext (fail-safe), and a NAK never stores a body at all | ``rides `[security].delete_message_bodies_after_days` `` | +| `outbox.payload` (**legacy**, pre-staged-pipeline schema) | **SQL Server only** | **Yes** — a full outbound body on a store upgraded from the flat-`outbox` schema | **Yes, when a key is set** — store cipher (on-open migration only); AAD `("outbox","payload",id)`; store DEK | **PL-1** | SQL Server's schema pass re-runs a **create-if-absent** `IF OBJECT_ID('outbox','U') IS NULL CREATE TABLE outbox` on every open, so the table always exists and an existing one is never re-created or emptied — and it is never migrated away or dropped (SQLite migrates its rows into `queue` then `DROP`s it; Postgres has no such table). Empty on a greenfield install. **Three honest gaps:** it is absent from `reencrypt_to_active`, so a key rotation followed by dropping the retired key leaves those bodies undecryptable; `outbox.last_error` is in neither the migration nor the rotation pass; and **no purge path on any backend touches it** ([§8](#8-retention--purge)) | `UNBOUNDED — honest gap` | +| `[store].uploads_dir/*.blob` + `*.meta` — the cipher cells `uploaded_file.body` / `uploaded_file.meta` (offline uploaded logs, ADR 0134) | all (filesystem, not the DB) | **Yes** — an operator-uploaded diagnostic message file, held for offline browsing decoupled from any connection | **Yes, when a key is set** — the **same store cipher** (`build_store_cipher`); AAD `("uploaded_file","body")` / `("uploaded_file","meta")` + `file_id`; store DEK. **identity/plaintext-on-disk otherwise** (the File-connector-spill tier below) | **PL-1** | A PHI-at-rest location **outside** the message store, opt-in (unset ⇒ the subsystem is disabled — no surface). On-disk identity is a random 32-hex `file_id` (path-traversal guard); the operator filename is display-only. Every access is `files:*`-gated, browse is step-up + PHI-hop-guarded, all audited (metadata only). **Not** re-encrypted by `rotate-key` (outside the store) — stays readable via the decrypt keyring. The dir is created `0o700` and the sidecar written `0o600` **best-effort, and both are no-ops on Windows** — the engine applies no ACL here (it does not call the `icacls` enforcer). **Retention + quotas (ASVS 5.2.4):** uploaded files auto-prune after `[store].uploads_retention_days` (default **30**) — swept opportunistically at save time and by a periodic task; every prune is audited (`upload.prune`, file_id + uploader only, never content). Per-uploader caps `[store].max_upload_files_per_user` (default **100**) / `[store].max_upload_total_bytes_per_user` (default **250 MiB**) bound the at-rest volume; a would-be over-quota upload is refused **HTTP 409** with an `upload.reject_quota` audit before anything is written (defaults-ON, `ge=1` floors; quota is per-process per-`uploads_dir`). Harden the dir + volume encryption ([§10](#10-secure-deployment--operations-checklist)) | `` `[store].uploads_retention_days` `` | +| `[backup].destination/mefor-backup-*.mfbak` (ADR 0049 DR backup) | **SQLite only** carries bodies | **SQLite: Yes** — a consistent store snapshot (full inbound + outbound bodies) + the config bundle. **SQL Server / Postgres: No** — config bundle only | **Yes** — `.mfbak` chunked-AEAD codec under the **store DEK** (`resolve_active_key`); an identity-cipher (no-key) box is **refused** unless `[backup].allow_unencrypted` writes a `.mfbak.plain` | **PL-1** (SQLite) / **PL-4** (server backends) | On a **server-DB store `snapshot_to` raises `DbaDelegatedError`**, so the BackupRunner writes a **config-only** archive — or skips entirely when `[backup].config_only_on_server_db = false`. There is therefore **no `.mfbak` containing message bodies on SQL Server or Postgres**; the DB-tier backup there is `BACKUP DATABASE` / Always On / `pg_dump` / PITR, infra-owned. Where bodies *are* present it is a second at-rest PHI copy, bounded by keep-N retention; like `uploads_dir` it is **not** re-encrypted by `rotate-key`. The share's own ACLs are infra-owned | ``keep-N `[backup].retention_keep` `` | +| `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs (OS temp dir, ADR 0049) | SQLite carries bodies; server backends config-only | **Yes** — a full store snapshot, and on verify a **decrypted** archive | **No** — the snapshot keeps the store's own column cipher, but the staging tar and the verify extraction are **plaintext on disk**; no engine ACL (`_secure_file` is never called on these paths) | **PL-1** | `run_backup` snapshots the store to `/store.db` and tars it **plaintext** before sealing it into the `.mfbak` (`pipeline/dr_backup.py`), and `[backup].verify_after_backup` (**default `true`**) decrypts the archive straight back out to a second temp dir on **every** run — independent of `full_restore_verify`. Transient (the `TemporaryDirectory` unlinks on exit) but **not** on a crash or `SIGKILL`. Lives under `%TEMP%` / `TMPDIR`, **not** the ACL'd data dir: cover the temp volume with FDE and point `TMP`/`TMPDIR` at an owner-only path ([§10](#10-secure-deployment--operations-checklist)) | `UNBOUNDED — honest gap` | +| File-connector output / spill dirs (`.hl7`, `.processed`, `.error`) | all | **Yes** — plaintext on disk | **No** — no cipher at all on this path | **PL-1** | Written by the File transport; treat the directory as PHI and cover it with volume/share encryption + an ACL | `UNBOUNDED — honest gap` | +| Application log files (`[logging].log_dir`; under NSSM, `\logs\service.out.log` and `service.err.log`) | all (filesystem, not the DB) | **Possibly** — redaction is best-effort; a single-token identifier can survive it | **No** — plaintext on disk, no app-level cipher | **PL-1** | The engine installs no file handler; NSSM captures stdout/stderr. The defence is the three handler filters + `safe_exc()`/`safe_text()` + the never-log-bodies rule ([§7](#7-logging--phi-redaction) row 1), and the residual is stated there. The directory ACL is the NSSM installer's **best-effort** `icacls /inheritance:r`; age deletion is `[retention].app_log_days` (files by **mtime** — content is never read, so nothing selective happens here) and optional in-place gzip is `[retention].app_log_compress_days` (the compressor **does** read a file's bytes to archive + integrity-verify them, but only in-process — nothing is logged, and the archive stays inside the same ACL'd directory at the source's mtime). A support bundle copies a 500-line tail of this file out of the ACL'd directory entirely ([§7](#7-logging--phi-redaction)). Cover the volume with FDE ([§10](#10-secure-deployment--operations-checklist)) | `` `[retention].app_log_days` `` | +| `messages.summary` | all three | **Yes** — MRN / patient name / order | **Yes, when a key is set** — store cipher; AAD `("messages","summary",id)`; store DEK (EF-3) | **PL-2** | Ingest-derived; no SQL search or index exists on it, so encrypting it costs nothing. NULL/blank stay as-is | ``rides `[security].delete_message_bodies_after_days` `` | +| `messages.metadata` | all three | **Yes** — operator/handler-attached values | **Yes, when a key is set** — store cipher; AAD `("messages","metadata",id)`; store DEK (EF-3) | **PL-2** | **Nulled by `purge_message_bodies` on the `[retention].messages_days` window, in the same statement as the body** (ASVS 14.2.7) — see [§8](#8-retention--purge) | ``rides `[security].delete_message_bodies_after_days` `` | +| `messages.error` | all three | **Possibly** — may embed raw fragments from exceptions | **Yes, when a key is set** — store cipher; AAD `("messages","error",id)`; store DEK (WP-5) | **PL-2** | Also `safe_exc()`-redacted **before** write. NULL/blank values stay as-is | ``rides `[security].delete_message_bodies_after_days` `` | +| `queue.last_error` | all three | **Possibly** — same | **Yes, when a key is set** — store cipher; AAD `("queue","last_error",id)`; store DEK (WP-5) | **PL-2** | Same double defence (`safe_exc()` then cipher) | ``rides `[security].delete_message_bodies_after_days` `` | +| `message_events.detail` | all three | **Possibly** — per-message disposition detail | **Yes, when a key is set** — store cipher; AAD `("message_events","detail",message_id,ts,event)`; store DEK | **PL-2** | `id` is AUTOINCREMENT/IDENTITY and unknown at INSERT, so the AAD binds the natural tuple and the column rides its own composite migration/rotation pass. `safe_text()`-scrubbed before write | ``rides `[security].delete_message_bodies_after_days` `` | +| `response.detail`, `response.resp_headers` | all three | **Possibly** — reply diagnostics / partner response headers | **Yes, when a key is set** — store cipher; AAD `("response","detail", …)` / `("response","resp_headers",message_id,destination_name,response_seq)`; store DEK | **PL-2** | `detail` is `safe_text()`-scrubbed and 200-char bounded before the cipher | ``rides `[security].delete_message_bodies_after_days` `` | +| `state.value` (ADR 0005 transform state) | all three | **Possibly** — a correlation map (e.g. MRN→surrogate) written by a Handler | **Yes, when a key is set** — JSON-encoded then store cipher; AAD `("state","value",namespace,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API** — reachable only from a Handler via `state_get`/`state_set` | `` `[retention].state_max_age_days` `` | +| `reference.value` (ADR 0006 versioned lookup snapshots) | all three | **Possibly** — a snapshot row may be patient-keyed | **Yes, when a key is set** — store cipher; AAD `("reference","value",name,version,key)`; store DEK | **PL-2** | Composite PK; own migration/rotation pass. **No read API.** **`[retention].reference_snapshot_days`** — `purge_reference_snapshots` DELETEs the rows of a set config **no longer declares** whose active version was synced before the cutoff (`0` = keep forever, the default) ([§8](#8-retention--purge)). **Orphan-only, and the limit is the point:** a set that IS still declared is never touched however old its `synced_at`, because its snapshot is live data the engine serves — so the normal case, a wired set holding live PHI, is still bounded by nothing. Do not restate this as a plain window over `reference.value` | ``orphan-only `[retention].reference_snapshot_days` `` | +| `search_presets.criteria` (ADR 0136 saved Log-Search filters) | all three | **Yes** — the operator's saved `content` / `field_value` needle is PHI-shaped by construction | **Yes, when a key is set** — store cipher; AAD `("search_presets","criteria",id)`; store DEK | **PL-2** | Never returned by the API: `GET /search/presets` lists names + timestamps only; the needle is loaded server-side by `GET /search/layered`. **`[retention].search_preset_days`** — `purge_search_presets` DELETEs the whole row past the window on every backend, keyed on last-**used** — the later of `updated_at` and `last_used_at`, #306 (`0` = keep forever, the default) ([§8](#8-retention--purge)) | `` `[retention].search_preset_days` `` | +| `connection_event.reason` (#46 transport/lifecycle log, **default on**) | all three | **Possibly** — a free-text diagnostic fragment | **Yes, when a key is set** — store cipher; AAD `("connection_event","reason",connection,ts,kind)`; store DEK | **PL-2** | Defended twice: the emit site passes a `safe_exc()`-scrubbed string and the store re-applies `safe_text(reason)[:200]`. IDENTITY `id`, so its own composite pass. Every other column is bounded engine/config metadata **except `peer_host`** (its own PL-4 row below) — the table carries no frame, body or HL7 field value. Read under `monitoring:read`, **not** a PHI permission ([§7](#7-logging--phi-redaction)) | `` `[retention].connection_event_retention_hours` `` | +| `connection_event.peer_host` | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The connecting peer's IP, taken from the socket; `NULL` for outbound/unknown. Personal data, not PHI — the **same class and the same decision** as `audit_log.client` / `sessions.client`: plaintext so it stays greppable for incident response. Returned to operators by `GET /events` under `monitoring:read`. Purged with its row by `[retention].connection_event_retention_hours` | ``rides `[retention].connection_event_retention_hours` `` | +| `alert_instance.reason` (ADR 0044 operator alerts) | all three | **Possibly** — the alert's `detail`/`reason`/`label` free text | **Yes, when a key is set** — store cipher; AAD `("alert_instance","reason",event_type,connection)`; store DEK | **PL-2** | `safe_text(reason)[:200]` before the cipher. The AAD binds the de-dup grain, so the same AAD covers both the INSERT and the re-fire upsert UPDATE that never sees the `id`. Read under `monitoring:diagnose` ([§7](#7-logging--phi-redaction)) | ``rides `[retention].connection_event_retention_hours` `` | +| `users.totp_secret` | all three | **No** — not PHI | **Yes, when a key is set** — store cipher; AAD `("users","totp_secret",id)`; store DEK | **PL-3** | The base32 TOTP MFA seed. Never returned by any API response model. Its siblings `users.password_hash` and `users.totp_recovery_codes` are **argon2id one-way hashes** and are deliberately **not** ciphered | `keep-forever by design` — it lives and dies with the user row | +| `queue.handler_name` / `destination_name` / `channel_id` | all three | No — names, not bodies | No (metadata, deliberately not ciphered) | **PL-4** | The handler the transform worker runs; the destination the delivery worker drains | `n/a — not PHI` | +| `messages.control_id`, `messages.message_type` | all three | Low (MSH-10/MSH-9) | **No** — plaintext by design | **PL-4** | Needed plaintext for dedup/routing/indexes (`ix_messages_control`). Covered only by the whole-DB / volume layer | `keep-forever by design` — dedup/routing keys that live and die with the message row | +| `audit_log.detail` | all three | Low — exposed IDs/counts, not bodies | **No** — plaintext by design | **PL-4** | JSON metadata about PHI *access*, not the PHI itself. Its writers only ever store filter shapes, counts and ids | `keep-forever by design` — 45 CFR 164.316(b)(2)(i) six-year documentation retention; deleting rows would break the tamper-evident hash chain | +| `audit_log.client` (ADR 0150) | all three | **No** — a network address; identifies a *host*, not a patient | No (metadata, deliberately not ciphered) | **PL-4** | The caller's client address — the "from where" of an audited action; `NULL` for engine-internal/`system` writes. **Personal data, but not PHI**, and exactly what HIPAA §164.312(b) audit controls exist to capture. Plaintext by decision: it must stay greppable/indexable for incident response, it already appears in the clear in `sessions.client`, and it is folded **inside** the tamper-evident hash chain — so it carries **integrity** protection even without confidentiality. Widens a store-file compromise from *who did what* to *who did what from where*; volume encryption + owner-only ACLs on whichever host owns the files — the engine's own `_secure_file` covers the **SQLite** store only ([§10](#10-secure-deployment--operations-checklist)) — are the control | `keep-forever by design` — same `audit_log` row lifetime as `detail`; the value is folded **inside** the hash chain | +| `delivered_keys` (H2 idempotency ledger) | all three | **No** — hashes + ids only | No (deliberately not ciphered — nothing to protect) | **PL-4** | One row per completed outbound delivery: a SHA-256 `delivery_key` over non-PHI ids + a replay-stable seq, plus `outbox_id`/`message_id`/`destination_name`/`delivery_seq`. **Never a body or any PHI** — `control_id` is only *folded into the hash input*, never stored in the clear here. Lets the FIFO claim skip-and-complete a re-claimed already-delivered head without re-sending | `keep-forever by design` — the idempotency ledger a re-claimed already-delivered row checks instead of re-sending | +| `state.namespace` / `state.key` | all three | **Possibly** — a Handler that keys correlation state on a raw MRN stores that identifier here in the clear | **No** — plaintext by construction: the pair is the composite primary key **and** the AAD input for `state.value`, so it cannot be ciphered without losing the lookup | **PL-4** | Authors must key state on a **surrogate, never a raw identifier**. Covered only by the whole-DB / volume layer. Rides `[retention].state_max_age_days` with its value | ``rides `[retention].state_max_age_days` `` | +| `reference.name` / `reference.version` / `reference.key` | all three | **Possibly** — §2's `reference.value` row notes a snapshot row may be patient-keyed; the key column is where that identifier would sit | **No** — plaintext by construction, same reason as `state` | **PL-4** | Same rule: key reference sets on a surrogate. The key columns ride the same delete: `purge_reference_snapshots` removes whole rows of an **undeclared** set, so these go with the `value` they key. A **declared** set is never touched — same orphan-only limit as `reference.value` above | ``orphan-only `[retention].reference_snapshot_days` `` | +| `sessions.token_hash` / `client`, `resend_log`, `processed_files`, `pending_approvals.params`, `webauthn_credentials.public_key` | all three | **No** | No (deliberately not ciphered) | **PL-4** | Session tokens are stored as SHA-256 only; `processed_files` holds a hashed derived file key, never a path; approval params carry connection names / channel ids / a config dir by construction; COSE public keys (ADR 0068) are **verification material, not secrets** and are explicitly excluded from the cipher and from rekey | `n/a — not PHI` | +| `secret_rotation_meta` (`secret_key` / `fingerprint` / `tracked_since` / `last_rotated`) | **All three backends** (#1186 — SQLite `MessageStore`, `SqlServerStore` and `PostgresStore` each create the table at open and implement the `SecretRotationMetaStore` protocol) | **No** — non-secret rotation state: a **keyed MAC** (DEK-derived, one-way — never the secret value) + ISO dates only | No (deliberately not ciphered — it is neither PHI nor a secret; the MAC is one-way and un-guessable without the DEK) | **PL-4** | ASVS 13.3.4 rotation watcher (BACKLOG #282). `fingerprint` is a keyed MAC so obtaining the rows leaks no secret. Written on a keyed store; absent on a keyless one | `n/a — not PHI` | +| SQLite DB file + `-wal` / `-shm` / temp files, and every index | SQLite | **Yes** (mirror the above) | **No** — the app cipher cannot reach them | **PL-5** | WAL/shm hold recently-written PHI outside any app-level encryption. Cover: SQLCipher (whole-DB) and/or FDE on the engine host's data volume | `n/a — not PHI` | +| SQL Server `.ldf` **transaction log** + the **tempdb version store**, and every index | SQL Server | **Yes** (row images of `messages`/`queue`, ciphertext columns **plus** the always-plaintext `control_id`/`message_type`) | **No** — outside the app-level AEAD entirely | **PL-5** | The engine itself makes this load-bearing: it **force-enables `READ_COMMITTED_SNAPSHOT` and `ALLOW_SNAPSHOT_ISOLATION`** on the store database at open, so tempdb's version store holds row images for the lifetime of every open snapshot. The `.ldf` holds every row image for the same reason. Additional tempdb objects: the `#eligible` temp table used by `purge_message_bodies` and the FIFO-claim table variables (ids only, non-PHI). Cover: **SQL Server TDE at the database + FDE on the *SQL Server host's* volumes** — **not** BitLocker on the engine host | `n/a — not PHI` | +| PostgreSQL WAL (`pg_wal`), base files and every index | Postgres | **Yes** (mirror the above) | **No** — outside the app-level AEAD | **PL-5** | Cover: cluster-level / filesystem encryption on the database host, infra-owned | `n/a — not PHI` | + +> **Retention column — vocabulary (ASVS 14.2.7).** Every cell is exactly one of seven forms. The serve +> gate's tier list and the §2↔§8 drift test are GENERATED from these, so there are no prose variants — +> a hand-typed tuple with a longer literal is the same defect this column exists to remove. +> +> - **`[section].window`** — bounded by its **own** named window, cited exactly as an operator types it. +> - **rides `[section].window`** — no window of its own; deleted or nulled by another tier's purge. +> Valid **only** when §8's row for that window names this table/column in its Mechanism cell. This is +> the form most likely to be wrong, because it asserts coverage that lives somewhere else. +> - **orphan-only `[retention].reference_snapshot_days`** — `reference.*` only: purged **only** when +> config no longer declares the set. A **declared** set is never touched, whatever its age. +> - **keep-forever by design** — a deliberate decision, not a gap; the clause after the dash is the reason. +> - **n/a — not PHI** — PL-4 operational metadata or PL-5 engine-unreachable substrate. +> - **keep-N `[backup].retention_keep`** — bounded by a **count** of retained artifacts, not an age window. +> - **UNBOUNDED — honest gap** — no purge covers this tier. Stated plainly on purpose: an honest gap is +> worth more than a coverage claim that cannot be evidenced, and every one of these is also listed in +> [§8](#8-retention--purge). +> +> Machine-readable by construction: the form keyword is **un-backticked** and the setting is +> **backticked**, so one pattern extracts the window from every bounded form, and the three prose forms +> contain no backticked setting at all. **Per-backend cipher coverage, stated exactly.** The store cipher covers **18** `(table, column)` pairs on SQLite. **SQL Server** covers 18 = the SQLite set **minus** `shared_body.body` (never written @@ -1056,6 +1077,8 @@ window purely so a replay can be richer — is precisely the defect ASVS 14.2.7 |---|---| | `reference.value` (a **declared** set) | **orphan-only** coverage. `purge_reference_snapshots` bounds a set config has DROPPED; a set still declared is never purged whatever its age, because its snapshot is live data. The wired-set case remains unbounded — the honest gap | | `outbox.payload` (SQL Server legacy) | recreated on every open, **touched by no purge on any backend** — legacy PHI there is retained forever | +| `queue.payload` (stage=`ingress`/`routed`, **DEAD**) | **no purge reaches it** — added 2026-07-30 by the ASVS 14.2.7 classification sweep, which is the only reason it was found. The happy path consumes these rows at `route_handoff`/`transform_handoff`, so they are documented as transient — but a router or handler content fault calls `dead_letter_now` (`pipeline/wiring_runner.py:4415`, `:4449`), which sets status/`last_error` and **never touches `payload`**, while both purges are scoped `Stage.OUTBOUND` (`store/store.py:8384`, `:8659`). So `messages.raw` blanks on its window and the message reads as purged while a **full raw PHI body survives here indefinitely**. Filed as its own defect; the likely fix is to let a DEAD ingress/routed row ride `[retention].dead_letter_days` exactly as a dead outbound row does | +| `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs | no window — a `TemporaryDirectory` unlinks on exit, but **not** on a crash or `SIGKILL`, and `verify_after_backup` (default `true`) decrypts a full archive back out to a second temp dir on **every** run. The engine applies **no ACL** on these paths (`_secure_file` is never called on them, and on a server-DB store — the deployed posture — it applies no file ACL at all), so the cover is the operator's: FDE on the temp volume plus `TMP`/`TMPDIR` pointed at a directory the OS restricts to the service account ([§10](#10-secure-deployment--operations-checklist)) | | `users.totp_secret` | no window by design — it lives and dies with the user row | | `audit_log`, `delivered_keys`, `resend_log` | keep-forever by design (see below) | | `messages.control_id`, `messages.message_type` | kept for the life of the message row by design (dedup/routing keys) | From 346fa8375b18c6341a6e24f2ee9cb782fbfabddf Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 12:38:37 -0500 Subject: [PATCH 6/8] feat(retention): generate the serve gate from the classification, not a literal (14.2.7) The startup retention gate checked a two-element literal. That literal is why this cell broke once: a new PHI tier landed and nobody widened it. Widening the literal is the same defect with more characters, so the tier list is now GENERATED from config/retention_classification.py, and tests/test_retention_classification_drift.py holds that constant equal -- in BOTH directions -- to docs/PHI.md 2's Retention column. Add a tier to the doc without adding it here, or here without the doc, and the suite reds. Nine windows across FIVE settings sections, not one. The first drift test checked every window against RetentionSettings and exempted [store]/[security] by prefix; adding [backup].retention_keep reded it and the tempting fix was to widen the exemption, which would have stopped verifying three of the five sections. Routing each window to its own model instead makes the check stronger than it was before it failed. MIN_PHI_RETENTION_WINDOWS is a FLOOR, not an emptiness check: `if not TUPLE` passes for a one-element tuple, so a bad merge would leave the gate checking one window while reporting success. The floor was itself corrected by its own drift test -- written as 7 by hand, the two-way equality immediately reported [backup].retention_keep and [retention].connection_event_retention_hours as documented-but-absent, before either had ever run. Behaviour change, deliberate and owner-approved (2026-07-30): the three PHI-BODY windows now DEFAULT to 30 days when UNSET, on both dials. Previously an unset window on the shipped `enforce` posture REFUSED TO START, which forced an operator to pick a number; it now starts bounded at 30. What survives is the fail-closed path for an EXPLICIT 0 -- typing 0 is a deliberate act and is still refused unless [security].allow_keeping_phi_indefinitely is set. So "unbounded by accident" is still prevented; "unbounded by inattention" becomes "30 days by inattention". test_cli.py carries both halves as a pair. state_max_age_days / search_preset_days / app_log_days / backup.retention_keep are classified and WARNED, never silently bounded -- also a ruling, not an omission. Their timestamps only move on a WRITE (state.set_at is never refreshed by state_get), so an auto-bound would delete a Handler's live MRN->surrogate crosswalk mid-stream, and the next message would transform WRONGLY, post-ACK, with no ERROR disposition. connection_event_retention_hours and uploads_retention_days are classified but never tested for unboundedness: on the first, 0 means INHERIT the body window, so a `days <= 0` predicate would refuse a start over a window that is doing its job; the second carries a ge=1 floor, so the clause is unfireable by construction. Both are recorded as zero_is_unbounded=False rather than dropped, so the generated list still matches 2. --- messagefoundry/__main__.py | 114 ++++++--- .../config/retention_classification.py | 205 +++++++++++++++ tests/test_cli.py | 49 +++- tests/test_retention_classification_drift.py | 242 ++++++++++++++++++ 4 files changed, 573 insertions(+), 37 deletions(-) create mode 100644 messagefoundry/config/retention_classification.py create mode 100644 tests/test_retention_classification_drift.py diff --git a/messagefoundry/__main__.py b/messagefoundry/__main__.py index 8d1ecb23..1af9a6ea 100644 --- a/messagefoundry/__main__.py +++ b/messagefoundry/__main__.py @@ -2007,45 +2007,97 @@ def _serve(args: argparse.Namespace) -> int: # default threads through to the RetentionRunner (no forbidden-file edit). # messages_days moved to [security].delete_message_bodies_after_days (ADR 0118); # dead_letter_days stays [retention] plumbing — label each window at its real home. - _RETENTION_WINDOW_LABEL = { - "messages_days": "[security].delete_message_bodies_after_days", - "dead_letter_days": "[retention].dead_letter_days", - } - if not enforcing and not settings.retention.allow_unbounded_phi: - auto_bounded = [ - field - for field in ("messages_days", "dead_letter_days") - if field not in settings.retention.model_fields_set + # ASVS 14.2.7: the tier list is GENERATED from the classification in + # config/retention_classification.py, which a drift test holds equal — in both directions — to + # docs/PHI.md §2's Retention column. It used to be a two-element literal here, and the cell + # broke once because a new PHI tier landed and nobody widened it. A wider literal with no + # binding to the classification is the same defect with more characters. + from messagefoundry.config.retention_classification import ( + MIN_PHI_RETENTION_WINDOWS, + PHI_RETENTION_WINDOWS, + auto_bounded_windows, + ) + from messagefoundry.config.retention_classification import ( + unbounded_windows as _unbounded_windows, + ) + + # A FLOOR, not an emptiness check. `if not PHI_RETENTION_WINDOWS` passes for a one-element + # tuple, so a bad merge dropping most entries would leave this gate checking one window while + # reporting success — the precise shape of failure this whole change set exists to remove. + if len(PHI_RETENTION_WINDOWS) < MIN_PHI_RETENTION_WINDOWS: + print( + f"error: the PHI retention classification has shrunk to " + f"{len(PHI_RETENTION_WINDOWS)} windows (floor {MIN_PHI_RETENTION_WINDOWS}); refusing " + "to start rather than gate on a partial classification. This is a build defect, not a " + "configuration one — see messagefoundry/config/retention_classification.py.", + file=sys.stderr, + ) + return 2 + + # AUTO-BOUND. Owner ruling 2026-07-30: the three PHI-BODY windows default to 30 days when + # UNSET, on BOTH dials — previously this ran only when `not enforcing`, so on the shipped + # `enforce` posture an unset window took the refusal below instead of a default. + # + # THE SAFETY TRADE IS DELIBERATE AND WORTH STATING: a production PHI instance with an unset + # window used to REFUSE TO START, which forced an operator to choose a number. It now starts + # with 30. What survives is the fail-closed path for an EXPLICIT 0 — choosing keep-forever is + # still refused unless the audited opt-out is set. So "unbounded by accident" is still + # prevented; "unbounded by inattention" becomes "30 days by inattention". + # + # The warn-only windows are NOT auto-bounded, and that is also a ruling rather than an + # omission: `purge_state` and `purge_search_presets` key on timestamps that only move on a + # WRITE, so silently bounding them deletes live operational data a Handler is still reading. + if not settings.retention.allow_unbounded_phi: + defaulted = [ + w + for w in auto_bounded_windows() + if w.field not in getattr(settings, w.reads_from.strip("[]")).model_fields_set ] - for field in auto_bounded: - setattr(settings.retention, field, 30) - if auto_bounded: - auto_desc = ", ".join(_RETENTION_WINDOW_LABEL[field] for field in auto_bounded) + for window in defaulted: + setattr( + getattr(settings, window.reads_from.strip("[]")), + window.field, + window.auto_bound_days, + ) + if defaulted: print( - f"info: {auto_desc} defaulted ON (30 days) for a PHI instance ({env_name!r}) — " - "PHI message bodies are now bounded at rest (secure-by-default, ASVS 14.2.7). Set an " - "explicit [retention] window to override, or [security].allow_keeping_phi_indefinitely=true to " - "retain indefinitely.", + f"info: {', '.join(w.setting for w in defaulted)} defaulted ON (30 days) for a PHI " + f"instance ({env_name!r}) — these PHI tiers are now bounded at rest " + "(secure-by-default, ASVS 14.2.7). Set an explicit window to override, or " + "[security].allow_keeping_phi_indefinitely=true to retain indefinitely.", file=sys.stderr, ) - unbounded_windows = [ - field - for field, days in ( - ("messages_days", settings.retention.messages_days), - ("dead_letter_days", settings.retention.dead_letter_days), + + # REFUSE / WARN. `unbounded_windows` skips the tiers where 0 does not mean unbounded + # (`connection_event_retention_hours` INHERITS the body window; `uploads_retention_days` has a + # ge=1 floor so 0 is unrepresentable) and those whose `requires_setting` is unmet — with no + # [logging].log_dir there is nothing for the app-log sweep to sweep. + still_unbounded = _unbounded_windows(settings) + refusable = [w for w in still_unbounded if w.auto_bound_days is not None] + warn_only = [w for w in still_unbounded if w.auto_bound_days is None] + + if warn_only: + # Classified and warned, never refused. Naming the tier AND its protection level is the + # point: an operator who sees "PL-1" knows a full body is involved. + print( + "warning: these classified PHI tiers have no retention window on a PHI instance " + f"({env_name!r}) and will accumulate without bound: " + + ", ".join(f"{w.setting} ({w.level})" for w in warn_only) + + ". They are deliberately NOT defaulted — each keys on a timestamp that only moves on " + "a write, so a silent default would delete data still in use (ASVS 14.2.7).", + file=sys.stderr, ) - if days <= 0 - ] - if unbounded_windows: - windows_desc = ", ".join(_RETENTION_WINDOW_LABEL[field] for field in unbounded_windows) + + if refusable: + windows_desc = ", ".join(w.setting for w in refusable) if not settings.retention.allow_unbounded_phi: if enforcing: print( - f"error: no data-retention window is configured for {windows_desc} on a " - f"{'production ' if production else ''}PHI instance ({env_name!r}); refusing to " - "start — PHI message bodies would be retained indefinitely (unbounded PHI at " - "rest, ASVS 14.2.4). Set the window(s) to a positive number of days (e.g. 30) to " - "bound PHI at rest; or, to deliberately retain forever, set " + f"error: a data-retention window is explicitly disabled for {windows_desc} on " + f"a {'production ' if production else ''}PHI instance ({env_name!r}); refusing " + "to start — PHI message bodies would be retained indefinitely (unbounded PHI " + "at rest, ASVS 14.2.4/14.2.7). Set the window(s) to a positive number of days " + "(e.g. 30); or, to deliberately retain forever, set " "[security].allow_keeping_phi_indefinitely=true (audited).", file=sys.stderr, ) diff --git a/messagefoundry/config/retention_classification.py b/messagefoundry/config/retention_classification.py new file mode 100644 index 00000000..8b5fe28a --- /dev/null +++ b/messagefoundry/config/retention_classification.py @@ -0,0 +1,205 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The PHI retention classification the startup gate is generated from (ASVS 14.2.7). + +**Why a constant rather than a hand-typed tuple in `__main__.py`.** This cell broke once because a new +PHI tier landed and nobody widened a literal in the serve gate. A wider literal with no binding to the +classification is the same defect with more characters. So the gate's tier list is built from +:data:`PHI_RETENTION_WINDOWS`, and `tests/test_retention_classification_drift.py` asserts — in BOTH +directions — that this tuple and `docs/PHI.md` §2's Retention column describe the same set. Add a PHI +tier to the doc without adding it here, or here without the doc, and the suite reds. + +**Why the doc is the co-authority and not just prose.** `docs/PHI.md` is git-TRACKED, so a +PHI.md-anchored test actually runs in CI. (`docs/security/` is git-ignored and its doc-anchored tests +take module-level skips — a distinction that has already produced one guard which red locally and +passed in CI.) That single fact is what makes the two-way equality real rather than decorative. + +**What is deliberately NOT here.** `[retention].audit_days` is reserved and unenforced by design — +there is no `purge_audit*` on the Store protocol, and the keep-forever rationale rests on the +audit-retention requirement itself (see `docs/PHI.md` §8), not on chain-breakage. Tiers classified +`UNBOUNDED — honest gap` in §2 are absent for the same reason: this tuple describes windows that +EXIST, and inventing an entry for a tier nothing purges would be the false-coverage claim the whole +column was built to prevent. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Final + + +@dataclass(frozen=True, slots=True) +class RetentionWindow: + """One classified PHI tier and the window that bounds it.""" + + #: The operator-facing setting, spelled exactly as it appears in config and in PHI.md §2. + setting: str + #: The attribute the serve gate reads. + field: str + #: The `[section]` whose settings model actually HOLDS :attr:`field`. Usually the same section as + #: :attr:`setting` — but not always, and the exception is load-bearing: ADR 0118 moved the + #: message-body window to the operator-facing `[security].delete_message_bodies_after_days` while + #: the gate still reads `retention.messages_days` after desugaring. Modelling the two separately is + #: what lets a test resolve every field against its real model instead of exempting the ones that + #: do not fit — and an exemption is how a field name silently stops being checked. + reads_from: str + #: Protection level from PHI.md §2 — PL-1 (body) or PL-2 (identifier/fragment). + level: str + #: Auto-bound to this many days when UNSET on a PHI instance, or ``None`` to leave it alone. + #: + #: ``None`` is a deliberate per-window judgement, NOT an oversight. Auto-bounding a window whose + #: timestamp only moves on a WRITE silently deletes live operational data: `state.set_at` is never + #: refreshed by `state_get`, so a Handler's MRN→surrogate crosswalk would vanish mid-stream and the + #: next message would transform WRONGLY, post-ACK, with no ERROR disposition. `search_presets` is + #: the same shape and `retention.py` forbids exactly that inheritance in prose. These are + #: classified and WARNED, never silently bounded (owner ruling, 2026-07-30). + auto_bound_days: int | None + #: Another setting whose presence this window depends on; the gate skips it when unset. + requires_setting: tuple[str, str] | None = None + #: Whether ``0`` on this window actually means UNBOUNDED. Not universal, and the exceptions are + #: exactly the ones a `days <= 0` predicate would get wrong: + #: + #: * ``[retention].connection_event_retention_hours`` — ``0`` means **INHERIT** the message-body + #: window, so an unset value is already bounded transitively. Flagging it would refuse or warn + #: over a window that is doing its job. + #: * ``[store].uploads_retention_days`` — carries a ``ge=1`` floor, so ``0`` is unrepresentable and + #: the clause would be unfireable by construction. Kept classified, never tested. + zero_is_unbounded: bool = True + + +#: Every PL-1/PL-2 tier that HAS a window, keyed to the classification in `docs/PHI.md` §2. +#: +#: Ordered as §2 orders them, so a reviewer can read the two side by side. +PHI_RETENTION_WINDOWS: Final[tuple[RetentionWindow, ...]] = ( + # PL-1 message bodies — the packet's core intent, and the two the gate already refused on. + RetentionWindow( + setting="[security].delete_message_bodies_after_days", + field="messages_days", + reads_from="[retention]", + level="PL-1", + auto_bound_days=30, + ), + RetentionWindow( + setting="[retention].dead_letter_days", + field="dead_letter_days", + reads_from="[retention]", + level="PL-1", + auto_bound_days=30, + ), + # PL-2 orphaned reference snapshots (ADR 0006). ORPHAN-ONLY: a still-declared set is never purged + # whatever its age, so this window does NOT bound `reference.value` in general — §2 says so, and a + # plain "rides"/window claim there would be false. + RetentionWindow( + setting="[retention].reference_snapshot_days", + field="reference_snapshot_days", + reads_from="[retention]", + level="PL-2", + auto_bound_days=30, + ), + # --- classified and WARNED, never auto-bounded (see auto_bound_days above) --------------------- + RetentionWindow( + setting="[retention].state_max_age_days", + field="state_max_age_days", + reads_from="[retention]", + level="PL-2", + auto_bound_days=None, + ), + RetentionWindow( + setting="[retention].search_preset_days", + field="search_preset_days", + reads_from="[retention]", + level="PL-2", + auto_bound_days=None, + ), + # PL-1 only because redaction is best-effort — a lone identifier can survive it. Gated on a + # log_dir: with none configured the sweep has nothing to sweep, so refusing over it would refuse + # over a knob that cannot do anything. + RetentionWindow( + setting="[retention].app_log_days", + field="app_log_days", + reads_from="[retention]", + level="PL-1", + auto_bound_days=None, + requires_setting=("logging", "log_dir"), + ), + # PL-1 operator-uploaded diagnostic files. Already defaults to 30 with a `ge=1` floor, so it can + # never appear unbounded — kept here so the generated list matches §2 rather than quietly omitting + # a classified tier because it happens to be safe today. + RetentionWindow( + setting="[store].uploads_retention_days", + field="uploads_retention_days", + reads_from="[store]", + level="PL-1", + auto_bound_days=None, + zero_is_unbounded=False, + ), + # PL-2 `connection_event.reason`. NOT a refuse/auto-bound candidate, and the reason is a genuine + # trap: here `0` means INHERIT the message-body window, not keep-forever (settings.py, and + # retention.py resolves it that way). So an unset value is already bounded transitively, and a + # `days <= 0` predicate would refuse a start over a window that is doing its job. + RetentionWindow( + setting="[retention].connection_event_retention_hours", + field="connection_event_retention_hours", + reads_from="[retention]", + level="PL-2", + auto_bound_days=None, + zero_is_unbounded=False, + ), + # PL-1 `.mfbak` archives, which on SQLite carry FULL inbound+outbound bodies. The odd one out: it + # is bounded by a COUNT (keep-N), not an age window, and its default of 7 already bounds it — the + # only way to reach unbounded is an operator explicitly typing 0, which is a deliberate choice + # (plausibly a DR requirement). So: classified and warned, never auto-bounded and never refused, + # and it only applies when `[backup].destination` is configured — with no destination there are no + # archives to bound. Owner ruling, 2026-07-30. + RetentionWindow( + setting="[backup].retention_keep", + field="retention_keep", + reads_from="[backup]", + level="PL-1", + auto_bound_days=None, + requires_setting=("backup", "destination"), + ), +) + +#: Floor for the derived list. `if not PHI_RETENTION_WINDOWS` is NOT sufficient: a bad merge dropping +#: most of the entries leaves a non-empty tuple, and a startup gate would then check two windows while +#: reporting success. +#: +#: NINE, and the number was corrected by its own drift test rather than by counting. It was first +#: written as 7 — the count I derived by hand from the classification. The two-way equality against +#: docs/PHI.md §2 immediately reported `[backup].retention_keep` and +#: `[retention].connection_event_retention_hours` as documented-but-absent. That is precisely the +#: failure this floor exists to catch, caught in the constant it protects, before either had ever run. +MIN_PHI_RETENTION_WINDOWS: Final[int] = 9 + + +def auto_bounded_windows() -> tuple[RetentionWindow, ...]: + """The windows a PHI instance silently defaults to 30 days when they are UNSET.""" + return tuple(w for w in PHI_RETENTION_WINDOWS if w.auto_bound_days is not None) + + +def warn_only_windows() -> tuple[RetentionWindow, ...]: + """The windows that are classified and WARNED but never silently bounded.""" + return tuple(w for w in PHI_RETENTION_WINDOWS if w.auto_bound_days is None) + + +def unbounded_windows(read: object) -> tuple[RetentionWindow, ...]: + """Windows whose configured value is genuinely unbounded on ``read`` (a loaded ``Settings``). + + Skips windows where ``0`` does not mean unbounded, and windows whose ``requires_setting`` + dependency is unmet — with no ``[logging].log_dir`` there is nothing for the app-log sweep to + sweep, so refusing or warning over it would fire on a knob that cannot act. + """ + out: list[RetentionWindow] = [] + for window in PHI_RETENTION_WINDOWS: + if not window.zero_is_unbounded: + continue + if window.requires_setting is not None: + section, dep = window.requires_setting + if not getattr(getattr(read, section, None), dep, None): + continue + section_obj = getattr(read, window.reads_from.strip("[]"), None) + value = getattr(section_obj, window.field, None) + if isinstance(value, int) and value <= 0: + out.append(window) + return tuple(out) diff --git a/tests/test_cli.py b/tests/test_cli.py index 12b3ec97..84a4e27d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1367,17 +1367,47 @@ def test_serve_ui_default_on_public_origin_degrades_json_only( # --- #186a secure-by-default data retention (bounded PHI-body windows) ---------------------------- -def test_serve_refuses_unbounded_messages_retention_in_prod( +def test_serve_auto_bounds_an_unset_body_window_in_prod( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: - # A production PHI instance with the inbound-body window unbounded refuses to start — PHI bodies - # would accumulate forever. Egress + SMTP pre-satisfied so only the retention gate decides. - rc, _ = _run_secure_serve( + """UNSET now DEFAULTS to 30 on the enforce dial rather than refusing (owner ruling 2026-07-30). + + This test previously asserted rc == 2 for exactly this config. The change is deliberate and the + trade is real: a production PHI instance with an unset window used to refuse to start, which forced + an operator to choose a number; it now starts bounded at 30. What survives is the fail-closed path + for an EXPLICIT 0 — see the refusal test below, which is the other half of this pair. + + Mutation: drop the auto-bound block — reds, because rc becomes 2 and the window stays 0. + """ + rc, captured = _run_secure_serve( tmp_path, monkeypatch, "security.block_unlisted_outbound = true\n[retention]\ndead_letter_days = 30\n" + _SECURE_ALERTS, ) + assert rc == 0 + assert captured["retention_settings"].messages_days == 30 # type: ignore[attr-defined] + err = capsys.readouterr().err + assert "[security].delete_message_bodies_after_days" in err and "defaulted ON (30 days)" in err + + +def test_serve_refuses_an_explicitly_disabled_body_window_in_prod( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """An EXPLICIT 0 still refuses — the fail-closed path the auto-bound narrows but does not remove. + + This is the half of the posture that must not regress: "unbounded by accident" is still prevented, + because typing 0 is a deliberate act and is still refused unless the audited opt-out is set. + + Mutation: treat an explicit 0 as unset (drop `model_fields_set`) — reds, because the window is + silently defaulted to 30 and rc becomes 0. + """ + rc, _ = _run_secure_serve( + tmp_path, + monkeypatch, + "security.block_unlisted_outbound = true\nsecurity.delete_message_bodies_after_days = 0\n" + "[retention]\ndead_letter_days = 30\n" + _SECURE_ALERTS, + ) assert rc == 2 err = capsys.readouterr().err assert "[security].delete_message_bodies_after_days" in err and "refusing to start" in err @@ -1393,7 +1423,7 @@ def test_serve_refuses_unbounded_dead_letter_retention_in_prod( tmp_path, monkeypatch, "security.block_unlisted_outbound = true\nsecurity.delete_message_bodies_after_days = 30\n" - + _SECURE_ALERTS, + "[retention]\ndead_letter_days = 0\n" + _SECURE_ALERTS, ) assert rc == 2 err = capsys.readouterr().err @@ -1418,7 +1448,14 @@ def test_serve_retention_auto_bounds_in_staging( assert retention.messages_days == 30 and retention.dead_letter_days == 30 # type: ignore[attr-defined] err = capsys.readouterr().err assert "defaulted ON (30 days)" in err - assert "accumulate without bound" not in err # auto-bounded, no longer merely warns + # The AUTO-BOUNDED windows must not appear in the warn-only line — that is what "auto-bound rather + # than merely warn" means. This used to assert the phrase was absent ENTIRELY, which worked only + # while the classification held nothing but auto-bounded windows; state_max_age_days and + # search_preset_days now legitimately warn, so the narrow assertion is the one that keeps testing + # the original intent instead of the coincidence. + warn_line = next((ln for ln in err.splitlines() if "accumulate without bound" in ln), "") + assert "[security].delete_message_bodies_after_days" not in warn_line, warn_line + assert "[retention].dead_letter_days" not in warn_line, warn_line assert "refusing to start" not in err diff --git a/tests/test_retention_classification_drift.py b/tests/test_retention_classification_drift.py new file mode 100644 index 00000000..aede287e --- /dev/null +++ b/tests/test_retention_classification_drift.py @@ -0,0 +1,242 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""`PHI_RETENTION_WINDOWS` and docs/PHI.md §2's Retention column must describe the SAME set. + +ASVS 14.2.7 wants retention derived from a data classification. The serve gate's tier list is built +from :data:`PHI_RETENTION_WINDOWS`; this test is what makes that constant *derived* rather than +hand-typed, by asserting two-way equality against the column. Add a PHI tier to the doc without adding +it here, or here without the doc, and this reds. + +**The parser is bounded STRUCTURALLY, and that is the whole difficulty.** §2 contains THREE contiguous +pipe blocks — a 2-column PL legend, the 7-column inventory, and a 4-column at-rest threat matrix. A +naive "every pipe row in §2" scan trips on the other two; and the natural repair (skip rows whose cell +count is wrong) DISARMS the cell-count check, so the drift test would then pass over a mangled table. +So the inventory is located by its own header and asserted UNIQUE, the cell-count check applies only +inside it, and there is a floor on the row count. +""" + +from __future__ import annotations + +import pathlib +import re + +import pytest + +from messagefoundry.config.retention_classification import ( + MIN_PHI_RETENTION_WINDOWS, + PHI_RETENTION_WINDOWS, + auto_bounded_windows, + warn_only_windows, +) +from messagefoundry.config.settings import ( + BackupSettings, + LoggingSettings, + RetentionSettings, + SecuritySettings, + StoreSettings, +) + +_PHI_MD = pathlib.Path(__file__).resolve().parents[1] / "docs" / "PHI.md" + +#: A backticked `[section].setting` — the machine-readable half of every bounded vocabulary form. +_SETTING = re.compile(r"`(\[[a-z_]+\]\.[a-z_]+)`") + +#: The prose forms, which carry NO backticked setting. Listed so an unrecognised cell is an error +#: rather than being silently read as "no window". +_PROSE_FORMS = ("UNBOUNDED — honest gap", "n/a — not PHI", "keep-forever by design") + + +def _inventory_rows() -> list[list[str]]: + """The §2 at-rest inventory as cell lists — located structurally, asserted unique.""" + lines = _PHI_MD.read_text(encoding="utf-8").splitlines() + headers = [i for i, line in enumerate(lines) if line.startswith("| Location | Backends |")] + assert len(headers) == 1, ( + f"expected exactly ONE §2 inventory header in docs/PHI.md, found {len(headers)} at lines " + f"{[i + 1 for i in headers]}. Zero means the table was renamed and this guard is asserting " + f"nothing; more than one means the scan would silently merge two tables." + ) + start = headers[0] + ncols = lines[start].count("|") - 1 + + rows: list[list[str]] = [] + i = start + 2 # skip the header and its separator + while i < len(lines) and lines[i].lstrip().startswith("|"): + line = lines[i] + cells = [c.strip() for c in line.strip().strip("|").split("|")] + assert len(cells) == ncols, ( + f"docs/PHI.md:{i + 1} has {len(cells)} cells, header has {ncols}. Do NOT 'fix' this by " + f"skipping mismatched rows — that disarms this check and lets a mangled table pass.\n" + f" {line[:120]}" + ) + rows.append(cells) + i += 1 + + # Floor + liveness receipt. A parser that silently matched nothing would make every assertion + # below pass vacuously, which is the exact failure mode this whole change set exists to remove. + assert len(rows) >= 35, ( + f"only {len(rows)} inventory rows parsed from docs/PHI.md §2 (expected ~38). The table shape " + f"changed or the scan is broken — either way the equality assertions below are worthless." + ) + return rows + + +def _documented_settings() -> set[str]: + """Every `[section].setting` named in the Retention column.""" + rows = _inventory_rows() + documented: set[str] = set() + for row in rows: + cell = row[-1] # Retention is the last column + found = _SETTING.findall(cell) + if found: + documented.update(found) + continue + assert any(form in cell for form in _PROSE_FORMS), ( + f"unrecognised Retention cell {cell!r}. Every cell must be one of the seven vocabulary " + f"forms — a bounded form naming a backticked `[section].setting`, or one of {_PROSE_FORMS}. " + f"An unparsed cell would otherwise read as 'no window' and silently drop a PHI tier out of " + f"the generated gate list." + ) + return documented + + +def test_the_constant_and_the_doc_describe_the_same_windows() -> None: + """Two-way equality. This is the control that makes the constant *derived*. + + Mutation: delete any entry from `PHI_RETENTION_WINDOWS` — reds on the doc-not-code side. Change a + §2 cell to name a window that does not exist — reds on the code-not-doc side. + """ + documented = _documented_settings() + declared = {w.setting for w in PHI_RETENTION_WINDOWS} + + doc_not_code = documented - declared + code_not_doc = declared - documented + assert not doc_not_code, ( + f"docs/PHI.md §2 classifies these windows but PHI_RETENTION_WINDOWS does not carry them: " + f"{sorted(doc_not_code)}. The serve gate is generated from the constant, so a tier documented " + f"only in the doc is NOT checked at startup." + ) + assert not code_not_doc, ( + f"PHI_RETENTION_WINDOWS carries these but docs/PHI.md §2 does not classify them: " + f"{sorted(code_not_doc)}. Either the doc lost a row or the constant invented coverage." + ) + + +#: `[section]` -> the settings model that owns it. The classification spans FIVE sections, not just +#: `[retention]`, so a check against one model would have to exempt the rest — and an exemption list is +#: how a field name silently stops being verified. +_SECTION_MODELS = { + "[retention]": RetentionSettings, + "[store]": StoreSettings, + "[backup]": BackupSettings, + "[security]": SecuritySettings, + "[logging]": LoggingSettings, +} + + +def test_every_declared_field_exists_on_its_own_settings_model() -> None: + """Each window's `field` must exist on the model for ITS section — no exemptions. + + The first version of this test checked every window against `RetentionSettings` and exempted + `[store]`/`[security]` by prefix. Adding `[backup].retention_keep` reded it, and the tempting fix + was to widen the exemption — which would have stopped verifying three of the five sections. Routing + each window to its own model instead makes the check STRONGER than before it failed. + + Mutation: rename any `field` — reds here, at import time, rather than in a serve path CI never runs. + """ + unknown_section = [ + w.setting for w in PHI_RETENTION_WINDOWS if w.reads_from not in _SECTION_MODELS + ] + assert not unknown_section, ( + f"windows in a section with no model mapping: {unknown_section} — add it to _SECTION_MODELS " + f"rather than exempting it, or the field name stops being checked" + ) + missing = [ + f"{w.setting} -> {_SECTION_MODELS[w.reads_from].__name__}.{w.field}" + for w in PHI_RETENTION_WINDOWS + if w.field not in _SECTION_MODELS[w.reads_from].model_fields + ] + assert not missing, f"windows naming a field their own settings model does not have: {missing}" + + +def test_every_requires_setting_resolves() -> None: + """A `requires_setting` naming a field that does not exist would make the gate skip forever. + + This is the `requires_setting` trap in its quiet form: the gate skips a window whose dependency is + unmet, so a MISSPELLED dependency is indistinguishable from an unmet one — the window is simply + never checked, and nothing reds. Resolve every pair against the real models. + """ + models = {"logging": LoggingSettings, "backup": BackupSettings, "store": StoreSettings} + for w in PHI_RETENTION_WINDOWS: + if w.requires_setting is None: + continue + section, field = w.requires_setting + assert section in models, f"{w.setting}: requires_setting names unknown section {section!r}" + assert field in models[section].model_fields, ( + f"{w.setting}: requires_setting ({section}, {field}) does not resolve — the gate would " + f"skip this window forever and report success" + ) + + +def test_the_floor_is_seven_not_six_and_is_enforced() -> None: + """`if not TUPLE` is not a floor — a merge dropping five of seven leaves a non-empty tuple. + + Seven because `reference_snapshot_days` made the classification-derived list seven entries; any + floor written as six was wrong the day the reference purge landed. + + Mutation: set MIN to 6 — reds, because the real length is checked against it AND against the + documented reason for the value. + """ + assert MIN_PHI_RETENTION_WINDOWS == 9, ( + "the floor changed. It is 9 — and that number was CORRECTED BY THIS TEST rather than counted: " + "it was first written as 7, and the two-way equality above immediately reported " + "[backup].retention_keep and [retention].connection_event_retention_hours as " + "documented-but-absent. If the classification genuinely grew or shrank, update this assertion " + "and say why in the same commit." + ) + assert len(PHI_RETENTION_WINDOWS) >= MIN_PHI_RETENTION_WINDOWS, ( + f"PHI_RETENTION_WINDOWS has {len(PHI_RETENTION_WINDOWS)} entries, floor is " + f"{MIN_PHI_RETENTION_WINDOWS} — a startup gate built from this would check fewer windows than " + f"the classification defines while still reporting success." + ) + + +def test_the_auto_bound_and_warn_only_split_matches_the_owner_ruling() -> None: + """The owner ruled which windows silently default and which only warn (2026-07-30). + + Auto-bounding a window whose timestamp moves only on a WRITE deletes live operational data — + `state.set_at` is never refreshed by `state_get`, so a long-lived correlation map would vanish + mid-stream. This asserts the split rather than trusting a comment. + + Mutation: give `state_max_age_days` an `auto_bound_days=30` — reds. + """ + auto = {w.setting for w in auto_bounded_windows()} + warn = {w.setting for w in warn_only_windows()} + assert auto == { + "[security].delete_message_bodies_after_days", + "[retention].dead_letter_days", + "[retention].reference_snapshot_days", + }, f"auto-bound set drifted from the owner ruling: {sorted(auto)}" + assert "[retention].state_max_age_days" in warn, ( + "state_max_age_days must NOT auto-bound — purge_state deletes by `set_at`, which a read never " + "refreshes, so a Handler's correlation map would be deleted while still in use" + ) + assert "[retention].search_preset_days" in warn, ( + "search_preset_days must NOT auto-bound — pipeline/retention.py forbids exactly that " + "inheritance in prose, and no test anchors that comment" + ) + assert not (auto & warn), "a window cannot be both auto-bound and warn-only" + + +@pytest.mark.parametrize("form", _PROSE_FORMS) +def test_each_prose_form_is_actually_used(form: str) -> None: + """The vocabulary must not contain dead forms. + + A form nothing uses is either a classification nobody applied or a leftover — both mean the legend + describes a scheme the table does not follow. `UNBOUNDED — honest gap` in particular MUST appear: + this codebase has unbounded PHI tiers, and a classification with none is flattering, not complete. + """ + cells = [row[-1] for row in _inventory_rows()] + assert any(form in cell for cell in cells), ( + f"the vocabulary form {form!r} appears in no §2 Retention cell. If a classification genuinely " + f"no longer applies, remove it from the legend in the same commit." + ) From 1cd441b113b56d961b9f4abd5a4f44308f579a07 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 13:53:55 -0500 Subject: [PATCH 7/8] fix(store): the legacy SQL Server `outbox` table held PHI nothing could purge (14.2.7) `outbox` was recreated by `_SCHEMA` on every open and read by NOTHING -- the staged pipeline and every delivery-side method use `queue`. So on a store upgraded from the pre-staged-pipeline layout, `outbox.payload` (a full transformed PHI body) was reached by no purge on any backend: `purge_old_messages` and `purge_dead_letters` both scope to `queue`. Meanwhile `messages.raw` blanked on its own window, so the message READ as purged while the body survived indefinitely. It also sat outside `reencrypt_to_active`, so a rotation that retired the old key left those bodies undecryptable. Both close the same way: a guarded `_SCHEMA` statement folds surviving rows into `queue` as stage='outbound' -- payload carried over VERBATIM, so encryption at rest is preserved -- and then DROPs the table. Migrated rows become ordinary outbound queue rows: bounded by [security].delete_message_bodies_after_days / [retention].dead_letter_days, swept by the on-open cipher migration, rotated by reencrypt_to_active. SQLite already did the equivalent (`_migrate_outbox_to_queue`); Postgres never had the table. The three `outbox` DDL statements are gone, so a fresh store never creates it again. THREE PLACEMENT DECISIONS, each load-bearing rather than stylistic: * It lives in `_SCHEMA`, not an open-path method like SQLite's. `_schema_hash()` is the ONLY thing deciding whether the DDL batch runs -- a current marker skips it entirely -- so migration code outside `_SCHEMA` would be invisible to that decision and would never fire on an upgrade. It also inherits the batch's `raw.timeout = 0`, so a large legacy backlog cannot be killed mid-INSERT into a rollback-and-re-fail crash-loop. * It sits AFTER the `queue` DDL, not where the `outbox` DDL was. SQL Server defers name resolution, so an earlier placement would parse fine and then fail at RUN time on a legacy DB old enough to have `outbox` but not `queue`. * Two filters guard the INSERT. EXISTS(messages) skips FK orphans -- unreplayable anyway, but one would abort the batch with an opaque FK error. NOT EXISTS(queue) makes a PK collision a no-op instead of a 2627 that rolls back and re-fails on every restart, and makes a partially-applied migration safely re-runnable. VERIFIED AGAINST A REAL SQL SERVER (2022 CU25 in Docker), not just asserted. The new live suite simulates an UPGRADE rather than a fresh open, because a fresh open never executes the migration at all (the existence guard is false) and a re-open with a current `schema_meta` marker skips the whole batch -- both would have passed while measuring nothing. Each test seeds a real legacy table and clears the marker, which is the state a genuine upgrade presents. All three filters were then mutation-proved against the live database: dropping the DROP reds 3 tests, removing EXISTS(messages) reds the orphan test with an IntegrityError, removing NOT EXISTS(queue) reds the collision test with an IntegrityError. FIFO on a migrated backlog was MEASURED, not assumed: rows INSERTed into `outbox` in the exact reverse of their created_at order came out with `seq` ascending by created_at. It is still not a documented SQL Server guarantee, so the comment says so and no test asserts it -- a flaky gate on a best-effort property is worse than none. `("outbox", "payload")` is removed from `_encrypt_existing_rows`; leaving it would have failed every KEYED open with *Invalid object name*. No CI leg would have caught that -- every SQL Server leg runs keyless and returns before the sweep -- which is why the AST guard from the previous commit exists. Mutation-proved: re-adding the entry reds it, naming the line. 27 `"outbox"` entries removed from reset lists across 24 files. Nine of those files run on NO SQL Server CI step, so "the SS leg is green" would not have been evidence; the source scanner is what covers them. Its `xfail(strict=True)` self-cleared on the last fix -- XPASS, which strict reports as a failure -- so removing the marker here was forced rather than remembered. The `test_no_engine_code_still_references_the_retired_table` sweep was rewritten mid-change after it correctly fired on BOTH migrations. A flat ban is unsatisfiable (a migration must name the table it migrates), but a plain file exemption would hide a stray read inside those same files forever. It now pins the reference COUNT per migration module and asserts each still contains the DROP, so a new `FROM outbox` in `store/sqlserver.py` reds on the count. Path keys are POSIX-normalised: native separators would have keyed on backslashes and missed on ubuntu -- a guard red on one OS and green on another. docs/PHI.md: the section 2 inventory row and the section 8 gap row are removed rather than reworded, because the at-rest location no longer exists. The per-backend cipher counts drop to 18/17/17 in both places they are stated -- and those are DERIVED from live code by test_phi_at_rest_inventory.py, which is what reported the change rather than my counting. The two registered retired claims ("touched by no purge on any backend", "is purged by nothing on any backend") now go green, which is what they were registered a commit early to prove. The bare "no purge on any backend" is still NOT registered, and the reason CHANGED without changing the answer: it used to collide with the legacy row, and now collides with this commit's own past-tense account of what the retirement fixed. A phrase whose collision merely MOVED is not a phrase that became safe to register. CI WIRING, without which the live suite above would have been decorative. CI's SQL Server steps run an explicit FILE LIST, not the whole suite, so a new MEFOR_TEST_SQLSERVER-gated file that nobody appends runs in NO environment: it skips on the plain legs, and a skip reads exactly like a pass. The new file is appended to the catch-all SQL Server step, and that step's comment now says it is the catch-all for ANY gated file rather than only throughput levers. Measuring that gap turned up a larger one, filed rather than fixed here: 17 test files are wholly skipped without a live SQL Server, and ELEVEN of them are named in no CI step at all. That is why several of the pre-existing failures below had rotted unnoticed -- nothing runs them anywhere. Unrelated pre-existing failures found and filed, NOT introduced here. A full local suite showed 29 failures; 25 were proven pre-existing by stashing this change out and re-running (the identical set failed). Of those, a large share are LOCAL-ONLY artifacts of exporting the SQL Server env vars across the WHOLE suite, which CI never does -- it sets them per step for a named file list. MEFOR_ALLOW_INSECURE_TLS=1 in particular defeats refusals that test_tls_trust_anchor and test_hop_refusal_rawtcp assert; test_audit_integrity.py goes 29/29 green on a clean env. Separately, 5 of 6 tests in test_adr0114_claim_proc_live.py fail against a live SQL Server (claim_proc_effective is False; the deployed proc body does not match the shipped definition, and the warning's own DELETE FROM schema_meta remedy does not fix it) -- also confirmed pre-existing the same way, and one of the eleven files that run nowhere. --- .github/workflows/ci.yml | 8 + docs/PHI.md | 38 +- harness/load/connscale/runner.py | 6 +- harness/load/shardcert.py | 1 - messagefoundry/store/sqlserver.py | 77 +++- .../test_adr0071_dispatch_wiring_sqlserver.py | 1 - .../test_adr0071_fused_callables_sqlserver.py | 1 - tests/test_adr0075_batch_sqlserver.py | 1 - tests/test_adr0114_claim_proc_live.py | 2 +- tests/test_batch_claim_fifo.py | 1 - tests/test_batch_claim_locking.py | 1 - tests/test_batch_completion.py | 2 +- tests/test_claim_fifo_heads.py | 1 - tests/test_cluster_failover_postgres.py | 2 +- tests/test_cluster_failover_sqlserver.py | 2 +- tests/test_fifo_index_migration.py | 1 - tests/test_fixture_outbox_reset.py | 123 ++++++- tests/test_load_failover_postgres.py | 1 - tests/test_load_failover_sqlserver.py | 1 - tests/test_metadata_bag.py | 2 +- tests/test_outbound_batch.py | 2 +- tests/test_phi_at_rest_inventory.py | 25 +- tests/test_pooled_rider.py | 1 - tests/test_seq_only_fifo.py | 1 - tests/test_shard_recovery_sqlserver.py | 1 - .../test_sqlserver_legacy_outbox_migration.py | 331 ++++++++++++++++++ tests/test_sqlserver_store.py | 7 +- tests/test_sqlserver_sync_handoff.py | 1 - tests/test_stage_dispatcher.py | 1 - tests/test_x12_rte.py | 1 - 30 files changed, 566 insertions(+), 77 deletions(-) create mode 100644 tests/test_sqlserver_legacy_outbox_migration.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 25327fb2..d2626669 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -678,6 +678,13 @@ jobs: # suites above never picked them up and they ran only on SQLite. Run them here against the real # backend. APPEND each new lever's test file per docs/throughput-build-plan.md. # + # NOT only throughput levers: this step is the catch-all for ANY MEFOR_TEST_SQLSERVER-gated file + # the named suites above do not collect. CI runs an explicit file LIST here, not the whole suite, + # so a new gated file that nobody appends never runs in CI at all — it just reports as skipped on + # the plain legs, which reads identical to passing. `tests/test_sqlserver_legacy_outbox_migration.py` + # (ASVS 14.2.7) is here for exactly that reason: it verifies a one-way schema migration that can + # only be observed against a real server. + # # RETRY WRAPPER: pyodbc 5.3.0 intermittently segfaults in its C parameter-binding path under # py3.14 against the SQL Server 2025 container (upstream mkleehammer/pyodbc#1459, unfixed; 5.3.0 # is the newest pyodbc and the first with py3.14 wheels, so there is nothing to bump to). The @@ -698,6 +705,7 @@ jobs: tests/test_per_lane_wake.py tests/test_stage_dispatcher.py tests/test_pooled_rider.py + tests/test_sqlserver_legacy_outbox_migration.py - name: Run the RTE capture / re-ingress loop on real SQL Server (ADR 0013/0016) env: diff --git a/docs/PHI.md b/docs/PHI.md index fb310d8d..200102be 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -95,7 +95,6 @@ destruction) are documented in [§3](#3-encryption-at-rest) under the matching h | `attachment_chunk.ciphertext` (ADR 0105 / #149) | all three | **Yes** — one slice of a detached very-large document (e.g. a base64 PDF from OBX-5.5) | **Yes, when a key is set** — store cipher, **sealed per chunk on write**; AAD `("attachment_chunk","ciphertext",attachment_id,seq)`; store DEK | **PL-1** | The document is detached at ingress, content-addressed (`sha256` of the verbatim plaintext) and chunked; the message keeps only an `mfdoc:v1:ref:` handle. Read back via `GET /messages/{id}/attachments/{id}` (§3) | ``rides `[security].delete_message_bodies_after_days` `` | | `attachment` header row (`content_type`, `total_bytes`, `refcount`, `created_at`) + `message_attachment` linkage | all three | **No** — size/type/linkage only | No (metadata, deliberately not ciphered) | **PL-4** | The linkage row is the security crux of the download route: it scopes a content address to a message the caller may already read | ``rides `[security].delete_message_bodies_after_days` `` | | `response.body` (ADR 0013 captured replies; ADR 0021 `kind='ack_sent'`) | all three | **Yes** — the partner's reply body, or the ACK/NAK the engine returned | **Yes, when a key is set** — store cipher; AAD `("response","body",message_id,destination_name,response_seq)`; store DEK | **PL-1** | Composite PK, so it rides its own migration/rotation pass. An **ACK body is stored only when the store cipher is active** — on a keyless store it is `NULL` rather than plaintext (fail-safe), and a NAK never stores a body at all | ``rides `[security].delete_message_bodies_after_days` `` | -| `outbox.payload` (**legacy**, pre-staged-pipeline schema) | **SQL Server only** | **Yes** — a full outbound body on a store upgraded from the flat-`outbox` schema | **Yes, when a key is set** — store cipher (on-open migration only); AAD `("outbox","payload",id)`; store DEK | **PL-1** | SQL Server's schema pass re-runs a **create-if-absent** `IF OBJECT_ID('outbox','U') IS NULL CREATE TABLE outbox` on every open, so the table always exists and an existing one is never re-created or emptied — and it is never migrated away or dropped (SQLite migrates its rows into `queue` then `DROP`s it; Postgres has no such table). Empty on a greenfield install. **Three honest gaps:** it is absent from `reencrypt_to_active`, so a key rotation followed by dropping the retired key leaves those bodies undecryptable; `outbox.last_error` is in neither the migration nor the rotation pass; and **no purge path on any backend touches it** ([§8](#8-retention--purge)) | `UNBOUNDED — honest gap` | | `[store].uploads_dir/*.blob` + `*.meta` — the cipher cells `uploaded_file.body` / `uploaded_file.meta` (offline uploaded logs, ADR 0134) | all (filesystem, not the DB) | **Yes** — an operator-uploaded diagnostic message file, held for offline browsing decoupled from any connection | **Yes, when a key is set** — the **same store cipher** (`build_store_cipher`); AAD `("uploaded_file","body")` / `("uploaded_file","meta")` + `file_id`; store DEK. **identity/plaintext-on-disk otherwise** (the File-connector-spill tier below) | **PL-1** | A PHI-at-rest location **outside** the message store, opt-in (unset ⇒ the subsystem is disabled — no surface). On-disk identity is a random 32-hex `file_id` (path-traversal guard); the operator filename is display-only. Every access is `files:*`-gated, browse is step-up + PHI-hop-guarded, all audited (metadata only). **Not** re-encrypted by `rotate-key` (outside the store) — stays readable via the decrypt keyring. The dir is created `0o700` and the sidecar written `0o600` **best-effort, and both are no-ops on Windows** — the engine applies no ACL here (it does not call the `icacls` enforcer). **Retention + quotas (ASVS 5.2.4):** uploaded files auto-prune after `[store].uploads_retention_days` (default **30**) — swept opportunistically at save time and by a periodic task; every prune is audited (`upload.prune`, file_id + uploader only, never content). Per-uploader caps `[store].max_upload_files_per_user` (default **100**) / `[store].max_upload_total_bytes_per_user` (default **250 MiB**) bound the at-rest volume; a would-be over-quota upload is refused **HTTP 409** with an `upload.reject_quota` audit before anything is written (defaults-ON, `ge=1` floors; quota is per-process per-`uploads_dir`). Harden the dir + volume encryption ([§10](#10-secure-deployment--operations-checklist)) | `` `[store].uploads_retention_days` `` | | `[backup].destination/mefor-backup-*.mfbak` (ADR 0049 DR backup) | **SQLite only** carries bodies | **SQLite: Yes** — a consistent store snapshot (full inbound + outbound bodies) + the config bundle. **SQL Server / Postgres: No** — config bundle only | **Yes** — `.mfbak` chunked-AEAD codec under the **store DEK** (`resolve_active_key`); an identity-cipher (no-key) box is **refused** unless `[backup].allow_unencrypted` writes a `.mfbak.plain` | **PL-1** (SQLite) / **PL-4** (server backends) | On a **server-DB store `snapshot_to` raises `DbaDelegatedError`**, so the BackupRunner writes a **config-only** archive — or skips entirely when `[backup].config_only_on_server_db = false`. There is therefore **no `.mfbak` containing message bodies on SQL Server or Postgres**; the DB-tier backup there is `BACKUP DATABASE` / Always On / `pg_dump` / PITR, infra-owned. Where bodies *are* present it is a second at-rest PHI copy, bounded by keep-N retention; like `uploads_dir` it is **not** re-encrypted by `rotate-key`. The share's own ACLs are infra-owned | ``keep-N `[backup].retention_keep` `` | | `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs (OS temp dir, ADR 0049) | SQLite carries bodies; server backends config-only | **Yes** — a full store snapshot, and on verify a **decrypted** archive | **No** — the snapshot keeps the store's own column cipher, but the staging tar and the verify extraction are **plaintext on disk**; no engine ACL (`_secure_file` is never called on these paths) | **PL-1** | `run_backup` snapshots the store to `/store.db` and tars it **plaintext** before sealing it into the `.mfbak` (`pipeline/dr_backup.py`), and `[backup].verify_after_backup` (**default `true`**) decrypts the archive straight back out to a second temp dir on **every** run — independent of `full_restore_verify`. Transient (the `TemporaryDirectory` unlinks on exit) but **not** on a crash or `SIGKILL`. Lives under `%TEMP%` / `TMPDIR`, **not** the ACL'd data dir: cover the temp volume with FDE and point `TMP`/`TMPDIR` at an owner-only path ([§10](#10-secure-deployment--operations-checklist)) | `UNBOUNDED — honest gap` | @@ -149,14 +148,26 @@ destruction) are documented in [§3](#3-encryption-at-rest) under the matching h > contain no backticked setting at all. **Per-backend cipher coverage, stated exactly.** The store cipher covers **18** `(table, column)` -pairs on SQLite. **SQL Server** covers 18 = the SQLite set **minus** `shared_body.body` (never written -there) **plus** the legacy `outbox.payload`. **Postgres** covers 17 = the SQLite set **minus** -`shared_body.body`. Two further asymmetries are real and worth knowing. **(1)** Neither SQL Server nor -Postgres sweeps `attachment_chunk` in its *on-open* plaintext→cipher migration (only in `rotate-key`), -which is harmless today because `put_attachment` always seals on write, but means a legacy -no-key→key transition would not sweep chunks the way SQLite's does. **(2)** SQL Server's legacy -`outbox.payload` is swept by the on-open migration but is **absent from `reencrypt_to_active`**, so a -rotation that retires the old key leaves those bodies undecryptable (the row above says the same). +pairs on SQLite. **SQL Server** covers 17 = the SQLite set **minus** `shared_body.body` (never written +there). **Postgres** covers 17 = the SQLite set **minus** `shared_body.body`. SQL Server's count was +18 until the legacy `outbox.payload` was retired (below); the two server backends now carry the same +set. One asymmetry remains and is worth knowing: neither SQL Server nor Postgres sweeps +`attachment_chunk` in its *on-open* plaintext→cipher migration (only in `rotate-key`), which is +harmless today because `put_attachment` always seals on write, but means a legacy no-key→key +transition would not sweep chunks the way SQLite's does. + +**The legacy SQL Server `outbox` table is gone (ASVS 14.2.7), and that closed two real gaps.** It was +recreated by the schema pass on every open and read by nothing, so a store upgraded from the +pre-staged-pipeline layout kept full outbound PHI bodies there that no purge on any backend reached — +while `messages.raw` blanked on its own window, so the message read as purged. It also sat outside +`reencrypt_to_active`, so a key rotation that retired the old key left those bodies undecryptable. +Both close the same way: a guarded statement in the SQL Server schema batch folds surviving rows into +`queue` as `stage='outbound'` — payload carried over verbatim, so encryption at rest is preserved — +and then `DROP`s the table. Migrated rows are ordinary outbound queue rows, bounded by +`[security].delete_message_bodies_after_days` / `[retention].dead_letter_days`, swept by the on-open +cipher migration and rotated by `reencrypt_to_active`, so the tier no longer needs its own row here. +SQLite already performed the equivalent migration (`_migrate_outbox_to_queue`); Postgres never had the +table. **The body cipher `[BUILT]`.** Each backend routes the columns above through the store's `_cipher` ([store/crypto.py](../messagefoundry/store/crypto.py)) on write/read — **AES-256-GCM when a store key @@ -246,7 +257,7 @@ for defense-in-depth without swapping the `aiosqlite` connector. 1. **Application-level AES-256-GCM `[BUILT]`.** The store's `_cipher` ([store/crypto.py](../messagefoundry/store/crypto.py)) encrypts the cipher-covered columns. **[§2](#2-where-phi-lives--data-at-rest-inventory) is the normative list** — 18 `(table, column)` - pairs on SQLite, 18 on SQL Server, 17 on Postgres, plus the two `uploaded_file` sidecar cells. The + pairs on SQLite, 17 on SQL Server, 17 on Postgres, plus the two `uploaded_file` sidecar cells. The per-level blocks below group that same set; they do not redefine it, and CI pins the counts **and** the membership of both sections to the store's cipher registry, so they cannot diverge. Stored format `mfenc:v1 ‖ key_id ‖ base64(nonce ‖ ciphertext ‖ GCM tag)` — the GCM tag @@ -378,7 +389,7 @@ a statement about *what is built today*; where a control does not exist, it says #### PL-1 · PHI body **Applies to:** `messages.raw` · `queue.payload` · `shared_body.body` · `attachment_chunk.ciphertext` · -`response.body` · `outbox.payload` (SQL Server legacy) · `[store].uploads_dir` blobs +`response.body` · `[store].uploads_dir` blobs (`uploaded_file.body` / `uploaded_file.meta`) · `.mfbak` archives (SQLite) · `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs (OS temp dir) · File-connector spill dirs · application log files (`[logging].log_dir`). @@ -426,7 +437,9 @@ application log files (`[logging].log_dir`). by `purge_dead_letters`; `response.body` is set to `NULL` in place by the same pass; `shared_body.body` is refcount-decremented and GC'd at 0 when its **last** referrer is purged; streaming attachments are decref'd + GC'd at 0 (plus a startup `sweep_orphan_attachments`). - **`outbox.payload` is purged by nothing on any backend**, while `[store].uploads_dir` blobs auto-prune + The legacy SQL Server `outbox.payload` no longer exists to be purged — its rows were folded into + `queue` and the table DROPped ([§2](#2-where-phi-lives--data-at-rest-inventory)), so they now ride the + same window as any other outbound row. `[store].uploads_dir` blobs auto-prune after `[store].uploads_retention_days` (default **30**) via an age-based sweep — a periodic `UploadRetentionRunner` plus an opportunistic pass at save time, each pruned pair audited `upload.prune` (ASVS 5.2.4, #291); an operator `DELETE` is an additional removal path. **`.mfbak` archives** are bounded @@ -1076,7 +1089,6 @@ window purely so a replay can be richer — is precisely the defect ASVS 14.2.7 | Tier | Status | |---|---| | `reference.value` (a **declared** set) | **orphan-only** coverage. `purge_reference_snapshots` bounds a set config has DROPPED; a set still declared is never purged whatever its age, because its snapshot is live data. The wired-set case remains unbounded — the honest gap | -| `outbox.payload` (SQL Server legacy) | recreated on every open, **touched by no purge on any backend** — legacy PHI there is retained forever | | `queue.payload` (stage=`ingress`/`routed`, **DEAD**) | **no purge reaches it** — added 2026-07-30 by the ASVS 14.2.7 classification sweep, which is the only reason it was found. The happy path consumes these rows at `route_handoff`/`transform_handoff`, so they are documented as transient — but a router or handler content fault calls `dead_letter_now` (`pipeline/wiring_runner.py:4415`, `:4449`), which sets status/`last_error` and **never touches `payload`**, while both purges are scoped `Stage.OUTBOUND` (`store/store.py:8384`, `:8659`). So `messages.raw` blanks on its window and the message reads as purged while a **full raw PHI body survives here indefinitely**. Filed as its own defect; the likely fix is to let a DEAD ingress/routed row ride `[retention].dead_letter_days` exactly as a dead outbound row does | | `mefor-backup-*` / `mefor-tar-*` / `mefor-verify-*` staging dirs | no window — a `TemporaryDirectory` unlinks on exit, but **not** on a crash or `SIGKILL`, and `verify_after_backup` (default `true`) decrypts a full archive back out to a second temp dir on **every** run. The engine applies **no ACL** on these paths (`_secure_file` is never called on them, and on a server-DB store — the deployed posture — it applies no file ACL at all), so the cover is the operator's: FDE on the temp volume plus `TMP`/`TMPDIR` pointed at a directory the OS restricts to the service account ([§10](#10-secure-deployment--operations-checklist)) | | `users.totp_secret` | no window by design — it lives and dies with the user row | diff --git a/harness/load/connscale/runner.py b/harness/load/connscale/runner.py index a08fd2bb..74bc7cea 100644 --- a/harness/load/connscale/runner.py +++ b/harness/load/connscale/runner.py @@ -72,14 +72,14 @@ # to). On a SERVER backend every (mode, count) step shares ONE database, so — mirroring the SQLite # fresh-file-per-step path — each step first EMPTIES these so the pooled arm never runs second against # the per_lane arm's residue (the carryover confound). Table lists verified against the store schemas -# (store/sqlserver.py, store/postgres.py) and match the reset the store tests use: SQL Server carries -# the legacy `outbox` table alongside the unified `queue`; the Postgres backend has no `outbox`. +# (store/sqlserver.py, store/postgres.py) and match the reset the store tests use. Neither backend has +# an `outbox` table: SQL Server's legacy one was folded into `queue` and DROPped (ASVS 14.2.7), and +# Postgres never had one. Naming it here would fail the reset with *Invalid object name*. _SQLSERVER_PIPELINE_TABLES = ( "message_events", "queue", "response", "delivered_keys", - "outbox", "messages", ) diff --git a/harness/load/shardcert.py b/harness/load/shardcert.py index 0be15449..a151c70a 100644 --- a/harness/load/shardcert.py +++ b/harness/load/shardcert.py @@ -1180,7 +1180,6 @@ async def _reset_store(env: Mapping[str, str]) -> None: cur = await conn.cursor() for table in ( "queue", - "outbox", "response", "delivered_keys", "state", diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 609237f7..9ae433d1 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -993,22 +993,13 @@ def __init__(self, conn: Any, cur: Any) -> None: CREATE INDEX ix_messages_channel ON messages(channel_id, received_at)""", """IF INDEXPROPERTY(OBJECT_ID('messages'),'ix_messages_control','IndexID') IS NULL CREATE INDEX ix_messages_control ON messages(channel_id, control_id)""", - """IF OBJECT_ID('outbox','U') IS NULL CREATE TABLE outbox ( - id NVARCHAR(64) NOT NULL PRIMARY KEY, message_id NVARCHAR(64) NOT NULL, - channel_id NVARCHAR(256) NOT NULL, destination_name NVARCHAR(256) NOT NULL, - payload NVARCHAR(MAX) NOT NULL, status NVARCHAR(32) NOT NULL, - attempts INT NOT NULL DEFAULT 0, next_attempt_at FLOAT NOT NULL, last_error NVARCHAR(MAX) NULL, - created_at FLOAT NOT NULL, updated_at FLOAT NOT NULL)""", - """IF INDEXPROPERTY(OBJECT_ID('outbox'),'ix_outbox_ready','IndexID') IS NULL - CREATE INDEX ix_outbox_ready ON outbox(status, next_attempt_at)""", - """IF INDEXPROPERTY(OBJECT_ID('outbox'),'ix_outbox_message','IndexID') IS NULL - CREATE INDEX ix_outbox_message ON outbox(message_id)""", # Unified staged queue (ADR 0001) — ingress -> routed -> outbound, one row per stage-unit with a - # `stage` discriminator. The SQL Server backend originally shipped only the flat `outbox`; the - # staged pipeline (enqueue_ingress + the handoffs) and ALL delivery-side methods now read/write - # this table (outbox is retained only for legacy read-compat). `seq` IDENTITY is the FIFO - # insertion-order tiebreak (PG uses BIGSERIAL); owner/lease_expires_at are present for parity but - # written NULL on this single-node backend (reset_stale_inflight is the recovery path). + # `stage` discriminator. The SQL Server backend originally shipped a flat `outbox` table; it was + # RECREATED by this very `_SCHEMA` on every open and read by nothing (the staged pipeline and every + # delivery-side method read/write `queue`), so any legacy PHI body left in it was retained forever + # with no purge on any backend — see the migrate-and-drop below, and docs/PHI.md §2. `seq` IDENTITY + # is the FIFO insertion-order tiebreak (PG uses BIGSERIAL); owner/lease_expires_at are present for + # parity but written NULL on this single-node backend (reset_stale_inflight is the recovery path). """IF OBJECT_ID('queue','U') IS NULL CREATE TABLE queue ( id NVARCHAR(64) NOT NULL PRIMARY KEY, seq BIGINT IDENTITY(1,1) NOT NULL, message_id NVARCHAR(64) NOT NULL, stage NVARCHAR(16) NOT NULL, @@ -1069,6 +1060,54 @@ def __init__(self, conn: Any, cur: Any) -> None: # Sch-M lock on the hot queue table (review). lock_escalation 2 = DISABLE. """IF (SELECT lock_escalation FROM sys.tables WHERE object_id=OBJECT_ID('queue')) <> 2 ALTER TABLE queue SET (LOCK_ESCALATION = DISABLE)""", + # ASVS 14.2.7 — fold the legacy flat `outbox` into `queue` (stage=outbound) and DROP it. + # + # WHY THIS IS A PHI FIX AND NOT A TIDY-UP. The table was recreated on every open and read by + # nothing, so `outbox.payload` — a FULL transformed PHI body — was **purged by nothing on any + # backend**: `purge_old_messages` and `purge_dead_letters` both scope to `queue`. A store upgraded + # from the pre-staged-pipeline layout therefore kept every legacy body forever while `messages.raw` + # blanked on its window, so the message READ as purged. Migrating the rows in puts them under + # `[security].delete_message_bodies_after_days` / `[retention].dead_letter_days` like any other + # outbound row; dropping the table is what makes the retirement true rather than merely documented. + # + # WHY IT LIVES IN `_SCHEMA` AND NOT AN OPEN-PATH METHOD (the SQLite backend uses a method, + # `_migrate_outbox_to_queue`): on this backend `_schema_hash()` is the ONLY thing that decides + # whether the DDL batch runs at all — a current marker skips it entirely — so migration code + # outside `_SCHEMA` would be invisible to that decision. See `_schema_hash`'s docstring. + # + # WHY IT IS PLACED HERE, after the `queue` DDL rather than where the `outbox` DDL used to be: + # SQL Server defers name resolution, so an earlier placement would PARSE fine and then fail at RUN + # time against a legacy DB old enough to have `outbox` but not yet `queue` — a failed open, which + # under the batch's single transaction is a startup crash-loop, not a degraded start. + # + # The two filters are each load-bearing: + # * EXISTS(messages) — `fk_queue_message` is enforced, and an orphan row (a `message_id` with no + # surviving message) would abort the whole batch with an opaque FK error. It was unreplayable + # anyway: nothing can view or route it. Same call the SQLite migration makes. + # * NOT EXISTS(queue) — `queue.id` is the PK. Ids are uuid4 so a collision is not a real + # expectation, but the cost of being wrong is a 2627 that rolls back the batch and re-fails on + # every restart. A cheap anti-join buys immunity from that, and also makes a partially-applied + # migration (rolled back after the INSERT, before the marker) safely re-runnable. + # ORDER BY created_at feeds the `seq` IDENTITY in arrival order so a legacy backlog drains FIFO + # (ADR 0059 orders a lane by `seq` alone). MEASURED, not assumed: against SQL Server 2022 CU25, + # rows INSERTed into `outbox` in the exact reverse of their created_at order came out of the + # migration with `seq` ascending by created_at. It is still not a documented guarantee — a parallel + # plan over a large backlog may assign otherwise — so it is deliberately not asserted by a test + # that would then be flaky. Each row's own created_at is carried over verbatim either way, so a + # reorder costs ordering, never data. + f"""IF OBJECT_ID('outbox','U') IS NOT NULL + BEGIN + INSERT INTO queue (id, message_id, stage, channel_id, destination_name, payload, + status, attempts, next_attempt_at, last_error, created_at, updated_at) + SELECT o.id, o.message_id, '{Stage.OUTBOUND.value}', o.channel_id, o.destination_name, + o.payload, o.status, o.attempts, o.next_attempt_at, o.last_error, + o.created_at, o.updated_at + FROM outbox o + WHERE EXISTS (SELECT 1 FROM messages m WHERE m.id = o.message_id) + AND NOT EXISTS (SELECT 1 FROM queue q WHERE q.id = o.id) + ORDER BY o.created_at, o.id; + DROP TABLE outbox; + END""", # #47/ADR 0042: messages.documents_pruned (the "embedded doc evicted vs never present" flag). NULL on # existing rows = never pruned; COL_LENGTH-gated like the others so a re-open is a no-op. """IF COL_LENGTH('messages','documents_pruned') IS NULL @@ -2224,10 +2263,16 @@ async def _encrypt_existing_rows(self) -> None: # recognised as already-encrypted and skipped — never re-wrapped. like = f"{_ENC_MARKER_PREFIX}%" total = 0 + # `("outbox", "payload")` was here until the legacy table was migrated + DROPped (ASVS + # 14.2.7). Leaving it would fail this pass with *Invalid object name 'outbox'* on EVERY keyed + # open — and the keyed path does not run in CI (every SQL Server leg is keyless), so + # `tests/test_sqlserver_encrypt_pass_tables.py` is the only mechanism that can catch a stale + # entry here. Rows the migration folded in are covered by `("queue", "payload")` below: the + # migration carries the payload over VERBATIM, so a legacy plaintext body arrives unencrypted + # and this pass — which runs after `_ensure_schema` — seals it on the same open. for table, column in ( ("messages", "raw"), ("queue", "payload"), - ("outbox", "payload"), ( "users", "totp_secret", diff --git a/tests/test_adr0071_dispatch_wiring_sqlserver.py b/tests/test_adr0071_dispatch_wiring_sqlserver.py index fa3d445f..26df7299 100644 --- a/tests/test_adr0071_dispatch_wiring_sqlserver.py +++ b/tests/test_adr0071_dispatch_wiring_sqlserver.py @@ -78,7 +78,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_adr0071_fused_callables_sqlserver.py b/tests/test_adr0071_fused_callables_sqlserver.py index 0039385a..cf11c300 100644 --- a/tests/test_adr0071_fused_callables_sqlserver.py +++ b/tests/test_adr0071_fused_callables_sqlserver.py @@ -62,7 +62,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_adr0075_batch_sqlserver.py b/tests/test_adr0075_batch_sqlserver.py index 27803fa9..4689abf2 100644 --- a/tests/test_adr0075_batch_sqlserver.py +++ b/tests/test_adr0075_batch_sqlserver.py @@ -63,7 +63,6 @@ async def _clear(store: Any) -> None: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_adr0114_claim_proc_live.py b/tests/test_adr0114_claim_proc_live.py index f59c0b45..081cf8ce 100644 --- a/tests/test_adr0114_claim_proc_live.py +++ b/tests/test_adr0114_claim_proc_live.py @@ -65,7 +65,7 @@ async def _open(**flag_overrides: Any) -> SqlServerStore: async def _clean(store: SqlServerStore) -> None: # The canonical live-suite cleanup order (test_claim_fifo_heads._open_sqlserver): children # before messages (response FKs it), so no 547 on a shared live DB. - for table in ("message_events", "queue", "response", "delivered_keys", "outbox", "messages"): + for table in ("message_events", "queue", "response", "delivered_keys", "messages"): await store._execute(f"DELETE FROM {table}") # noqa: S608 - test cleanup, fixed names diff --git a/tests/test_batch_claim_fifo.py b/tests/test_batch_claim_fifo.py index 8da17274..a5ea71d8 100644 --- a/tests/test_batch_claim_fifo.py +++ b/tests/test_batch_claim_fifo.py @@ -53,7 +53,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_batch_claim_locking.py b/tests/test_batch_claim_locking.py index efb98ad0..94c49cf8 100644 --- a/tests/test_batch_claim_locking.py +++ b/tests/test_batch_claim_locking.py @@ -88,7 +88,6 @@ async def ss_store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_batch_completion.py b/tests/test_batch_completion.py index b5b6ad1a..dea5232a 100644 --- a/tests/test_batch_completion.py +++ b/tests/test_batch_completion.py @@ -29,7 +29,7 @@ async def _open_sqlserver() -> Any: store = await SqlServerStore.open(load_settings(environ=os.environ).store) async with store._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + for table in ("message_events", "state", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() return store diff --git a/tests/test_claim_fifo_heads.py b/tests/test_claim_fifo_heads.py index 8fd0850d..66e9062d 100644 --- a/tests/test_claim_fifo_heads.py +++ b/tests/test_claim_fifo_heads.py @@ -61,7 +61,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_cluster_failover_postgres.py b/tests/test_cluster_failover_postgres.py index de26d23d..3ac9f184 100644 --- a/tests/test_cluster_failover_postgres.py +++ b/tests/test_cluster_failover_postgres.py @@ -294,7 +294,7 @@ async def _clear_data(store: object) -> None: "SELECT tablename FROM pg_tables WHERE schemaname = ANY (current_schemas(false))" ) existing = {r["tablename"] for r in rows} - targets = [t for t in ("messages", "queue", "outbox", "response") if t in existing] + targets = [t for t in ("messages", "queue", "response") if t in existing] if targets: await conn.execute(f"TRUNCATE {', '.join(targets)} RESTART IDENTITY CASCADE") diff --git a/tests/test_cluster_failover_sqlserver.py b/tests/test_cluster_failover_sqlserver.py index fb1c1b04..925cd092 100644 --- a/tests/test_cluster_failover_sqlserver.py +++ b/tests/test_cluster_failover_sqlserver.py @@ -292,7 +292,7 @@ async def _clear_data(store: object) -> None: """Empty the message/queue data tables (FK order) so the FIFO claim starts from a known single head.""" async with store._pool.acquire() as conn: # type: ignore[attr-defined] cur = await conn.cursor() - for table in ("queue", "response", "outbox", "messages"): + for table in ("queue", "response", "messages"): await cur.execute(f"IF OBJECT_ID(N'{table}', N'U') IS NOT NULL DELETE FROM {table}") await conn.commit() diff --git a/tests/test_fifo_index_migration.py b/tests/test_fifo_index_migration.py index fbbea005..36a24a76 100644 --- a/tests/test_fifo_index_migration.py +++ b/tests/test_fifo_index_migration.py @@ -58,7 +58,6 @@ async def _open_sqlserver(_: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_fixture_outbox_reset.py b/tests/test_fixture_outbox_reset.py index ca6a0f9f..fd5e8e44 100644 --- a/tests/test_fixture_outbox_reset.py +++ b/tests/test_fixture_outbox_reset.py @@ -24,6 +24,12 @@ — it is a no-op once the table is gone. That distinction is structural, not stylistic, which is why this scanner reads the loop body rather than grepping for the word ``outbox``. + +**The two source-level retirement guards at the bottom live here for the same reason.** They assert +things about `_SCHEMA` and about the engine package that need no database, and the live migration +suite (`tests/test_sqlserver_legacy_outbox_migration.py`) carries a MODULE-level +``skipif(not MEFOR_TEST_SQLSERVER)``. Leaving them there would have gated a pure source check behind a +running SQL Server — the same "runs on a leg that may not exist" trap this module was built to avoid. """ from __future__ import annotations @@ -33,6 +39,8 @@ import pytest +from messagefoundry.store import Stage + _TESTS = pathlib.Path(__file__).resolve().parent #: Existence-guard markers. A reset whose statement contains one of these degrades to a no-op when the @@ -95,17 +103,6 @@ def _scanned_files() -> list[pathlib.Path]: return sorted(p for p in _TESTS.glob("test_*.py") if p.name not in _NOT_RESET_LISTS) -@pytest.mark.xfail( - strict=True, - reason=( - "EXPECTED RED until the legacy SQL Server `outbox` table is retired. 21 unguarded reset sites " - "across 18 files still name it, and they are CORRECT today — the table exists. This guard " - "lands FIRST, before the retirement, so the retirement cannot ship half-done; its failure list " - "is the work list. strict=True makes this self-clearing: the moment the last reset list is " - "fixed the test XPASSes, which strict turns into a FAILURE, forcing this marker to be removed " - "in the same commit. A non-strict xfail would let the guard rot silently green forever." - ), -) def test_no_fixture_issues_an_unguarded_outbox_delete() -> None: """After the legacy table is retired, an unguarded reset is a module-wide failure. @@ -113,8 +110,10 @@ def test_no_fixture_issues_an_unguarded_outbox_delete() -> None: leg naming the file and line, which is the only place it can be caught for the nine files that run on no SQL Server CI step. - NOTE this test is expected to FAIL until Commit 6 updates the reset lists. That is deliberate and is - the point of landing the guard first: the failure list below IS the work list. + This test shipped one commit ahead of the retirement carrying `xfail(strict=True)`, with the 21 + offending sites as its failure list. `strict` is what made that self-clearing: fixing the last reset + list turned the xfail into an XPASS, which strict reports as a FAILURE, so the marker could not be + left behind to rot silently green — removing it here was forced, not remembered. """ scanned = _scanned_files() # Liveness receipt: report what was EXAMINED, not just what was found. A glob that silently matched @@ -178,3 +177,101 @@ def test_every_exemption_still_exists_and_still_needs_exempting(exempt: str) -> f"{exempt} no longer mentions 'outbox' — the exemption is stale and should be removed, " f"otherwise a genuine reset list landing in this file later would be silently skipped" ) + + +def test_the_migration_ships_in_the_schema_batch_not_open_path_code() -> None: + """No database needed. `_schema_hash()` alone decides whether the DDL batch runs at all, so a + migration outside `_SCHEMA` is invisible to that decision and would never fire on an upgrade. + + Mutation: move the statement into an open-path method — reds here, on the plain leg, rather than + silently never running against a real customer upgrade. + """ + from messagefoundry.store.sqlserver import _SCHEMA + + migrations = [s for s in _SCHEMA if "DROP TABLE outbox" in s] + assert len(migrations) == 1, ( + f"expected exactly one outbox migrate-and-drop statement in _SCHEMA, found {len(migrations)}" + ) + stmt = migrations[0] + assert "IF OBJECT_ID('outbox','U') IS NOT NULL" in stmt, ( + "the migration is not existence-guarded" + ) + assert "INSERT INTO queue" in stmt, "the statement drops the table without migrating its rows" + assert f"'{Stage.OUTBOUND.value}'" in stmt, "migrated rows must land at the outbound stage" + assert "EXISTS (SELECT 1 FROM messages" in stmt, "the FK orphan filter is gone" + assert "NOT EXISTS (SELECT 1 FROM queue" in stmt, "the PK anti-join is gone" + + # Placement matters as much as presence: SQL Server defers name resolution, so a statement placed + # before `queue` exists would parse and then fail at RUN time on a legacy DB. + queue_ddl = [i for i, s in enumerate(_SCHEMA) if "CREATE TABLE queue" in s] + assert len(queue_ddl) == 1, "could not locate the queue DDL to check migration ordering" + assert _SCHEMA.index(stmt) > queue_ddl[0], ( + "the migration runs BEFORE `queue` is created; on a legacy DB old enough to lack `queue` that " + "is a failed open inside the batch's single transaction — a startup crash-loop" + ) + + +#: The ONLY engine modules allowed to name the retired table, and how many SQL references each may +#: carry. A migration must obviously still address the table it is migrating away, so a flat ban would +#: be unsatisfiable — but "some references are fine" is how a stray read hides forever. Pinning the +#: COUNT is what keeps the carve-out honest: a new `FROM outbox` anywhere in these files moves the +#: number and reds, even though the file is exempt from the ban itself. +_MIGRATION_MODULES = { + # POSIX keys, matched against a POSIX-normalised path below. Native separators would key on + # backslashes and MISS on the ubuntu leg, reporting both migrations as stray — a guard that is red + # on one OS and green on another, which is worse than no guard. + "store/store.py": (2, "_migrate_outbox_to_queue — the SQLite fold-into-queue + DROP"), + "store/sqlserver.py": (1, "the _SCHEMA migrate-and-drop statement (ASVS 14.2.7)"), +} + + +def test_only_the_two_migrations_still_reference_the_retired_table() -> None: + """No database needed. Anywhere else, a surviving read/write is *Invalid object name* at runtime. + + The keyed encrypt pass is covered separately and structurally by + `tests/test_sqlserver_encrypt_pass_tables.py`; this is the broad sweep for everything else. + + Mutation: add a `SELECT ... FROM outbox` to any engine module — reds naming the file and line. Add + one to a MIGRATION module — still reds, on the pinned count, which is the case a plain + file-exemption list would have missed. + """ + import pathlib + import re + + root = pathlib.Path(__file__).resolve().parents[1] / "messagefoundry" + files = sorted(root.rglob("*.py")) + # Liveness receipt: report what was EXAMINED. A glob matching nothing would pass vacuously. + assert len(files) > 40, ( + f"liveness: only {len(files)} engine modules scanned — the glob is broken" + ) + + # Statements that would actually touch the table. `outbox_id`, `outbox_for`, `outbox_by_status` + # and friends are API/method names, not the table, so the pattern is anchored on SQL keywords. + sql = re.compile(r"\b(FROM|INTO|UPDATE|TABLE|JOIN)\s+outbox\b", re.I) + stray: list[str] = [] + counts: dict[str, int] = {} + for path in files: + rel = path.relative_to(root).as_posix() + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + if not sql.search(line) or "DROP TABLE outbox" in line: + continue + if rel in _MIGRATION_MODULES: + counts[rel] = counts.get(rel, 0) + 1 + continue + stray.append(f" {rel}:{lineno}: {line.strip()[:100]}") + + assert not stray, ( + f"scanned {len(files)} engine modules; these still address the retired `outbox` table " + f"outside a migration:\n" + "\n".join(stray) + ) + for rel, (expected, reason) in _MIGRATION_MODULES.items(): + got = counts.get(rel, 0) + assert got == expected, ( + f"{rel} carries {got} SQL references to `outbox`, expected {expected} ({reason}). " + f"More means a new one crept into a module exempt from the ban; fewer means the migration " + f"itself was removed or rewritten — in which case a legacy store never gets folded in and " + f"its PHI bodies stay unpurgeable, which is the whole point of the retirement." + ) + assert "DROP TABLE outbox" in (root / rel).read_text(encoding="utf-8"), ( + f"{rel} is exempted as a MIGRATION but no longer DROPs the table — the exemption is stale" + ) diff --git a/tests/test_load_failover_postgres.py b/tests/test_load_failover_postgres.py index 98800fb0..2542a558 100644 --- a/tests/test_load_failover_postgres.py +++ b/tests/test_load_failover_postgres.py @@ -38,7 +38,6 @@ _RESET_TABLES = ( "messages", "queue", - "outbox", "response", "leader_lease", "nodes", diff --git a/tests/test_load_failover_sqlserver.py b/tests/test_load_failover_sqlserver.py index 67bcf8a6..089d6166 100644 --- a/tests/test_load_failover_sqlserver.py +++ b/tests/test_load_failover_sqlserver.py @@ -39,7 +39,6 @@ # table already exists (a fresh DB has none yet — the nodes create them empty on start). _RESET_TABLES = ( "queue", - "outbox", "response", "leader_lease", "nodes", diff --git a/tests/test_metadata_bag.py b/tests/test_metadata_bag.py index 23aa1757..c297dbd3 100644 --- a/tests/test_metadata_bag.py +++ b/tests/test_metadata_bag.py @@ -37,7 +37,7 @@ async def _open_sqlserver() -> Any: store = await SqlServerStore.open(load_settings(environ=os.environ).store) async with store._pool.acquire() as conn: # a clean slate — mirrors the other SS suites cur = await conn.cursor() - for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + for table in ("message_events", "state", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() return store diff --git a/tests/test_outbound_batch.py b/tests/test_outbound_batch.py index d9673e2e..11cf964f 100644 --- a/tests/test_outbound_batch.py +++ b/tests/test_outbound_batch.py @@ -58,7 +58,7 @@ async def _open_sqlserver() -> Any: store = await SqlServerStore.open(load_settings(environ=os.environ).store) async with store._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "queue", "response", "outbox", "messages"): + for table in ("message_events", "state", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() return store diff --git a/tests/test_phi_at_rest_inventory.py b/tests/test_phi_at_rest_inventory.py index 01eb84b3..c6ebe56d 100644 --- a/tests/test_phi_at_rest_inventory.py +++ b/tests/test_phi_at_rest_inventory.py @@ -83,8 +83,12 @@ # exactly the opposite — so the suite stayed green while the docs said there was no purge. # # Each string is deliberately NARROWER than it looks. The bare "no purge on any backend" is NOT - # listed: it is a substring of PHI.md's legacy `outbox.payload` row, which is still TRUE. Adding - # the short form would red the suite on a correct statement. + # listed, and the reason CHANGED without changing the answer. It used to collide with PHI.md's + # legacy `outbox.payload` row, which was then still true. ASVS 14.2.7 retired that row — but the + # same edit added a PAST-TENSE account of what the retirement fixed ("...kept full outbound PHI + # bodies there that no purge on any backend reached"), which is a true statement about history and + # would red on the short form just as the old row did. Re-verified by grep at retirement time; a + # phrase whose collision merely MOVED is not a phrase that became safe to register. "nulled by no purge on any backend", "No retention purge nulls this column", "No retention purge on any backend", @@ -325,7 +329,10 @@ def test_cipher_registry_is_enumerable_from_code() -> None: "users.totp_secret", "connection_event.reason", "alert_instance.reason", - "outbox.payload", + # `outbox.payload` was here until the legacy SQL Server table was migrated into `queue` and + # DROPped (ASVS 14.2.7). It is gone from the registry BECAUSE the cell no longer exists — + # `test_per_backend_cipher_counts_are_derived_not_hand_written` is what proves that is a real + # retirement and not a silently narrowed enumeration: it re-derives 18/17/17 from the code. "uploaded_file.body", ): assert expected in cells, f"{expected} vanished from the code-derived cipher registry" @@ -523,10 +530,16 @@ def test_guard_detects_a_planted_omission() -> None: _assert_purge_surface_documented(["purge_alert_instances"], damaged_purge) -@pytest.mark.parametrize("victim", ["shared_body.body", "outbox.payload"]) +@pytest.mark.parametrize("victim", ["shared_body.body"]) def test_guard_detects_a_planted_full_row_deletion(victim: str) -> None: - """The case the whole-section check passed: both tokens survive in §2's coverage PROSE, so - deleting their inventory ROW was undetectable until the assertion was table-scoped.""" + """The case the whole-section check passed: the token survives in §2's coverage PROSE, so + deleting its inventory ROW was undetectable until the assertion was table-scoped. + + `outbox.payload` was the second parameter until ASVS 14.2.7 retired the legacy SQL Server table. + It is dropped rather than kept because this case needs a victim that is BOTH in the code-derived + cipher registry AND named in §2's prose; a retired cell is in neither, so keeping it would fail on + the `assert victim in cells` precondition — a fixture failure, not a guard failure. + """ cells = _cipher_cells() assert victim in cells rows = _table_rows(_section(2)) diff --git a/tests/test_pooled_rider.py b/tests/test_pooled_rider.py index 4e2c0ed6..8c13c903 100644 --- a/tests/test_pooled_rider.py +++ b/tests/test_pooled_rider.py @@ -132,7 +132,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_seq_only_fifo.py b/tests/test_seq_only_fifo.py index dcb35af5..7db181fe 100644 --- a/tests/test_seq_only_fifo.py +++ b/tests/test_seq_only_fifo.py @@ -60,7 +60,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_shard_recovery_sqlserver.py b/tests/test_shard_recovery_sqlserver.py index a51900fe..8862dc38 100644 --- a/tests/test_shard_recovery_sqlserver.py +++ b/tests/test_shard_recovery_sqlserver.py @@ -85,7 +85,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_sqlserver_legacy_outbox_migration.py b/tests/test_sqlserver_legacy_outbox_migration.py new file mode 100644 index 00000000..01aa1af8 --- /dev/null +++ b/tests/test_sqlserver_legacy_outbox_migration.py @@ -0,0 +1,331 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The legacy SQL Server ``outbox`` table is folded into ``queue`` and DROPped (ASVS 14.2.7). + +**What was actually wrong.** The table was recreated by `_SCHEMA` on every open and read by nothing: +the staged pipeline and every delivery-side method use `queue`. So `outbox.payload` — a full outbound +PHI body on any store upgraded from the pre-staged-pipeline layout — was reached by no purge on any +backend, while `messages.raw` blanked on its own window. The message read as purged and the body +survived indefinitely. Migrating the rows in puts them under the ordinary outbound windows; dropping +the table is what makes the retirement true rather than merely documented. + +**Why this test simulates an UPGRADE rather than just checking a fresh open.** On a fresh DB the +migration statement's `IF OBJECT_ID('outbox','U') IS NOT NULL` guard is false and the body never runs +— a test that only opened a clean store would pass without executing a single line of the thing it +claims to verify. Worse, `_ensure_schema` SKIPS the whole DDL batch when `schema_meta` already records +the current hash, so creating a legacy table after a normal open and re-opening would ALSO migrate +nothing. Both are the "green because nothing was measured" failure. So each test here seeds a real +legacy table and then clears the schema marker, which is exactly the state a genuine upgrade presents: +`_SCHEMA` changed, so its hash no longer matches, so the batch re-runs. +""" + +from __future__ import annotations + +import os +from collections.abc import AsyncIterator +from typing import Any + +import pytest + +from messagefoundry.store import OutboxStatus, Stage + +pytestmark = pytest.mark.skipif( + not os.getenv("MEFOR_TEST_SQLSERVER"), + reason="set MEFOR_TEST_SQLSERVER=1 (+ MEFOR_STORE_* connection env) to run SQL Server tests", +) + +RAW = "MSH|^~\\&|A|B|C|D|20260101||ADT^A01|MSG1|P|2.5.1\r" + +#: The legacy DDL, verbatim as it stood in `_SCHEMA` before the retirement. Kept here rather than +#: imported because the point is to reconstruct a schema the code no longer contains. +_LEGACY_OUTBOX_DDL = """CREATE TABLE outbox ( + id NVARCHAR(64) NOT NULL PRIMARY KEY, message_id NVARCHAR(64) NOT NULL, + channel_id NVARCHAR(256) NOT NULL, destination_name NVARCHAR(256) NOT NULL, + payload NVARCHAR(MAX) NOT NULL, status NVARCHAR(32) NOT NULL, + attempts INT NOT NULL DEFAULT 0, next_attempt_at FLOAT NOT NULL, last_error NVARCHAR(MAX) NULL, + created_at FLOAT NOT NULL, updated_at FLOAT NOT NULL)""" + + +async def _open_store() -> Any: + from messagefoundry.config.settings import load_settings + from messagefoundry.store.sqlserver import SqlServerStore + + settings = load_settings(environ=os.environ).store + return await SqlServerStore.open(settings) + + +async def _reset(store: Any) -> None: + """Clean slate. `outbox` is dropped defensively: a previous test in this module may have left one + behind, and it is NOT in any other suite's reset list any more (the table is retired).""" + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute("IF OBJECT_ID('outbox','U') IS NOT NULL DROP TABLE outbox") + for table in ("message_events", "queue", "response", "delivered_keys", "messages"): + await cur.execute(f"DELETE FROM {table}") + await conn.commit() + + +@pytest.fixture +async def store() -> AsyncIterator[Any]: + s = await _open_store() + await _reset(s) + try: + yield s + finally: + await _reset(s) + await s.close() + + +async def _seed_legacy(store: Any, rows: list[tuple[Any, ...]], *, messages: list[str]) -> None: + """Recreate the legacy table, seed it, and put the DB into the state an UPGRADE presents. + + Clearing `schema_meta` is the load-bearing step: with a current marker `_ensure_schema` returns + before executing anything, so without this the next open would migrate nothing and every + assertion below would pass vacuously. A real upgrade reaches the same state by a different route + — `_SCHEMA` changed, so its content hash no longer matches what the marker records. + """ + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute("IF OBJECT_ID('outbox','U') IS NOT NULL DROP TABLE outbox") + await cur.execute(_LEGACY_OUTBOX_DDL) + for mid in messages: + await cur.execute( + "INSERT INTO messages (id, channel_id, received_at, source_type, control_id," + " message_type, raw, status) VALUES (?,?,?,?,?,?,?,?)", + (mid, "IB", 1.0, "mllp", "C", "ADT^A01", RAW, "routed"), + ) + for row in rows: + await cur.execute( + "INSERT INTO outbox (id, message_id, channel_id, destination_name, payload, status," + " attempts, next_attempt_at, last_error, created_at, updated_at)" + " VALUES (?,?,?,?,?,?,?,?,?,?,?)", + row, + ) + await cur.execute("DELETE FROM schema_meta") + await conn.commit() + + +async def _tables(store: Any) -> set[str]: + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute("SELECT name FROM sys.tables") + return {r[0] for r in await cur.fetchall()} + + +async def _queue_rows(store: Any) -> list[dict[str, Any]]: + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute( + "SELECT id, message_id, stage, channel_id, destination_name, payload, status," + " attempts, created_at, seq FROM queue ORDER BY seq" + ) + cols = [c[0] for c in cur.description] + return [dict(zip(cols, r, strict=True)) for r in await cur.fetchall()] + + +async def test_legacy_outbox_rows_migrate_into_queue_and_the_table_is_dropped(store: Any) -> None: + """The core claim: rows survive as stage='outbound' queue rows, and the table is gone. + + Mutation: delete the `DROP TABLE outbox` from the `_SCHEMA` statement — reds on the table + assertion. Delete the whole statement — reds on the row assertion first. + """ + await _seed_legacy( + store, + [ + ( + "o1", + "m1", + "IB", + "OB_A", + "PAYLOAD-1", + OutboxStatus.PENDING.value, + 0, + 0.0, + None, + 1.0, + 1.0, + ), + ( + "o2", + "m1", + "IB", + "OB_B", + "PAYLOAD-2", + OutboxStatus.PENDING.value, + 3, + 9.0, + "boom", + 2.0, + 2.0, + ), + ], + messages=["m1"], + ) + + reopened = await _open_store() + try: + assert "outbox" not in await _tables(reopened), ( + "the legacy table survived the schema batch — a DROP that did not fire leaves the " + "unpurgeable PHI tier exactly where it was" + ) + rows = await _queue_rows(reopened) + assert len(rows) == 2, f"expected both legacy rows folded in, got {rows}" + by_id = {r["id"]: r for r in rows} + assert set(by_id) == {"o1", "o2"}, ( + "row ids must be preserved (they are referenced elsewhere)" + ) + for r in rows: + assert r["stage"] == Stage.OUTBOUND.value + assert r["message_id"] == "m1" + assert r["channel_id"] == "IB" + # Every column carried over verbatim — a migration that silently reset attempts or dropped + # last_error would resurrect a dead row as deliverable. + assert by_id["o1"]["destination_name"] == "OB_A" + assert by_id["o1"]["payload"] == "PAYLOAD-1" + assert by_id["o2"]["destination_name"] == "OB_B" + assert by_id["o2"]["payload"] == "PAYLOAD-2" + assert by_id["o2"]["attempts"] == 3 + assert by_id["o1"]["created_at"] == 1.0 and by_id["o2"]["created_at"] == 2.0 + finally: + await reopened.close() + + +async def test_migrated_rows_are_reachable_by_the_delivery_path(store: Any) -> None: + """A migrated row is not merely present — it must be CLAIMABLE, or the backlog silently stalls. + + This is the assertion that distinguishes a real migration from rows parked in a table with the + wrong stage/status: `claim_next_fifo` filters on both. + """ + await _seed_legacy( + store, + [ + ( + "o1", + "m1", + "IB", + "OB_A", + "PAYLOAD-1", + OutboxStatus.PENDING.value, + 0, + 0.0, + None, + 1.0, + 1.0, + ) + ], + messages=["m1"], + ) + reopened = await _open_store() + try: + item = await reopened.claim_next_fifo("OB_A") + assert item is not None, ( + "the migrated row is not claimable — the legacy backlog would stall" + ) + assert item.payload == "PAYLOAD-1" + finally: + await reopened.close() + + +async def test_an_orphaned_legacy_row_is_skipped_rather_than_failing_the_open(store: Any) -> None: + """`queue.message_id` is FK-enforced; an orphan would abort the whole DDL batch. + + That is not a degraded start — the batch is one transaction, so it rolls back and re-fails on + every restart: a crash-loop. The orphan was unreplayable anyway (nothing can view or route a row + whose message is gone), so it is dropped and the surviving row still migrates. + + Mutation: remove the `EXISTS (SELECT 1 FROM messages ...)` filter — this test reds with an FK + violation at open, which is precisely the crash-loop it exists to prevent. + """ + await _seed_legacy( + store, + [ + ("o1", "m1", "IB", "OB_A", "GOOD", OutboxStatus.PENDING.value, 0, 0.0, None, 1.0, 1.0), + # message 'gone' was never inserted + ( + "o2", + "gone", + "IB", + "OB_A", + "ORPHAN", + OutboxStatus.PENDING.value, + 0, + 0.0, + None, + 1.0, + 1.0, + ), + ], + messages=["m1"], + ) + reopened = await _open_store() + try: + assert "outbox" not in await _tables(reopened) + rows = await _queue_rows(reopened) + assert [r["id"] for r in rows] == ["o1"], ( + f"the orphan should be skipped and the good row kept, got {[r['id'] for r in rows]}" + ) + finally: + await reopened.close() + + +async def test_an_id_already_present_in_queue_does_not_abort_the_batch(store: Any) -> None: + """`queue.id` is the PK, so a colliding legacy id would raise 2627 and roll the batch back. + + uuid4 ids make a genuine collision implausible, which is exactly why this needs a test rather + than an argument: the anti-join is unreachable in normal operation, so nothing else would ever + exercise it, and a silent regression would surface only as a customer's crash-looping upgrade. + + Mutation: remove the `NOT EXISTS (SELECT 1 FROM queue ...)` filter — reds with a PK violation. + """ + await _seed_legacy( + store, + [("dup", "m1", "IB", "OB_A", "LEGACY", OutboxStatus.PENDING.value, 0, 0.0, None, 1.0, 1.0)], + messages=["m1"], + ) + # Plant a `queue` row that already owns the id, then re-clear the marker the insert did not touch. + async with store._pool.acquire() as conn: + cur = await conn.cursor() + await cur.execute( + "INSERT INTO queue (id, message_id, stage, channel_id, destination_name, payload," + " status, attempts, next_attempt_at, created_at, updated_at)" + " VALUES (?,?,?,?,?,?,?,?,?,?,?)", + ( + "dup", + "m1", + Stage.OUTBOUND.value, + "IB", + "OB_A", + "ALREADY-HERE", + OutboxStatus.PENDING.value, + 0, + 0.0, + 5.0, + 5.0, + ), + ) + await conn.commit() + + reopened = await _open_store() + try: + assert "outbox" not in await _tables(reopened), ( + "the batch aborted — the table is still here" + ) + rows = await _queue_rows(reopened) + assert len(rows) == 1, f"the colliding legacy row must not be inserted twice, got {rows}" + assert rows[0]["payload"] == "ALREADY-HERE", ( + "the pre-existing queue row must win, not be lost" + ) + finally: + await reopened.close() + + +async def test_a_fresh_store_has_no_outbox_table_at_all(store: Any) -> None: + """The other half of the retirement: `_SCHEMA` must no longer CREATE the table. + + Cheap, and it is the assertion that catches a revert of the DDL removal — the migration statement + alone would then drop a table the very same batch had just recreated, which is stable but leaves + every open re-creating an empty unpurgeable tier. + """ + tables = await _tables(store) + assert "outbox" not in tables, "a fresh open recreated the retired legacy table" + assert "queue" in tables, "liveness: the table enumeration is reading a real schema" diff --git a/tests/test_sqlserver_store.py b/tests/test_sqlserver_store.py index 6c337eac..58ae589a 100644 --- a/tests/test_sqlserver_store.py +++ b/tests/test_sqlserver_store.py @@ -97,7 +97,6 @@ async def store() -> AsyncIterator[object]: # key-rotation reencrypt scan, and this suite asserts EXACT rotate # counts (the == 6 / == 2 above), so a preset left behind by one test # silently miscounts an unrelated one - "outbox", "messages", "sessions", "webauthn_credentials", # ADR 0068: FK to users(id) — must clear before users @@ -1042,7 +1041,7 @@ async def test_reencrypt_to_active_rotates_all_columns_including_state(store) -> try: async with rotated._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "response", "queue", "outbox", "messages"): + for table in ("message_events", "state", "response", "queue", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() finally: @@ -1837,7 +1836,7 @@ async def test_keyless_open_of_encrypted_state_fails_closed(store) -> None: try: async with cleanup._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "state", "response", "queue", "outbox", "messages"): + for table in ("message_events", "state", "response", "queue", "messages"): await cur.execute(f"DELETE FROM {table}") await conn.commit() finally: @@ -4006,7 +4005,7 @@ async def test_the_BOUNDED_path_survives_a_keyed_reopen(store) -> None: try: async with cleanup._pool.acquire() as conn: cur = await conn.cursor() - for table in ("message_events", "queue", "outbox", "response", "messages"): + for table in ("message_events", "queue", "response", "messages"): await cur.execute(f"DELETE FROM {table}") # FK order: children first await cur.execute("DELETE FROM cipher_meta WHERE key_id = ?", (key_id,)) await conn.commit() diff --git a/tests/test_sqlserver_sync_handoff.py b/tests/test_sqlserver_sync_handoff.py index 752b4b21..8ebc3738 100644 --- a/tests/test_sqlserver_sync_handoff.py +++ b/tests/test_sqlserver_sync_handoff.py @@ -44,7 +44,6 @@ async def store() -> AsyncIterator[Any]: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_stage_dispatcher.py b/tests/test_stage_dispatcher.py index bdd4f0d2..1e40d189 100644 --- a/tests/test_stage_dispatcher.py +++ b/tests/test_stage_dispatcher.py @@ -75,7 +75,6 @@ async def _open_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") diff --git a/tests/test_x12_rte.py b/tests/test_x12_rte.py index abba26b0..e1a56e8b 100644 --- a/tests/test_x12_rte.py +++ b/tests/test_x12_rte.py @@ -345,7 +345,6 @@ async def _open_rte_sqlserver(tmp_path: Path) -> Any: "queue", "response", "delivered_keys", - "outbox", "messages", ): await cur.execute(f"DELETE FROM {table}") From 0cffaac9ea20518e7a15aa6134a523bb8749bb09 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 14:27:09 -0500 Subject: [PATCH 8/8] docs(ci): three workflow comments still pointed at the pre-archive plan path origin/main moved docs/throughput-build-plan.md under docs/archive/throughput/ (#69). The rename updated docs/AOAG-DEPLOYMENT.md but not these three ci.yml comments, and nothing catches it: no guard scans workflow COMMENTS for doc paths, so the drift is invisible. Comment-only, no behaviour change. Fixed here rather than left because one of the three sits directly above the file list this branch appends to -- shipping a stale path in a block I just rewrote is my drift, not inherited drift. --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc2c67a1..435da9eb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -426,7 +426,7 @@ jobs: # the ADR 0013/0016 RTE capture/re-ingress case); those suites are MEFOR_TEST_SQLSERVER-gated # and run ONLY on the server-DB legs, so a file not matched here edits with NO real SS/PG # coverage (it merely skips the gated leg). Keep this in sync with those steps - # (docs/throughput-build-plan.md). + # (docs/archive/throughput/throughput-build-plan.md). # The transports/ + config/wiring arms are the CAPTURE SURFACE: capture_response / reingress_to # are declared there (ADR 0013), and SQL Server / Postgres both declare supports_response_capture # + supports_pt_reingress True — so a regression in that surface is a SERVER-DB regression and must @@ -676,7 +676,7 @@ jobs: # reliability-core invariant tests — crash-replay, poison-bound, and the per-lane-FIFO locked-head- # BLOCKS (#285) gate — that are MEFOR_TEST_SQLSERVER-gated but live in their OWN files, so the named # suites above never picked them up and they ran only on SQLite. Run them here against the real - # backend. APPEND each new lever's test file per docs/throughput-build-plan.md. + # backend. APPEND each new lever's test file per docs/archive/throughput/throughput-build-plan.md. # # NOT only throughput levers: this step is the catch-all for ANY MEFOR_TEST_SQLSERVER-gated file # the named suites above do not collect. CI runs an explicit file LIST here, not the whole suite, @@ -823,7 +823,7 @@ jobs: MEFOR_ALLOW_INSECURE_TLS: "1" # The throughput levers' MEFOR_TEST_POSTGRES-gated invariants (esp. the FOR-UPDATE no-SKIP-LOCKED # locked-head-BLOCKS twin of #285, and the batch FIFO/crash tests) live in their own files. APPEND - # each new lever's test file per docs/throughput-build-plan.md. + # each new lever's test file per docs/archive/throughput/throughput-build-plan.md. run: >- pytest -v tests/test_inline_fast_path.py