Document the Gitea Actions CI setup and the follow-up security review
Adds CONTINUOUS_INTEGRATION.md (mirrors HOP's CI doc structure), links it from README, and records the CI changelog entry plus the follow-up review's open findings (scheduler crash-loop, System.gc(), connection pooling, the PostgresService field-routing invariant, and the mislabeled main-method "tests") in the roadmap's Phase 2/Phase 5 checklists.
This commit is contained in:
@@ -57,9 +57,10 @@ Status: in progress
|
||||
- [ ] Add database integration tests for aggregation and export behavior.
|
||||
- [ ] Restore and expand CSV parser tests.
|
||||
- [ ] Add GRIB parser boundary and malformed-file tests.
|
||||
- [ ] Add security tests for invalid fields, filenames, offsets, lengths, and date ranges.
|
||||
- [ ] Add frontend type checking and critical workflow smoke tests.
|
||||
- [ ] Run tests automatically before staging deployment.
|
||||
- [ ] Add security tests for invalid fields, filenames, offsets, lengths, and date ranges. Highest-priority target per an independent review: `ValidateFileName`/`ValidateField`/`ValidateInt` are pure functions with zero I/O and are currently the entire path-traversal/SQL-injection security boundary, verified only by manual curl.
|
||||
- [ ] `PostgresService.query`'s "list" branch (`byField` match, ~13 hardcoded literal cases, no `case _ =>`) is only safe today because `ValidateField`'s allowlist and `WeatherData`'s case class fields happen to stay in sync with it — nothing enforces that. Add a field to `WeatherData` without updating this match and any request for it with `granularity=hour` throws an uncaught `MatchError` (a real risk given the DMI/open-data provider work in progress). A test iterating every `WeatherData.getKeys` value through this path would catch it before it ships; needs either a test-container Postgres or refactoring SQL-fragment-building apart from execution so it's testable without a DB.
|
||||
- [x] Add frontend type checking (`npm run typecheck`, part of the dependency-maintenance work in Phase 3) — now also runs automatically in CI (see below). Critical workflow smoke tests remain manual/headless-browser only, not automated.
|
||||
- [ ] Run tests automatically before staging deployment. Partial: Gitea Actions CI (see `CONTINUOUS_INTEGRATION.md`) now runs `sbt test` and the frontend typecheck/build/audit routine automatically on every push/PR to `codex/staging-baseline` — this is CI, not CD; it isn't yet wired as a required gate before Rocky/VPS deployment, which remains a manual decision independent of CI status.
|
||||
|
||||
## Phase 3 — Dependency modernization
|
||||
|
||||
@@ -103,8 +104,9 @@ Status: pending
|
||||
Status: pending
|
||||
|
||||
- [ ] Manage custom executors as resources and close them cleanly.
|
||||
- [ ] Remove explicit `System.gc()` calls.
|
||||
- [ ] Supervise scheduled jobs independently instead of recursively restarting the application.
|
||||
- [ ] Remove explicit `System.gc()` calls. Confirmed still present in `DataService.getBinaryChunk` (runs on every GRIB binary-chunk request — likely a latency source on what's probably a hot path; a commented-out `logMemory` block next to it suggests leftover debugging code, not an intentional design choice), per a 2026-08-24 independent review.
|
||||
- [ ] Supervise scheduled jobs independently instead of recursively restarting the application. Confirmed still fully live by a 2026-08-24 independent review: `Scheduler.scheduleTask` has zero `.attempt`/error handling on any task, none of the task bodies wired into it (FTP fetch, open-data fetch, cleanup, GRIB fetch) handle their own errors either, and `(serverTask, scheduledTasks).parMapN(...)` means any one failure cancels the HTTP server itself — this is the same defect that caused the actual VPS/Rocky crash-loop incidents earlier this session, patched around with feature flags rather than closed. `OpenDataStationService` also has no client timeout configured, unlike the DMI fetch service. `WarningService`/`WaterTemperatureService` already show the correct pattern (`.attempt` + stale-cache fallback) the scheduler tasks should follow.
|
||||
- [ ] Add a pooled JDBC transactor. `DBConnection.scala` uses `Transactor.fromDriverManager`, which opens a brand-new physical connection for every query — doobie's documented behavior for scripts/tests, not production services. Not urgent at current traffic, but an easy, well-known swap (`HikariTransactor`) worth doing before real load.
|
||||
- [ ] Add application health and readiness endpoints.
|
||||
- [ ] Add structured logging without leaking secrets.
|
||||
- [ ] Define PostgreSQL and GRIB-data backup/restore procedures.
|
||||
@@ -446,4 +448,6 @@ Record completed work here by date and commit after the Git workflow is establis
|
||||
| 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 | `0be325f` | Deploy the security-review fixes (`6b9c7cf`, `8c45d8d`) to the VPS as image `weathertool:0be325fbbbf92b03bc8d574dc6f7b9449ef310ea`; release `b0b58d2` remains the immediate application rollback. Application-only release — PostgreSQL and Authelia were not restarted | Yes — exact release image smoke-tested on Rocky against a throwaway local Postgres before transfer (traversal/injection payloads 404, gated FTP route 503, legitimate queries 200); checksum verified on both ends; `app` service recreated cleanly (healthy, no auth/DB errors in logs); loopback re-verification of the same traversal/injection/FTP-gate checks plus real `/api/warnings` and `/api/water-temperatures` responses; public HTTPS confirmed unchanged (302 unauthenticated page, 401 unauthenticated API) |
|
||||
| 2026-08-24 | `9bab93d` | A follow-up independent fresh-eyes review (requested specifically as a second pass, not a rerun of the same pentest) caught that the `6b9c7cf` FTP auth-bypass fix was incomplete: only `/api/fetch/lvgmc/stations` (the one route the original review named) was gated behind `ENABLE_LVGMC_FTP_JOBS`. A sibling route, `/api/show/lvgmc-forecast/{fileName}`, calls the same `fetch.fetchFile` — a real, unauthenticated LVGMC FTP login — and had only gotten the traversal fix (`ValidateFileName`), not the auth-bypass fix, because the two routes were fixed along different mental categories (traversal batch vs. the one named auth-bypass fix) instead of by tracing every caller of the dangerous method. Gated this route the same way; traced every HTTP-reachable caller of `fetch.fetchFile`/`fetchWeatherStations` this time to confirm no others remain. Deployed to both Rocky and the VPS (image `weathertool:9bab93daa6b3a0fb99165a8fac9accfa3a3ccbf1`) immediately given this was live and exploitable in production | Yes — Scala tests, Rocky rebuild and curl verification (both FTP routes 503 while the flag is off), VPS image smoke-tested against a throwaway local Postgres before transfer, checksum verified, `app` service recreated cleanly, loopback re-verification of both gated routes, public HTTPS unchanged (302/401) |
|
||||
| 2026-08-24 | *(review, no commit)* | A follow-up independent fresh-eyes review (explicitly scoped as a second, different pass — not a rerun of the original pentest) sanity-checked the `6b9c7cf`/`8c45d8d` fixes and did a broader skeptical pass. Caught the incomplete FTP fix documented under `9bab93d` above. Confirmed `ValidateFileName`'s minor `"."` edge case is harmless (resolves to a directory, not exploitable) and found no other unvalidated `Fragment.const` usages. Surfaced several open architectural/testability findings now tracked in Phase 2/Phase 5 above: the `parMapN` scheduler crash-loop pattern (still fully live), per-request `System.gc()` in `getBinaryChunk`, `PostgresService.query`'s unenforced field-routing invariant, no JDBC connection pooling, and several files under `src/main/scala` (`DataServiceTest`, `FetchServiceTest` ×2, `OpenDataStationServiceTest`, `GribParserTest`) that look like tests but are actually `main`-method scripts never run by `sbt test` — compiled straight into the production jar, at least one capable of writing to the real database or hitting live endpoints if run by mistake |
|
||||
| 2026-08-24 | `5b88e69` | Add Gitea Actions CI: a repository-scoped `weathertool-ci-rocky-01` runner on Rocky (isolated behind its own Docker-in-Docker daemon, mirroring the isolation pattern already proven by HOP's Forgejo runner on the same host — the two coexist without conflict, each an independent client registered to a different server) runs `sbt test` and the frontend typecheck/build/audit routine on every push/PR to `codex/staging-baseline` plus manual dispatch. Verified `sbt test` compiles and passes with no `.env` present at all (via a `git archive HEAD` dry run) before writing the workflow, since CI must never see real credentials. Repo mirrored to Gitea (`bot/WeatherTool`) as an additional remote alongside the existing `rocky`/`origin` ones — deliberately not replacing either, pending an open question about the GitHub `origin` (`guntisdev/WeatherTool`) that won't be resolved until 2026-08-26. See `CONTINUOUS_INTEGRATION.md` | Yes — first run (`ci.yml #1`, commit `5b88e69`) verified green in the Gitea Actions UI (33s), and confirmed via the runner's nested Docker daemon that both job images (`hseeberger/scala-sbt`, `node:20-bookworm`) were actually pulled and used, not just orchestration logs |
|
||||
| 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