From 30ae27fe1e2f5f5b22dd760f74737b41c0635a74 Mon Sep 17 00:00:00 2001 From: Andrey Fedorov Date: Fri, 2 Oct 2026 12:25:16 -0400 Subject: [PATCH] fix(filters): validate range bounds against the column type NumericRange admits strings because StudyDate/SeriesDate are string columns. That let two bad inputs through compile_filters: - A non-numeric string on a numeric column (e.g. instanceCount gte "1 AND 1=1", seen in production logs) failed inside DuckDB's cast, escaped every typed handler, and surfaced as an internal error / HTTP 500 with a logged traceback. The value was always a bound parameter, so this was never an injection vector. - A non-date string on a date column compared lexically and silently matched nothing. Numeric bounds are now coerced to finite floats and date bounds normalized to YYYY-MM-DD (DICOM YYYYMMDD accepted); anything else raises InvalidQueryError -> 400 / clean MCP ToolError. A correctly formatted but impossible date (2020-02-30) gets its own "not a real calendar date" message rather than being told to use the format it already has. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 +++++ docs/user-guide.md | 5 +++- src/idc_api/core/filters.py | 60 +++++++++++++++++++++++++++++++++++++ src/idc_api/core/schema.py | 20 +++++++++++-- src/idc_api/mcp/server.py | 3 +- tests/test_filter_shape.py | 56 ++++++++++++++++++++++++++++++++++ 6 files changed, 147 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d043576..9dde1db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,13 @@ Refactors, CI, and formatting land in the git history, not here. ## [Unreleased] +### Fixed + +- Range filters now validate their bounds instead of failing or silently matching nothing: + numeric attributes require a number (previously an internal error), and `StudyDate` / + `SeriesDate` require a real date as `"YYYY-MM-DD"` (`"YYYYMMDD"` is normalized). Invalid + bounds return a 400 `invalid_query` / MCP tool error naming the field. + ## [3.0.0b4] — 2026-10-01 Security-maintenance release; no API or MCP contract changes. diff --git a/docs/user-guide.md b/docs/user-guide.md index d7e7637..ef400e1 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -315,7 +315,10 @@ curl -s localhost:8000/v3/cohort/manifest \ ``` Filters: `terms` is `{attribute: [values]}` (equality/IN — OR within an attribute, AND across -attributes); `ranges` is `{attribute: {"gte": x, "lte": y}}`. +attributes); `ranges` is `{attribute: {"gte": x, "lte": y}}`. Bounds on numeric attributes +must be numbers; bounds on `StudyDate` / `SeriesDate` must be dates as `"YYYY-MM-DD"` (DICOM +`"YYYYMMDD"` is accepted and normalized). Any other bound is rejected with a 400 rather than +silently matching nothing. **Get the full manifest as plain text** (for `idc download-from-manifest` / `s5cmd`): diff --git a/src/idc_api/core/filters.py b/src/idc_api/core/filters.py index 49c9650..424bbd5 100644 --- a/src/idc_api/core/filters.py +++ b/src/idc_api/core/filters.py @@ -12,12 +12,18 @@ from __future__ import annotations +import math +import re +from datetime import date from typing import Any, NamedTuple from . import schema from .errors import InvalidQueryError from .models import CohortFilters, NumericRange +# ISO extended (2020-01-31) or DICOM DA / ISO basic (20200131); both normalize to the stored form. +_DATE_RE = re.compile(r"\d{4}-?\d{2}-?\d{2}") + UNFILTERED_WARNING = ( "No filter predicates were applied, so this result describes the ENTIRE IDC archive, not a " "cohort. If you meant to filter, the filter did not arrive in a usable shape — compare " @@ -77,6 +83,14 @@ def compile_filters(filters: CohortFilters) -> CompiledFilters: "constrains nothing." ) continue + if attr in schema.numeric_range_attributes(): + rng = NumericRange( + gte=_numeric_bound(attr, "gte", rng.gte), lte=_numeric_bound(attr, "lte", rng.lte) + ) + elif attr in schema.DATE_RANGE_ATTRIBUTES: + rng = NumericRange( + gte=_date_bound(attr, "gte", rng.gte), lte=_date_bound(attr, "lte", rng.lte) + ) if rng.gte is not None: clauses.append(f'"{attr}" >= ?') params.append(rng.gte) @@ -96,6 +110,52 @@ def compile_filters(filters: CohortFilters) -> CompiledFilters: ) +def _numeric_bound(attr: str, key: str, value: float | str | None) -> float | None: + """Coerce a bound on a numeric column to a finite number. + + ``NumericRange`` admits strings because the date columns are strings. Bound against a + numeric column, a non-numeric string fails inside DuckDB's cast — an engine error, not a + caller one — so it is refused here with the reason instead. NaN/inf parse as floats but + compare to nothing useful, so they are refused too. + """ + if value is None: + return None + try: + number = float(value) + except ValueError: + number = math.nan + if not math.isfinite(number): + raise InvalidQueryError( + f"Range filter {attr!r} is numeric, but {key!r} is {value!r}, which is not a number." + ) + return number + + +def _date_bound(attr: str, key: str, value: float | str | None) -> str | None: + """Normalize a bound on a date column to the stored ``YYYY-MM-DD`` form. + + The date columns are strings, so DuckDB compares a bound lexically: anything that isn't an + ISO date (``"nope"``, ``"01/31/2020"``, a bare number) runs fine and silently matches the + wrong series — usually none. Refuse it instead of answering with a plausible-looking zero. + """ + if value is None: + return None + if isinstance(value, str) and _DATE_RE.fullmatch(value.strip()): + try: + return date.fromisoformat(value.strip()).isoformat() + except ValueError: + # Right shape, impossible date (2020-02-30): repeating "use YYYY-MM-DD" back to a + # caller who did would only prompt a retry of the same value. + raise InvalidQueryError( + f"Range filter {attr!r} bound {key!r} is {value!r}, which is not a real " + "calendar date." + ) from None + raise InvalidQueryError( + f"Range filter {attr!r} is a date, but {key!r} is {value!r}; give it as 'YYYY-MM-DD' " + "(e.g. '2020-01-31')." + ) + + def require_filter(compiled: CompiledFilters, action: str) -> None: """Refuse ``action`` when no predicate survived compilation. diff --git a/src/idc_api/core/schema.py b/src/idc_api/core/schema.py index 83d42aa..0c4dc28 100644 --- a/src/idc_api/core/schema.py +++ b/src/idc_api/core/schema.py @@ -186,6 +186,8 @@ def table_schema(table: str) -> dict: # AND across attributes (the standard cohort-filter convention). # - "range": numeric or lexically-ordered (ISO date) column, filtered by gte/lte. # ``categorical`` flags low-cardinality columns worth offering value-discovery for. +# ``date`` marks a range column stored as an ISO ``YYYY-MM-DD`` string; its bounds must be +# dates in that form, since a string column compares any other text lexically. # ``note`` is a semantic caveat surfaced wherever the attribute is described (list_attributes # descriptions and get_attribute_values responses) — use it when the obvious reading of an # attribute is wrong for some series and the right column lives elsewhere. Durable facts @@ -220,13 +222,14 @@ def table_schema(table: str) -> dict: {"name": "series_size_MB", "kind": "range", "categorical": False}, {"name": "series_init_idc_version", "kind": "range", "categorical": False}, {"name": "series_revised_idc_version", "kind": "range", "categorical": False}, - {"name": "StudyDate", "kind": "range", "categorical": False}, - {"name": "SeriesDate", "kind": "range", "categorical": False}, + {"name": "StudyDate", "kind": "range", "categorical": False, "date": True}, + {"name": "SeriesDate", "kind": "range", "categorical": False, "date": True}, ] _ATTR_BY_NAME = {a["name"]: a for a in FILTERABLE_ATTRIBUTES} TERM_ATTRIBUTES = {a["name"] for a in FILTERABLE_ATTRIBUTES if a["kind"] == "term"} RANGE_ATTRIBUTES = {a["name"] for a in FILTERABLE_ATTRIBUTES if a["kind"] == "range"} +DATE_RANGE_ATTRIBUTES = {a["name"] for a in FILTERABLE_ATTRIBUTES if a.get("date")} @lru_cache(maxsize=1) @@ -240,6 +243,19 @@ def index_columns() -> frozenset[str]: return frozenset(_index_column_descriptions().keys()) +_NUMERIC_TYPES = {"INTEGER", "INT64", "FLOAT", "FLOAT64", "NUMERIC", "BIGNUMERIC"} + + +@lru_cache(maxsize=1) +def numeric_range_attributes() -> frozenset[str]: + """Range attributes backed by a numeric column. The rest are the string-typed date columns + (DATE_RANGE_ATTRIBUTES), which is why a range bound may be a str at all.""" + col_desc = _index_column_descriptions() + return frozenset( + a for a in RANGE_ATTRIBUTES if col_desc.get(a, {}).get("type", "").upper() in _NUMERIC_TYPES + ) + + def filterable_attributes() -> list[dict]: """Attributes for the `list_attributes` capability, enriched with type + description.""" col_desc = _index_column_descriptions() diff --git a/src/idc_api/mcp/server.py b/src/idc_api/mcp/server.py index a5db696..02f0b1f 100644 --- a/src/idc_api/mcp/server.py +++ b/src/idc_api/mcp/server.py @@ -379,7 +379,8 @@ def build_cohort( `terms` is {attribute: [values]} for equality/IN (e.g. {"Modality": ["MR"], "BodyPartExamined": ["BREAST"]}). `ranges` is {attribute: {"gte": x, "lte": y}} for - numeric/date ranges. Discover valid attributes with list_attributes and valid values with + numeric/date ranges — numbers for numeric attributes, 'YYYY-MM-DD' strings for StudyDate/ + SeriesDate. Discover valid attributes with list_attributes and valid values with get_attribute_values. For anything these structured filters can't express, use run_sql. At least one filter predicate is required — an unfiltered cohort is the whole 100+ TB archive; diff --git a/tests/test_filter_shape.py b/tests/test_filter_shape.py index 116f7ec..07e744c 100644 --- a/tests/test_filter_shape.py +++ b/tests/test_filter_shape.py @@ -55,6 +55,62 @@ def test_misspelled_filter_keys_are_rejected(client): assert client.post("/v3/cohort/counts", json=body).status_code == 422, body +def test_non_numeric_bound_on_numeric_range_is_a_400(client): + """Range bounds admit strings (the date columns are strings); against a numeric column a + non-numeric one used to fail inside DuckDB's cast and surface as a 500.""" + for attr in ("instanceCount", "series_size_MB"): + body = {"filters": {"terms": _TERMS, "ranges": {attr: {"gte": "1 AND 1=1"}}}} + r = client.post("/v3/cohort/counts", json=body) + assert r.status_code == 400, (attr, r.text[:200]) + assert "is not a number" in r.json()["error"]["message"] + + # A numeric string is still a number, and is echoed as one. + body = {"filters": {"terms": _TERMS, "ranges": {"instanceCount": {"gte": "2"}}}} + r = client.post("/v3/cohort/counts", json=body).json() + assert r["filters_applied"]["ranges"] == {"instanceCount": {"gte": 2.0, "lte": None}} + + # NaN parses as a float but matches nothing; it is refused like any other non-number. + body = {"filters": {"terms": _TERMS, "ranges": {"instanceCount": {"gte": "nan"}}}} + assert client.post("/v3/cohort/counts", json=body).status_code == 400 + + +def test_date_range_bounds_must_be_dates(client): + """The date columns are strings, so a non-date bound used to compare lexically and answer + with a plausible-looking zero instead of an error.""" + for bad in ("nope", "01/31/2020", "2020", 20200101): + body = {"filters": {"terms": _TERMS, "ranges": {"StudyDate": {"gte": bad}}}} + r = client.post("/v3/cohort/counts", json=body) + assert r.status_code == 400, (bad, r.text[:200]) + assert "YYYY-MM-DD" in r.json()["error"]["message"] + + # Correctly formatted but impossible: say so, rather than repeat the format it already has. + for bad in ("2020-02-30", "20201301"): + body = {"filters": {"terms": _TERMS, "ranges": {"StudyDate": {"lte": bad}}}} + r = client.post("/v3/cohort/counts", json=body) + assert r.status_code == 400, (bad, r.text[:200]) + message = r.json()["error"]["message"] + assert "not a real calendar date" in message and "YYYY-MM-DD" not in message + + # ISO dates pass; DICOM DA (YYYYMMDD) is normalized to the stored form and echoed as such. + iso = {"filters": {"terms": _TERMS, "ranges": {"StudyDate": {"gte": "1900-01-01"}}}} + dicom = {"filters": {"terms": _TERMS, "ranges": {"StudyDate": {"gte": "19000101"}}}} + a = client.post("/v3/cohort/counts", json=iso).json() + b = client.post("/v3/cohort/counts", json=dicom).json() + assert a["series"] > 0 and a["series"] == b["series"] + assert b["filters_applied"]["ranges"] == {"StudyDate": {"gte": "1900-01-01", "lte": None}} + + +async def test_mcp_bad_range_bounds_are_clean_tool_errors(parse_mcp): + with pytest.raises(ToolError, match="is not a number"): + await mcp.call_tool( + "build_cohort", {"terms": _TERMS, "ranges": {"instanceCount": {"gte": "1 AND 1=1"}}} + ) + with pytest.raises(ToolError, match="YYYY-MM-DD"): + await mcp.call_tool( + "build_cohort", {"terms": _TERMS, "ranges": {"SeriesDate": {"lte": "yesterday"}}} + ) + + # --- what survived compilation is always reported ------------------------------------------