security: track A hardening (closes #201, closes #202, closes #203, closes #204, closes #205, closes #206, closes #207) - #222
Merged
Conversation
The previous stub pointed at a CLAUDE.md section (In-flight multi-PR series) that no longer exists. Replace it with the five audit track tables referencing the 2026-07 backlog (#201-#221) and ordering notes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…ices The DISCORD_WEBHOOK_URL default in duckdb-service/services/discord.py AND image-service/services/discord.py was a live webhook URL — a bearer credential shipped to every clone. Default is now empty (= notifications disabled), the value flows only through the env var (wired in both compose files), and check-no-hardcoded-api-keys.sh gains a discord.com/api/webhooks pattern so the class can't recur. The webhook itself must be rotated by the operator before this merges; the old URL stays in git history either way. For #201 (scope widened: the audit issue cited only duckdb-service; image-service carried the same default). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…filename identity delete_image joined the client-supplied <path:filename> straight into os.remove — ../ escaped the upload folder (reads were only safe because send_from_directory contains). New services/paths.py provides safe_child_path (realpath containment, used by delete + both serve routes) and sanitize_upload_filename + dedupe_filename: fleet-grammar names pass byte-identical, hostile names normalize, and colliding names get a -N suffix instead of silently overwriting (fleet filenames carry no module identity — same-second captures from two modules used to clobber each other). The stored name now flows to the DB row, sidecar, snips, Discord, and response. For #202. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…kdb-service in dev /upload gets a 5 MB MAX_CONTENT_LENGTH ceiling (JSON 413) and a per-module sliding-window rate guard (services/upload_throttle.py, default 30/h, env-overridable, tracked-MAC dict bounded against spoof floods). Over-budget uploads are accepted-and-discarded with a 200, deliberately not a 429: any non-2xx counts toward the firmware's 5-failure circuit breaker (client.cpp) and would reboot a module mid-capture-storm, amplifying the storm capture_gate (ADR-024) already bounds device-side. Dev compose now binds duckdb-service to 127.0.0.1:8002 matching prod — the sole DB writer's unauthenticated internal endpoints were LAN-reachable from every dev box. For #203. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…production Both Flask services resolved key-or-dev-fallback silently for their admin-gated /logs endpoints; prod safety rested entirely on the compose :? interpolation, which the PM2/bare-metal path doesn't have. New twin services/prod_guard.py (per-service copy, log_ring precedent) raises at app import when HIGHFIVE_ENV=production and HIGHFIVE_API_KEY is unset, blank, or the dev fallback in any casing — mirroring backend/src/auth.ts edge cases. docker-compose.prod.yml sets the marker for both services; PM2 hosts set it in server-side process config (documented in auth.md). Dev behaviour byte-identical. For #204. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…backend's progress fetch add_progress_for_module raised straight to a 500 on any non-JSON or schema-violating body — the only write route without the validation idiom its siblings use. Now: silent JSON parse + ValidationError -> 400. get_progress returned the entire daily_progress table unconditionally; it now accepts optional module_id/since/until/limit params (400 on bad values) with an explicit date-ascending ordering contract, and limit keeps the most recent rows — the latest-is-last invariant the backend totalHatches roll-up depends on is now pinned by test and documented. The backend passes limit=100000 as a safety valve (no behaviour change today; oldest-dropped-first if it ever bites). For #205. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…serLocation cache POST /api/waitlist was an unthrottled anonymous relay into the operator's Discord alert channel — one client could flood it and drown real silence-watcher alerts. New generic SlidingWindowLimiter (rateLimit.ts, separate instance + semantics from the login failure-counter in session.ts) gates it at 3/hour/IP with a 429 the homepage renders as a translated retry-later message (EN+DE keys). userLocation's per-IP cache — whose own comment admitted unbounded growth — now sweeps expired entries and evicts oldest-inserted at a 5000-entry cap. Both limiter and cache bounds are test-pinned, including the address-diverse-flood cases. For #206. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
Full-tree sweep found no remaining emitter of the legacy typo — the only in-tree sender (upload_pipeline's _record_progress) emits the canonical module_id, and the last modul_id fixture (the dev-tools Postman collection) is corrected in this commit. AliasChoices removed from ClassificationOutput; a typo'd payload now fails validation with a clean 400, pinned by the flipped rejection tests. Docs (chapter 11, api-contracts, glossary, api-reference, upload-flow) updated to window-closed. For #207. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…only waitlist budget Three review-caught fixes on the audit branch: (1) the filename dedupe was check-then-write and raced under threaded Flask — reserve_filename now claims the name atomically via O_CREAT|O_EXCL placeholder; (2) get_progress's limit guard used isdigit(), which admits Unicode superscripts that int() rejects — a malformed-input 500 reintroduced through the back door, now try/except with a pinned '³' case; (3) the waitlist limiter consumed budget before validation and the webhook check, so three email typos or a Discord outage locked a legitimate signer out for an hour while providing zero flood protection — budget is now consumed only by submissions that reach the relay. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
… fails reserve_filename claims a name by creating an empty placeholder; a failed req.image.save (disk full, truncated multipart) previously left that 0-byte ghost behind, shifting future same-name uploads to -1 suffixes. The reservation is now unlinked on any save failure so the directory is left exactly as found. Test-pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
…-plan-dypztx # Conflicts: # docs/11-risks-and-technical-debt/README.md
…eploy
Senior review of the track-A hardening. The controls are sound; several were
wired to a deployment the fleet and the auto-deploy driver don't use.
P0 -- the dev loopback bind breaks the hardware bench outright. Verified
against the firmware, not the commit message: ESP32-CAM/extra_scripts.py bakes
HF_INIT_URL_DEFAULT = http://<DEV_SERVER_HOST>:8002/new_module into every
LAN-dev build (HF_DEV_BUILD=1 hard-fails without DEV_SERVER_HOST, so it is the
only supported dev firmware path), esp_init.cpp registers against that URL, and
client.cpp's sendHeartbeat reuses it "purely as the carrier of host+port".
Binding dev's 8002 to 127.0.0.1 removes the module's only route in: registration
fails and heartbeats stop, silently. Prod can be loopback-bound because
host-Nginx proxies exactly those two paths -- docker-compose.prod.yml's own
comment says so, and calls them "the only two ESP firmware paths that hit
duckdb-service directly". Dev has no Nginx.
The PR also shipped a doc asserting the opposite ("that flow talks to
image-service and the backend, never to duckdb-service directly"), which is
the failure CLAUDE.md's never-trust-commit-messages rule exists to catch.
Reverted the dev bind, rewrote the doc to state the real dev/prod split and
what to do on an untrusted network, and left prod's loopback bind untouched --
it was already there, so the dev change bought nothing and cost the bench.
P1 -- two controls were no-ops on the deployment that actually ships.
HIGHFIVE_ENV appears only in docker-compose.prod.yml, but scripts/deploy.sh
reloads pm2 apps whose env comes from the ecosystem.config.js template in
production-runbook.md. So require_prod_key() returned at its first `if` and
#204 never fired on the live host; the same omission left DISCORD_WEBHOOK_URL
unset there, which silently disables the ADR-005 silence watcher -- the
operator's primary field-failure signal -- because send_discord_message
degrades to a print(). Added the required env for both Python services to the
runbook with a concrete pm2 snippet, a table of what breaks without each, and
a --update-env verification command.
P1 -- auth.md called /upload "bounded, not authenticated". The guard keys on
the client-supplied, unauthenticated MAC, so it bounds a runaway MODULE and
not a hostile client, which can rotate MACs for a fresh budget each time;
_MAX_TRACKED bounds the dict, not the writes. Corrected to say what is
actually bounded, and filed #224 -- deliberately not "fixed" here, because the
obvious per-IP budget throttles a whole site behind one NAT egress, and a
security fix that breaks ingestion is worse than the bug. Added a test that
plays the actual adversary and asserts the gap, so closing it later fails
loudly rather than leaving a green suite over a hole. (The existing
bounded-dict test performs that exact attack and asserts only the dict stays
small -- it demonstrates the hole while looking like a defence.)
Cross-version bug: routes/progress.py used date.fromisoformat, which widened
in 3.11 to accept 20260719 and 2026-W29-1. With CI spanning 3.10-3.14
(ADR-029) the same query string 400s on 3.10 and 200s on 3.12 -- an API
contract that depends on the interpreter. Now strptime with the documented
format, pinned by a test over all the widened forms.
upload_throttle: added a lock and switched the call site to time.monotonic().
The clock bug is real (an NTP step backwards leaves future-stamped events that
never age out). On the lock I am being explicit that I could NOT reproduce the
race on 3.12 even with setswitchinterval(1e-9) and 16 contending threads; it
stays because the GIL is an implementation detail and the CI matrix already
runs 3.13/3.14, and the test says exactly that rather than pretending to prove
a race it cannot trigger.
Verified: image-service 145/145, duckdb-service 261/261, backend 292/292,
homepage 193/193, shellcheck clean, compose config valid, doc-citation and
hardcoded-key gates green.
Co-Authored-By: WOZCODE <contact@withwoz.com>
….py file The pre-commit hook invokes bare `ruff` on duckdb-service/ and image-service/ Python files, but pip installs ruff.exe into Python's Scripts\ directory, which is not on PATH by default on Windows. So `py -m ruff --version` works while `ruff --version` does not, and only Python-touching commits fail — which reads as intermittent. Cost a debugging cycle in this session; records the fix, the one-off Git Bash workaround, and why --no-verify is the wrong answer. Also notes the concurrent prettier SIGKILL as memory pressure rather than a real prettier failure. Co-Authored-By: WOZCODE <contact@withwoz.com>
…nippet Round-2 review of the track-A hardening. The embarrassing one first: round 1 was about a bind change whose docs said the opposite of the code, and the revert fixed the compose file and one doc while leaving auth.md -- the security document's own enumeration of unauthenticated surfaces -- still asserting "both compose files bind 127.0.0.1:8002". A branch that contradicted itself two files apart. Rewritten to state the real asymmetry: loopback in prod because host-Nginx proxies /new_module and /heartbeat, LAN-published in dev because there is no Nginx and the firmware's baked HF_INIT_URL_DEFAULT has no other route in, therefore treat a running dev stack as trusted-LAN-only. Since a grep would have caught that in two seconds and `make check-citations` cannot (it proves a citation RESOLVES, not that a sentence is TRUE), this adds scripts/check-duckdb-bind-claims.sh: prod must stay loopback-bound, dev must not be, and no doc may claim both. Verified it fails on the exact regression (re-applied the loopback bind, watched it fail, reverted). Wired into the Makefile, .husky/pre-push, and CI alongside the sibling gates, and recorded in ci-gates.md. Second P0 -- #201 removed a hardcoded webhook (a live credential in a public repo, correct to remove) and wired DISCORD_WEBHOOK_URL through the compose files. The previous round documented it for the PM2 path only, so on the SUPPORTED Docker production path .env.production.example and production-deployment.md still said nothing about it, and the file that promises "compose will FAIL FAST if any of these are unset" interpolates this one as :- rather than :?. Net effect of a security PR: the ADR-005 silence watcher -- the operator's only signal that a field module went quiet -- goes dark on deploy with nothing to notice. Both files now say so, with a verify command. The pm2 snippet added last round did not work as written. process.env.X inside ecosystem.config.js is evaluated by the pm2 CLI, whose environment does not include /var/www/highfive/.env, and neither Python service imports dotenv (the backend does). Following it verbatim gave HIGHFIVE_ENV=production with no key -> prod_guard raises at import -> autorestart crash loop on both services. Now loads dotenv explicitly. Also corrected "pm2 restart ignores edits without --update-env": --update-env on a NAME refreshes from the invoking shell and does not re-read the ecosystem file at all -- and since deploy.sh reloads with --update-env from a shell sourcing .deploy.env, that file can strip these values on the next tick, so it now carries HIGHFIVE_ENV/HIGHFIVE_API_KEY too. api-reference.md and the handler comment both claimed a webhook outage does not consume waitlist budget. allow() runs before the fetch, so it does. Kept the behaviour (refunding on failure would let a flooder farm unlimited relay attempts while Discord is down) and fixed both claims to describe it. Two chapter-11 lessons added, since round 1 and round 2 each produced one and neither had a durable home: "hardening dev to match prod removed the compensating mechanism prod has and dev doesn't", and "a control wired into docker-compose.prod.yml is inert on the host that actually deploys". P2s: the hardcoded-key gate missed the legacy discordapp.com webhook host -- confirmed by planting one and watching it pass -- now discord(app)?\.com and confirmed failing on both hosts; the ruff troubleshooting entry no longer hardcodes one user's profile path; the strptime docstring no longer claims "strictly" (2026-7-9 is accepted, identically on every matrix Python, which is the actual point); the runbook's LOG_DIR no longer points at /data, which is the Docker volume path and does not exist on a bare-metal host. Verified: duckdb-service 261/261, image-service 145/145, all five repo gates green including the new one, shellcheck -S info clean, compose config valid. Co-Authored-By: WOZCODE <contact@withwoz.com>
…tring
Round-3 review. The gate I added last round to stop this class of regression
was itself defeated in one keystroke, which is the finding worth leading with.
The old gate matched the literal `'127.0.0.1:8002:8000'`. Three evasions,
all reproduced here before fixing:
* switching dev to DOUBLE quotes reintroduced the exact regression and the
gate printed OK;
* deleting the dev mapping entirely also printed "LAN-reachable", which was
then a lie;
* a quote-style change in PROD made it fail on an unchanged bind.
It was a snapshot test of one sentence sold as a semantic invariant.
Rewritten to ask `docker compose config --format json` for the RESOLVED
host_ip of the 8002 publisher. That is immune to quoting, short-vs-long ports
syntax, and interpolation. Verified against all four shapes: double-quoted
loopback FAILS, long-form `host_ip: 127.0.0.1` FAILS, ports-key-removed FAILS
with a message about the fleet losing its only route in, an invalid ports list
FAILS as a parse error, and the correct tree passes. Also dropped the
doc-sentence arm entirely -- a tripwire keyed to one phrasing costs more to
maintain than it buys, and it would have false-failed on the chapter-11 lesson
that narrates the incident using the same words.
Two script bugs found while testing it: the first version exited 0 with NO
output when compose rejected the file (set -e killed it inside a command
substitution), and it called python3, which does not exist in Git Bash on
Windows -- the same detection ESP32-CAM/build.sh already does for #99.
Self-contradictions the review found, all now fixed:
- api-reference claimed a 503 consumes waitlist budget. The
DISCORD_WEBHOOK_URL-unset branch returns BEFORE the limiter, so it does not.
The status list is now derived from where the check actually sits: 400 no,
503 no, 502 yes.
- The runbook's env table told a bare-metal operator to use LOG_DIR=/data
while the snippet twelve lines below said "NOT /data - that is the Docker
volume path". The table row is now correct.
- .deploy.env.example shipped HIGHFIVE_ENV=production beside a BLANK
HIGHFIVE_API_KEY. Under the precedence that same file documents, an operator
who filled only the webhook would export production-with-no-key into both
Flask services and crash-loop them via the guard this PR added. Both lines
are now commented out with "set these together or neither".
- .env.production.example promised "FAIL FAST if any of these are unset" while
this PR added a `:-` variable directly below it.
Wire-shape documentation CLAUDE.md's table required and the PR had skipped:
/upload gained a 413 and a SECOND 200 shape carrying none of
mac/battery/filename/classification. Both are now in api-reference.md 2.2 and
api-contracts.md, with the instruction to branch on the presence of `filename`
rather than on the status code, and the reason the throttle is a 200 and must
not be "fixed" to a 429 (it would reboot a storming module via the firmware's
circuit breaker).
Also: Makefile help entry for the new gate.
Verified: all five repo gates green, shellcheck -S info clean, compose config
valid, CI workflow YAML parses.
Co-Authored-By: WOZCODE <contact@withwoz.com>
The gate passed locally and failed on its first CI run: docker-compose.yml declares `env_file: - .env`, and `docker compose config` refuses to render a file whose env_file is missing. `.env` is gitignored, so it exists on a developer box and never on a runner — the one environment difference that a gate resolving compose files was always going to trip over, and that grepping the file would not have. The gate now creates a throwaway .env when absent and removes ONLY what it created, so a developer's real .env is never touched. Verified all four ways: with no .env it passes on a correct tree and still FAILS on a reintroduced loopback bind; with a real .env present it passes and leaves the file byte-identical; and it leaves no stray .env behind either way. Co-Authored-By: WOZCODE <contact@withwoz.com>
…lean The cleanup trap was `[ "$CREATED_ENV" = "1" ] && rm -f …`. When the repo already has a .env — i.e. every developer machine — the test fails, and an EXIT trap whose last command fails sets the script's exit status. So the gate printed three OK lines and exited 1. It only showed up under the pre-push hook, because running it interactively and reading the output looks like success; you have to check $? to see it. Now the trap ends with an explicit `return 0`. Verified all four combinations by exit code, not by eyeballing output: passes with a real .env present, passes with none, still FAILS on a reintroduced loopback bind, and the full pre-push hook now exits 0. Co-Authored-By: WOZCODE <contact@withwoz.com>
cofade
added a commit
that referenced
this pull request
Aug 18, 2026
Senior review of the production-branch adoption. Two P0s, both verified with single git commands against the repo rather than read off the prose. 1. THE DOCUMENTED CUTOVER WOULD HAVE REVERTED PRODUCTION. The procedure said "git checkout production && git reset --hard origin/production" with no promotion step. But origin/production is a0e7374, four commits behind main: git show origin/production:scripts/deploy.sh | grep '^BRANCH=' -> "main" ...and 0 hits for FAILED_MARKER|NPM_CI_RAN|PIP_FAILED|add_reload| rollback_health_targets|HUSKY=0 (26 on this branch) So running it today rolls the live host back past #193, #196 and #222 -- discarding the deploy hardening and the security track-A work -- and leaves the checkout on `production` running a driver that tracks `main`. That takes the old bare `log "skip"; exit 0` path: deploys stop silently, forever, and the wrong-branch alert that would have said so was reverted away with everything else. Silent, permanent, self-concealing. The procedure is now explicitly ordered: promote, VERIFY (`git show origin/production:scripts/deploy.sh | grep '^BRANCH='` must print production), then check out. The same ordering and the same verify command are in the Discord alert body, because that is what the operator actually reads at 2am. 2. ADR-030's recorded root cause was FALSE, and it was the sole justification for force-resetting a release branch: git rev-list --max-parents=0 main -> d9ac93d (one root) git rev-list --max-parents=0 bf8b314 -> d9ac93d (the SAME root) git merge-base main bf8b314 -> da1b21d git rev-list --count main..bf8b314 -> 25 (not 136) There was no orphan root, no rebuilt history, no unrelated ancestry -- a merge was available the entire time and was rejected for tidiness. The cited #124 is a senior-reviewer config commit. Corrected in ADR-030 and in chapter 11, kept as a visible correction rather than a quiet edit, because ch11 had already generalised the false cause into a "how to avoid this next time" rule that future maintainers would have applied to a scenario that never happened. Also from this round: - "A real promotion gate" was overstated: neither branch has protection, CI runs on main only, and any fast-forwarding commit is accepted. Now says gate-by-convention, and states that production MUST stay unprotected -- protecting it would reject publish_firmware's push and ship OTAs whose SEQUENCE bump is not in git. - The ADR's one acknowledged invariant violation (publish_firmware commits to production) had "tracked as a follow-up" with nothing behind it. Filed #225. - The OTA notification -- attached to the single irreversible action in the system -- cited ADR-028 (ML inference server-side) instead of ADR-030. This is the ADR-renumber-on-collision trap the repo already documented. - The archive tag is NOT on the remote (`git ls-remote --tags origin` finds nothing), so the 25 commits survive only via a stale branch clean_gone would delete. Both docs now say so instead of claiming a recovery point exists. - CONTRIBUTING.md, which CLAUDE.md names as the authority for the branch model, said only "branch off main" and never mentioned production. It now documents the promote-don't-PR rule and the never-force-push constraint. - The branch-mismatch marker is hoisted to a BRANCH_MARKER constant next to FAILED_MARKER (it was a local var re-typed as a literal in the rm), and the marker is now written BEFORE notify -- notify can fail under set -e, which would have produced the every-two-minutes alert the marker exists to stop. Not done, stated plainly: image-service/tests/test_upload.py carries ruff format reflow. main's version is not ruff-clean, and the pre-commit hook reformats any staged .py, so it cannot be reverted without bypassing the hook. Verified: bash -n and shellcheck -S info clean; #196's 11-case rollback harness and 13-case gate matrix still pass; all five repo gates green. Co-Authored-By: WOZCODE <contact@withwoz.com>
cofade
added a commit
that referenced
this pull request
Aug 18, 2026
* chore: adopt production as the gated release source (#152) Reconcile the documented services deploy source with reality and unify it with firmware OTA on a single gated `production` branch. Investigation for #152 found three stacked problems: the docs named `production` while the live auto-deploy pulled `main`; firmware OTA and the services track were documented as separate; and `main`/`production` shared no common git ancestor (main's history was rebuilt), so `production` could never fast-forward and silently rotted. Decision (per maintainer): `production` becomes the single gated release branch for both web services and firmware OTA. `main` is the integration line; a release is a fast-forward of `production` onto a chosen `main` commit. `prod-*` tags are cut on `production`. - scripts/deploy.sh: BRANCH main -> production; branch-agnostic notify text - production-deployment.md: drop drift warning; add release/promotion + one-time host cutover section - production-runbook.md: document the promote-then-pull model - firmware-release.md: rewrite the branch & tag model (both tracks on production); replace the "known drift" callout with a history note; update the release-checklist commit/tag step - chapter 11: mark the drift lesson RESOLVED; record the unrelated-history root cause and the fast-forwardable-deploy-branch rule - new ADR-028; update README/esp-flashing/CLAUDE.md pointers The branch reconciliation (archive tag + force-reset of origin/production) and the one-time prod-host checkout are operator steps documented in ADR-028 and production-deployment.md, to run after this lands on main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017drgAN84qrn61eZ1yZTdgS * docs: add "never release from main" critical rule to CLAUDE.md The production-as-gated-release-branch model (#152 / ADR-030) was only spelled out in the firmware-OTA section. Add a concise hard rule to the top-level "Critical rules (do NOT violate)" list so every session knows prod releases ship from `production`, never from `main`. Links to the full mechanics rather than duplicating them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: correct ADR-030's root cause and invert the cutover order Senior review of the production-branch adoption. Two P0s, both verified with single git commands against the repo rather than read off the prose. 1. THE DOCUMENTED CUTOVER WOULD HAVE REVERTED PRODUCTION. The procedure said "git checkout production && git reset --hard origin/production" with no promotion step. But origin/production is a0e7374, four commits behind main: git show origin/production:scripts/deploy.sh | grep '^BRANCH=' -> "main" ...and 0 hits for FAILED_MARKER|NPM_CI_RAN|PIP_FAILED|add_reload| rollback_health_targets|HUSKY=0 (26 on this branch) So running it today rolls the live host back past #193, #196 and #222 -- discarding the deploy hardening and the security track-A work -- and leaves the checkout on `production` running a driver that tracks `main`. That takes the old bare `log "skip"; exit 0` path: deploys stop silently, forever, and the wrong-branch alert that would have said so was reverted away with everything else. Silent, permanent, self-concealing. The procedure is now explicitly ordered: promote, VERIFY (`git show origin/production:scripts/deploy.sh | grep '^BRANCH='` must print production), then check out. The same ordering and the same verify command are in the Discord alert body, because that is what the operator actually reads at 2am. 2. ADR-030's recorded root cause was FALSE, and it was the sole justification for force-resetting a release branch: git rev-list --max-parents=0 main -> d9ac93d (one root) git rev-list --max-parents=0 bf8b314 -> d9ac93d (the SAME root) git merge-base main bf8b314 -> da1b21d git rev-list --count main..bf8b314 -> 25 (not 136) There was no orphan root, no rebuilt history, no unrelated ancestry -- a merge was available the entire time and was rejected for tidiness. The cited #124 is a senior-reviewer config commit. Corrected in ADR-030 and in chapter 11, kept as a visible correction rather than a quiet edit, because ch11 had already generalised the false cause into a "how to avoid this next time" rule that future maintainers would have applied to a scenario that never happened. Also from this round: - "A real promotion gate" was overstated: neither branch has protection, CI runs on main only, and any fast-forwarding commit is accepted. Now says gate-by-convention, and states that production MUST stay unprotected -- protecting it would reject publish_firmware's push and ship OTAs whose SEQUENCE bump is not in git. - The ADR's one acknowledged invariant violation (publish_firmware commits to production) had "tracked as a follow-up" with nothing behind it. Filed #225. - The OTA notification -- attached to the single irreversible action in the system -- cited ADR-028 (ML inference server-side) instead of ADR-030. This is the ADR-renumber-on-collision trap the repo already documented. - The archive tag is NOT on the remote (`git ls-remote --tags origin` finds nothing), so the 25 commits survive only via a stale branch clean_gone would delete. Both docs now say so instead of claiming a recovery point exists. - CONTRIBUTING.md, which CLAUDE.md names as the authority for the branch model, said only "branch off main" and never mentioned production. It now documents the promote-don't-PR rule and the never-force-push constraint. - The branch-mismatch marker is hoisted to a BRANCH_MARKER constant next to FAILED_MARKER (it was a local var re-typed as a literal in the rm), and the marker is now written BEFORE notify -- notify can fail under set -e, which would have produced the every-two-minutes alert the marker exists to stop. Not done, stated plainly: image-service/tests/test_upload.py carries ruff format reflow. main's version is not ruff-clean, and the pre-commit hook reformats any staged .py, so it cannot be reverted without bypassing the hook. Verified: bash -n and shellcheck -S info clean; #196's 11-case rollback harness and 13-case gate matrix still pass; all five repo gates green. Co-Authored-By: WOZCODE <contact@withwoz.com> * docs: retract the history claim in the runbook too, and price the cutover honestly Round-2 review. The pattern in the finding is the same one this PR is about. 1. The false "shared no common ancestor" claim was fixed in ADR-030 and chapter 11 and LEFT STANDING in docs/07-deployment-view/firmware-release.md -- so the branch shipped three documents describing one event, two of which called the third a fabrication. The survivor was the runbook: the file someone opens WHILE cutting a release, while the retractions sat in an ADR and a tech-debt log nobody reads mid-release. Fixed, with the retraction visible there too. Swept the tree afterwards; the only remaining matches are inside the correction blocks that quote the claim in order to retract it. 2. "verified stale" did not survive its own standard. The ADR had just spent twenty lines explaining why unverified assertions here are dangerous, and then rested the whole justification for discarding 25 commits on an enumeration that omitted four test files and never mentioned the ESP work. Re-derived from the repo, and one finding is worth the trouble: the archived TIP (bf8b314, "use esp_task_wdt_reconfigure and defer loopTask subscribe past AP setup") uses an API that appears NOWHERE on main -- `git grep esp_task_wdt_reconfigure origin/main -- ESP32-CAM/` is empty, and main still uses the IDF-4 esp_task_wdt_init/add pair. Main fixes the same AP-mode reboot loop a different way (>=60s TASK_WDT_TIMEOUT_S plus runAccessPoint feeding the watchdog, recorded as fixed in troubleshooting.md), so nothing live is lost -- but "already exists on main" was the wrong description, and the ADR now says what was actually checked. 3. The OTA notification told the operator to run `git checkout main` with no statement of WHERE. It arrives while they are looking at the host, and the commands run there -- where `git checkout main` immediately trips the branch-mismatch guard this same PR adds and pauses every deploy. Now says "FROM A MAINTAINER CLONE, NOT THIS HOST" and explains the consequence. 4. The reordered cutover fixed the direction but not the rebuild. `git reset --hard` restores the source tree only; backend/dist, homepage/dist and node_modules stay at whatever the host last built, and no later tick repairs that because deploy.sh exits at `[ "$PREV_SHA" = "$REMOTE_SHA" ] && exit 0`. Step 3's own health checks pass in that skewed state. Added an explicit rebuild, and said when it can be skipped. 5. docs/02-constraints/README.md -- which CLAUDE.md's critical-rules section names as the full list -- had no production-branch rule at all. This PR found and fixed exactly that gap in CONTRIBUTING.md and left the file CLAUDE.md points at. Added, including the never-force-push and must-stay-unprotected constraints. P2s: the ADR said CI "runs on main only" (it triggers on main push+PR and never on production -- same conclusion, wrong sentence); the Decision section described the OTA publish as unconditional when the whole block is gated behind FIRMWARE_AUTO_OTA=1; and chapter 11's "how to avoid" paragraph still led with the history-rewrite scenario the correction above it calls fictional -- it now leads with the real rule (no promotion mechanism, no staleness signal). Verified: bash -n and shellcheck -S info clean, the 11-case rollback harness passes, all repo gates green. Co-Authored-By: WOZCODE <contact@withwoz.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: WOZCODE <contact@withwoz.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
What changed
The complete Security & Data Integrity track of the 2026-07 audit, one commit per issue plus two review-fix commits (the branch also carries the earlier audit roadmap update):
DISCORD_WEBHOOK_URLdefault is now""in bothservices/discord.pycopies (scope widened: the audit issue cited only duckdb-service; image-service carried the same live token). Env wiring in both compose files;check-no-hardcoded-api-keys.shgains adiscord.com/api/webhooks/<id>pattern.image-service/services/paths.py:safe_child_path(realpath containment —delete_imagehados.removeon an unguarded<path:filename>; both serve routes hardened too),sanitize_upload_filename(byte-identical for fleet-grammar names), andreserve_filename(atomicO_CREAT|O_EXCLclaim — same-second captures from two modules used to silently clobber each other). The stored name threads through DB row, sidecar, snips, Discord, and response./uploadgets a 5 MBMAX_CONTENT_LENGTH(JSON 413) and a per-MAC throttle (default 30/h, env-overridable). Over-budget uploads are accepted-and-discarded with a 200, deliberately not a 429: any non-2xx feeds the firmware's 5-failure circuit breaker (client.cpp) and would reboot a storming module. Dev compose now binds duckdb-service to127.0.0.1:8002(prod parity — the sole DB writer's unauthenticated endpoints were LAN-reachable from every dev box).services/prod_guard.py: both Flask services refuse to boot whenHIGHFIVE_ENV=productionandHIGHFIVE_API_KEYis unset/blank/the dev fallback (any casing), mirroringbackend/src/auth.ts. Prod compose sets the marker.add_progress_for_modulereturns clean 400s on malformed bodies (previously an uncaught 500);get_progressgainsmodule_id/since/until/limitfilters with an explicit date-ascending ordering contract (limittrims oldest — the latest-is-last invarianttotalHatchesdepends on is test-pinned and documented); backend passeslimit=100000as a safety valve.POST /api/waitlistrate-limited 3/h/IP via a new genericSlidingWindowLimiter(budget consumed only by submissions that reach the Discord relay); homepage shows a translated EN/DE retry-later message on 429;userLocation's per-IP cache capped at 5000 entries.modul_iddeprecation alias removed after a full-tree sweep found no emitter (the dev-tools Postman fixture was the last holdout, corrected here); rejection test-pinned.limit400 (was a 500), relay-only waitlist budget, placeholder released on failed save.Why
The 2026-07 audit found a live webhook token committed in source, a path traversal on the image delete route, an unbounded unauthenticated upload surface, silent dev-key fallbacks in production Flask, an uncaught-500 write path, and two unbounded in-memory maps. Each issue (#201–#207) carries the full background; this PR implements the track as planned, one reviewable commit per issue.
How tested
pio test -e native) — firmware untouched; pio unavailable in the CI container this was built in, runs in repo CIpytest tests/e2e) — docker unavailable in the build container; runs in repo CI (throttle default 30/h clears the seeders' 6-8 rapid uploads with headroom)tscbuild cleanmake check-citations: 7 OK, 0 problems. Two independentsenior-reviewergate runs (full branch + scoped re-review of fixes): mergeable, no P0/P1; all actionable P2s addressed in the two review commits.Checklist
check-no-hardcoded-api-keys.shextended and green)modul_idpayloads now 400 (no known emitter); dev-stack duckdb host port now loopback-only; uploads over 5 MB rejected; waitlist throttled. Operator prerequisites in the banner above.🤖 Generated with Claude Code
https://claude.ai/code/session_01GoGDGqYSuTjBCQqgtFDnFr
Generated by Claude Code