diff --git a/changelog.d/2812-fhir-repeated-json-key-refused.security.md b/changelog.d/2812-fhir-repeated-json-key-refused.security.md new file mode 100644 index 0000000000..3682b2438a --- /dev/null +++ b/changelog.d/2812-fhir-repeated-json-key-refused.security.md @@ -0,0 +1,12 @@ +- **`FhirPeek.parse` now refuses a FHIR body in which a JSON object repeats a key.** A body for a + single-resource interaction, such as create, update or a conditional one, that repeats a key is + dead-lettered with code `bad-body`, and nothing is sent. A Router that peeks such a body takes the + error path, and a `fhir_lookup` whose reply repeats a key fails. A wrapped update's 2xx reply that + repeats a key is read keeping the first and keeping the last value of each key. When the two + readings would be judged differently, the update is a permanent refusal, code + `reply-repeats-a-key`, and is not recorded as delivered; ADR 0095 Amendment B.1 states the rule. + The text of the `bad-body` refusal now names which check fired. A `transaction` or `batch` Bundle + was already refused for a repeated key (vault BACKLOG #2804). Other reads still keep the last of + two equal keys, at least `FhirResource.parse`, `RawMessage.json()` and the classification of + other FHIR replies. See [CONNECTIONS.md](../docs/CONNECTIONS.md), "Interactions". + (`vault BACKLOG #2812`) diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index 7027092eef..5ae8ad970e 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -2389,6 +2389,13 @@ source (`Http()`, File, a `Loopback` re-ingress) as a `RawMessage`. per entry); the engine never orchestrates cross-entry atomicity. The engine does screen every entry first, as [A Bundle connection screens every entry](#a-bundle-connection-screens-every-entry) says. +On every interaction but `transaction` and `batch`, a body in which a JSON object repeats a key is refused +before a byte is sent, for the reason rule 2 of +[the Bundle screen](#a-bundle-connection-screens-every-entry) gives: a permanent refusal, code +`bad-body` (vault BACKLOG #2812). A Bundle that repeats a key is refused by the screen instead, code +`bundle-entry-refused`. `FhirPeek.parse` raises `FhirPeekError` on such a body, so a Router that peeks one +takes the error path, and so does a `fhir_lookup` whose reply repeats a key. + **Conditional knobs (idempotency / concurrency).** FHIR's native answer to the at-least-once duplicate problem — opt-in, off by default: `if-none-exist` (create only if no match; the search rides the `If-None-Exist` **header**), `conditional-update` (the server resolves which resource to update; the search @@ -2456,6 +2463,15 @@ What a site needs to know: - If a server answers 2xx while the entry's own `response.status` failed, the message is classified on that entry status, like any other HTTP status. An entry status that does not read as an HTTP code is logged as a warning, and the 2xx reply counts as delivered. +- A reply in which a JSON object repeats a key is read twice, once keeping the first value of each key + and once keeping the last (vault BACKLOG #2812). When the two readings reach the same judgment and + agree on what a delivery takes, and no occurrence of a key repeated three or more times differs + from both its first and last value, the reply is judged as above. Otherwise the update is a permanent + refusal, code `reply-repeats-a-key`, in either capture mode. It is not sent again and not recorded + as delivered, as with a reply over the byte bound below. The full rule, with what counts as + agreement and its costs, is in + [ADR 0095](adr/0095-connection-lifecycle-scheduler-and-credential-fault-stop.md), Amendment B.1, + the 2026-10-17 entry. - A reply body over the byte bound is not kept, so the engine cannot see the entry's status. The update is then a permanent refusal, code `reply-too-large`, in either capture mode, and it is not sent again. It is not recorded as delivered. See *A 2xx reply over the byte bound* under [REST](#rest--rest). diff --git a/docs/adr/0095-connection-lifecycle-scheduler-and-credential-fault-stop.md b/docs/adr/0095-connection-lifecycle-scheduler-and-credential-fault-stop.md index 36e94c2040..de28634f2f 100644 --- a/docs/adr/0095-connection-lifecycle-scheduler-and-credential-fault-stop.md +++ b/docs/adr/0095-connection-lifecycle-scheduler-and-credential-fault-stop.md @@ -413,3 +413,41 @@ the lane, and only because the proxy handler screened a challenge that was not i Tests: `tests/test_digest_refusal_classification.py`, the section headed BACKLOG #2989, and `tests/test_over_cap_2xx_reply_is_sent_once.py`, the section at its end. + +#### B.1, 2026-10-17: a FHIR update reply that repeats a JSON key and reads two ways (vault BACKLOG #2812, batch 265) + +> **Recorded 2026-10-17 by a Builder seat for batch 265.** It describes the code in the pull +> request for vault BACKLOG #2812, not the #2989 work above. This entry is the one statement of the +> rule; other documents link here. + +A FHIR `update` or `if-match` goes out as a one-entry `transaction`, and the engine reads the +entry's status in the 2xx reply to judge it. `json` keeps the last of two equal keys and a server +may mean the first, so a reply that repeats a key is read twice, keeping the first value of each +key and keeping the last. When both readings reach the same judgment (delivered with one capture +outcome, retried, or dead-lettered with one code) and agree on the values a delivery takes (the +resource type, whether there is an entry response, and the entry's `etag`, `location` and +`lastModified`), the reply is judged as any other. So `200 OK` against `201 Created` is delivered, +and two statuses that both classify as transient are retried. When the readings differ in any of +those, in either capture mode, or the keep-first read cannot parse a body the plain read takes, +the delivery is a permanent refusal, code `reply-repeats-a-key`, not sent again and not recorded +as delivered, as a reply the engine needs and cannot read is above. The same refusal applies when +a key occurs three or more times in one object, at any depth, and some occurrence equals neither +its first nor its last value, because the two readings alone would never see it (so +`200 OK`, `404 Not Found`, `200 OK` dead-letters, as does such a repeat inside `meta`). + +It fails closed only on disagreement because a reply that reads one way under both rules carries +no ambiguity to resolve, and refusing it would dead-letter an update that applied, which a replay +of an `if-match` update turns into a 412. On `main` before this, `404 Not Found` then `200 OK` was +delivered on the last key. + +**Provenance: decided 2026-10-17 under the owner's standing driver rule, by the Lander's send-back +of engine PR 2356, and narrowed by the batch 265 Manager to compare judgments rather than status +text. The owner has not ruled on it and may overturn it.** + +Costs, at least these. A reply nested in the narrow band between the hook's depth limit and plain +`json`'s is refused with this code although it may repeat no key, and its text then says it does. +An ETag that differs with capture off refuses, though nothing reads it in that mode. + +It is raised in `transports/fhir.py` (`_agreed_reading`, comparing `_reply_reading`), not by +`read_2xx_reply_text`. Tests: `tests/test_fhir_transport.py`, the section headed vault BACKLOG +#2812. diff --git a/messagefoundry/parsing/fhir/peek.py b/messagefoundry/parsing/fhir/peek.py index ebf7a49e44..3aeb389421 100644 --- a/messagefoundry/parsing/fhir/peek.py +++ b/messagefoundry/parsing/fhir/peek.py @@ -31,7 +31,11 @@ from messagefoundry.parsing.fhir._deps import load_fhirpathpy from messagefoundry.parsing.fhir.errors import FhirPeekError -from messagefoundry.redaction import json_loads_or_refusal +from messagefoundry.redaction import ( + RepeatedKeyError, + json_loads_or_refusal, + refuse_repeated_keys, +) __all__ = ["FhirPeek"] @@ -56,7 +60,17 @@ def parse(cls, raw: str | bytes, *, format: str | None = None) -> FhirPeek: """Peek the FHIR resource in ``raw``. Raises :class:`FhirPeekError` if the body is not parseable FHIR JSON (not JSON, or not a resource - *object*), or if it is FHIR-XML (deferred to a hardened path; JSON-only MVP per ADR 0022).""" + *object*), or if it is FHIR-XML (deferred to a hardened path; JSON-only MVP per ADR 0022). + + Also refused: a JSON object that repeats a key, at any depth (vault BACKLOG #2812; see + :func:`~messagefoundry.redaction.refuse_repeated_keys`). A caller gates on the peeked value + while the body itself goes on, sent verbatim or read by other code, so it is refused rather + than canonicalised. A caller may judge a refused body further on its own terms, as the + wrapped-update reply does. Only keys equal once JSON-decoded count, so gaps remain, + at least these: a server that folds case, normalises Unicode, or cuts a key at an escaped + NUL can still read two keys the peek reads as distinct. The other way, ``bytes`` are decoded + with replacement first, so two distinct invalid-UTF-8 keys can collapse into one and be + refused. The refusal names neither the key nor a value.""" if isinstance(raw, (bytes, bytearray)): raw = bytes(raw).decode("utf-8", "replace") raw = raw.lstrip("") # tolerate a leading UTF-8 BOM (json.loads would otherwise choke) @@ -67,7 +81,9 @@ def parse(cls, raw: str | bytes, *, format: str | None = None) -> FhirPeek: ) # PHI rule: name the failure, never echo the body. No handler here, so the decode error # (which holds the body) is on neither chain (BACKLOG #2048). - parsed, refusal = json_loads_or_refusal(raw) + parsed, refusal = json_loads_or_refusal(raw, object_pairs_hook=refuse_repeated_keys) + if refusal == RepeatedKeyError.__name__: + raise FhirPeekError("a JSON object in the FHIR body repeats a key") if refusal is not None: raise FhirPeekError(f"body is not parseable JSON for FHIR ({refusal})") if not isinstance(parsed, dict): diff --git a/messagefoundry/redaction.py b/messagefoundry/redaction.py index e565b67e1f..7f305a9854 100644 --- a/messagefoundry/redaction.py +++ b/messagefoundry/redaction.py @@ -90,6 +90,7 @@ from typing import Any __all__ = [ + "RepeatedKeyError", "clamp_untrusted", "codec_safe_line", "codec_safe_str", @@ -99,6 +100,7 @@ "prints_a_codec_error", "redact", "redact_untrusted", + "refuse_repeated_keys", "safe_error", "safe_exc", "safe_name", @@ -3178,7 +3180,7 @@ def json_loads_or_refusal( ``UnicodeDecodeError`` on bytes, or json's depth-limit ``RecursionError``, which is a ``RuntimeError`` a ``ValueError`` arm does not reach -- BACKLOG #1600). A ``ValueError`` subclass that ``object_pairs_hook`` raises comes back as its class name too, so a caller can - tell its own refusal apart (vault BACKLOG #2804 refuses a repeated key that way). + tell its own refusal apart: :func:`refuse_repeated_keys` refuses a repeated key that way. It returns rather than raises so a caller's refusal is raised outside any handler (BACKLOG #2048). The decode error holds the WHOLE input (``JSONDecodeError.doc``, @@ -3193,3 +3195,21 @@ def json_loads_or_refusal( except (ValueError, RecursionError) as exc: refusal = type(exc).__name__ return None, refusal + + +class RepeatedKeyError(ValueError): + """A JSON object repeats a key. Carries neither the key nor any value, so its class name, the + hint :func:`json_loads_or_refusal` returns for it, is content-free.""" + + +def refuse_repeated_keys(pairs: list[tuple[str, Any]]) -> dict[str, Any]: + """An ``object_pairs_hook`` for :func:`json_loads_or_refusal` that refuses a repeated key at any + depth, raising :class:`RepeatedKeyError`. + + ``json.loads`` keeps the LAST of two equal keys; a server may keep the first. A body the engine + gates on one value and forwards verbatim would then be read two ways (vault BACKLOG #2804 for a + Bundle, #2812 for a single FHIR resource), so it is refused rather than canonicalised.""" + obj = dict(pairs) + if len(obj) != len(pairs): + raise RepeatedKeyError + return obj diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index aace7d144e..f306f81f61 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -71,7 +71,11 @@ from messagefoundry.controlchars import has_control_char from messagefoundry.fhirsearch import FhirSearchParams, resolve_search_pairs from messagefoundry.parsing.fhir import FhirPeek, FhirPeekError -from messagefoundry.redaction import json_loads_or_refusal +from messagefoundry.redaction import ( + RepeatedKeyError, + json_loads_or_refusal, + refuse_repeated_keys, +) from messagefoundry.transports.base import ( DeliveryError, DeliveryResponse, @@ -301,19 +305,6 @@ def entry_screen_settings(interaction: str, methods: object, types: object) -> _ ) -class _RepeatedKey(ValueError): - """A JSON object in the body repeats a key. Carries neither the key nor any value.""" - - -def _refuse_repeated_keys(pairs: list[tuple[str, Any]]) -> dict[str, Any]: - # json.loads keeps the LAST of two equal keys; a server may keep the first. Refused, so the - # screen and the server can never read two different entries out of one body. - obj = dict(pairs) - if len(obj) != len(pairs): - raise _RepeatedKey - return obj - - #: A percent-encoded control character (%00-%1F, %7F). A server decodes it back into one. _ENCODED_CONTROL_RE = re.compile(r"%(?:[01][0-9A-Fa-f]|7[Ff])") @@ -405,12 +396,116 @@ def _screen_bundle(payload: str, interaction: str, screen: _EntryScreen) -> None ) +#: The ``code`` of the permanent refusal for a wrapped update's 2xx reply that repeats a JSON key. +_REPLY_REPEATS_A_KEY_CODE = "reply-repeats-a-key" + + +class _ThirdReading(ValueError): + """A key occurs with a value equal to neither its first nor its last. Carries no key or value.""" + + +def _keep_first_key(pairs: list[tuple[str, Any]]) -> dict[str, Any]: + """An ``object_pairs_hook`` that keeps the FIRST of two equal keys, where ``json`` keeps the + last: the reading a server that keeps the first would make. Raises :class:`_ThirdReading` when + a key occurs three or more times and some occurrence equals neither its first nor its last + value, since comparing the first and last readings would never see it.""" + first: dict[str, Any] = {} + last: dict[str, Any] = {} + for key, value in pairs: + first.setdefault(key, value) + last[key] = value + if len(first) != len(pairs) and any( + value != first[key] and value != last[key] for key, value in pairs + ): + raise _ThirdReading + return first + + +def _entry_response(obj: dict[str, Any]) -> dict[str, Any] | None: + """The first entry's ``response`` of a ``Bundle`` reply, or None. The one walk both + :func:`_unwrap_transaction_reply` and :func:`_reply_reading` take, so they cannot drift.""" + entries = obj.get("entry") if obj.get("resourceType") == "Bundle" else None + first = entries[0] if isinstance(entries, list) and entries else None + response = first.get("response") if isinstance(first, dict) else None + return response if isinstance(response, dict) else None + + +def _entry_status(response: dict[str, Any]) -> int | None: + """The entry ``response.status`` as an HTTP code, or None when it does not read as one.""" + status = response.get("status") + m = _ENTRY_STATUS_RE.match(str(status)) if isinstance(status, (str, int, float)) else None + return None if m is None else int(m.group(1)) + + +def _entry_verdict(obj: dict[str, Any]) -> DeliveryError | str: + """What :func:`_unwrap_transaction_reply` decides from a parsed reply: the classified failure + it raises, or the capture outcome of a delivery. An entry status that does not read as an HTTP + code is a delivery (the caller logs it).""" + response = _entry_response(obj) + if response is None: + return _capture_outcome_of(obj) + outcome = _as_operation_outcome(response.get("outcome")) + status = _entry_status(response) + if status is not None: + failure = _classify_status(status, outcome, where="transaction entry status") + if failure is not None: + return failure + return _capture_outcome_of(outcome) if outcome is not None else "accepted" + + +def _reply_reading(obj: dict[str, Any]) -> tuple[object, ...]: + """The judgment :func:`_unwrap_transaction_reply` reaches on a parsed reply, plus the values a + delivery takes from it. Two readings that give equal tuples are judged identically; the rule is + stated once, in ADR 0095 Amendment B.1 (the 2026-10-17 entry).""" + verdict = _entry_verdict(obj) + judged: tuple[object, ...] = ( + ( + type(verdict).__name__, + getattr(verdict, "code", None), + getattr(verdict, "permanent", None), + ) + if isinstance(verdict, DeliveryError) + else ("delivered", verdict) + ) + response = _entry_response(obj) + consumed = ( + () if response is None else tuple(response.get(f) for f in _ENTRY_RESPONSE_HEADERS.values()) + ) + return (obj.get("resourceType"), response is not None, judged, consumed) + + +def _agreed_reading(body: str) -> dict[str, Any] | None: + """A reply :class:`FhirPeek` refused, read once keeping the first and once keeping the last + value of each repeated key (vault BACKLOG #2812). Returns the reading when both are JSON objects + and :func:`_reply_reading` gives them equal tuples, or ``None`` when the plain read is not a + JSON object, which is judged ``unparseable`` as before. Otherwise, including a body only the + plain read can parse, raises the permanent ``reply-repeats-a-key`` refusal, which names neither + a key nor a value. ADR 0095 Amendment B.1 (the 2026-10-17 entry) states the rule and its + costs.""" + text = body.lstrip(_BOM) + # The plain read first: a body that is not a JSON object stops here, after one more parse. + last, hint = json_loads_or_refusal(text) + if hint is not None or not isinstance(last, dict): + return None + first, hint = json_loads_or_refusal(text, object_pairs_hook=_keep_first_key) + if hint is not None or _reply_reading(first) != _reply_reading(last): + raise NegativeAckError( + f"{_REPLY_REPEATS_A_KEY_CODE}, not sent again. A 2xx reply body repeats a JSON key " + "and is needed here. Check the partner before a replay (FHIR destination)", + code=_REPLY_REPEATS_A_KEY_CODE, + permanent=True, + ) + return last + + def _bundle_refusal(payload: str, interaction: str, screen: _EntryScreen) -> str | None: """Why :func:`_screen_bundle` refuses ``payload``, or None when every entry passes.""" + # A repeated key is refused, so the screen and the server can never read two different + # entries out of one body. parsed, hint = json_loads_or_refusal( - payload.lstrip(_BOM), object_pairs_hook=_refuse_repeated_keys + payload.lstrip(_BOM), object_pairs_hook=refuse_repeated_keys ) - if hint == _RepeatedKey.__name__: + if hint == RepeatedKeyError.__name__: return "a JSON object in it repeats a key" if hint is not None: return f"it is not parseable JSON ({hint})" @@ -653,32 +748,36 @@ def _unwrap_transaction_reply( end in a 412 and a false dead-letter. PHI-safe: only the code is named, never the entry's ``status`` text or ``outcome``. + A reply that repeats a JSON key is read twice, keeping the first and keeping the last value of + each key (vault BACKLOG #2812), and judged as any other only when the readings agree; see + :func:`_agreed_reading`. + The entry's ``etag``/``location``/``lastModified`` describe the updated resource, so a field the entry carries suppresses the reply header of the same name, which describes the Bundle, even when the entry's value is then dropped. A value carrying a control character, or longer than a header value may be, is dropped: it came from a JSON body, which, unlike an HTTP header line, bounds neither. The rest are offered to the #154 allow-list.""" try: - peek = FhirPeek.parse(body) + obj: dict[str, Any] | None = FhirPeek.parse(body).obj except FhirPeekError: - return headers, "unparseable" - entries = peek.obj.get("entry") if peek.resource_type == "Bundle" else None - first = entries[0] if isinstance(entries, list) and entries else None - response = first.get("response") if isinstance(first, dict) else None - if not isinstance(response, dict): - return headers, _capture_outcome_of(peek.obj) - outcome = _as_operation_outcome(response.get("outcome")) - status = response.get("status") - m = _ENTRY_STATUS_RE.match(str(status)) if isinstance(status, (str, int, float)) else None - if m is None: + obj = None + # Judged outside the handler, so nothing rides on the refusal's __context__ (BACKLOG #2048). + if obj is None: + # FhirPeek refuses a repeated key; a reply whose two readings agree is judged as before. + obj = _agreed_reading(body) + if obj is None: + return headers, "unparseable" + verdict = _entry_verdict(obj) + if isinstance(verdict, DeliveryError): + raise verdict + response = _entry_response(obj) + if response is None: + return headers, verdict + if _entry_status(response) is None: logger.warning( "FHIR transaction-response entry carries no readable status; taking the 2xx reply " "as delivered" ) - else: - failure = _classify_status(int(m.group(1)), outcome, where="transaction entry status") - if failure is not None: - raise failure present = { name: response[field] for name, field in _ENTRY_RESPONSE_HEADERS.items() @@ -694,7 +793,7 @@ def _unwrap_transaction_reply( taken = {name.lower() for name in present} kept = {name: value for name, value in headers.items() if name.lower() not in taken} captured = {**kept, **capture_response_headers(usable, allowlist)} - return captured, _capture_outcome_of(outcome) if outcome is not None else "accepted" + return captured, verdict class FhirDestination(DestinationConnector): @@ -986,8 +1085,10 @@ def _peek(self, payload: str) -> FhirPeek: try: return FhirPeek.parse(payload) except FhirPeekError as exc: + # A FhirPeekError's text is content-free; it says which refusal fired (a repeated key + # is valid JSON, so "not parseable" alone would mislead the operator). raise NegativeAckError( - "outgoing FHIR body is not parseable JSON", code="bad-body", permanent=True + f"outgoing FHIR body refused: {exc}", code="bad-body", permanent=True ) from exc def _resolve_request(self, payload: str) -> _FhirRequest: diff --git a/tests/test_dependency_boundaries.py b/tests/test_dependency_boundaries.py index 0f8c77843b..a8bde61193 100644 --- a/tests/test_dependency_boundaries.py +++ b/tests/test_dependency_boundaries.py @@ -1245,8 +1245,9 @@ def test_the_tray_leaf_probe_sees_an_engine_import_and_a_third_party_one(tmp_pat "messagefoundry.parsing", "messagefoundry.content_type", "messagefoundry.controlchars", - # `parsing/fhir` takes `json_loads_or_refusal` from here (BACKLOG #2048). Stdlib-only, and the - # static walk below holds it to that. + # `parsing/fhir` takes `json_loads_or_refusal` from here (BACKLOG #2048), and its repeated-key + # hook (vault BACKLOG #2812), among other names. Stdlib-only, and the static walk below holds it + # to that. "messagefoundry.redaction", "messagefoundry.timezone", # The vendored defusedxml copy `parsing/message.py` parses XML through. Upstream code that diff --git a/tests/test_fhir_lookup.py b/tests/test_fhir_lookup.py index 2ace059127..5c654ccdd1 100644 --- a/tests/test_fhir_lookup.py +++ b/tests/test_fhir_lookup.py @@ -590,6 +590,17 @@ async def test_error_on_unparseable_body() -> None: # AC-6 (unparseable) await ex.read("epic", "Patient/123") +async def test_error_on_a_reply_that_repeats_a_key() -> None: + # Vault BACKLOG #2812: json keeps the last of two equal keys, so a Handler would read a value + # a first-key reader would not. Refused as unparseable; the text names neither id. + canary = "mf2812-second-id" + body = '{"resourceType": "Patient", "id": "first-id", "id": "' + canary + '"}' + ex, _ = _executor(body=body.encode()) + with pytest.raises(FhirLookupError, match="unparseable") as ei: + await ex.read("epic", "Patient/123") + assert canary not in str(ei.value) and "first-id" not in str(ei.value) + + async def test_error_on_network_failure() -> None: # AC-6 (timeout/conn) ex, _ = _executor(exc=urllib.error.URLError("connection refused")) with pytest.raises(FhirLookupError, match="unreachable"): diff --git a/tests/test_fhir_parsing.py b/tests/test_fhir_parsing.py index 7972694669..1aeef53a22 100644 --- a/tests/test_fhir_parsing.py +++ b/tests/test_fhir_parsing.py @@ -162,6 +162,60 @@ def test_peek_error_never_leaks_the_body() -> None: assert exc.__cause__ is None and exc.__context__ is None +# --- a repeated JSON key is refused (vault BACKLOG #2812) --------------------- +# +# json keeps the LAST of two equal keys and a server may keep the first, while the engine gates and +# routes on the peeked value and sends the body verbatim. Every key and value below is synthetic; the +# canary is the value the refusal must not echo. + +_REPEATED_ID = '{"resourceType": "Patient", "id": "first-id", "id": "' + PHI_CANARY + '"}' + + +@pytest.mark.parametrize( + "body", + [ + _REPEATED_ID, + # The measured case: the URL took Patient while a server keeping the first key reads + # Observation. + '{"resourceType": "Observation", "resourceType": "Patient", "id": "' + PHI_CANARY + '"}', + '{"resourceType": "Patient", "id": "p-1", "meta": {"versionId": "1", "versionId": "' + + PHI_CANARY + + '"}}', + _REPEATED_ID.encode("utf-8"), + "\N{ZERO WIDTH NO-BREAK SPACE}" + _REPEATED_ID, + ], + ids=["top-level-id", "observation-then-patient", "nested", "bytes", "behind-a-bom"], +) +def test_peek_refuses_a_repeated_key_at_any_depth(body: str | bytes) -> None: + with pytest.raises(FhirPeekError, match="repeats a key") as excinfo: + FhirPeek.parse(body) + exc = excinfo.value + # Neither the key nor a value is named, and nothing is chained. + for word in ("id", "resourceType", "versionId", "Patient", "first-id", PHI_CANARY): + assert word not in str(exc) + assert exc.__cause__ is None and exc.__context__ is None + + +def test_peek_accepts_equal_keys_in_different_objects() -> None: + # Control: the refusal is per object. Sibling and nested objects may each carry the same key, + # as every Bundle entry carries its own resource id. + body = json.dumps( + { + "resourceType": "Bundle", + "id": "b-1", + "type": "transaction", + "meta": {"id": "m-1"}, + "entry": [ + {"resource": {"resourceType": "Patient", "id": "p-1"}}, + {"resource": {"resourceType": "Observation", "id": "o-1"}}, + ], + } + ) + peek = FhirPeek.parse(body) + assert peek.id == "b-1" + assert peek.entry_resource_types() == ["Patient", "Observation"] + + # --- console carve-out: import purity (mirrors tests/test_x12_parsing.py) ---- diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index b97feabe6f..70b0f9b9e6 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -25,6 +25,7 @@ OPERATION_OUTCOME_SUCCESS, OPERATION_OUTCOME_TRANSIENT, PATIENT_R4B, + PHI_CANARY, as_json, ) @@ -725,6 +726,271 @@ async def test_too_deep_2xx_reply_captures_as_unparseable() -> None: assert isinstance(resp, DeliveryResponse) and resp.outcome == "unparseable" +# --- vault BACKLOG #2812: an outgoing body that repeats a JSON key is refused; a wrapped update's +# reply that does is judged only when its two readings agree --------------------------------------- + +#: Synthetic. json keeps the last id, so the engine would gate on the canary (and, in the path +#: form, put it in the URL) while a server keeping the first key would write first-id. +_REPEATED_ID = '{"resourceType": "Patient", "id": "first-id", "id": "' + PHI_CANARY + '"}' + + +@pytest.mark.parametrize( + "over", + [ + {"interaction": "create"}, + {"interaction": "update"}, + {"conditional": "if-match"}, + {"interaction": "update", "update_url_form": "path"}, + {"conditional": "conditional-update", "conditional_query": "identifier=sys|val"}, + {"conditional": "if-none-exist", "conditional_query": "identifier=sys|val"}, + ], + ids=["create", "update", "if-match", "path-form", "conditional-update", "if-none-exist"], +) +async def test_an_outgoing_body_that_repeats_a_key_is_dead_lettered_unsent( + over: dict[str, str], +) -> None: + dest = _dest(**over) + opener = _FakeOpener(body=_transaction_response("200 OK")) + dest._opener = opener # type: ignore[assignment] + with pytest.raises(NegativeAckError) as ei: + await dest.send(_REPEATED_ID) + assert ei.value.permanent is True + assert ei.value.code == "bad-body" + assert opener.requests == [] + assert "repeats a key" in str(ei.value) # the peek's reason reaches the operator + assert PHI_CANARY not in str(ei.value) and "first-id" not in str(ei.value) + + +@pytest.mark.parametrize( + "body", + [ + _REPEATED_ID, + '{"resourceType": "Patient", "id": ' + PHI_CANARY, # not JSON + '["' + PHI_CANARY + '"]', # not an object + "", # FHIR-XML + ], + ids=["repeated-key", "not-json", "not-an-object", "xml"], +) +async def test_the_bad_body_text_carries_nothing_of_the_body(body: str) -> None: + # _peek puts the FhirPeekError's text into the refusal, so every refusal the peek can raise is + # pinned here as free of the body (CLAUDE.md section 9). + dest = _dest(interaction="create") + dest._opener = _FakeOpener() # type: ignore[assignment] + with pytest.raises(NegativeAckError) as ei: + await dest.send(body) + assert ei.value.code == "bad-body" + assert PHI_CANARY not in str(ei.value) and "first-id" not in str(ei.value) + + +async def test_an_outgoing_body_with_one_of_each_key_is_sent() -> None: + # Control for the refusal above: the same resource with a single id goes out. + dest = _dest(interaction="update") + opener = _FakeOpener(body=_transaction_response("200 OK")) + dest._opener = opener # type: ignore[assignment] + await dest.send('{"resourceType": "Patient", "id": "first-id", "active": true}') + assert len(opener.requests) == 1 + + +def _reply_with(response: str, *, meta: str = "{}") -> bytes: + """A transaction-response written by hand, since json.dumps cannot repeat a key.""" + return ( + '{"resourceType": "Bundle", "type": "transaction-response", "meta": ' + + meta + + ', "entry": [{"response": ' + + response + + "}]}" + ).encode() + + +@pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"]) +@pytest.mark.parametrize( + "reply", + [ + # Keeping the last key, as json does, reads the 404 and dead-letters; a server keeping + # the first meant 200. The readings disagree, so it dead-letters either way round. + _reply_with('{"status":"200 OK","status":"404 Not Found"}'), + # json alone reads 200 here and delivers, while a first-key reader sees the 404. + _reply_with('{"status": "404 Not Found", "status": "200 OK"}'), + # A disagreement in a value the delivery takes is enough, whatever the capture mode. + _reply_with( + '{"status": "200 OK", "etag": "W/\\"1\\"", "etag": "W/\\"' + PHI_CANARY + '\\""}' + ), + _reply_with('{"status": "200 OK", "location": "Patient/a", "location": "Patient/b"}'), + _reply_with( + '{"status": "200 OK", "lastModified": "2026-01-01T00:00:00Z", ' + '"lastModified": "2026-01-02T00:00:00Z"}' + ), + # The outcome's severity sets the capture outcome, which is part of the judgment. + _reply_with( + '{"status": "200 OK", "outcome": {"resourceType": "OperationOutcome", "issue": ' + '[{"severity": "error", "severity": "information"}]}}' + ), + # A repeated resourceType changes the entry shape the reply is read through. + b'{"resourceType": "OperationOutcome", "resourceType": "Bundle", ' + b'"entry": [{"response": {"status": "404 Not Found"}}]}', + # Two permanent rejects with different codes are two judgments. + _reply_with('{"status": "404 Not Found", "status": "400 Bad Request"}'), + # A transient failure against a permanent one. + _reply_with('{"status": "503 Service Unavailable", "status": "404 Not Found"}'), + ], + ids=[ + "measured-200-then-404", + "404-then-200", + "etag", + "location", + "last-modified", + "outcome-severity", + "resource-type", + "two-reject-codes", + "transient-against-reject", + ], +) +async def test_a_wrapped_update_reply_whose_readings_disagree_is_dead_lettered( + reply: bytes, capture: bool +) -> None: + # Vault BACKLOG #2812: the reply is read keeping the first and keeping the last value of each + # key. When the two reach different judgments, or differ in a value the delivery takes, the + # update fails closed, like a reply over the byte bound (#2180), and is never recorded as + # delivered. + dest = _dest(interaction="update", capture_response=capture) + dest._opener = _FakeOpener(body=reply) # type: ignore[assignment] + with pytest.raises(NegativeAckError) as ei: + await dest.send(UPDATE_1965) + assert ei.value.permanent is True + assert ei.value.code == "reply-repeats-a-key" + text = str(ei.value) + for word in ("status", "etag", "Not Found", "Patient/", "2026-", PHI_CANARY): + assert word not in text + + +@pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"]) +@pytest.mark.parametrize( + "reply", + [ + # A repeat nothing reads: both readings are a 200. + _reply_with('{"status": "200 OK"}', meta='{"tag": 1, "tag": "' + PHI_CANARY + '"}'), + _reply_with('{"status": "200 OK", "status": "200 OK"}'), + # Two successes whose text differs reach one judgment: delivered. + _reply_with('{"status": "200 OK", "status": "201 Created"}'), + b'{"resourceType": "OperationOutcome", "issue": [{"severity": "information", ' + b'"severity": "information"}]}', + ], + ids=["in-meta", "equal-values", "two-successes", "not-a-transaction-response"], +) +async def test_a_wrapped_update_reply_whose_readings_agree_is_delivered( + reply: bytes, capture: bool +) -> None: + dest = _dest(interaction="update", capture_response=capture) + dest._opener = _FakeOpener(body=reply) # type: ignore[assignment] + resp = await dest.send(UPDATE_1965) + if capture: + assert isinstance(resp, DeliveryResponse) and resp.outcome == "accepted" + else: + assert resp is None + + +@pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"]) +@pytest.mark.parametrize( + "reply", + [ + # First and last agree on 200; the middle 404 is a third reading they would never see. + _reply_with('{"status": "200 OK", "status": "404 Not Found", "status": "200 OK"}'), + # First and last disagree already. + _reply_with('{"status": "200 OK", "status": "200 OK", "status": "404 Not Found"}'), + # A, B, A inside meta refuses too: the fail-closed direction, accepted. + _reply_with('{"status": "200 OK"}', meta='{"tag": "a", "tag": "b", "tag": "a"}'), + ], + ids=["200-404-200", "200-200-404", "a-b-a-in-meta"], +) +async def test_a_key_repeated_three_times_with_a_third_value_is_dead_lettered( + reply: bytes, capture: bool +) -> None: + dest = _dest(interaction="update", capture_response=capture) + dest._opener = _FakeOpener(body=reply) # type: ignore[assignment] + with pytest.raises(NegativeAckError) as ei: + await dest.send(UPDATE_1965) + assert ei.value.permanent is True and ei.value.code == "reply-repeats-a-key" + + +@pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"]) +async def test_a_key_repeated_three_times_with_one_value_is_delivered(capture: bool) -> None: + reply = _reply_with('{"status": "200 OK", "status": "200 OK", "status": "200 OK"}') + dest = _dest(interaction="update", capture_response=capture) + dest._opener = _FakeOpener(body=reply) # type: ignore[assignment] + resp = await dest.send(UPDATE_1965) + if capture: + assert isinstance(resp, DeliveryResponse) and resp.outcome == "accepted" + else: + assert resp is None + + +@pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"]) +async def test_two_transient_readings_retry_as_the_shared_classification_says( + capture: bool, +) -> None: + # 503 and 500 both classify as transient (_classify_status), so the readings agree and the + # update is retried, not refused. + reply = _reply_with( + '{"status": "503 Service Unavailable", "status": "500 Internal Server Error"}' + ) + dest = _dest(interaction="update", capture_response=capture) + dest._opener = _FakeOpener(body=reply) # type: ignore[assignment] + with pytest.raises(DeliveryError) as ei: + await dest.send(UPDATE_1965) + assert not isinstance(ei.value, NegativeAckError) + + +async def test_a_reply_the_keep_first_read_cannot_take_is_dead_lettered( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The hook lowers json's depth limit a little, so a reply in that band parses plain but not + # keeping the first key. It can no longer be shown to read one way, so it fails closed. The + # trigger is a raised RecursionError, since the real depth is a property of the runner. + import messagefoundry.transports.fhir as fhir_module + + def _too_deep(pairs: list[tuple[str, Any]]) -> dict[str, Any]: + raise RecursionError("simulated deep nesting") + + monkeypatch.setattr(fhir_module, "_keep_first_key", _too_deep) + dest = _dest(interaction="update") + reply = _reply_with('{"status": "404 Not Found"}', meta='{"tag": 1, "tag": 1}') + dest._opener = _FakeOpener(body=reply) # type: ignore[assignment] + with pytest.raises(NegativeAckError) as ei: + await dest.send(UPDATE_1965) + assert ei.value.permanent is True and ei.value.code == "reply-repeats-a-key" + + +@pytest.mark.parametrize( + ("reply", "code"), + [ + (_reply_with('{"status": "404 Not Found", "status": "404 Not Found"}'), "404"), + (_reply_with('{"status": "400 Bad Request"}', meta='{"tag": 1, "tag": 2}'), "400"), + ], + ids=["equal-values", "repeat-elsewhere"], +) +async def test_a_wrapped_update_reply_whose_readings_agree_on_a_failure_is_classified( + reply: bytes, code: str +) -> None: + # Agreement means judged as on main, so an agreed failure dead-letters on its own status. + dest = _dest(interaction="update") + dest._opener = _FakeOpener(body=reply) # type: ignore[assignment] + with pytest.raises(NegativeAckError) as ei: + await dest.send(UPDATE_1965) + assert ei.value.code == code + + +@pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"]) +async def test_a_wrapped_update_reply_with_one_of_each_key_is_delivered(capture: bool) -> None: + # Control for the refusal above. + dest = _dest(interaction="update", capture_response=capture) + dest._opener = _FakeOpener(body=_reply_with('{"status": "200 OK"}')) # type: ignore[assignment] + resp = await dest.send(UPDATE_1965) + if capture: + assert isinstance(resp, DeliveryResponse) and resp.outcome == "accepted" + else: + assert resp is None + + async def test_reply_that_is_not_a_transaction_response_is_delivered() -> None: # Control arm for the entry checks: a 2xx with no entry is delivered, as any 2xx was before. dest = _dest(interaction="update") diff --git a/tests/test_redaction.py b/tests/test_redaction.py index 6d36a7b8db..fb1f43afaf 100644 --- a/tests/test_redaction.py +++ b/tests/test_redaction.py @@ -1998,6 +1998,34 @@ def test_json_loads_or_refusal_hint_is_content_free( assert marker not in refusal +@pytest.mark.parametrize( + ("build", "hint"), + [ + pytest.param(lambda m: '{"k": 1, "k": "' + m + '"}', "RepeatedKeyError", id="top-level"), + pytest.param( + lambda m: '[{"o": {"k": 1, "k": "' + m + '"}}]', "RepeatedKeyError", id="deep" + ), + # Control: the same key in two different objects is not a repeat. + pytest.param(lambda m: '[{"k": 1}, {"k": "' + m + '"}]', None, id="sibling-objects"), + ], +) +def test_refuse_repeated_keys_hint_is_the_class_name_only( + build: Callable[[str], str], hint: str | None +) -> None: + """vault BACKLOG #2812: the hook refuses a repeated key at any depth, and the hint names neither + the key nor a value.""" + assert {"RepeatedKeyError", "refuse_repeated_keys"} <= set(redaction.__all__) + marker = "SYNTH" + str(random.getrandbits(64)) + value, refusal = redaction.json_loads_or_refusal( + build(marker), object_pairs_hook=redaction.refuse_repeated_keys + ) + assert refusal == hint + if hint is None: + assert value == [{"k": 1}, {"k": marker}] + else: + assert value is None and refusal == redaction.RepeatedKeyError.__name__ + + # --- a labelled MRN in prose (BACKLOG #2079) ---------------------------------- # # A bare ``MRN 12345678`` carried no delimiter, no date and no second capitalized token, so every