feat(sleep): local control-panel dashboard - #152
Conversation
Yif-Yang
left a comment
There was a problem hiding this comment.
Thanks for building this local dashboard—the observability and prompt-steering direction is useful. We reviewed the current head (fd6dc429).
Since #151 has now merged, please first rebase onto current main and drop its duplicate evidence-chain commit, leaving only the dashboard changes here.
One security issue blocks merging: binding to 127.0.0.1 does not by itself protect these privileged POST endpoints from browser-origin attacks. They can rewrite config/prompts, launch a real run, or adopt artifacts. For example, an empty cross-origin form POST to /api/run currently parses as {} and starts a run.
Please:
- Validate the loopback
Hoston every request. - For every state-changing POST, require a matching trusted
Origin,application/json, and an unguessable per-process capability/CSRF token in a custom header. Reject missing/foreign origins, invalid content types/tokens, and do not enable permissive CORS. - Add negative tests proving foreign
Host/Origin, missingOrigin, form ortext/plainbodies, and invalid tokens cannot change config/prompts, start a run, or adopt anything, while retaining same-origin JSON happy-path tests.
Once rebased and covered by those tests, we will re-review promptly.
|
Hi @Alphaxalchemy — following up after rechecking the current head. The main blocker is that the local dashboard's mutating endpoints have no request-authentication or CSRF capability. POST /api/run, /api/adopt, /api/config, and /api/prompts accept browser-origin requests without Host, Origin, content-type, or token validation, and an empty or malformed body is coerced to {}. Binding to localhost alone does not prevent hostile cross-origin browser requests or DNS-rebinding-style attacks. To move this forward, please:
The dashboard direction is useful, but endpoints that can start runs, adopt skills, or modify configuration need this boundary before merge. |
A zero-dependency (stdlib http.server) web UI over one project's sleep
pipeline, arranged to mirror the data flow: per-stage role/model/prompt
display with live prompt editing, a per-night evidentiary chain browser
over evidence.jsonl, the gate arithmetic spelled out, a config editor, a
run/dry-run launcher, and explicit adopt (nothing auto-adopts from the UI).
python -m skillopt_sleep dashboard [--project DIR] [--port N]
Addresses the review on microsoft#152:
* Rebased onto current main; the evidence-chain commit landed as microsoft#151 and
is dropped here. This commit is only the dashboard and its tests.
* Request authorization. Binding to 127.0.0.1 is not an authorization
boundary — any page in the user's browser can POST cross-origin to
loopback, and a hostile name resolving to loopback (DNS rebinding) makes
those requests look local. Every request now passes through _authorize:
- loopback Host is required on every request, GET included, with the
port matching the bound port. This is what defeats rebinding: the
browser sends the attacker's name, not ours.
- state-changing POSTs additionally require a trusted Origin (missing
and foreign are both rejected), an application/json content type, and
an unguessable per-process capability token in a custom header.
Each is independently sufficient to stop a cross-origin page: a form
cannot set a custom header or a JSON content type without a preflight,
which this server deliberately does not answer. No CORS headers are
emitted anywhere, so foreign origins cannot read responses either.
The token is minted per server process and substituted into the served
HTML, so it never travels in a URL and restarting the dashboard
invalidates every previously served page.
* Request bodies are validated, not coerced. An absent, empty, malformed,
non-object or oversized body is now a specific 4xx instead of silently
becoming {}. That coercion is what let a contentless cross-origin POST
to /api/run start a real run. Oversized bodies are drained within a
bound before the 413 so well-behaved clients read the status rather than
a connection reset.
tests/test_dashboard_security.py adds 25 tests. Every negative case
asserts twice — that the request was refused, and that the side effect did
not happen, since a 403 that still started a run would pass a status-only
test. Covers foreign Host (all seven endpoints), wrong/absent port,
rebinding names, foreign/missing/null/lookalike Origins, form,
text/plain, multipart and absent content types, missing/wrong/
cross-process tokens, empty, malformed, non-object and oversized bodies,
absence of CORS headers, unanswered preflight, and the reported vector
verbatim (empty cross-origin form POST to /api/run). Same-origin JSON
happy paths for config, prompts, run and adopt are retained.
fd6dc42 to
8c88eef
Compare
|
Thanks — the CSRF analysis was correct, including the specific vector. All of it is addressed in Rebase. Onto current 1. Loopback 2. Origin, content type, and capability token on every state-changing POST.
The token is substituted into the served HTML at request time rather than passed in a URL, so it stays out of referrers, shell history and logs; only a same-origin document can read the page body, and restarting the dashboard invalidates every previously served page. The handler is now a per-server subclass, so the token and port cannot leak between servers. No permissive CORS. No Bodies are validated, not coerced. 3. Negative tests. Coverage: foreign Your reported vector is in there verbatim as Full suite: 275 tests, 20 failures — byte-identical to the same run against pristine One note for your judgment: the run/adopt/config/prompt endpoints are protected, but |
There was a problem hiding this comment.
Pull request overview
Adds a stdlib-only local “control panel” dashboard for SkillOpt-Sleep, exposing the per-night evidence chain and allowing controlled editing of config/prompts and launching/adopting runs via a loopback-bound HTTP server.
Changes:
- Introduces
skillopt_sleep.dashboard(ThreadingHTTPServer) with JSON APIs for overview/night inspection, config & prompt edits, run control, and adoption. - Adds a single-file
dashboard.htmlUI that renders the pipeline stages, evidence chain, gate math, and editors for prompts/config. - Wires a new
python -m skillopt_sleep dashboardCLI subcommand and adds a focused security test suite for the authorization boundary.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/test_dashboard_security.py |
Adds extensive negative/positive tests for Host/Origin/token/content-type/body validation and CORS/preflight behavior. |
skillopt_sleep/dashboard.py |
Implements the local HTTP server, request authorization, config/prompt mutation endpoints, run launcher, and night/adopt endpoints. |
skillopt_sleep/dashboard.html |
Provides the in-browser UI for browsing nights/evidence and performing safe edits and actions. |
skillopt_sleep/__main__.py |
Adds the dashboard CLI command to start the server. |
Suppressed comments (1)
skillopt_sleep/dashboard.py:402
/api/adoptbuilds the staging directory fromos.path.basename(body["ts"]), which still permits".."and symlink escapes. A craftedtscan make the dashboard calladopt_staging()on a directory outside the staging root, potentially overwriting arbitrary live paths if that directory contains a compatiblemanifest.json. Validatetsand ensure the resolved path remains withinstaging_root(project)before adopting.
ts = os.path.basename(str(body.get("ts", "")))
d = os.path.join(staging_root(self.project), ts)
if not os.path.isdir(d):
self._json({"error": "unknown night"}, 404)
return
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(kind==="select"){ | ||
| d.innerHTML = `<label>${esc(label)}</label><select data-k='${key}'>` + | ||
| opts.map(o=>`<option value='${esc(o)}' ${String(v)===o?"selected":""}>${esc(o||"(inherit)")}</option>`).join("") + "</select>"; | ||
| } else { | ||
| d.innerHTML = `<label>${esc(label)}</label><input data-k='${key}' type='${kind==="number"?"text":"text"}' value='${esc(v??"")}'>`; | ||
| } | ||
| g.appendChild(d); | ||
| } | ||
| } | ||
| async function saveConfig(){ | ||
| const updates = {}; | ||
| document.querySelectorAll("#cfgGrid [data-k]").forEach(el=>{ | ||
| let v = el.value; | ||
| if(v==="true") v = true; else if(v==="false") v = false; | ||
| else if(v!=="" && !isNaN(Number(v)) && el.closest(".cfgitem").querySelector("label").textContent.match(/weight|budget|fraction|hours|tasks|rollouts|factor|K$|chars/i)) v = Number(v); | ||
| updates[el.dataset.k] = v; | ||
| }); |
| ts = os.path.basename(path[len("/api/night/"):]) | ||
| d = os.path.join(staging_root(self.project), ts) | ||
| if not os.path.isdir(d): | ||
| self._json({"error": "unknown night"}, 404) | ||
| return |
| with self.lock: | ||
| if self.running(): | ||
| return {"ok": False, "error": "a run is already in progress"} | ||
| cfg = load_config(invoked_project=project) | ||
| self.log_path = os.path.join(cfg.state_dir, "dashboard-run.log") | ||
| os.makedirs(os.path.dirname(self.log_path), exist_ok=True) | ||
| self.mode = "dry-run" if dry_run else "run" | ||
| cmd = [sys.executable, "-m", "skillopt_sleep", self.mode, | ||
| "--project", project, "--progress"] | ||
| log = open(self.log_path, "w", encoding="utf-8") | ||
| no_window = getattr(subprocess, "CREATE_NO_WINDOW", 0) if os.name == "nt" else 0 | ||
| self.proc = subprocess.Popen( | ||
| cmd, stdout=log, stderr=subprocess.STDOUT, | ||
| creationflags=no_window, cwd=project or None, | ||
| ) | ||
| return {"ok": True, "mode": self.mode} |
|
Thanks @copilot — all three are real, and all three are fixed in 1. You were right that The adopt path was the worse of the two: it fed that directory straight to That last step is what catches the symlink case you flagged. Verified with a directory junction, which is the unprivileged Windows equivalent: Non-string 2. The On the handle: in CPython the local 3. Config editor typing — fixed, and pushed down to the server. Confirmed, and the ternary was doing nothing at all: type='${kind==="number"?"text":"text"}'Fields now declare a kind ( I also took your parenthetical — " Fixing the blanket coercion also fixed a bug in the same line: 12 tests added (37 in the module, no skips): traversal and link-escape attempts across both endpoints — each asserting nothing was adopted, not just the status code — spawn-failure handling and log-handle release, and config typing including the partial-write and free-text cases. Full suite: 287 tests, 20 failures, byte-identical to the same run against pristine |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
skillopt_sleep/dashboard.py:236
- _RunState.status() is intended to return a tail of the run log, but it reads only the first 200k bytes via _read_text() and then slices the last 6000 characters from that chunk. Once the log grows beyond 200k, the UI will stop showing the true end of the log (it will show the end of the first chunk instead).
def status(self) -> Dict[str, Any]:
tail = ""
if self.log_path:
text = _read_text(self.log_path)
tail = text[-6000:]
rc = None
if self.proc is not None:
rc = self.proc.poll()
return {"running": self.running(), "returncode": rc,
"mode": self.mode, "tail": tail}
skillopt_sleep/dashboard.py:124
- _coerce_config_value() accepts float strings like "nan"/"inf" because float() parses them successfully. Persisting NaN/Infinity into config will propagate non-finite values into gate arithmetic and can produce confusing results or non-JSON-safe outputs in downstream reporting.
if isinstance(default, float):
try:
return float(str(value).strip())
except (TypeError, ValueError):
raise ValueError(f"{key} must be a number") from None
skillopt_sleep/dashboard.py:94
- The dashboard always writes to ~/.skillopt-sleep/config.json. If a user currently uses config.yaml/config.yml (supported by load_config()), saving from the dashboard will create/overwrite config.json and silently change precedence (config.json is loaded before YAML), potentially altering behavior in a hard-to-debug way.
def _user_config_file() -> str:
return os.path.join(HOME_STATE_DIR, "config.json")
A zero-dependency (stdlib http.server) web UI over one project's sleep
pipeline, arranged to mirror the data flow: per-stage role/model/prompt
display with live prompt editing, a per-night evidentiary chain browser
over evidence.jsonl, the gate arithmetic spelled out, a config editor, a
run/dry-run launcher, and explicit adopt (nothing auto-adopts from the UI).
python -m skillopt_sleep dashboard [--project DIR] [--port N]
Addresses the review on microsoft#152:
* Rebased onto current main; the evidence-chain commit landed as microsoft#151 and
is dropped here. This commit is only the dashboard and its tests.
* Request authorization. Binding to 127.0.0.1 is not an authorization
boundary — any page in the user's browser can POST cross-origin to
loopback, and a hostile name resolving to loopback (DNS rebinding) makes
those requests look local. Every request now passes through _authorize:
- loopback Host is required on every request, GET included, with the
port matching the bound port. This is what defeats rebinding: the
browser sends the attacker's name, not ours.
- state-changing POSTs additionally require a trusted Origin (missing
and foreign are both rejected), an application/json content type, and
an unguessable per-process capability token in a custom header.
Each is independently sufficient to stop a cross-origin page: a form
cannot set a custom header or a JSON content type without a preflight,
which this server deliberately does not answer. No CORS headers are
emitted anywhere, so foreign origins cannot read responses either.
The token is minted per server process and substituted into the served
HTML, so it never travels in a URL and restarting the dashboard
invalidates every previously served page.
* Request bodies are validated, not coerced. An absent, empty, malformed,
non-object or oversized body is now a specific 4xx instead of silently
becoming {}. That coercion is what let a contentless cross-origin POST
to /api/run start a real run. Oversized bodies are drained within a
bound before the 413 so well-behaved clients read the status rather than
a connection reset.
tests/test_dashboard_security.py adds 25 tests. Every negative case
asserts twice — that the request was refused, and that the side effect did
not happen, since a 403 that still started a run would pass a status-only
test. Covers foreign Host (all seven endpoints), wrong/absent port,
rebinding names, foreign/missing/null/lookalike Origins, form,
text/plain, multipart and absent content types, missing/wrong/
cross-process tokens, empty, malformed, non-object and oversized bodies,
absence of CORS headers, unanswered preflight, and the reported vector
verbatim (empty cross-origin form POST to /api/run). Same-origin JSON
happy paths for config, prompts, run and adopt are retained.
…rites
Addresses three review findings on the dashboard.
* Staging path containment. `os.path.basename` is not containment:
`basename("..")` is `".."`, so `/api/night/..` resolved to the staging
parent and `/api/adopt` would have handed that directory to
adopt_staging, copying whatever it found over the live SKILL.md and
CLAUDE.md. Both endpoints now share `_night_dir`, which percent-decodes
first, rejects anything that is not a plain single path component, and
then confirms the *resolved* path is still under the staging root — so a
symlink or junction planted inside staging cannot redirect the read
either. Non-string `ts` values are rejected rather than stringified.
* Run launcher resilience. A failing `subprocess.Popen` raised straight
out of the request-handler thread, dropping the connection and leaving
the UI waiting; it now returns {ok: false, error} and the launcher stays
usable. The parent also closes its own copy of the log descriptor after
spawning instead of relying on refcounting to release it, which on
Windows kept the file locked.
* Config value typing. The editor declared fields as "number" but rendered
every one as `type="text"` (a no-op ternary) and decided what to coerce
by matching a regex against the *label text* — so renaming a label would
silently start writing strings into config.json, and `load_config` does
not type-coerce. Fields now declare a kind (text/select/bool/int/float),
rendering and parsing both key off that kind, and the server validates
independently in `_coerce_config_value` against the type of each built-in
default, since the client is not the only possible caller. A value that
cannot be coerced is a 400 and nothing is written, so a bad field cannot
half-apply a form. Free text is no longer coerced: "true" in the house
rules stays the string "true".
Adds 12 tests: traversal and link-escape attempts on both endpoints
(asserting nothing was adopted, with a junction fallback so the link case
runs on unprivileged Windows too), spawn-failure handling and log-handle
release, and config typing including the partial-write and free-text cases.
714159d to
b3db5ef
Compare
Local control-panel dashboard for SkillOpt-Sleep
A zero-dependency (stdlib
http.server) web UI over one project's sleeppipeline, served on
127.0.0.1.Rebased onto current
main. #151 has merged, so its evidence-chain commit isdropped — this PR is now just the dashboard and its tests (4 files).
Why
#151 makes every night's chain recorded; this PR makes it inspectable and
steerable by a human. The sleep gate is strict by design, and when it rejects a
night there was no way to see why without spelunking JSONL by hand — and no way
to adjust the four prompts driving the pipeline without editing source. If the
goal is validating the miner and evolving the prompts, a human needs a place to
read the exchanges and try variants first.
What
skillopt_sleep/dashboard.py+dashboard.html— laid out to mirror theactual data flow:
the registry (overrides apply on the next model call);
evidence.jsonl— harvest,miner exchanges, mined tasks + checks, replay attempts, reflect edits;
auto-adopts from the UI).
CLI:
python -m skillopt_sleep dashboard [--port] [--no-browser].Security boundary
Binding to loopback is not an authorization boundary. Any page in the user's
browser can POST cross-origin to
127.0.0.1, and a hostile name resolving toloopback (DNS rebinding) makes those requests look local. Since these endpoints
start real runs, adopt artifacts, and rewrite config and prompts, every request
passes through
_authorize:Hoston every request, GET included, with the port matching thebound port. This is what defeats rebinding — the browser sends the attacker's
name, not ours.
Origin(missing andforeign both rejected), an
application/jsoncontent type, and an unguessableper-process capability token in a custom header.
Each is independently sufficient to stop a cross-origin page: a form cannot set a
custom header or a JSON content type without a preflight, which this server
deliberately does not answer. No CORS headers are emitted anywhere, so a foreign
origin cannot read responses either. The token is minted per server process and
substituted into the served HTML, so it never travels in a URL, and restarting
the dashboard invalidates every previously served page.
Request bodies are validated rather than coerced: absent, empty, malformed,
non-object and oversized bodies each get a specific 4xx instead of silently
becoming
{}.Tests
tests/test_dashboard_security.py— 25 tests. Every negative case asserts twice:that the request was refused, and that the side effect did not happen (a 403
that still started a run would pass a status-code-only test).
Covers foreign
Hostacross all seven endpoints, wrong and absent port,rebinding names, foreign/missing/
null/lookalikeOrigins, form,text/plain,multipart and absent content types, missing/wrong/cross-process tokens, empty,
malformed, non-object and oversized bodies, absence of CORS headers, unanswered
preflight, and the reported vector verbatim. Same-origin JSON happy paths for
config, prompts, run and adopt are retained.
Full suite: 20 pre-existing failures on this Windows checkout, byte-identical to
pristine
main(e7014cd) — no new failures.Series
Part 2 of 3: #151 (evidence chain + prompt registry, merged) → this PR →
#153 (Antigravity harvest source, per the "more sources" direction of #97).