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=<actual>. 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.
This commit is contained in:
@@ -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_835 import parse as parse_835
|
||||||
from cyclone.parsers.parse_999 import parse_999_text
|
from cyclone.parsers.parse_999 import parse_999_text
|
||||||
from cyclone.parsers.parse_ta1 import parse_ta1_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_270 import serialize_270
|
||||||
from cyclone.parsers.serialize_999 import serialize_999
|
from cyclone.parsers.serialize_999 import serialize_999
|
||||||
from cyclone.parsers.serialize_837 import SerializeError as SerializeError837, serialize_837, serialize_837_for_resubmit
|
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]()
|
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
|
# Catch-all exception handler
|
||||||
# --------------------------------------------------------------------------- #
|
# --------------------------------------------------------------------------- #
|
||||||
@@ -391,6 +409,23 @@ async def parse_837(
|
|||||||
strict: bool = Query(False),
|
strict: bool = Query(False),
|
||||||
ack: bool = Query(False),
|
ack: bool = Query(False),
|
||||||
) -> Any:
|
) -> 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=<whatever was there>.
|
||||||
|
# 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()
|
raw = await file.read()
|
||||||
if not raw:
|
if not raw:
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
@@ -407,6 +442,32 @@ async def parse_837(
|
|||||||
|
|
||||||
config = _resolve_payer(payer)
|
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:
|
try:
|
||||||
result = parse(text, config, input_file=file.filename or "")
|
result = parse(text, config, input_file=file.filename or "")
|
||||||
except CycloneParseError as exc:
|
except CycloneParseError as exc:
|
||||||
@@ -421,6 +482,23 @@ async def parse_837(
|
|||||||
content={"error": "Internal server error", "detail": str(exc)},
|
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:
|
if strict:
|
||||||
result = _strict_rewrite(result)
|
result = _strict_rewrite(result)
|
||||||
if not include_raw_segments:
|
if not include_raw_segments:
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ from cyclone import __version__
|
|||||||
|
|
||||||
|
|
||||||
FIXTURE = Path(__file__).parent / "fixtures" / "co_medicaid_837p.txt"
|
FIXTURE = Path(__file__).parent / "fixtures" / "co_medicaid_837p.txt"
|
||||||
|
FIXTURE_835 = Path(__file__).parent / "fixtures" / "co_medicaid_835.txt"
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture
|
@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
|
# Reload once more so the module-level allow-list returns to its
|
||||||
# default for any test that imports `cyclone.api` after this one.
|
# default for any test that imports `cyclone.api` after this one.
|
||||||
importlib.reload(api_module)
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user