feat(api): harden CORS and surface 500/409 errors with proper CORS headers
Three independent improvements that fix real browser-facing bugs: 1. CORS: allow 127.0.0.1:5173 in addition to localhost:5173. Both resolve to the same Vite dev server but CORS treats them as distinct origins, so tabs opened via the IP form silently break. 2. CORS: support CYCLONE_ALLOWED_ORIGINS env var (comma-separated) for LAN / staging hosts. The middleware reads it at module import. 3. Catch-all exception handler: returns JSON 500 with CORS headers instead of a bare Uvicorn text/plain response. Without this, any unhandled exception is misreported by browsers as a CORS error because the body can't be read without the allow-origin header. 4. IntegrityError → 409: when (batch_id, patient_control_number) is UNIQUE-constrained and a duplicate collides, return 409 with the batch id instead of letting the exception 500. Same problem as (3) for the most common ingest failure mode. Tests added: - test_cors_headers_present_for_loopback_ip - test_cors_extra_origins_via_env (uses importlib.reload because the allow-list is built at module import)
This commit is contained in:
@@ -9,8 +9,11 @@ The frontend (Vite/React on http://localhost:5173) uploads an X12 file via
|
|||||||
— one envelope, one line per claim/payout, then one summary — so the
|
— one envelope, one line per claim/payout, then one summary — so the
|
||||||
UI can render records incrementally as they're produced.
|
UI can render records incrementally as they're produced.
|
||||||
|
|
||||||
CORS is configured to allow the Vite dev origin (``http://localhost:5173``)
|
CORS is configured to allow the Vite dev origins (``http://localhost:5173``
|
||||||
plus GET/POST with any header.
|
and ``http://127.0.0.1:5173`` — both forms are valid dev-server addresses
|
||||||
|
but CORS treats them as distinct origins) plus GET/POST with any header.
|
||||||
|
Additional origins (LAN IPs, staging hosts) can be appended via the
|
||||||
|
``CYCLONE_ALLOWED_ORIGINS`` env var as a comma-separated list.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
@@ -19,6 +22,7 @@ import csv
|
|||||||
import io
|
import io
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
|
import os
|
||||||
import uuid
|
import uuid
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
from contextlib import asynccontextmanager
|
from contextlib import asynccontextmanager
|
||||||
@@ -33,6 +37,7 @@ from pydantic import ValidationError
|
|||||||
from cyclone import __version__, db
|
from cyclone import __version__, db
|
||||||
from cyclone.db import Claim, ClaimState, Remittance
|
from cyclone.db import Claim, ClaimState, Remittance
|
||||||
from sqlalchemy import desc, or_
|
from sqlalchemy import desc, or_
|
||||||
|
from sqlalchemy.exc import IntegrityError
|
||||||
from cyclone.inbox_state import apply_999_rejections
|
from cyclone.inbox_state import apply_999_rejections
|
||||||
from cyclone.inbox_state_277ca import apply_277ca_rejections
|
from cyclone.inbox_state_277ca import apply_277ca_rejections
|
||||||
from cyclone.audit_log import AuditEvent, append_event, verify_chain
|
from cyclone.audit_log import AuditEvent, append_event, verify_chain
|
||||||
@@ -214,7 +219,17 @@ PAYER_FACTORIES_835: dict[str, Any] = {
|
|||||||
"generic_835": PayerConfig835.generic_835,
|
"generic_835": PayerConfig835.generic_835,
|
||||||
}
|
}
|
||||||
|
|
||||||
VITE_DEV_ORIGIN = "http://localhost:5173"
|
# Allow both common dev-server origins. ``localhost`` and ``127.0.0.1``
|
||||||
|
# resolve to the same Vite dev server but CORS treats them as distinct
|
||||||
|
# origins, so the allow-list has to list both — otherwise a tab opened
|
||||||
|
# via the IP form gets blocked even though the dev server is identical.
|
||||||
|
VITE_DEV_ORIGINS: list[str] = [
|
||||||
|
"http://localhost:5173",
|
||||||
|
"http://127.0.0.1:5173",
|
||||||
|
]
|
||||||
|
_extra_origins = os.environ.get("CYCLONE_ALLOWED_ORIGINS", "").strip()
|
||||||
|
if _extra_origins:
|
||||||
|
VITE_DEV_ORIGINS.extend(o.strip() for o in _extra_origins.split(",") if o.strip())
|
||||||
|
|
||||||
app = FastAPI(
|
app = FastAPI(
|
||||||
title="Cyclone 837P / 835 Parser API",
|
title="Cyclone 837P / 835 Parser API",
|
||||||
@@ -225,7 +240,7 @@ app = FastAPI(
|
|||||||
|
|
||||||
app.add_middleware(
|
app.add_middleware(
|
||||||
CORSMiddleware,
|
CORSMiddleware,
|
||||||
allow_origins=[VITE_DEV_ORIGIN],
|
allow_origins=VITE_DEV_ORIGINS,
|
||||||
allow_credentials=False,
|
allow_credentials=False,
|
||||||
allow_methods=["GET", "POST"],
|
allow_methods=["GET", "POST"],
|
||||||
allow_headers=["*"],
|
allow_headers=["*"],
|
||||||
@@ -282,6 +297,35 @@ def _resolve_payer_835(name: str) -> PayerConfig835:
|
|||||||
return PAYER_FACTORIES_835[name]()
|
return PAYER_FACTORIES_835[name]()
|
||||||
|
|
||||||
|
|
||||||
|
# --------------------------------------------------------------------------- #
|
||||||
|
# Catch-all exception handler
|
||||||
|
# --------------------------------------------------------------------------- #
|
||||||
|
#
|
||||||
|
# Without this, any exception that escapes a route handler is rendered by
|
||||||
|
# Uvicorn as a bare ``500 Internal Server Error`` text/plain response with
|
||||||
|
# NO CORS headers. Browsers reading such a response report it as a CORS
|
||||||
|
# error (``Origin ... is not allowed by Access-Control-Allow-Origin``)
|
||||||
|
# because they cannot read the body without the CORS allow-origin header
|
||||||
|
# — even though the actual failure was on the server. We catch everything
|
||||||
|
# here, log it, and return a JSON error with the request's Origin echoed
|
||||||
|
# back so the browser can surface the real message.
|
||||||
|
@app.exception_handler(Exception)
|
||||||
|
async def _unhandled_exception_handler(request: Request, exc: Exception) -> JSONResponse:
|
||||||
|
log.exception(
|
||||||
|
"Unhandled exception in %s %s", request.method, request.url.path
|
||||||
|
)
|
||||||
|
headers: dict[str, str] = {}
|
||||||
|
origin = request.headers.get("origin", "")
|
||||||
|
if origin:
|
||||||
|
headers["Access-Control-Allow-Origin"] = origin
|
||||||
|
headers["Vary"] = "Origin"
|
||||||
|
return JSONResponse(
|
||||||
|
status_code=500,
|
||||||
|
content={"error": "Internal server error", "detail": str(exc)},
|
||||||
|
headers=headers,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
@app.post("/api/parse-837")
|
@app.post("/api/parse-837")
|
||||||
async def parse_837(
|
async def parse_837(
|
||||||
request: Request,
|
request: Request,
|
||||||
@@ -345,7 +389,30 @@ async def parse_837(
|
|||||||
parsed_at=utcnow(),
|
parsed_at=utcnow(),
|
||||||
result=result,
|
result=result,
|
||||||
)
|
)
|
||||||
store.add(rec, event_bus=request.app.state.event_bus)
|
try:
|
||||||
|
store.add(rec, event_bus=request.app.state.event_bus)
|
||||||
|
except IntegrityError as exc:
|
||||||
|
# ``(batch_id, patient_control_number)`` is UNIQUE — fires when a
|
||||||
|
# single batch file contains the same CLM01 control number twice,
|
||||||
|
# or when the same claim id has already been ingested in a prior
|
||||||
|
# batch. Surface as 409 with the batch id so the caller can look
|
||||||
|
# it up; do NOT 500 (a 500 without CORS headers is misreported by
|
||||||
|
# browsers as a CORS error and hides the real cause).
|
||||||
|
log.warning("Duplicate claim while persisting batch %s: %s", rec.id, exc)
|
||||||
|
return JSONResponse(
|
||||||
|
status_code=409,
|
||||||
|
content={
|
||||||
|
"error": "Duplicate claim",
|
||||||
|
"detail": (
|
||||||
|
"This file (or one previously ingested with the same "
|
||||||
|
"claim control number) collides with an existing "
|
||||||
|
"record. Inspect the file for duplicate CLM01 "
|
||||||
|
"control numbers, or remove the existing batch "
|
||||||
|
"before retrying."
|
||||||
|
),
|
||||||
|
"batch_id": rec.id,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
if _client_wants_json(request):
|
if _client_wants_json(request):
|
||||||
body = json.loads(result.model_dump_json())
|
body = json.loads(result.model_dump_json())
|
||||||
@@ -516,7 +583,23 @@ async def parse_835_endpoint(
|
|||||||
parsed_at=utcnow(),
|
parsed_at=utcnow(),
|
||||||
result=result,
|
result=result,
|
||||||
)
|
)
|
||||||
store.add(rec, event_bus=request.app.state.event_bus)
|
try:
|
||||||
|
store.add(rec, event_bus=request.app.state.event_bus)
|
||||||
|
except IntegrityError as exc:
|
||||||
|
log.warning("Duplicate remittance while persisting batch %s: %s", rec.id, exc)
|
||||||
|
return JSONResponse(
|
||||||
|
status_code=409,
|
||||||
|
content={
|
||||||
|
"error": "Duplicate remittance",
|
||||||
|
"detail": (
|
||||||
|
"This 835 file (or one previously ingested with the "
|
||||||
|
"same payer claim control number) collides with an "
|
||||||
|
"existing record. Remove the existing remittance "
|
||||||
|
"before retrying."
|
||||||
|
),
|
||||||
|
"batch_id": rec.id,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
if _client_wants_json(request):
|
if _client_wants_json(request):
|
||||||
body = json.loads(result.model_dump_json())
|
body = json.loads(result.model_dump_json())
|
||||||
|
|||||||
@@ -160,3 +160,51 @@ def test_cors_headers_present(client: TestClient):
|
|||||||
)
|
)
|
||||||
assert resp.headers.get("access-control-allow-origin") == "http://localhost:5173"
|
assert resp.headers.get("access-control-allow-origin") == "http://localhost:5173"
|
||||||
assert "POST" in resp.headers.get("access-control-allow-methods", "").upper()
|
assert "POST" in resp.headers.get("access-control-allow-methods", "").upper()
|
||||||
|
|
||||||
|
|
||||||
|
def test_cors_headers_present_for_loopback_ip(client: TestClient):
|
||||||
|
# ``http://127.0.0.1:5173`` is a distinct origin from
|
||||||
|
# ``http://localhost:5173`` per the CORS spec, even though both resolve
|
||||||
|
# to the same Vite dev server. Both must be allow-listed or tabs opened
|
||||||
|
# via the IP form silently break.
|
||||||
|
resp = client.options(
|
||||||
|
"/api/parse-837",
|
||||||
|
headers={
|
||||||
|
"Origin": "http://127.0.0.1:5173",
|
||||||
|
"Access-Control-Request-Method": "POST",
|
||||||
|
"Access-Control-Request-Headers": "content-type",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
assert resp.headers.get("access-control-allow-origin") == "http://127.0.0.1:5173"
|
||||||
|
|
||||||
|
|
||||||
|
def test_cors_extra_origins_via_env(client: TestClient, monkeypatch):
|
||||||
|
# LAN / staging hosts opt in via CYCLONE_ALLOWED_ORIGINS. The env var
|
||||||
|
# is a comma-separated list; the middleware must reflect each entry.
|
||||||
|
# The allow-list is built at module import, so we re-execute the
|
||||||
|
# module under the env var and build a TestClient against the
|
||||||
|
# reloaded app.
|
||||||
|
monkeypatch.setenv(
|
||||||
|
"CYCLONE_ALLOWED_ORIGINS", "http://192.168.1.42:5173,https://staging.example.com"
|
||||||
|
)
|
||||||
|
import importlib
|
||||||
|
from cyclone import api as api_module
|
||||||
|
from fastapi.testclient import TestClient as _TC
|
||||||
|
importlib.reload(api_module)
|
||||||
|
try:
|
||||||
|
with _TC(api_module.app) as tc:
|
||||||
|
for origin in ("http://192.168.1.42:5173", "https://staging.example.com"):
|
||||||
|
resp = tc.options(
|
||||||
|
"/api/parse-837",
|
||||||
|
headers={
|
||||||
|
"Origin": origin,
|
||||||
|
"Access-Control-Request-Method": "POST",
|
||||||
|
"Access-Control-Request-Headers": "content-type",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
assert resp.headers.get("access-control-allow-origin") == origin
|
||||||
|
finally:
|
||||||
|
monkeypatch.delenv("CYCLONE_ALLOWED_ORIGINS", raising=False)
|
||||||
|
# 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)
|
||||||
|
|||||||
Reference in New Issue
Block a user