keep Athena queries out of auto-approve unless the target allows it

An Athena archive query is reviewed even for a waiver holder: waivers and the
fingerprint cache do not apply unless the target sets
engine_config.auto_approve to true. What an archive query costs depends on the
partitions it reads, so a fleet-wide waiver, or a fingerprint that ignores
literal values, would let the expensive variant of a cheap, once-approved query
run unreviewed. A super-admin's own submission is unchanged. Every caller that
decides or announces auto-approval asks one helper,
engines.auto_approve_allowed, and a test scans the package for callers that
skip it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: ahmetrende

CHANGELOG.md

@@ -161,6 +161,10 @@ frontend and the endpoints it calls are explicitly outside it.
 
 ### Fixed
 
+- **Athena archive queries are reviewed, even for someone with auto-approve.**
+  A fleet-wide waiver or a fingerprint match would have run them with no
+  review. To allow it, set `engine_config.auto_approve` to `true` on the target
+  (docs/CONFIGURATION.md). A super-admin's own query is unchanged.
 - **"All databases" in the web grant forms wrote a grant on a database named
   `*`.** It matched nothing. Any spelling of every database (`*`, empty, `all`,
   `any`) now means every database, for person and team grants alike.

docs/CONFIGURATION.md

@@ -134,7 +134,7 @@ default rather than stopping the process.
 
 | Key | Default | What it does |
 |---|---|---|
-| `fingerprint_cache_enabled` | `on` | Auto-approve a re-submission identical to a previously approved query. |
+| `fingerprint_cache_enabled` | `on` | Auto-approve a re-submission identical to a previously approved query. Not on an Athena target unless it allows auto-approve (see "Amazon Athena targets"). |
 | `fingerprint_cache_ttl_days` | `30` | How long a fingerprint stays eligible for auto-approve. |
 | `require_justification` | `false` | Require a justification note on every submission. |
 | `max_open_access_requests_per_user` | `5` | Cap on pending target-access requests per user. |
@@ -228,6 +228,22 @@ described by `target_servers.engine_config`, a JSON object per target:
 | `role_arn` | yes | Read-only role the gateway assumes for every call: Glue, Athena and S3. |
 | `catalog` | no | Data catalog. Default `AwsDataCatalog`. |
 | `freshness_marker` | no | `s3://bucket/key` of the archive's freshness marker. When set, the approver's hint says how far the archive reaches. |
+| `auto_approve` | no | Default `false`: auto-approve waivers and the fingerprint approval cache do not apply, so every query goes to an approver. Set `true` to let them apply. |
+
+Auto-approve is off on Athena because a query's cost depends on the partitions
+it reads, and neither a waiver nor a fingerprint match sees which ones. Only a
+JSON `true` or `false` counts; any other value means the default. A
+super-admin's own query is auto-approved either way. With auto-approve off,
+neither the Slack badge nor the web editor and connection list say a query
+will skip review, and a request for an auto-approve window on the target is
+refused. The key works the same on a PostgreSQL, SQL Server or ClickHouse
+target, where the default is `true`.
+
+```sql
+UPDATE target_servers
+   SET engine_config = COALESCE(engine_config, '{}') || '{"auto_approve": true}'
+ WHERE alias = 'example-archive';
+```
 
 The freshness marker is one JSON object that the archive writes with a single
 PutObject:

src/queryhub/auto_approve_requests.py

@@ -113,12 +113,13 @@ def submit_window(*, principal_id: str, name: str | None, target_id: int,
     enforces the same ones: the bot is not halted, the window is one of the
     offered lengths, the reason says something, the requester can reach the
     target (and the database, when one is named -- the approval carries it into
-    the waiver), and there is not already a request pending for it.
+    the waiver), there is not already a request pending for it, and the target
+    lets a waiver apply at all.
 
     Returns (row, target). Notifying the admins is the caller's, because it
     needs a Slack client and the two surfaces hold theirs differently.
     """
-    from . import auto_approve, core_submit, targets, teams
+    from . import auto_approve, core_submit, engines, targets, teams
     if core_submit.kill_switch_on():
         raise WindowRequestRefused("reason", core_submit.kill_switch_message(), 503)
     tier = (tier or "ro").strip().lower()
@@ -152,6 +153,13 @@ def submit_window(*, principal_id: str, name: str | None, target_id: int,
     t = targets.get(target_id)
     if t is None:
         raise WindowRequestRefused("target", "That target no longer exists.", 404)
+    # A window is a waiver, and no waiver applies where the target keeps
+    # auto-approve off (an Athena archive). Approving one would tell the
+    # requester their reads skip review there while every one of them waits.
+    if not engines.auto_approve_allowed(t):
+        raise WindowRequestRefused(
+            "target", "Queries on this connection are always reviewed, so a "
+            "window would not apply.", 409)
     if tier == "rw":
         # A waiver covers what the access allows and no more, so asking to skip
         # review on writes needs write access to start with. Asked of the

src/queryhub/core_submit.py

@@ -25,7 +25,7 @@
 from datetime import datetime, timedelta, timezone
 from zoneinfo import ZoneInfo, ZoneInfoNotFoundError
 
-from . import admins, ast_safety, audit, auto_approve, db, pre_flight
+from . import admins, ast_safety, audit, auto_approve, db, engines, pre_flight
 from . import config as cfg
 from . import profile_sync, query_safety, query_secrets, replicas, requesters, targets, teams
 
@@ -423,16 +423,21 @@ def validate_submission(
     # may expire before the run time, in which case the caller falls back to
     # normal approval and the reason is needed after all. And any error resolving
     # the grant leaves the requirement in place.
+    #
+    # A waiver only exempts where create_request will let it decide: on a target
+    # that keeps auto-approve off (an Athena archive), the request reaches an
+    # approver whatever the requester holds.
     justification = (justification or "").strip() or None
     auto_approve_exempt = False
     if not justification and not (schedule_date or schedule_time):
         try:
             auto_approve_exempt = (
                 admins.is_super_admin(user_id)
-                or auto_approve.effective_grant(
-                    user_id, required_mode,
-                    target_server_id=target_server_id,
-                    database_name=database) is not None)
+                or (engines.auto_approve_allowed(target)
+                    and auto_approve.effective_grant(
+                        user_id, required_mode,
+                        target_server_id=target_server_id,
+                        database_name=database) is not None))
         except Exception:
             log.exception(
                 "auto-approve lookup failed while deciding whether a "
@@ -797,12 +802,26 @@ def create_request(
     #   - if scheduled_for is in the future, also evaluate at that moment;
     #     a grant that expires before the run time must fall back to the
     #     normal approval flow (with a note to the user).
+    #
+    # Neither a waiver nor the fingerprint cache decides on a target that keeps
+    # auto-approve off (an Athena archive, unless its engine_config says
+    # otherwise). The request goes to an approver like anybody else's. The
+    # lookup still runs so that a waiver holder, whose reads skip review
+    # everywhere else, is told why this one waits.
+    aa_allowed = engines.auto_approve_allowed(prep.target)
     aa_now = auto_approve.effective_grant(
         prep.user_id, prep.required_mode,
         target_server_id=prep.target.id, database_name=prep.database,
     )
-    aa_grant = aa_now
     aa_warn = None
+    if aa_now is not None and not aa_allowed:
+        log.info("auto-approve: waiver %s not applied for %s on %s: auto-approve "
+                 "is off for this target (engine %s)", aa_now["id"], prep.user_id,
+                 prep.target.alias, prep.target.engine)
+        aa_now = None
+        aa_warn = (":warning: Auto-approve is off for this connection, so this "
+                   "request needs admin approval.")
+    aa_grant = aa_now
     if aa_now is not None and prep.sched_for is not None:
         aa_at_sched = auto_approve.effective_grant(
             prep.user_id, prep.required_mode,
@@ -828,12 +847,14 @@ def create_request(
     # the fingerprint so this request seeds the cache, but a super-admin
     # already auto-approves as super — matching the cache on top of that
     # would only mislabel the decision as "fingerprint" and send a redundant
-    # auto-approve notification, so skip the lookup for them.
+    # auto-approve notification, so skip the lookup for them. Where
+    # auto-approve is off it is skipped for everybody: the fingerprint ignores
+    # literal values, and on Athena those decide how much a query scans.
     query_fingerprint: str | None = None
     fp_hit: dict | None = None
     if prep.required_mode == "ro":
         query_fingerprint = ast_safety.fingerprint(prep.query, engine=prep.target.engine)
-        if not super_auto and aa_grant is None and query_fingerprint:
+        if not super_auto and aa_grant is None and query_fingerprint and aa_allowed:
             fp_hit = auto_approve.fingerprint_cache_hit(
                 prep.user_id, prep.target.id, prep.database, query_fingerprint,
             )

src/queryhub/engines.py

@@ -38,8 +38,9 @@
     whose Lambda call is a statement PREFIX rather than something buried
     in a SELECT (measured against the parser, 2026-09-19). A federated
     catalog is reached by naming it, so the cross-catalog rule written for
-    SQL Server does that job here unchanged. Spec only; no execution path
-    yet.
+    SQL Server does that job here unchanged. Executes through
+    athena_exec.py. Its queries are reviewed even for a waiver holder,
+    unless the target turns auto-approve on (`auto_approve_allowed`).
 
 `WIRED_ENGINES` gates execution: an engine can carry a spec (so its
 safety profile is enforced the moment a target is tagged with it) before
@@ -142,6 +143,14 @@ class EngineSpec:
     # Read by the schema refresh and by the execution dispatch -- not a label.
     requires_credentials: bool = True
 
+    # --- approval -----------------------------------------------------
+    # Whether an auto-approve waiver or the fingerprint approval cache may
+    # skip review on this engine's targets when the target itself says
+    # nothing. A target overrides it with `engine_config.auto_approve`; read
+    # it through `auto_approve_allowed`, never from here directly. A
+    # super-admin's own submission is a separate rule and does not ask.
+    auto_approve_default: bool = True
+
 
 # Routines a Postgres RO login may call. pg_proc rather than
 # information_schema.routines: the latter hides anything the role cannot execute,
@@ -357,7 +366,7 @@ class EngineSpec:
 
 
 # ---------------------------------------------------------------------------
-# Amazon Athena — read-only (spec only; no execution path yet).
+# Amazon Athena — read-only, executed through athena_exec.py.
 # ---------------------------------------------------------------------------
 #
 # Athena is Trino, so `blocked_functions` is EMPTY on purpose rather than by
@@ -379,6 +388,14 @@ class EngineSpec:
 # instance; here, the first part names a CATALOG, which is how a federated
 # connector (`"lambda:fn".db.tbl`) is addressed. One approved target means one
 # catalog, so a reference that names its own is refused with the rest.
+#
+# `auto_approve_default` is False: an archive query is reviewed unless the
+# target turns auto-approve on (operator decision, 2026-09-20). What a query
+# costs here depends on the partitions it reads, so the same statement with a
+# wider date range scans more data and costs more. A fleet-wide waiver was not
+# granted with that in mind, and the fingerprint cache ignores literal values by
+# design, so either one would let through the expensive variant of a query
+# whose cheap variant was approved once.
 
 ATHENA = EngineSpec(
     name="athena",
@@ -396,6 +413,7 @@ class EngineSpec:
     driver="athena",
     default_port=443,            # HTTPS to the regional Athena endpoint
     requires_credentials=False,  # the gateway assumes a role; nothing is stored
+    auto_approve_default=False,  # reviewed unless engine_config.auto_approve
 )
 
 
@@ -413,6 +431,29 @@ def spec(engine: str | None) -> EngineSpec:
     return _ENGINES.get((engine or "postgres").strip().lower(), POSTGRES)
 
 
+def auto_approve_allowed(target) -> bool:
+    """May an auto-approve waiver or the fingerprint cache skip review here?
+
+    The one place that answers it. Everything that decides auto-approval, or
+    tells someone a query will be auto-approved, asks this first.
+
+    `engine_config.auto_approve` on the target wins when it is a JSON boolean.
+    Anything else there (the string "false", a 0, null) is not an answer and
+    falls back to the engine's default: a string that reads as false must not
+    act as true, and a typo must not switch on what the engine keeps off.
+
+    No target means no exemption. A super-admin's own submission is a separate
+    rule, decided by the caller, and does not ask this.
+    """
+    if target is None:
+        return False
+    config = getattr(target, "engine_config", None)
+    override = config.get("auto_approve") if isinstance(config, dict) else None
+    if isinstance(override, bool):
+        return override
+    return spec(getattr(target, "engine", None)).auto_approve_default
+
+
 # Engines with a WIRED execution path (driver + dispatch + engine-aware
 # safety). An engine can carry a spec (safety data) before it can execute.
 # A target whose engine is known-but-not-yet-executable must FAIL CLOSED,

src/queryhub/slack_app/handlers.py

@@ -50,7 +50,7 @@ def _kill_switch_message() -> str:
 from slack_bolt import Ack, App
 from slack_sdk.web import WebClient
 
-from .. import access_requests, admins, audit, auto_approve, auto_approve_requests, bundles, csv_import, db, executor, favorites, grants, idp_sync_guard, manual_runs, pre_flight, profile_sync, query_safety, ratings, requesters, schema_catalog, targets, teams, templates
+from .. import access_requests, admins, audit, auto_approve, auto_approve_requests, bundles, csv_import, db, engines, executor, favorites, grants, idp_sync_guard, manual_runs, pre_flight, profile_sync, query_safety, ratings, requesters, schema_catalog, targets, teams, templates
 from .. import config as cfg
 from .. import core_submit
 from .. import core_decide
@@ -1280,6 +1280,12 @@ def _maybe_dm_ro_burst(client: WebClient, principal_id: str, required_mode: str)
         burst = modal._recent_ro_burst(principal_id)
         if not burst or burst["count"] != cfg.get_int("ro_burst_threshold", 3):
             return
+        # The nudge offers a window on the burst's own target, so there is
+        # nothing to offer where auto-approve is off (an Athena archive): the
+        # window would be granted and never apply.
+        t = targets.get(burst["target_server_id"])
+        if t is None or not engines.auto_approve_allowed(t):
+            return
         # Noise only when the burst's OWN database already skips review. A
         # waiver somewhere else says nothing about this one: asking "any
         # waiver at all" silenced the nudge for everyone covered on one server
@@ -1288,13 +1294,11 @@ def _maybe_dm_ro_burst(client: WebClient, principal_id: str, required_mode: str)
                 principal_id, "ro", target_server_id=burst["target_server_id"],
                 database_name=burst["database_name"]) is not None:
             return
-        t = targets.get(burst["target_server_id"])
-        alias = t.alias if t else f"target #{burst['target_server_id']}"
         blocks = ro_window.nudge_blocks(
             count=burst["count"],
             window_min=cfg.get_int("ro_burst_window_min", 10),
             window_minutes=cfg.get_int("ro_window_minutes", 60),
-            target_alias=alias,
+            target_alias=t.alias,
             target_server_id=burst["target_server_id"],
             database_name=burst["database_name"],
             has_active_grant=False,
@@ -1616,10 +1620,18 @@ def handle_batch_submission(ack: Ack, body: dict, client: WebClient) -> None:
     ack()
 
     # Per-item auto-approve decision — same logic as single-shot:
-    # cover at submit time AND at scheduled run time (if scheduled).
+    # cover at submit time AND at scheduled run time (if scheduled), and
+    # nothing on a target that keeps auto-approve off (an Athena archive).
     aa_grants: list[dict | None] = []
     aa_expired_warning_items: list[int] = []   # 1-based positions
+    aa_allowed: dict[int, bool] = {}           # per target, asked once
     for i, vi in enumerate(validated_items, start=1):
+        tid = vi["target_server_id"]
+        if tid not in aa_allowed:
+            aa_allowed[tid] = engines.auto_approve_allowed(targets.get(tid))
+        if not aa_allowed[tid]:
+            aa_grants.append(None)
+            continue
         g = auto_approve.effective_grant(
             user["id"], vi["required_mode"],
             target_server_id=vi["target_server_id"],