# 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:**

```bash
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.