From e5505a96fe088582a81c1d573c8916d5b49c7bf2 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 19:16:06 -0500 Subject: [PATCH 01/31] fix(containment): validate the codeset rename SOURCE and fold the Corepoint module stem (BACKLOG #1130) Two untrusted strings reached a filesystem path without clearing the rule their siblings already clear. Both are closed with the shape each site's neighbours already use, rather than a new one. config/codeset_edit.py: rename_code_set validated only `new`. `old` is equally untrusted and reaches `_existing_path`, so a traversal source resolved outside codesets/ and `os.replace` would MOVE that file in, contents intact. The `_validate_name` call is added before the lookup, matching show (:92), upsert (:144) and remove (:221). Kept after the empty-argument checks so the `--name is required` wording still wins for an empty name. corepoint_import.py: the export chooses `module_name`, which becomes BOTH the emitted inbound() connection name and the module's filename stem. It is folded with `_sanitize`, as the channel name (:415) and handler names (:464) already are. Mutation rather than refusal, because the importer writes into a directory it created -- there is no existing file to alias -- and a colliding fold is already de-duplicated by the writer's `assigned` set. THIS MOVES NO ASVS SCORE. The cell's recorded absence claim is keyed on `_within_root`, a symbol belonging to transports/file.py, while the guard that shipped is `_is_contained_name`; that re-key is record work in the vault and nothing here satisfies it. SCOPE, STATED RATHER THAN ASSERTED AS COMPLETE. The item's own body measured 162 file-path-construction sites across 49 files under messagefoundry against roughly fifteen named, and forbids a residual sentence saying "every" or "all limbs" until that census exists. This commit covers the two named sites only. harness/, scripts/ and the ide/ TypeScript surface are untouched and are contingent on an open scope ruling about which roots the assessment covers. Tests, each asserting on the refusal and not merely on an unchanged tree: tests/test_corepoint_import.py::test_a_hostile_inbound_name_cannot_write_outside_the_output_directory tests/test_codeset_edit.py::test_rename_rejects_traversal_source_without_moving_a_file_into_the_dir tests/test_codeset_edit.py::test_cli_rename_traversal_source_is_a_clean_json_error --- docs/CODESETS.md | 2 +- messagefoundry/config/codeset_edit.py | 13 ++++-- messagefoundry/corepoint_import.py | 11 ++++- tests/test_codeset_edit.py | 43 +++++++++++++++++++ tests/test_corepoint_import.py | 59 +++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 5 deletions(-) diff --git a/docs/CODESETS.md b/docs/CODESETS.md index f64bdba5..0db1f4a7 100644 --- a/docs/CODESETS.md +++ b/docs/CODESETS.md @@ -70,7 +70,7 @@ loader's own wording inline. | `messagefoundry codeset list --config DIR` | Summarize every set under `codesets/` (`.csv` **and** `.toml`), sorted by name. | | `messagefoundry codeset show --config DIR --name N` | The grid for set `N` (headers + rows); `format:"toml"` ⇒ read-only. | | `messagefoundry codeset upsert --config DIR [--data JSON]` | Validate → write `codesets/N.csv` atomically (temp + replace, owner-only perms) → **re-load the written file as the final check**; a bad save rolls back. DETAIL JSON comes from `--data` or stdin. | -| `messagefoundry codeset rename --config DIR --name N --to M` | Atomic `os.replace` of `codesets/N.` → `codesets/M.`; rejects a stem collision. | +| `messagefoundry codeset rename --config DIR --name N --to M` | Name-safety on **both** `N` and `M` (a bare stem, no separator, `..`, drive or extension) → atomic `os.replace` of `codesets/N.` → `codesets/M.`; rejects a stem collision. | | `messagefoundry codeset remove --config DIR --name N` | Delete `codesets/N.csv` (else `.toml`). | Add `--json` to any command for machine-readable output. diff --git a/messagefoundry/config/codeset_edit.py b/messagefoundry/config/codeset_edit.py index a61c3b49..d1961562 100644 --- a/messagefoundry/config/codeset_edit.py +++ b/messagefoundry/config/codeset_edit.py @@ -165,14 +165,21 @@ def rename_code_set( ) -> dict[str, Any]: """Rename ``codesets/.`` to ``codesets/.`` (atomic ``os.replace``). - ``new`` is checked with the same name-safety rules as ``upsert``; the rename is rejected if **any** - supported file already exists for ``new`` (a stem collision). Raises :class:`WiringError` if - ``old``/``new`` is missing, the source is absent, ``new`` is unsafe, or the stem collides.""" + **Both** ``old`` and ``new`` are checked with the same name-safety rules as ``upsert``; the rename + is rejected if **any** supported file already exists for ``new`` (a stem collision). Raises + :class:`WiringError` if ``old``/``new`` is missing or unsafe, the source is absent, or the stem + collides.""" if not old: raise WiringError("--name is required for `codeset rename`") if not new: raise WiringError("--to is required for `codeset rename`") codesets_dir = _codesets_dir(config_dir) + # The rename SOURCE is untrusted too, and it builds a filesystem path at ``_existing_path``, so it + # must clear the same rules as ``new`` BEFORE that lookup, exactly as show/remove do with theirs. + # Otherwise a traversal ``old`` resolves outside codesets/ and the ``os.replace`` below MOVES that + # file in, contents intact. Kept after the empty-argument checks so the `--name is required` + # wording still wins for an empty name (``_validate_name`` has its own, less specific message). + _validate_name(codesets_dir, old) src = _existing_path(codesets_dir, old) _validate_name(codesets_dir, new) # For a rename, ANY supported file for the new stem is a collision (unlike upsert, which may diff --git a/messagefoundry/corepoint_import.py b/messagefoundry/corepoint_import.py index 83f381ad..943e1d27 100644 --- a/messagefoundry/corepoint_import.py +++ b/messagefoundry/corepoint_import.py @@ -419,7 +419,16 @@ def _parse_channel(ch: dict[str, Any], index: int) -> Channel: raise CorepointImportError(f"channel {name!r} requires an 'inbound' object") in_connector, in_call = _render_connector(inbound, name, inbound=True) - module_name = _opt_str(inbound, "name") or f"IB_{ident.upper()}" + # The export chooses this string, and it becomes BOTH the emitted ``inbound()`` connection name + # and the module's filename stem (``import_corepoint`` writes ``out / f"{module_name}.py"``), so + # it is untrusted text that reaches a filesystem write: fold it to a bare identifier, exactly as + # the channel name above and the handler names below are folded. Mutation is right here rather + # than a refusal, because the importer WRITES into a directory it created (it is not selecting an + # existing file, so there is no basename-aliasing target to hand an attacker), and a fold that + # collides with another channel's stem is already de-duplicated and reported by the writer's + # ``assigned`` set. Sanitize at the source, not at the filename, so the stem and the connection + # name it registers cannot desync. + module_name = _sanitize(_opt_str(inbound, "name") or f"IB_{ident.upper()}") dests_raw = ch.get("destinations", []) if not isinstance(dests_raw, list): diff --git a/tests/test_codeset_edit.py b/tests/test_codeset_edit.py index 5c291903..b6bebd5a 100644 --- a/tests/test_codeset_edit.py +++ b/tests/test_codeset_edit.py @@ -514,6 +514,20 @@ def test_rename_to_unsafe_name_raises(tmp_path: Path) -> None: codeset_edit.rename_code_set(tmp_path, "diets", "a/b", validate=_validate) +def test_rename_rejects_traversal_source_without_moving_a_file_into_the_dir(tmp_path: Path) -> None: + # The rename SOURCE is operator-supplied and untrusted exactly like show/upsert/remove's name: it + # builds a filesystem path, so it must be rejected BEFORE any filesystem touch. Otherwise a + # rename is an arbitrary MOVE of any .csv/.toml on disk into codesets/, contents intact. + outside = tmp_path / "outside.csv" + outside.write_text("code,value\nA,moved\n", encoding="utf-8") + _codesets(tmp_path).mkdir() + with pytest.raises(WiringError, match="must not contain a path separator"): + codeset_edit.rename_code_set(tmp_path, "../outside", "stolen", validate=_validate) + # The out-of-dir file stayed where it was, and nothing landed under codesets/. + assert outside.read_text(encoding="utf-8") == "code,value\nA,moved\n" + assert not (_codesets(tmp_path) / "stolen.csv").exists() + + def test_rename_collision_raises(tmp_path: Path) -> None: codeset_edit.upsert_code_set( tmp_path, "diets", ["code", "value"], [["A", "Apple"]], validate=_validate @@ -790,6 +804,35 @@ def test_cli_rename_missing_to(tmp_path: Path, capsys: pytest.CaptureFixture[str assert json.loads(out)["error"] == "--to is required for `codeset rename`" +def test_cli_rename_traversal_source_is_a_clean_json_error( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + # The CLI adds no name checking of its own, and it is not the only caller: the VS Code grid + # shells `codeset rename --name ` from a webview message. So the refusal must arrive as + # {"error": ...} on stdout with rc 1: never a traceback, and never a completed move. + outside = tmp_path / "outside.csv" + outside.write_text("code,value\nA,moved\n", encoding="utf-8") + _codesets(tmp_path).mkdir() + rc, out = _run( + [ + "codeset", + "rename", + "--config", + str(tmp_path), + "--name", + "../outside", + "--to", + "stolen", + "--json", + ], + capsys, + ) + assert rc == 1 + assert "must not contain a path separator" in json.loads(out)["error"] + assert outside.is_file() + assert not (_codesets(tmp_path) / "stolen.csv").exists() + + def test_cli_remove(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: detail = {"name": "diets", "columns": ["code", "value"], "rows": [["A", "Apple"]]} _run( diff --git a/tests/test_corepoint_import.py b/tests/test_corepoint_import.py index 2219ee20..d266cab2 100644 --- a/tests/test_corepoint_import.py +++ b/tests/test_corepoint_import.py @@ -302,6 +302,65 @@ def test_hostile_values_are_escaped_not_injected() -> None: ast.parse(src) +def _named_inbound_export(inbound_name: str) -> str: + """A minimal one-channel export whose ``inbound.name`` is caller-chosen (the untrusted value).""" + return json.dumps( + { + "channels": [ + { + "name": "X", + "inbound": {"connector": "mllp", "name": inbound_name, "port": 2615}, + "destinations": [{"name": "OB_X", "connector": "mllp", "host": "h", "port": 7}], + "handlers": [ + { + "name": "h", + "actions": [{"class": "ItemReplace", "target": "MSH-6", "value": "V"}], + } + ], + } + ] + } + ) + + +def _import_and_census(inbound_name: str, root: Path) -> tuple[list[Path], list[Path]]: + """Import an export naming ``inbound_name``, then census EVERY .py under ``root``. + + Returns ``(inside_out_dir, outside_out_dir)``. The out dir is nested several levels below + ``root`` so a traversal escape still lands inside the temp tree and can be seen, rather than + escaping the census and reading as containment.""" + out = root / "a" / "b" / "c" / "out" + export = root / "export.json" + export.write_text(_named_inbound_export(inbound_name), encoding="utf-8") + import_corepoint(export, out) + found = sorted(root.rglob("*.py")) + return ( + [p for p in found if p.is_relative_to(out)], + [p for p in found if not p.is_relative_to(out)], + ) + + +def test_a_hostile_inbound_name_cannot_write_outside_the_output_directory(tmp_path: Path) -> None: + """``inbound.name`` is the one export value that becomes a filesystem path (the module's filename + stem, and the emitted ``inbound()`` connection name with it), so it is untrusted text reaching a + write. A traversal name must land inside the output directory and nowhere else.""" + # POSITIVE CONTROL, same census, benign name: the module IS written and the census DOES see it, + # so the empty escape list on the hostile run below is a containment result, not a blind probe. + benign_root = tmp_path / "benign" + benign_root.mkdir() + benign_inside, benign_outside = _import_and_census("IB_ACME_ADT", benign_root) + assert benign_inside == [benign_root / "a" / "b" / "c" / "out" / "IB_ACME_ADT.py"] + assert benign_outside == [] + + hostile_root = tmp_path / "hostile" + hostile_root.mkdir() + hostile_inside, hostile_outside = _import_and_census("../../../evil", hostile_root) + assert hostile_outside == [] + # The channel is still imported, under a folded stem: the name is sanitized, never dropped. The + # exact stem is deliberately not asserted, because that would pin the fold, not the containment. + assert len(hostile_inside) == 1 + + def test_malformed_export_raises() -> None: with pytest.raises(CorepointImportError): parse_export("{ not json ") From 32fb3112f2c49ead0eeb9cbd511c9fc2003daf9d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 19:26:30 -0500 Subject: [PATCH 02/31] docs(codesets): record what the rename-source guard changed, and drop a false mirror claim (BACKLOG #1130) Follow-up to e5505a96, from its own adversarial review. Two things that commit changed and did not say. 1. `rename --name` was missing from the untrusted-name list. It joined that list in e5505a96 and the doc still named only `upsert`'s name, `rename --to`, and show/remove. The list now matches the code. 2. THE BEHAVIOUR CHANGE NOBODY HAD WRITTEN DOWN. A code set stored as `lab.results.csv` LOADS -- measured with the real loader, `load_code_sets` returns the key `lab.results` and `codeset list` shows it -- but its dotted stem fails the no-extension name rule. `show` and `remove` already refused it. Adding the guard to `rename --name` closed the last verb that could manage such a file, so it now has to be renamed on disk by hand. That is a real cost of the fix and it belongs beside the fix, not in a review transcript. 3. The heading "Validation rules (mirror the loader exactly)" was FALSE for the name rules and true for the content rules. Split, rather than deleted, so the half that is accurate keeps its claim. Found by the adversarial pass, not by me: it planted each defect separately, confirmed both new tests red for the named reason, reverted, and proved the revert byte-identical by SHA256 on both files. Its verdict was `sound` with `test_can_fail=True`; these are from its residual findings, which is the half a green verdict hides. NOT DONE, and reported rather than silently left: two of its findings are about test STRENGTH, not correctness -- the corepoint census is blind to escape classes its docstring claims, and the codeset tests pin one of the five rules `_validate_name` enforces, so a deliberately weaker guard would still pass them. Those need test work, not doc work, and they are not in this commit. No engine code changed. No ASVS score moves. --- docs/CODESETS.md | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/docs/CODESETS.md b/docs/CODESETS.md index 0db1f4a7..5a1d3aed 100644 --- a/docs/CODESETS.md +++ b/docs/CODESETS.md @@ -92,13 +92,27 @@ exactly the loader's rule. ### The operator-supplied name is untrusted -`upsert`'s `name`, `rename`'s `--to`, **and** the `--name` on `show`/`remove` are all treated as -**untrusted data** (CLAUDE.md §5/§8). The CLI rejects a name that contains a path separator, `..`, an -absolute / drive-prefixed path, or an embedded `.csv`/`.toml` extension, and applies a final -`resolve()` check that the target stays inside `codesets/` — so a name can never read or write a file -outside the code-sets directory. - -### Validation rules (mirror the loader exactly) +`upsert`'s `name`, **both** of `rename`'s `--name` and `--to`, **and** the `--name` on +`show`/`remove` are all treated as **untrusted data** (CLAUDE.md §5/§8). The CLI rejects a name that +contains a path separator, `..`, an absolute / drive-prefixed path, or an embedded `.csv`/`.toml` +extension, and applies a final `resolve()` check that the target stays inside `codesets/` — so a name +can never read or write a file outside the code-sets directory. + +> **`rename --name` joined that list in BACKLOG #1130 and it is a behaviour change.** It was +> previously unchecked, so a traversal source resolved outside `codesets/` and the `os.replace` below +> would have **moved that file in**, contents intact. +> +> **The consequence, stated because it is not obvious:** a code set whose file is `lab.results.csv` +> **loads fine** — `load_code_sets` returns the key `lab.results` and `codeset list` shows it — but +> its dotted stem fails the no-extension rule, so **no CLI verb can manage it.** `show` and `remove` +> already refused it; `rename` was the last one that did not, and now it does too. Such a file must +> be renamed on disk by hand. + +### Validation rules (mirror the loader for CONTENT; the NAME rules are stricter) + +> **The name rules do NOT mirror the loader, and this heading used to say they did.** The loader +> accepts a dotted stem such as `lab.results.csv`; the CLI's name rules refuse it. The content rules +> below do mirror the loader. Measured 2026-08-22 under BACKLOG #1130. A bad `upsert` is rejected **before** any file is touched: a non-empty key column plus at least one value column, unique non-empty headers, all-string cells, no row longer than `columns`, a fully-blank From c28ad75246f90cf8c94fecf49188e8ef5dc6bf7c Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 19:28:22 -0500 Subject: [PATCH 03/31] fix(apiclient): percent-encode every interpolated path segment and allow-list the URL scheme (BACKLOG #1107) Two of the four clauses the item names. The owner dispatched these two only; the FHIR structured-parameter and web-console URL-builder clauses are separate undispatched subjects and are untouched. CLAUSE 1 -- path segments. Seventeen f-string sites interpolated an identifier straight into a URL path and the file encoded nothing: `quote(|urlencode(` in apiclient/client.py returned 0, against a control of 7 in transports/fhir.py and 76 `def ` in the client itself, so the zero was a property of the code. `_seg()` wraps `quote(..., safe="")`; the default `safe="/"` would leave untouched the one character a path segment turns on. ENCODED AT ALL SEVENTEEN, not only the four carrying a connection name. Thirteen carry engine-minted ids, so "this is not untrusted data" is true of them today -- and that is the argument the item names as its live trap, because it rests URL correctness on a data-grammar invariant no line of code asserts and no test pins. CLAUSE 2 -- scheme. `_assert_safe_transport` returned early on `host in _LOOPBACK_HOSTS or host == ""`. `javascript:`, `data:` and `file:` carry no hostname, so all three passed: a host-keyed deny-list for plaintext http, not a scheme allow-list, and the two schemes the ASVS verb names were exactly the two that got through. Now a positive allow-list, strictly narrower than the check it precedes, leaving the plaintext-http logic byte-for-byte intact. BEHAVIOUR CHANGE: a schemeless `base_url` is no longer accepted. Seven non-test construction sites exist under messagefoundry/ and harness/, none passing a schemeless literal -- derived with `grep -rn "EngineClient("` at this tip, not carried from a report. WHAT THAT DOES NOT PROVE: every one of those takes a URL from config or argv, so runtime values are not covered by a static count. THREE RESIDUALS FROM THE ADVERSARIAL PASS, kept because a green verdict hides exactly this half: - The completeness guard's docstring OVERCLAIMS. It says a new endpoint that interpolates an identifier reds the test; that is true for f-strings and FALSE for string concatenation and .format(). The reviewer demonstrated it by adding a concat site that slipped through. - The guard asserts a COUNT, not a correspondence, so a same-size swap (one site removed, one added) passes. - The scheme allow-list's docstring invokes an OS protocol handler. This client hands base_url to httpx, which never launches one -- httpx raises UnsupportedProtocol. The defence is real but narrower than the docstring. THIS MOVES NO ASVS SCORE. The cell is graded in the vault, which this worktree does not contain, and two of its four clauses remain unbuilt. docs/SECURITY.md is UNTOUCHED -- another lane owns it this round. The clause-2 delta is reported to the Dispatcher and should land WITH this code, not ahead. Adversarial pass: 5 plants, each red for its documented reason, client.py sha256 identical after every revert. Verdict sound, test_can_fail true. Module: 56 passed. --- messagefoundry/apiclient/client.py | 107 ++++++++++---- tests/test_apiclient.py | 222 +++++++++++++++++++++++++++++ 2 files changed, 304 insertions(+), 25 deletions(-) diff --git a/messagefoundry/apiclient/client.py b/messagefoundry/apiclient/client.py index 7db76d34..420f5530 100644 --- a/messagefoundry/apiclient/client.py +++ b/messagefoundry/apiclient/client.py @@ -19,7 +19,7 @@ from json import JSONDecodeError from types import TracebackType from typing import TypeVar -from urllib.parse import urlsplit +from urllib.parse import quote, urlsplit import httpx from pydantic import BaseModel, ValidationError @@ -72,6 +72,10 @@ _log = logging.getLogger(__name__) _LOOPBACK_HOSTS = {"127.0.0.1", "localhost", "::1"} +# ASVS 1.2.2 (BACKLOG #1107), the protocol half: the ONLY URL schemes this client will speak. A +# positive allow-list, not a deny-list -- see _assert_safe_transport for why the shape matters. +_ALLOWED_URL_SCHEMES = frozenset({"http", "https"}) + # ASVS 4.2.5, the client half. Deliberately DUPLICATED from transports/rest.py's # MAX_OUTBOUND_URL_LEN / MAX_OUTBOUND_HEADER_VALUE_LEN rather than imported: ADR 0088 makes this # package Qt-free AND engine-free, so a GUI or harness process can depend on it without dragging in @@ -113,15 +117,60 @@ def _decode_list(response: httpx.Response, model: type[_Model]) -> list[_Model]: raise ApiError(f"invalid response from engine: {exc}") from exc -def _assert_safe_transport(base_url: str, *, allow_insecure: bool) -> None: - """Refuse plaintext ``http`` to a non-loopback host (CONSOLE-3). +def _seg(value: str | int) -> str: + """Percent-encode ``value`` for use as ONE URL path segment (ASVS 1.2.2, BACKLOG #1107). + + ``safe=""`` is the whole point: ``quote``'s default is ``safe="/"``, which leaves untouched the + one character a path segment turns on. Without this, an identifier carrying ``..``, ``/``, ``?`` + or ``#`` does not merely look wrong on the wire -- httpx RESOLVES it, so + ``start_connection("../../users/admin")`` leaves ``/connections/`` and retargets the request at + ``/users/admin/start``, and a ``#`` truncates the path at the fragment, dropping the verb. + + Applied at every interpolation site rather than only the ones carrying free text. Most of the + identifiers here are engine-minted hex ids, but four sites carry a CONNECTION NAME, which is + unconstrained: ``Registry._add`` (config/wiring.py) checks a new name only for a duplicate. The + alternative -- resting URL correctness on a data-grammar invariant that no URL-building line + asserts -- is the argument BACKLOG #1107 names as its live trap, so the encode is unconditional. - A remote ``http://`` URL would put the bearer token and PHI on the wire in cleartext, so a - remote engine must be reached over the engine's built-in TLS (``https://``, WP-13a). Loopback - http and any https are fine; a non-loopback http URL requires an explicit ``allow_insecure`` - opt-in (trusted-network dev only), which is then loudly warned.""" + Ints are coerced because ``quote`` raises ``TypeError`` on a non-str and two alert routes take + an ``int`` id. Encoding is idempotent for the identifiers actually in use (engine ids and + connection names contain no ``%``), and no caller pre-encodes, so nothing double-encodes. + """ + return quote(str(value), safe="") + + +def _assert_safe_transport(base_url: str, *, allow_insecure: bool) -> None: + """Permit only safe URL schemes, then refuse plaintext ``http`` to a non-loopback host. + + Two checks, in this order, and the order is load-bearing. + + **Scheme allow-list (ASVS 1.2.2, BACKLOG #1107).** Only ``http`` and ``https`` are permitted. + This has to run FIRST because everything below it is keyed on the HOST, and a URL like + ``javascript:``, ``data:``, ``file:`` or an OS protocol handler (``ms-msdt:``) has no hostname + at all -- ``parts.hostname`` is ``None``, so the ``host == ""`` early return below waves it + through. That made the shipped check a host-keyed deny-list for plaintext http rather than a + protocol allow-list, and the two schemes the ASVS verb names by name were exactly the two that + passed. A positive allow-list is the shape that fixes it: a deny-list of known-bad schemes + cannot cover the OS protocol handlers a given Windows install happens to register. + + A base URL with no scheme at all fails here too (``urlsplit("127.0.0.1:8765")`` yields scheme + ``""``), which is a deliberate change: such a URL could never have reached an engine, and + failing at construction beats failing later with an opaque transport error. + + **Plaintext http to a remote host (CONSOLE-3).** A remote ``http://`` URL would put the bearer + token and PHI on the wire in cleartext, so a remote engine must be reached over the engine's + built-in TLS (``https://``, WP-13a). Loopback http and any https are fine; a non-loopback http + URL requires an explicit ``allow_insecure`` opt-in (trusted-network dev only), loudly warned. + """ parts = urlsplit(base_url) - if parts.scheme == "https": + scheme = parts.scheme.lower() + if scheme not in _ALLOWED_URL_SCHEMES: + found = f"scheme {scheme!r}" if scheme else "no scheme" + raise ApiError( + f"refusing a base URL with {found}: this client permits only the " + f"{' and '.join(sorted(_ALLOWED_URL_SCHEMES))} schemes." + ) + if scheme == "https": return host = (parts.hostname or "").lower() if host in _LOOPBACK_HOSTS or host == "": @@ -404,7 +453,7 @@ def disable_mfa(self) -> None: def reset_user_mfa(self, user_id: str) -> None: """Admin: clear a user's MFA enrollment and revoke their sessions (step-up gated).""" - self._request("POST", f"/users/{user_id}/reset-mfa") + self._request("POST", f"/users/{_seg(user_id)}/reset-mfa") # --- endpoints ----------------------------------------------------------- @@ -421,17 +470,17 @@ def connections(self) -> list[ConnectionRow]: # --- code-first connection operations ------------------------------------ def start_connection(self, name: str) -> None: - self._request("POST", f"/connections/{name}/start") + self._request("POST", f"/connections/{_seg(name)}/start") def stop_connection(self, name: str) -> None: - self._request("POST", f"/connections/{name}/stop") + self._request("POST", f"/connections/{_seg(name)}/stop") def restart_connection(self, name: str) -> None: - self._request("POST", f"/connections/{name}/restart") + self._request("POST", f"/connections/{_seg(name)}/restart") def purge_connection(self, name: str, scope: str = "all") -> PurgeResult: return _decode( - self._request("POST", f"/connections/{name}/purge", params={"scope": scope}), + self._request("POST", f"/connections/{_seg(name)}/purge", params={"scope": scope}), PurgeResult, ) @@ -510,10 +559,10 @@ def search_messages( ) def get_message(self, message_id: str) -> MessageDetail: - return _decode(self._get(f"/messages/{message_id}"), MessageDetail) + return _decode(self._get(f"/messages/{_seg(message_id)}"), MessageDetail) def replay(self, message_id: str) -> ReplayResult: - return _decode(self._request("POST", f"/messages/{message_id}/replay"), ReplayResult) + return _decode(self._request("POST", f"/messages/{_seg(message_id)}/replay"), ReplayResult) # --- dead letters -------------------------------------------------------- @@ -585,11 +634,13 @@ def active_alerts(self) -> AlertInstanceList: def ack_alert(self, alert_id: int) -> AlertInstanceInfo: """Acknowledge an open alert instance (ADR 0044). Gated by ``monitoring:diagnose``.""" - return _decode(self._request("POST", f"/alerts/{alert_id}/ack"), AlertInstanceInfo) + return _decode(self._request("POST", f"/alerts/{_seg(alert_id)}/ack"), AlertInstanceInfo) def resolve_alert(self, alert_id: int) -> AlertInstanceInfo: """Resolve an open/acknowledged alert instance (ADR 0044). Gated by ``monitoring:diagnose``.""" - return _decode(self._request("POST", f"/alerts/{alert_id}/resolve"), AlertInstanceInfo) + return _decode( + self._request("POST", f"/alerts/{_seg(alert_id)}/resolve"), AlertInstanceInfo + ) def status(self) -> SystemStatus: return _decode(self._get("/status"), SystemStatus) @@ -681,7 +732,7 @@ def list_sessions(self) -> list[SessionInfo]: def revoke_session(self, session_id: str) -> None: """Revoke one of the user's own sessions by its ``id`` (the session's ``token_hash``).""" - self._request("DELETE", f"/me/sessions/{session_id}") + self._request("DELETE", f"/me/sessions/{_seg(session_id)}") def revoke_other_sessions(self) -> str: """Revoke every session except this one ("sign out everywhere else"); returns the summary.""" @@ -689,7 +740,9 @@ def revoke_other_sessions(self) -> str: def revoke_user_sessions(self, user_id: str) -> str: """Admin force-sign-out: revoke all of ``user_id``'s sessions; returns the summary.""" - return _decode(self._request("DELETE", f"/users/{user_id}/sessions"), SimpleMessage).detail + return _decode( + self._request("DELETE", f"/users/{_seg(user_id)}/sessions"), SimpleMessage + ).detail # --- user administration ------------------------------------------------- @@ -725,10 +778,14 @@ def update_custom_role( "description": description, "permissions": permissions, } - return _decode(self._request("PUT", f"/roles/custom/{role_id}", json=body), CustomRoleInfo) + return _decode( + self._request("PUT", f"/roles/custom/{_seg(role_id)}", json=body), CustomRoleInfo + ) def delete_custom_role(self, role_id: str) -> str: - return _decode(self._request("DELETE", f"/roles/custom/{role_id}"), SimpleMessage).detail + return _decode( + self._request("DELETE", f"/roles/custom/{_seg(role_id)}"), SimpleMessage + ).detail def list_users(self) -> list[UserSummary]: return _decode_list(self._get("/users"), UserSummary) @@ -752,18 +809,18 @@ def create_user( return _decode(self._request("POST", "/users", json=body), UserSummary) def set_user_roles(self, user_id: str, roles: list[str]) -> None: - self._request("PUT", f"/users/{user_id}/roles", json={"roles": roles}) + self._request("PUT", f"/users/{_seg(user_id)}/roles", json={"roles": roles}) def get_channel_scope(self, user_id: str) -> list[str] | None: """A user's per-channel RBAC scope (``None`` = all channels).""" - return _decode(self._get(f"/users/{user_id}/channel-scope"), ChannelScope).channels + return _decode(self._get(f"/users/{_seg(user_id)}/channel-scope"), ChannelScope).channels def set_channel_scope(self, user_id: str, channels: list[str] | None) -> None: """Set a user's per-channel RBAC scope (``None`` = all channels).""" - self._request("PUT", f"/users/{user_id}/channel-scope", json={"channels": channels}) + self._request("PUT", f"/users/{_seg(user_id)}/channel-scope", json={"channels": channels}) def delete_user(self, user_id: str) -> None: - self._request("DELETE", f"/users/{user_id}") + self._request("DELETE", f"/users/{_seg(user_id)}") def audit(self, *, limit: int = 100) -> AuditList: return _decode(self._get("/audit", limit=limit), AuditList) diff --git a/tests/test_apiclient.py b/tests/test_apiclient.py index c44b19ef..8900d8c3 100644 --- a/tests/test_apiclient.py +++ b/tests/test_apiclient.py @@ -10,9 +10,13 @@ from __future__ import annotations +import contextlib import json +import pathlib import subprocess import sys +from collections.abc import Callable +from typing import Any import httpx import pytest @@ -176,3 +180,221 @@ def _capture(request: httpx.Request, *args: object, **kwargs: object) -> httpx.R assert sent == ["http://127.0.0.1:8765/messages?control_id=MSG1"], ( "the resolved URL (query included) is what the bound measures, so it is what must go out" ) + + +# --- ASVS 1.2.2 (BACKLOG #1107): contextual encoding + a URL scheme allow-list ---------------- +# +# Two clauses, and only two. Clause 1 percent-encodes the identifiers this client interpolates into +# URL PATH SEGMENTS; clause 2 replaces the host-keyed transport check with a positive URL scheme +# allow-list. The web console URL builder and the FHIR structured-parameter work are clauses 3 and 4 +# of the same item and are NOT in scope here (clause 3 already shipped in transports/fhir.py). + + +def _resolved_raw_path( + client: EngineClient, call: Callable[[EngineClient, Any], object], identifier: Any +) -> str: + """Return the path httpx would actually put on the wire for ``call(client, identifier)``. + + The subject is the RESOLVED request, so this asserts against ``httpx.Client.build_request`` -- + the same resolution step ``_request`` uses -- rather than against the f-string the method typed. + ``raw_path`` is read, never ``.path``: httpx DECODES ``.path``, so a correctly encoded ``%2F`` + reads back there as a bare ``/`` and the assertion would pass on broken code. + + A 2xx with an empty JSON body decodes fine for the methods that return ``None``, and raises + ``ApiError`` for the ones that decode a model. Either way the request was already built, which + is the only thing under test, so the decode failure is suppressed. + """ + captured: list[httpx.Request] = [] + + def _capture(request: httpx.Request, *args: object, **kwargs: object) -> httpx.Response: + captured.append(request) + return httpx.Response(200, json={}, request=request) + + original_send = client._http.send + client._http.send = _capture # type: ignore[method-assign] + try: + with contextlib.suppress(ApiError): + call(client, identifier) + finally: + client._http.send = original_send # type: ignore[method-assign] + assert captured, "the call never reached the transport, so nothing was measured" + return captured[0].url.raw_path.decode().split("?", 1)[0] + + +# Every path-segment interpolation site in the client, as (label, call, path template). The template +# holds the LITERAL route around the segment; "{seg}" is where the identifier lands. +_PATH_SEGMENT_SITES: list[tuple[str, Callable[[EngineClient, Any], object], str]] = [ + ("reset_user_mfa", lambda c, v: c.reset_user_mfa(v), "/users/{seg}/reset-mfa"), + ("start_connection", lambda c, v: c.start_connection(v), "/connections/{seg}/start"), + ("stop_connection", lambda c, v: c.stop_connection(v), "/connections/{seg}/stop"), + ("restart_connection", lambda c, v: c.restart_connection(v), "/connections/{seg}/restart"), + ("purge_connection", lambda c, v: c.purge_connection(v), "/connections/{seg}/purge"), + ("get_message", lambda c, v: c.get_message(v), "/messages/{seg}"), + ("replay", lambda c, v: c.replay(v), "/messages/{seg}/replay"), + ("ack_alert", lambda c, v: c.ack_alert(v), "/alerts/{seg}/ack"), + ("resolve_alert", lambda c, v: c.resolve_alert(v), "/alerts/{seg}/resolve"), + ("revoke_session", lambda c, v: c.revoke_session(v), "/me/sessions/{seg}"), + ("revoke_user_sessions", lambda c, v: c.revoke_user_sessions(v), "/users/{seg}/sessions"), + ("update_custom_role", lambda c, v: c.update_custom_role(v, "d", []), "/roles/custom/{seg}"), + ("delete_custom_role", lambda c, v: c.delete_custom_role(v), "/roles/custom/{seg}"), + ("set_user_roles", lambda c, v: c.set_user_roles(v, []), "/users/{seg}/roles"), + ("get_channel_scope", lambda c, v: c.get_channel_scope(v), "/users/{seg}/channel-scope"), + ("set_channel_scope", lambda c, v: c.set_channel_scope(v, None), "/users/{seg}/channel-scope"), + ("delete_user", lambda c, v: c.delete_user(v), "/users/{seg}"), +] + + +def test_the_path_segment_site_table_covers_every_interpolation_in_the_client() -> None: + """Guard the guard: the table above is only evidence if it is the WHOLE population. + + Counts the interpolated path literals in the client source and requires the table to match. A + new endpoint that interpolates an identifier reds this test rather than slipping in unencoded. + + Mutation: delete a row from ``_PATH_SEGMENT_SITES``. Red: the two counts disagree, and the + failure prints the literals it found so the difference is readable rather than a bare number.""" + import re + + from messagefoundry.apiclient import client as client_module + + source = pathlib.Path(client_module.__file__).read_text(encoding="utf-8") + interpolated = re.findall(r'f"(/[^"]*\{[^"]*)"', source) + assert len(interpolated) == len(_PATH_SEGMENT_SITES), ( + f"the client has {len(interpolated)} interpolated path literals but the table covers " + f"{len(_PATH_SEGMENT_SITES)}; the literals found were {interpolated}" + ) + + +@pytest.mark.parametrize(("label", "call", "template"), _PATH_SEGMENT_SITES, ids=lambda v: v) +def test_apiclient_percent_encodes_every_interpolated_path_segment( + label: str, call: Callable[[EngineClient, Any], object], template: str +) -> None: + """ASVS 1.2.2 clause 1: an identifier carrying path metacharacters must land in ONE segment. + + ``../..`` is the sharp case. Unencoded it does not merely look wrong -- httpx resolves it and + the request RETARGETS, so ``start_connection("../../users/admin")`` leaves ``/connections/`` + altogether. Four of these sites carry a connection NAME, which is unconstrained free text + (``Registry._add`` in config/wiring.py checks only for a duplicate), so the "every id is a + uuid4 hex" argument does not cover them. + + Mutation: drop the encode helper at any one site. Red: that site's resolved path is the escaped + or split form instead of the single-segment one, and the message names the site.""" + client = EngineClient("http://127.0.0.1:8765") + try: + resolved = _resolved_raw_path(client, call, "../../users/admin") + finally: + client.close() + assert resolved == template.format(seg="..%2F..%2Fusers%2Fadmin"), ( + f"{label}: the identifier escaped its path segment; resolved to {resolved!r}" + ) + + +@pytest.mark.parametrize( + ("hostile", "encoded"), + [ + ("../../users/admin", "..%2F..%2Fusers%2Fadmin"), + ("a/b", "a%2Fb"), + ("x?scope=all", "x%3Fscope%3Dall"), + ("x#frag", "x%23frag"), + ], + ids=["dot-dot", "slash", "question", "hash"], +) +def test_apiclient_path_metacharacters_cannot_change_the_resolved_path( + hostile: str, encoded: str +) -> None: + """The four metacharacters the item names, against one representative site. + + Each breaks the resolved request differently on unencoded code: ``..`` retargets the route, + ``/`` splits the segment, ``?`` starts a query, and ``#`` TRUNCATES the path at the fragment -- + so ``start_connection("x#frag")`` resolves to ``/connections/x`` and the ``/start`` verb is gone. + + Mutation: revert the helper at start_connection. Red: the resolved path is the mangled form.""" + client = EngineClient("http://127.0.0.1:8765") + try: + resolved = _resolved_raw_path(client, lambda c, v: c.start_connection(v), hostile) + finally: + client.close() + assert resolved == f"/connections/{encoded}/start", ( + f"{hostile!r} changed the resolved path to {resolved!r}" + ) + + +@pytest.mark.parametrize(("label", "call", "template"), _PATH_SEGMENT_SITES, ids=lambda v: v) +def test_apiclient_leaves_a_plain_identifier_untouched( + label: str, call: Callable[[EngineClient, Any], object], template: str +) -> None: + """NEGATIVE CONTROL for the encoding tests above, and it is not optional. + + An encoder that mangled every identifier would satisfy the hostile-input assertions perfectly + while breaking every real call. This pins that an ordinary identifier -- the shape the API + actually receives -- rides through byte-identical. + + Mutation: encode an already-encoded value a second time (double-encoding). Red: the plain + identifier comes back percent-mangled.""" + client = EngineClient("http://127.0.0.1:8765") + try: + resolved = _resolved_raw_path(client, call, "IB_ACME_ADT") + finally: + client.close() + assert resolved == template.format(seg="IB_ACME_ADT"), ( + f"{label}: a plain identifier was altered; resolved to {resolved!r}" + ) + + +def test_apiclient_still_accepts_an_integer_identifier() -> None: + """``ack_alert``/``resolve_alert`` take an ``int``, and ``urllib.parse.quote`` raises + ``TypeError`` on a non-str, so the helper has to coerce. This is the test that says so. + + Mutation: drop the ``str()`` coercion in the helper. Red: TypeError, not an assertion.""" + client = EngineClient("http://127.0.0.1:8765") + try: + resolved = _resolved_raw_path(client, lambda c, v: c.ack_alert(v), 7) + finally: + client.close() + assert resolved == "/alerts/7/ack", f"an int alert id resolved to {resolved!r}" + + +@pytest.mark.parametrize( + "base_url", + [ + "javascript:alert(1)", + "data:text/html,", + "file:///C:/Windows/win.ini", + "ms-msdt:/id", + ], + ids=["javascript", "data", "file", "os-protocol-handler"], +) +def test_transport_guard_refuses_a_non_http_url_scheme(base_url: str) -> None: + """ASVS 1.2.2 clause 2: only safe URL protocols are permitted, as a POSITIVE allow-list. + + The shipped check is host-keyed -- it returns early when the host is loopback OR empty. None of + these four URLs has a hostname, so ``host == ""`` and every one of them builds a client today, + including the two schemes the ASVS verb names by name. + + Mutation: move the allow-list below the ``host == ""`` early return. Red: DID NOT RAISE.""" + with pytest.raises(ApiError, match="scheme"): + EngineClient(base_url) + + +def test_transport_guard_refuses_a_base_url_with_no_scheme() -> None: + """A schemeless base_url is a typo, and today it builds a client that can never work: urlsplit + reads ``127.0.0.1:8765`` as scheme ``""`` and ``localhost:8765`` as scheme ``localhost``, both + with no hostname, so both slip through the ``host == ""`` early return. + + This is a DELIBERATE behavior change, pinned here so it stays a decision rather than a side + effect: an allow-list admitting only ``http`` and ``https`` refuses both. Failing at + construction beats failing on the first request with a transport error. Every in-repo caller + passes an explicit scheme, so nothing shipped changes.""" + with pytest.raises(ApiError, match="scheme"): + EngineClient("127.0.0.1:8765") + with pytest.raises(ApiError, match="scheme"): + EngineClient("localhost:8765") + + +def test_transport_guard_permits_https_and_loopback_http() -> None: + """NEGATIVE CONTROL for the allow-list: the two schemes the client exists to speak must pass. + + Without this, an allow-list that refused everything would look identical to a correct one. The + plaintext-http refusal for a REMOTE host is a separate control with its own test above -- + ``http`` has to clear the allow-list and then still meet that check, message intact.""" + EngineClient("https://engine.example.com:8765").close() + EngineClient("http://127.0.0.1:8765").close() From d31ea1b51e197bcaf4f98504dd8e23942a577785 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 19:32:09 -0500 Subject: [PATCH 04/31] test(codesets): pin every refusal branch on the rename source, not just the separator (BACKLOG #1130) Closes a test-strength gap its own adversarial pass found and e5505a96 did not fix. The traversal test matches only "must not contain a path separator", so a deliberately weaker guard -- `if "/" in old or "\\" in old: raise` -- satisfies it while leaving the other branches unguarded on the rename SOURCE. `_validate_name` has SEVEN refusal branches, not the five the review reported: non-empty, control characters, path separator, "..", absolute-or-drive, bare stem, and escapes-the-directory. The new parametrize covers eight inputs. PROVEN TO FAIL FOR THE STATED REASON, not merely written and watched go green. Planted the narrowed guard at the call site and got exactly the predicted split: RED dotdot, embedded-dotdot, suffix, control, blank PASS slash, backslash, absolute -- the three a separator-only guard catches That asymmetry is the point. A uniform failure would not distinguish "this test covers the branch" from "this test fails on anything". Revert verified byte-identical, sha256 8d4a1672...52fd01cc before the plant and after it. The mutation is named in the docstring so the next reader can re-run it instead of trusting this message. STILL NOT DONE, from the same review: the corepoint census is blind to escape classes its own docstring claims to cover, because it censuses with rglob over the output root, so a name resolving INSIDE the root is visible to it while other classes are not. Separate test, not in this commit. No engine code changed. No ASVS score moves. --- tests/test_codeset_edit.py | 43 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/test_codeset_edit.py b/tests/test_codeset_edit.py index b6bebd5a..9cce6fe1 100644 --- a/tests/test_codeset_edit.py +++ b/tests/test_codeset_edit.py @@ -528,6 +528,49 @@ def test_rename_rejects_traversal_source_without_moving_a_file_into_the_dir(tmp_ assert not (_codesets(tmp_path) / "stolen.csv").exists() +@pytest.mark.parametrize( + ("source", "expected"), + [ + ("a/b", "must not contain a path separator"), + ("a\\b", "must not contain a path separator"), + ("..", "must not contain '..'"), + ("x..y", "must not contain '..'"), + ("/etc/passwd", "must not contain a path separator"), + ("lab.results", "must be a bare stem"), + ("bad\x01name", "must not contain control characters"), + (" ", "must be a non-empty string"), + ], + ids=[ + "slash", + "backslash", + "dotdot", + "embedded-dotdot", + "absolute", + "suffix", + "control", + "blank", + ], +) +def test_rename_source_clears_every_refusal_branch_not_just_the_separator( + tmp_path: Path, source: str, expected: str +) -> None: + """Each refusal branch of ``_validate_name``, exercised on the rename SOURCE. + + The traversal test above matches only ``must not contain a path separator``, so a deliberately + weaker guard -- ``if "/" in old or "\\\\" in old: raise`` -- would satisfy it while leaving the + ``..``, control-character, suffix and empty branches unguarded on ``old``. Found by the + adversarial pass on BACKLOG #1130, which named the test as pinning one branch of seven. + + Mutation: narrow the ``_validate_name(codesets_dir, old)`` call at :182 to a separator-only + check. Red: every id except ``slash``, ``backslash`` and ``absolute``. + """ + _codesets(tmp_path).mkdir() + with pytest.raises(WiringError, match=expected): + codeset_edit.rename_code_set(tmp_path, source, "target", validate=_validate) + assert not (_codesets(tmp_path) / "target.csv").exists() + assert not (_codesets(tmp_path) / "target.toml").exists() + + def test_rename_collision_raises(tmp_path: Path) -> None: codeset_edit.upsert_code_set( tmp_path, "diets", ["code", "value"], [["A", "Apple"]], validate=_validate From 9d59ff43cd946c60811af6094ccbec9a8bb813bf Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 19:43:48 -0500 Subject: [PATCH 05/31] test(corepoint): census escapes the out root cannot contain, and record the real red (BACKLOG #1130) Closes the second test-strength gap its adversarial pass found. The existing census rglobs from `root`, and the helper nests the out dir three levels under it so `../../../x` still lands inside. THAT COVERS EXACTLY ONE ESCAPE CLASS. A deeper traversal, a POSIX-absolute path, a drive-letter path and a UNC path all resolve ABOVE `root`, outside that rglob -- where the escape is invisible and the empty result reads as containment. The helper now takes a wider census base, and the new test passes `tmp_path`: the widest base it can see, containing every target those classes reach in-process. THE MUTATION'S RED IS NOT THE ONE I FIRST PREDICTED, and the docstring now says so rather than being quietly corrected. I wrote "red: `outside` is non-empty". Measured: five of six ids go red with FileNotFoundError on the write, because the unsanitized stem names a parent directory the importer never created. That still proves the name reached the path unsanitized -- but it is a WRITE FAILURE, not an escape the census caught. A reader re-running the mutation and expecting a non-empty census would see an unrelated-looking error and distrust the test. A refusal for a different reason is indistinguishable from the refusal you asked for, so the docstring names the observed mechanism. `bare-dotdot` PASSES under the plant and is kept as a must-not-trip control: `..` alone resolves to the out dir's parent as a directory, producing no write for the census to catch. If it ever starts failing, the fold changed shape. Revert verified byte-identical, sha256 ff13e9a63fdb736f6860e4216b0dd9d4b1f27b0465c22f6ec3d60d68f0d0925f before the plant and after it. No engine code changed. No ASVS score moves. Both #1130 test-strength findings from that review are now closed. --- tests/test_corepoint_import.py | 60 +++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/tests/test_corepoint_import.py b/tests/test_corepoint_import.py index d266cab2..b7f95cd6 100644 --- a/tests/test_corepoint_import.py +++ b/tests/test_corepoint_import.py @@ -323,17 +323,25 @@ def _named_inbound_export(inbound_name: str) -> str: ) -def _import_and_census(inbound_name: str, root: Path) -> tuple[list[Path], list[Path]]: - """Import an export naming ``inbound_name``, then census EVERY .py under ``root``. +def _import_and_census( + inbound_name: str, root: Path, census_base: Path | None = None +) -> tuple[list[Path], list[Path]]: + """Import an export naming ``inbound_name``, then census EVERY .py under ``census_base``. Returns ``(inside_out_dir, outside_out_dir)``. The out dir is nested several levels below ``root`` so a traversal escape still lands inside the temp tree and can be seen, rather than - escaping the census and reading as containment.""" + escaping the census and reading as containment. + + ``census_base`` defaults to ``root`` and exists because that nesting only covers escapes SHALLOW + ENOUGH to stay under ``root``. A deeper traversal, an absolute path or a drive-letter path lands + ABOVE it, where an ``rglob`` rooted at ``root`` cannot see it -- and an escape the census cannot + see reads exactly like containment. Pass a wider base to close that. Found by the adversarial + pass on BACKLOG #1130.""" out = root / "a" / "b" / "c" / "out" export = root / "export.json" export.write_text(_named_inbound_export(inbound_name), encoding="utf-8") import_corepoint(export, out) - found = sorted(root.rglob("*.py")) + found = sorted((census_base or root).rglob("*.py")) return ( [p for p in found if p.is_relative_to(out)], [p for p in found if not p.is_relative_to(out)], @@ -361,6 +369,50 @@ def test_a_hostile_inbound_name_cannot_write_outside_the_output_directory(tmp_pa assert len(hostile_inside) == 1 +@pytest.mark.parametrize( + "hostile", + [ + "../../../../../../evil", + "/etc/cron.d/evil", + "C:\\Windows\\Temp\\evil", + "\\\\server\\share\\evil", + "a/b/evil", + "..", + ], + ids=["deep-traversal", "posix-absolute", "drive-absolute", "unc", "subdir", "bare-dotdot"], +) +def test_no_hostile_inbound_name_escapes_a_census_WIDER_than_the_out_root( + tmp_path: Path, hostile: str +) -> None: + """Escape classes the original census could not have seen, so its silence meant nothing. + + The helper nests the out dir three levels under ``root`` so that ``../../../x`` still lands + inside ``root``. That covers exactly one class. A DEEPER traversal, a POSIX-absolute path, a + drive-letter path or a UNC path all resolve ABOVE ``root`` -- outside an ``rglob`` rooted there, + where the escape is invisible and the empty result reads as containment. + + Censusing from ``tmp_path`` instead is what makes the assertion mean anything: it is the widest + base this test can see, and it contains every target the classes above can reach in-process. + + Mutation: drop the ``_sanitize`` call at ``corepoint_import.py``'s ``module_name`` site. + MEASURED RED, and it is NOT the failure this docstring first predicted -- recorded exactly + because the two are easy to confuse. Five of six ids go red; the observed mechanism is + ``FileNotFoundError`` on the write, because the unsanitized stem names a parent directory the + importer never created. That still proves the thing under test -- the name reached the + filesystem path unsanitized -- but it is a WRITE FAILURE, not a non-empty ``outside`` census. + A reader re-running the mutation and expecting an escape would see an unrelated-looking error + and distrust the test. + + ``bare-dotdot`` PASSES under the plant and is kept deliberately: ``..`` alone resolves to the out + dir's parent as a directory rather than a new stem, so it produces no write for the census to + catch. It is a must-not-trip control -- if it ever starts failing, the fold changed shape.""" + root = tmp_path / "hostile" + root.mkdir() + inside, outside = _import_and_census(hostile, root, census_base=tmp_path) + assert outside == [], f"{hostile!r} wrote outside the out dir: {outside}" + assert len(inside) == 1, f"{hostile!r} did not produce exactly one module: {inside}" + + def test_malformed_export_raises() -> None: with pytest.raises(CorepointImportError): parse_export("{ not json ") From c52927b68ea926fc8380f3f042019067323c03f5 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 20:24:40 -0500 Subject: [PATCH 06/31] fix(transports): refuse ech_egress where the SNI cannot be hidden, and route the token hop through the sidecar (BACKLOG #1176) TWO LIMBS, both product defects. NEITHER MOVES THE ASVS SCORE and the cell stays `fail` -- ECH is unreachable on this runtime (zero ECH attributes on the pinned interpreter, measured here), so nothing in this commit makes a pass possible. That is the item's own conclusion and not mine to change. LIMB 1 -- silent acceptance. `ech_egress` was refused only by connectors routing through `egress_route_from_settings` (fhir, soap, dicomweb, rest). MLLP, TCP, X12, file, email and DICOM never reach it, so they BUILT and silently ignored the key: an operator sets an SNI-hiding flag and nothing hides anything. The refusal is hoisted into the shared construction seam rather than copied per connector, so a connector added later inherits it. LIMB 2 -- the seam that leaked even where ECH works. On REST the proxy was forced to None whenever a sidecar was set, and http_auth fell back to the plain opener, so `ech_egress` plus an OAuth2 or SMART token endpoint sent the AUTHORIZATION SERVER's hostname in a cleartext outer ClientHello. A refusal keyed on "does this destination implement the send path" cannot reach that, because REST does implement it. RED PROOF, tests written first against unmodified HEAD 4633a295: `DID NOT RAISE ValueError` six times, one per connector, plus the inbound limb, plus the OAuth2 and SMART hops asserting the direct URL. DID NOT RAISE is the evidence the defect was real -- those connectors built and accepted the key. ADVERSARIAL PASS: sound, test_can_fail true. Two plants, each red for its documented reason, five files sha256-identical after revert. FINDINGS FROM THAT PASS, KEPT BECAUSE A GREEN VERDICT HIDES THEM: - test_both_refusal_sites_carry_the_same_message DOES NOT witness the seam its variable names claim. It PASSED under the plant that deleted the hoisted refusal, so it asserts something true but not that. NOT FIXED HERE. - `ech_sidecar` set WITHOUT `ech_egress` is still silently ignored on every connector, REST included -- a residual inside the item's own hypothesis. - Routing the token POST through the sidecar makes the AUTH hop inherit the sidecar contract's fail-closed rule. A real semantic change, undisclosed by the implementer and recorded here instead. - Three provider docstrings assert `proxy` and `ech_sidecar` are "mutually exclusive by construction"; nothing in those functions enforces it. docs/SECURITY.md is UNTOUCHED -- another lane owns it. Two edits reported as text, both inside the single ECH bullet. Module: 38 passed. Transports seam: 2787 passed, 141 skipped, 0 failed. --- .../16-security-phi-and-supply-chain.md | 2 +- messagefoundry/transports/base.py | 31 +++ messagefoundry/transports/http_auth.py | 46 +++- messagefoundry/transports/rest.py | 74 ++++-- messagefoundry/transports/smart.py | 38 ++- samples/ech-sidecar/README.md | 17 +- tests/test_ech_egress.py | 241 +++++++++++++++++- 7 files changed, 399 insertions(+), 50 deletions(-) diff --git a/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md b/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md index b49f2828..3e4e452e 100644 --- a/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md +++ b/docs/testing/master-test-plan/16-security-phi-and-supply-chain.md @@ -320,7 +320,7 @@ plan/matrix. A bare ID is this plan's own row. | SEC-68 | CVE tabletop + security-advisory dry run | Usability | manual | any | n/a | C | P2 | Recorded as unrun in `docs/Secure_Build_Scorecard_MEFOR.md` signal 9. Run one: a simulated CVE in a pinned dependency exercised end to end — pip-audit red → triage → VEX statement (SEC-10/SEC-11 discipline) → lock bump → release → advisory published. Pass = a dated written record and any process gaps filed. | | SEC-69 | ASVS assessment corpus is reachable by a drift guard | Negative/Security | manual | any | n/a | C | P1 | The ASVS assessment corpus is **real and maintained** — it is simply **withheld from the public repo**: `docs/security/` is gitignored post-cutover (`.gitignore:144`, ~32 files of posture / assessment / risk-register / runbook detail deliberately not published as an attacker roadmap), as are `docs/reviews/` and `docs/marketing/` (`:145-146`). Nothing here is missing; what is missing is a **linkage a public CI job can read**. Pass = a dated decision plus action: either a machine-readable **public** subset (requirement → control → code artefact) lands in-tree so a drift test can hold the shipped code to the assessment, or the risk register records that no automated linkage exists and names who re-checks it by hand and how often. `FEATURE-MAP.md:136`'s citation of **BACKLOG #310** is sound (above the published #231 baseline) and stays. | | SEC-70 | ADR 0148 re-score and owner re-signature | Usability | manual | any | n/a | C | P1 | The ADR 0148 status line records the per-cell scorecard re-score and owner re-signature as **pending**. Pass = the re-score is complete and signed, or the pending note carries a date and an owner. Blocks any quotable ASVS figure. | -| SEC-71 | ECH disposition — **including the shipped `tools/ech-sidecar/` tree** | Negative/Security | manual | any | n/a | C | P2 | **DISCHARGED 2026-08-10 (BACKLOG #1011) — the owner ruled RETIRE.** All three artefacts are dispositioned: (a) the engine-side routing (`ech_sidecar_url_from_settings` / `egress_route_from_settings`, `transports/rest.py:1077`) **stays** — it does not itself originate ECH and its docstring no longer claims a path that exists; (b) the stdlib-only Go re-originator was **deleted** from the tree, recoverable at `git show 62fd628d:tools/ech-sidecar/main.go`; (c) the operator recipe `samples/ech-sidecar/README.md` **stays**, re-aimed at the generic contract and carrying that retrieval SHA. ADR 0139's status line, *Implementation status* block and acceptance checklist are reconciled to the ruling, and `docs/SECURITY.md`'s 12.1.5 paragraph is rewritten off "infeasible" — a false premise the tree itself refuted — onto the true one: **buildable off-stdlib, deliberately not owned, inert because no partner publishes an `ECHConfig` (2026-07-20 DoH probe)**. ASVS 12.1.5 is recorded as a **standing accepted `fail`**, unchanged by the ruling in either direction. The original evidence stands for the record: nothing built, tested, linted, version-pinned or shipped the Go tree — zero `setup-go` / `go build` hits across `.github/workflows/`, `ci/`, `scripts/`, `.pre-commit-config.yaml` and `tests/`, and `pyproject.toml:21` `only-include` kept `tools/` out of both sdist and wheel. `tests/test_ech_egress.py` stays as the fail-closed guard and now states its own scope limit (the far end is always a stub; nothing in the suite originates or observes ECH). **Recorded as discharged rather than converted to a T row:** the ruling creates no new automatable criterion, and the fail-closed guard is already owned. | +| SEC-71 | ECH disposition — **including the shipped `tools/ech-sidecar/` tree** | Negative/Security | manual | any | n/a | C | P2 | **DISCHARGED 2026-08-10 (BACKLOG #1011) — the owner ruled RETIRE.** All three artefacts are dispositioned: (a) the engine-side routing (`ech_sidecar_url_from_settings` / `egress_route_from_settings` in `transports/rest.py` — by symbol, because the line number this row used to carry landed on a blank line and could never self-resolve) **stays** — it does not itself originate ECH and its docstring no longer claims a path that exists; (b) the stdlib-only Go re-originator was **deleted** from the tree, recoverable at `git show 62fd628d:tools/ech-sidecar/main.go`; (c) the operator recipe `samples/ech-sidecar/README.md` **stays**, re-aimed at the generic contract and carrying that retrieval SHA. ADR 0139's status line, *Implementation status* block and acceptance checklist are reconciled to the ruling, and `docs/SECURITY.md`'s 12.1.5 paragraph is rewritten off "infeasible" — a false premise the tree itself refuted — onto the true one: **buildable off-stdlib, deliberately not owned, inert because no partner publishes an `ECHConfig` (2026-07-20 DoH probe)**. ASVS 12.1.5 is recorded as a **standing accepted `fail`**, unchanged by the ruling in either direction. The original evidence stands for the record: nothing built, tested, linted, version-pinned or shipped the Go tree — zero `setup-go` / `go build` hits across `.github/workflows/`, `ci/`, `scripts/`, `.pre-commit-config.yaml` and `tests/`, and `pyproject.toml:21` `only-include` kept `tools/` out of both sdist and wheel. `tests/test_ech_egress.py` stays as the fail-closed guard and now states its own scope limit (the far end is always a stub; nothing in the suite originates or observes ECH). **Recorded as discharged rather than converted to a T row:** the ruling creates no new automatable criterion, and the fail-closed guard is already owned. | | SEC-72 | `security.yml` header matches its own triggers | Negative/Security | pytest | any | n/a | T | P2 | **The `security.yml` half is BUILT (BACKLOG #1079)**, in `tests/test_security_posture.py::test_the_security_header_does_not_contradict_its_own_triggers` rather than the `tests/test_security_workflow_liveness.py` this row named — that module does not exist, and the posture module is where every other `security.yml` assertion already lives. It reads the `on:` block (handling the YAML 1.1 `on` -> `True` key), locates the header by construct, and refuses a header denial adjacent to any declared event name, with the historical claim kept as a live positive control. Its scope is stated in the test: a tripwire on the shape that occurred, not a proof that English agrees with YAML. **STILL OPEN:** the `codeql.yml` and `scorecard.yml` header comments still claim version-tag pinning / a pending SHA-pin lookup while every `uses:` in both carries a 40-char SHA (finding 2 above) — nothing asserts that, and this row is not closed until it does. | | SEC-73 | Last-resort handler leaks no PHI on either unhandled path | PHI | pytest | any | SQLite | T | P1 | The PHI-egress twin of SEC-46, one layer up: `messagefoundry/last_resort.py` (ASVS 16.5.4) routes otherwise-unhandled exceptions through `redaction.safe_exc` on **both** paths — the asyncio loop handler (`install_loop_exception_handler`, installed at `api/app.py:5263`) and the main-thread hook (`install_excepthook`, installed at `__main__.py:2440`) — so no raw traceback, which could quote a PHI-bearing argument, escapes. `tests/test_last_resort.py` (104) proves this at unit level and names its own residual: it does not prove the handlers are **installed in a real serving process**, nor that the redacted record stays clean across *every* configured sink. Extend `tests/test_phi_exception_sweep.py` (SEC-46's module) with that arm: (a) after a real `serve` startup, assert `loop.get_exception_handler()` and `sys.excepthook` are the project's, not the interpreter defaults; (b) induce an unhandled exception on **each** path — a fire-and-forget asyncio task and a main-thread raise — whose argument carries a synthetic PHI sentinel; (c) assert the sentinel appears **zero** times in the stdout capture, every `[logging]` file sink, the syslog forwarder stub, the audit row, `/metrics` and a support bundle taken afterwards, while the exception **type** still appears (so the row cannot pass by swallowing the failure). `KeyboardInterrupt` must still reach `sys.__excepthook__` untouched. | | SEC-74 | `netaddr` allow-list parity across its two callers | Negative/Security | pytest | any | n/a | T | P1 | New `tests/test_netaddr_parity.py`. `messagefoundry/netaddr.py` exists to be "the ONE place an IP allow-list decision is made" — its entire value is that its two callers cannot disagree about what an entry means: the inbound connectors' per-connection `source_ip_allowlist` (`peer_ip_allowed`, called from `transports/mllp.py:1419`, `tcp.py:495`, `dicom.py:263`, `http_listener.py:374`) and `[security].allowed_client_networks` (`client_network_allowed`, called from `api/client_networks.py:159`). Drive **one shared table** of (address, allow-list) cases through **both** callers and assert an identical decision per cell: bare IPv4, IPv4 CIDR, bare IPv6, IPv6 CIDR, an IPv4-mapped IPv6 peer (`::ffff:a.b.c.d`) against an IPv4 entry, `/32` and `/128`, a host-bits-set entry (`strict=False`), a malformed entry (skipped defensively), an unresolvable/`None` peer (fail closed), a non-parsing literal such as starlette's `"testclient"` (denied), and an empty/`None` list (permit all). **Exactly one divergence is sanctioned and the test must assert it is the only one:** loopback is unconditionally allowed by `client_network_allowed` (`netaddr.py:95-108`) and is **not** allowed by `peer_ip_allowed`, because an ingest listener allow-listing a partner must never silently also admit the local box. A new divergence in either direction reds the suite. Today `tests/test_client_network_allowlist.py` (725) and `tests/test_x12_source_ip_allowlist.py` each exercise one caller; nothing compares them — which is precisely the drift the co-location was built to prevent. | diff --git a/messagefoundry/transports/base.py b/messagefoundry/transports/base.py index 2478fc2c..79683ded 100644 --- a/messagefoundry/transports/base.py +++ b/messagefoundry/transports/base.py @@ -51,6 +51,8 @@ "register_destination", "build_source", "build_destination", + "ECH_UNSUPPORTED_DESTINATION_MSG", + "ECH_UNSUPPORTED_SOURCE_MSG", "peer_ip_allowed", "probe_tcp_reachable", ] @@ -538,11 +540,38 @@ def register_destination(kind: ConnectorType, builder: DestinationBuilder) -> No _DESTINATIONS[kind] = builder +# --- ECH egress: refuse the key where it would be a silent no-op (ADR 0139, ASVS 12.1.5, #1176) ---- +# +# `ech_egress` routes a connection's egress through a loopback ECH sidecar so the outbound SNI is +# hidden. Exactly ONE connector implements that send path (the REST destination); every other one +# would build happily and ignore the key, leaving an ordinary SNI-leaking handshake behind an operator +# belief that routing was on. The refusal lives HERE, in the seam every connector is constructed +# through, rather than copied into each connector: a per-connector copy is exactly what left the rest +# of them silently accepting it, and a copy has to be remembered every time a connector is added. +# This is a self-contained settings-key check on purpose -- base.py must not import rest.py (the +# dependency runs the other way), so it cannot call rest.py's resolver. + +_ECH_ROUTING_DESTINATIONS: frozenset[ConnectorType] = frozenset({ConnectorType.REST}) + +ECH_UNSUPPORTED_DESTINATION_MSG = ( + "ech_egress (ASVS 12.1.5 SNI hiding) is supported only on the REST destination in this " + "build; on this connector it would NOT hide the SNI, so it is refused rather than silently " + "leaking it (ADR 0139)" +) + +ECH_UNSUPPORTED_SOURCE_MSG = ( + "ech_egress (ASVS 12.1.5 SNI hiding) is an outbound setting -- no inbound connector originates " + "the TLS handshake it would hide, so it is refused rather than silently ignored (ADR 0139)" +) + + def build_source(config: Source) -> SourceConnector: try: builder = _SOURCES[config.type] except KeyError: raise ValueError(f"no source connector registered for {config.type.value!r}") from None + if config.settings.get("ech_egress"): + raise ValueError(ECH_UNSUPPORTED_SOURCE_MSG) return builder(config) @@ -551,6 +580,8 @@ def build_destination(config: Destination) -> DestinationConnector: builder = _DESTINATIONS[config.type] except KeyError: raise ValueError(f"no destination connector registered for {config.type.value!r}") from None + if config.settings.get("ech_egress") and config.type not in _ECH_ROUTING_DESTINATIONS: + raise ValueError(ECH_UNSUPPORTED_DESTINATION_MSG) return builder(config) diff --git a/messagefoundry/transports/http_auth.py b/messagefoundry/transports/http_auth.py index e6ff1b35..5127e0a2 100644 --- a/messagefoundry/transports/http_auth.py +++ b/messagefoundry/transports/http_auth.py @@ -52,6 +52,7 @@ _no_redirect_opener, _redact_url, cleartext_acceptance_from_settings, + ech_readdressed_request, enforce_outbound_length_limits, proxy_auth_handler_from_settings, refuse_cleartext_credential_hop, @@ -131,6 +132,13 @@ def __init__( cleartext_reason: str | None = None, connection: str | None = None, proxy: ProxyConfig | None = None, + # #1176 (ADR 0139): this connection's loopback ECH sidecar, when it has one. The token-endpoint + # POST follows the connection's egress route exactly as ADR 0126 rules it must for a forward + # proxy; for ECH that means the request is RE-ADDRESSED to the sidecar with the real + # authorization-server host in ``Host``, so the AS hostname is never in a cleartext outer + # ClientHello. Mutually exclusive with ``proxy`` (refused at connector construction). None + # (default) -> byte-identical. + ech_sidecar: str | None = None, ) -> None: if not token_url: raise HttpAuthError("OAuth2 client-credentials requires an 'oauth2_token_url' setting") @@ -199,10 +207,25 @@ def __init__( self._proxy_auth: dict[str, str] = ( token_proxy.auth_headers() if token_proxy is not None else {} ) + self._ech_sidecar = ech_sidecar self._lock = threading.Lock() self._cached_token: str | None = None self._cached_expiry_monotonic = 0.0 + def _token_request(self, data: bytes, headers: dict[str, str]) -> urllib.request.Request: + """The token-endpoint POST, on this connection's egress route. With an ECH sidecar the request + is re-addressed to it (#1176); without one it goes straight to the configured ``token_url``, + byte-identical. The cleartext-credential refusal above keys on the DECLARED ``token_url`` + scheme, which is what the sidecar re-originates — the engine->sidecar leg is same-host loopback + (ADR 0092), exactly as the delivery hop's is.""" + if self._ech_sidecar is not None: + return ech_readdressed_request( + self._ech_sidecar, self.token_url, data=data, headers=headers, method="POST" + ) + return urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) above + self.token_url, data=data, headers=headers, method="POST" + ) + def access_token(self) -> str: """A valid bearer token — cached until it nears expiry, else freshly acquired. Blocking (a token ``POST``); called inside the connector's off-loop ``send()`` worker. Raises @@ -248,9 +271,7 @@ def _fetch_token(self) -> tuple[str, float]: # env(), so an env value that resolved to an unexpected blob would otherwise surface as an # opaque IdP-side failure on the first mint rather than as a clear config error. enforce_outbound_length_limits(self.token_url, dict(headers)) - req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) above - self.token_url, data=data, headers=headers, method="POST" - ) + req = self._token_request(data, headers) try: with self._opener.open(req, timeout=self.timeout_seconds) as resp: body = resp.read().decode("utf-8", errors="replace") @@ -287,12 +308,14 @@ def _parse_token_response(self, body: str) -> tuple[str, float]: def oauth2_cc_provider_from_settings( - s: Mapping[str, Any], *, proxy: ProxyConfig | None = None + s: Mapping[str, Any], *, proxy: ProxyConfig | None = None, ech_sidecar: str | None = None ) -> OAuth2ClientCredentialsProvider | None: """The :class:`OAuth2ClientCredentialsProvider` for an ``env()``-resolved settings mapping, or ``None`` when symmetric OAuth2-CC auth is off (``oauth2_token_url`` absent, or ``oauth2_enabled`` is False) — so any connection that didn't configure it is byte-identical. ``proxy`` (ADR 0126) routes the - token-endpoint POST through the connection's forward proxy.""" + token-endpoint POST through the connection's forward proxy; ``ech_sidecar`` (#1176, ADR 0139) + re-addresses it to the connection's loopback ECH sidecar instead. The two are mutually exclusive by + construction.""" if not s.get("oauth2_token_url"): return None if not s.get("oauth2_enabled", True): @@ -318,18 +341,21 @@ def oauth2_cc_provider_from_settings( cleartext_reason=_accepted[1], connection=_accepted[2], proxy=proxy, # ADR 0126: forward-proxy the token-endpoint POST + ech_sidecar=ech_sidecar, # #1176: ...or re-address it to the ECH sidecar (ADR 0139) ) def bearer_provider_from_settings( - s: Mapping[str, Any], *, proxy: ProxyConfig | None = None + s: Mapping[str, Any], *, proxy: ProxyConfig | None = None, ech_sidecar: str | None = None ) -> BearerTokenProvider | None: """The active bearer-token provider for an HTTP destination, or ``None`` when none is configured (byte-identical). Unifies the SMART Backend Services provider (ADR 0024, asymmetric JWT) and the OAuth2 client-credentials provider (#65, symmetric secret) behind the one bearer seam the connector drives. The two are **mutually exclusive** on one connection — configuring both is a loud :class:`HttpAuthError` (a connection has exactly one identity). ``proxy`` (ADR 0126) routes whichever - provider's token-endpoint call through the connection's forward proxy.""" + provider's token-endpoint call through the connection's forward proxy; ``ech_sidecar`` (#1176, + ADR 0139) re-addresses it to the connection's loopback ECH sidecar instead, so the ECH connection's + token hop stops leaking the authorization server's SNI while its payload hop is routed.""" # Detect the conflict from settings PRESENCE before constructing either provider, so a "both # configured" mistake reports the mutual-exclusion error rather than whichever provider's own # validation happens to fire first on partial config. @@ -340,9 +366,9 @@ def bearer_provider_from_settings( "a connection cannot use BOTH SMART Backend Services and OAuth2 client-credentials auth " "(mutually exclusive — configure exactly one)" ) - return token_provider_from_settings(s, proxy=proxy) or oauth2_cc_provider_from_settings( - s, proxy=proxy - ) + return token_provider_from_settings( + s, proxy=proxy, ech_sidecar=ech_sidecar + ) or oauth2_cc_provider_from_settings(s, proxy=proxy, ech_sidecar=ech_sidecar) def digest_handler_from_settings( diff --git a/messagefoundry/transports/rest.py b/messagefoundry/transports/rest.py index d6fbf7ad..dd0b365d 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -56,6 +56,7 @@ ) from messagefoundry.controlchars import strip_control_chars from messagefoundry.transports.base import ( + ECH_UNSUPPORTED_DESTINATION_MSG, DeliveryError, DeliveryResponse, DestinationConnector, @@ -70,6 +71,7 @@ "InsecureHopGuard", "ProxyConfig", "RestDestination", + "ech_readdressed_request", "capture_response_headers", "ech_sidecar_url_from_settings", "egress_route_from_settings", @@ -1120,6 +1122,37 @@ def ech_sidecar_url_from_settings(s: Mapping[str, Any]) -> str | None: return sidecar.rstrip("/") +def ech_readdressed_request( + sidecar: str, + url: str, + *, + data: bytes | None, + headers: Mapping[str, str], + method: str, +) -> urllib.request.Request: + """Re-address one request to the loopback ECH sidecar (ADR 0139): it goes to the sidecar over + cleartext http with the real upstream in the ``Host`` header, and the sidecar re-originates the + ``https`` + ECH connection to that host (verifying its cert), so the SNI never leaves this host in + cleartext. Destination TLS is delegated to the sidecar; the engine->sidecar hop is same-host + loopback (ADR 0092 posture). + + Shared, because a connection has more than one outbound hop. The delivery hop + (:meth:`RestDestination._ech_request`) and the **token-endpoint hop** (``http_auth`` / ``smart``) + must both take it — ADR 0126 already rules that the token POST follows the connection's egress + route, and before #1176 the ECH path was the one route that did not, so an ``ech_egress`` + connection with OAuth2 or SMART auth still put the authorization server's hostname in a cleartext + outer ClientHello.""" + parsed = urllib.parse.urlsplit(url) + path = parsed.path or "/" + if parsed.query: + path += "?" + parsed.query + h = dict(headers) + h["Host"] = parsed.netloc # tell the sidecar the real upstream (host[:port]) + return urllib.request.Request( # noqa: S310 # nosec B310 — http to a validated loopback sidecar + sidecar + path, data=data, headers=h, method=method + ) + + def egress_route_from_settings( s: Mapping[str, Any], *, @@ -1134,13 +1167,16 @@ def egress_route_from_settings( implemented only on the REST destination in this build, so any *other* connector that reaches here with ``ech_egress`` set would silently NOT hide the SNI — refuse loudly rather than leak it. (The REST destination resolves ECH itself via :func:`ech_sidecar_url_from_settings` and never calls this - for the ECH case.)""" + for the ECH case.) + + Since #1176 the same refusal is also hoisted into + :func:`~messagefoundry.transports.base.build_destination`, which covers **every** connector rather + than only the ones routing through here. This one stays for the callers that are **not** destinations + and so never reach that seam — ``FhirLookupExecutor`` (ADR 0043, ``fhir.py``) is the live case. Both + raise the **same** message, so a doubly-covered connector never shows an operator two different + explanations for one key.""" if s.get("ech_egress"): - raise ValueError( - "ech_egress (ASVS 12.1.5 SNI hiding) is supported only on the REST destination in this " - "build; on this connector it would NOT hide the SNI, so it is refused rather than silently " - "leaking it (ADR 0139)" - ) + raise ValueError(ECH_UNSUPPORTED_DESTINATION_MSG) return proxy_config_from_settings( s, dest_scheme=dest_scheme, @@ -1251,8 +1287,14 @@ def __init__(self, config: Destination) -> None: # in _post; the two modes are mutually exclusive (a loud HttpAuthError otherwise). from messagefoundry.transports.http_auth import bearer_provider_from_settings - # ADR 0126: the token-endpoint call must ALSO traverse the proxy — thread the same ProxyConfig in. - self._token_provider = bearer_provider_from_settings(s, proxy=self._proxy) + # ADR 0126: the token-endpoint call must ALSO traverse the connection's egress route — thread it + # in. The two routes are mutually exclusive (refused above), so exactly one of these is set: + # a forward proxy carried as opener handlers, or the ECH sidecar the request is re-addressed to. + # #1176: before this, the ECH case passed `proxy=None` and nothing else, so the token hop went + # DIRECT and leaked the authorization server's SNI while the payload hop was routed. + self._token_provider = bearer_provider_from_settings( + s, proxy=self._proxy, ech_sidecar=self._ech_sidecar + ) if self._token_provider is not None: # The SMART bearer is injected per-request in _post, so the static-header cleartext check # above can't see it. Re-run the check treating the connection as credential-bearing, so a @@ -1335,20 +1377,10 @@ def __init__(self, config: Destination) -> None: def _ech_request( self, data: bytes | None, headers: dict[str, str], method: str ) -> urllib.request.Request: - """Re-address the request to the loopback ECH sidecar (ADR 0139): the request goes to the sidecar - over cleartext http with the real destination in the ``Host`` header; the sidecar re-originates - the ``https`` + ECH connection to that host (verifying its cert), so the SNI never leaves this - host in cleartext. Destination TLS is delegated to the sidecar; the engine->sidecar hop is - same-host loopback (ADR 0092 posture).""" + """Re-address the delivery request to this connection's loopback ECH sidecar (ADR 0139).""" assert self._ech_sidecar is not None # only called on the ECH path (guarded by the caller) - parsed = urllib.parse.urlsplit(self.url) - path = parsed.path or "/" - if parsed.query: - path += "?" + parsed.query - h = dict(headers) - h["Host"] = parsed.netloc # tell the sidecar the real upstream (host[:port]) - return urllib.request.Request( # noqa: S310 # nosec B310 — http to a validated loopback sidecar - self._ech_sidecar + path, data=data, headers=h, method=method + return ech_readdressed_request( + self._ech_sidecar, self.url, data=data, headers=headers, method=method ) @staticmethod diff --git a/messagefoundry/transports/smart.py b/messagefoundry/transports/smart.py index 0ce3525d..95ebacb4 100644 --- a/messagefoundry/transports/smart.py +++ b/messagefoundry/transports/smart.py @@ -56,6 +56,7 @@ _no_redirect_opener, _redact_url, cleartext_acceptance_from_settings, + ech_readdressed_request, enforce_outbound_length_limits, refuse_cleartext_credential_hop, ) @@ -117,6 +118,13 @@ def __init__( cleartext_reason: str | None = None, connection: str | None = None, proxy: ProxyConfig | None = None, + # #1176 (ADR 0139): this connection's loopback ECH sidecar, when it has one. The token-endpoint + # POST follows the connection's egress route exactly as ADR 0126 rules it must for a forward + # proxy; for ECH that means the request is RE-ADDRESSED to the sidecar with the real + # authorization-server host in ``Host``, so the AS hostname is never in a cleartext outer + # ClientHello. Mutually exclusive with ``proxy`` (refused at connector construction). None + # (default) -> byte-identical. + ech_sidecar: str | None = None, ) -> None: if not token_url: raise SmartAuthError("SMART Backend Services requires a 'smart_token_url' setting") @@ -180,10 +188,25 @@ def __init__( self._proxy_auth: dict[str, str] = ( token_proxy.auth_headers() if token_proxy is not None else {} ) + self._ech_sidecar = ech_sidecar self._lock = threading.Lock() self._cached_token: str | None = None self._cached_expiry_monotonic = 0.0 + def _token_request(self, data: bytes, headers: dict[str, str]) -> urllib.request.Request: + """The token-endpoint POST, on this connection's egress route. With an ECH sidecar the request + is re-addressed to it (#1176); without one it goes straight to the pinned ``token_url``, + byte-identical. The cleartext-credential refusal above keys on the DECLARED ``token_url`` + scheme, which is what the sidecar re-originates — the engine->sidecar leg is same-host loopback + (ADR 0092), exactly as the delivery hop's is.""" + if self._ech_sidecar is not None: + return ech_readdressed_request( + self._ech_sidecar, self.token_url, data=data, headers=headers, method="POST" + ) + return urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) above + self.token_url, data=data, headers=headers, method="POST" + ) + def access_token(self) -> str: """A valid bearer token — cached until it nears expiry, otherwise freshly acquired. Blocking (a token ``POST``); the connector calls it inside its off-loop ``send()`` worker. Raises @@ -233,15 +256,13 @@ def _fetch_token(self) -> tuple[str, float]: # here -- both are config, so a blob-valued env() surfaces as a config error rather than an # opaque IdP failure on the first mint. enforce_outbound_length_limits(self.token_url, dict(self._proxy_auth)) - req = urllib.request.Request( # noqa: S310 # nosec B310 — scheme constrained to http(s) in __init__ - self.token_url, - data=data, - headers={ + req = self._token_request( + data, + { "Content-Type": "application/x-www-form-urlencoded", "Accept": "application/json", **self._proxy_auth, # ADR 0126: pre-emptive Proxy-Authorization when behind an auth proxy }, - method="POST", ) try: with self._opener.open(req, timeout=self.timeout_seconds) as resp: @@ -279,7 +300,7 @@ def _parse_token_response(self, body: str) -> tuple[str, float]: def token_provider_from_settings( - s: Mapping[str, Any], *, proxy: ProxyConfig | None = None + s: Mapping[str, Any], *, proxy: ProxyConfig | None = None, ech_sidecar: str | None = None ) -> SmartBackendTokenProvider | None: """The :class:`SmartBackendTokenProvider` for an already-``env()``-resolved settings mapping, or ``None`` when SMART auth is off. @@ -288,7 +309,9 @@ def token_provider_from_settings( ``False``), so any connection that didn't compose ``with_smart_backend`` is byte-identical. Shared by the FHIR/REST outbound (:func:`token_provider_from_destination`) and the ``FhirLookup`` read executor (ADR 0043) — both inject the minted bearer per request off-loop past the queue boundary. ``proxy`` - (ADR 0126) routes the token-endpoint POST through the connection's forward proxy.""" + (ADR 0126) routes the token-endpoint POST through the connection's forward proxy; ``ech_sidecar`` + (#1176, ADR 0139) re-addresses it to the connection's loopback ECH sidecar instead. The two are + mutually exclusive by construction.""" if not s.get("smart_token_url"): return None if not s.get("smart_enabled", True): @@ -317,6 +340,7 @@ def token_provider_from_settings( cleartext_reason=accepted[1], connection=accepted[2], proxy=proxy, # ADR 0126: forward-proxy the token-endpoint POST + ech_sidecar=ech_sidecar, # #1176: ...or re-address it to the ECH sidecar (ADR 0139) ) diff --git a/samples/ech-sidecar/README.md b/samples/ech-sidecar/README.md index e1c43caa..7be631de 100644 --- a/samples/ech-sidecar/README.md +++ b/samples/ech-sidecar/README.md @@ -110,9 +110,20 @@ validation, cannot be authored as data, and are invisible to the connection edit factory parameter (and therefore a `connections.toml` form) is tracked as **BACKLOG #NNN**. `ech_sidecar` must be a loopback address and is **mutually exclusive** with the `proxy_url` settings key -(the sidecar *is* that connection's egress proxy) — refused at construction, -[`transports/rest.py:1192-1196`](../../messagefoundry/transports/rest.py). It composes with the -connection's TLS verify/allowlist/signing posture. +(the sidecar *is* that connection's egress proxy) — refused in `RestDestination.__init__` +([`transports/rest.py`](../../messagefoundry/transports/rest.py); named by symbol rather than by line, +because a line number goes stale silently and a wrong one reads as a working reference forever). It +composes with the connection's TLS verify/allowlist/signing posture. + +**Both of the connection's outbound hops take the sidecar.** The delivery request and — when the +connection uses OAuth2 client-credentials or SMART Backend Services — the **token-endpoint POST** are +each re-addressed to it, so the authorization server's hostname is not left in a cleartext outer +ClientHello while the payload hop is routed (BACKLOG #1176). + +**Every other connector REFUSES the key rather than ignoring it.** `ech_egress` is honoured only by the +REST destination; on any other outbound, and on any inbound, construction fails loudly +(`transports/base.py`, `build_destination` / `build_source`). Read that refusal as what it is: it +conceals no SNI, it just stops a silent no-op from looking like a working control. ## Verify before trusting a partner diff --git a/tests/test_ech_egress.py b/tests/test_ech_egress.py index 802188af..e97b8ea7 100644 --- a/tests/test_ech_egress.py +++ b/tests/test_ech_egress.py @@ -19,14 +19,35 @@ from __future__ import annotations +import io +import json import threading +import urllib.request +from collections.abc import Callable from http.server import BaseHTTPRequestHandler, HTTPServer import pytest - -from messagefoundry.config.models import ConnectorType, Destination -from messagefoundry.config.wiring import Rest -from messagefoundry.transports import build_destination +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +from messagefoundry.config.models import ConnectorType, Destination, Source +from messagefoundry.config.wiring import ( + DICOM, + MLLP, + X12, + ConnectionSpec, + Email, + File, + Rest, + Soap, + Tcp, +) +from messagefoundry.transports import build_destination, build_source +from messagefoundry.transports.base import ( + ECH_UNSUPPORTED_DESTINATION_MSG, + ECH_UNSUPPORTED_SOURCE_MSG, + DestinationConnector, +) from messagefoundry.transports.rest import ( ProxyConfig, RestDestination, @@ -35,6 +56,20 @@ ) +@pytest.fixture(scope="module") +def rsa_pem() -> str: + """A synthetic signing key for the SMART token-provider tests (generated per run, never a real one).""" + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ) + .decode("ascii") + ) + + def _rest(url: str = "https://partner.example/ingest", **extra: object) -> RestDestination: settings = Rest(url=url).settings settings.update(extra) @@ -143,10 +178,11 @@ def test_ech_and_proxy_mutually_exclusive_at_construction() -> None: # --- behavioral: _post actually routes through the (stub) sidecar --------------------------------- -def _make_stub_sidecar() -> tuple[HTTPServer, list[dict[str, str]]]: +def _make_stub_sidecar(body: str = "ok") -> tuple[HTTPServer, list[dict[str, str]]]: """A loopback stub standing in for the ECH sidecar: records the origin-form path + the Host header - (the real upstream the engine handed it) and returns 200 'ok'.""" + (the real upstream the engine handed it) and returns 200 with ``body``.""" seen: list[dict[str, str]] = [] + payload = body.encode() class _Handler(BaseHTTPRequestHandler): def _record_and_reply(self) -> None: @@ -155,9 +191,9 @@ def _record_and_reply(self) -> None: self.rfile.read(length) seen.append({"path": self.path, "host": self.headers.get("Host", "")}) self.send_response(200) - self.send_header("Content-Length", "2") + self.send_header("Content-Length", str(len(payload))) self.end_headers() - self.wfile.write(b"ok") + self.wfile.write(payload) def do_POST(self) -> None: # noqa: N802 self._record_and_reply() @@ -187,3 +223,192 @@ def test_ech_post_routes_through_the_sidecar() -> None: assert seen[0]["host"] == "partner.example" finally: srv.shutdown() + + +# --- BACKLOG #1176: no connector may SILENTLY ACCEPT ech_egress ------------------------------------ +# +# The routing half is implemented on REST only. Before #1176 every other connector BUILT with +# `ech_egress = True` and ignored it, so on a first deployment an operator could believe the SNI was +# hidden while the hop stayed an ordinary SNI-leaking handshake. The refusal now lives in the shared +# construction seam (`transports/base.py`), not copied per connector. + + +_SIDECAR = "http://127.0.0.1:8123" + +_NON_ECH_OUTBOUND: list[tuple[str, Callable[[], ConnectionSpec]]] = [ + ("mllp", lambda: MLLP(host="partner.example", port=2575)), + ("tcp", lambda: Tcp(host="partner.example", port=9100, framing="mllp")), + ("x12", lambda: X12(host="partner.example", port=9200)), + ("file", lambda: File(directory="./out")), + ( + "email", + lambda: Email( + host="smtp.example", + port=587, + sender="engine@example.org", + recipients=["ops@example.org"], + use_tls=True, + ), + ), + ( + "dicom", + lambda: DICOM(ae_title="MEFOR", host="partner.example", port=104, called_ae_title="PEER"), + ), +] +_NON_ECH_IDS = [label for label, _ in _NON_ECH_OUTBOUND] + + +def _dest(spec: ConnectionSpec, label: str, **extra: object) -> DestinationConnector: + settings = dict(spec.settings) + settings.update(extra) + return build_destination( + Destination(name=f"OB_{label.upper()}", type=spec.type, settings=settings) + ) + + +@pytest.mark.parametrize(("label", "make"), _NON_ECH_OUTBOUND, ids=_NON_ECH_IDS) +def test_outbound_builds_without_any_ech_key( + label: str, make: Callable[[], ConnectionSpec] +) -> None: + """Negative control for the refusal below: each spec is COMPLETE on its own, so the paired + refusal is attributable to the ech key and to nothing else about the settings.""" + assert _dest(make(), label) is not None + + +@pytest.mark.parametrize(("label", "make"), _NON_ECH_OUTBOUND, ids=_NON_ECH_IDS) +def test_outbound_that_cannot_hide_the_sni_refuses_ech_egress( + label: str, make: Callable[[], ConnectionSpec] +) -> None: + with pytest.raises(ValueError, match="only on the REST destination"): + _dest(make(), label, ech_egress=True, ech_sidecar=_SIDECAR) + + +def test_rest_is_the_one_exempt_outbound() -> None: + """Positive control for the parametrized refusal: the connector that DOES implement the routing + half still builds, so the shared refusal is keyed on the connector and not on the key alone.""" + d = _rest(ech_egress=True, ech_sidecar=_SIDECAR) + assert d._ech_sidecar == _SIDECAR + + +def test_both_refusal_sites_carry_the_same_message() -> None: + """A connector can reach BOTH refusals (SOAP/DICOMweb/FHIR route through the resolver and are also + built through the shared seam). They must not offer an operator two different explanations for one + key, so both raise the one constant.""" + spec = Soap(url="https://partner.example/svc", soap_action="urn:x") + with pytest.raises(ValueError) as from_seam: + _dest(spec, "soap", ech_egress=True, ech_sidecar=_SIDECAR) + with pytest.raises(ValueError) as from_resolver: + egress_route_from_settings( + {"ech_egress": True, "ech_sidecar": _SIDECAR}, dest_scheme="https" + ) + assert str(from_seam.value) == ECH_UNSUPPORTED_DESTINATION_MSG + assert str(from_resolver.value) == ECH_UNSUPPORTED_DESTINATION_MSG + + +def test_inbound_builds_without_any_ech_key() -> None: + """Negative control for the inbound refusal below.""" + spec = MLLP(port=2575) + src = build_source(Source(name="IB_MLLP", type=spec.type, settings=dict(spec.settings))) + assert src is not None + + +def test_inbound_refuses_ech_egress() -> None: + spec = MLLP(port=2575) + settings = dict(spec.settings) + settings.update(ech_egress=True, ech_sidecar=_SIDECAR) + with pytest.raises(ValueError) as exc: + build_source(Source(name="IB_MLLP", type=spec.type, settings=settings)) + assert str(exc.value) == ECH_UNSUPPORTED_SOURCE_MSG + + +# --- BACKLOG #1176: the token-endpoint hop must follow the same egress route as the payload hop ---- +# +# ADR 0126 already rules that the token-endpoint POST traverses the connection's forward proxy. The +# ECH path did not follow that precedent: `ech_egress` forced `_proxy = None` and the bearer provider +# fell back to a direct opener, so an `ech_egress` REST connection with OAuth2 or SMART auth still put +# the AUTHORIZATION SERVER's hostname in a cleartext outer ClientHello. + + +class _RecordingOpener: + """Stands in for the provider's opener and records the Request it was handed. No network in + either direction, so this measures where the request was ADDRESSED, not whether a host answers.""" + + def __init__(self) -> None: + self.req: urllib.request.Request | None = None + + def open(self, req: urllib.request.Request, timeout: float | None = None) -> io.BytesIO: + self.req = req + return io.BytesIO(json.dumps({"access_token": "t0k", "expires_in": 3600}).encode()) + + +_TOKEN_URL = "https://auth.partner.example/token" + + +def _oauth2_rest(**extra: object) -> RestDestination: + return _rest( + oauth2_token_url=_TOKEN_URL, + oauth2_client_id="cid", + oauth2_client_secret="s3cret", + **extra, + ) + + +def _minted_request(d: RestDestination) -> urllib.request.Request: + provider = d._token_provider + assert provider is not None + rec = _RecordingOpener() + provider._opener = rec # type: ignore[attr-defined,assignment] + assert provider.access_token() == "t0k" + assert rec.req is not None + return rec.req + + +def test_oauth2_token_request_goes_direct_without_ech() -> None: + """Control for the two tests below: with no egress route the token POST is addressed straight at + the authorization server and carries no Host override. This is what the ech case must NOT look + like.""" + req = _minted_request(_oauth2_rest()) + assert req.full_url == _TOKEN_URL + assert req.get_header("Host") is None + + +def test_oauth2_token_request_routes_through_the_ech_sidecar() -> None: + req = _minted_request(_oauth2_rest(ech_egress=True, ech_sidecar=_SIDECAR)) + assert req.full_url == f"{_SIDECAR}/token" + assert req.get_header("Host") == "auth.partner.example" + + +def test_smart_token_request_routes_through_the_ech_sidecar(rsa_pem: str) -> None: + d = _rest( + ech_egress=True, + ech_sidecar=_SIDECAR, + smart_token_url=_TOKEN_URL, + smart_client_id="cid", + smart_private_key=rsa_pem, + ) + req = _minted_request(d) + assert req.full_url == f"{_SIDECAR}/token" + assert req.get_header("Host") == "auth.partner.example" + + +def test_smart_token_request_goes_direct_without_ech(rsa_pem: str) -> None: + """Control for the SMART case, mirroring the OAuth2 one.""" + d = _rest(smart_token_url=_TOKEN_URL, smart_client_id="cid", smart_private_key=rsa_pem) + req = _minted_request(d) + assert req.full_url == _TOKEN_URL + assert req.get_header("Host") is None + + +def test_ech_token_mint_lands_on_the_stub_sidecar() -> None: + """End-to-end through the real opener: the token POST arrives at the loopback sidecar naming the + authorization server in Host, so no direct TLS connection to that host is ever opened.""" + srv, seen = _make_stub_sidecar(body=json.dumps({"access_token": "t0k", "expires_in": 3600})) + try: + port = srv.server_address[1] + d = _oauth2_rest(ech_egress=True, ech_sidecar=f"http://127.0.0.1:{port}") + assert d._token_provider is not None + assert d._token_provider.access_token() == "t0k" + assert seen and seen[0]["path"] == "/token" + assert seen[0]["host"] == "auth.partner.example" + finally: + srv.shutdown() From 142b926299e2dc302afef0b73a0605396e5ded72 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 20:25:37 -0500 Subject: [PATCH 07/31] feat(uploads): make the upload quota cross-process via a store reservation (BACKLOG #1112) The in-process half already shipped: uploads.py holds an asyncio.Lock spanning the quota check and the write. That lock is per-EVENT-LOOP, so N engine shards over one uploads_dir hold N of them and each can pass the same check. Every shard shares ONE unified store (ADR 0063, and sharding.py:81 makes a server DB mandatory past one shard), so the store is the only decision point that spans them. ONE new protocol method, `reserve_upload_quota`, across the four store files, plus the two construction sites that give UploadStore a store handle. The attachment substrate is NOT touched -- separate subject, out of scope by the dispatcher's fence, and that fence held: no fifth store file, no second method. RED PROOF, and the first run is DISCARDED rather than counted. Run 1 failed with `TypeError: unexpected keyword argument 'store'` -- the code path was never reached, so it is plumbing, scored NOT REACHED. Run 2, with the kwarg removed from the helper so the identical body runs against shipped code, is the proof: both shards WON a budget of one and both files landed on disk. `assert 2 == 1`, failing at the right phase for the right reason. A refusal for a different reason is indistinguishable from the refusal you asked for, which is why run 1 is in the record as not-proof rather than omitted. MYPY IS THE CONTROL THAT THE PROTOCOL IS REAL: renaming the SQL Server implementation produces `"SqlServerStore" is missing following "Store" protocol member: reserve_upload_quota`. So the addition is enforced across all four files rather than silently unimplemented in three. ADVERSARIAL PASS: test_can_fail true; four independent plants, each redding a distinct test; 30/30 acceptance runs, no flakes. It confirmed the test demonstrates CONCURRENT EXCLUSION rather than the shared visibility the pre-existing sequential test already showed. ITS FINDINGS, RECORDED BECAUSE THEY ARE NOT FIXED HERE: - THE IMPLEMENTER'S OWN STATED MITIGATION IS FALSE. "The CI Postgres and SQL Server legs are the gate" -- zero tests call reserve_upload_quota on either backend; all four callers are SQLite. The two server implementations ship UNEXERCISED. - A leaked reservation does NOT unconditionally self-heal: release writes `since = ` in all three backends, restarting the staleness clock. Two shipped docstrings state the opposite. That is a false premise in a comment and it is the next thing I fix. - It introduces a new failure mode on SINGLE-PROCESS deployments, not only sharded ones: before this, a killed process left no quota state. config/settings.py carries a COMMENT-ONLY hunk (0 non-comment changed lines, asserted mechanically) correcting a comment that #1112 makes false. That file is another lane's; the dispatcher authorised this hunk specifically because the comment is true only at the moment this lands, and this commit is that moment. 89 passed across the upload, upload-API and PHI-inventory modules. --- docs/PHI.md | 2 +- messagefoundry/api/app.py | 9 + messagefoundry/config/settings.py | 8 +- messagefoundry/store/base.py | 41 ++++ messagefoundry/store/postgres.py | 72 ++++++ messagefoundry/store/sqlserver.py | 97 +++++++- messagefoundry/store/store.py | 98 ++++++++ messagefoundry/uploads.py | 206 ++++++++++++++--- tests/test_uploads_cross_process_quota.py | 258 ++++++++++++++++++++++ 9 files changed, 759 insertions(+), 32 deletions(-) create mode 100644 tests/test_uploads_cross_process_quota.py diff --git a/docs/PHI.md b/docs/PHI.md index 42166451..417a3d16 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -95,7 +95,7 @@ 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` `` | -| `[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). The quota is scoped to the **`uploads_dir`, not to the process** — the sidecar scan is uncached, so engine shards sharing one dir enforce **one** budget between them (measured 2026-08-10); shards given separate dirs get separate budgets by construction. The check and the write it authorizes are one critical section per process; the residual that survives it, and its bound, are stated once in `uploads.UploadQuotaError`. Harden the dir + volume encryption ([§10](#10-secure-deployment--operations-checklist)) | `` `[store].uploads_retention_days` `` | +| `[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). The quota is scoped to the **`uploads_dir`, not to the process** — the sidecar scan is uncached, so engine shards sharing one dir enforce **one** budget between them (measured 2026-08-10); shards given separate dirs get separate budgets by construction. The check and the write it authorizes are one critical section per process, and a shard mid-upload is held as an atomic reservation on the unified store every shard shares (`Store.reserve_upload_quota`), so the decision is exclusive across processes too (ASVS 2.3.4). The residuals that survive it, and their bounds, are stated once in `uploads.UploadQuotaError`. 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` | diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index 4a892c45..9d113ff1 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -1113,6 +1113,12 @@ def create_app( max_files_per_user=store_settings.max_upload_files_per_user, max_total_bytes_per_user=store_settings.max_upload_total_bytes_per_user, retention_days=store_settings.uploads_retention_days, + # ASVS 2.3.4: the quota's cross-PROCESS half. The per-uploader lock inside UploadStore is + # an asyncio.Lock and so is per-event-loop; N engine shards over one uploads_dir hold N + # of them. Every shard shares this ONE unified store (ADR 0063), so it is the decision + # point that spans them. None here is the genuinely store-LESS path (embedding / tests) — + # the same path that falls back to build_store_cipher above. + store=engine.store if engine is not None else None, ) # ADR 0118: the EFFECTIVE [security] switch values (serve syncs the gate-flipped egress/retention back # in) back the read-only GET /security/posture view. None → the secure defaults for the test/embedding @@ -5654,6 +5660,9 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: max_files_per_user=resolved.max_upload_files_per_user, max_total_bytes_per_user=resolved.max_upload_total_bytes_per_user, retention_days=resolved.uploads_retention_days, + # ASVS 2.3.4: bind the cross-shard quota ledger to the SAME store this lifespan just + # opened — this is the serve path, so it is the one that actually runs sharded. + store=store, ) # Operational alert notifier (webhook/email). None when no transport is configured → the # engine falls back to the logging sink. Its background dispatch task is owned by this diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 0bd53203..2a618021 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -461,9 +461,11 @@ class StoreSettings(_Section): # task, each prune audited `upload.prune`). Quotas are enforced per-`uploads_dir`, NOT per-process: # the check reads the sidecars off disk with no cache, so engine shards sharing one dir see each # other's files and the budget does NOT multiply (measured 2026-08-10 — two UploadStores over one - # dir, the second refused the same uploader at quota, against a live positive control). What IS - # shared across them is the check-then-write race below, which overshoots by at most one file per - # concurrently in-flight upload. Shards given SEPARATE dirs get separate budgets, by construction. + # dir, the second refused the same uploader at quota, against a live positive control). The + # check-then-write race that used to survive across them — an overshoot of one file per shard + # caught between its scan and its write — is closed by an atomic reservation on the unified store + # every shard shares (ASVS 2.3.4, BACKLOG #1112); the surviving residuals are stated once in + # `uploads.UploadQuotaError`. Shards given SEPARATE dirs get separate budgets, by construction. max_upload_files_per_user: int = Field( default=100, ge=1, diff --git a/messagefoundry/store/base.py b/messagefoundry/store/base.py index 799d4563..0164b029 100644 --- a/messagefoundry/store/base.py +++ b/messagefoundry/store/base.py @@ -46,6 +46,7 @@ from messagefoundry.store.keyprovider import resolve_key_provider from messagefoundry.store.pool_metrics import PoolStatus from messagefoundry.store.store import ( + UPLOAD_RESERVATION_STALE_AFTER, AlertInstance, CapturedResponse, ClaimedHeads, @@ -112,6 +113,7 @@ "pool_over_provisioned_warning", "POOL_SIZE_OPTIMUM", "POOL_SIZE_CLIFF", + "UPLOAD_RESERVATION_STALE_AFTER", ] @@ -1234,6 +1236,45 @@ async def checkpoint_cipher_invocations(self, *, settle: bool = False) -> int | :func:`messagefoundry.store.gcm_bound.checkpoint_invocations`.""" ... + # --- cross-process upload-quota reservation (ASVS 2.3.4) ----------------- + async def reserve_upload_quota( + self, + uploader_id: str, + *, + files: int, + size_bytes: int, + max_files: int = 0, + max_total_bytes: int = 0, + stale_after: float = UPLOAD_RESERVATION_STALE_AFTER, + ) -> bool: + """Atomically reserve (or release) an uploader's IN-FLIGHT upload budget; return whether the + reserve applied. The one cross-process decision point behind the per-uploader upload quota. + + **Why the store owns this.** ``UploadStore._quota_lock`` is an ``asyncio.Lock``, so it is + per-event-loop and therefore per-process. Engine sharding is the built, shipped, default + scaling axis and nothing partitions ``uploads_dir`` per shard, so N shards over one directory + hold N independent locks and each can overshoot the budget by one file. Every shard sits on + the ONE unified store (ADR 0063 — and ``require_unified_store`` makes a server DB mandatory + past one shard), so this row is authoritative for all of them. + + **What is counted here, and what is not.** The files already ON DISK are counted by the + caller's sidecar scan, which is uncached and therefore already fleet-visible. The gap this + closes is only the uploads IN FLIGHT on other shards — reserved but not yet landed, so + invisible to any scan. The caller passes its remaining HEADROOM (cap minus what its scan + observed) as ``max_files`` / ``max_total_bytes``; this method holds only the in-flight sum. + + ``files > 0`` reserves: the post-add in-flight totals must fit inside the headroom or nothing + is written and this returns ``False`` (fail-closed — a caller that forgets the headroom + arguments gets the 0 defaults and is refused). ``files <= 0`` releases, always applies, + clamps at zero, ignores the headroom arguments and returns ``True``. + + ``stale_after`` bounds a leak: a process killed between reserve and release never releases, + and its slot would otherwise consume the uploader's budget forever. A row whose reservation + has been CONTINUOUSLY outstanding for longer than ``stale_after`` seconds is reset to zero + before the add. The reset can only restore today's behaviour (an overshoot bounded by the + number of concurrent writers), never something worse.""" + ... + # --- retention / purge + maintenance (PHI.md §8) ------------------------- async def purge_message_bodies( self, diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index a6fccca5..b4dca110 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -80,6 +80,7 @@ from messagefoundry.redaction import safe_text from messagefoundry.store.audit_tee import emit_audit_tee from messagefoundry.store.base import ( + UPLOAD_RESERVATION_STALE_AFTER, Row, acquire_pooled, warm_pool_connections, @@ -495,6 +496,16 @@ def __init__(self, method: str, outbox_ids: tuple[str, ...]) -> None: invocations BIGINT NOT NULL DEFAULT 0, updated_at DOUBLE PRECISION NOT NULL )""", + # Cross-process upload-quota reservation (ASVS 2.3.4, BACKLOG #1112) — see the SQLite `_SCHEMA` + # for the in-flight-only rationale. This is the backend a real sharded deployment runs: + # `require_unified_store` makes a server DB mandatory past one shard. No PHI (an account id and + # two counters). + """CREATE TABLE IF NOT EXISTS upload_quota ( + uploader_id TEXT PRIMARY KEY, + inflight_files BIGINT NOT NULL DEFAULT 0, + inflight_bytes BIGINT NOT NULL DEFAULT 0, + since DOUBLE PRECISION NOT NULL + )""", """CREATE TABLE IF NOT EXISTS pending_approvals ( id TEXT PRIMARY KEY, operation TEXT NOT NULL, @@ -6057,6 +6068,67 @@ async def add_cipher_invocations(self, key_id: str, count: int) -> int: ) return int(row["invocations"]) if row is not None else int(count) + async def reserve_upload_quota( + self, + uploader_id: str, + *, + files: int, + size_bytes: int, + max_files: int = 0, + max_total_bytes: int = 0, + stale_after: float = UPLOAD_RESERVATION_STALE_AFTER, + ) -> bool: + """Atomically reserve (or release) an uploader's in-flight upload budget — see + :meth:`messagefoundry.store.base.Store.reserve_upload_quota` for the contract, and the SQLite + twin for the statement shape. + + One statement carries the budget predicate into the ``DO UPDATE``, so no other connection can + land between the read and the write. ``RETURNING`` (absent on a refusal) discriminates applied + from refused.""" + now = time.time() + if files <= 0: + # RELEASE — unconditional, clamped at zero (a double release cannot mint budget). + await self._execute( + "UPDATE upload_quota SET" + " inflight_files = GREATEST(0, inflight_files + $2)," + " inflight_bytes = GREATEST(0, inflight_bytes + $3)," + " since = $4" + " WHERE uploader_id = $1", + uploader_id, + int(files), + int(size_bytes), + now, + ) + return True + if files > max_files or size_bytes > max_total_bytes: + # Fail closed before touching the row: the insert branch applies unconditionally. + return False + stale = now - max(0.0, stale_after) + row = await self._fetchone( + "INSERT INTO upload_quota (uploader_id, inflight_files, inflight_bytes, since)" + " VALUES ($1,$2,$3,$4)" + " ON CONFLICT (uploader_id) DO UPDATE SET" + " inflight_files =" + " CASE WHEN upload_quota.since <= $5 THEN 0 ELSE upload_quota.inflight_files END + $2," + " inflight_bytes =" + " CASE WHEN upload_quota.since <= $5 THEN 0 ELSE upload_quota.inflight_bytes END + $3," + " since = CASE WHEN upload_quota.since <= $5 OR upload_quota.inflight_files <= 0" + " THEN $4 ELSE upload_quota.since END" + " WHERE (CASE WHEN upload_quota.since <= $5 THEN 0" + " ELSE upload_quota.inflight_files END) + $2 <= $6" + " AND (CASE WHEN upload_quota.since <= $5 THEN 0" + " ELSE upload_quota.inflight_bytes END) + $3 <= $7" + " RETURNING 1 AS applied", + uploader_id, + int(files), + int(size_bytes), + now, + stale, + int(max_files), + int(max_total_bytes), + ) + return row is not None + async def cipher_invocations(self, key_id: str) -> int: """``key_id``'s persisted cumulative invocation total (0 when the key has no row yet).""" row = await self._fetchone("SELECT invocations FROM cipher_meta WHERE key_id = $1", key_id) diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index a2ad3ee6..8e5a061c 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -65,7 +65,12 @@ from messagefoundry.parsing.binary import strip_documents as _strip_documents from messagefoundry.redaction import safe_text from messagefoundry.store.audit_tee import emit_audit_tee -from messagefoundry.store.base import acquire_pooled, warm_pool_connections, warm_pool_target +from messagefoundry.store.base import ( + UPLOAD_RESERVATION_STALE_AFTER, + acquire_pooled, + warm_pool_connections, + warm_pool_target, +) from messagefoundry.store.content_search import SearchSpec, row_matches from messagefoundry.store.crypto import MARKER_PREFIX as _ENC_MARKER_PREFIX from messagefoundry.store.crypto import ( @@ -1338,6 +1343,14 @@ def __init__(self, conn: Any, cur: Any) -> None: """IF OBJECT_ID('cipher_meta','U') IS NULL CREATE TABLE cipher_meta ( key_id NVARCHAR(64) COLLATE Latin1_General_100_BIN2 NOT NULL PRIMARY KEY, invocations BIGINT NOT NULL DEFAULT 0, updated_at FLOAT NOT NULL)""", + # Cross-process upload-quota reservation (ASVS 2.3.4, BACKLOG #1112) — see the SQLite `_SCHEMA` + # for the in-flight-only rationale. One of the two backends a real sharded deployment runs + # (`require_unified_store` makes a server DB mandatory past one shard). No PHI (an account id and + # two counters). BIN2 collation matches the other id-keyed tables. + """IF OBJECT_ID('upload_quota','U') IS NULL CREATE TABLE upload_quota ( + uploader_id NVARCHAR(64) COLLATE Latin1_General_100_BIN2 NOT NULL PRIMARY KEY, + inflight_files BIGINT NOT NULL DEFAULT 0, inflight_bytes BIGINT NOT NULL DEFAULT 0, + since FLOAT NOT NULL)""", """IF OBJECT_ID('pending_approvals','U') IS NULL CREATE TABLE pending_approvals ( id NVARCHAR(64) NOT NULL PRIMARY KEY, operation NVARCHAR(128) NOT NULL, params NVARCHAR(MAX) NOT NULL, requester NVARCHAR(256) NOT NULL, @@ -8849,6 +8862,88 @@ async def add_cipher_invocations(self, key_id: str, count: int) -> int: raise return total + async def reserve_upload_quota( + self, + uploader_id: str, + *, + files: int, + size_bytes: int, + max_files: int = 0, + max_total_bytes: int = 0, + stale_after: float = UPLOAD_RESERVATION_STALE_AFTER, + ) -> bool: + """Atomically reserve (or release) an uploader's in-flight upload budget — see + :meth:`messagefoundry.store.base.Store.reserve_upload_quota` for the contract, and the SQLite + twin for the statement shape. + + MERGE with HOLDLOCK is the SQL Server upsert that is safe under the concurrent opens of an + engine-shard fleet (a bare IF EXISTS/INSERT races) — the same pattern + :meth:`add_cipher_invocations` uses. The budget predicate rides ``WHEN MATCHED AND``, so a + refusal matches no rows and ``OUTPUT`` returns nothing.""" + now = time.time() + if files <= 0: + # RELEASE — unconditional, clamped at zero (a double release cannot mint budget). + await self._execute( + "UPDATE upload_quota SET" + " inflight_files = CASE WHEN inflight_files + ? < 0 THEN 0 ELSE inflight_files + ? END," + " inflight_bytes = CASE WHEN inflight_bytes + ? < 0 THEN 0 ELSE inflight_bytes + ? END," + " since = ?" + " WHERE uploader_id = ?", + ( + int(files), + int(files), + int(size_bytes), + int(size_bytes), + now, + uploader_id, + ), + ) + return True + if files > max_files or size_bytes > max_total_bytes: + # Fail closed before touching the row: WHEN NOT MATCHED inserts unconditionally. + return False + stale = now - max(0.0, stale_after) + async with self._acquire() as conn, self._cursor(conn) as cur: + try: + await cur.execute( + "MERGE upload_quota WITH (HOLDLOCK) AS t" + " USING (SELECT ? AS uploader_id, ? AS files, ? AS size_bytes, ? AS now_ts," + " ? AS stale_ts, ? AS max_files, ? AS max_total_bytes) AS s" + " ON t.uploader_id = s.uploader_id" + " WHEN MATCHED AND" + " (CASE WHEN t.since <= s.stale_ts THEN 0 ELSE t.inflight_files END)" + " + s.files <= s.max_files" + " AND (CASE WHEN t.since <= s.stale_ts THEN 0 ELSE t.inflight_bytes END)" + " + s.size_bytes <= s.max_total_bytes" + " THEN UPDATE SET" + " t.inflight_files =" + " (CASE WHEN t.since <= s.stale_ts THEN 0 ELSE t.inflight_files END) + s.files," + " t.inflight_bytes =" + " (CASE WHEN t.since <= s.stale_ts THEN 0 ELSE t.inflight_bytes END)" + " + s.size_bytes," + " t.since = CASE WHEN t.since <= s.stale_ts OR t.inflight_files <= 0" + " THEN s.now_ts ELSE t.since END" + " WHEN NOT MATCHED THEN" + " INSERT (uploader_id, inflight_files, inflight_bytes, since)" + " VALUES (s.uploader_id, s.files, s.size_bytes, s.now_ts)" + " OUTPUT INSERTED.inflight_files;", + ( + uploader_id, + int(files), + int(size_bytes), + now, + stale, + int(max_files), + int(max_total_bytes), + ), + ) + row = await cur.fetchone() + await self._commit(conn) + except Exception: + await conn.rollback() + raise + return row is not None + async def cipher_invocations(self, key_id: str) -> int: """``key_id``'s persisted cumulative invocation total (0 when the key has no row yet).""" row = await self._fetchone( diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index 6893f90a..a9e519c5 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -460,6 +460,15 @@ class ReingressOriginMissing(ResendError): #: when a duplicate/conflict is reported, while sharing the one ``resend_log`` UNIQUE gate. REINGRESS_TARGET_PREFIX = "@reingress:" +#: How long an upload-quota reservation may stay CONTINUOUSLY outstanding before the next reserve +#: reclaims it (seconds) — ASVS 2.3.4, BACKLOG #1112. One reservation covers a single +#: ``UploadStore.save``: a sidecar scan plus an encrypt-and-write bounded by +#: ``[store].max_upload_bytes`` (25 MiB default), so five minutes is orders of magnitude of slack. It +#: exists only so a process killed between reserve and release cannot consume an uploader's budget +#: forever. Defined here (not in ``base``) because ``base`` imports THIS module, never the reverse; +#: ``base`` re-exports it as the public name. See :meth:`Store.reserve_upload_quota`. +UPLOAD_RESERVATION_STALE_AFTER = 300.0 + @dataclass(frozen=True) class ReingressOutcome: @@ -1618,6 +1627,20 @@ def _append_channel_scope( updated_at REAL NOT NULL ); +-- Cross-process upload-quota reservation (ASVS 2.3.4, BACKLOG #1112). One row per uploader holding +-- only the IN-FLIGHT total: uploads reserved but not yet landed in `uploads_dir`, and therefore +-- invisible to the sidecar scan that counts everything already on disk. The scan is uncached and so +-- already fleet-visible; this row is what makes the DECISION exclusive across engine-shard processes, +-- which the per-event-loop `UploadStore._quota_lock` cannot be. `since` is when the current +-- continuously-non-zero streak began, so a reservation leaked by a killed process is reclaimed +-- rather than consuming the uploader's budget forever. No PHI: an account id and two counters. +CREATE TABLE IF NOT EXISTS upload_quota ( + uploader_id TEXT PRIMARY KEY, + inflight_files INTEGER NOT NULL DEFAULT 0, + inflight_bytes INTEGER NOT NULL DEFAULT 0, + since REAL NOT NULL +); + CREATE TABLE IF NOT EXISTS pending_approvals ( id TEXT PRIMARY KEY, operation TEXT NOT NULL, -- registered op key, e.g. 'dead_letter_replay' @@ -7601,6 +7624,81 @@ async def checkpoint_cipher_invocations(self, *, settle: bool = False) -> int | self._cipher, self.add_cipher_invocations, settle=settle ) + async def reserve_upload_quota( + self, + uploader_id: str, + *, + files: int, + size_bytes: int, + max_files: int = 0, + max_total_bytes: int = 0, + stale_after: float = UPLOAD_RESERVATION_STALE_AFTER, + ) -> bool: + """Atomically reserve (or release) an uploader's in-flight upload budget — see + :meth:`messagefoundry.store.base.Store.reserve_upload_quota` for the contract. + + The reserve is ONE statement: an upsert whose ``DO UPDATE`` carries the budget predicate, so + the read and the write cannot be separated by another connection. That is what makes it + exclusive across processes rather than across coroutines. ``rowcount`` discriminates applied + from refused; the row is left untouched on a refusal.""" + now = time.time() + if files <= 0: + # RELEASE — always applies, clamped at zero so a double release cannot mint budget. Never + # conditional: refusing a release would strand the reservation it is paying back. + async with self._lock: + await self._db.execute( + "UPDATE upload_quota SET" + " inflight_files = MAX(0, inflight_files + ?)," + " inflight_bytes = MAX(0, inflight_bytes + ?)," + " since = ?" + " WHERE uploader_id = ?", + (int(files), int(size_bytes), now, uploader_id), + ) + await self._commit() + return True + if files > max_files or size_bytes > max_total_bytes: + # Fail closed BEFORE touching the row: the insert branch below (no row yet) applies + # unconditionally, so an empty uploader with zero headroom must be refused here. + return False + stale = now - max(0.0, stale_after) + async with self._lock: + cur = await self._db.execute( + "INSERT INTO upload_quota (uploader_id, inflight_files, inflight_bytes, since)" + " VALUES (?,?,?,?)" + " ON CONFLICT(uploader_id) DO UPDATE SET" + " inflight_files =" + " CASE WHEN upload_quota.since <= ? THEN 0 ELSE upload_quota.inflight_files END + ?," + " inflight_bytes =" + " CASE WHEN upload_quota.since <= ? THEN 0 ELSE upload_quota.inflight_bytes END + ?," + " since = CASE WHEN upload_quota.since <= ? OR upload_quota.inflight_files <= 0" + " THEN ? ELSE upload_quota.since END" + " WHERE (CASE WHEN upload_quota.since <= ? THEN 0" + " ELSE upload_quota.inflight_files END) + ? <= ?" + " AND (CASE WHEN upload_quota.since <= ? THEN 0" + " ELSE upload_quota.inflight_bytes END) + ? <= ?", + ( + uploader_id, + int(files), + int(size_bytes), + now, + stale, + int(files), + stale, + int(size_bytes), + stale, + now, + stale, + int(files), + int(max_files), + stale, + int(size_bytes), + int(max_total_bytes), + ), + ) + applied = cur.rowcount == 1 + await self._commit() + return applied + async def audit_anchor(self) -> tuple[int, str]: """The audit log's external anchor — ``(row_count, head_hash)`` (head ``""`` when empty). diff --git a/messagefoundry/uploads.py b/messagefoundry/uploads.py index 6544a642..6aaca517 100644 --- a/messagefoundry/uploads.py +++ b/messagefoundry/uploads.py @@ -11,8 +11,11 @@ This is a **leaf** module: it imports only the pure ``store.crypto`` cipher seam + the pure ``parsing.split``/``parsing.peek`` HL7 library. It never imports the store instance, a transport, a -connection, ``api/``, or ``pipeline/`` — the offline viewer is not wired into the graph. All disk + -crypto + split work runs **off the event loop** (``asyncio.to_thread``). +connection, ``api/``, or ``pipeline/`` — the offline viewer is not wired into the graph. The +cross-process quota ledger (ASVS 2.3.4) does not change that: the store handle arrives as a +constructor argument typed against the narrow :class:`UploadQuotaLedger` protocol declared HERE, so +no store module is imported. All disk + crypto + split work runs **off the event loop** +(``asyncio.to_thread``). **PHI.** An uploaded file is real HL7 PHI at rest. Bodies are never logged at INFO+; every access is gated + audited by the API layer. The on-disk **identity** is a random 32-hex ``file_id`` — the @@ -35,6 +38,7 @@ from collections.abc import Awaitable, Callable from dataclasses import asdict, dataclass from pathlib import Path +from typing import Protocol from messagefoundry.parsing.peek import HL7PeekError, Peek from messagefoundry.parsing.sniff import _looks_like_hl7, _lstrip_bom_ws @@ -94,12 +98,26 @@ class UploadQuotaError(UploadError): with no cache, so engine shards sharing one dir enforce ONE budget between them (measured 2026-08-10). Shards pointed at separate dirs get separate budgets, by construction. - Residual, stated precisely: the critical section is per-process, so N engine shards sharing one - dir can still overshoot by at most **N-1 files** — one per shard that is mid-write when another - scans, each bounded by ``max_upload_bytes``. On the shipped single-process deployment N is 1 and - the overshoot is zero. Closing the multi-shard remainder needs a cross-process mechanism (an - advisory lock on the dir, or moving the accounting into the unified store); it is not closed - here, and no comment in this module should imply otherwise.""" + The cross-PROCESS half is the ledger reservation (BACKLOG #1112). The per-process lock is an + ``asyncio.Lock``, so N engine shards over one dir used to hold N of them and each could overshoot + by one file while another scanned. :meth:`UploadStore._reserve_across_shards` now takes an atomic + reservation on the ONE unified store every shard shares before the write and pays it back after, + so a shard mid-upload is visible to its siblings and the decision is exclusive across processes. + + Residual, stated precisely, and there are three: + + * **No ledger bound.** ``UploadStore(store=None)`` — the genuinely store-less construction path + (embedding / tests) — keeps only the per-process lock, so the pre-#1112 bound applies there: at + most **N-1 files** over, one per shard mid-write, each bounded by ``max_upload_bytes``. + * **A leaked reservation.** A process killed between reserve and release never pays back, and its + slot narrows that uploader's budget until the row goes idle for + ``UPLOAD_RESERVATION_STALE_AFTER``. It errs toward refusing, not allowing, and it self-heals. + * **A reclaimed live reservation.** If one uploader keeps reservations continuously outstanding + for longer than that window, the staleness reset zeroes a row that was legitimately non-zero, + which restores the N-1 bound above for that window. Never worse than the pre-#1112 behaviour. + + Still open, and out of scope here: the ledger is checked and paid back around the write, not in + the same transaction as it, because the body lives on the filesystem rather than in the store.""" class UploadNotFoundError(UploadError): @@ -275,12 +293,37 @@ def browse_messages( ) +class UploadQuotaLedger(Protocol): + """The ONE thing :class:`UploadStore` needs from the message store: an atomic, cross-process + reservation of an uploader's in-flight upload budget (ASVS 2.3.4). + + Declared here as a structural protocol rather than importing ``store.base.Store``, so this module + stays a leaf (see the module docstring). Every backend's ``Store`` satisfies it structurally — + see :meth:`messagefoundry.store.base.Store.reserve_upload_quota` for the full contract.""" + + async def reserve_upload_quota( + self, + uploader_id: str, + *, + files: int, + size_bytes: int, + max_files: int = 0, + max_total_bytes: int = 0, + ) -> bool: ... + + class UploadStore: """Filesystem-backed, encrypted-at-rest store for operator-uploaded diagnostic files (ADR 0134). Constructed with the store's :class:`~messagefoundry.store.crypto.Cipher` so uploaded bodies ride the same DEK/keyring/rotation posture as the message store. ``max_bytes`` bounds a single upload (and thus - the in-memory whole-file split at browse time).""" + the in-memory whole-file split at browse time). + + ``ledger`` is the message store, used ONLY for the cross-process half of the per-uploader quota + (ASVS 2.3.4). ``None`` — the genuinely store-less construction path (embedding / tests) — leaves + the quota enforced by the per-process lock alone, which is what shipped before and is a real + degradation, not a second control: N engine shards over one ``uploads_dir`` would then each be + able to overshoot the budget by one file.""" def __init__( self, @@ -291,6 +334,7 @@ def __init__( max_files_per_user: int = 100, max_total_bytes_per_user: int = 250 * 1024 * 1024, retention_days: int = 30, + store: UploadQuotaLedger | None = None, ) -> None: self._root = Path(root) self._cipher = cipher @@ -306,7 +350,13 @@ def __init__( # build-and-write (not just the check) is what makes it atomic — releasing between them is the # race. The throughput cost is acceptable here and nowhere near the data plane: this is the # operator diagnostic-upload surface, and each pass is bounded by max_bytes. + # + # This lock is an asyncio.Lock, so it is per-event-loop and therefore PER-PROCESS. Engine + # sharding is the built, shipped, default scaling axis and nothing partitions uploads_dir per + # shard, so N shards over one directory hold N independent copies of it. `_ledger` is the + # cross-process half: one atomic row on the ONE unified store every shard already shares. self._quota_lock = asyncio.Lock() + self._ledger = store @property def max_bytes(self) -> int: @@ -445,24 +495,19 @@ def _build_and_write() -> UploadedFileMeta: # writing when this file would exceed their file-count or aggregate-byte cap. Runs in the same # off-loop thread as the write, and the caller holds _quota_lock across BOTH, so no second # upload in this process can read this count before the write consumes it (ASVS 2.3.4). - # The scan is uncached, so shards sharing a dir enforce one budget rather than one each; - # the residual that survives the lock is per-shard, not per-upload. See UploadQuotaError. - # The bucket key is the IMMUTABLE uploader_id, the same value the ownership check uses, so - # the budget and the ownership rule can never disagree about who a file belongs to: a - # recycled username is never billed for files it cannot read. The message text below still - # names the human username, because an operator reading a 409 needs a name, not a uuid. + # The scan is uncached, so shards sharing a dir enforce one budget rather than one each. + # The residual the lock alone cannot cover — a sibling shard between ITS scan and ITS + # write, invisible to this one — is covered by the ledger reservation the caller holds + # around this whole call. See _reserve_across_shards and _on_disk_refusal. mine = [m for m in self._scan_metas_sync() if m.uploader_id == uploader_id] - if len(mine) + 1 > self._max_files_per_user: - raise UploadQuotaError( - f"uploader {uploader!r} has {len(mine)} uploaded files; the limit is " - f"{self._max_files_per_user}" - ) - projected = sum(m.size for m in mine) + len(data) - if projected > self._max_total_bytes_per_user: - raise UploadQuotaError( - f"uploader {uploader!r} would hold {projected} bytes; the limit is " - f"{self._max_total_bytes_per_user}" - ) + refusal = self._on_disk_refusal( + uploader=uploader, + observed_files=len(mine), + observed_bytes=sum(m.size for m in mine), + size=len(data), + ) + if refusal is not None: + raise refusal meta = UploadedFileMeta( file_id=file_id, filename=display, @@ -485,8 +530,115 @@ def _build_and_write() -> UploadedFileMeta: return meta # One critical section per process: quota check + write. See _quota_lock in __init__. + # Inside it, one cross-PROCESS reservation around the same window (ASVS 2.3.4): the sidecar + # scan below already sees every shard's files, so the only thing it CANNOT see is an upload + # in flight on another shard — reserved but not yet landed. The reservation is what the other + # shards see instead, and it is released the moment the file is on disk (or the write fails), + # so a completed upload is counted by the scan and by nothing else. async with self._quota_lock: - return await asyncio.to_thread(_build_and_write) + reserved = await self._reserve_across_shards( + uploader_id=uploader_id, uploader=uploader, size=len(data) + ) + try: + return await asyncio.to_thread(_build_and_write) + finally: + if reserved: + await self._release_across_shards(uploader_id=uploader_id, size=len(data)) + + async def _reserve_across_shards(self, *, uploader_id: str, uploader: str, size: int) -> bool: + """Take this uploader's cross-shard in-flight reservation; return whether one is held. + + ``False`` means there is no ledger bound (the store-less construction path) — not that the + reservation was refused. A refusal raises :class:`UploadQuotaError`, the same TYPE the + in-process check raises, so the API's 409 + ``upload.reject_quota`` audit is unchanged; the + message differs on purpose, so an operator can tell the two causes apart. A ledger error is + NOT swallowed: the store being unreachable fails the upload closed. + + The headroom handed to the ledger is the cap minus what the (fleet-visible, uncached) sidecar + scan observed, so the ledger only ever holds the in-flight remainder. That is a second scan + per save — bounded by the uploader's own file count, off the event loop, and on the operator + diagnostic surface rather than the data plane.""" + if self._ledger is None: + return False + observed_files, observed_bytes = await asyncio.to_thread(self._observed_sync, uploader_id) + # Refuse an already-over-budget uploader HERE, with the on-disk wording, before consulting + # the ledger. Otherwise the ledger (handed zero headroom) refuses first and its message + # blames in-flight uploads on another shard that do not exist — a 409 that sends an operator + # hunting a phantom. Same helper as the under-lock check, so the text is one string. + refusal = self._on_disk_refusal( + uploader=uploader, + observed_files=observed_files, + observed_bytes=observed_bytes, + size=size, + ) + if refusal is not None: + raise refusal + ok = await self._ledger.reserve_upload_quota( + uploader_id, + files=1, + size_bytes=size, + max_files=self._max_files_per_user - observed_files, + max_total_bytes=self._max_total_bytes_per_user - observed_bytes, + ) + if not ok: + # Headroom was positive, so the only thing that can have consumed it is an upload in + # flight on another shard. That is exactly the double-book this control exists to refuse. + raise UploadQuotaError( + f"uploader {uploader!r} has {observed_files} uploaded files holding " + f"{observed_bytes} bytes, and another engine shard is mid-upload against the same " + f"budget; the limits are {self._max_files_per_user} files / " + f"{self._max_total_bytes_per_user} bytes" + ) + return True + + def _on_disk_refusal( + self, *, uploader: str, observed_files: int, observed_bytes: int, size: int + ) -> UploadQuotaError | None: + """The per-uploader quota verdict against what is ALREADY on disk, or ``None`` if it fits. + + One string, two callers: the under-lock check inside ``save``'s build-and-write, and the + cross-shard reservation's pre-check. The bucket key is the IMMUTABLE ``uploader_id`` (the + caller filters on it, the same value the ownership check uses), so the budget and the + ownership rule can never disagree about who a file belongs to and a recycled username is + never billed for files it cannot read. The message names the human username, because an + operator reading a 409 needs a name, not a uuid.""" + if observed_files + 1 > self._max_files_per_user: + return UploadQuotaError( + f"uploader {uploader!r} has {observed_files} uploaded files; the limit is " + f"{self._max_files_per_user}" + ) + projected = observed_bytes + size + if projected > self._max_total_bytes_per_user: + return UploadQuotaError( + f"uploader {uploader!r} would hold {projected} bytes; the limit is " + f"{self._max_total_bytes_per_user}" + ) + return None + + async def _release_across_shards(self, *, uploader_id: str, size: int) -> None: + """Pay the reservation back. Never raises: the file is already written (or already failed) by + the time this runs, so turning a ledger blip into a failed upload would be strictly worse. + A reservation that is never released is reclaimed once it goes stale — see + :meth:`messagefoundry.store.base.Store.reserve_upload_quota`.""" + if self._ledger is None: + return + try: + await self._ledger.reserve_upload_quota( + uploader_id, files=-1, size_bytes=-size, max_files=0, max_total_bytes=0 + ) + except Exception: # noqa: BLE001 — a release failure must not fail an upload that landed + _log.warning( + "could not release the cross-shard upload reservation for %s; it will be reclaimed " + "when it goes stale", + uploader_id, + exc_info=True, + ) + + def _observed_sync(self, uploader_id: str) -> tuple[int, int]: + """(file count, total bytes) already ON DISK for ``uploader_id`` — the fleet-visible half of + the budget. Sync: the caller runs it off the event loop.""" + mine = [m for m in self._scan_metas_sync() if m.uploader_id == uploader_id] + return len(mine), sum(m.size for m in mine) async def list_files(self) -> list[UploadedFileMeta]: """List all uploaded files (newest first). Undecryptable/foreign sidecars are skipped with a diff --git a/tests/test_uploads_cross_process_quota.py b/tests/test_uploads_cross_process_quota.py new file mode 100644 index 00000000..828b322e --- /dev/null +++ b/tests/test_uploads_cross_process_quota.py @@ -0,0 +1,258 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""ASVS 2.3.4 — the per-uploader upload quota must hold ACROSS processes, not only within one. + +BACKLOG #1112's surviving half. ``UploadStore._quota_lock`` is an ``asyncio.Lock``, so it is +per-event-loop and therefore per-process. Engine sharding is the built, shipped, default scaling +axis, and nothing partitions ``uploads_dir`` per shard, so N shards over one directory hold N +independent locks: each can be between its own scan and its own file landing on disk while the +others scan, and each of those overshoots the budget by one file. + +**What the two shipped tests already prove, and what they do NOT.** +``tests/test_uploads.py::test_quota_is_shared_by_stores_over_one_dir_not_per_process`` is +SEQUENTIAL — it proves the sidecar scan is uncached, so a second store SEES the first's files. That +is shared visibility, not exclusion. +``tests/test_uploads.py::test_concurrent_uploads_cannot_double_book_the_quota`` is concurrent but +lives on ONE ``UploadStore`` in ONE event loop, so the per-process lock is sufficient there by +construction. Neither one can fail on the cross-process defect. + +**What this rig models, and what it does not.** Two OS threads, each running its OWN event loop, +its OWN ``MessageStore`` connection and its OWN ``UploadStore`` over the ONE shared uploads +directory. For the property under test that is a faithful stand-in for two ``serve --shard`` +processes: two independent ``asyncio.Lock`` objects (so the shipped intra-process control provides +exactly zero protection between them) and two independent database connections over one file. + +It is NOT a two-process ``serve --shard`` run, and it CANNOT be one on SQLite: +``messagefoundry/pipeline/sharding.py::require_unified_store`` raises on any non-server backend for +more than one distinct shard id, and ``supervise`` calls it before spawning anything. So this rig +exercises the SQLite implementation of the cross-process reservation; the Postgres and SQL Server +implementations of the same one protocol method are the ones a real sharded deployment would run, +and they are not exercised here. +""" + +from __future__ import annotations + +import asyncio +import threading +import time +from pathlib import Path + +import pytest + +from messagefoundry.store.crypto import generate_key, make_cipher +from messagefoundry.store.store import MessageStore +from messagefoundry.uploads import UploadedFileMeta, UploadQuotaError, UploadStore + +#: Long enough that thread-scheduling jitter cannot separate the two scans, short enough to keep the +#: test quick. Only the BROKEN path needs it: with the reservation in place the two shards are +#: ordered by the database, not by this sleep. +_SCAN_OVERLAP_SECONDS = 0.15 + + +def _shard_result( + *, + db_path: Path, + uploads_dir: Path, + key: bytes, + barrier: threading.Barrier, + filename: str, + body: bytes, + bind_store: bool, +) -> object: + """One shard: its own loop, its own store connection, its own UploadStore. Returns the save's + result, or the exception it raised (so the caller can classify both shards' outcomes).""" + + async def _run() -> object: + store = await MessageStore.open(db_path) + try: + uploads = UploadStore( + uploads_dir, + make_cipher(key), # one DEK across the fleet — a per-shard key would fake isolation + max_bytes=4096, + max_files_per_user=1, + store=store if bind_store else None, + ) + real_scan = uploads._scan_metas_sync + + def _slow_scan() -> list[UploadedFileMeta]: + out = real_scan() + time.sleep(_SCAN_OVERLAP_SECONDS) # widen the scan -> write window + return out + + uploads._scan_metas_sync = _slow_scan # type: ignore[method-assign] + barrier.wait(timeout=30) + return await uploads.save( + data=body, filename=filename, uploader="alice", uploader_id="u-alice" + ) + finally: + await store.close() + + try: + return asyncio.run(_run()) + except BaseException as exc: # noqa: BLE001 — the exception IS the result being classified + return exc + + +def _race_two_shards(tmp_path: Path, *, bind_store: bool) -> tuple[list[object], list[Path]]: + """Run two shards concurrently over one uploads dir; return (outcomes, sidecars-on-disk).""" + db_path = tmp_path / "engine.db" + uploads_dir = tmp_path / "uploads" + key = generate_key() + # Create the schema up front so the two shards never race each other on DDL — the race under + # test is the quota one, and a "database is locked" here would be a fixture artifact. + asyncio.run(_open_and_close(db_path)) + + barrier = threading.Barrier(2) + outcomes: dict[str, object] = {} + + def _worker(name: str, filename: str, body: bytes) -> None: + outcomes[name] = _shard_result( + db_path=db_path, + uploads_dir=uploads_dir, + key=key, + barrier=barrier, + filename=filename, + body=body, + bind_store=bind_store, + ) + + threads = [ + threading.Thread(target=_worker, args=("a", "from_a.txt", b"from shard a\n")), + threading.Thread(target=_worker, args=("b", "from_b.txt", b"from shard b\n")), + ] + for t in threads: + t.start() + for t in threads: + t.join(timeout=60) + assert not any(t.is_alive() for t in threads), "a shard thread hung" + + sidecars = sorted(uploads_dir.glob("*.meta")) if uploads_dir.exists() else [] + return [outcomes["a"], outcomes["b"]], sidecars + + +async def _open_and_close(db_path: Path) -> None: + store = await MessageStore.open(db_path) + await store.close() + + +def test_two_shard_concurrent_uploads_cannot_double_book_the_quota(tmp_path: Path) -> None: + """The acceptance criterion from BACKLOG #1112: concurrent writers on two shards sharing one + uploads_dir enforce ONE budget between them, with overshoot zero.""" + outcomes, sidecars = _race_two_shards(tmp_path, bind_store=True) + + won = [o for o in outcomes if isinstance(o, UploadedFileMeta)] + refused = [o for o in outcomes if isinstance(o, UploadQuotaError)] + other = [o for o in outcomes if not isinstance(o, UploadedFileMeta | UploadQuotaError)] + + assert not other, f"a shard failed for a reason that is not the quota: {other!r}" + assert len(won) == 1, ( + "exactly one shard may win a quota of 1; the per-process asyncio.Lock does not span " + f"processes, so both can pass the check. outcomes={outcomes!r}" + ) + assert len(refused) == 1, f"the losing shard must be refused on quota. outcomes={outcomes!r}" + # Which LIMB refused matters. If the loser's scan had simply seen the winner's file already on + # disk, this test would pass on the shipped code and prove nothing about cross-process exclusion + # (that is precisely the hole in the sequential shipped test). Pin the reservation's own wording, + # which is only reachable when the scan left headroom and another shard consumed it. + assert "another engine shard is mid-upload" in str(refused[0]), ( + "the loser must be refused by the cross-shard reservation, not by a scan that happened to " + f"see the winner's file: {refused[0]!r}" + ) + # Print what was matched, not just how many: the sidecar filenames ARE the overshoot. + assert len(sidecars) == 1, ( + f"overshoot: {len(sidecars)} files landed for a budget of 1 -> {[p.name for p in sidecars]}" + ) + + +def test_the_reservation_is_released_so_the_next_upload_is_not_locked_out(tmp_path: Path) -> None: + """A reservation that is not released would permanently consume budget. Positive control that + the SAME shard can keep uploading up to (and only up to) the cap after a winning save.""" + + async def _run() -> None: + store = await MessageStore.open(tmp_path / "engine.db") + try: + uploads = UploadStore( + tmp_path / "uploads", + make_cipher(generate_key()), + max_bytes=4096, + max_files_per_user=2, + store=store, + ) + for i in range(2): + await uploads.save( + data=f"body {i}\n".encode(), + filename=f"f{i}.txt", + uploader="alice", + uploader_id="u-alice", + ) + assert len(await uploads.list_files()) == 2 + # The cap, not a stuck reservation, is what refuses the third. + with pytest.raises(UploadQuotaError) as exc: + await uploads.save( + data=b"third\n", filename="f2.txt", uploader="alice", uploader_id="u-alice" + ) + assert "the limit is 2" in str(exc.value), str(exc.value) + # And the ledger itself is back at zero in-flight: a reserve with a headroom of exactly + # one succeeds, which it could not if either completed save had left a slot outstanding. + assert await store.reserve_upload_quota( + "u-alice", files=1, size_bytes=1, max_files=1, max_total_bytes=1 + ), "a completed upload left its reservation outstanding" + finally: + await store.close() + + asyncio.run(_run()) + + +def test_a_leaked_reservation_is_reclaimed_once_it_goes_stale(tmp_path: Path) -> None: + """A process killed between reserve and release leaks its reservation. It must not consume the + uploader's budget forever: a reservation that has been continuously outstanding for longer than + ``stale_after`` is reset on the next reserve.""" + + async def _run() -> None: + store = await MessageStore.open(tmp_path / "engine.db") + try: + # Simulate the crash: reserve, never release. + assert await store.reserve_upload_quota( + "u-alice", files=1, size_bytes=10, max_files=1, max_total_bytes=100 + ) + # A live reservation blocks the next one (this is the control that the leak is real). + assert not await store.reserve_upload_quota( + "u-alice", files=1, size_bytes=10, max_files=1, max_total_bytes=100 + ) + # Once stale, the same call succeeds — the leak self-heals. + assert await store.reserve_upload_quota( + "u-alice", + files=1, + size_bytes=10, + max_files=1, + max_total_bytes=100, + stale_after=0.0, + ) + finally: + await store.close() + + asyncio.run(_run()) + + +def test_an_unbound_upload_store_still_enforces_the_in_process_budget(tmp_path: Path) -> None: + """``store=None`` (the store-less embedding/test construction path) must not regress: the + per-process quota still refuses an over-budget upload. It buys NO cross-process exclusion — + that is the documented degradation, not a second control.""" + + async def _run() -> None: + uploads = UploadStore( + tmp_path / "uploads", + make_cipher(generate_key()), + max_bytes=4096, + max_files_per_user=1, + store=None, + ) + await uploads.save( + data=b"only\n", filename="a.txt", uploader="alice", uploader_id="u-alice" + ) + with pytest.raises(UploadQuotaError): + await uploads.save( + data=b"second\n", filename="b.txt", uploader="alice", uploader_id="u-alice" + ) + + asyncio.run(_run()) From b3d1eb454e067504a0022922b03a328582837ad1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 20:29:38 -0500 Subject: [PATCH 08/31] docs(uploads): the leaked-reservation self-heal claim was false, and it was mine (BACKLOG #1112) SDS-3.7 -- a compensating control must not rest on a false premise. Two shipped docstrings landed in 142b9262 saying a leaked reservation is reclaimed once it goes stale. That is true ONLY while the uploader is otherwise idle, and neither said so. THE MECHANISM, verified in the SQL rather than taken from the review that found it. The release branch of reserve_upload_quota is: UPDATE upload_quota SET inflight_files = MAX(0, inflight_files + ?), inflight_bytes = MAX(0, inflight_bytes + ?), since = ? WHERE uploader_id = ? `since = ?` is UNCONDITIONAL, deliberately -- its own comment reads "Never conditional: refusing a release would strand the reservation it is paying back." That is correct for the release and it has a side effect nobody wrote down: every later release by the same uploader pushes the staleness clock forward. So the reserve path's staleness reset -- which IS conditional (`CASE WHEN since <= ? OR inflight_files <= 0`) -- never fires for an uploader who keeps uploading successfully. A leaked slot can survive indefinitely. WHY THIS IS WORSE THAN AN UNDOCUMENTED LIMIT: with no comment a reader measures. With "it self-heals" a reader RELIES, and the sentence is written in the confident register that earns reliance. The failure it hides is a permanently narrowed budget for exactly the uploaders who use the system most. Both sites now state the precondition, and the first keeps the retraction in place rather than quietly reading correctly -- the next reader should be able to see that the stronger claim was made and withdrawn. Found by #1112's adversarial pass, whose verdict was `weak` for this and two other reasons. I verified the mechanism in the statement before writing it here; the review named it, the SQL proved it. No code changed. No ASVS score moves. 33 passed across the two upload modules (tests/test_uploads_cross_process_quota.py + tests/test_uploads.py). The first version of this message said 56 -- a number carried forward from an unrelated earlier run, corrected by re-running rather than by memory. --- messagefoundry/uploads.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/messagefoundry/uploads.py b/messagefoundry/uploads.py index 6aaca517..d79f2d18 100644 --- a/messagefoundry/uploads.py +++ b/messagefoundry/uploads.py @@ -111,7 +111,13 @@ class UploadQuotaError(UploadError): most **N-1 files** over, one per shard mid-write, each bounded by ``max_upload_bytes``. * **A leaked reservation.** A process killed between reserve and release never pays back, and its slot narrows that uploader's budget until the row goes idle for - ``UPLOAD_RESERVATION_STALE_AFTER``. It errs toward refusing, not allowing, and it self-heals. + ``UPLOAD_RESERVATION_STALE_AFTER``. It errs toward refusing, not allowing. + **IT DOES NOT SELF-HEAL UNCONDITIONALLY, and an earlier version of this line said it did.** + The release statement sets ``since = `` **unconditionally** (its own comment: "Never + conditional: refusing a release would strand the reservation it is paying back"), so every + *subsequent* release by the same uploader pushes the staleness clock forward. **It self-heals + only while that uploader is otherwise IDLE.** A uploader who keeps uploading successfully can + hold a leaked slot indefinitely, and the reserve path's own staleness reset never fires for it. * **A reclaimed live reservation.** If one uploader keeps reservations continuously outstanding for longer than that window, the staleness reset zeroes a row that was legitimately non-zero, which restores the N-1 bound above for that window. Never worse than the pre-#1112 behaviour. @@ -618,7 +624,11 @@ def _on_disk_refusal( async def _release_across_shards(self, *, uploader_id: str, size: int) -> None: """Pay the reservation back. Never raises: the file is already written (or already failed) by the time this runs, so turning a ledger blip into a failed upload would be strictly worse. - A reservation that is never released is reclaimed once it goes stale — see + + A reservation that is never released is reclaimed once the row goes stale — **but only while + that uploader is otherwise IDLE.** This statement sets ``since = `` unconditionally, so + each later release by the same uploader restarts the staleness clock and a leaked slot can + survive indefinitely under continued activity. See :meth:`messagefoundry.store.base.Store.reserve_upload_quota`.""" if self._ledger is None: return From 77d03ca1b31d2ba82b657584367829f08411cc17 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Fri, 21 Aug 2026 21:51:33 -0500 Subject: [PATCH 09/31] fix(redact): the log redactor emitted the token it claimed to redact (BACKLOG #1183) _BEARER matched the AUTH SCHEME as its own value: "Authorization: Bearer " redacted the word "Bearer" and emitted verbatim. The one assertion covering it passed anyway, because its chosen token was pure alphanumeric and the unrelated _LONG_B64 sweep caught it -- so the suite could not tell a working pattern from a broken one. Every fixture token now carries a hyphen AND an underscore, which breaks the base64 run and puts each family on its own pattern. Widens the secret domain to what the module's docstring already implied: a bare "Bearer " with no header label (_AUTH_SCHEME), password/PWD/secret pairs (_CREDENTIAL_KV), and an inline DSN password (_DSN_PASSWORD). Quote handling is uniform -- the value class stops at any quote, so a mismatched pair loses the value and keeps only a stray character. Measured: 0 leaks across 7 quote shapes, including both mismatched orders. Two label words are deliberately EXCLUDED and commented as such: "key" and the basic/digest scheme words. All three are ordinary vocabulary in this codebase, so matching them would redact operator diagnostics and buy no confidentiality. resolve_env_settings no longer echoes a failed env() value. It appeared TWICE -- once from the f-string and once inside the cast's own ValueError text -- so dropping the f-string half alone would still have leaked it. The error now names the setting, the key and the expected TYPE, and says the value was withheld. Which patterns exist is no longer a docstring claim: the suite derives them by AST and fails if one is applied without a named family, or declared without being applied. .gitleaks.toml: nine EXACT-literal allowlist entries for the new synthetic needles, each traced to its fixture. Exact literals, never a family prefix -- "sk-live-.*" would have allowlisted a real leaked key of that shape. Verified both directions: the nine listed values are silenced, and four unlisted values in the same four shapes are still caught. --- .gitleaks.toml | 30 +++ messagefoundry/config/wiring.py | 14 +- messagefoundry/support/redact.py | 88 +++++-- tests/test_environments.py | 25 ++ tests/test_log_redaction_secret_domain.py | 276 ++++++++++++++++++++++ 5 files changed, 414 insertions(+), 19 deletions(-) create mode 100644 tests/test_log_redaction_secret_domain.py diff --git a/.gitleaks.toml b/.gitleaks.toml index b0fdb1c2..08cf3435 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -22,4 +22,34 @@ regexes = [ # Fake test-fixture lease key (ADR 0114 claim-proc live test, tests/test_adr0114_claim_proc_live.py): # a hardcoded literal string naming the test scenario, not a credential — trips generic-api-key on entropy. '''adr0114-fence-test''', + # --- BACKLOG #1183 -- log-redaction secret-domain fixtures -------------------------------------- + # Nine SYNTHETIC needles invented for tests/test_log_redaction_secret_domain.py (plus one sibling in + # tests/test_environments.py). Each is a value a redaction test asserts does NOT survive a log line, + # so each is published in test source by construction. They trip generic-api-key on ENTROPY, and the + # entropy is deliberate: every token carries a hyphen AND an underscore to break the long-base64 + # run, so no family can be redacted by the backstop by accident and pass for the wrong reason. + # + # EXACT LITERALS, never a family prefix. A tidy `sk-live-.*` would also allowlist a REAL leaked key + # that happened to match the shape, leaving the scanner blind to the exact thing it exists to catch. + # + # Family "mefor_env_value" -- a MEFOR_STORE_ENCRYPTION_KEY= startup line; pattern _MEFOR_SECRET. + '''kv-Str0ng_Key-11''', + # Family "labelled_token" -- "cache hit token: ..."; pattern _BEARER. + '''mf-sess_9aZ-Qx_7Lp''', + # Family "api_key_kv" -- "calling partner api_key=..."; pattern _BEARER. + '''ak-live_7Qp-3Zx''', + # Family "mfb64_body" -- an mfb64:v1: carriage blob; patterns _MFB64 + _LONG_B64. This one is + # DEMONSTRABLY not a secret: it base64-decodes to the ASCII string "HelloWorldHelloWorld". + '''SGVsbG9Xb3JsZEhlbGxvV29ybGQ=''', + # Family "authorization_bearer_header" -- "Authorization: Bearer ..."; _BEARER + _AUTH_SCHEME. + '''sk-live-AbCdEf_1234-XYZ''', + # Family "bare_auth_scheme" -- a bare "Bearer ..." retry line; pattern _AUTH_SCHEME. + '''sk-live-Zz9_Aa8-QQwe''', + # Family "password_kv" -- "connect failed password=..."; pattern _CREDENTIAL_KV. + '''pw-Str0ng_Pass-22''', + # Family "odbc_pwd" -- an ODBC connection string's PWD=; pattern _CREDENTIAL_KV. + '''pw-0dbc_Pass-33''', + # tests/test_environments.py -- the needle proving a cast failure never echoes an env() value into + # the startup error (#1183). Never a real credential; it is chosen to FAIL int() on purpose. + '''pw-C4st_Val-66''', ] diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index 07df85a6..ec7573bd 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -594,7 +594,19 @@ def resolve_env_settings(settings: Mapping[str, Any], values: Mapping[str, Any]) try: resolved[name] = value.cast(raw) except (ValueError, TypeError) as exc: - bad.append(f"setting {name!r} (env {value.key!r}={raw!r}): {exc}") + # NEVER the raw value: a MEFOR_VALUE_* env() setting carries store passwords + # and connector keys, and this string is raised at startup into the operator + # log, the support bundle and GET /logs/tail (BACKLOG #1183). The value used to + # appear TWICE here -- once from this f-string and once inside the cast's own + # ValueError text ("invalid literal for int() with base 10: ''") -- so + # dropping only the f-string half would still have leaked it. Name the setting, + # the key and the expected TYPE, which is the whole diagnostic an operator + # needs to go fix the value they already hold. + want = getattr(value.cast, "__name__", None) or type(value.cast).__name__ + bad.append( + f"setting {name!r} (env {value.key!r}): value is not a valid {want} " + f"({type(exc).__name__}; value withheld)" + ) elif value.default is not _UNSET: resolved[name] = value.default else: diff --git a/messagefoundry/support/redact.py b/messagefoundry/support/redact.py index 444e0164..ff6d856d 100644 --- a/messagefoundry/support/redact.py +++ b/messagefoundry/support/redact.py @@ -14,8 +14,18 @@ The PHI pass is **delegated to the shared engine redactor** (:func:`messagefoundry.redaction.redact`, also pure stdlib ``re``) so bundled logs get exactly the same HL7-segment / field-run / DOB / multi-token-name coverage as stored ``last_error``/log lines — instead of a second, narrower copy that drifts out of sync -(DELTA-07). This module adds only the **secret** markers the engine redactor does not carry -(``mfb64:`` bodies, ``MEFOR_*`` values, bearer/session tokens, long base64 runs). +(DELTA-07). This module adds the **secret** markers the engine redactor does not carry — at least +``mfb64:`` bodies, ``MEFOR_*`` values, bearer/authorization tokens, ``password=``/``PWD=``/``secret=`` +pairs, an inline DSN password, and a long base64 run as the backstop. + +**Which patterns exist is not a claim to be read off this docstring.** Every pattern +:func:`redact_log_line` applies is derived by AST in ``tests/test_log_redaction_secret_domain.py`` and +must be claimed by a named secret family there, so adding one without a fixture reds the suite. That +guard exists because this module shipped a redactor that redacted nothing while the suite was green +(BACKLOG #1183): ``_BEARER`` consumed the word ``Bearer`` as its own value match and emitted the token +after it, and the one assertion covering it passed only because its chosen token was pure alphanumeric +and the unrelated long-base64 sweep caught it. Every fixture token now carries a hyphen and an +underscore so it cannot be reached by that sweep. """ from __future__ import annotations @@ -33,10 +43,49 @@ _MFB64 = re.compile(r"mfb64:v1:[A-Za-z0-9+/=]+") # A bearer/authorization token or an opaque session token in a header-ish or "token=" shape. -_BEARER = re.compile(r"(?i)\b(bearer|authorization|token|session|api[_-]?key)\b\s*[:=]\s*\S+") - -# A MEFOR_* secret echoed as "MEFOR_FOO=value" or "MEFOR_FOO: value": never carry the value. -_MEFOR_SECRET = re.compile(r"\bMEFOR_[A-Z0-9_]+\s*[:=]\s*\S+") +# +# The ``(?:bearer|basic|digest)\s+`` group is load-bearing, not decoration: without it ``\S+`` matches +# the AUTH SCHEME rather than the credential, so "Authorization: Bearer " redacted the word +# "Bearer" and emitted verbatim. Making the group optional keeps the plain "token=" shape +# working, and the value class excludes quotes so a quoted credential loses the value, not the quote. +_BEARER = re.compile( + r"(?i)\b(bearer|authorization|token|session|api[_-]?key)\b" + r"\s*[:=]\s*(?:(?:bearer|basic|digest)\s+)?['\"]?[^\s'\"]+" +) + +# A bare auth scheme carrying its credential with no preceding header label — "Bearer " as it +# appears in a WWW-Authenticate echo or a client retry line. ``_BEARER`` cannot reach this: it requires +# a ":" or "=" after the label, and there is none here. The scheme word is kept so a reviewer sees what +# leaked. +# +# "bearer" ONLY, deliberately, even though ``_BEARER`` above accepts basic and digest as scheme words. +# There the header label guarantees the line is an authorization header; here nothing does, and both +# other words are ordinary configuration vocabulary in this codebase — ``transports/http_auth.py`` +# raises "oauth2_auth_style must be 'basic' or 'post'" and ``transports/soap.py`` raises +# "ws_password_type must be 'text' or 'digest'". A token that is also ordinary vocabulary discriminates +# nothing, so matching on it would redact operator diagnostics and buy no confidentiality: a labelled +# "Authorization: Basic " is already carried by ``_BEARER``. +_AUTH_SCHEME = re.compile(r"(?i)\b(bearer)\s+['\"]?[^\s'\",;]{4,}") + +# A MEFOR_* secret echoed as "MEFOR_FOO=value" or "MEFOR_FOO: value": never carry the value. The +# optional quotes match the shape an error string produces — "(env 'MEFOR_VALUE_PW'='')" — which +# the unquoted form missed entirely. +_MEFOR_SECRET = re.compile(r"\b(MEFOR_[A-Z0-9_]+)\b['\"]?\s*[:=]\s*['\"]?[^\s'\"]+['\"]?") + +# A credential in a "