6krrt/plans/router-admin-portal-implementation-review.md

Review: router-admin-portal implementation

Status: done -- review of shipped work

What it was reviewing: the shipped admin portal — admin.py, admin_schema.sql, admin/frontend/index.html, the dispatcher.py mount point, and 9 new test files — against the two documents that set its bar: plans/admin-portal-gap-analysis.md (15 pre-implementation findings) and plans/router-admin-portal-plan-review.md (the plan review's 4-item "don't approve as-is" list). All four of that list's items are checked below against the actual code, not the plan's description of it.

Verdict: the plan review's 4 required items are all genuinely fixed; one new stored-XSS gap in the dashboard itself was missed by both automated tests and the reported manual QA pass

The 4 required fixes — all confirmed in the code, not just claimed

1. admin_model_overrides reaches routing.select_candidates — yes. dispatcher.py:1857-1877 (_admin_deprecated_models) reads the table and is unioned into exclude_models at dispatcher.py:774 (exclude_models=_open_circuits(rows, cfg) | admin_deprecated), which feeds select_candidates at :783. This sits inside route() (:736), the single chokepoint every entry point calls (route_endpoint:1637, dispatch_endpoint:2873, and all three chat_completions call sites at :2304/:2325/:2345), so an override reaches every dispatch path, not just /route. tests/test_admin_routing_override.py is a real integration test, not a unit test of the API alone: it seeds two models, confirms the better one wins, marks it deprecated through the admin endpoint, and asserts /route now picks the other one — exactly the test the plan review asked for.
2. Auth/CSRF given an explicit decision, and the restart response-before-death mechanism specified — half done. The restart mechanism is fully addressed: admin_restart_service (admin.py:684-693) schedules _restart_service via BackgroundTasks and returns {"status": "restarting"} immediately, so the response flushes before systemctl sends the process a signal — exactly what was asked for. But the auth/CSRF half is not: README.md's new section says only "It has no auth layer yet, so like the other endpoints it is reachable only from 127.0.0.1" — the same read-only-surface answer the plan review said was being carried forward unexamined, restated rather than re-examined. There is no CSRF token, no Origin/Referer check, and no code comment anywhere in admin.py weighing that decision for a surface that now includes POST /api/restart-service, POST /api/config/{key}, and the three subprocess triggers. See the new finding below — it turns out this gap is worse than CSRF alone, because the dashboard has a stored-XSS hole that defeats even an Origin check.
3. History is on-demand GROUP BY, not a sampled second database — yes. _history_series (admin.py:85-112) is exactly a bucketed GROUP BY over route_decisions / energy_observations, no background sampling loop, no second SQLite file, no lifecycle question. Matches the review's recommendation exactly.
4. ruamel.yaml pinned in requirements.txt — yes, ruamel.yaml==0.18.10 with a comment explaining why pyyaml can't do the job (requirements.txt diff). admin_config_write (admin.py:745-778) round-trips through YAML(), validates the whole resulting config via RouterConfig(**store) before writing, and takes a timestamped backup first — comment preservation confirmed by load_config_store using YAML() with default (round-trip) mode rather than safe_load.

Also confirmed from the gap analysis's other items, cheaply: no admin.py → dispatcher import (admin.py imports only metrics and config, matching its own docstring's contract at the top of the file); subprocess triggers use sys.executable and asyncio.create_subprocess_exec under a wall-clock deadline (admin.py:604-656), not a blocking subprocess.run on the event loop; and pinch.relevance.enabled is exposed as its own knob distinct from pinch.enabled (_BOOL_KNOBS, admin.py:271-279).

New finding: stored/live XSS via task_category, reachable from the cheapest endpoint in the service, lands in the one surface with write privileges

TaskRequest.task_category (dispatcher.py:214) is an unvalidated Optional[str] — no enum, no length cap, no character filter. Supplying it together with task_tier and required_context_tokens skips the classifier entirely (dispatcher.py:739-746) and its raw value becomes classification.task_category, which persist_route_decision writes verbatim into route_decisions.task_category and fans out live to every SSE subscriber via events.publish_decision (dispatcher.py:1041). /route itself calls this on every request (dispatcher.py:1644) — it is documented elsewhere in this repo as the free, quota-safe probe endpoint, which makes it the lowest-friction way to get a string stored and pushed live, not an edge case.

The admin dashboard renders that field back into the DOM in two places without escaping it:

• renderDecisions (admin/frontend/index.html:490): <td title="${d.task_category || ''}">${d.task_category || '—'}</td>
• addDecision, the live SSE row-append path (index.html:403): identical unescaped interpolation.

Both are tbody.innerHTML = ... / insertBefore on a freshly-built <tr>, so this is a direct HTML injection, not merely an attribute-quoting issue. The same field, from the same data source, is escaped one panel over in renderCategoryBreakdown (index.html:577, escapeHtml(cat)) — confirming this is a missed spot rather than a considered choice, since escapeHtml already exists in the file (index.html:884-886) and is used correctly in renderModels, renderConfig, and renderWarnings.

Concrete reproduction:

curl -s localhost:8080/route -H 'content-type: application/json' -d '{
  "task": "x", "task_tier": 1, "required_context_tokens": 0,
  "task_category": "<img src=x onerror=fetch(\"/admin/api/restart-service\",{method:\"POST\"})>"
}'

The next time an operator has /admin/ open — including passively, since the live decisions table updates via SSE without a page reload — the onerror fires in the page that already holds every admin capability: restart-service, allowlisted config.yaml writes, and model-availability overrides, all same-origin fetch() calls away with no additional credential needed. This is why the auth/CSRF gap in item 2 above is worse than it looks in isolation: an Origin check on the write endpoints wouldn't stop this, because the malicious request originates from JavaScript already running on localhost:8080 itself.

No test in the 9 new test files touches HTML escaping or the dashboard's JS at all (tests/test_admin_frontend.py only checks the file is served and contains the Chart.js tag) — consistent with the orchestration report's "F3 Real manual QA: APPROVE" not having exercised this path.

Fix: run task_category (and rejected_reason, runner_up_models, and any other free-text decision field) through escapeHtml in renderDecisions and addDecision, matching the pattern already used three other places in the same file. Validating task_category server-side against classifier.py's known category set would also close it and is probably worth doing anyway — an unvalidated free-form override string being usable to skip the classifier is a second, smaller thing worth a look, independent of the rendering fix.

Second new finding, caught live in production rather than in review: concurrent config writes actually crash, and can transiently truncate config.yaml

The gap analysis's F10 ("concurrent config.yaml edits are a race condition") was accepted as deferred/"Recommended" on the theory that it's a rare, low-stakes race. It isn't rare: saveAllConfig() (admin/frontend/index.html:778-803) fires every allowlisted key as a parallel Promise.all of POST /admin/api/config/{key} calls, so the one-click "Save All Config" button — the documented way to use this feature — guarantees concurrent writes on every use, not just under load.

Caught live on this host at 02:35:34-36 on 2026-08-29: two of the ten parallel writes came back 500 Internal Server Error (pinch.enabled, routing.default_flex_preference), both TypeError: 'NoneType' object is not subscriptable at admin.py:332 (_dict_set_at) — meaning load_config_store (admin.py:336-340) returned None because it read config.yaml while a concurrent request's config_path.open("w") (admin.py:772) had already truncated the file to zero bytes and hadn't finished writing yet. Direct evidence this isn't theoretical: one of the timestamped backups made during that window, config.yaml.bak.1787985334, is itself 0 bytes — shutil.copyfile (admin.py:771) copied config.yaml mid-truncation, so the safety backup this feature exists to provide was itself corrupted for that request. config.yaml itself survived intact this time (the last writer to finish happened to write a complete, valid file), but nothing in the current code guarantees that outcome — the write is an in-place open("w") truncate, not a write-to-temp-file-then-rename, so a crash or a slower writer mid-race could leave config.yaml empty on disk with no valid backup to recover from.

Fix: two independent things, either one closes most of the risk: (1) make the write atomic — write to config.yaml.tmp and os.replace() over the real path, so a reader/writer never observes a truncated file; (2) serialize config writes with a lock (a module-level threading.Lock in admin.py is enough for a single-process service) so overlapping requests queue instead of interleaving. The frontend's saveAllConfig() sending requests sequentially instead of via Promise.all would also avoid triggering this in the one place it's currently guaranteed to happen, but the server-side fix is the one that actually closes the bug — a future curl script or a second browser tab hitting /admin/api/config/* concurrently would reproduce it regardless of what the shipped frontend does.

Recommendation

Ship the fix for the XSS finding before this goes live somewhere reachable by more than one trusted user — it's a small, mechanical change (two escapeHtml calls) with an exact reproduction above to verify against. Everything else checked against the plan review's required list is genuinely done, not just documented as done: the F1 routing-integration test in particular is exactly the test that review asked for, not a weaker substitute. The auth/CSRF decision is still owed a real paragraph (even one sentence: "accepted, personal single-operator tool, revisit if this becomes multi-user") rather than the pre-existing read-only-surface sentence carried forward unchanged — worth doing at the same time as the XSS fix since they're the same conversation.