Jinemi/thermograph

Fork 0

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

Open

admin_emi wants to merge 4 commits from test/wave0-safety-and-gating into dev

admin_emi commented

2026-07-25 08:37:23 +00:00

Owner

Verified before and after: backend 457 passed / 8 skipped in 9.7s; frontend Go gofmt-clean, vet-clean, all 7 packages pass; daemon Go gofmt-clean, vet-clean, all 4 packages pass; all 9 workflow files parse.

1. `lake_app.py` `/query` — unauthenticated file read and credential disclosure

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 matters most: bucket mode installed the credentials with SET s3_secret_access_key, and settings read back out. One unauthenticated POST returned the ERA5 bucket key in plaintext — exactly what the module docstring says the service exists to prevent. /query had no auth at all, while the comparable /internal/* router has gated its whole surface since it landed.

Fixed outermost-first, because a denylist over a query language is not winnable:

/query requires the internal token. Nothing calls it — the only in-repo caller of this service is era5lake.py hitting /history, and Centralis reaches the lake via its own DuckDB, not this endpoint. So this breaks no caller. /history is left open deliberately: gating it means threading the token through the web→lake hot path, which is a bigger change than wave 0.
Credentials become a DuckDB SECRET. Verified: current_setting() returns NULL and duckdb_secrets() lists 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 can't — its views read local parquet — so the function denylist remains as defence in depth for dev and tests.

The new tests assert the guard refused, not merely that the request failed. Several 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 — the trap the original four-case test fell into.

2. Double-escaped entities in `<title>` / `<meta description>` — live on `main`

pages.yaml title/description are interpolated as plain strings, so html/template escapes them; an entity in the YAML gets escaped twice and the user sees the source. Currently serving:

<meta name="description" content="... a &amp;plusmn;7-day seasonal window ...">
<title>Weather &amp;amp; climate glossary: ...</title>

i.e. a literal ±7-day in the SERP snippet and & in the browser tab. The Python→Go rewrite didn't change it: the entities are 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, so the entities and <b> 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 its builder stage, so it gates the image rather than only local runs. Confirmed it fails when the entity is put back.

3. A test run could message real subscribers, silently, and report green

Every send gate in notifications/ reads config from ambient env at import time, and conftest neutralised none of it. test_notify.py calls run_pass() seven times without patching notifications.discord, and notify.py:358 reaches discord.post_subscription_alert() for every notification created. 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 turned pytest tests/notifications into a live broadcast. CI was safe only by omission: no credentials in the container, and one added -e removed that.

Two layers: config blanked so every gate reports False (the state CI already runs in, now guaranteed rather than lucky), plus the transports replaced with sentinels that raise, so a forgotten patch fails loudly instead of sending. Tests needing a send path patch these themselves — the escape hatch becomes visible in the test that takes it.

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 only symptom being a message delivered to somebody.

Nothing in the suite relied on a live transport.

4. CI: four holes that let untested code reach an environment

PRs into release ran no build and no test. release 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 and nothing else.
shell-lint and secrets-guard sat outside gate, while pr-build's own setup notes tell the operator to require only gate. Taken literally, a commit adding a plaintext SOPS file was mergeable. Both are now reduced into gate, deliberately not domain-gated and not needs: changes — they're repo-wide invariants that must hold even on a PR touching no app domain, which is precisely when every domain job skips and gate passed vacuously. For the same reason skipped counts as failure for these two while staying a pass for the domain jobs it legitimately describes.
build-push.yml pushed images with no test step, and deploy.yml consumes a tag, not a commit status — so an image reaching the registry by any other route (direct push, dispatch, v*.*.* tag) was never tested, and beta/prod then rolled it. The test now sits between build and push: the artifact that deploys is the artifact that was tested. Gating the artifact is what makes this work; gating the branch wouldn't.
The 48 daemon Go tests ran nowhere. grep -rn "go test" .forgejo/workflows/ found nothing, while the daemon owns the prod Discord gateway websocket and every recurring-job timer. backend/Dockerfile now runs gofmt + vet + test before the build, matching the pattern frontend/Dockerfile already uses — and being inside docker build, it gates build-push too.

Notes for review

shell-lint / secrets-guard will run twice per PR until branch protection is confirmed to require only gate. Cheap, and it avoids breaking a rule that may still require them by name. Worth checking Settings → Branches and then dropping their standalone pull_request trigger.
lake_app.py imports _require_internal_token, a private name. Verified it adds no import weight — the lake role stays free of sqlalchemy/accounts/notifications/web. Promoting it to a public name would be tidier.
/history on the lake service is still unauthenticated (see item 1).
No Claude-Session trailer on these commits: backend/CLAUDE.md and frontend/CLAUDE.md both require that commits never reference automated authorship.

Remaining waves (deterministic fixtures, the 40%-tier-boundary estimator mismatch, golden reference values, migration tests, the Postgres tier, infra as a CI domain) are not in this PR.

Wave 0 of the testing-suite plan — the "stop the bleeding" items. Two live defects, one safety mechanism, four CI gating holes. Five of the six items are here; the sixth is Centralis and ships separately. Verified before and after: **backend 457 passed / 8 skipped in 9.7s**; frontend Go gofmt-clean, vet-clean, all 7 packages pass; daemon Go gofmt-clean, vet-clean, all 4 packages pass; all 9 workflow files parse. ## 1. `lake_app.py` `/query` — unauthenticated file read and credential disclosure 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 matters most: bucket mode installed the credentials with `SET s3_secret_access_key`, and settings read back out. One unauthenticated POST returned the ERA5 bucket key in plaintext — exactly what the module docstring says the service exists to prevent. `/query` had no auth at all, while the comparable `/internal/*` router has gated its whole surface since it landed. Fixed outermost-first, because a denylist over a query language is not winnable: - **`/query` requires the internal token.** Nothing calls it — the only in-repo caller of this service is `era5lake.py` hitting `/history`, and Centralis reaches the lake via its own DuckDB, not this endpoint. So this breaks no caller. `/history` is left open deliberately: gating it means threading the token through the web→lake hot path, which is a bigger change than wave 0. - **Credentials become a DuckDB SECRET.** Verified: `current_setting()` returns NULL and `duckdb_secrets()` lists 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 can't — its views read local parquet — so the function denylist remains as defence in depth for dev and tests. The new tests assert **the guard refused**, not merely that the request failed. Several 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 — the trap the original four-case test fell into. ## 2. Double-escaped entities in `<title>` / `<meta description>` — live on `main` `pages.yaml` title/description are interpolated as plain strings, so `html/template` escapes them; an entity in the YAML gets escaped twice and the user sees the source. Currently serving: ``` <meta name="description" content="... a &plusmn;7-day seasonal window ..."> <title>Weather &amp; climate glossary: ...</title> ``` i.e. a literal `±7-day` in the SERP snippet and `&` in the browser tab. The Python→Go rewrite didn't change it: the entities are 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, so the entities and `<b>` 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 its builder stage, so it gates the image rather than only local runs. Confirmed it fails when the entity is put back. ## 3. A test run could message real subscribers, silently, and report green Every send gate in `notifications/` reads config from ambient env at import time, and conftest neutralised none of it. `test_notify.py` calls `run_pass()` seven times without patching `notifications.discord`, and `notify.py:358` reaches `discord.post_subscription_alert()` for every notification created. 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 turned `pytest tests/notifications` into a live broadcast. CI was safe only by omission: no credentials in the container, and one added `-e` removed that. Two layers: config blanked so every gate reports False (the state CI already runs in, now guaranteed rather than lucky), plus the transports replaced with sentinels that **raise**, so a forgotten patch fails loudly instead of sending. Tests needing a send path patch these themselves — the escape hatch becomes visible in the test that takes it. `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 only symptom being a message delivered to somebody. Nothing in the suite relied on a live transport. ## 4. CI: four holes that let untested code reach an environment - **PRs into `release` ran no build and no test.** `release` 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 and nothing else. - **`shell-lint` and `secrets-guard` sat outside `gate`,** while pr-build's own setup notes tell the operator to require only `gate`. Taken literally, a commit adding a plaintext SOPS file was mergeable. Both are now reduced into `gate`, deliberately *not* domain-gated and *not* `needs: changes` — they're repo-wide invariants that must hold even on a PR touching no app domain, which is precisely when every domain job skips and `gate` passed vacuously. For the same reason `skipped` counts as failure for these two while staying a pass for the domain jobs it legitimately describes. - **`build-push.yml` pushed images with no test step,** and `deploy.yml` consumes a *tag*, not a commit status — so an image reaching the registry by any other route (direct push, dispatch, `v*.*.*` tag) was never tested, and beta/prod then rolled it. The test now sits between build and push: the artifact that deploys is the artifact that was tested. Gating the artifact is what makes this work; gating the branch wouldn't. - **The 48 daemon Go tests ran nowhere.** `grep -rn "go test" .forgejo/workflows/` found nothing, while the daemon owns the prod Discord gateway websocket and every recurring-job timer. `backend/Dockerfile` now runs gofmt + vet + test before the build, matching the pattern `frontend/Dockerfile` already uses — and being inside `docker build`, it gates `build-push` too. ## Notes for review - `shell-lint` / `secrets-guard` will run **twice** per PR until branch protection is confirmed to require only `gate`. Cheap, and it avoids breaking a rule that may still require them by name. Worth checking Settings → Branches and then dropping their standalone `pull_request` trigger. - `lake_app.py` imports `_require_internal_token`, a private name. Verified it adds no import weight — the lake role stays free of sqlalchemy/accounts/notifications/web. Promoting it to a public name would be tidier. - `/history` on the lake service is still unauthenticated (see item 1). - No `Claude-Session` trailer on these commits: `backend/CLAUDE.md` and `frontend/CLAUDE.md` both require that commits never reference automated authorship. Remaining waves (deterministic fixtures, the 40%-tier-boundary estimator mismatch, golden reference values, migration tests, the Postgres tier, `infra` as a CI domain) are not in this PR.

admin_emi added 4 commits 2026-07-25 08:37:24 +00:00

lake: gate /query and stop it reading outside the two lake views 63e1967bbb

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.

content: write the entities literally in pages.yaml titles and descriptions e377c4de03

pages.yaml's title/description are interpolated as plain strings into <title>
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 &amp;plusmn;7-day seasonal window ...">
  <title>Weather &amp;amp; climate glossary: ...</title>

which renders as a literal "&plusmn;7-day" in the SERP snippet and "&amp;" 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 <b> 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.

tests: hard-block outbound transports, and assert the block holds 35006466bf

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.

ci: gate the prod path, fold the repo-wide checks into gate, run the daemon tests

PR build (required check) / changes (pull_request) Successful in 7s

Details

secrets-guard / encrypted (pull_request) Successful in 6s

Details

PR build (required check) / validate-observability (pull_request) Has been skipped

Details

shell-lint / shellcheck (pull_request) Successful in 16s

Details

PR build (required check) / lint-shell (pull_request) Successful in 23s

Details

PR build (required check) / guard-secrets (pull_request) Successful in 26s

Details

PR build (required check) / build-frontend (pull_request) Successful in 1m41s

Details

PR build (required check) / build-backend (pull_request) Successful in 2m13s

Details

PR build (required check) / gate (pull_request) Successful in 2s

Details

8432144bb3

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.