From 63e1967bbb707e08e82285bcca74821e3c49afaa Mon Sep 17 00:00:00 2001 From: Emi Griffith Date: Sat, 25 Jul 2026 01:35:49 -0700 Subject: [PATCH 1/4] lake: gate /query and stop it reading outside the two lake views The SELECT guard was a denylist of DDL/DML verbs. DuckDB's file readers and setting introspection are plain SELECTs, so it passed all of these: SELECT * FROM read_csv_auto('/etc/passwd') -> 200, file contents SELECT * FROM glob('/etc/*') -> 200, directory listing SELECT current_setting('s3_secret_access_key') -> 200, the bucket credentials The last one is the worst: bucket mode installed the S3 credentials with SET s3_secret_access_key, and settings are readable back out. So one unauthenticated POST returned the ERA5 bucket key in plaintext -- precisely what the module docstring says this service exists to avoid ("without shipping S3 credentials to every web task"). /query had no authentication of any kind, while the comparable /internal/* router has gated its whole surface since it landed. Three changes, outermost first: - /query now requires the shared internal token, reusing internal_routes' dependency. Nothing calls it: the only in-repo caller of this service is era5lake.py hitting /history, and Centralis reaches the lake through its own DuckDB rather than this endpoint. /history is deliberately left open for now -- gating it means threading the token through the web->lake hot path. - Credentials are installed as a DuckDB SECRET instead of SET s3_*. Secret values are opaque: current_setting() returns NULL and duckdb_secrets() lists the name and scope but no key material. - Bucket mode disables LocalFileSystem after the views are created (they resolve lazily, so ordering matters) and locks the configuration. Local mode cannot do this -- its views read local parquet -- so the function denylist stays as defence in depth for dev and tests. The new guard tests assert the *guard* refused, not merely that the request failed: several of these shapes (read_parquet on a text file, a bare '/etc/passwd') error inside DuckDB anyway, so a status-only assertion would keep passing with the guard deleted. That is the trap the original four-case test fell into -- it tested negatives shaped like the regex that had been written. --- backend/lake_app.py | 54 +++++++++++-- backend/tests/web/test_lake_app.py | 125 ++++++++++++++++++++++++++++- 2 files changed, 169 insertions(+), 10 deletions(-) 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/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 -- 2.45.2 From e377c4de03b6606c6f964ee9ce23774c97c880b1 Mon Sep 17 00:00:00 2001 From: Emi Griffith Date: Sat, 25 Jul 2026 01:35:59 -0700 Subject: [PATCH 2/4] content: write the entities literally in pages.yaml titles and descriptions pages.yaml's title/description are interpolated as plain strings into and <meta name="description">, so html/template escapes them. Writing an HTML entity in the YAML therefore escapes it a second time and the user sees the source. Live on main right now: <meta name="description" content="... a &plusmn;7-day seasonal window ..."> <title>Weather &amp; climate glossary: ... which renders as a literal "±7-day" in the SERP snippet and "&" in the browser tab, on the about and glossary pages. The Python->Go rewrite did not change this: the entities live in the shared data file and both engines autoescape identically. glossary.yaml is deliberately NOT touched. Its `body` is typed template.HTML and rendered raw (glossary_term.html.tmpl), so the entities and tags there are correct and would break if "fixed". The regression test runs against the real content dir, which the frontend Dockerfile already copies into the builder stage, so it gates the image rather than only local runs. --- frontend/content/pages.yaml | 4 +-- .../internal/contentdata/loader_test.go | 36 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) 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) + } + } + } +} -- 2.45.2 From 35006466bf588f6da36052598841806d17c6dfd7 Mon Sep 17 00:00:00 2001 From: Emi Griffith <me@emigriffith.dev> Date: Sat, 25 Jul 2026 01:36:12 -0700 Subject: [PATCH 3/4] tests: hard-block outbound transports, and assert the block holds A test run could message real subscribers, and would have reported green while doing it. Every send gate in notifications/ reads its config from ambient env at import time, and conftest neutralised none of it -- it set four env vars, none touching Discord, SMTP or VAPID. test_notify.py calls run_pass() seven times without patching notifications.discord, and notify.py:358 reaches discord.post_subscription_alert() for every notification it creates. The subscription channel notifies real people. The failure mode is silent: notify.py's send path and discord.py's _bot_post swallow every exception, so a real send raises nothing and no assertion goes red. An operator who had sourced /etc/thermograph.env to debug -- routine -- turned `pytest tests/notifications` into a live broadcast. CI was safe only by omission: it runs in a container with no credentials baked in, and one added -e removed that. Two layers: 1. Config blanked, so every enabled()/dm_enabled()/subscription_enabled() gate reports False. This is the state CI already runs in, now guaranteed locally instead of depending on the shell. 2. The transports themselves replaced with sentinels that raise. A test that forgets to patch one fails loudly rather than sending. Tests that need to exercise a send path patch these themselves, which is the point: the escape hatch becomes explicit in the test that takes it. The idiom is lifted from tests/test_warm_content.py, which already did this for one call. test_send_safety.py asserts the guarantee, because a block like this fails in only one direction -- weaken it and every other test still passes, the sole symptom being a message delivered to somebody. Full suite: 457 passed, 8 skipped. Nothing relied on a live transport. --- backend/tests/conftest.py | 63 ++++++++++ .../tests/notifications/test_send_safety.py | 108 ++++++++++++++++++ 2 files changed, 171 insertions(+) create mode 100644 backend/tests/notifications/test_send_safety.py 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() -- 2.45.2 From 8432144bb374daf361048b8e087f47b778b255b3 Mon Sep 17 00:00:00 2001 From: Emi Griffith <me@emigriffith.dev> Date: Sat, 25 Jul 2026 01:36:31 -0700 Subject: [PATCH 4/4] ci: gate the prod path, fold the repo-wide checks into gate, run the daemon tests Four holes, all of which let untested or unchecked code reach an environment. - PRs into `release` ran no build and no test. release is the branch that deploys to prod, so the intended promotion path was the least-checked path in the repo: the last seven release PRs ran shellcheck and the secrets check, nothing else. pr-build now triggers on it. - shell-lint and secrets-guard reported as standalone statuses outside `gate`, and pr-build's own setup notes tell the operator to require only `gate`. Taken literally that means a commit adding a plaintext SOPS file was mergeable. Both are now called from pr-build and reduced into gate. They are deliberately not domain-gated and not `needs: changes`: they are repo-wide invariants that must hold on every PR, including one touching no app domain -- exactly the case where every domain job skips and gate used to pass vacuously. For the same reason `skipped` counts as a failure for these two, while it stays a pass for the domain jobs it legitimately describes. They keep their standalone pull_request trigger, so they will run twice on a PR until branch protection is confirmed to require only `gate`. Cheap, and it avoids breaking a rule that may still require them by name. - build-push.yml built and pushed with no test step, and deploy.yml consumes a TAG rather than a commit status -- so an image reaching the registry by any route other than a dev/main PR (a direct push, a dispatch, a v*.*.* tag) was never tested by CI, and beta/prod then rolled it. The test now sits between build and push, so the artifact that deploys is the artifact that was tested. Gating the artifact is what makes this work; gating the branch would not. - backend/Dockerfile ran `go build` on daemon/ but never its tests. All 48 of them (gateway, cron, apiclient, config) ran nowhere: `grep -rn "go test" .forgejo/workflows/` found nothing, while the daemon owns the prod Discord gateway websocket and every recurring-job timer. It now runs gofmt + vet + test before the build, which is the pattern frontend/Dockerfile already uses -- and being inside `docker build` it gates build-push too. Verified clean: gofmt reports nothing, vet passes, all four packages pass. --- .forgejo/workflows/build-push.yml | 20 +++++++++++++++ .forgejo/workflows/pr-build.yml | 37 +++++++++++++++++++++++++--- .forgejo/workflows/secrets-guard.yml | 5 ++++ .forgejo/workflows/shell-lint.yml | 5 ++++ backend/Dockerfile | 11 +++++++++ 5 files changed, 75 insertions(+), 3 deletions(-) diff --git a/.forgejo/workflows/build-push.yml b/.forgejo/workflows/build-push.yml index e9e1fc3..a3f7893 100644 --- a/.forgejo/workflows/build-push.yml +++ b/.forgejo/workflows/build-push.yml @@ -131,6 +131,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 578aa9d..9147952 100644 --- a/.forgejo/workflows/pr-build.yml +++ b/.forgejo/workflows/pr-build.yml @@ -19,7 +19,11 @@ name: PR build (required check) on: pull_request: - branches: [dev, main] + # `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: group: pr-build-${{ github.event.pull_request.number }} @@ -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. -- 2.45.2