diff --git a/docs/adr/0155-dast-dynamic-security-testing-of-the-running-engine.md b/docs/adr/0155-dast-dynamic-security-testing-of-the-running-engine.md index 008470ba59..f993b211dd 100644 --- a/docs/adr/0155-dast-dynamic-security-testing-of-the-running-engine.md +++ b/docs/adr/0155-dast-dynamic-security-testing-of-the-running-engine.md @@ -308,6 +308,15 @@ that list; see its amendment below. record of what was found. 2. **An alphanumeric MSH-1 gets an unreadable ACK.** The message is accepted, and the ACK echoes the letter separator, so MSA-1 (itself letters) cannot be read back. + + **Amendment 2026-10-16, defect 2 FIXED by BACKLOG #2875.** `build_ack` in + `messagefoundry/mllpcodec.py` now answers an inbound whose MSH-1 or MSH-2 holds a character of + the ACK's own fixed text, or whose delimiters share a character, under `|^~\&`, and the vendored + `tee/mllp.py` does the same. A letter the ACK never writes, such as `D`, is still echoed, because + the ACK reads back under it; `_ack_echo` holds the rule and what it does not cover. The strict + xfail became a plain test. The `alphanumeric-field-separator` policy entry and its discriminator + came out in the same change, so `known_defects` is empty and the run tolerates nothing. The + `letter-field-separator` catalogue case stays on as a regression case. 3. **The raw-TCP and X12 listeners have no frame deadline.** A peer trickling inside `receive_timeout` holds its slot indefinitely. `max_frame_seconds` is MLLP-only. This one is kept out of the run's catalogue and lives only as a strict xfail. diff --git a/messagefoundry/mllpcodec.py b/messagefoundry/mllpcodec.py index d39ea2c79f..4eb2649d22 100644 --- a/messagefoundry/mllpcodec.py +++ b/messagefoundry/mllpcodec.py @@ -11,7 +11,8 @@ across reads or pack several into one. ACKs are built from the inbound MSH (echoing its encoding characters, swapping -sender/receiver, copying the original control id into MSA-2). ``ack_mode`` selects the +sender/receiver, copying the original control id into MSA-2), except where those encoding +characters collide with the ACK's own text (BACKLOG #2875; ``_ack_echo``). ``ack_mode`` selects the MSA-1 code family: ``original`` → AA/AE/AR, ``enhanced`` → CA/CE/CR. **Why this module exists (BACKLOG #1697).** A client may not import the engine's runtime packages @@ -35,6 +36,7 @@ from __future__ import annotations import logging +import string from enum import Enum from typing import TYPE_CHECKING @@ -158,11 +160,42 @@ def _escape_ack_text(text: str, *, field_sep: str, enc: str) -> str: return text.replace(esc, f"{esc}E{esc}").replace(field_sep, f"{esc}F{esc}") +def _roles(field_sep: str, enc: str) -> dict[str, str]: + """Each delimiter character to its role, in MSH-2 order, a short MSH-2 filled from the defaults + as the parser fills it. Where two roles share a character the first wins; one with no role is + data.""" + roles: dict[str, str] = {} + enc = enc[:4] + _DEFAULT_ENC[len(enc[:4]) :] + for char, role in zip(field_sep + enc, "FSRET", strict=True): + roles.setdefault(char, role) + return roles + + +_DEFAULT_ROLES = _roles(_DEFAULT_FIELD_SEP, _DEFAULT_ENC) + _CODES = { AckMode.ORIGINAL: {"AA": "AA", "AE": "AE", "AR": "AR"}, AckMode.ENHANCED: {"AA": "CA", "AE": "CE", "AR": "CR"}, } +#: The characters of the ACK's fixed text: the segment ids, ``ACK``, ``P``, every MSA-1 code, the +#: letters of the ``\E\`` and ``\F\`` escapes :func:`_escape_ack_text` writes, the default MSH-7 +#: (digits and a ``+``/``-`` offset) and the default version ``2.5.1``. An inbound delimiter from +#: this set would split that text when the ACK is read back, so the ACK is built under the defaults +#: instead (BACKLOG #2875). Not covered: a caller's own ``timestamp`` or ``control_id``, and the +#: rest of MSA-3's free text, of which only the escape character and field separator are escaped. +_ACK_LITERAL_CHARS = frozenset( + "MSH" + + "MSA" + + "ACK" + + "P" + + "EF" + + "".join(_CODES[AckMode.ENHANCED].values()) + + "".join(_CODES[AckMode.ORIGINAL].values()) + + string.digits + + "+-." +) + def _ack_echo(peek: Peek | None) -> tuple[str, str, str, str, str, str, str, str]: """The inbound header values an ACK echoes, or their defaults when there is no ``peek``. @@ -170,18 +203,44 @@ def _ack_echo(peek: Peek | None) -> tuple[str, str, str, str, str, str, str, str Returns ``(field_sep, enc, sending_app, sending_fac, receiving_app, receiving_fac, version, control_id)``. Every value is echoed from the (untrusted) inbound message, so CR/LF is stripped to prevent segment injection into the ACK; MSA-3 free text is escaped separately (HL7-3). + + The inbound's MSH-1 and MSH-2 are echoed unless one of them holds a character of the ACK's + fixed text (:data:`_ACK_LITERAL_CHARS`), or two delimiters share a character. Then the defaults + ``|^~\\&`` are returned instead, so the ACK reads back as an ACK (BACKLOG #2875). An echoed + value is then kept only where it holds neither set's escape character and each default delimiter + in it has the same role in the inbound (``C^1`` under MSH-2 ``^AB``); any other is blanked. + Blanked, not escaped: the value is raw text split under the inbound's delimiters, so escaping it + would flatten its structure and re-escape its own escapes (``\\F\\`` names the field separator of + whichever set reads it), and an escaped control id would no longer match the one the store + recorded. Under the inbound's own delimiters the values need neither, because they were split + under them. """ if peek is None: return _DEFAULT_FIELD_SEP, _DEFAULT_ENC, "", "", "", "", "2.5.1", "" + field_sep = peek.field("MSH-1") or _DEFAULT_FIELD_SEP + enc = peek.field("MSH-2") or _DEFAULT_ENC + # The characters an echoed value may not hold; none unless the ACK falls back to the defaults. + unsafe: frozenset[str] = frozenset() + roles = _roles(field_sep, enc) + # Fewer than five roles means two share a character, the parser's default-filled slots included. + if len(roles) < 5 or not _ACK_LITERAL_CHARS.isdisjoint(field_sep + enc): + unsafe = frozenset(c for c, role in _DEFAULT_ROLES.items() if roles.get(c) != role) + unsafe |= {_DEFAULT_ENC[2], enc[2:3] or _DEFAULT_ENC[2]} + field_sep, enc = _DEFAULT_FIELD_SEP, _DEFAULT_ENC + + def echo(value: str | None, default: str = "") -> str: + value = _no_seg_sep(value or "") + return (value if unsafe.isdisjoint(value) else "") or default + return ( - peek.field("MSH-1") or _DEFAULT_FIELD_SEP, - peek.field("MSH-2") or _DEFAULT_ENC, - _no_seg_sep(peek.sending_app or ""), - _no_seg_sep(peek.sending_facility or ""), - _no_seg_sep(peek.receiving_app or ""), - _no_seg_sep(peek.receiving_facility or ""), - _no_seg_sep(peek.version or "2.5.1"), - _no_seg_sep(peek.control_id or ""), + field_sep, + enc, + echo(peek.sending_app), + echo(peek.sending_facility), + echo(peek.receiving_app), + echo(peek.receiving_facility), + echo(peek.version, "2.5.1"), + echo(peek.control_id), ) @@ -208,6 +267,11 @@ def build_ack( the same wall-clock hour occurs twice — so a receiver correlating an acknowledgement against its own UTC-stamped record would mis-order events by an hour. An explicit offset pins the instant. An operator-supplied ``timestamp`` is used verbatim; the caller owns its form. + + The ACK echoes the inbound's MSH-1 and MSH-2, except where one of them holds a character of the + ACK's fixed text; then the ACK, and the NAK this also builds, goes out under ``|^~\\&`` + (BACKLOG #2875; :func:`_ack_echo` holds the rule). MSA-3's escape character and field separator + are escaped under whichever set is used. """ # Imported here, not at module top, so a framing-only client never loads the parsing library. # It was also a cycle once: `parsing` reached `config.models`, which imports AckMode from here. diff --git a/scripts/security/dast-ingress-policy.json b/scripts/security/dast-ingress-policy.json index 782a859e1f..11a6e06c82 100644 --- a/scripts/security/dast-ingress-policy.json +++ b/scripts/security/dast-ingress-policy.json @@ -47,9 +47,7 @@ "min_liveness_probes": 50, "min_log_records_seen": 1 }, - "known_defects": { - "alphanumeric-field-separator": "Engine defect found by this pass on 2026-09-26: a message whose MSH-1 is a letter or digit is accepted, and the ACK echoes that separator, so MSA-1 (AA/AE/AR, themselves letters) cannot be read back by the sender. Pinned by a strict xfail in tests/test_dast_ingress_sweep.py." - }, + "known_defects": {}, "resource_bounds": { "passes": 2, "max_heap_growth_bytes": 4194304, diff --git a/scripts/security/dast_ingress_sweep.py b/scripts/security/dast_ingress_sweep.py index b6f5de87cb..aee4eda32b 100644 --- a/scripts/security/dast_ingress_sweep.py +++ b/scripts/security/dast_ingress_sweep.py @@ -312,19 +312,14 @@ def reference_frames(plane: str, data: bytes, cap: int) -> tuple[list[bytes], bo return frames, False -def _alphanumeric_field_separator(payload: bytes) -> bool: - return payload[:3] == b"MSH" and payload[3:4].isalnum() - - #: Known engine defects, each recognised by a NARROW structural condition on the frames a case sent, #: never by an exception type or a case name alone (a mutation can reproduce one by chance). Which of #: these a run tolerates is the policy's ``known_defects`` list; tests/test_dast_ingress_sweep.py holds #: a strict xfail per entry, so the day a defect is fixed its entry has to come out. Each is matched #: against every frame a case sent and explains each frame it matches. None is a handler fault: the #: one that was (a blank segment, BACKLOG #1594) is fixed, so a handler-fault NAK is never tolerated. -KNOWN_DEFECT_DISCRIMINATORS: dict[str, Callable[[bytes], bool]] = { - "alphanumeric-field-separator": _alphanumeric_field_separator, -} +#: Empty while no defect is open: the alphanumeric field separator was fixed by BACKLOG #2875. +KNOWN_DEFECT_DISCRIMINATORS: dict[str, Callable[[bytes], bool]] = {} #: The only detectors a known defect may silence. A liveness, time, resource or log finding on the #: same case is never tolerated: those would be a SECOND defect riding on the known one. KNOWN_DEFECT_DETECTORS = frozenset({"reply", "count_and_log"}) diff --git a/tee/mllp.py b/tee/mllp.py index 016ce57b45..f827fc3775 100644 --- a/tee/mllp.py +++ b/tee/mllp.py @@ -15,6 +15,7 @@ from __future__ import annotations import re +import string from collections.abc import Iterator from datetime import datetime @@ -39,6 +40,27 @@ _DEFAULT_FIELD_SEP = "|" _DEFAULT_ENC = "^~\\&" +# The characters of the ACK's fixed text: the segment ids, ACK, P, every MSA-1 code (original and +# enhanced), the letters of the engine's MSA-3 escapes (kept equal to the engine), MSH-7's digits and +/- offset, and the default version 2.5.1. An inbound delimiter from +# this set would split that text when the ACK is read back, so the ACK is built under the defaults +# instead (BACKLOG #2875). The engine's messagefoundry/mllpcodec.py holds the same rule, and +# tests/test_tee_mllp.py pins the two equal. +_ACK_LITERAL_CHARS = frozenset("MSHMSAACKP" + "EF" + "AAAEARCACECR" + string.digits + "+-.") + + +def _roles(field_sep: str, enc: str) -> dict[str, str]: + """Each delimiter character to its role, in MSH-2 order, a short MSH-2 filled from the defaults + as the engine's parser fills it. Where two roles share a character the first wins; one with no + role is data. The engine's mllpcodec has the same function.""" + roles: dict[str, str] = {} + enc = enc[:4] + _DEFAULT_ENC[len(enc[:4]) :] + for char, role in zip(field_sep + enc, "FSRET", strict=True): + roles.setdefault(char, role) + return roles + + +_DEFAULT_ROLES = _roles(_DEFAULT_FIELD_SEP, _DEFAULT_ENC) + # HL7 segments are CR-delimited, but tolerant peers emit LF/CRLF too — split on any run of either. _SEG_SPLIT = re.compile(r"[\r\n]+") @@ -110,25 +132,36 @@ def _msh_fields(message: bytes) -> tuple[str, list[str]]: if not first.startswith("MSH") or len(first) < 4: return _DEFAULT_FIELD_SEP, [] field_sep = first[3] - return field_sep, first.split(field_sep) + # Split after MSH-1, not the whole line: a field separator of M, S or H would split "MSH" too. + return field_sep, ["MSH", *first[4:].split(field_sep)] def build_ack(message: bytes, *, code: str = "AA", timestamp: str | None = None) -> bytes: """Build an HL7 acknowledgement for ``message`` (default ``AA`` — the relay always accepts). - Reads the field separator + encoding characters from the inbound MSH (never hardcoded), echoes the - control id into MSA-2, swaps sender/receiver so the ACK routes back the way it came, and sanitizes - every echoed field against segment injection. If ``message`` isn't parseable HL7 it still returns a - generic ``AA`` (the relay always ACKs on receipt). Encoded latin-1 to preserve the inbound's bytes. + Reads the field separator + encoding characters from the inbound MSH, echoes the control id into + MSA-2, swaps sender/receiver so the ACK routes back the way it came, and sanitizes every echoed + field against segment injection. If ``message`` isn't parseable HL7 it still returns a generic + ``AA`` (the relay always ACKs on receipt). Encoded latin-1 to preserve the inbound's bytes. + + The one exception to echoing the inbound's delimiters: where its MSH-1 or MSH-2 holds a + character of the ACK's fixed text (``_ACK_LITERAL_CHARS``), or two delimiters share a character, + the ACK goes out under ``|^~\\&`` so it reads back as an ACK. An echoed value holding either + set's escape character, or a default delimiter in a role it does not have in the inbound, is + then blanked rather than escaped, by the engine's rule (BACKLOG #2875). """ field_sep, parts = _msh_fields(message) enc = _field(parts, 1) or _DEFAULT_ENC - sending_app = _no_seg_sep(_field(parts, 2)) - sending_fac = _no_seg_sep(_field(parts, 3)) - receiving_app = _no_seg_sep(_field(parts, 4)) - receiving_fac = _no_seg_sep(_field(parts, 5)) - control = _no_seg_sep(_field(parts, 9)) - version = _no_seg_sep(_field(parts, 11)) or "2.5.1" + echoed = tuple(_no_seg_sep(_field(parts, index)) for index in (2, 3, 4, 5, 9, 11)) + roles = _roles(field_sep, enc) + # Fewer than five roles means two delimiters share a character, default-filled slots included. + if len(roles) < 5 or not _ACK_LITERAL_CHARS.isdisjoint(field_sep + enc): + unsafe = {c for c, role in _DEFAULT_ROLES.items() if roles.get(c) != role} + unsafe |= {_DEFAULT_ENC[2], enc[2:3] or _DEFAULT_ENC[2]} + field_sep, enc = _DEFAULT_FIELD_SEP, _DEFAULT_ENC + echoed = tuple(v if unsafe.isdisjoint(v) else "" for v in echoed) + sending_app, sending_fac, receiving_app, receiving_fac, control, version = echoed + version = version or "2.5.1" # MSH-7 carries an explicit numeric UTC offset (BACKLOG #1196), matching the engine's own # build_ack. A bare local stamp is ambiguous across a daylight-saving fall-back, where the # same wall-clock hour occurs twice, so a partner correlating this acknowledgement against @@ -174,7 +207,12 @@ def parse_ack(ack: bytes) -> tuple[str | None, str | None]: break for seg in segments: if seg.startswith("MSA"): - parts = seg.split(field_sep) + # Split after the segment id, not the whole line, as _msh_fields does: a field + # separator of M, S or A would split "MSA" too. + if seg[3:4] == field_sep: + parts = ["MSA", *seg[4:].split(field_sep)] + else: + parts = seg.split(field_sep) return (_field(parts, 1) or None), (_field(parts, 3) or None) return None, None diff --git a/tests/test_dast_ingress_sweep.py b/tests/test_dast_ingress_sweep.py index 02e9686f7d..fee0fde301 100644 --- a/tests/test_dast_ingress_sweep.py +++ b/tests/test_dast_ingress_sweep.py @@ -359,6 +359,8 @@ def test_an_unknown_profile_fails_closed() -> None: def test_a_known_defect_never_silences_liveness_time_or_log() -> None: # Judged on the canary path only because it needs no floors; the split happens first either way. + # No engine defect is open (the last, BACKLOG #2875, is fixed), so the policy names a stand-in. + policy = dict(_policy(), known_defects={"stand-in-defect": "A defect this test pretends to."}) receipt: dict[str, Any] = { "canary": "no-reply", "findings": [ @@ -367,12 +369,12 @@ def test_a_known_defect_never_silences_liveness_time_or_log() -> None: "plane": "mllp", "case": "c", "detail": "x", - "known_defect": "alphanumeric-field-separator", + "known_defect": "stand-in-defect", } for d in ("reply", "liveness", "time", "log_body") ], } - evaluate(receipt, _policy()) + evaluate(receipt, policy) tolerated = {f["detector"] for f in receipt["known_defect_findings"]} assert tolerated == {"reply"} @@ -537,7 +539,8 @@ def test_each_known_defect_discriminator_matches_its_catalogue_case() -> None: """A tolerance keyed on a condition no catalogue case carries would tolerate nothing, and one keyed on a condition EVERY case carries would tolerate everything.""" cases = {c.name: c for c in catalogue("ZZTESTSENTINEL", _CAP) if c.plane == "mllp"} - anchors = {"alphanumeric-field-separator": "letter-field-separator"} + # Empty since BACKLOG #2875 fixed the last open defect; an entry names its anchor case here. + anchors: dict[str, str] = {} assert set(anchors) == set(KNOWN_DEFECT_DISCRIMINATORS) == set(_policy()["known_defects"]) for defect, case_name in anchors.items(): hit = KNOWN_DEFECT_DISCRIMINATORS[defect] @@ -573,13 +576,9 @@ async def test_a_blank_segment_that_fails_utf8_decode_gets_the_runners_own_nak() assert not result.findings, result.findings -@pytest.mark.xfail( - strict=True, - reason=( - "ENGINE DEFECT: a message whose MSH-1 is a letter or digit is accepted, and the ACK echoes " - "that separator, so MSA-1 (itself letters) cannot be read back by the sender." - ), -) +# Defect 2, an alphanumeric MSH-1 echoed into an ACK that could not be read back, is FIXED by +# BACKLOG #2875. Its strict xfail became this plain test and its policy entry came out, so the run +# no longer tolerates a reply finding on such a frame. @_LIVE async def test_an_alphanumeric_field_separator_gets_a_readable_reply() -> None: result = await _run_alone(_case("mllp", "letter-field-separator"), _budget()) diff --git a/tests/test_harness_fuzz.py b/tests/test_harness_fuzz.py index 9658175e8a..4bce00e59e 100644 --- a/tests/test_harness_fuzz.py +++ b/tests/test_harness_fuzz.py @@ -46,7 +46,8 @@ from harness.scenarios._core import control_id_of from messagefoundry.api.models import Health, MessageList, MessageSummary from messagefoundry.apiclient import ApiError, EngineClient -from messagefoundry.mllpcodec import frame +from messagefoundry.mllpcodec import build_ack, frame +from messagefoundry.parsing import Peek from messagefoundry.store.store import MessageStatus from tests._harness_engine import REPO, ephemeral_overrides, free_port, serve_harness_config @@ -270,32 +271,112 @@ def mangled(kind: str, endpoints: Endpoints, key: str, *, timeout: float) -> Tra _ALPHANUMERIC_DELIMITERS = ((11, 5686), (13, 1071)) -@pytest.mark.xfail( - strict=True, - reason="engine defect: build_ack (messagefoundry/mllpcodec.py) echoes the inbound's MSH-1/MSH-2 " - "verbatim and writes its literal fields unescaped, so an inbound whose delimiters are letters " - "gets an AA whose MSH-9 and MSA-1 do not read back as ACK/AA under the ACK's own delimiters", -) @pytest.mark.parametrize(("seed", "iteration"), _ALPHANUMERIC_DELIMITERS) def test_an_inbound_with_letter_delimiters_gets_a_readable_ack( server: tuple[str, Endpoints], seed: int, iteration: int ) -> None: + # BACKLOG #2875: build_ack answers such an inbound under |^~\& rather than its own delimiters. _api_url, eps = server case = make_case(seed, iteration) + assert re.match(rb"MSH(\|\^AB\||D)", case.data.lstrip(b"\x0b")), "the case lost its shape" exchange = build_transport("mllp", eps, None, timeout=10.0).send(case.data) - # The engine did answer, in one frame; it is the answer's content that is unreadable. assert exchange.outcome == REPLY and len(exchange.replies) == 1 verdict = invariants.judge(exchange, expected=len(frames(case.data))) assert not verdict.problem, verdict.problem +#: Synthetic inbound headers whose delimiters collide with text the ACK writes itself (BACKLOG +#: #2875). MSH-2 "^AB" is the campaign's first shape; its MSH-1 "D" case also left letters in +#: MSH-2, and "K", a letter of "ACK", stands in for it. The rest split MSH-7's offset or the version +#: "2.5.1" the ACK writes when the inbound has none, or give two delimiters one character. +_COLLIDING_HEADERS = { + "msh2-letters": "MSH|^AB|APP|FAC|RCV|RFC|20260101||ORU^R01|C^1|P|2.5.1\r", + "msh1-letter": "MSHK^~\\&KAPPKFACKRCVKRFCK20260101KKORU^R01KC|1KPK2.5.1\r", + "msh1-in-msh2": "MSH^^APP^FAC^RCV^RFC^20260101^^ORU~R01^C1^P^2.5.1\r", + "msh2-duplicate": "MSH|^~^&|APP|FAC|RCV|RFC|20260101||ORU^R01|C1|P|2.5.1\r", + # "F" is in the \F\ escape that MSA-3 carries, not only in the fixed fields. + "msh1-escape-letter": "MSHF^~\\&FAPPFSACFRCVFRXCF20260101FFORU^R01FC1FPF2.5.1\r", + "msh2-minus": "MSH|-~\\&|APP|FAC|RCV|RFC|20260101||ORU^R01|C1|P|2.5.1\r", + "msh2-plus": "MSH|+~\\&|APP|FAC|RCV|RFC|20260101||ORU^R01|C1|P|2.5.1\r", + "msh2-dot": "MSH|^~\\.|APP|FAC|RCV|RFC|20260101||ORU^R01|C1|P\r", +} + + +@pytest.mark.parametrize("code", ["AA", "AE", "AR"]) +@pytest.mark.parametrize("timestamp", ["20260101000000-0500", "20260101000000+0100"]) +@pytest.mark.parametrize("inbound", _COLLIDING_HEADERS.values(), ids=list(_COLLIDING_HEADERS)) +def test_build_ack_reads_back_when_the_inbound_delimiters_collide_with_its_own_text( + inbound: str, timestamp: str, code: str +) -> None: + ack = build_ack(inbound, code=code, text="bad | data", timestamp=timestamp) + assert ack.startswith("MSH|^~\\&|"), ack + peek = Peek.parse(ack) + assert peek.message_code == "ACK" + assert peek.field("MSA-1") == code + # Down to the subcomponent: a whole-field read would not see a split. + assert peek.field("MSH-7.1.1") == timestamp + assert peek.field("MSH-12.1.1") == "2.5.1" + assert peek.field("MSA-3") == "bad \\F\\ data" + + +#: Inbounds whose MSH-10 holds a default delimiter as data or in another role, or an escape. +_DATA_CONTROL_IDS = { + "pipe-under-letter-field-sep": _COLLIDING_HEADERS["msh1-letter"], # C|1, field separator K + "backslash-under-letter-escape": "MSH|^AB|APP|FAC|RCV|RFC|20260101||ORU^R01|C\\1|P|2.5.1\r", + "caret-as-subcomponent": "MSH|-~\\^|APP|FAC|RCV|RFC|20260101||ORU^R01|C^1|P|2.5.1\r", + # \F\ is "K" under the inbound and "|" under the defaults, so even an escape is blanked. + "escape-sequence": "MSHK^~\\&KAPPKFACKRCVKRFCK20260101KKORU^R01KC\\F\\1KPK2.5.1\r", +} + + +@pytest.mark.parametrize("inbound", _DATA_CONTROL_IDS.values(), ids=list(_DATA_CONTROL_IDS)) +def test_build_ack_blanks_an_echoed_value_a_default_delimiter_would_restructure( + inbound: str, +) -> None: + # Escaping the control id would send one the store never recorded, so it is blanked. + peek = Peek.parse(build_ack(inbound, timestamp="20260101")) + assert peek.control_id is None and peek.field("MSA-2") is None + # The values that hold no default delimiter are still echoed, sender and receiver swapped. + assert [peek.field(f"MSH-{n}") for n in (3, 4, 5, 6)] == ["RCV", "RFC", "APP", "FAC"] + + +@pytest.mark.parametrize(("name", "control_id"), [("msh2-letters", "C^1"), ("msh2-minus", "C1")]) +def test_build_ack_keeps_an_echoed_value_that_reads_the_same_under_the_defaults( + name: str, control_id: str +) -> None: + # Under MSH-2 "^AB", "^" is the component separator in both sets, so "C^1" means the same. + peek = Peek.parse(build_ack(_COLLIDING_HEADERS[name], timestamp="20260101")) + assert (peek.control_id, peek.field("MSA-2")) == (control_id, control_id) + + +def test_build_ack_still_echoes_punctuation_delimiters() -> None: + # The control: delimiters the ACK never writes as text are echoed, as before BACKLOG #2875. + ack = build_ack( + "MSH#@*!%#APP#FAC#RCV#RFC#20260101##ORU@R01#C|1#P#2.5.1\r", timestamp="20260101" + ) + assert ack.startswith("MSH#@*!%#RCV#RFC#APP#FAC#"), ack + peek = Peek.parse(ack) + assert (peek.message_code, peek.field("MSA-1"), peek.field("MSA-2")) == ("ACK", "AA", "C|1") + + +@pytest.mark.parametrize("code", ["AA", "AE", "AR"]) +def test_build_ack_echoes_a_letter_field_separator_its_own_text_never_holds(code: str) -> None: + # MSH-1 "D" splits nothing the ACK writes, so it is echoed and the control id still correlates. + inbound = "MSHD^~\\&DAPPDFACDRCVDRFCD20260101DDORU^R01DC|1DPD2.5.1\r" + ack = build_ack(inbound, code=code, timestamp="20260101000000-0500") + assert ack.startswith("MSHD^~\\&DRCVDRFCDAPPDFACD"), ack + peek = Peek.parse(ack) + assert (peek.message_code, peek.field("MSA-1"), peek.field("MSA-2")) == ("ACK", code, "C|1") + assert peek.field("MSH-7.1.1") == "20260101000000-0500" + + # --- invariants without an engine ---------------------------------------------------------------- def _plain_ack(payload: bytes) -> bytes: """An AA for ``payload`` under the standard delimiters, echoing its control id when that id is - plain text. Not build_ack: that echoes the payload's own delimiters, which is the engine defect - the xfail above pins, and these fakes stand in for an engine without it.""" + plain text. Not build_ack: these fakes stand in for an engine, and their replies must not move + when build_ack's echo rules do.""" cid = control_id_of(payload) or "" if not queryable(cid) or any(ch in cid for ch in "|^~\\&"): cid = "" diff --git a/tests/test_tee_mllp.py b/tests/test_tee_mllp.py index c773a65215..eec247c811 100644 --- a/tests/test_tee_mllp.py +++ b/tests/test_tee_mllp.py @@ -6,6 +6,8 @@ import pytest +from messagefoundry import mllpcodec +from messagefoundry.parsing import Peek from tee import mllp # A synthetic HL7 ADT message (segments CR-delimited). No real PHI. @@ -81,6 +83,12 @@ def test_parse_ack_with_detail() -> None: assert mllp.parse_ack(ack) == ("AR", "rejected reason") +@pytest.mark.parametrize("sep", ["M", "S"]) +def test_parse_ack_reads_a_field_separator_that_is_a_letter_of_msa(sep: str) -> None: + ack = f"MSH{sep}^~\\&{sep}X\rMSA{sep}AE{sep}C1{sep}why\r".encode() + assert mllp.parse_ack(ack) == ("AE", "why") + + def test_parse_ack_unreadable() -> None: assert mllp.parse_ack(b"junk") == (None, None) @@ -96,3 +104,78 @@ def test_segment_injection_is_stripped() -> None: ack = mllp.build_ack(evil).decode("latin-1") # The injected 'PV1|evil' must not appear as its own segment in the ACK. assert "\rPV1|evil" not in ack + + +#: Synthetic inbound headers whose delimiters collide with text the ACK writes itself (BACKLOG +#: #2875): MSH-2 "^AB", MSH-1 "K" (a letter of "ACK"), a component separator that splits MSH-7's +#: offset, and an MSH-10 that holds an escape. +_COLLIDING_HEADERS = { + "msh2-letters": b"MSH|^AB|APP|FAC|RCV|RFC|20260101||ORU^R01|C^1|P|2.5.1\r", + "msh1-letter": b"MSHK^~\\&KAPPKFACKRCVKRFCK20260101KKORU^R01KC|1KPK2.5.1\r", + "msh2-minus": b"MSH|-~\\&|APP|FAC|RCV|RFC|20260101||ORU^R01|C1|P|2.5.1\r", + "msh1-escape": b"MSHK^~\\&KAPPKFACKRCVKRFCK20260101KKORU^R01KC\\F\\1KPK2.5.1\r", +} + + +@pytest.mark.parametrize("code", ["AA", "AE", "AR"]) +@pytest.mark.parametrize("inbound", _COLLIDING_HEADERS.values(), ids=list(_COLLIDING_HEADERS)) +def test_build_ack_reads_back_when_the_inbound_delimiters_collide_with_its_own_text( + inbound: bytes, code: str +) -> None: + ack = mllp.build_ack(inbound, code=code, timestamp="20260101000000-0500") + assert ack.startswith(b"MSH|^~\\&|"), ack + assert mllp.parse_ack(ack) == (code, None) + assert mllp.peek_fields(ack)[1] == "ACK" + # The tee reads no component; the engine's parser reads the ACK down to the subcomponent. + peek = Peek.parse(ack.decode("latin-1")) + assert (peek.field("MSH-9.1.1"), peek.field("MSA-1.1.1")) == ("ACK", code) + assert peek.field("MSH-7.1.1") == "20260101000000-0500" + + +@pytest.mark.parametrize( + ("name", "msa"), + [("msh1-letter", b"MSA|AA|"), ("msh1-escape", b"MSA|AA|"), ("msh2-letters", b"MSA|AA|C^1")], +) +def test_build_ack_blanks_an_echoed_value_a_default_delimiter_would_restructure( + name: str, msa: bytes +) -> None: + # "|" is data under field separator K, so "C|1" is blanked; under MSH-2 "^AB" the "^" is the + # component separator in both sets, so "C^1" is kept. The engine's build_ack has the same rule. + ack = mllp.build_ack(_COLLIDING_HEADERS[name]) + assert ack.endswith(b"\r" + msa + b"\r"), ack + assert ack.startswith(b"MSH|^~\\&|RCV|RFC|APP|FAC|"), ack + + +@pytest.mark.parametrize("sep", ["M", "S", "H"]) +def test_build_ack_reads_a_field_separator_that_is_a_letter_of_msh(sep: str) -> None: + inbound = f"MSH{sep}^~\\&{sep}APP{sep}FAC{sep}RCV{sep}RFC{sep}20260101{sep}{sep}ORU^R01{sep}C1" + ack = mllp.build_ack(f"{inbound}{sep}P{sep}2.5.1\r".encode()) + assert ack.startswith(b"MSH|^~\\&|RCV|RFC|APP|FAC|"), ack + assert ack.endswith(b"|ACK|C1|P|2.5.1\rMSA|AA|C1\r"), ack + + +def test_the_tee_and_the_engine_share_the_fallback_rule() -> None: + # The tee cannot import the engine, so its copy of the rule is pinned to the engine's here. + assert mllp._ACK_LITERAL_CHARS == mllpcodec._ACK_LITERAL_CHARS + assert mllp._DEFAULT_ROLES == mllpcodec._DEFAULT_ROLES + for field_sep, enc in (("|", "^AB"), ("D", "^~\\&"), ("|", "-~\\^"), ("#", "@@!%"), ("|", "A")): + assert mllp._roles(field_sep, enc) == mllpcodec._roles(field_sep, enc) + others = ( + b"MSH#@*!%#APP#FAC#RCV#RFC#20260101##ORU@R01#C|1#P#2.5.1\r", # punctuation: echoed + b"MSHD^~\\&DAPPDFACDRCVDRFCD20260101DDORU^R01DC|1DPD2.5.1\r", # a letter ACKs never hold + b"MSH^^APP^FAC^RCV^RFC^20260101^^ORU~R01^C1^P^2.5.1\r", # the field separator in MSH-2 + b"MSH|^~^&|APP|FAC|RCV|RFC|20260101||ORU^R01|C1|P|2.5.1\r", # two roles, one character + b"MSHF^~\\&FAPPFSACFRCVFRXCF20260101FFORU^R01FC1FPF2.5.1\r", # the F of an escape + b"MSH|A|APP|FAC|RCV|RFC|20260101||ORU^R01|X~Y|P|2.5.1\r", # a short MSH-2 + b"MSH|^~E&|APP|FAC|RCV|RFC|20260101||ORU^R01|CEFE1|P|2.5.1\r", # a letter escape + ) + for inbound in (*_COLLIDING_HEADERS.values(), *others): + engine = mllpcodec.build_ack(inbound.decode("latin-1"), timestamp="20260101") + assert mllp.build_ack(inbound, timestamp="20260101").decode("latin-1") == engine + + +def test_build_ack_still_echoes_punctuation_delimiters() -> None: + # The control: delimiters the ACK never writes as text are echoed, as before BACKLOG #2875. + ack = mllp.build_ack(b"MSH#@*!%#APP#FAC#RCV#RFC#20260101##ORU@R01#C|1#P#2.5.1\r") + assert ack.startswith(b"MSH#@*!%#RCV#RFC#APP#FAC#"), ack + assert ack.endswith(b"\rMSA#AA#C|1\r"), ack