Testing plan wave 0: close two live defects, hard-block outbound sends, gate the prod path (#88)

Co-authored-by: admin_emi <admin_emi@noreply.dev.jinemi.com>
Co-committed-by: admin_emi <admin_emi@noreply.dev.jinemi.com>

.forgejo/workflows/build-push.yml

@@ -172,6 +172,26 @@ jobs:
           fi
           docker build "${tag_args[@]}" "${{ matrix.domain }}/"
 
+      # Build -> TEST -> push. Without this the only pytest run in CI was
+      # pr-build's, so anything reaching a branch by another route (a direct
+      # push, a workflow_dispatch, a v*.*.* tag) published an image that CI had
+      # never tested — and deploy.yml then rolls that exact tag onto beta/prod.
+      # Gating the artifact rather than the branch is what makes the deploy safe,
+      # since deploy.yml consumes a tag, not a commit status.
+      #
+      # Same invocation as build.yml's, deliberately: -u 0 because the image's
+      # default user cannot write to site-packages, --entrypoint sh because the
+      # real entrypoint is alembic-migrate-then-serve and would swallow the
+      # command, and `unset THERMOGRAPH_BASE` because the image bakes the prod
+      # root mount (/) while the tests are written against the /thermograph
+      # prefix. The Go daemon and Go frontend suites need no step here: both run
+      # inside their Dockerfiles, so `docker build` above already gated them.
+      - name: Test the built image (backend only)
+        if: steps.image.outputs.changed == 'true' && matrix.domain == 'backend'
+        run: |
+          docker run --rm -u 0 --entrypoint sh "${{ steps.image.outputs.sha_tag }}" \
+            -c "unset THERMOGRAPH_BASE; pip install -q --no-cache-dir pytest==8.4.1 && python -m pytest tests -q"
+
       - name: Push (SHA tag, every push)
         if: steps.image.outputs.changed == 'true'
         run: docker push "${{ steps.image.outputs.sha_tag }}"

.forgejo/workflows/pr-build.yml

@@ -19,6 +19,10 @@ name: PR build (required check)
 
 on:
   pull_request:
+    # `release` included: it is the branch that deploys to PROD, and it used to
+    # be the least-checked branch in the repo. A PR into release ran neither a
+    # build nor a test — only the two standalone lint workflows — so the intended
+    # promotion path was the one with no gate on it.
     branches: [dev, main, release]
 
 concurrency:
@@ -73,18 +77,32 @@ jobs:
     if: needs.changes.outputs.observability == 'true'
     uses: ./.forgejo/workflows/observability-validate.yml
 
+  # NOT domain-gated and NOT `needs: changes`: both are repo-wide invariants that
+  # must hold on every PR regardless of which directories it touches. A plaintext
+  # secret or a broken script can arrive in a commit that changes no app domain at
+  # all — precisely the case where every domain job skips and `gate` used to pass
+  # vacuously.
+  lint-shell:
+    uses: ./.forgejo/workflows/shell-lint.yml
+
+  guard-secrets:
+    uses: ./.forgejo/workflows/secrets-guard.yml
+
   gate:
     name: gate
     # always(): skipped domain jobs (nothing changed there) must not skip the
     # gate itself -- the required check has to report on EVERY PR.
     if: always()
-    needs: [changes, build-backend, build-frontend, validate-observability]
+    needs: [changes, build-backend, build-frontend, validate-observability,
+            lint-shell, guard-secrets]
     runs-on: docker
     steps:
       - name: Reduce domain results
         run: |
           set -euo pipefail
           ok=1
+          # Domain jobs: `skipped` is a pass — it means the PR did not touch that
+          # domain, which is the whole point of the path filtering.
           for pair in \
             "backend=${{ needs.build-backend.result }}" \
             "frontend=${{ needs.build-frontend.result }}" \
@@ -95,4 +113,17 @@ jobs:
               *) ok=0 ;;
             esac
           done
-          [ "$ok" = 1 ] || { echo "a changed domain's check failed"; exit 1; }
+          # Repo-wide invariants: `skipped` is a FAILURE. These have no `if:` and
+          # no `needs:`, so they run on every PR; a skip means the job did not
+          # execute and the invariant is therefore unverified, which must not be
+          # reported as a pass.
+          for pair in \
+            "shellcheck=${{ needs.lint-shell.result }}" \
+            "secrets-encrypted=${{ needs.guard-secrets.result }}"; do
+            echo "$pair"
+            case "${pair#*=}" in
+              success) ;;
+              *) ok=0 ;;
+            esac
+          done
+          [ "$ok" = 1 ] || { echo "a required check failed"; exit 1; }

.forgejo/workflows/secrets-guard.yml

@@ -11,6 +11,11 @@ name: secrets-guard
 # runs when you expect it to isn't a backstop.
 
 on:
+  # workflow_call so pr-build.yml can fold this into the single required `gate`
+  # status. This is the one that mattered most: branch protection requires only
+  # `gate`, so a commit adding a PLAINTEXT secrets file failed this check as a
+  # separate status and was still mergeable.
+  workflow_call: {}
   pull_request:
   push:
     branches: [dev, main, release]

.forgejo/workflows/shell-lint.yml

@@ -20,6 +20,11 @@ name: shell-lint
 # to fix any new findings in the same PR as the bump.
 
 on:
+  # workflow_call so pr-build.yml can fold this into the single required `gate`
+  # status. It stays independently triggered as well: branch protection is
+  # configured to require only `gate`, so before this was reachable from there a
+  # shellcheck regression reported as a separate status that nothing enforced.
+  workflow_call: {}
   pull_request:
   push:
     branches: [dev, main, release]

backend/Dockerfile

@@ -15,6 +15,17 @@ WORKDIR /src
 COPY daemon/go.mod daemon/go.sum ./
 RUN go mod download
 COPY daemon/ ./
+# gofmt + vet + the full daemon test suite BEFORE the build, so no published
+# image can contain a daemon that failed its own tests — the same guarantee
+# frontend/Dockerfile already gives the Go frontend, and the reason the tests
+# live here rather than in a workflow step: build-push.yml builds this image on
+# every push to dev/main/release with no test job of its own, so a check that is
+# not part of `docker build` does not gate the artifact that actually deploys.
+#
+# These 48 tests (gateway, cron, apiclient, config) previously ran nowhere:
+# `grep -rn "go test" .forgejo/workflows/` found nothing. The daemon owns the
+# prod Discord gateway websocket and every recurring-job timer.
+RUN test -z "$(gofmt -l .)" && go vet ./... && go test ./...
 # -trimpath keeps the build reproducible (identical sources -> identical
 # binary), which is what lets the final stage's COPY layer cache-hit when only
 # Python code changed.

backend/lake_app.py

@@ -31,10 +31,11 @@ import threading
 import time
 
 import polars as pl
-from fastapi import FastAPI, HTTPException
+from fastapi import Depends, FastAPI, HTTPException
 from fastapi.responses import FileResponse, Response
 from pydantic import BaseModel
 
+from api.internal_routes import _require_internal_token
 from data import era5lake
 
 CACHE_DIR = os.environ.get("THERMOGRAPH_LAKE_CACHE", "/state/lake-cache")
@@ -133,6 +134,32 @@ _FORBIDDEN = re.compile(
     r"\b(insert|update|delete|drop|create|alter|attach|copy|truncate|install|load"
     r"|export|import|pragma|set|call)\b", re.I)
 
+# The verb denylist above is not a security boundary on its own: DuckDB's file
+# readers and setting introspection are plain SELECTs, so every one of these used
+# to sail past it. `SELECT current_setting('s3_secret_access_key')` handed the
+# bucket credentials straight back to the caller -- the precise outcome the
+# module docstring says this service exists to prevent -- and read_csv_auto()/
+# glob() turned /query into an arbitrary read of the container filesystem.
+#
+# A denylist over a full query language is not winnable, so this is defence in
+# depth only. The real boundaries are, in order: the shared-secret gate on the
+# route, DuckDB SECRETs (opaque to current_setting) instead of SET s3_*, and
+# disabled_filesystems in bucket mode. See _connect().
+_DENIED_FUNCS = re.compile(
+    r"\b(read_csv|read_csv_auto|read_parquet|parquet_scan|parquet_metadata"
+    r"|parquet_schema|read_json|read_json_auto|read_json_objects|read_ndjson"
+    r"|read_ndjson_auto|read_text|read_blob|glob|iceberg_scan|iceberg_metadata"
+    r"|delta_scan|sniff_csv|current_setting|getenv|duckdb_settings"
+    r"|duckdb_secrets|duckdb_extensions|duckdb_functions|sql_auto_complete"
+    r"|which_secret|open|shell)\s*\(", re.I)
+
+# `FROM 'some/path'` is DuckDB's replacement scan: a bare string literal in table
+# position reads that path as a file. Legitimate queries name era5_daily or
+# manifest, never a literal.
+_FROM_LITERAL = re.compile(r"\b(from|join)\s*\(?\s*['\"]", re.I)
+
+_GUARD_DETAIL = "only reads of the era5_daily and manifest views are allowed"
+
 
 def _connect():
     """A fresh in-memory DuckDB with the lake views registered. Returns None
@@ -164,11 +191,14 @@ def _connect():
     con.execute("LOAD httpfs")   # both baked into the image at build time,
     con.execute("LOAD iceberg")  # as the runtime user (see Dockerfile)
     host = cfg["endpoint"].split("://", 1)[-1]
-    con.execute(f"SET s3_endpoint='{host}'")
+    # A SECRET, not `SET s3_secret_access_key`: settings are readable back out
-    con.execute("SET s3_url_style='path'")
+    # with current_setting(), so the old form let any caller of /query retrieve
-    con.execute(f"SET s3_region='{cfg['region']}'")
+    # the bucket credentials. Secret values are opaque -- duckdb_secrets() lists
-    con.execute("SET s3_access_key_id=?", [cfg["access_key"]])
+    # the name and scope but never the key material.
-    con.execute("SET s3_secret_access_key=?", [cfg["secret_key"]])
+    con.execute(
+        "CREATE OR REPLACE SECRET lake (TYPE S3, KEY_ID ?, SECRET ?, "
+        "REGION ?, ENDPOINT ?, URL_STYLE 'path')",
+        [cfg["access_key"], cfg["secret_key"], cfg["region"], host])
     # Newest metadata JSON wins (one single-page LIST of metadata/ — cheap);
     # pyiceberg names them NNNNN-uuid.metadata.json, so lexicographic max is
     # the numeric max.
@@ -180,16 +210,26 @@ def _connect():
     con.execute(f"CREATE VIEW era5_daily AS SELECT * FROM iceberg_scan('{meta}')")
     con.execute(f"""CREATE VIEW manifest AS SELECT * FROM
         read_parquet('s3://{cfg['bucket']}/{era5lake.MANIFEST_KEY}')""")
+    # Views are resolved lazily, so this must come after they are created --
+    # and only in bucket mode, where every scan goes over httpfs and nothing
+    # legitimate touches local disk. It makes read_csv_auto('/etc/passwd') and
+    # glob('/etc/*') a PermissionException in the engine rather than relying on
+    # the regex above. lock_configuration then stops a caller re-enabling it
+    # (belt and braces: `set` is already a forbidden verb).
+    con.execute("SET disabled_filesystems='LocalFileSystem'")
+    con.execute("SET lock_configuration=true")
     return con
 
 
-@app.post("/query")
+@app.post("/query", dependencies=[Depends(_require_internal_token)])
 def query(q: Query):
     sql = q.sql.strip().rstrip(";").strip()
     if not sql.lower().startswith(("select", "with")) or ";" in sql \
             or _FORBIDDEN.search(sql):
         raise HTTPException(status_code=400,
                             detail="only a single SELECT statement is allowed")
+    if _DENIED_FUNCS.search(sql) or _FROM_LITERAL.search(sql):
+        raise HTTPException(status_code=400, detail=_GUARD_DETAIL)
     con = _connect()
     if con is None:
         raise HTTPException(status_code=503, detail="lake not configured")

backend/tests/conftest.py

@@ -53,6 +53,69 @@ audit.ACTIVITY_DIR = os.path.join(_TMP, "logs", "activity")
 audit.HEARTBEAT_DIR = os.path.join(_TMP, "logs", "heartbeat")
 
 
+# --- outbound transports: hard-blocked, not merely unconfigured ---------------
+#
+# Every send gate in notifications/ is read from ambient env AT IMPORT TIME, and
+# every send site swallows its exceptions (notify.py's _flush_sends,
+# discord.py's _bot_post). So before this block existed, an operator who had
+# sourced /etc/thermograph.env in the shell — routine when debugging — turned
+# `pytest tests/notifications` into a live broadcast to real subscribers, and it
+# reported GREEN because the exceptions never surfaced. test_notify.py alone
+# calls run_pass() seven times without patching notifications.discord.
+#
+# CI was safe only by omission: it runs the suite in a container with no
+# credentials baked in. One added `-e` removed that.
+#
+# Two layers, deliberately:
+#   1. Blank the config so every enabled()/dm_enabled()/subscription_enabled()
+#      gate reports False — the same state CI runs in, now guaranteed locally.
+#   2. Replace the actual transports with sentinels that RAISE. A test that
+#      forgets to patch one now fails loudly instead of sending silently.
+#
+# Tests that need to exercise a send path patch these themselves, which is the
+# point: the escape hatch is explicit and visible in the test that takes it.
+# The idiom is borrowed from tests/test_warm_content.py.
+import smtplib  # noqa: E402
+
+import indexnow  # noqa: E402
+from notifications import discord, mailer, push  # noqa: E402
+
+
+class OutboundBlocked(AssertionError):
+    """Raised when a test reaches a real outbound transport.
+
+    Seeing this is not a flaky test — it means the code under test tried to
+    contact Discord, an SMTP host, a push endpoint or IndexNow for real. Patch
+    the transport in the test, or assert the no-send path.
+    """
+
+
+def _blocked(what):
+    def _raise(*a, **kw):
+        raise OutboundBlocked(
+            f"test tried to reach {what} for real. Patch it in the test "
+            f"(see tests/conftest.py's outbound-transport block).")
+    return _raise
+
+
+# 1. Config blanked: every gate reports False.
+discord.BOT_TOKEN = ""
+discord.WEBHOOK_URL = ""
+discord.SUBSCRIPTION_CHANNEL_ID = ""
+discord.WEATHER_CHANNEL_ID = ""
+mailer.BACKEND = "console"
+
+# 2. Transports replaced with raisers.
+discord._client.post = _blocked("Discord")
+discord._client.patch = _blocked("Discord")
+discord._client.put = _blocked("Discord")
+discord._client.delete = _blocked("Discord")
+indexnow._client.post = _blocked("IndexNow")
+push.webpush = _blocked("a Web Push endpoint")
+smtplib.SMTP = _blocked("an SMTP host")
+smtplib.SMTP_SSL = _blocked("an SMTP host")
+
+
 def _years_before(d: datetime.date, years: int) -> datetime.date:
     """`d` shifted back `years` years, same month/day (Feb 29 → Feb 28)."""
     try:

backend/tests/notifications/test_send_safety.py

@@ -0,0 +1,108 @@
+"""The guarantee that a test run cannot reach a real subscriber.
+
+tests/conftest.py hard-blocks every outbound transport. That block is worth
+nothing unless something asserts it, because it fails in exactly one direction:
+if a future edit weakens it, every other test still passes and the only symptom
+is a real Discord message, a real email, or a real push to somebody's phone —
+delivered silently, because notify.py and discord.py swallow send exceptions.
+
+So this file tests the test harness. If anything here fails, do not weaken the
+assertion; fix the block in conftest.py.
+"""
+import smtplib
+
+import pytest
+
+import indexnow
+from notifications import discord, mailer, push
+from tests.conftest import OutboundBlocked
+
+
+# --- layer 1: every send gate reports "not configured" ------------------------
+
+def test_no_discord_surface_is_enabled():
+    """A stray THERMOGRAPH_DISCORD_* in the operator's shell must not arm the
+    bot. These four gates are what stand between run_pass() and a real post to
+    the subscription channel, which notifies real people."""
+    assert discord.enabled() is False
+    assert discord.dm_enabled() is False
+    assert discord.subscription_enabled() is False
+    assert discord.weather_enabled() is False
+
+
+def test_no_discord_credentials_are_live():
+    assert discord.BOT_TOKEN == ""
+    assert discord.WEBHOOK_URL == ""
+    assert discord.SUBSCRIPTION_CHANNEL_ID == ""
+    assert discord.WEATHER_CHANNEL_ID == ""
+
+
+def test_mail_never_goes_over_smtp():
+    """`console` prints; `smtp` hands the message to a real host. digest.py
+    branches on exactly this value."""
+    assert mailer.BACKEND == "console"
+    assert mailer.enabled() is False
+
+
+# --- layer 2: the transports themselves raise --------------------------------
+#
+# Layer 1 is config, and config can be re-enabled by a test that monkeypatches a
+# token to exercise a branch. These are the backstop for that case.
+
+def test_discord_transport_raises_rather_than_posting():
+    with pytest.raises(OutboundBlocked):
+        discord._client.post("https://discord.com/api/v10/channels/1/messages",
+                             json={"content": "should never leave the box"})
+
+
+def test_indexnow_transport_raises_rather_than_posting():
+    with pytest.raises(OutboundBlocked):
+        indexnow._client.post("https://api.indexnow.org/indexnow", json={})
+
+
+def test_push_transport_raises_rather_than_sending():
+    with pytest.raises(OutboundBlocked):
+        push.webpush(subscription_info={}, data="{}")
+
+
+def test_smtp_raises_rather_than_connecting():
+    with pytest.raises(OutboundBlocked):
+        smtplib.SMTP("127.0.0.1", 25)
+    with pytest.raises(OutboundBlocked):
+        smtplib.SMTP_SSL("127.0.0.1", 465)
+
+
+# --- the paths that used to be able to send silently -------------------------
+
+def test_bot_post_cannot_reach_discord_even_with_a_token(monkeypatch):
+    """_bot_post is the funnel for DMs and channel posts. Re-arm the token the
+    way a careless test might and confirm the transport still refuses.
+
+    Note _bot_post swallows exceptions by design, so it returns None rather than
+    raising — the assertion that matters is that the sentinel ran at all, i.e.
+    that no HTTP request was attempted against the real API.
+    """
+    monkeypatch.setattr(discord, "BOT_TOKEN", "not-a-real-token")
+    attempted = []
+    monkeypatch.setattr(discord._client, "post",
+                        lambda *a, **kw: attempted.append(a) or (_ for _ in ()).throw(
+                            OutboundBlocked("blocked")))
+    assert discord._bot_post("/channels/1/messages", {"content": "x"}) is None
+    assert attempted, "_bot_post should have gone through _client.post"
+
+
+def test_run_pass_sends_nothing_when_nothing_is_configured():
+    """The regression this whole block exists for: notify.run_pass() reaches
+    discord.post_subscription_alert() for every notification it creates, and
+    test_notify.py calls it seven times without patching discord. With the gates
+    closed it must complete without touching a transport."""
+    from accounts import db
+    from notifications import notify
+
+    # The app lifespan (which normally creates these) does not run under
+    # TestClient-less tests, so build the schema the way test_notify.py does.
+    db.Base.metadata.create_all(db.sync_engine)
+
+    # No subscriptions in the throwaway DB => no candidates, no sends. The point
+    # is that this completes without raising OutboundBlocked.
+    notify.run_pass()

backend/tests/web/test_lake_app.py

@@ -7,12 +7,20 @@ import polars as pl
 import pytest
 from fastapi.testclient import TestClient
 
+# /query is gated on the same shared secret as /internal/* (it is an operator
+# surface, not a public one). The fixture provisions it so the existing query
+# tests exercise the authorised path; the auth tests below drive the closed ones.
+TOKEN = "test-internal-token"
+HDR = {"X-Thermograph-Internal-Token": TOKEN}
+
 
 @pytest.fixture
 def lake(tmp_path, monkeypatch):
     """A tiny two-point lake (points, hive partitions, manifest) on disk."""
     from data import era5lake
 
+    monkeypatch.setenv("THERMOGRAPH_INTERNAL_TOKEN", TOKEN)
+
     points = [(154, 1439), (154, 0)]           # around London
     d0 = datetime.date(1940, 1, 1)
     dates = [d0 + datetime.timedelta(days=k) for k in range(12000)]
@@ -89,7 +97,7 @@ def test_history_404_for_point_outside_lake(lake):
 
 
 def test_query_selects_with_partition_predicates(lake):
-    r = lake.post("/query", json={"sql":
+    r = lake.post("/query", headers=HDR, json={"sql":
         "SELECT year, month, count(*) AS n FROM era5_daily "
         "WHERE year = 1940 AND month = 1 GROUP BY year, month"})
     assert r.status_code == 200
@@ -99,7 +107,7 @@ def test_query_selects_with_partition_predicates(lake):
 
 
 def test_query_manifest_view(lake):
-    r = lake.post("/query", json={"sql": "SELECT count(*) FROM manifest"})
+    r = lake.post("/query", headers=HDR, json={"sql": "SELECT count(*) FROM manifest"})
     assert r.status_code == 200
     assert r.json()["rows"][0][0] == 2
 
@@ -111,4 +119,115 @@ def test_query_manifest_view(lake):
     "COPY era5_daily TO 'x'",
 ])
 def test_query_rejects_non_select(lake, sql):
-    assert lake.post("/query", json={"sql": sql}).status_code == 400
+    assert lake.post("/query", headers=HDR,
+                     json={"sql": sql}).status_code == 400
+
+
+# --- /query is an operator surface, not a public one -------------------------
+
+def test_query_requires_the_internal_token(lake):
+    """No header at all: the caller is not the operator. 401, not a result."""
+    r = lake.post("/query", json={"sql": "SELECT count(*) FROM manifest"})
+    assert r.status_code == 401
+
+
+def test_query_rejects_a_wrong_internal_token(lake):
+    r = lake.post("/query", headers={"X-Thermograph-Internal-Token": "nope"},
+                  json={"sql": "SELECT count(*) FROM manifest"})
+    assert r.status_code == 401
+
+
+def test_query_is_closed_when_no_token_is_provisioned(lake, monkeypatch):
+    """Unprovisioned means the surface does not exist (404) rather than falling
+    open to no-auth — same posture as /internal/* and /api/v2/metrics."""
+    monkeypatch.delenv("THERMOGRAPH_INTERNAL_TOKEN", raising=False)
+    monkeypatch.delenv("THERMOGRAPH_AUTH_SECRET", raising=False)
+    r = lake.post("/query", headers=HDR,
+                  json={"sql": "SELECT count(*) FROM manifest"})
+    assert r.status_code == 404
+
+
+# --- the SELECT guard must reject reads outside the two lake views -----------
+#
+# The old guard was a denylist of DDL/DML *verbs*, so it passed every one of
+# these: DuckDB's reader functions and setting introspection are plain SELECTs.
+# In bucket mode `SET s3_secret_access_key` made the bucket credentials readable
+# straight back out via current_setting() -- the exact thing this service exists
+# to avoid (see the module docstring). These are the attack shapes, not the
+# shapes the old regex happened to be written for.
+
+@pytest.mark.parametrize("sql", [
+    "SELECT * FROM read_csv_auto('/etc/passwd')",
+    "SELECT * FROM read_csv('/etc/passwd')",
+    "SELECT * FROM read_parquet('/etc/passwd')",
+    "SELECT * FROM read_json_auto('/etc/passwd')",
+    "SELECT * FROM read_text('/etc/passwd')",
+    "SELECT * FROM read_blob('/etc/passwd')",
+    "SELECT * FROM glob('/etc/*')",
+    "SELECT * FROM duckdb_settings()",
+    "SELECT * FROM duckdb_secrets()",
+    "SELECT current_setting('s3_secret_access_key')",
+    "SELECT getenv('THERMOGRAPH_LAKE_S3_SECRET')",
+    "SELECT * FROM 'file:///etc/passwd'",
+    "SELECT * FROM '/etc/passwd'",
+    # Nested one level down, so a prefix-only check doesn't pass it.
+    "SELECT * FROM (SELECT * FROM read_csv_auto('/etc/passwd'))",
+    "WITH x AS (SELECT * FROM glob('/etc/*')) SELECT * FROM x",
+])
+def test_query_rejects_reads_outside_the_lake_views(lake, sql):
+    r = lake.post("/query", headers=HDR, json={"sql": sql})
+    assert r.status_code == 400, f"guard let this through: {sql}"
+    # Assert the GUARD refused it, not that DuckDB happened to choke on the file
+    # format. Several of these (read_parquet on a text file, a bare '/etc/passwd')
+    # error in the engine anyway, so a status-only assertion would still pass with
+    # the guard deleted -- exactly the "negatives shaped like the regex" trap the
+    # original four-case test fell into.
+    detail = r.json()["detail"]
+    assert not detail.startswith("query failed"), (
+        f"rejected by the engine, not the guard: {sql} -> {detail}")
+
+
+def test_query_still_allows_a_block_comment_inside_a_real_select(lake):
+    """Defence in depth must not become a false positive: an inline comment in
+    an otherwise ordinary query is fine."""
+    r = lake.post("/query", headers=HDR,
+                  json={"sql": "SELECT count(*) /* rows */ FROM manifest"})
+    assert r.status_code == 200
+
+
+def test_bucket_mode_uses_a_secret_not_a_readable_setting(monkeypatch, tmp_path):
+    """Regression guard for the credential-disclosure shape: the S3 credentials
+    must be installed as a DuckDB SECRET (opaque to current_setting) rather than
+    via SET s3_secret_access_key, which hands them back to any caller."""
+    import lake_app
+
+    statements = []
+
+    class _FakeCon:
+        def execute(self, sql, *args):
+            statements.append(sql)
+            return self
+
+        def fetchone(self):
+            return ("s3://b/iceberg/era5_daily/metadata/1.metadata.json",)
+
+        def close(self):
+            pass
+
+    monkeypatch.setattr(lake_app, "_source", lambda: ("bucket", {
+        "endpoint": "https://example.com", "bucket": "b", "region": "r",
+        "access_key": "AK", "secret_key": "SUPER-SECRET-KEY"}))
+    monkeypatch.setitem(__import__("sys").modules, "duckdb",
+                        type("m", (), {"connect": staticmethod(lambda: _FakeCon())}))
+
+    lake_app._connect()
+    joined = "\n".join(statements)
+    assert "SECRET" in joined.upper() and "TYPE S3" in joined.upper()
+    assert "SET s3_secret_access_key" not in joined
+    assert "SET s3_access_key_id" not in joined
+    # The key material goes as a bound parameter, never interpolated into SQL.
+    assert "SUPER-SECRET-KEY" not in joined
+    # The local filesystem is unreachable in bucket mode, and the caller cannot
+    # turn it back on.
+    assert "disabled_filesystems" in joined
+    assert "lock_configuration" in joined

frontend/content/pages.yaml

@@ -6,7 +6,7 @@ pages:
   about:
     title: "About Thermograph: how the weather grades are calculated"
     description: >-
-      How Thermograph works: ~45 years of ERA5 climate history, a ±7-day
+      How Thermograph works: ~45 years of ERA5 climate history, a ±7-day
       seasonal window, and empirical percentiles that grade each day relative to
       its own location.
   privacy:
@@ -21,7 +21,7 @@ pages:
       temperatures by month, all-time records, and how today's weather compares
       to local history.
   glossary_index:
-    title: "Weather & climate glossary: heat index, feels-like, percentile, and more"
+    title: "Weather & climate glossary: heat index, feels-like, percentile, and more"
     description: >-
       Plain-language definitions of weather and climate terms: climate normal,
       percentile, temperature anomaly, feels-like, heat index, wind chill,

frontend/server/internal/contentdata/loader_test.go

@@ -3,6 +3,7 @@ package contentdata
 import (
 	"os"
 	"path/filepath"
+	"regexp"
 	"testing"
 )
 
@@ -109,3 +110,38 @@ func TestLoadRealContentFiles(t *testing.T) {
 		}
 	}
 }
+
+// pages.yaml's title/description are interpolated as plain strings into
+// <title> and <meta name="description"> (base.html.tmpl), so html/template
+// escapes them. An HTML entity written in the YAML is therefore escaped a
+// second time and the user sees the literal source: "&plusmn;7-day" in the
+// SERP snippet, "Weather &amp; climate glossary" in the browser tab. Both
+// shipped live until this test existed.
+//
+// glossary.yaml's `body` is deliberately NOT checked: it is typed
+// template.HTML (see glossary_term.html.tmpl) and rendered raw, so entities
+// and <b> tags there are correct and must stay.
+func TestRealPagesHaveNoDoubleEscapedEntities(t *testing.T) {
+	dir := filepath.Join("..", "..", "..", "content")
+	if _, err := os.Stat(filepath.Join(dir, "pages.yaml")); err != nil {
+		t.Skipf("real content dir not available: %v", err)
+	}
+	pages, err := LoadPages(dir)
+	if err != nil {
+		t.Fatalf("LoadPages(real): %v", err)
+	}
+	// Named (&amp;), decimal (&#177;) and hex (&#xB1;) entity forms.
+	entity := regexp.MustCompile(`&([a-zA-Z][a-zA-Z0-9]*|#[0-9]+|#[xX][0-9a-fA-F]+);`)
+	for key, p := range pages {
+		for field, value := range map[string]string{
+			"title": p.Title, "description": p.Description,
+		} {
+			if m := entity.FindString(value); m != "" {
+				t.Errorf("pages.yaml[%s].%s contains the HTML entity %q; "+
+					"write the character literally (this field is escaped at "+
+					"render time, so the entity reaches the user as source)",
+					key, field, m)
+			}
+		}
+	}
+}