security: harden /upload, /new_module, /heartbeat against anonymous abuse (closes #228, closes #229) - #269
Merged
Conversation
…buse (closes #228, closes #229) Both trust boundaries flagged by the 2026-08-18 audit were internet-reachable and credential-free with no bounds beyond "accepts the request": - image-service /upload never inspected upload bytes, so a stored .html/.svg could be served back same-origin with a sniffed Content-Type, and a crafted JPEG could decompression-bomb the single-process service. Fixed by services/image_guard.py::probe_jpeg (header-only JPEG validation, no pixel decode) running before anything is saved, plus a forced .jpg stored extension and a pinned image/jpeg Content-Type on the serve routes. - duckdb-service /new_module let any anonymous re-POST overwrite a placed module's name/email/location (public-map-defacement) and spammed Discord on every call; /heartbeat let an unregistered MAC grow module_heartbeats without bound. Fixed by preserving identity fields unless unset, dropping heartbeats for unknown MACs, clamping battery, and removing two raw-body/ precise-coordinate log lines that leaked into the admin-readable log ring. Also ships (deliberately not code): ADR-032 records the device-identity decision (interim fleet key) for a follow-up issue, and deploy/nginx/ highfive-ingest.conf is a syntax-verified (nginx -t) rate-limit template for an operator to apply to the live host. Went through three independent senior-review rounds. Round 1 caught a P0: the first-pass lat/lng UPSERT guard inverted its own gating condition and still let a re-POST relocate a placed module (verified end-to-end: one POST moved a module from Germany to Sydney) — fixed and re-verified across 11 scenarios in round 2. Round 3 was documentation-drift sweeps. See docs/11-risks-and- technical-debt/README.md for both incidents' write-ups. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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
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.
Summary
Both internet-reachable, credential-free trust boundaries flagged by the 2026-08-18 repo audit (epic #262) had no bounds beyond "accepts the request":
/upload(addresses security: validate upload content — JPEG magic + dimension guard + forced .jpg + explicit image/jpeg on serve (stored-HTML, decompression bomb, sidecar leak) #228). Never inspected upload bytes — a stored.html/.svgcould be served back same-origin with a sniffed Content-Type (same origin as the admin session cookie), and a crafted JPEG could decompression-bomb the single-process service before any decoder ran. Fixed byservices/image_guard.py::probe_jpeg— a header-only JPEG magic-byte + dimension check, never a pixel decode — running before anything is saved or handed tocv2.imread. The stored extension is now always forced to.jpg, andGET /images/GET /snips(image-service + backend proxies) pinContent-Type: image/jpeginstead of guessing from the extension, which also closes an unrelated.log.jsontelemetry-sidecar-reachability gap./new_module+/heartbeat(addresses security: harden the internet-proxied /new_module and /heartbeat writes (no-overwrite on re-registration, unknown-MAC drop, clamps, nginx limits, no raw-body logging) + device-identity ADR #229). Any anonymous re-POST to/new_modulecould overwrite a placed module'sname/email/location (public-map defacement) and spammed the Discord webhook on every call;/heartbeatlet an unregistered MAC growmodule_heartbeatswithout bound. Fixed by preserving identity fields on re-registration unless unset, firing Discord only on first insert, dropping heartbeats for unknown MACs (still200, nothing written), clampingbatteryinstead of storing it unbounded, and removing two log lines that leaked the raw pre-validation body (including operator email) and sub-2dp-precision coordinates into the admin-readable, disk-persisted log ring.Also ships, deliberately as documentation/template rather than code: ADR-032 records the device-identity decision (an interim compiled-in fleet key) for a separate follow-up issue, and
deploy/nginx/highfive-ingest.confis a syntax-verified (nginx -tvia Docker) rate/body-size-limit template for an operator to apply to the live host — not yet deployed there.Review history
This branch went through three independent rounds of the mandated
senior-reviewergate:lat/lngUPSERT guard inadd_moduleinverted its own gating condition and still let an anonymous re-POST relocate a placed module — verified end-to-end (one POST moved a module from Germany to Sydney).OPENCV_IO_MAX_IMAGE_PIXELSwas hardcoded separately fromMAX_IMAGE_DIM, so the troubleshooting doc's own advice to raise the latter would have silently broken hole detection. Both fixed and verified (the OpenCV fix via a subprocess test, since the protection is native-library state that a same-process module reload can't exercise).grepsweep (not justdocs/) to confirm no stale copies remained.Full write-ups of both incidents (the CASE inversion and the drift pattern) are in
docs/11-risks-and-technical-debt/README.md.Test plan
All run locally, all green:
cd image-service; py -m pytest tests/ -q— 181 passedcd duckdb-service; py -m pytest tests/ -q— 304 passedcd backend; npm test— 296 passedpy -m pytest tests/e2e/ -v(realdocker composestack) — 6/6 passeddocker compose -f tests/ui/docker-compose.ui.yml ... ; npx playwright test) — 16/16 passed (1 pre-existing, unrelated skip)make check-citationsand all other pre-push guard scripts — cleannginx -tagainst the mergeddeploy/nginx/highfive-ingest.conf+ site config (Docker,nginx:alpine) — syntax valid🤖 Generated with Claude Code