From b9c0e26b6a709eca7d59ac9f1bcbb24ed29b299c Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 21:26:59 -0500 Subject: [PATCH 1/8] FhirPeek.parse refuses a FHIR body that repeats a JSON key (BACKLOG #2812) json keeps the last of two equal keys and a server may keep the first. The engine gated and routed on the last while sending the body verbatim. - redaction.py: RepeatedKeyError + refuse_repeated_keys, moved from transports/fhir.py (parsing/ may not import transports/). - FhirPeek.parse refuses a repeated key at any depth. Paths: outgoing write peek (bad-body), wrapped-update reply (unparseable, warning), fhir_lookup reply (FhirLookupError). The bad-body text names the check. - Tests, docs/CONNECTIONS.md, changelog fragment. Closes PR 2244 follow-up item 3. Proposed PR title: FhirPeek refuses a repeated JSON key on every parse path (BACKLOG #2812) Proposed banner: SHIPPED: FhirPeek.parse refuses a JSON object with a repeated key at any depth; single-resource writes dead-letter as bad-body, also closing the PR 2244 follow-up item 3. --- ...fhir-repeated-json-key-refused.security.md | 9 ++ docs/CONNECTIONS.md | 11 ++- messagefoundry/parsing/fhir/peek.py | 18 +++- messagefoundry/redaction.py | 22 ++++- messagefoundry/transports/fhir.py | 37 ++++---- tests/test_dependency_boundaries.py | 5 +- tests/test_fhir_lookup.py | 11 +++ tests/test_fhir_parsing.py | 54 +++++++++++ tests/test_fhir_transport.py | 94 +++++++++++++++++++ tests/test_redaction.py | 28 ++++++ 10 files changed, 265 insertions(+), 24 deletions(-) create mode 100644 changelog.d/2812-fhir-repeated-json-key-refused.security.md 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..ac6e8db980 --- /dev/null +++ b/changelog.d/2812-fhir-repeated-json-key-refused.security.md @@ -0,0 +1,9 @@ +- **`FhirPeek.parse` now refuses a FHIR body in which a JSON object repeats a key, and so does + every path that peeks with it.** 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 counts as delivered with + outcome `unparseable` and a logged warning. 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 reply reads may still keep the last of two equal keys. See + [CONNECTIONS.md](../docs/CONNECTIONS.md), "Interactions". (`vault BACKLOG #2812`) diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index 7027092eef..f325a264b3 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 @@ -2455,7 +2462,9 @@ What a site needs to know: captured. The capture outcome comes from the entry's `response.outcome`. - 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. + logged as a warning, and the 2xx reply counts as delivered. So does a non-empty reply that is not a + readable FHIR resource, including one that repeats a JSON key: its entry status is not read at all, and + the capture outcome is `unparseable`. - 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/messagefoundry/parsing/fhir/peek.py b/messagefoundry/parsing/fhir/peek.py index ebf7a49e44..60634e557a 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,13 @@ 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`). The body is sent verbatim, so it is + refused rather than canonicalised. Only keys equal once JSON-decoded count: a server that + folds case, or cuts a key at an escaped NUL, can still read two keys the peek reads as + distinct. 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 +77,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..9f569efe19 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])") @@ -407,10 +398,12 @@ def _screen_bundle(payload: str, interaction: str, screen: _EntryScreen) -> None 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})" @@ -661,6 +654,14 @@ def _unwrap_transaction_reply( try: peek = FhirPeek.parse(body) except FhirPeekError: + # Among them a reply that repeats a key (vault BACKLOG #2812): the entry status cannot be + # read either way, so, like an unreadable status below, the 2xx counts as delivered. Said + # aloud, because a failed entry hides behind it. An empty reply is send()'s no_reply. + if body.strip(): + logger.warning( + "FHIR transaction-response is not a readable FHIR resource; taking the 2xx reply " + "as delivered without its entry status" + ) 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 @@ -986,8 +987,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..0aca11b18d 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,99 @@ async def test_too_deep_2xx_reply_captures_as_unparseable() -> None: assert isinstance(resp, DeliveryResponse) and resp.outcome == "unparseable" +# --- vault BACKLOG #2812: a repeated JSON key is refused on every FhirPeek path ---------------- + +#: Synthetic. json keeps the last id, so the URL would name the canary 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 + + +@pytest.mark.parametrize( + ("response", "outcome"), + [ + ('{"status": "404 Not Found", "status": "200 OK"}', "unparseable"), + ('{"status": "200 OK", "status": "404 Not Found"}', "unparseable"), + ('{"status": "200 OK"}', "accepted"), # control + ], + ids=["failure-first", "failure-last", "control"], +) +async def test_a_wrapped_update_reply_that_repeats_a_key_captures_as_unparseable( + response: str, outcome: str, caplog: pytest.LogCaptureFixture +) -> None: + # The two orders read differently by which key a reader keeps, so the peek refuses the reply + # and the entry status is not read in either. The 2xx then counts as delivered, as an + # unreadable entry status does, and a warning says so. Before #2812 the failure-last order + # dead-lettered on the 404 and the failure-first order was delivered. + reply = ( + '{"resourceType": "Bundle", "type": "transaction-response", "entry": [{"response": ' + + response + + "}]}" + ) + dest = _dest(interaction="update", capture_response=True) + dest._opener = _FakeOpener(body=reply.encode()) # type: ignore[assignment] + with caplog.at_level("WARNING", logger="messagefoundry.transports.fhir"): + resp = await dest.send(UPDATE_1965) + assert isinstance(resp, DeliveryResponse) and resp.outcome == outcome + assert ("without its entry status" in caplog.text) is (outcome == "unparseable") + + 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 From b1dcfd4d7cc6b2b85a025e662d602fda3c8663c6 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 22:16:31 -0500 Subject: [PATCH 2/8] A wrapped update's reply that repeats a JSON key fails closed (BACKLOG #2812) Reverses the first commit's reply handling, which counted such a 2xx reply as delivered. That turned a reply the #2180 control used to dead-letter, e.g. {"status":"200 OK","status":"404 Not Found"}, into a delivery. - _unwrap_transaction_reply: a reply that repeats a key anywhere is a permanent refusal, code reply-repeats-a-key, in the reply-too-large text shape, and is not recorded as delivered. The refusal is raised outside the handler and names no key or value. The warning is gone. - Tests: both key orders, the measured case, a repeat elsewhere, all-success readings and a non-transaction-response, with capture on and off; a clean-reply control is still delivered. - CONNECTIONS.md update-reply bullet and changelog fragment updated. Proposed PR title: FhirPeek refuses a repeated JSON key on every parse path (BACKLOG #2812) Proposed banner: SHIPPED: FhirPeek.parse refuses a repeated JSON key at any depth; single-resource writes dead-letter as bad-body and a wrapped update's repeated-key reply fails closed as reply-repeats-a-key. Also closes PR 2244 follow-up item 3. --- ...fhir-repeated-json-key-refused.security.md | 9 ++- docs/CONNECTIONS.md | 9 ++- messagefoundry/transports/fhir.py | 36 +++++++-- tests/test_fhir_transport.py | 79 +++++++++++++------ 4 files changed, 96 insertions(+), 37 deletions(-) diff --git a/changelog.d/2812-fhir-repeated-json-key-refused.security.md b/changelog.d/2812-fhir-repeated-json-key-refused.security.md index ac6e8db980..54dc0f00d7 100644 --- a/changelog.d/2812-fhir-repeated-json-key-refused.security.md +++ b/changelog.d/2812-fhir-repeated-json-key-refused.security.md @@ -2,8 +2,9 @@ every path that peeks with it.** 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 counts as delivered with - outcome `unparseable` and a logged warning. 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 reply reads may still keep the last of two equal keys. See + repeats a key fails. A wrapped update's 2xx reply that repeats a key is a permanent refusal, code + `reply-repeats-a-key`, and is not recorded as delivered. 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 f325a264b3..bd7d293bb9 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -2462,9 +2462,12 @@ What a site needs to know: captured. The capture outcome comes from the entry's `response.outcome`. - 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. So does a non-empty reply that is not a - readable FHIR resource, including one that repeats a JSON key: its entry status is not read at all, and - the capture outcome is `unparseable`. + logged as a warning, and the 2xx reply counts as delivered. +- A reply in which a JSON object repeats a key, anywhere in it, is read one way by the engine and maybe + another by the server, so the engine does not judge it. The update is a permanent refusal, code + `reply-repeats-a-key`, in either capture mode, and it is not sent again. It is not recorded as + delivered, even when every reading of the reply is a success, as with a reply over the byte bound + below (vault BACKLOG #2812). - 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/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 9f569efe19..a9b6672fa6 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -396,6 +396,17 @@ 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" + + +def _repeats_a_key(body: str) -> bool: + """Whether parsing ``body`` stops at a repeated JSON key. Run only after :class:`FhirPeek` + refused it, to tell that refusal apart from a body that is not JSON at all.""" + _, hint = json_loads_or_refusal(body.lstrip(_BOM), object_pairs_hook=refuse_repeated_keys) + return hint == RepeatedKeyError.__name__ + + 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 @@ -646,21 +657,30 @@ 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 a permanent refusal, code ``reply-repeats-a-key``, and is not + recorded as delivered (vault BACKLOG #2812). Its entry status could read either way, so, like a + reply over the byte bound, it fails closed; the refusal names neither the key nor a value. + 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) + peek: FhirPeek | None = FhirPeek.parse(body) except FhirPeekError: - # Among them a reply that repeats a key (vault BACKLOG #2812): the entry status cannot be - # read either way, so, like an unreadable status below, the 2xx counts as delivered. Said - # aloud, because a failed entry hides behind it. An empty reply is send()'s no_reply. - if body.strip(): - logger.warning( - "FHIR transaction-response is not a readable FHIR resource; taking the 2xx reply " - "as delivered without its entry status" + peek = None + # Judged outside the handler, so nothing rides on the refusal's __context__ (BACKLOG #2048). + if peek is None: + if _repeats_a_key(body): + # Vault BACKLOG #2812: the reply reads two ways, and json alone would take the last. + # Like a reply over the byte bound (#2180), it is needed and unreadable, so the update + # fails closed rather than counting as delivered. + 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 headers, "unparseable" entries = peek.obj.get("entry") if peek.resource_type == "Bundle" else None diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 0aca11b18d..d19e5f6564 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -790,33 +790,68 @@ async def test_an_outgoing_body_with_one_of_each_key_is_sent() -> None: 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( - ("response", "outcome"), + "reply", [ - ('{"status": "404 Not Found", "status": "200 OK"}', "unparseable"), - ('{"status": "200 OK", "status": "404 Not Found"}', "unparseable"), - ('{"status": "200 OK"}', "accepted"), # control + # Keeping the last key, as json does, reads the 404 and dead-letters, which the #2180 + # control did before #2812; refusing the reply must not turn it into a delivery. + _reply_with('{"status":"200 OK","status":"404 Not Found"}'), + _reply_with('{"status": "404 Not Found", "status": "200 OK"}'), + # A repeat away from the status still makes the reply read two ways. + _reply_with( + '{"status": "400 Bad Request"}', meta='{"tag": 1, "tag": "' + PHI_CANARY + '"}' + ), + _reply_with('{"status": "200 OK", "status": "201 Created"}'), + # Not a transaction-response at all: refused the same way, since it too reads two ways. + b'{"resourceType": "OperationOutcome", "issue": [{"severity": "information", ' + b'"severity": "information"}]}', + ], + ids=[ + "measured-failure-last", + "failure-first", + "elsewhere", + "both-succeed", + "not-a-transaction-response", ], - ids=["failure-first", "failure-last", "control"], ) -async def test_a_wrapped_update_reply_that_repeats_a_key_captures_as_unparseable( - response: str, outcome: str, caplog: pytest.LogCaptureFixture +async def test_a_wrapped_update_reply_that_repeats_a_key_is_dead_lettered( + reply: bytes, capture: bool ) -> None: - # The two orders read differently by which key a reader keeps, so the peek refuses the reply - # and the entry status is not read in either. The 2xx then counts as delivered, as an - # unreadable entry status does, and a warning says so. Before #2812 the failure-last order - # dead-lettered on the 404 and the failure-first order was delivered. - reply = ( - '{"resourceType": "Bundle", "type": "transaction-response", "entry": [{"response": ' - + response - + "}]}" - ) - dest = _dest(interaction="update", capture_response=True) - dest._opener = _FakeOpener(body=reply.encode()) # type: ignore[assignment] - with caplog.at_level("WARNING", logger="messagefoundry.transports.fhir"): - resp = await dest.send(UPDATE_1965) - assert isinstance(resp, DeliveryResponse) and resp.outcome == outcome - assert ("without its entry status" in caplog.text) is (outcome == "unparseable") + # Vault BACKLOG #2812: fails closed, like a reply over the byte bound (#2180). It is never + # recorded as delivered, whichever order the keys come in. + 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", "tag", "Not Found", "Bad Request", PHI_CANARY): + assert word not in text + + +@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: From c90f3e93a4e83508017897756dfe016dc41a83b9 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sat, 17 Oct 2026 11:18:45 -0500 Subject: [PATCH 3/8] A repeated-key update reply is judged when its two readings agree (BACKLOG #2812) Narrows b1dcfd4d per the PR 2356 send-back. The reply is read keeping the first and keeping the last value of each key. It is judged as on main when both agree on everything the unwrap reads: resource type, entry shape, entry status text, outcome issue severities and codes, etag, location, lastModified. Otherwise it is the permanent reply-repeats-a-key refusal, code and text unchanged. So 200-then-404 and 404-then-200 both dead-letter (the second fails open on main), and an agreeing repeat is delivered. A reply the keep-first read cannot parse also fails closed. One shared walk, _entry_response, serves both the comparison and the judgment. The body-side refusal is unchanged. Proposed PR title: FhirPeek refuses a repeated JSON key on every parse path (BACKLOG #2812) Proposed banner: SHIPPED: FhirPeek.parse refuses a repeated JSON key at any depth, so single-resource writes dead-letter as bad-body; a wrapped update's repeated-key reply is judged only when its first-key and last-key readings agree, else reply-repeats-a-key. Also closes PR 2244 follow-up item 3. --- ...fhir-repeated-json-key-refused.security.md | 19 +-- docs/CONNECTIONS.md | 14 +- messagefoundry/parsing/fhir/peek.py | 12 +- messagefoundry/transports/fhir.py | 107 ++++++++++++---- tests/test_fhir_transport.py | 121 +++++++++++++++--- 5 files changed, 210 insertions(+), 63 deletions(-) diff --git a/changelog.d/2812-fhir-repeated-json-key-refused.security.md b/changelog.d/2812-fhir-repeated-json-key-refused.security.md index 54dc0f00d7..f8b5fd0dd8 100644 --- a/changelog.d/2812-fhir-repeated-json-key-refused.security.md +++ b/changelog.d/2812-fhir-repeated-json-key-refused.security.md @@ -1,10 +1,13 @@ -- **`FhirPeek.parse` now refuses a FHIR body in which a JSON object repeats a key, and so does - every path that peeks with it.** 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 a permanent refusal, code - `reply-repeats-a-key`, and is not recorded as delivered. 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 +- **`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 disagree on a field the engine reads from the reply, such as the entry status text or + the ETag, the update is a permanent refusal, code `reply-repeats-a-key`, and is not recorded as + delivered, even where the field goes unused or both readings would deliver. When they agree, it + is judged as before, and the reply is captured as received. 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 bd7d293bb9..84446e5dfe 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -2463,11 +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, anywhere in it, is read one way by the engine and maybe - another by the server, so the engine does not judge it. The update is a permanent refusal, code - `reply-repeats-a-key`, in either capture mode, and it is not sent again. It is not recorded as - delivered, even when every reading of the reply is a success, as with a reply over the byte bound - below (vault BACKLOG #2812). +- 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). If the two readings agree on the resource type, the + entry shape and the entry's `status` text, outcome issue severities and codes, `etag`, `location` and + `lastModified` (for a reply with no entry response, on the outcome of the whole body), the reply is + judged as above. If they disagree in any of those, the update is a permanent refusal, code + `reply-repeats-a-key`, in either capture mode, and it is not sent again. That holds even where both + readings would deliver or retry, for example `200 OK` against `201 Created`, an ETag with capture off, + or two transient statuses, and for a reply only one of the two reads can parse. It is not recorded as + delivered, as with a reply over the byte bound below. - 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/messagefoundry/parsing/fhir/peek.py b/messagefoundry/parsing/fhir/peek.py index 60634e557a..3aeb389421 100644 --- a/messagefoundry/parsing/fhir/peek.py +++ b/messagefoundry/parsing/fhir/peek.py @@ -63,10 +63,14 @@ def parse(cls, raw: str | bytes, *, format: str | None = None) -> FhirPeek: *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`). The body is sent verbatim, so it is - refused rather than canonicalised. Only keys equal once JSON-decoded count: a server that - folds case, or cuts a key at an escaped NUL, can still read two keys the peek reads as - distinct. The refusal names neither the key nor a value.""" + :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) diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index a9b6672fa6..a6afba09c4 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -400,11 +400,72 @@ def _screen_bundle(payload: str, interaction: str, screen: _EntryScreen) -> None _REPLY_REPEATS_A_KEY_CODE = "reply-repeats-a-key" -def _repeats_a_key(body: str) -> bool: - """Whether parsing ``body`` stops at a repeated JSON key. Run only after :class:`FhirPeek` - refused it, to tell that refusal apart from a body that is not JSON at all.""" - _, hint = json_loads_or_refusal(body.lstrip(_BOM), object_pairs_hook=refuse_repeated_keys) - return hint == RepeatedKeyError.__name__ +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.""" + obj: dict[str, Any] = {} + for key, value in pairs: + obj.setdefault(key, value) + return obj + + +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 _reply_reading(obj: dict[str, Any]) -> tuple[object, ...]: + """Everything :func:`_unwrap_transaction_reply` reads from a parsed reply: whether it has an + entry response (which the resource type and entry shape decide), then that response's + ``status``, its outcome's issue severities and codes, ``etag``, ``location`` and + ``lastModified``; for a reply with no entry response, its resource type and the capture + outcome of the whole body. Two readings that give equal tuples are judged identically.""" + response = _entry_response(obj) + if response is None: + return (obj.get("resourceType"), "no entry response", _capture_outcome_of(obj)) + outcome = _as_operation_outcome(response.get("outcome")) + # Of an outcome, the severities set the capture outcome and the codes the retry decision. + issues = ( + None + if outcome is None + else (_issue_field(outcome, "severity"), _issue_field(outcome, "code")) + ) + return ( + "entry response", + response.get("status"), + issues, + *(response.get(field) for field in _ENTRY_RESPONSE_HEADERS.values()), + ) + + +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 agree on everything :func:`_reply_reading` names, or ``None`` when the plain read is not a + JSON object, which is judged ``unparseable`` as before. Raises the permanent + ``reply-repeats-a-key`` refusal when they disagree: the server may have meant either, so, like + a reply over the byte bound (#2180), the update fails closed rather than counting as delivered. + So does a reply the plain read takes but the keep-first read cannot, such as one nested just + past the hook's lower depth limit, since it can no longer be shown to read one way. The refusal + names neither a key nor a value.""" + 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: @@ -657,9 +718,10 @@ 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 a permanent refusal, code ``reply-repeats-a-key``, and is not - recorded as delivered (vault BACKLOG #2812). Its entry status could read either way, so, like a - reply over the byte bound, it fails closed; the refusal names neither the key nor a value. + A reply that repeats a JSON key is read twice, keeping the first and keeping the last value of + each key (vault BACKLOG #2812). When the two readings agree on everything this function reads, + the reply is judged as any other. When they disagree, it is a permanent refusal, code + ``reply-repeats-a-key``, and is not recorded as delivered (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 @@ -667,27 +729,18 @@ def _unwrap_transaction_reply( 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 | None = FhirPeek.parse(body) + obj: dict[str, Any] | None = FhirPeek.parse(body).obj except FhirPeekError: - peek = None + obj = None # Judged outside the handler, so nothing rides on the refusal's __context__ (BACKLOG #2048). - if peek is None: - if _repeats_a_key(body): - # Vault BACKLOG #2812: the reply reads two ways, and json alone would take the last. - # Like a reply over the byte bound (#2180), it is needed and unreadable, so the update - # fails closed rather than counting as delivered. - 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 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) + 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" + response = _entry_response(obj) + if response is None: + return headers, _capture_outcome_of(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 diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index d19e5f6564..7cf4871f9b 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -726,10 +726,11 @@ async def test_too_deep_2xx_reply_captures_as_unparseable() -> None: assert isinstance(resp, DeliveryResponse) and resp.outcome == "unparseable" -# --- vault BACKLOG #2812: a repeated JSON key is refused on every FhirPeek path ---------------- +# --- 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 URL would name the canary while a server keeping the -#: first key would write first-id. +#: 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 + '"}' @@ -805,32 +806,51 @@ def _reply_with(response: str, *, meta: str = "{}") -> bytes: @pytest.mark.parametrize( "reply", [ - # Keeping the last key, as json does, reads the 404 and dead-letters, which the #2180 - # control did before #2812; refusing the reply must not turn it into a delivery. + # 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 repeat away from the status still makes the reply read two ways. + # A disagreement in a captured entry field alone is enough. _reply_with( - '{"status": "400 Bad Request"}', meta='{"tag": 1, "tag": "' + PHI_CANARY + '"}' + '{"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. + _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"}}]}', + # Pinned as the agreement rule reads: the status TEXT must agree, so two successes, or two + # transient failures, that differ are refused too. _reply_with('{"status": "200 OK", "status": "201 Created"}'), - # Not a transaction-response at all: refused the same way, since it too reads two ways. - b'{"resourceType": "OperationOutcome", "issue": [{"severity": "information", ' - b'"severity": "information"}]}', + _reply_with('{"status": "503 Service Unavailable", "status": "500 Internal Server Error"}'), ], ids=[ - "measured-failure-last", - "failure-first", - "elsewhere", - "both-succeed", - "not-a-transaction-response", + "measured-200-then-404", + "404-then-200", + "etag", + "location", + "last-modified", + "outcome-severity", + "resource-type", + "two-successes", + "two-transients", ], ) -async def test_a_wrapped_update_reply_that_repeats_a_key_is_dead_lettered( +async def test_a_wrapped_update_reply_whose_readings_disagree_is_dead_lettered( reply: bytes, capture: bool ) -> None: - # Vault BACKLOG #2812: fails closed, like a reply over the byte bound (#2180). It is never - # recorded as delivered, whichever order the keys come in. + # Vault BACKLOG #2812: the reply is read keeping the first and keeping the last value of each + # key. When the two differ in anything the unwrap reads, 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: @@ -838,10 +858,73 @@ async def test_a_wrapped_update_reply_that_repeats_a_key_is_dead_lettered( assert ei.value.permanent is True assert ei.value.code == "reply-repeats-a-key" text = str(ei.value) - for word in ("status", "tag", "Not Found", "Bad Request", PHI_CANARY): + 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"}'), + b'{"resourceType": "OperationOutcome", "issue": [{"severity": "information", ' + b'"severity": "information"}]}', + ], + ids=["in-meta", "equal-values", "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 + + +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. From 770b0f2a02b2eae77238b596afaffd914f590655 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sat, 17 Oct 2026 11:43:03 -0500 Subject: [PATCH 4/8] A repeated-key update reply agrees when both readings reach one judgment (BACKLOG #2812) Batch 265 Manager decisions on findings 1 and 3. - Agreement now compares the judgment each reading reaches (delivered with its capture outcome, retried, or dead-lettered with its code) plus the values a delivery takes: resource type, whether there is an entry response, and the entry's etag, location, lastModified. So 200 OK against 201 Created delivers and 503 against 500 retries. An etag difference still refuses in either capture mode, and the depth-band case still fails closed. _entry_verdict is the one judgment both the comparison and the unwrap use. - ADR 0095 Amendment B.1 records the reply-repeats-a-key refusal, with its provenance. - CONNECTIONS.md bullet, docstrings and changelog fragment state the judgment rule. Proposed PR title: FhirPeek refuses a repeated JSON key on every parse path (BACKLOG #2812) Proposed banner: SHIPPED: FhirPeek.parse refuses a repeated JSON key at any depth, so single-resource writes dead-letter as bad-body; a wrapped update's repeated-key reply is judged only when its first-key and last-key readings reach the same judgment and agree on etag, location and lastModified, else reply-repeats-a-key (ADR 0095 B.1). Also closes PR 2244 follow-up item 3. --- ...fhir-repeated-json-key-refused.security.md | 17 ++-- docs/CONNECTIONS.md | 16 ++-- ...cle-scheduler-and-credential-fault-stop.md | 21 +++++ messagefoundry/transports/fhir.py | 80 ++++++++++++------- tests/test_fhir_transport.py | 41 +++++++--- 5 files changed, 118 insertions(+), 57 deletions(-) diff --git a/changelog.d/2812-fhir-repeated-json-key-refused.security.md b/changelog.d/2812-fhir-repeated-json-key-refused.security.md index f8b5fd0dd8..313375ac13 100644 --- a/changelog.d/2812-fhir-repeated-json-key-refused.security.md +++ b/changelog.d/2812-fhir-repeated-json-key-refused.security.md @@ -3,11 +3,12 @@ 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 disagree on a field the engine reads from the reply, such as the entry status text or - the ETag, the update is a permanent refusal, code `reply-repeats-a-key`, and is not recorded as - delivered, even where the field goes unused or both readings would deliver. When they agree, it - is judged as before, and the reply is captured as received. 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`) + readings reach different judgments (delivered, retried, or dead-lettered with a code), or differ + in the ETag, Location or Last-Modified the entry carries, the update is a permanent refusal, code + `reply-repeats-a-key`, and is not recorded as delivered. When they agree, it is judged as before, + so `200 OK` against `201 Created` is delivered, and the reply is captured as received. 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", and ADR 0095 Amendment + B.1. (`vault BACKLOG #2812`) diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index 84446e5dfe..8b1196b76b 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -2464,14 +2464,14 @@ What a site needs to know: 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). If the two readings agree on the resource type, the - entry shape and the entry's `status` text, outcome issue severities and codes, `etag`, `location` and - `lastModified` (for a reply with no entry response, on the outcome of the whole body), the reply is - judged as above. If they disagree in any of those, the update is a permanent refusal, code - `reply-repeats-a-key`, in either capture mode, and it is not sent again. That holds even where both - readings would deliver or retry, for example `200 OK` against `201 Created`, an ETag with capture off, - or two transient statuses, and for a reply only one of the two reads can parse. It is not recorded as - delivered, as with a reply over the byte bound below. + and once keeping the last (vault BACKLOG #2812). If both readings reach the same judgment (delivered + with one capture outcome, retried, or dead-lettered with one code) and agree on the resource type, + whether there is an entry response, and the entry's `etag`, `location` and `lastModified`, the reply + is judged as above. So `200 OK` against `201 Created` is delivered, and two statuses that both + classify as transient are retried. Otherwise the update is a permanent refusal, code + `reply-repeats-a-key`, in either capture mode, and it is not sent again. That includes an ETag that + differs with capture off, and a reply only one of the two reads can parse. It is not recorded as + delivered, as with a reply over the byte bound below. ADR 0095 Amendment B.1 records the decision. - 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..cfc3757c3b 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 @@ -405,6 +405,27 @@ the declared length counts as cut short, so where the body is needed that messag once rather than being sent again. A read that times out, or one that a connection reset cuts off, part-way through a 2xx body is still retried, and sends the request again. +**A 2xx reply to a wrapped FHIR update that repeats a JSON key, and reads two ways, is refused +(2026-10-17, vault BACKLOG #2812).** 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, 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. 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.** It is raised in +`transports/fhir.py` (`_agreed_reading`), not by `read_2xx_reply_text`. Tests: +`tests/test_fhir_transport.py`, the section headed vault BACKLOG #2812. + **A 407 on a bypassed request dead-letters each message, and does not stop the lane.** A 407 on a request that went direct reaches each send's status classification, which makes it a permanent reject of that one message, so a queue meeting it drains to dead-letter one row at a time. An MD5 or SHA-256 diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index a6afba09c4..f4bac7ff6f 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -418,28 +418,51 @@ def _entry_response(obj: dict[str, Any]) -> dict[str, Any] | None: return response if isinstance(response, dict) else None -def _reply_reading(obj: dict[str, Any]) -> tuple[object, ...]: - """Everything :func:`_unwrap_transaction_reply` reads from a parsed reply: whether it has an - entry response (which the resource type and entry shape decide), then that response's - ``status``, its outcome's issue severities and codes, ``etag``, ``location`` and - ``lastModified``; for a reply with no entry response, its resource type and the capture - outcome of the whole body. Two readings that give equal tuples are judged identically.""" +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 (obj.get("resourceType"), "no entry response", _capture_outcome_of(obj)) + return _capture_outcome_of(obj) outcome = _as_operation_outcome(response.get("outcome")) - # Of an outcome, the severities set the capture outcome and the codes the retry decision. - issues = ( - None - if outcome is None - else (_issue_field(outcome, "severity"), _issue_field(outcome, "code")) + 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: the resource type, whether there is an entry response, and the entry's + ``etag``, ``location`` and ``lastModified``. The judgment is a delivery with its capture + outcome, a retry, or a permanent refusal with its code, so two readings that differ only in a + status's text, such as ``200 OK`` and ``201 Created``, or ``503`` and ``500``, agree. Two + readings that give equal tuples are judged identically.""" + 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) ) - return ( - "entry response", - response.get("status"), - issues, - *(response.get(field) for field in _ENTRY_RESPONSE_HEADERS.values()), + 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: @@ -719,9 +742,10 @@ def _unwrap_transaction_reply( ``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). When the two readings agree on everything this function reads, - the reply is judged as any other. When they disagree, it is a permanent refusal, code - ``reply-repeats-a-key``, and is not recorded as delivered (see :func:`_agreed_reading`). + each key (vault BACKLOG #2812). When the two readings reach the same judgment (deliver, retry, + or dead-letter with one code) and agree on the values a delivery takes, the reply is judged as + any other. Otherwise it is a permanent refusal, code ``reply-repeats-a-key``, and is not + recorded as delivered (see :func:`_agreed_reading`; recorded in ADR 0095 Amendment B.1). 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 @@ -738,21 +762,17 @@ def _unwrap_transaction_reply( 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, _capture_outcome_of(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: + 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() @@ -768,7 +788,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): diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 7cf4871f9b..7d83d9dcf5 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -811,7 +811,7 @@ def _reply_with(response: str, *, meta: str = "{}") -> bytes: _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 captured entry field alone is enough. + # 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 + '\\""}' ), @@ -820,7 +820,7 @@ def _reply_with(response: str, *, meta: str = "{}") -> bytes: '{"status": "200 OK", "lastModified": "2026-01-01T00:00:00Z", ' '"lastModified": "2026-01-02T00:00:00Z"}' ), - # The outcome's severity sets the capture outcome. + # 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"}]}}' @@ -828,10 +828,10 @@ def _reply_with(response: str, *, meta: str = "{}") -> bytes: # A repeated resourceType changes the entry shape the reply is read through. b'{"resourceType": "OperationOutcome", "resourceType": "Bundle", ' b'"entry": [{"response": {"status": "404 Not Found"}}]}', - # Pinned as the agreement rule reads: the status TEXT must agree, so two successes, or two - # transient failures, that differ are refused too. - _reply_with('{"status": "200 OK", "status": "201 Created"}'), - _reply_with('{"status": "503 Service Unavailable", "status": "500 Internal Server Error"}'), + # 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", @@ -841,16 +841,17 @@ def _reply_with(response: str, *, meta: str = "{}") -> bytes: "last-modified", "outcome-severity", "resource-type", - "two-successes", - "two-transients", + "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 differ in anything the unwrap reads, the update fails closed, like a reply - # over the byte bound (#2180), and is never recorded as delivered. + # 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: @@ -869,10 +870,12 @@ async def test_a_wrapped_update_reply_whose_readings_disagree_is_dead_lettered( # 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", "not-a-transaction-response"], + 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 @@ -886,6 +889,22 @@ async def test_a_wrapped_update_reply_whose_readings_agree_is_delivered( 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: From 696d1571a172755555316c7393b933d437f412c0 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sat, 17 Oct 2026 12:10:26 -0500 Subject: [PATCH 5/8] The repeated-key reply rule gets its own ADR 0095 B.1 entry (BACKLOG #2812) Review repair on 770b0f2a. The 2026-10-17 decision sat inside the batch 250 B.1 text, whose banner names #2989, so it read as theirs. It now has its own dated entry at the end of B.1, with its own banner, provenance and costs. CONNECTIONS.md, the changelog fragment and the docstrings link to it rather than restating the agreement rule (SDS-3.5). No behaviour change. Proposed PR title: FhirPeek refuses a repeated JSON key on every parse path (BACKLOG #2812) Proposed banner: SHIPPED: FhirPeek.parse refuses a repeated JSON key at any depth, so single-resource writes dead-letter as bad-body; a wrapped update's repeated-key reply is judged only when its first-key and last-key readings reach the same judgment and agree on etag, location and lastModified, else reply-repeats-a-key (ADR 0095 B.1). Also closes PR 2244 follow-up item 3. --- ...fhir-repeated-json-key-refused.security.md | 16 +++--- docs/CONNECTIONS.md | 15 +++-- ...cle-scheduler-and-credential-fault-stop.md | 56 ++++++++++++------- messagefoundry/transports/fhir.py | 25 +++------ 4 files changed, 58 insertions(+), 54 deletions(-) diff --git a/changelog.d/2812-fhir-repeated-json-key-refused.security.md b/changelog.d/2812-fhir-repeated-json-key-refused.security.md index 313375ac13..3682b2438a 100644 --- a/changelog.d/2812-fhir-repeated-json-key-refused.security.md +++ b/changelog.d/2812-fhir-repeated-json-key-refused.security.md @@ -3,12 +3,10 @@ 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 reach different judgments (delivered, retried, or dead-lettered with a code), or differ - in the ETag, Location or Last-Modified the entry carries, the update is a permanent refusal, code - `reply-repeats-a-key`, and is not recorded as delivered. When they agree, it is judged as before, - so `200 OK` against `201 Created` is delivered, and the reply is captured as received. 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", and ADR 0095 Amendment - B.1. (`vault BACKLOG #2812`) + 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 8b1196b76b..9e93ccaeee 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -2464,14 +2464,13 @@ What a site needs to know: 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). If both readings reach the same judgment (delivered - with one capture outcome, retried, or dead-lettered with one code) and agree on the resource type, - whether there is an entry response, and the entry's `etag`, `location` and `lastModified`, the reply - is judged as above. So `200 OK` against `201 Created` is delivered, and two statuses that both - classify as transient are retried. Otherwise the update is a permanent refusal, code - `reply-repeats-a-key`, in either capture mode, and it is not sent again. That includes an ETag that - differs with capture off, and a reply only one of the two reads can parse. It is not recorded as - delivered, as with a reply over the byte bound below. ADR 0095 Amendment B.1 records the decision. + and once keeping the last (vault BACKLOG #2812). When the two readings reach the same judgment and + agree on what a delivery takes, 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 cfc3757c3b..a43a7771b8 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 @@ -405,27 +405,6 @@ the declared length counts as cut short, so where the body is needed that messag once rather than being sent again. A read that times out, or one that a connection reset cuts off, part-way through a 2xx body is still retried, and sends the request again. -**A 2xx reply to a wrapped FHIR update that repeats a JSON key, and reads two ways, is refused -(2026-10-17, vault BACKLOG #2812).** 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, 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. 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.** It is raised in -`transports/fhir.py` (`_agreed_reading`), not by `read_2xx_reply_text`. Tests: -`tests/test_fhir_transport.py`, the section headed vault BACKLOG #2812. - **A 407 on a bypassed request dead-letters each message, and does not stop the lane.** A 407 on a request that went direct reaches each send's status classification, which makes it a permanent reject of that one message, so a queue meeting it drains to dead-letter one row at a time. An MD5 or SHA-256 @@ -434,3 +413,38 @@ 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. + +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/transports/fhir.py b/messagefoundry/transports/fhir.py index f4bac7ff6f..4b7d0ee2ff 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -443,11 +443,8 @@ def _entry_verdict(obj: dict[str, Any]) -> DeliveryError | str: 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: the resource type, whether there is an entry response, and the entry's - ``etag``, ``location`` and ``lastModified``. The judgment is a delivery with its capture - outcome, a retry, or a permanent refusal with its code, so two readings that differ only in a - status's text, such as ``200 OK`` and ``201 Created``, or ``503`` and ``500``, agree. Two - readings that give equal tuples are judged identically.""" + 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, ...] = ( ( @@ -468,13 +465,11 @@ def _reply_reading(obj: dict[str, Any]) -> tuple[object, ...]: 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 agree on everything :func:`_reply_reading` names, or ``None`` when the plain read is not a - JSON object, which is judged ``unparseable`` as before. Raises the permanent - ``reply-repeats-a-key`` refusal when they disagree: the server may have meant either, so, like - a reply over the byte bound (#2180), the update fails closed rather than counting as delivered. - So does a reply the plain read takes but the keep-first read cannot, such as one nested just - past the hook's lower depth limit, since it can no longer be shown to read one way. The refusal - names neither a key nor a value.""" + 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) @@ -742,10 +737,8 @@ def _unwrap_transaction_reply( ``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). When the two readings reach the same judgment (deliver, retry, - or dead-letter with one code) and agree on the values a delivery takes, the reply is judged as - any other. Otherwise it is a permanent refusal, code ``reply-repeats-a-key``, and is not - recorded as delivered (see :func:`_agreed_reading`; recorded in ADR 0095 Amendment B.1). + 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 From cc9d9d0c9aed3779722af633b3a13d2b748be832 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sat, 17 Oct 2026 12:39:12 -0500 Subject: [PATCH 6/8] A key repeated three or more times must not hide a third value (BACKLOG #2812) Batch 265 Manager decision on the review-only finding against 696d1571. Comparing the first-key and last-key readings never saw a middle occurrence, so {"status":"200 OK","status":"404 Not Found", "status":"200 OK"} was delivered. The keep-first object_pairs_hook now also refuses, in the same pass, any occurrence equal to neither its key's first nor last value in that object, at any depth. That makes the reply the permanent reply-repeats-a-key refusal. Two occurrences behave as before. A,B,A inside meta refuses too (fail-closed). ADR 0095 B.1 (2026-10-17 entry) and the CONNECTIONS.md bullet each gain one sentence. Proposed PR title: FhirPeek refuses a repeated JSON key on every parse path (BACKLOG #2812) Proposed banner: SHIPPED: FhirPeek.parse refuses a repeated JSON key at any depth, so single-resource writes dead-letter as bad-body; a wrapped update's repeated-key reply is judged only when its first-key and last-key readings reach the same judgment, agree on etag, location and lastModified, and no occurrence differs from both, else reply-repeats-a-key (ADR 0095 B.1). Also closes PR 2244 follow-up item 3. --- docs/CONNECTIONS.md | 3 +- ...cle-scheduler-and-credential-fault-stop.md | 5 ++- messagefoundry/transports/fhir.py | 20 ++++++++--- tests/test_fhir_transport.py | 35 +++++++++++++++++++ 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index 9e93ccaeee..5ae8ad970e 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -2465,7 +2465,8 @@ What a site needs to know: 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, the reply is judged as above. Otherwise the update is a permanent + 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 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 a43a7771b8..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 @@ -430,7 +430,10 @@ resource type, whether there is an entry response, and the entry's `etag`, `loca 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. +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 diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 4b7d0ee2ff..f306f81f61 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -400,13 +400,25 @@ def _screen_bundle(payload: str, interaction: str, screen: _EntryScreen) -> None _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.""" - obj: dict[str, Any] = {} + 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: - obj.setdefault(key, value) - return obj + 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: diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 7d83d9dcf5..70b0f9b9e6 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -889,6 +889,41 @@ async def test_a_wrapped_update_reply_whose_readings_agree_is_delivered( 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, From 78dd64a55d2cb215e1a996ca16af7a342d7e0f74 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sat, 17 Oct 2026 12:58:30 -0500 Subject: [PATCH 7/8] The occurrence check sees both readings of a nested value (BACKLOG #2812) Repair after the review-only pass on cc9d9d0c. Three correctness defects against the occurrence rule: - A middle occurrence was compared by its keep-first reading only, so a middle object that read 404 under keep-last passed while its mirror was refused. The keep-first object now carries its keep-last reading (_BothReadings), and equality checks both at every depth. - Every pair was compared, so a NaN (NaN != NaN) refused a key seen twice or a lone key beside a repeat. Only keys seen three or more times are checked now, and the same object counts as equal. - 1 and true compared equal; a value now must also match in type. The ADR 0095 B.1 2026-10-17 entry no longer says the rule fails closed only on disagreement for three or more occurrences, and lists that cost (200/201/200 and 503/500/503 refuse). Tests pin each case. Proposed PR title: FhirPeek refuses a repeated JSON key on every parse path (BACKLOG #2812) Proposed banner: SHIPPED: FhirPeek.parse refuses a repeated JSON key at any depth, so single-resource writes dead-letter as bad-body; a wrapped update's repeated-key reply is judged only when its first-key and last-key readings reach the same judgment, agree on etag, location and lastModified, and no occurrence differs from both, else reply-repeats-a-key (ADR 0095 B.1). Also closes PR 2244 follow-up item 3. --- ...cle-scheduler-and-credential-fault-stop.md | 16 ++++-- messagefoundry/transports/fhir.py | 52 +++++++++++++++---- tests/test_fhir_transport.py | 46 ++++++++++++++-- 3 files changed, 96 insertions(+), 18 deletions(-) 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 de28634f2f..04a1859b8d 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 @@ -435,10 +435,12 @@ a key occurs three or more times in one object, at any depth, and some occurrenc 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. +With two occurrences of a key 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. With three or more, the +occurrence rule compares values rather than judgments, so it also refuses some replies every +reading would deliver or retry; that cost is listed below and was accepted as the fail-closed +direction. 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 @@ -446,7 +448,11 @@ 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. +An ETag that differs with capture off refuses, though nothing reads it in that mode. A key that +occurs three or more times with a third value refuses wherever it is, even in `meta`, and even +when every reading reaches one judgment: `200 OK`, `201 Created`, `200 OK` is a permanent refusal +of an update that applied, and `503`, `500`, `503` is a permanent refusal where two occurrences +would be retried. 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 diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index f306f81f61..4fb0e602d0 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -404,21 +404,52 @@ class _ThirdReading(ValueError): """A key occurs with a value equal to neither its first nor its last. Carries no key or value.""" +class _BothReadings(dict[str, Any]): + """A keep-first JSON object that also carries its keep-last reading (``last``), so that two + occurrences compare equal only when both readings are equal, at every depth below them.""" + + __slots__ = ("last",) + + def __init__(self, first: dict[str, Any], last: dict[str, Any]) -> None: + super().__init__(first) + self.last = last + + def __eq__(self, other: object) -> bool: + if not isinstance(other, dict): + return NotImplemented + other_last = other.last if isinstance(other, _BothReadings) else other + return dict.__eq__(self, other) is True and dict.__eq__(self.last, other_last) is True + + def __ne__(self, other: object) -> bool: + equal = self.__eq__(other) + return equal if equal is NotImplemented else not equal + + +def _same(a: Any, b: Any) -> bool: + """JSON-value equality: the same object (so a NaN equals itself), or equal and of one type + (so ``1`` and ``true`` differ).""" + return a is b or (type(a) is type(b) and a == b) + + 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.""" + last: the reading a server that keeps the first would make. The object it returns carries its + keep-last reading too (:class:`_BothReadings`). Raises :class:`_ThirdReading` when a key occurs + three or more times and some occurrence equals neither its first nor its last value under both + readings, since comparing the first and last readings would never see it.""" first: dict[str, Any] = {} last: dict[str, Any] = {} + count: dict[str, int] = {} for key, value in pairs: first.setdefault(key, value) last[key] = value + count[key] = count.get(key, 0) + 1 if len(first) != len(pairs) and any( - value != first[key] and value != last[key] for key, value in pairs + count[key] >= 3 and not (_same(value, first[key]) or _same(value, last[key])) + for key, value in pairs ): raise _ThirdReading - return first + return _BothReadings(first, last) def _entry_response(obj: dict[str, Any]) -> dict[str, Any] | None: @@ -478,9 +509,10 @@ 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 + JSON object, which is judged ``unparseable`` as before. Otherwise raises the permanent + ``reply-repeats-a-key`` refusal, which names neither a key nor a value: also for a body only + the plain read can parse, and for a key whose third or later occurrence differs from both its + first and last value (:func:`_keep_first_key` refuses that in the same pass). 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. @@ -749,8 +781,8 @@ def _unwrap_transaction_reply( ``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`. + each key (vault BACKLOG #2812), and judged as any other only when the readings agree and no + occurrence differs from both; 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 diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 70b0f9b9e6..0034d7d7c8 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -895,12 +895,36 @@ async def test_a_wrapped_update_reply_whose_readings_agree_is_delivered( [ # 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. + # First and last disagree already (the two-reading comparison refuses it on its own). _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"}'), + _reply_with( + '{"status": "200 OK"}', meta='{"tag": "a", "tag": "' + PHI_CANARY + '", "tag": "a"}' + ), + # The middle response reads 200 keeping the first status and 404 keeping the last; both + # readings of every occurrence count, so this and its mirror are refused alike. + b'{"resourceType": "Bundle", "entry": [{"response": {"status": "200 OK"}, ' + b'"response": {"status": "200 OK", "status": "404 Not Found"}, ' + b'"response": {"status": "200 OK"}}]}', + b'{"resourceType": "Bundle", "entry": [{"response": {"status": "200 OK"}, ' + b'"response": {"status": "404 Not Found", "status": "200 OK"}, ' + b'"response": {"status": "200 OK"}}]}', + # A number and a boolean are different JSON values, though Python calls 1 == True. + _reply_with('{"status": "200 OK"}', meta='{"tag": 1, "tag": true, "tag": 1}'), + # Pinned as the occurrence rule reads: values, not judgments, so these are refused too. + _reply_with('{"status": "200 OK", "status": "201 Created", "status": "200 OK"}'), + _reply_with('{"status": "503", "status": "500", "status": "503"}'), + ], + ids=[ + "200-404-200", + "200-200-404", + "a-b-a-in-meta", + "middle-reads-404-last", + "middle-reads-404-first", + "number-boolean-number", + "200-201-200", + "503-500-503", ], - 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 @@ -910,6 +934,22 @@ async def test_a_key_repeated_three_times_with_a_third_value_is_dead_lettered( 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" + for word in ("tag", "status", "Not Found", PHI_CANARY): + assert word not in str(ei.value) + + +@pytest.mark.parametrize( + "meta", + ['{"x": NaN, "x": NaN}', '{"x": NaN, "tag": 1, "tag": 1}'], + ids=["nan-twice", "nan-beside-a-repeat"], +) +async def test_a_nan_beside_or_in_a_two_occurrence_repeat_behaves_as_before(meta: str) -> None: + # NaN != NaN, so a value check that walked every pair would refuse these; two occurrences + # behave as before, and a lone key is never a third reading. + dest = _dest(interaction="update", capture_response=True) + dest._opener = _FakeOpener(body=_reply_with('{"status": "200 OK"}', meta=meta)) # type: ignore[assignment] + resp = await dest.send(UPDATE_1965) + assert isinstance(resp, DeliveryResponse) and resp.outcome == "accepted" @pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"]) From e94b9cdd0ba9532638b3727850a968674d7b9a6c Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sat, 17 Oct 2026 13:11:53 -0500 Subject: [PATCH 8/8] Revert "The occurrence check sees both readings of a nested value (BACKLOG #2812)" This reverts commit 78dd64a55d2cb215e1a996ca16af7a342d7e0f74. --- ...cle-scheduler-and-credential-fault-stop.md | 16 ++---- messagefoundry/transports/fhir.py | 52 ++++--------------- tests/test_fhir_transport.py | 46 ++-------------- 3 files changed, 18 insertions(+), 96 deletions(-) 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 04a1859b8d..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 @@ -435,12 +435,10 @@ a key occurs three or more times in one object, at any depth, and some occurrenc 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`). -With two occurrences of a key 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. With three or more, the -occurrence rule compares values rather than judgments, so it also refuses some replies every -reading would deliver or retry; that cost is listed below and was accepted as the fail-closed -direction. On `main` before this, `404 Not Found` then `200 OK` was delivered on the last key. +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 @@ -448,11 +446,7 @@ 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. A key that -occurs three or more times with a third value refuses wherever it is, even in `meta`, and even -when every reading reaches one judgment: `200 OK`, `201 Created`, `200 OK` is a permanent refusal -of an update that applied, and `503`, `500`, `503` is a permanent refusal where two occurrences -would be retried. +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 diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index 4fb0e602d0..f306f81f61 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -404,52 +404,21 @@ class _ThirdReading(ValueError): """A key occurs with a value equal to neither its first nor its last. Carries no key or value.""" -class _BothReadings(dict[str, Any]): - """A keep-first JSON object that also carries its keep-last reading (``last``), so that two - occurrences compare equal only when both readings are equal, at every depth below them.""" - - __slots__ = ("last",) - - def __init__(self, first: dict[str, Any], last: dict[str, Any]) -> None: - super().__init__(first) - self.last = last - - def __eq__(self, other: object) -> bool: - if not isinstance(other, dict): - return NotImplemented - other_last = other.last if isinstance(other, _BothReadings) else other - return dict.__eq__(self, other) is True and dict.__eq__(self.last, other_last) is True - - def __ne__(self, other: object) -> bool: - equal = self.__eq__(other) - return equal if equal is NotImplemented else not equal - - -def _same(a: Any, b: Any) -> bool: - """JSON-value equality: the same object (so a NaN equals itself), or equal and of one type - (so ``1`` and ``true`` differ).""" - return a is b or (type(a) is type(b) and a == b) - - 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. The object it returns carries its - keep-last reading too (:class:`_BothReadings`). Raises :class:`_ThirdReading` when a key occurs - three or more times and some occurrence equals neither its first nor its last value under both - readings, since comparing the first and last readings would never see it.""" + 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] = {} - count: dict[str, int] = {} for key, value in pairs: first.setdefault(key, value) last[key] = value - count[key] = count.get(key, 0) + 1 if len(first) != len(pairs) and any( - count[key] >= 3 and not (_same(value, first[key]) or _same(value, last[key])) - for key, value in pairs + value != first[key] and value != last[key] for key, value in pairs ): raise _ThirdReading - return _BothReadings(first, last) + return first def _entry_response(obj: dict[str, Any]) -> dict[str, Any] | None: @@ -509,10 +478,9 @@ 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 raises the permanent - ``reply-repeats-a-key`` refusal, which names neither a key nor a value: also for a body only - the plain read can parse, and for a key whose third or later occurrence differs from both its - first and last value (:func:`_keep_first_key` refuses that in the same pass). ADR 0095 Amendment B.1 (the 2026-10-17 entry) states the rule and its + 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. @@ -781,8 +749,8 @@ def _unwrap_transaction_reply( ``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 and no - occurrence differs from both; see :func:`_agreed_reading`. + 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 diff --git a/tests/test_fhir_transport.py b/tests/test_fhir_transport.py index 0034d7d7c8..70b0f9b9e6 100644 --- a/tests/test_fhir_transport.py +++ b/tests/test_fhir_transport.py @@ -895,36 +895,12 @@ async def test_a_wrapped_update_reply_whose_readings_agree_is_delivered( [ # 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 (the two-reading comparison refuses it on its own). + # 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": "' + PHI_CANARY + '", "tag": "a"}' - ), - # The middle response reads 200 keeping the first status and 404 keeping the last; both - # readings of every occurrence count, so this and its mirror are refused alike. - b'{"resourceType": "Bundle", "entry": [{"response": {"status": "200 OK"}, ' - b'"response": {"status": "200 OK", "status": "404 Not Found"}, ' - b'"response": {"status": "200 OK"}}]}', - b'{"resourceType": "Bundle", "entry": [{"response": {"status": "200 OK"}, ' - b'"response": {"status": "404 Not Found", "status": "200 OK"}, ' - b'"response": {"status": "200 OK"}}]}', - # A number and a boolean are different JSON values, though Python calls 1 == True. - _reply_with('{"status": "200 OK"}', meta='{"tag": 1, "tag": true, "tag": 1}'), - # Pinned as the occurrence rule reads: values, not judgments, so these are refused too. - _reply_with('{"status": "200 OK", "status": "201 Created", "status": "200 OK"}'), - _reply_with('{"status": "503", "status": "500", "status": "503"}'), - ], - ids=[ - "200-404-200", - "200-200-404", - "a-b-a-in-meta", - "middle-reads-404-last", - "middle-reads-404-first", - "number-boolean-number", - "200-201-200", - "503-500-503", + _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 @@ -934,22 +910,6 @@ async def test_a_key_repeated_three_times_with_a_third_value_is_dead_lettered( 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" - for word in ("tag", "status", "Not Found", PHI_CANARY): - assert word not in str(ei.value) - - -@pytest.mark.parametrize( - "meta", - ['{"x": NaN, "x": NaN}', '{"x": NaN, "tag": 1, "tag": 1}'], - ids=["nan-twice", "nan-beside-a-repeat"], -) -async def test_a_nan_beside_or_in_a_two_occurrence_repeat_behaves_as_before(meta: str) -> None: - # NaN != NaN, so a value check that walked every pair would refuse these; two occurrences - # behave as before, and a lone key is never a third reading. - dest = _dest(interaction="update", capture_response=True) - dest._opener = _FakeOpener(body=_reply_with('{"status": "200 OK"}', meta=meta)) # type: ignore[assignment] - resp = await dest.send(UPDATE_1965) - assert isinstance(resp, DeliveryResponse) and resp.outcome == "accepted" @pytest.mark.parametrize("capture", [True, False], ids=["capture", "no-capture"])