From dca1ebb6fe58929ac5bcc631d6de93adca4884a6 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 10 Sep 2026 16:03:20 +0200 Subject: [PATCH] fix: admin routers authenticate themselves; track the audit in TODO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - adminMilestones was reached only because adminConfig is mounted on /api/admin ahead of it and guards the whole path. adminMiddleware checks req.user.role and nothing else, so it failed closed (403) rather than open — but on mount order, not intent. It now states the requirement, with a test covering all four admin routers. - TODO.md records the whole audit: what was verified working (live transcription round trip, voice mode wiring), what was fixed, the two advisories that are unreachable and why, and the CI/CD and Kubernetes work worth doing before scaling out. Security review found nothing else exploitable: parameterised SQL throughout (the one interpolated table name is allowlisted), CORS refuses to start open in production, JWT_SECRET refuses to start unset in production, rate limits on /api and each auth route, a real CSP, and no secrets in the repo. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU --- TODO.md | 97 ++++++++++++++++++++++++++++++---- src/routes/adminMilestones.js | 8 ++- test/backend-hardening.test.js | 16 ++++++ 3 files changed, 110 insertions(+), 11 deletions(-) diff --git a/TODO.md b/TODO.md index 79f57d03..ab1af3c8 100644 --- a/TODO.md +++ b/TODO.md @@ -1,14 +1,30 @@ # TODO -Live state as of 2026-09-10. Everything not listed here is deployed and green. +Live state as of 2026-09-11. Everything not listed under **Open** is deployed +and green (674 tests, three consecutive clean runs). ## Open -- [ ] **Basic index has no reader.** `MilvusVectorStore.search()` exists now, but no - tool calls it. Decide where the query path lives: put pymilvus into the - deliberately-lean `nextcloud-basic-mcp` server image, or expose a query API - from the indexer container. Nothing can read that index until this is done. +### Needs your decision +- [ ] **Replace Cloudflare Turnstile.** Used on registration and password reset + only (`src/routes/auth.js`); login is not gated, it relies on a + 10-per-15-min limit and a constant-time credential check. Recommended + replacement: **ALTCHA** — open source, self-hosted, proof-of-work, no + third-party calls and no tracking, which also lets three CSP entries and + `frameSrc` go away. Alternatives: **mCaptcha** (open source, self-hosted, + heavier to run) and **Cap** (newer, smaller). hCaptcha is neither Google + nor open source, so it trades one third party for another. +- [ ] **Kubernetes / CI-CD hardening.** Details under *Deployment readiness*. +- [ ] **Basic index has no reader.** `MilvusVectorStore.search()` exists, but no + tool calls it. Decide where the query path lives: pymilvus inside the + deliberately-lean `nextcloud-basic-mcp` image, or a query API from the + indexer container. Nothing can read that index until this is settled. Source: `/home/danvics/docker/nextcloud-basic-mcp` +- [ ] **Apply the restored clinical vector-store compose.** Written, committed + and validated at `/home/danvics/docker/nextcloud-mcp-server`, deliberately + NOT applied — `up -d` recreates the live clinical Milvus. + +### Known gaps - [ ] **Multi-collection, ped-ai half.** The MCP side is deployed (`clinical_semantic_search(collection=…)` + `clinical_list_collections`, allowlisted by `MILVUS_COLLECTIONS`). ped-ai still searches one collection @@ -16,15 +32,76 @@ Live state as of 2026-09-10. Everything not listed here is deployed and green. fan-out and merge — `dedupeSources` in `src/utils/clinicalRetrieval.js` already merges and renumbers. See `clinical-assist/COLLECTIONS.md`. - [ ] **Confirm mail indexing end to end.** Six separate breaks are fixed and the - scan now reaches mail, but the file pass has not finished a full cycle - since the Milvus rebuild, so no mail has been indexed yet. Watch for + scan reaches mail, but no full file pass has completed since the Milvus + rebuild, so no mail is indexed yet. Watch for `[SCAN-*] Mail messages: N seen, M queued`. -- [ ] **Apply the restored clinical vector-store compose.** Written, committed and - validated at `/home/danvics/docker/nextcloud-mcp-server`, deliberately NOT - applied — `up -d` recreates the live clinical Milvus. Owner's call when. +- [ ] **`image-size` DoS advisory (high), no upstream fix.** Every published + version up to 2.0.2 is affected; `npm audit fix` only offers a breaking + downgrade of pptxgenjs. Not reachable here: the only `addImage` call + (`src/routes/learningAI.js`) is fed PNGs the app generated itself and + fetched with an ownership check, never an uploaded file. Uploads are + allowlisted to jpeg/png/gif and checked against their magic bytes. Revisit + when pptxgenjs ships a patched dependency. +- [ ] **`uuid` advisory (moderate) via gaxios via Google auth.** Not reachable: + the flaw needs v3/v5/v6 with an explicit buffer, and gaxios calls only + `v4()` for a multipart boundary. Forcing an override risks Vertex auth. + +## Deployment readiness (CI/CD and Kubernetes) + +What already exists: `ci.yml` (tests on PR and main), `security.yml` (weekly +audit), `docker-publish.yml` and Android/versioning workflows, a Dockerfile +`HEALTHCHECK`, and `/api/health`. + +Worth doing before Kubernetes, roughly in order: +- [ ] **Fail CI on vulnerabilities.** `security.yml` runs weekly and does not + gate merges. `npm audit --audit-level=high` in `ci.yml` would have caught + the nodemailer advisories at the PR. +- [ ] **Build the image on PRs too.** `docker-publish.yml` only runs on tags, so + a Dockerfile break is found at release time. +- [ ] **Run the e2e suite in CI.** `scripts/e2e.sh` and + `docker-compose.e2e.yml` exist but nothing calls them. +- [ ] **Separate liveness from readiness.** `/api/health` is one endpoint; + Kubernetes wants liveness (process up) apart from readiness (database, + gateway and MinIO reachable), or rollouts take traffic too early. +- [ ] **Graceful shutdown.** No SIGTERM handler, so a rolling update can cut off + an in-flight transcription or image job. +- [ ] **Externalise state.** Uploads and audio backups assume local paths and a + single instance; more than one replica needs them all in MinIO/Postgres. +- [ ] **Config as secrets.** Everything is env vars in compose today, which maps + to ConfigMap/Secret cleanly, but `JWT_SECRET`, gateway keys and database + credentials should be a Secret from the start. +- [ ] **Pin the base image by digest** and keep the SBOM the build already has. ## Done since this file was written +### 2026-09-11 +- **Live transcription**: proved working end to end against the live gateway — + `local-kokoro-tts` produced 92KB of speech and + `mistral-voxtral-mini-transcribe` returned the sentence back verbatim. The + Settings picker offered six hardcoded ids that do not exist on this gateway + (`local-whisper-large-v3-turbo` → 400 Invalid model name); it now lists the + nine the gateway advertises, cached, with the admin default marked. +- **Assistant voice mode** is wired end to end: record → browser recognition, + falling back to server transcription → send → spoken answer. All four helpers + it needs exist. +- **Recordings can be exported** (server and local copies) and a recorder that + dies — an error, or the microphone taken by another app, unplugged or + revoked — now says so instead of appearing to record silence. No wake lock, + deliberately: stopping on sleep or sign-out is the behaviour you want. +- **nodemailer 9.0.1 → 9.1.1**, clearing four high advisories, two of them + delivery bugs that can route mail to an attacker-controlled domain. +- **Admin routers state their own authentication.** `adminMilestones` relied on + `adminConfig` being mounted first on `/api/admin`; it failed closed, but on + mount order rather than intent. +- **Test suite made deterministic.** A file failed about one run in four with + "Unable to deserialize cloned data": node:test parses each child's stdout, and + page/server logging was landing inside those frames. Every test child's stdout + is now pure TAP. +- **iOS**: text fields are 16px on phones, so Safari no longer zooms the page on + focus — which was also why fixed chrome (the menu button) scrolled away. +- **Settings claim corrected**: there is no "use my normal physical exam" + trigger, and the prompt forbids copying template content. + - Storage stack is under version control (`/home/danvics/docker/ped-ai-storage`), secrets verified excluded, with a README recording the misleading project names and the MinIO/separate-etcd requirements. diff --git a/src/routes/adminMilestones.js b/src/routes/adminMilestones.js index 7784521c..b12f8476 100644 --- a/src/routes/adminMilestones.js +++ b/src/routes/adminMilestones.js @@ -1,8 +1,14 @@ const express = require('express'); const router = express.Router(); -const { adminMiddleware } = require('../middleware/auth'); +const { authMiddleware, adminMiddleware } = require('../middleware/auth'); const db = require('../db/database'); +// adminMiddleware only checks req.user.role — it does not authenticate. These +// routes were reached only because adminConfig is mounted on /api/admin ahead +// of them and runs authMiddleware for the whole path. That is mount order, not +// a guarantee: state the requirement here so it cannot be reordered away. +router.use(authMiddleware); + // Get all milestones (optionally filtered by age group) router.get('/milestones', adminMiddleware, async (req, res) => { try { diff --git a/test/backend-hardening.test.js b/test/backend-hardening.test.js index e1d54c80..2d084b62 100644 --- a/test/backend-hardening.test.js +++ b/test/backend-hardening.test.js @@ -65,3 +65,19 @@ test('logs and audits avoid unbounded limits and PHI-prone details', () => { assert.doesNotMatch(documents, /Uploaded: ' \+ \(req\.file/); assert.doesNotMatch(learningAI, /Char context/); }); + +// adminMiddleware only checks req.user.role; without authMiddleware having run, +// req.user is undefined. These routes were protected only because adminConfig +// happens to be mounted on /api/admin first and guards the whole path. +test('admin routers state their own authentication, not mount order', () => { + const fs = require('node:fs'); + const path = require('node:path'); + const root = path.join(__dirname, '..'); + for (const file of ['adminMilestones.js', 'admin.js', 'adminConfig.js', 'adminDocs.js']) { + const src = fs.readFileSync(path.join(root, 'src/routes', file), 'utf8'); + assert.match(src, /router\.use\(authMiddleware\);/, file + ' authenticates every route it serves'); + } + const auth = fs.readFileSync(path.join(root, 'src/middleware/auth.js'), 'utf8'); + assert.match(auth, /async function adminMiddleware\(req, res, next\) \{\s*\n\s*if \(!req\.user \|\| req\.user\.role !== 'admin'\)/, + 'and the role check stays a role check, so it fails closed on its own'); +});