diff --git a/.forgejo/workflows/build-push.yml b/.forgejo/workflows/build-push.yml index 1b3eeab..f330850 100644 --- a/.forgejo/workflows/build-push.yml +++ b/.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 }}" diff --git a/.forgejo/workflows/pr-build.yml b/.forgejo/workflows/pr-build.yml index 8ac368d..9147952 100644 --- a/.forgejo/workflows/pr-build.yml +++ b/.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; } diff --git a/.forgejo/workflows/secrets-guard.yml b/.forgejo/workflows/secrets-guard.yml index 09b2049..9b65967 100644 --- a/.forgejo/workflows/secrets-guard.yml +++ b/.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] diff --git a/.forgejo/workflows/shell-lint.yml b/.forgejo/workflows/shell-lint.yml index 6aa69eb..0c1af34 100644 --- a/.forgejo/workflows/shell-lint.yml +++ b/.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] diff --git a/backend/Dockerfile b/backend/Dockerfile index ab97519..96d5f6e 100644 --- a/backend/Dockerfile +++ b/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. diff --git a/backend/lake_app.py b/backend/lake_app.py index c6a6100..b79b95b 100644 --- a/backend/lake_app.py +++ b/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}'") - con.execute("SET s3_url_style='path'") - con.execute(f"SET s3_region='{cfg['region']}'") - con.execute("SET s3_access_key_id=?", [cfg["access_key"]]) - con.execute("SET s3_secret_access_key=?", [cfg["secret_key"]]) + # A SECRET, not `SET s3_secret_access_key`: settings are readable back out + # with current_setting(), so the old form let any caller of /query retrieve + # the bucket credentials. Secret values are opaque -- duckdb_secrets() lists + # the name and scope but never the key material. + 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") diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index f4f92fb..a4200da 100644 --- a/backend/tests/conftest.py +++ b/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: diff --git a/backend/tests/notifications/test_send_safety.py b/backend/tests/notifications/test_send_safety.py new file mode 100644 index 0000000..24c5fee --- /dev/null +++ b/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() diff --git a/backend/tests/web/test_lake_app.py b/backend/tests/web/test_lake_app.py index 2dd1f1e..3966a8f 100644 --- a/backend/tests/web/test_lake_app.py +++ b/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 diff --git a/frontend/content/pages.yaml b/frontend/content/pages.yaml index a379964..7e3fca1 100644 --- a/frontend/content/pages.yaml +++ b/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, diff --git a/frontend/server/internal/contentdata/loader_test.go b/frontend/server/internal/contentdata/loader_test.go index ab23ca5..4ea8cba 100644 --- a/frontend/server/internal/contentdata/loader_test.go +++ b/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 +// 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: "±7-day" in the +// SERP snippet, "Weather & 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 (&), decimal (±) and hex (±) 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) + } + } + } +}