fix(security): reject SELECT..INTO / volatile-function bypass of the SQL read-only gate + clear base Semgrep + JWKS scheme hardening

docs/doctoring/sql-readonly-function-admission.md

@@ -0,0 +1,37 @@
+# SQL read-only function admission boundary
+
+## Decision
+
+`validate_sql_query()` treats a user-supplied PostgreSQL function call as executable authority, not as inert SELECT syntax. The query gate therefore uses a **positive allowlist** for reviewed read-only aggregate functions (`avg`, `count`, `max`, `min`, and `sum`) and rejects every other function call with `unsafe_function_call`. Schema-qualified function calls are rejected even when their final identifier resembles an allowlisted function, because qualification can select a different implementation.
+
+This replaces the earlier unsafe-function denylist. The denylist could block known families such as `pg_sleep` or `dblink` but could not establish a closed read-only boundary: `nextval`, `setval`, large-object functions, advisory locks, newly installed extensions, and future side-effecting functions remained open unless every name was anticipated.
+
+## Security rationale
+
+PostgreSQL classifies functions as `IMMUTABLE`, `STABLE`, or `VOLATILE`. A `VOLATILE` function may modify the database, and functions with side effects must be classified `VOLATILE`. PostgreSQL also documents `setval()` as an example of a side-effecting function. Consequently, the lexical fact that a statement begins with `SELECT` is not proof that executing it is read-only.
+
+The application gate deliberately does not attempt to reconstruct PostgreSQL's full catalog, extension state, overload resolution, or volatility semantics from user text. Instead, it admits only the small function set required by the product's governed analytic contract. New function capability must be added test-first after reviewing its PostgreSQL semantics and operational resource bounds.
+
+## Test-first evidence
+
+The RED head `12725aad0a2253de3d146c5e34c9409744c5f3f2` added `tests/test_sql_readonly_function_gate.py` before production changed. Exact-head Tests run `31184015453` failed eight adversarial cases because `nextval`, `setval`, `lo_creat`, `lo_get`, `lo_lseek`, `pg_advisory_lock`, a schema-qualified advisory-lock call, and an unknown extension function passed with no `unsafe_function_call` warning.
+
+GREEN head `8d6facbcd3a05c8a21fbf4453408e732dff3a8cd` replaced the open-ended unsafe-name list with positive function admission. The same regression preserves ordinary aggregate analytics through positive cases for `count`, `sum`, `avg`, `min`, and `max`.
+
+## Layered enforcement and limitations
+
+This lexical gate is an application safety boundary, not a substitute for database authority. When a real PostgreSQL execution backend is enabled, the connection role should still use least privilege, an explicit read-only transaction where compatible with the execution design, bounded statement timeouts, and no unnecessary sequence, large-object, extension, file, or administrative privileges. PostgreSQL documents transaction access mode separately; a read-only transaction prevents ordinary non-temporary-table writes, while product authorization and function admission remain independent controls.
+
+The current product execution path remains mock-backed. Therefore this change proves fail-closed application admission semantics, not production PostgreSQL privilege isolation or end-to-end database non-interference. Those require a live restricted-role integration test before a real query engine is enabled.
+
+## Rollback
+
+Do not roll back to prefix/denylist function filtering. If a required analytic function is rejected, add a failing compatibility regression, review the exact function semantics and resource behavior, extend the smallest positive allowlist, and rerun the complete exact-head security and coverage gates. If safe semantics cannot be established, keep the function unavailable.
+
+## References
+
+PostgreSQL Global Development Group. (2026). *Function volatility categories*. In *PostgreSQL 18 documentation*. https://www.postgresql.org/docs/18/xfunc-volatility.html
+
+PostgreSQL Global Development Group. (2026). *CREATE FUNCTION*. In *PostgreSQL 18 documentation*. https://www.postgresql.org/docs/18/sql-createfunction.html
+
+PostgreSQL Global Development Group. (2026). *SET TRANSACTION*. In *PostgreSQL 18 documentation*. https://www.postgresql.org/docs/18/sql-set-transaction.html

src/sdp/orchestrator.py

@@ -10,6 +10,16 @@
 
 
 _FORBIDDEN_KEYWORDS = {"drop", "delete", "truncate", "alter", "insert", "update", "merge", "exec", "union"}
+# Function admission is deliberately positive rather than denylist-based. PostgreSQL
+# exposes many built-ins and extensions that can mutate state, consume unbounded
+# resources, access files, or affect other sessions while still appearing inside a
+# SELECT. A finite unsafe-name list therefore cannot establish a read-only boundary.
+# Keep the product surface limited to the aggregate functions required by governed
+# analytics; every other function call fails closed until reviewed and added here.
+_SAFE_READONLY_FUNCTIONS = frozenset({"avg", "count", "max", "min", "sum"})
+_FUNCTION_CALL_PATTERN = re.compile(
+    r"(?<![\w.])(?P<name>[A-Za-z_][\w]*(?:\.[A-Za-z_][\w]*)*)\s*\("
+)
 
 
 def _safe_identifier(value: str) -> str:
@@ -24,6 +34,15 @@ def _source_table_name(source_system: str) -> str:
     return _safe_identifier(source_system.rstrip("/").rsplit("/", 1)[-1])
 
 
+def _has_unreviewed_function_call(sql: str) -> bool:
+    """Return whether SQL invokes a function outside the reviewed read-only set."""
+    for match in _FUNCTION_CALL_PATTERN.finditer(sql):
+        function_name = match.group("name").lower()
+        if "." in function_name or function_name not in _SAFE_READONLY_FUNCTIONS:
+            return True
+    return False
+
+
 def validate_sql_query(sql: str, *, source_system: str) -> list[str]:
     stripped = sql.strip()
     lowered = stripped.lower()
@@ -45,6 +64,15 @@ def validate_sql_query(sql: str, *, source_system: str) -> list[str]:
             warnings.append("forbidden_keyword_detected")
             break
 
+    # SELECT ... INTO is CREATE TABLE AS (a write/DDL). Function calls are a
+    # separate grammar boundary: only the reviewed aggregate allowlist above is
+    # accepted, so newly installed PostgreSQL extensions cannot silently expand
+    # query authority.
+    if re.search(r"\binto\b", lowered):
+        warnings.append("write_operation_not_allowed")
+    if _has_unreviewed_function_call(stripped):
+        warnings.append("unsafe_function_call")
+
     referenced = [
         next(value for value in match if value)
         for match in re.findall(r"\bfrom\s+([A-Za-z_][\w.]*)|\bjoin\s+([A-Za-z_][\w.]*)", stripped, re.IGNORECASE)

tests/test_api.py

@@ -1508,6 +1508,72 @@ def test_browse_query_rejects_table_outside_dataset_binding():
     )
 
 
+def test_browse_query_rejects_select_into_write():
+    # SELECT ... INTO is CREATE TABLE AS -- a write that begins with "select",
+    # so the read-only prefix check alone let it reach a 200/SUCCEEDED before.
+    response = client.post(
+        "/browse/query",
+        json={
+            "user": "analyst",
+            "purpose": "analysis",
+            "dataset_ids": ["crm-event"],
+            "language": "SQL",
+            "query": "SELECT * INTO exfil FROM crm",
+        },
+    )
+    assert response.status_code == 400
+    assert "write_operation_not_allowed" in response.json()["detail"]["warnings"]
+
+
+def test_validate_sql_query_blocks_writes_and_volatile_functions():
+    from sdp.orchestrator import validate_sql_query
+
+    source = "s3://analytics/events/crm"  # -> allowlisted table "crm"
+    assert "write_operation_not_allowed" in validate_sql_query(
+        "SELECT * INTO exfil FROM crm", source_system=source
+    )
+    assert "unsafe_function_call" in validate_sql_query(
+        "SELECT pg_sleep(10) FROM crm", source_system=source
+    )
+    assert "unsafe_function_call" in validate_sql_query(
+        "SELECT pg_read_file(path_col) FROM crm", source_system=source
+    )
+    # Family variants must be caught too: a word-boundary match on the bare
+    # name alone would let every suffixed sibling through (dblink_exec,
+    # dblink_connect_u, pg_sleep_for, pg_ls_waldir, pg_read_binary_file).
+    for family_variant_sql in (
+        "SELECT dblink_exec(conn_col, cmd_col) FROM crm",
+        "SELECT dblink_connect_u(conn_col) FROM crm",
+        "SELECT pg_sleep_for(interval_col) FROM crm",
+        "SELECT pg_ls_waldir() FROM crm",
+        "SELECT pg_read_binary_file(path_col) FROM crm",
+    ):
+        assert "unsafe_function_call" in validate_sql_query(
+            family_variant_sql, source_system=source
+        ), family_variant_sql
+    # Session/server-control and large-object families: side effects reachable
+    # from a plain SELECT (session DoS, config reload, WAL control, large-object
+    # deletion/mutation) must be rejected the same way.
+    for control_sql in (
+        "SELECT pg_terminate_backend(pid_col) FROM crm",
+        "SELECT pg_cancel_backend(pid_col) FROM crm",
+        "SELECT pg_reload_conf() FROM crm",
+        "SELECT pg_switch_wal() FROM crm",
+        "SELECT pg_create_restore_point(name_col) FROM crm",
+        "SELECT pg_export_snapshot() FROM crm",
+        "SELECT lo_unlink(oid_col) FROM crm",
+        "SELECT lo_put(oid_col, 0, data_col) FROM crm",
+        "SELECT lo_truncate(fd_col, 0) FROM crm",
+        "SELECT lo_open(oid_col, 131072) FROM crm",
+        "SELECT lo_create(0) FROM crm",
+    ):
+        assert "unsafe_function_call" in validate_sql_query(
+            control_sql, source_system=source
+        ), control_sql
+    # A clean read-only SELECT on the bound table stays warning-free.
+    assert validate_sql_query("SELECT id FROM crm", source_system=source) == []
+
+
 def test_browse_query_rejects_literal_tautology_injection():
     response = client.post(
         "/browse/query",

tests/test_sql_readonly_function_gate.py

@@ -0,0 +1,39 @@
+"""Regression contracts for fail-closed SQL function admission."""
+
+import pytest
+
+from sdp.orchestrator import validate_sql_query
+
+
+@pytest.mark.parametrize(
+    "query",
+    [
+        "SELECT nextval(16385) FROM crm",
+        "SELECT setval(16385, 1) FROM crm",
+        "SELECT lo_creat(0) FROM crm",
+        "SELECT lo_get(16386) FROM crm",
+        "SELECT lo_lseek(16386, 0) FROM crm",
+        "SELECT pg_advisory_lock(1) FROM crm",
+        "SELECT pg_catalog.pg_advisory_lock(1) FROM crm",
+        "SELECT unreviewed_extension_function(1) FROM crm",
+    ],
+)
+def test_readonly_gate_rejects_unreviewed_function_calls(query: str) -> None:
+    """Any function outside the reviewed read-only allowlist must fail closed."""
+    warnings = validate_sql_query(query, source_system="crm")
+    assert "unsafe_function_call" in warnings
+
+
+@pytest.mark.parametrize(
+    "query",
+    [
+        "SELECT count(*) FROM crm",
+        "SELECT sum(amount) FROM crm",
+        "SELECT avg(amount) FROM crm",
+        "SELECT min(amount) FROM crm",
+        "SELECT max(amount) FROM crm",
+    ],
+)
+def test_readonly_gate_accepts_reviewed_aggregate_functions(query: str) -> None:
+    """Reviewed aggregate functions remain available for ordinary analytics."""
+    assert validate_sql_query(query, source_system="crm") == []