3523dcf93e
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.
Now `grep -H '^Status:' plans/*.md` is the answer:
50 done 3 in progress 2 planned 2 reference 1 parked
Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.
The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.
test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.
Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
186 lines
11 KiB
Markdown
186 lines
11 KiB
Markdown
# 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.
|