fix(security): FLIP-PT-002 gate row-level cohort egress, FLIP-PT-088 replace SQL denylists

AGENTS.md

@@ -433,6 +433,8 @@ TruffleHog, detect-secrets, large file check (max 1000KB), merge conflict marker
 - Internal service key for fl-server-to-hub auth (separate from trust keys).
 - Trust-internal service key for trust-api / imaging-api / fl-client → imaging-api / data-access-api auth (per-trust, never leaves trust env). See **Trust-internal Service Authentication** below.
 - FL clients intentionally have no Central Hub credentials.
+- Cohort-query validation is three-layer and **deliberately asymmetric — do not "sync" the layers**. Only the trust-side `data_access_api.services.cohort.validate_query` is authoritative (single parse-validate-emit; length, single-statement, SELECT-only, no `INSERT`/`UPDATE`/`DELETE`/`MERGE` anywhere in the tree — a writable CTE parses as a top-level `Select`, so the shape check alone misses it — `omop`-schema pin, literal `LIMIT`/`OFFSET`; re-emits from the checked AST; backed by the read-only `data_analyst_reader` role — pass its return value to the engine, never the caller's raw string). The hub-side `flip_api.cohort_services.submit_cohort_query.validate_query` is a *fast-feedback validity pre-check only, not a security control*: it exists so a malformed query fails in-hand instead of after an async fan-out to every trust, and enforces only what every trust would reject anyway. The flip-ui cohort form validates required-field only. A trust must stay safe regardless of what the hub checked, so hub drift is safe by construction. **No layer uses a keyword denylist** — the removed one blocked legitimate `SUBSTRING()` while stopping nothing; blind extraction is defeated by the literal-`LIMIT` rule and DDL/DML by the read-only role. See [`trust/data-access-api/README.md`](trust/data-access-api/README.md#cohort-query-validation).
+- Row-level cohort egress is gated on `COHORT_QUERY_THRESHOLD` at **both** row-level routes — `/cohort/dataframe` (FL training data) and `/cohort/accession-ids` (the accession list that decides whose imaging is pulled into XNAT) — sharing one fixed refusal string so a below-threshold cohort is indistinguishable from an empty one. The threshold is the trust's own disclosure floor (default 10, set per trust in its kit file), enforced trust-side rather than relying on the hub's staging guard. Both gates evaluate the **live** cohort on every call: FLIP stores the cohort only as a SQL string and re-runs it against OMOP at every stage, so a project can import cleanly and later start refusing (FLIP#857).
 - Do not hardcode env values in Dockerfiles or compose files.
 - 72-hour supply-chain cooldown on Python/npm package installs — enforced by uv `exclude-newer` (`[tool.uv]` in every `pyproject.toml`) and npm `min-release-age` (`flip-ui/.npmrc`, requires npm >= 11.10 which Node 24 LTS ships), backstopped by a `uv lock --check` CI gate in `secret-scanning.yml`. See CONTRIBUTING.md ("Dependency cooldown").
 

CLAUDE.md

@@ -433,6 +433,8 @@ TruffleHog, detect-secrets, large file check (max 1000KB), merge conflict marker
 - Internal service key for fl-server-to-hub auth (separate from trust keys).
 - Trust-internal service key for trust-api / imaging-api / fl-client → imaging-api / data-access-api auth (per-trust, never leaves trust env). See **Trust-internal Service Authentication** below.
 - FL clients intentionally have no Central Hub credentials.
+- Cohort-query validation is three-layer and **deliberately asymmetric — do not "sync" the layers**. Only the trust-side `data_access_api.services.cohort.validate_query` is authoritative (single parse-validate-emit; length, single-statement, SELECT-only, no `INSERT`/`UPDATE`/`DELETE`/`MERGE` anywhere in the tree — a writable CTE parses as a top-level `Select`, so the shape check alone misses it — `omop`-schema pin, literal `LIMIT`/`OFFSET`; re-emits from the checked AST; backed by the read-only `data_analyst_reader` role — pass its return value to the engine, never the caller's raw string). The hub-side `flip_api.cohort_services.submit_cohort_query.validate_query` is a *fast-feedback validity pre-check only, not a security control*: it exists so a malformed query fails in-hand instead of after an async fan-out to every trust, and enforces only what every trust would reject anyway. The flip-ui cohort form validates required-field only. A trust must stay safe regardless of what the hub checked, so hub drift is safe by construction. **No layer uses a keyword denylist** — the removed one blocked legitimate `SUBSTRING()` while stopping nothing; blind extraction is defeated by the literal-`LIMIT` rule and DDL/DML by the read-only role. See [`trust/data-access-api/README.md`](trust/data-access-api/README.md#cohort-query-validation).
+- Row-level cohort egress is gated on `COHORT_QUERY_THRESHOLD` at **both** row-level routes — `/cohort/dataframe` (FL training data) and `/cohort/accession-ids` (the accession list that decides whose imaging is pulled into XNAT) — sharing one fixed refusal string so a below-threshold cohort is indistinguishable from an empty one. The threshold is the trust's own disclosure floor (default 10, set per trust in its kit file), enforced trust-side rather than relying on the hub's staging guard. Both gates evaluate the **live** cohort on every call: FLIP stores the cohort only as a SQL string and re-runs it against OMOP at every stage, so a project can import cleanly and later start refusing (FLIP#857).
 - Do not hardcode env values in Dockerfiles or compose files.
 - 72-hour supply-chain cooldown on Python/npm package installs — enforced by uv `exclude-newer` (`[tool.uv]` in every `pyproject.toml`) and npm `min-release-age` (`flip-ui/.npmrc`, requires npm >= 11.10 which Node 24 LTS ships), backstopped by a `uv lock --check` CI gate in `secret-scanning.yml`. See CONTRIBUTING.md ("Dependency cooldown").
 

flip-api/pyproject.toml

@@ -36,7 +36,7 @@ dependencies = [
     "cryptography>=46.0.7",
     "python-dotenv>=1.2.2",
     "psycopg2-binary>=2.9.10",
-    "sqlparse>=0.5.4",
+    "sqlglot>=26.33.0",
     "pydantic-settings>=2.9.1",
     "apscheduler>=3.11.0",
     "slowapi>=0.1.9",

flip-api/src/flip_api/cohort_services/submit_cohort_query.py

@@ -11,13 +11,12 @@
 #
 
 import json
-import re
 from uuid import UUID
 
-# SQL parser library - would need Python equivalent
-# For this example using sqlparse, but may need a more robust solution
-import sqlparse  # type: ignore[import]
+import sqlglot
+import sqlglot.expressions
 from fastapi import APIRouter, Body, Depends, HTTPException, Request
+from sqlglot.errors import SqlglotError
 from sqlmodel import Session, select
 
 from flip_api.auth.access_manager import can_modify_project
@@ -37,58 +36,83 @@
 router = APIRouter(prefix="/cohort", tags=["cohort_services"])
 
 
-FORBIDDEN_COMMANDS = [
-    "alter user",
-    "alter table",
-    "alter database",
-    "drop table",
-    "drop user",
-    "drop role",
-    "drop database",
-    "create table",
-    "substring",
-]
+# Reject pathologically large queries before the parser allocates an AST. Kept
+# equal to the trust-side MAX_QUERY_LENGTH so the hub does not accept a query
+# that every trust would then refuse.
+MAX_QUERY_LENGTH = 10_240  # 10 KiB
 
-# Match any occurrence of any forbidden commands
-REGEX = re.compile(f"({'|'.join(FORBIDDEN_COMMANDS)})", re.IGNORECASE)
-
-
-def contains_forbidden_commands(query: str) -> bool:
-    """
-    Check if the query contains any forbidden commands
-
-    Args:
-        query: SQL query string
-
-    Returns:
-        bool: True if the query contains forbidden commands, False otherwise
-    """
-    return bool(REGEX.search(query))
+# Top-level statement shapes that count as SELECT-like for the cohort API.
+_SELECT_LIKE_STATEMENTS: tuple[type[sqlglot.expressions.Expression], ...] = (
+    sqlglot.expressions.Select,
+    sqlglot.expressions.Union,
+    sqlglot.expressions.Intersect,
+    sqlglot.expressions.Except,
+)
 
 
 def validate_query(query: str) -> None:
     """
-    Validate the SQL query syntax
+    Fast-feedback validity pre-check for a cohort query. **Not a security control.**
+
+    Why this exists at all
+    ----------------------
+    The trust-side ``data-access-api`` is the *authority* on what a cohort query
+    may do (see ``data_access_api.services.cohort.validate_query``), and it is
+    deliberately self-sufficient: a trust holds patient data and must stay safe
+    regardless of what the hub did or did not check, because the hub is a
+    separate administrative domain that the trust does not trust. Anything this
+    function enforced for safety would therefore have to be enforced trust-side
+    anyway, so duplicating the trust's rules here would buy no security — only
+    two copies of one policy to keep in sync.
+
+    What it buys instead is **fast feedback**. Submitting a cohort query fans it
+    out to every registered trust as an encrypted task, each of which runs its
+    own validation asynchronously and reports back. Without a pre-check, a
+    researcher who typos their SQL waits for that whole round-trip, across N
+    trusts, to be told. Catching "this cannot possibly succeed anywhere" while
+    the request is still in-hand turns a multi-minute fan-out into a 400.
+
+    The asymmetry is intentional and safe by construction
+    -----------------------------------------------------
+    This check is deliberately *weaker* than the trust's, and only ever rejects
+    queries that every trust would reject too. It intentionally does **not**
+    enforce trust-local policy — the ``omop`` schema pin, literal-``LIMIT``
+    rule, read-only role, or minimum-cohort threshold — because those depend on
+    facts the hub has no authority over and which may legitimately differ per
+    trust.
+
+    That makes drift between the two harmless in the direction that matters: a
+    hub lagging behind the trust merely wastes a fan-out on a query the trust
+    then refuses, never a bypass. Do not "fix" the asymmetry by copying the
+    trust's rules in here.
 
     Args:
-        query: SQL query string
+        query (str): The SQL query string submitted by the researcher.
 
     Raises:
-        ValueError: If the query is not valid SQL
+        ValueError: If the query is over length, unparseable, not exactly one
+            statement, or not SELECT-shaped.
     """
+    if len(query) > MAX_QUERY_LENGTH:
+        raise ValueError(f"Query exceeds maximum length of {MAX_QUERY_LENGTH} characters.")
+
     try:
-        # Use sqlparse to verify SQL validity
-        # This is a simplified version - you might need a more robust parser
-        # TODO: Replace with a more robust SQL parser if needed.
-        # TODO: Create tests to veryfy if sql injection can be done
-        parsed = sqlparse.parse(query)
-        if not parsed:
-            raise ValueError("Empty or invalid SQL")
-
-        logger.info("Query is valid SQL")
-    except Exception as e:
-        logger.error({"message": "Query is not valid SQL.", "error": str(e)})
-        raise ValueError("Invalid SQL Query")
+        statements = sqlglot.parse(query, read="postgres")
+    except SqlglotError as e:
+        # SqlglotError covers both ParseError and TokenError (e.g. an
+        # unterminated string literal), so a tokenizer failure cannot bubble up
+        # as an unhandled 500.
+        logger.info({"message": "Cohort query rejected: could not be parsed as SQL.", "error": str(e)})
+        raise ValueError("Query could not be parsed as SQL.") from e
+
+    # sqlglot yields None for empty input and for stray semicolons (``SELECT 1; ;``
+    # parses to ``[Select, None]``), so filtering None out would let a caller
+    # smuggle an extra statement past the count check.
+    if len(statements) != 1 or statements[0] is None:
+        raise ValueError("Exactly one SQL statement is allowed per request.")
+
+    if not isinstance(statements[0], _SELECT_LIKE_STATEMENTS):
+        raise ValueError("Only SELECT statements are allowed.")
 
 
 # TODO [#114] This endpoint was not defined in the old repo. The old repo defined a step function that ran the
@@ -123,11 +147,9 @@ def submit_cohort_query(
                 detail=f"User with ID: {user_id} is not allowed to modify this project",
             )
 
-        # Additional validation
-        if contains_forbidden_commands(cohort_query.query):
-            raise HTTPException(status_code=400, detail="Invalid query: Contains forbidden SQL commands")
-
-        # Validate SQL syntax
+        # Fast-feedback validity pre-check only — the trust is the authority on
+        # query safety. See validate_query's docstring for why this is
+        # deliberately weaker than the trust-side check.
         try:
             validate_query(cohort_query.query)
         except ValueError as e:

flip-api/tests/unit/cohort_services/test_submit_cohort_query.py

@@ -16,7 +16,11 @@
 import pytest
 from fastapi import HTTPException, Request
 
-from flip_api.cohort_services.submit_cohort_query import submit_cohort_query
+from flip_api.cohort_services.submit_cohort_query import (
+    MAX_QUERY_LENGTH,
+    submit_cohort_query,
+    validate_query,
+)
 from flip_api.db.models.main_models import TrustTask
 from flip_api.domain.schemas.cohort import SubmitCohortQuery
 from flip_api.domain.schemas.status import TaskType
@@ -155,22 +159,80 @@ def test_submit_cohort_query_warns_when_query_row_missing(
     assert "queried_trust_ids was not persisted" in caplog.text
 
 
-@patch("flip_api.cohort_services.submit_cohort_query.can_modify_project", return_value=True)
-def test_submit_cohort_query_forbidden_sql(mock_can_modify, mock_auth_request):
-    """Queries with forbidden SQL commands should be rejected."""
-    query = SubmitCohortQuery(
-        name="Hack",
-        query="DROP TABLE patients;",
+def _query(sql: str) -> SubmitCohortQuery:
+    """Build a SubmitCohortQuery carrying ``sql``, for the pre-check tests below."""
+    return SubmitCohortQuery(
+        name="Test Query",
+        query=sql,
         project_id=project_id,
         query_id=query_id,
-        authenticationToken=mock_auth_request.headers.get("Authorization", ""),
+        authenticationToken="Bearer test-token",
     )
 
+
+@patch("flip_api.cohort_services.submit_cohort_query.can_modify_project", return_value=True)
+def test_submit_cohort_query_rejects_non_select(mock_can_modify, mock_auth_request):
+    """Statements that are not SELECT-shaped are rejected by the hub pre-check."""
+    with pytest.raises(HTTPException) as exc_info:
+        submit_cohort_query(mock_auth_request, _query("DROP TABLE patients;"), MagicMock(), user_id)
+
+    assert exc_info.value.status_code == 400
+    assert "SELECT" in str(exc_info.value.detail)
+
+
+@patch("flip_api.cohort_services.submit_cohort_query.can_modify_project", return_value=True)
+def test_submit_cohort_query_rejects_unparseable_sql(mock_can_modify, mock_auth_request):
+    """Input that is not SQL at all is rejected.
+
+    The previous sqlparse-based check accepted this: ``sqlparse.parse`` is a
+    non-validating tokenizer and returns a truthy result for arbitrary text.
+    """
+    with pytest.raises(HTTPException) as exc_info:
+        submit_cohort_query(mock_auth_request, _query("$$$$ not sql at all !!!"), MagicMock(), user_id)
+
+    assert exc_info.value.status_code == 400
+
+
+@patch("flip_api.cohort_services.submit_cohort_query.can_modify_project", return_value=True)
+def test_submit_cohort_query_rejects_stacked_statements(mock_can_modify, mock_auth_request):
+    """Query stacking is rejected — only one statement may be submitted."""
+    with pytest.raises(HTTPException) as exc_info:
+        submit_cohort_query(
+            mock_auth_request, _query("SELECT 1; DROP TABLE patients"), MagicMock(), user_id
+        )
+
+    assert exc_info.value.status_code == 400
+
+
+@patch("flip_api.cohort_services.submit_cohort_query.can_modify_project", return_value=True)
+def test_submit_cohort_query_rejects_oversized_query(mock_can_modify, mock_auth_request):
+    """Pathologically large queries are rejected before the parser allocates an AST."""
+    oversized = f"SELECT {'a' * (MAX_QUERY_LENGTH + 1)} FROM omop.person"
+
     with pytest.raises(HTTPException) as exc_info:
-        submit_cohort_query(mock_auth_request, query, MagicMock(), user_id)
+        submit_cohort_query(mock_auth_request, _query(oversized), MagicMock(), user_id)
 
     assert exc_info.value.status_code == 400
-    assert "forbidden SQL commands" in str(exc_info.value.detail)
+    assert "length" in str(exc_info.value.detail).lower()
+
+
+def test_validate_query_accepts_substring_function():
+    """``SUBSTRING()`` is legitimate SQL and must not be rejected.
+
+    The removed denylist contained the bare token "substring", so every query
+    using the standard function was refused — the flexibility cost of a keyword
+    denylist. Blind data extraction via ``SUBSTRING`` is defeated trust-side by
+    the literal-LIMIT rule, not by banning the function name.
+    """
+    validate_query("SELECT SUBSTRING(gender_source_value, 1, 1) FROM omop.person")
+
+
+def test_validate_query_accepts_cte_with_set_operation():
+    """Real cohort queries use CTEs and set operations; both are SELECT-shaped."""
+    validate_query(
+        "WITH a AS (SELECT person_id FROM omop.person) "
+        "SELECT person_id FROM a UNION SELECT person_id FROM omop.observation"
+    )
 
 
 @patch("flip_api.cohort_services.submit_cohort_query.can_modify_project", return_value=True)