Repository navigation
Harden weather.cgi against hostile query strings - #7
Merged
Merged
Conversation
weather.cgi is public and unauthenticated, so treat every parameter as
hostile and stop one request from being able to cost the server much.
Validate input, and distinguish client errors from upstream ones:
- lat/lon rejected unless finite and in range. float() accepts "nan",
"inf" and "1e999", all of which went upstream as latitude=nan and came
back as an error we reported as our own 502.
- name stripped to place-name characters and capped. It lands in
payload["location"], which the templates render as raw markup --
Liquid's {{ }} does not escape, and the templates depend on that to
inline the SVGs, so `?name=<img src=x onerror=...>` was live markup in
TRMNL's renderer. The templates now also pipe it through `escape`.
- tz must look like an IANA name; q is length-capped.
- Bad input is now 400, not 502, so a malformed request is no longer
indistinguishable from an Open-Meteo outage in the logs.
- Errors return a fixed string. str(e) could carry the upstream URL or
local filesystem paths; the detail goes to the Apache error log.
Bound the cost of a request:
- Responses cached on disk for 5 minutes, keyed on the raw query before
any upstream call -- keying on the resolved location would still pay a
geocode per request, which is the expensive half.
- At most 4 concurrent upstream fetches (flock on slot files). Without a
cap, N concurrent requests pin N CGI workers for a whole round-trip.
Over the cap, serve a stale entry or a fast 503 + Retry-After.
- Upstream timeout 5s, not 15s. Geocode and forecast are sequential, so
worst case per request goes 30s -> 10s.
- A stale entry is preferred over a 502 when upstream is down.
Also add X-Content-Type-Options: nosniff, and document denying .git/ and
sources in the docroot -- the README's config served the whole checkout.
Co-Authored-By: Claude Opus 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.
Security review of the CGI request path (
weather.cgi→trmnl_report.py→weather_circle.py) and the templates that consume its output.What was already fine
Worth stating, because the dangerous things aren't here: no shell-out in the request path (
subprocessis CLI-only), no path traversal, no header injection, and no URL/parameter injection or SSRF — every attacker value reaching an upstream URL goes throughurlencode(verified:tz=a b&c=d→timezone=a+b%26c%3Dd).daysis a strict whitelist andresolve_slotsfails closed on every garbage input tried, including a 4300-digit integer.Fixed here
Input validation.
lat/lonnow must be finite and in range —float()acceptsnan,infand1e999, all of which went upstream aslatitude=nanand came back as an error we reported as our own 502.tzmust look like an IANA name,qis length-capped.HTML injection via
name.namelands inpayload["location"], rendered by the templates as raw markup — Liquid's{{ }}doesn't escape, and the templates rely on that to inline the SVGs.?name=<img src=x onerror=...>was live markup in TRMNL's server-side renderer.nameis now stripped to place-name characters and capped at 64;{{ location }}also gained| escapeas defence in depth. The local preview's mini-Liquid renderer learned theescapefilter so it doesn't diverge from TRMNL.Error handling. Bad input returns 400 instead of 502, so a malformed request is no longer indistinguishable from an Open-Meteo outage in the logs. Error bodies are fixed strings —
str(e)could carry the upstream URL or local filesystem paths — with the detail going to the Apache error log.Request cost. Each miss was two sequential 15s upstream calls, so one request could pin an Apache CGI worker for 30 seconds; a few hundred concurrent requests exhausted the pool. Now: responses cached on disk for 5 minutes (keyed on the raw query before any upstream call, so repeats don't pay a geocode), at most 4 concurrent upstream fetches via
flockon slot files, 5s timeouts, and a stale cache entry preferred over a 502 when upstream is down.X-Cachereportshit/miss/stale.Caching doesn't stop a determined flood — varying
?q=misses every time — so the README now also recommends a real rate limit in front. Open-Meteo's free tier is per-IP, so an unthrottled endpoint gets your server blocked.Deployment. The README's Apache config served the whole checkout, including
.git/. Now denies.git/and source files, and notes that the docroot must not be writable by the Apache user (weather.cgiputs its own directory onsys.path).Testing
Manually exercised against the live endpoint:
lat=nan,lat=1e999,lat=abc,lat=91,latwithoutlon, CRLF intz, over-longq→ all 400 with no echo of input?name=<img src=x onerror=alert(1)>London→ stored asimg srcx onerroralert(1)London, no markup characters survivelat/lon,q=Paris,rolling=1) return well-formed payloads with the expected cell countsX-Cache: miss, secondX-Cache: hitRetry-After; saturated with a backdated entry →X-Cache: staletrmnl_report.py --jsonand the HTML preview still render; preview escapeslocationwhile keeping SVGs rawtrmnlp lintisn't installed locally, so CI is the check on the| escapetemplate edit.🤖 Generated with Claude Code