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.
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.
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 &plusmn;7-day seasonal window ...">
<title>Weather &amp; climate glossary: ...</title>
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 <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.
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.
