Document the security review fixes and the Rocky Postgres password rotation
Records the path-traversal/auth-bypass/DoS fixes (6b9c7cf), the SQL injection fix (8c45d8d), and the subsequent Postgres password rotation on Rocky in the roadmap changelog and Phase 4 checklist, and notes current status in README/DEVELOPMENT_AND_STAGING. VPS deployment of these fixes is still pending.
This commit is contained in:
@@ -118,6 +118,8 @@ At commit `de6f279`, all five checks pass and both audit scopes report zero know
|
||||
|
||||
Fixed 2026-08-24 (`2c44888`): direct browser refreshes on any client-side route now correctly serve the app via a general SPA fallback, replacing the old hardcoded per-route static-file list.
|
||||
|
||||
Fixed 2026-08-24 (`6b9c7cf`, `8c45d8d`): a fresh-eyes security review found a live-verified CRITICAL path-traversal vulnerability and several HIGH/MEDIUM findings (unauthenticated FTP-trigger route, SQL injection via an unvalidated `field` path segment, unbounded-allocation DoS, `/api/*` falling through to the SPA shell) — all fixed and live-verified on Rocky; see the roadmap changelog for details. Rocky's `POSTGRES_PASSWORD` was rotated afterward via `ALTER ROLE` against the running container, since the old value had been exposed into the review's output.
|
||||
|
||||
## Authenticated API requests
|
||||
|
||||
The public Nginx configuration deliberately handles `/api/` separately from
|
||||
|
||||
@@ -16,6 +16,7 @@ This directory contains the working documentation for the WeatherTool modernizat
|
||||
- **Brīdinājumi** renders current LVĢMC warning polygons over a production border overlay with feathered severity fills, plus draggable/resizable per-warning weather-symbol placement. Its lon/lat-to-pixel projection is an affine fit calibrated against the same validated city pixel positions Kartes/Faktiskā already use, replacing an earlier bounding-box calibration that drifted up to ~200px on the 3840 canvas.
|
||||
- Confirmed local Monda Regular/Bold files provide interface and generated-graphic typography; weather symbols use normalized transparent image assets.
|
||||
- Release `b0b58d2` is deployed as immutable image `weathertool:b0b58d2f1e0ed47ca13795b64595386ec2f0e0c7`; release `3eddf95` remains the immediate application rollback. On top of the earlier frontend design pass and the timezone/upsert/scheduler-split fixes, this release replaces the hardcoded per-route static-file list with a general SPA fallback — a real, user-reported bug where refreshing `/faktiska` or `/udens-temperatura` 404ed instead of loading the app (those two routes were never added to the old list). A missing `/assets` file still 404s properly rather than silently serving HTML.
|
||||
- An independent fresh-eyes security review (requested against the local Rocky version only) found a live-verified CRITICAL path-traversal vulnerability plus several HIGH/MEDIUM findings — see the roadmap changelog for `6b9c7cf`/`8c45d8d` for the full list. All are fixed, compiled, and live-verified on Rocky; Rocky's `POSTGRES_PASSWORD` was rotated afterward since the old value had been exposed into the review's own output via `/proc/self/environ`. VPS deployment of these fixes is pending. `LVGMC_PASSWORD` rotation is not yet done — that needs the user's own action via the LVGMC contact.
|
||||
- FTP (`ENABLE_LVGMC_FTP_JOBS`) is currently `false` on both Rocky and the VPS — re-testing the morning after enabling it turned up a second, unexplained failure (Rocky alone, VPS confirmed off, ~4 minutes after a successful manual test) that doesn't fit the original two-machine-collision theory. Decided to stop self-testing via trial and error and wait for the user to ask the LVGMC contact directly about the `ltv` account's connection/rate policy, rather than risk repeatedly tripping an unknown limit. See the roadmap Phase 7 for the full diagnosis timeline.
|
||||
- The isolated VPS UAT stack is running and healthy: WeatherTool is bound to `127.0.0.1:8002`, Authelia to `127.0.0.1:9091`, and PostgreSQL has no host port. Public access is routed through Cloudflare, Nginx, and Authelia.
|
||||
- Cloudflare delegation is active, strict origin TLS covers only `laikapstak.li` and `auth.laikapstak.li`, and the public Nginx/Authelia login flow is operational without changing the existing HOP site.
|
||||
|
||||
@@ -88,12 +88,12 @@ Dependency changes must not be combined with a visual redesign unless a package
|
||||
Status: pending
|
||||
|
||||
- [ ] Rotate and remove the API key exposed in a source comment (`src/main/scala/fetch/dmi/FetchService.scala:56`, a DMI HARMONIE key introduced 2025-02-01; confirmed the repo's GitHub `origin` is public, so this has been externally exposed for roughly 18 months — decision on rotation vs. a git-history rewrite is still open).
|
||||
- [ ] Remove credentials from connection-error messages.
|
||||
- [ ] Protect or remove debug and administrative endpoints.
|
||||
- [x] Remove credentials/internal error details from connection-error messages: `/api/fetch/lvgmc/stations` was returning `error.getMessage` straight to the caller; now logs it server-side and returns a generic message.
|
||||
- [ ] Protect or remove debug and administrative endpoints. Partial: `/api/fetch/lvgmc/stations` (the one that could trigger a real FTP login) is now gated behind `ENABLE_LVGMC_FTP_JOBS`, but `/api/debug/time`, `/api/debug/folder-structure`, and `/api/debug/delete-tmp` remain unauthenticated.
|
||||
- [ ] Convert state-changing `GET` routes to appropriate methods.
|
||||
- [ ] Introduce closed, validated weather-field and aggregation types.
|
||||
- [ ] Eliminate raw user-controlled SQL identifiers.
|
||||
- [ ] Validate and constrain filenames, resolved paths, offsets, and byte lengths.
|
||||
- [ ] Introduce closed, validated weather-field and aggregation types. Partial: the one route that let a raw field string reach SQL now validates against `WeatherData.getKeys` (see below), but this is a route-level allowlist, not a closed type threaded through the codebase.
|
||||
- [x] Eliminate raw user-controlled SQL identifiers: `/api/query/city/.../{field}/...` let an arbitrary path segment reach `Fragment.const` unescaped; added `ValidateField`, the same allowlist-against-`WeatherData.getKeys` pattern `/query/country` already used.
|
||||
- [x] Validate and constrain filenames, resolved paths, offsets, and byte lengths: added `ValidateFileName` (allowlist regex, rejects `..` and separators including decoded `%2f`) and applied it to every route that concatenated a raw path segment into a filesystem or FTP path (`/debug/file`, `/show/grib`, `/grib/binary-chunk`, `/show/lvgmc-forecast`); hardened `ValidateInt` to reject negative integers; capped `binary-chunk` read length at 64MB to remove an unbounded allocation.
|
||||
- [ ] Add query-range, response-size, request-rate, and timeout limits.
|
||||
- [ ] Restrict CORS to intended origins.
|
||||
- [ ] Define authentication and authorization requirements for workplace deployment.
|
||||
@@ -441,4 +441,7 @@ Record completed work here by date and commit after the Git workflow is establis
|
||||
| 2026-08-23 | `3315f00` | Fix the same UTC-vs-local mismatch in Ūdens's `observedAt` display (`WaterTemperatureService`) — same open-data portal, same root cause, display-only (internal recency filtering was already self-consistent either way) | Yes — Scala tests; verified live via `/api/water-temperatures`: `observedAt` now matches real local time |
|
||||
| 2026-08-23 | `3eddf95` | Split `ENABLE_LEGACY_PROVIDER_JOBS` into independent `ENABLE_LVGMC_FTP_JOBS`/`ENABLE_HARMONIE_JOBS` — real LVGMC FTP credentials arrived today, real DMI HARMONIE credentials haven't, and the combined flag would have enabled both together, crash-looping the app on HARMONIE's still-placeholder values via `parMapN` | Yes — Scala tests; caught before it could happen (the Grib job was ~15 min from its first scheduled run when noticed) and reverted within under a minute; re-verified after the fix that only "Fetch Weather Stations" scheduled, not "Fetch Grib" |
|
||||
| 2026-08-23 | `dc04f66` | Fix `deploy/vps/compose.yml` hardcoding `LVGMC_USER`/`PASSWORD`/`URL` to inert placeholder strings directly in the file (unlike `POSTGRES_*`, which already read from `.env.staging`) — real credentials added to `.env.staging` alone would have had no effect until this switched to the same `${VAR}` substitution pattern. Enabled `ENABLE_LVGMC_FTP_JOBS` on the VPS | Yes — `docker compose config` syntax valid; VPS `app` container came up healthy with real credentials loaded (would have crashed immediately on missing-config if not) |
|
||||
| 2026-08-24 | `6b9c7cf` | Fix path traversal, auth-bypass, and DoS findings from an independent fresh-eyes security review (requested by the user specifically to get a second, skeptical pass against the local Rocky version): a live-verified CRITICAL — `/api/debug/file/{fileName}` allowed unauthenticated arbitrary file read, confirmed by reading `/proc/self/environ` and leaking `LVGMC_PASSWORD`/`POSTGRES_PASSWORD` into the review's own output — plus the same raw-filename pattern in `/show/grib`, `/grib/binary-chunk`, and `/show/lvgmc-forecast`. Added `ValidateFileName` (allowlist regex; rejects `..` and separators including decoded `%2f`) and applied it everywhere a path segment reached a filesystem or FTP path; hardened `ValidateInt` to reject negatives; gated `/api/fetch/lvgmc/stations` behind `ENABLE_LVGMC_FTP_JOBS` (it could otherwise trigger a real, unauthenticated FTP login regardless of the flag) and stopped it leaking `error.getMessage`; added an explicit `/api/*` catch-all so an unmatched API route can never fall through to the SPA shell; capped `binary-chunk` read length at 64MB | Yes — Scala tests, Rocky rebuild, live curl verification of `%2f`-encoded and literal traversal payloads (404), the gated FTP route (503 while the flag is off), an unmatched `/api/*` route (404, not `index.html`), and legitimate filenames/queries still returning correct 200s |
|
||||
| 2026-08-24 | `8c45d8d` | Fix SQL injection surfaced by the same security review: `/api/query/city/.../{field}/...` passed its `field` path segment unvalidated into `PostgresService.query`, which splices it into SQL via `Fragment.const` (unescaped) whenever the aggregate key is min/max/avg/sum/distinct, or whenever granularity is `hour` in the list branch. Added `ValidateField`, reusing the `WeatherData.getKeys` allowlist `AggFieldList` already applied to `/query/country`. `AggregateKey` values reaching `Fragment.const` elsewhere were already safe — they come from a closed ADT (`AggregateKey.fromString`), not raw input | Yes — Scala tests, Rocky rebuild, live curl verification that SQL-injection payloads in the `field` segment 404 and a legitimate query still returns correct data |
|
||||
| 2026-08-24 | *(operational, no commit)* | Rotated Rocky's `POSTGRES_PASSWORD` following the security review, since the old value had been exposed into this session's context multiple times (two of my own sloppy shell commands, plus the `/proc/self/environ` read the review used as its traversal proof). Changed the role's actual password via `ALTER ROLE` against the running container (editing `.env` alone has no effect on an already-initialized PostgreSQL data directory), then updated `.env` and recreated the `scala` service. The Postgres container itself was also recreated as a side effect (its own `POSTGRES_PASSWORD` env interpolation changed too), which is harmless — that variable only takes effect on a fresh, empty data directory, not an existing one — but is worth knowing about | Yes — clean scala container startup with no auth errors, and a real query (`/api/query/latest-temperatures/Rīga`) returning live data over the new password |
|
||||
| 2026-08-24 | `2c44888`–`b0b58d2` | Replace the hardcoded per-route static-file list (`/station`, `/cities`, `/latvia`, `/database`, `/harmonie`, `/lvgmc-forecast`, `/bridinajumi`) with a general SPA fallback: real files serve as-is, anything else falls back to `index.html` so the SolidJS router handles it client-side — fixes a real, user-reported bug where a direct hit (e.g. a browser refresh) on `/faktiska` or `/udens-temperatura` 404ed instead of loading the app, since those two routes were never added to the old list. A missing file under `/assets` specifically still 404s properly rather than silently serving HTML, so a stale tab after a future deploy gets a clean error instead of a confusing JS parse failure | Yes — first attempt silently no-op'd because the fix was built into a `git archive HEAD` image before being committed (classic mistake, caught immediately by re-testing and finding identical old behavior); after committing, verified via curl on all previously-working routes, both previously-broken routes, a missing asset (404), a real asset (200), and `/api` (200), then a full headless-browser render check (zero console errors, real data, correct nav state) on a genuine direct hit — not just HTTP status codes; deployed to VPS in release `b0b58d2`, verified the same way through both the loopback port and the public domain (Authelia gate still correctly redirects unauthenticated requests) |
|
||||
|
||||
Reference in New Issue
Block a user