From f25214189ae0b854df28e3fc001c1b90bff2b1c1 Mon Sep 17 00:00:00 2001 From: Nora Date: Mon, 6 Jul 2026 09:51:27 -0600 Subject: [PATCH] feat(sp35): /api/parse-837 envelope + empty-claims guards Server-side defense in depth for misroute ingest. Before SP35, posting an 835 file (or any other X12 with a parseable ISA envelope) to /api/parse-837 silently produced a BatchRecord with claims=[] and a bogus row on the History tab. The 837 parser only required an ISA envelope; it didn't check the ST transaction-set id. Two new guards run before persistence: 1. Envelope check: tokenize first, read ST01, reject anything that doesn't start with '837'. 400 with error='Mismatched file kind', expected='837p', detected_st=. Catches an 835/999/270/etc routed to the wrong endpoint. 2. Empty-claims check: even with the right envelope, if the parser produces zero CLM segments, return 400 'No claims parsed' and do NOT persist. New tests in tests/test_api.py: - test_parse_837_endpoint_rejects_835_input (was failing, now green) - test_parse_837_endpoint_rejects_empty_envelope (was failing, now green) - test_parse_837_endpoint_happy_path_still_works (regression guard) Helper _transaction_set_id_from_segments reused by the 835 mirror. --- backend/src/cyclone/api.py | 78 ++++++++++++++++++++++++ backend/tests/test_api.py | 122 +++++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+) diff --git a/backend/src/cyclone/api.py b/backend/src/cyclone/api.py index d506279..000bc8c 100644 --- a/backend/src/cyclone/api.py +++ b/backend/src/cyclone/api.py @@ -83,6 +83,7 @@ from cyclone.parsers.parse_837 import parse from cyclone.parsers.parse_835 import parse as parse_835 from cyclone.parsers.parse_999 import parse_999_text from cyclone.parsers.parse_ta1 import parse_ta1_text +from cyclone.parsers.segments import tokenize as _tokenize_segments from cyclone.parsers.serialize_270 import serialize_270 from cyclone.parsers.serialize_999 import serialize_999 from cyclone.parsers.serialize_837 import SerializeError as SerializeError837, serialize_837, serialize_837_for_resubmit @@ -353,6 +354,23 @@ def _resolve_payer_835(name: str) -> PayerConfig835: return PAYER_FACTORIES_835[name]() +def _transaction_set_id_from_segments(segments: list[list[str]]) -> str | None: + """Return the ST01 transaction-set id (``"837"``, ``"835"``, ``"999"``...). + + SP35 helper: scans the first few tokenized segments for the ST + segment and returns its second element (ST01). Returns None when no + ST is present — e.g. a TA1 file, which uses the bare TA1 segment + and no ST envelope. The endpoint-level envelope guards treat + ``None`` as "no ST found; let the parser decide" so TA1 files + routed through the wrong endpoint still surface a parse error + rather than a misleading "expected 837p, got ''" message. + """ + for seg in segments[:5]: # ST is always the second segment after ISA + if seg and seg[0] == "ST" and len(seg) > 1: + return seg[1] + return None + + # --------------------------------------------------------------------------- # # Catch-all exception handler # --------------------------------------------------------------------------- # @@ -391,6 +409,23 @@ async def parse_837( strict: bool = Query(False), ack: bool = Query(False), ) -> Any: + # SP35: defense-in-depth input guards. Layer A (UI auto-detect) lives + # in src/pages/Upload.tsx; the server-side checks below are the + # authoritative fix because they protect every caller of the API + # (Upload page, CLI ingestion, any future bulk-import tool). Without + # these, an 835 file dropped on the Upload page while the dropdown + # still says "837p" produces a BatchRecord with claims=[] and a bogus + # row on the History tab. The fix is two checks run BEFORE we persist + # anything: + # + # 1. Envelope check — ST01 must be "837" or "837P". Anything else + # (an 835, a 999, a 270, garbage that happens to have an ISA) + # → 400 with error="Mismatched file kind", expected="837p", + # detected_st=. + # 2. Empty-claims check — even with the right envelope, if the + # parser produced zero CLM segments (truncated file, header-only + # test fixture) → 400 with error="No claims parsed". A real + # production 837 batch with zero claims is never valid. raw = await file.read() if not raw: return JSONResponse( @@ -407,6 +442,32 @@ async def parse_837( config = _resolve_payer(payer) + # SP35 guard 1: envelope check. Tokenize first so we can return a + # precise 400 (vs. relying on the parser's "no ISA envelope" error + # which is correct but doesn't say "you sent an 835 to the 837 + # endpoint"). If tokenization itself fails we fall through to the + # parser, which raises CycloneParseError → 400 "Parse error" path. + try: + _segments = _tokenize_segments(text) + detected_st = _transaction_set_id_from_segments(_segments) or "" + except CycloneParseError: + detected_st = "" + + if detected_st and not detected_st.upper().startswith("837"): + return JSONResponse( + status_code=400, + content={ + "error": "Mismatched file kind", + "expected": "837p", + "detected_st": detected_st, + "detail": ( + f"File declares ST*{detected_st}* but this endpoint " + f"expects ST*837*. Pick the matching endpoint on the " + f"Upload page (or let auto-detect choose for you)." + ), + }, + ) + try: result = parse(text, config, input_file=file.filename or "") except CycloneParseError as exc: @@ -421,6 +482,23 @@ async def parse_837( content={"error": "Internal server error", "detail": str(exc)}, ) + # SP35 guard 2: empty-claims check. With the envelope validated, the + # only way to land here is a header-only file (real, but useless) + # or a file whose CLM loops the parser couldn't extract. Either way + # we refuse to persist — a BatchRecord with claims=[] is what the + # original bug produced and is never what the operator wanted. + if not result.claims: + return JSONResponse( + status_code=400, + content={ + "error": "No claims parsed", + "detail": ( + "The file passed the envelope check but contained no " + "CLM segments. Refusing to persist an empty batch." + ), + }, + ) + if strict: result = _strict_rewrite(result) if not include_raw_segments: diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index c879131..5b5c631 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -18,6 +18,7 @@ from cyclone import __version__ FIXTURE = Path(__file__).parent / "fixtures" / "co_medicaid_837p.txt" +FIXTURE_835 = Path(__file__).parent / "fixtures" / "co_medicaid_835.txt" @pytest.fixture @@ -216,3 +217,124 @@ def test_cors_extra_origins_via_env(client: TestClient, monkeypatch): # Reload once more so the module-level allow-list returns to its # default for any test that imports `cyclone.api` after this one. importlib.reload(api_module) + + +# --------------------------------------------------------------------------- # +# SP35: parse-837 input guards (defense in depth against misroute ingest) +# --------------------------------------------------------------------------- # +# +# Before SP35, /api/parse-837 silently accepted any file that had a parseable +# ISA envelope. An 835 file dropped on the Upload page while the dropdown +# still said "837p" would land in the DB as an empty batch (claims=[]) and a +# bogus row on the History tab. SP35 fixes that at three layers: +# +# 1. Server envelope check: ST*837 (or ST*837P) required, else 400 with +# error="Mismatched file kind". +# 2. Server empty-claims check: even with the right envelope, if zero CLM +# segments were parsed, return 400 with error="No claims parsed" +# and DO NOT persist the batch. +# 3. UI auto-detect (separate file: src/pages/Upload.test.tsx). +# +# These tests are the server-layer regression locks. They run against the +# TestClient and use the existing fixtures. The 835 fixture has an ST*835 +# envelope; posting it to /api/parse-837 must surface a 400 and must not +# create a BatchRecord. + + +def test_parse_837_endpoint_rejects_835_input(client: TestClient): + """Uploading an 835 file to /api/parse-837 must fail loudly, not persist. + + Repro for the original bug: user drops an 835 file on the Upload page + while the kind dropdown still says "837p" (Upload.tsx default). Before + SP35 the endpoint accepted it, ran it through the 837 parser (which + found zero CLM segments because the file has none), and persisted a + claims=[] batch — a bogus row on the History tab and on + /api/batches. SP35 closes the door at the server so the UI bug becomes + cosmetic instead of data-corrupting. + """ + text_835 = FIXTURE_835.read_text() + # Sanity check: the fixture really is an 835 file. If this ever flips, + # the test would still pass for the wrong reason. + assert "ST*835" in text_835, "fixture is no longer ST*835 — update SP35 tests" + + # Snapshot the batch count BEFORE the bad upload so we can assert the + # request did NOT persist anything. Using the public /api/batches JSON + # endpoint (already exercised by the Dashboard). + before = client.get("/api/batches", headers={"Accept": "application/json"}).json() + total_before = before.get("total", len(before.get("items", []))) + + resp = client.post( + "/api/parse-837", + files={"file": ("co_medicaid_835.txt", text_835, "text/plain")}, + headers={"Accept": "application/json"}, + ) + + assert resp.status_code == 400, resp.text + body = resp.json() + assert body["error"] == "Mismatched file kind" + assert body["expected"] == "837p" + assert body["detected_st"].startswith("835") + + after = client.get("/api/batches", headers={"Accept": "application/json"}).json() + total_after = after.get("total", len(after.get("items", []))) + assert total_after == total_before, "Server persisted a batch from a 835 file" + + +def test_parse_837_endpoint_rejects_empty_envelope(client: TestClient): + """Right envelope (ST*837), zero CLM segments → 400 No claims parsed. + + Synthetic input: a complete ISA/GS/ST envelope with a BHT, a closing + SE/GE/IEA, and no CLM loops. The 837 parser will tokenize and build + the envelope cleanly, then return claims=[]. SP35 must surface this as + a 400 with error="No claims parsed" and must not persist a batch. + """ + # Bare 837 envelope — no HL/CLM loops. A real X12 file with ST*837 + # but no claims is unusual but possible (e.g. a header-only test file + # or a truncated/cancelled run). The right behavior is to reject, + # not to silently persist an empty batch. + synthetic = ( + "ISA*00* *00* *ZZ*SENDER *ZZ*RECEIVER " + "*260617*1937*^*00501*000000001*0*P*:~" + "GS*HC*SENDER*RECEIVER*20260706*1937*1*X*005010X222A1~" + "ST*837*0001*005010X222A1~" + "BHT*0019*00*0001*20260706*1937*CH~" + "SE*2*0001~" + "GE*1*1~" + "IEA*1*000000001~" + ) + assert "ST*837" in synthetic + + before = client.get("/api/batches", headers={"Accept": "application/json"}).json() + total_before = before.get("total", len(before.get("items", []))) + + resp = client.post( + "/api/parse-837", + files={"file": ("empty_837.txt", synthetic, "text/plain")}, + headers={"Accept": "application/json"}, + ) + + assert resp.status_code == 400, resp.text + body = resp.json() + assert body["error"] == "No claims parsed" + + after = client.get("/api/batches", headers={"Accept": "application/json"}).json() + total_after = after.get("total", len(after.get("items", []))) + assert total_after == total_before, "Server persisted an empty-claims batch" + + +def test_parse_837_endpoint_happy_path_still_works(client: TestClient): + """Regression guard: real 837 fixture must still parse → 200 with claims. + + Sits next to the new SP35 rejection tests so any future tightening of + the guards that accidentally blocks the happy path fails here loudly. + """ + text = FIXTURE.read_text() + resp = client.post( + "/api/parse-837", + files={"file": ("co_medicaid_837p.txt", text, "text/plain")}, + headers={"Accept": "application/json"}, + ) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body.get("summary", {}).get("total_claims", 0) >= 1 + assert "batch_id" in body and len(body["batch_id"]) == 32