fix: admin routers authenticate themselves; track the audit in TODO
- 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU
This commit is contained in:
parent
48a3b06ebb
commit
dca1ebb6fe
3 changed files with 110 additions and 11 deletions
97
TODO.md
97
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.
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue