Skip to content

Commit b3db5ef

Browse files
committed
fix(sleep): contain staging paths, harden run launcher, type config writes
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.
1 parent 3f23736 commit b3db5ef

3 files changed

Lines changed: 305 additions & 38 deletions

File tree

‎skillopt_sleep/dashboard.html‎

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -517,47 +517,67 @@ <h2 class='sec' style='margin-top:13px'>Evidence — night ${esc(SEL||"—")}</h
517517
["azure_endpoint","azure / compat endpoint","text"],
518518
["gate_mode","gate mode","select",["on","off"]],
519519
["gate_metric","gate metric","select",["mixed","hard","soft"]],
520-
["gate_mixed_weight","mixed weight (soft share)","number"],
521-
["edit_budget","edit budget / night","number"],
522-
["holdout_fraction","holdout fraction (val)","number"],
523-
["max_tasks_per_night","max tasks / night","number"],
524-
["lookback_hours","lookback hours (first run)","number"],
525-
["dream_rollouts","dream rollouts (K)","number"],
526-
["dream_factor","dream factor","number"],
527-
["recall_k","recall K","number"],
528-
["llm_mine","LLM mining","select",["true","false"]],
529-
["evolve_skill","evolve SKILL.md","select",["true","false"]],
530-
["evolve_memory","evolve CLAUDE.md","select",["true","false"]],
531-
["evidence_log","evidence log","select",["true","false"]],
532-
["auto_adopt","auto-adopt (careful)","select",["false","true"]],
520+
["gate_mixed_weight","mixed weight (soft share)","float"],
521+
["edit_budget","edit budget / night","int"],
522+
["holdout_fraction","holdout fraction (val)","float"],
523+
["max_tasks_per_night","max tasks / night","int"],
524+
["lookback_hours","lookback hours (first run)","int"],
525+
["dream_rollouts","dream rollouts (K)","int"],
526+
["dream_factor","dream factor","int"],
527+
["recall_k","recall K","int"],
528+
["llm_mine","LLM mining","bool"],
529+
["evolve_skill","evolve SKILL.md","bool"],
530+
["evolve_memory","evolve CLAUDE.md","bool"],
531+
["evidence_log","evidence log","bool"],
532+
["auto_adopt","auto-adopt (careful)","bool",["false","true"]],
533533
["preferences","house rules for the optimizer","text"],
534534
];
535+
/* Every field declares its own kind, and both rendering and parsing key off
536+
that kind — never off the label text. Renaming a label is a copy change,
537+
not a silent type change that writes a string into config.json. */
535538
function renderConfig(){
536539
const g = $("#cfgGrid"); g.innerHTML = "";
537540
for(const [key,label,kind,opts] of CFG_FIELDS){
538541
const v = OV.config[key];
539542
const d = document.createElement("div"); d.className = "cfgitem";
540-
if(kind==="select"){
541-
d.innerHTML = `<label>${esc(label)}</label><select data-k='${key}'>` +
542-
opts.map(o=>`<option value='${esc(o)}' ${String(v)===o?"selected":""}>${esc(o||"(inherit)")}</option>`).join("") + "</select>";
543+
const choices = kind==="bool" ? (opts || ["true","false"]) : opts;
544+
if(kind==="select" || kind==="bool"){
545+
d.innerHTML = `<label>${esc(label)}</label><select data-k='${key}' data-kind='${kind}'>` +
546+
choices.map(o=>`<option value='${esc(o)}' ${String(v)===o?"selected":""}>${esc(o||"(inherit)")}</option>`).join("") + "</select>";
547+
} else if(kind==="int" || kind==="float"){
548+
d.innerHTML = `<label>${esc(label)}</label><input data-k='${key}' data-kind='${kind}'` +
549+
` type='number' step='${kind==="int"?"1":"any"}' value='${esc(v??"")}'>`;
543550
} else {
544-
d.innerHTML = `<label>${esc(label)}</label><input data-k='${key}' type='${kind==="number"?"text":"text"}' value='${esc(v??"")}'>`;
551+
d.innerHTML = `<label>${esc(label)}</label><input data-k='${key}' data-kind='text' type='text' value='${esc(v??"")}'>`;
545552
}
546553
g.appendChild(d);
547554
}
548555
}
556+
function parseCfgValue(kind, raw){
557+
const v = String(raw ?? "");
558+
if(v === "") return ""; // empty resets to the default
559+
if(kind === "bool") return v === "true";
560+
if(kind === "int" || kind === "float"){
561+
const n = Number(v);
562+
if(!Number.isFinite(n)) return null; // signals a validation failure
563+
return kind === "int" ? Math.trunc(n) : n;
564+
}
565+
return v; // text/select stay strings
566+
}
549567
async function saveConfig(){
550568
const updates = {};
569+
const bad = [];
551570
document.querySelectorAll("#cfgGrid [data-k]").forEach(el=>{
552-
let v = el.value;
553-
if(v==="true") v = true; else if(v==="false") v = false;
554-
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);
555-
updates[el.dataset.k] = v;
571+
const parsed = parseCfgValue(el.dataset.kind, el.value);
572+
if(parsed === null){ bad.push(el.dataset.k); return; }
573+
updates[el.dataset.k] = parsed;
556574
});
575+
if(bad.length){ alert("Not a valid number: " + bad.join(", ")); return; }
557576
const r = await post("/api/config", {updates});
558577
if(r.ok){ OV.config = r.config; const m = $("#cfgMsg");
559578
m.classList.add("show"); setTimeout(()=>m.classList.remove("show"), 1600);
560579
renderConfig(); renderMap(); renderStages(); }
580+
else if(r.error){ alert("Config not saved: " + r.error); }
561581
}
562582

563583
/* ── run control ────────────────────────────────────────────────────── */

‎skillopt_sleep/dashboard.py‎

Lines changed: 109 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import threading
3737
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
3838
from typing import Any, Dict, List, Optional, Tuple
39+
from urllib.parse import unquote
3940

4041
from skillopt_sleep import prompts as prompt_registry
4142
from skillopt_sleep.config import DEFAULTS, HOME_STATE_DIR, load_config
@@ -93,17 +94,61 @@ def _user_config_file() -> str:
9394
return os.path.join(HOME_STATE_DIR, "config.json")
9495

9596

97+
def _coerce_config_value(key: str, value: Any) -> Any:
98+
"""Coerce a submitted value to the type of the built-in default.
99+
100+
``load_config`` does not type-coerce, so a string where a number belongs
101+
reaches the arithmetic downstream (``edit_budget: "4"``). The default's
102+
type is the schema — this is the only place that knows it, and it is
103+
enforced server-side because the client is not the only possible caller.
104+
"""
105+
default = DEFAULTS.get(key)
106+
if isinstance(default, bool): # before int: bool is a subclass of int
107+
if isinstance(value, bool):
108+
return value
109+
text = str(value).strip().lower()
110+
if text in {"true", "1", "yes", "on"}:
111+
return True
112+
if text in {"false", "0", "no", "off"}:
113+
return False
114+
raise ValueError(f"{key} must be a boolean")
115+
if isinstance(default, int):
116+
try:
117+
return int(str(value).strip())
118+
except (TypeError, ValueError):
119+
raise ValueError(f"{key} must be an integer") from None
120+
if isinstance(default, float):
121+
try:
122+
return float(str(value).strip())
123+
except (TypeError, ValueError):
124+
raise ValueError(f"{key} must be a number") from None
125+
if isinstance(default, str):
126+
if isinstance(value, (dict, list)):
127+
raise ValueError(f"{key} must be a string")
128+
return str(value)
129+
return value
130+
131+
96132
def _write_config(updates: Dict[str, Any]) -> Dict[str, Any]:
133+
"""Merge validated updates into the user config file.
134+
135+
Raises ValueError if any submitted value cannot be coerced; nothing is
136+
written in that case, so a bad field cannot half-apply a form.
137+
"""
97138
path = _user_config_file()
98139
current = _read_json(path) or {}
140+
accepted: Dict[str, Any] = {}
141+
removed = []
99142
for k, v in updates.items():
100143
if k not in _EDITABLE_KEYS:
101144
continue
102145
if v is None or v == "":
103-
# empty resets the key to the built-in default
104-
current.pop(k, None)
146+
removed.append(k) # empty resets the key to the built-in default
105147
else:
106-
current[k] = v
148+
accepted[k] = _coerce_config_value(k, v)
149+
for k in removed:
150+
current.pop(k, None)
151+
current.update(accepted)
107152
os.makedirs(os.path.dirname(path), exist_ok=True)
108153
with open(path, "w", encoding="utf-8") as f:
109154
json.dump(current, f, ensure_ascii=False, indent=2)
@@ -158,15 +203,25 @@ def start(self, project: str, dry_run: bool) -> Dict[str, Any]:
158203
cfg = load_config(invoked_project=project)
159204
self.log_path = os.path.join(cfg.state_dir, "dashboard-run.log")
160205
os.makedirs(os.path.dirname(self.log_path), exist_ok=True)
161-
self.mode = "dry-run" if dry_run else "run"
162-
cmd = [sys.executable, "-m", "skillopt_sleep", self.mode,
206+
mode = "dry-run" if dry_run else "run"
207+
cmd = [sys.executable, "-m", "skillopt_sleep", mode,
163208
"--project", project, "--progress"]
164-
log = open(self.log_path, "w", encoding="utf-8")
165209
no_window = getattr(subprocess, "CREATE_NO_WINDOW", 0) if os.name == "nt" else 0
166-
self.proc = subprocess.Popen(
167-
cmd, stdout=log, stderr=subprocess.STDOUT,
168-
creationflags=no_window, cwd=project or None,
169-
)
210+
try:
211+
# The child inherits a dup of this descriptor, so the parent
212+
# closes its own copy immediately rather than leaving the log
213+
# held open (and, on Windows, locked) for the server's life.
214+
with open(self.log_path, "w", encoding="utf-8") as log:
215+
self.proc = subprocess.Popen(
216+
cmd, stdout=log, stderr=subprocess.STDOUT,
217+
creationflags=no_window, cwd=project or None,
218+
)
219+
except OSError as exc:
220+
# A failed spawn must not raise out of the request thread —
221+
# that would drop the connection and leave the UI hanging.
222+
self.proc = None
223+
return {"ok": False, "error": f"could not start {mode}: {exc}"}
224+
self.mode = mode
170225
return {"ok": True, "mode": self.mode}
171226

172227
def status(self) -> Dict[str, Any]:
@@ -181,6 +236,40 @@ def status(self) -> Dict[str, Any]:
181236
"mode": self.mode, "tail": tail}
182237

183238

239+
def _night_dir(project: str, ts: str) -> Optional[str]:
240+
"""Resolve a night id to its staging directory, or None if it is not one.
241+
242+
``os.path.basename`` alone is not containment: it leaves ``".."`` intact,
243+
so ``/api/night/..`` resolved to the staging parent and ``/api/adopt``
244+
would have copied whatever it found there over the live SKILL.md and
245+
CLAUDE.md. Reject anything that is not a plain single path component, then
246+
confirm the *resolved* path is still under the staging root so a symlink
247+
planted in staging cannot redirect the read either.
248+
"""
249+
name = str(ts or "")
250+
if not name or name in {".", ".."}:
251+
return None
252+
if name != os.path.basename(name): # separators, drive letters, absolutes
253+
return None
254+
if os.sep in name or (os.altsep and os.altsep in name):
255+
return None
256+
257+
root = staging_root(project)
258+
candidate = os.path.join(root, name)
259+
try:
260+
real_root = os.path.realpath(root)
261+
real_candidate = os.path.realpath(candidate)
262+
if os.path.commonpath([real_root, real_candidate]) != real_root:
263+
return None
264+
if real_candidate == real_root:
265+
return None
266+
except (OSError, ValueError): # unrelated roots / different drives
267+
return None
268+
if not os.path.isdir(real_candidate):
269+
return None
270+
return candidate
271+
272+
184273
def _split_host(value: str) -> Tuple[str, str]:
185274
"""Split a Host header into (hostname, port), tolerating IPv6 brackets."""
186275
host = (value or "").strip()
@@ -342,9 +431,9 @@ def do_GET(self) -> None: # noqa: N802 (http.server API)
342431
})
343432
return
344433
if path.startswith("/api/night/"):
345-
ts = os.path.basename(path[len("/api/night/"):])
346-
d = os.path.join(staging_root(self.project), ts)
347-
if not os.path.isdir(d):
434+
ts = unquote(path[len("/api/night/"):])
435+
d = _night_dir(self.project, ts)
436+
if d is None:
348437
self._json({"error": "unknown night"}, 404)
349438
return
350439
self._json({
@@ -378,7 +467,11 @@ def do_POST(self) -> None: # noqa: N802
378467
if not isinstance(updates, dict):
379468
self._json({"error": "updates must be an object"}, 400)
380469
return
381-
saved = _write_config(updates)
470+
try:
471+
saved = _write_config(updates)
472+
except ValueError as exc:
473+
self._json({"error": str(exc)}, 400)
474+
return
382475
cfg = load_config(invoked_project=self.project)
383476
self._json({"ok": True, "saved": saved,
384477
"config": {k: cfg.get(k) for k in sorted(_EDITABLE_KEYS)}})
@@ -395,9 +488,8 @@ def do_POST(self) -> None: # noqa: N802
395488
self._json(self.run_state.start(self.project, bool(body.get("dry_run"))))
396489
return
397490
if path == "/api/adopt":
398-
ts = os.path.basename(str(body.get("ts", "")))
399-
d = os.path.join(staging_root(self.project), ts)
400-
if not os.path.isdir(d):
491+
d = _night_dir(self.project, body.get("ts", ""))
492+
if d is None:
401493
self._json({"error": "unknown night"}, 404)
402494
return
403495
try:

0 commit comments

Comments
 (0)