Fix critical WebSocket origin bypass, zombie processes, and review findings
Addresses findings from Devin and Codex reviews on the merged PR #10: - CRITICAL: /ws/terminal accepted connections from any origin - Starlette's CORSMiddleware does not protect WebSocket handshakes, so any webpage could open a socket to the dashboard's terminal and get an interactive shell on the user's machine. Now validates the Origin header against the same allowed-origins list used for CORS before accepting. - PtySession.close() sent SIGKILL to the shell's PID but never reaped it via os.waitpid(), leaking a zombie process per closed terminal session. - hermes_available() ran a real subprocess (possibly bridged through WSL) on every /api/status poll, which the dashboard hits every 15s. Added a 60s TTL cache. - create_skill() now also creates learnings.md and the context/ directory, matching the standard skill template (_template/) instead of only writing SKILL.md. - The '+ New Skill' button stayed visible in the Skills Hub detail view since only its sibling filter input was hidden; both now live under a shared #skillActions container that's hidden/shown together. Verified: malicious/missing-origin WebSocket connections are rejected at the handshake (HTTP 403) before any shell spawns; a valid dashboard origin still connects and works; closing a session leaves no zombie/orphaned process; the hermes availability cache avoids repeat subprocess spawns.
This commit is contained in:
parent
71650cbc0b
commit
d149356a22
|
|
@ -6,7 +6,7 @@ async function renderSkills() {
|
|||
<h1 class="page-title">Skills Hub</h1>
|
||||
<p class="page-subtitle">Browse, run, and monitor skill performance</p>
|
||||
</div>
|
||||
<div class="btn-group">
|
||||
<div class="btn-group" id="skillActions">
|
||||
<input id="skillFilter" class="form-input" style="width:200px" placeholder="Filter skills..." oninput="filterSkills()">
|
||||
<button class="btn btn-primary" onclick="showAddSkill()">+ New Skill</button>
|
||||
</div>
|
||||
|
|
@ -83,7 +83,7 @@ function filterSkills() {
|
|||
async function showSkillDetail(name) {
|
||||
document.getElementById('skillsContainer').style.display = 'none';
|
||||
document.getElementById('skillTabs').style.display = 'none';
|
||||
document.getElementById('skillFilter').style.display = 'none';
|
||||
document.getElementById('skillActions').style.display = 'none';
|
||||
const detail = document.getElementById('skillDetail');
|
||||
detail.style.display = 'block';
|
||||
detail.innerHTML = `<div class="loading"><div class="loading-spinner"></div></div>`;
|
||||
|
|
@ -152,7 +152,7 @@ async function showSkillDetail(name) {
|
|||
function backToSkills() {
|
||||
document.getElementById('skillsContainer').style.display = '';
|
||||
document.getElementById('skillTabs').style.display = '';
|
||||
document.getElementById('skillFilter').style.display = '';
|
||||
document.getElementById('skillActions').style.display = '';
|
||||
document.getElementById('skillDetail').style.display = 'none';
|
||||
}
|
||||
|
||||
|
|
|
|||
27
server.py
27
server.py
|
|
@ -166,12 +166,22 @@ def hermes_cli_args(*args: str) -> list:
|
|||
return ["wsl", "-e", "bash", "-lc", f"hermes {quoted}"]
|
||||
return ["hermes", *args]
|
||||
|
||||
_hermes_available_cache = {"checked_at": 0.0, "result": False}
|
||||
HERMES_AVAILABLE_CACHE_TTL = 60
|
||||
|
||||
def hermes_available() -> bool:
|
||||
"""Cached: this spawns a subprocess (possibly via WSL), and /api/status is polled every 15s."""
|
||||
now = time.time()
|
||||
if now - _hermes_available_cache["checked_at"] < HERMES_AVAILABLE_CACHE_TTL:
|
||||
return _hermes_available_cache["result"]
|
||||
try:
|
||||
r = subprocess.run(hermes_cli_args("--version"), capture_output=True, text=True, timeout=10)
|
||||
return r.returncode == 0
|
||||
result = r.returncode == 0
|
||||
except Exception:
|
||||
return False
|
||||
result = False
|
||||
_hermes_available_cache["checked_at"] = now
|
||||
_hermes_available_cache["result"] = result
|
||||
return result
|
||||
|
||||
def check_agent(name: str) -> dict:
|
||||
"""Filesystem-based check for opencode/gemini; hermes needs a real subprocess since it may live inside WSL."""
|
||||
|
|
@ -300,7 +310,9 @@ def create_skill(data: SkillCreate):
|
|||
if path.exists():
|
||||
raise HTTPException(409, "Skill already exists")
|
||||
path.mkdir(parents=True)
|
||||
(path / "context").mkdir()
|
||||
(path / "SKILL.md").write_text(data.skill_md, encoding="utf-8")
|
||||
(path / "learnings.md").write_text("", encoding="utf-8")
|
||||
append_audit({"action": "skill_created", "skill": data.name})
|
||||
return {"name": data.name}
|
||||
|
||||
|
|
@ -838,9 +850,20 @@ class PtySession:
|
|||
os.kill(self.pid, signal.SIGKILL)
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
os.waitpid(self.pid, 0) # reap the killed child so it doesn't stay a zombie
|
||||
except ChildProcessError:
|
||||
pass
|
||||
|
||||
@app.websocket("/ws/terminal")
|
||||
async def ws_terminal(websocket: WebSocket):
|
||||
# CORSMiddleware does not protect WebSocket handshakes, so this endpoint - which spawns a
|
||||
# full interactive shell - must check the Origin header itself, or any webpage could open
|
||||
# this socket and get command execution on the machine running the dashboard.
|
||||
origin = websocket.headers.get("origin")
|
||||
if origin not in get_cors_origins():
|
||||
await websocket.close(code=1008)
|
||||
return
|
||||
await websocket.accept()
|
||||
session = PtySession()
|
||||
try:
|
||||
|
|
|
|||
Loading…
Reference in New Issue