[Task]: FLIP-PT pentest remediation tracker — Final Report v2.0 (2026-05-06)

AGENTS.md

@@ -437,6 +437,7 @@ TruffleHog, detect-secrets, large file check (max 1000KB), merge conflict marker
 - 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).
+- Logging policy (`docs/source/sys-admin.rst`, "Logging policy" / `#logging-policy`): sensitive values never appear in logs at **any** level — cohort SQL (log the 12-hex SHA-256 fingerprint from each service's `utils/log_hygiene.py`; raw driver/parser error text embeds the statement, so log exception class + SQLSTATE + fingerprint instead — both SQLAlchemy engines also set `hide_parameters=True`), accession numbers / patient-level attributes (ordinals, counts, or `flip.utils.Utils.hash_for_log`; response bodies from trust data/imaging APIs are patient-level and never logged), secrets (no request headers or bodies, no presigned URLs), full URLs (host + path only, query string always dropped), and verbatim S3 keys (bucket + `hash_s3_key` prefix, or the model-id/file-name identifiers the key derives from). Pin new log lines with a caplog test asserting the sensitive value stays out.
 - 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

@@ -437,6 +437,7 @@ TruffleHog, detect-secrets, large file check (max 1000KB), merge conflict marker
 - 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).
+- Logging policy (`docs/source/sys-admin.rst`, "Logging policy" / `#logging-policy`): sensitive values never appear in logs at **any** level — cohort SQL (log the 12-hex SHA-256 fingerprint from each service's `utils/log_hygiene.py`; raw driver/parser error text embeds the statement, so log exception class + SQLSTATE + fingerprint instead — both SQLAlchemy engines also set `hide_parameters=True`), accession numbers / patient-level attributes (ordinals, counts, or `flip.utils.Utils.hash_for_log`; response bodies from trust data/imaging APIs are patient-level and never logged), secrets (no request headers or bodies, no presigned URLs), full URLs (host + path only, query string always dropped), and verbatim S3 keys (bucket + `hash_s3_key` prefix, or the model-id/file-name identifiers the key derives from). Pin new log lines with a caplog test asserting the sensitive value stays out.
 - 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").
 

docs/source/components/component-logging-stack.rst

@@ -52,6 +52,11 @@ pre-provisioned datasource.
 Application Logging
 *******************
 
+.. seealso::
+
+   What may appear in a log line — and what never may — is governed by the
+   platform :ref:`logging-policy`.
+
 Shared library: ``log_config``
 ================================
 

docs/source/sys-admin.rst

@@ -22,3 +22,72 @@ System administration
    ``deploy/providers/kubernetes/`` and the
    `K8s README <https://github.com/londonaicentre/FLIP/blob/develop/deploy/providers/kubernetes/README.md>`_
    for operational notes, troubleshooting, and configuration reference.
+
+.. _logging-policy:
+
+**************
+Logging policy
+**************
+
+FLIP handles patient-level data, and log pipelines have far weaker access
+control than the data paths they sit beside: trust logs are aggregated into the
+observability stack, hub logs into CloudWatch, and FL run logs can be read by
+model developers outside the trust. Every service therefore follows one rule
+set for what may appear in a log line, at **every** log level — ``DEBUG``
+included, since a dev-tuned log level must never change what can leak.
+
+What is never logged
+====================
+
+**Cohort SQL.** A cohort query encodes patient-level selection criteria.
+Log its *fingerprint* instead: the SHA-256 of the whitespace-normalised,
+lower-cased query, truncated to 12 hex chars (``query sha256:ab12…``). Every
+service carries the same helper (``utils/log_hygiene.py`` in flip-api,
+data-access-api, and imaging-api) with the same normalisation, so hub and
+trust log lines for one query carry one fingerprint — and because the hub
+stores every cohort query, an operator can re-hash the stored SQL to find the
+log lines it produced. This also means raw driver/parser error text
+(psycopg2's ``LINE 1:`` context, SQLAlchemy's ``[SQL: …]`` suffix, sqlglot's
+quoted fragment) never reaches a log — log the exception class, the SQLSTATE
+where available, and the fingerprint. Both SQLAlchemy engines are additionally
+built with ``hide_parameters=True`` so bound parameter values never render in
+wrapped driver errors.
+
+**Accession numbers and any patient-level identifier or attribute.** Log
+ordinals (``accession 3/17``), counts, or a fingerprint
+(``flip.utils.Utils.hash_for_log``) — never the value. Response *bodies* from
+the trust data and imaging APIs are patient-level material (a cohort dataframe,
+DQR study metadata) and are never logged; log the status code and row/object
+counts only.
+
+**Secrets and credentials.** No request headers (they carry the trust-internal
+service key and XNAT tokens), no request bodies, no presigned URLs in any form.
+
+**Full URLs.** Log scheme-less host + path only; the query string is always
+dropped (``encoded_query`` is base64-wrapped SQL). Exception text from HTTP
+client libraries interpolates the full URL, so those messages are reduced to
+the exception class before logging.
+
+**S3 object keys.** Log the bucket plus either a SHA-256 prefix of the key
+(``flip_api.utils.s3_client.hash_s3_key``, which hashes exact bytes — keys are
+case-sensitive) or the platform identifiers the key is derived from (model id,
+uploaded file name — both already user-visible metadata). Never the verbatim
+key or full ``s3://`` path, and never a presigned form of it.
+
+Working with scrubbed logs
+==========================
+
+Fingerprints are correlation handles, not redaction theatre: the party that
+legitimately holds a value (the hub's stored cohort SQL, a model developer's
+own accession list, a caller's S3 key) can re-derive the fingerprint and grep
+for it. When you need more context than a class name — for example a failing
+cohort query — re-run the stored query against the target system rather than
+relaxing a log line.
+
+HTTP **error response bodies** are governed separately (category-only details;
+see the cohort validation notes in
+`trust/data-access-api/README.md <https://github.com/londonaicentre/FLIP/blob/develop/trust/data-access-api/README.md>`_)
+— this policy covers what lands in logs. When adding a log line, assume it will
+be read by someone who must not see patient data, and pin the behaviour with a
+unit test asserting the sensitive value stays out of ``caplog`` (see
+``tests/services/test_cohort.py`` in data-access-api for the pattern).

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

@@ -105,5 +105,7 @@ def save_cohort_query(
         # Re-raise HTTP exceptions
         raise
     except Exception as e:
-        logger.error(f"Error saving cohort query: {str(e)}")
+        # Class-only: a DB error on this path renders the INSERT whose bound
+        # parameters include the raw cohort SQL (logging policy).
+        logger.error(f"Error saving cohort query: {type(e).__name__}")
         raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}")

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

@@ -31,6 +31,7 @@
 )
 from flip_api.domain.schemas.status import TaskType
 from flip_api.utils.encryption import encrypt
+from flip_api.utils.log_hygiene import hash_query
 from flip_api.utils.logger import logger
 
 router = APIRouter(prefix="/cohort", tags=["cohort_services"])
@@ -101,8 +102,13 @@ def validate_query(query: str) -> None:
     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)})
+        # as an unhandled 500. Class + fingerprint only: sqlglot interpolates
+        # the offending SQL fragment into its message (logging policy).
+        logger.info({
+            "message": "Cohort query rejected: could not be parsed as SQL.",
+            "error": type(e).__name__,
+            "query_sha256": hash_query(query),
+        })
         raise ValueError("Query could not be parsed as SQL.") from e
 
     # sqlglot yields None for empty input and for stray semicolons (``SELECT 1; ;``
@@ -242,5 +248,7 @@ def submit_cohort_query(
         raise
     except Exception as e:
         db.rollback()
-        logger.error(f"Error submitting cohort query: {str(e)}")
+        # Class-only: a DB error on this path renders the INSERT whose bound
+        # parameters include the raw cohort SQL (logging policy).
+        logger.error(f"Error submitting cohort query: {type(e).__name__}")
         raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}")

flip-api/src/flip_api/db/database.py

@@ -125,6 +125,11 @@ def _build_engine() -> Engine:
         engine = create_engine(
             db_url,
             echo=False,
+            # Keep bound parameter values (cohort SQL, user emails, ...) out of
+            # SQLAlchemy error text, which otherwise renders them in a
+            # ``[parameters: ...]`` suffix on every wrapped driver error
+            # (logging policy).
+            hide_parameters=True,
             pool_pre_ping=True,
             pool_recycle=_POOL_RECYCLE_SECONDS,
             connect_args={"sslmode": "require"},
@@ -140,7 +145,8 @@ def _build_engine() -> Engine:
     # encoded, which the default quote(safe="/") would not.
     encoded_password = quote(stt.POSTGRES_PASSWORD.strip(), safe="")
     db_url = f"postgresql+psycopg2://{stt.POSTGRES_USER}:{encoded_password}@{stt.DB_HOST}:{stt.DB_PORT}/{stt.POSTGRES_DB}"
-    return create_engine(db_url, echo=False)
+    # hide_parameters for the same reason as the production engine above.
+    return create_engine(db_url, echo=False, hide_parameters=True)
 
 
 # Lazily-created, reused engine. Built on first use rather than at import time

flip-api/src/flip_api/model_services/retrieve_model.py

@@ -149,13 +149,14 @@ def retrieve_model(
             ModelStatus(result["status"]) if result["status"] in ModelStatus.__members__ else ModelStatus.ERROR
         )
 
-        # Parse query from SQL result
+        # Parse query from SQL result. Log id-only: the IQuery repr carries the
+        # raw cohort SQL (logging policy).
         query = parse_query_from_result(result.get("query")) if result.get("query") else None
-        logger.debug(f"Parsed query: {query}")
+        logger.debug(f"Parsed query: {query.id if query else None}")
 
         # Parse files from SQL result
         files = parse_files_from_result(result.get("files"), model_id) if result.get("files") else []
-        logger.debug(f"Parsed files: {files}")
+        logger.debug(f"Parsed {len(files)} files")
 
         # Lifecycle timestamps — one tiny query for creation_timestamp, one for
         # the tracked audit dates. Cheap; the raw SQL was already opinionated

flip-api/src/flip_api/step_functions_services/cohort_query_step_function.py

@@ -91,7 +91,10 @@ def cohort_query_step_function_endpoint(
         # Re-raise HTTP exceptions
         raise
     except Exception as e:
-        logger.exception(f"Unhandled error in cohort_query: {str(e)}")
+        # No ``str(e)`` in the message: the traceback logger.exception emits
+        # already carries the detail, and the hub engine's ``hide_parameters``
+        # keeps the cohort SQL bound into DB statements out of it (logging policy).
+        logger.exception("Unhandled error in cohort_query")
         raise HTTPException(
             status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=f"Failed to process cohort query: {str(e)}"
         )

flip-api/src/flip_api/utils/log_hygiene.py

@@ -0,0 +1,37 @@
+# Copyright (c) Guy's and St Thomas' NHS Foundation Trust & King's College London
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#     http://www.apache.org/licenses/LICENSE-2.0
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+import hashlib
+
+
+def hash_query(query: object) -> str:
+    """Returns a short, stable fingerprint of a SQL query for log correlation.
+
+    Cohort SQL encodes patient-level selection criteria, so the platform logging
+    policy (``docs/source/sys-admin.rst``, "Logging policy") keeps it out of logs
+    entirely. Log this fingerprint instead: the SHA-256 of the whitespace-normalised,
+    lower-cased query, truncated to 12 hex chars. The normalisation matches the
+    trust services' ``utils.log_hygiene.hash_query``, so hub and trust log lines
+    for the same cohort query carry the same fingerprint — and since the hub
+    stores every cohort query, an operator can re-hash the stored SQL to find
+    the log lines it produced. (S3 keys have their own helper:
+    ``utils.s3_client.hash_s3_key``, which hashes the exact bytes because keys
+    are case-sensitive.)
+
+    Args:
+        query: The SQL query, as a string or anything whose ``str()`` is the SQL text.
+
+    Returns:
+        str: A 12-hex-char fingerprint of the query.
+    """
+    normalised = " ".join(str(query).strip().lower().split())
+    return hashlib.sha256(normalised.encode()).hexdigest()[:12]