fix: resolve federation, settings, and session creation bugs
Seven bug fixes across five files: 1. auth.py: Add .json to static extension allowlist so /manifest.json bypasses auth. Previously, unauthenticated requests got redirected to the login page HTML which the browser tried to parse as JSON. 2. sessions.py: Catch FileNotFoundError in enumerate_sessions() alongside RuntimeError. If the session command binary is missing from PATH, asyncio.create_subprocess_exec raises FileNotFoundError, which was unhandled and broke the poll loop. 3. settings.py: Fix federation key erasure when remote instance URLs are edited. The key-preservation logic only restored keys by exact URL match. Editing a URL (e.g. http:// to https://) changed the lookup key, permanently erasing the real federation key. Added position-based fallback for same-slot URL edits. 4. main.py: Replace fire-and-forget subprocess.Popen in create_session with asyncio.create_subprocess_shell that checks the return code and returns proper HTTP 500 errors. Added shutil.which() pre-flight check. Commands that exit non-zero but still create the session (e.g. amplifier-workspace which tries to attach after create) are detected by checking enumerate_sessions() and treated as success. 5. main.py: Redact sensitive keys in PATCH /api/settings response, matching the existing GET endpoint behavior. 6. main.py: Include exception type and message in federation 503 error responses (connect, bell-clear, create-session). 7. app.js: Fix auto_open vs auto_open_created key mismatch. The settings toggle read/wrote 'auto_open' but the backend key is 'auto_open_created'. The PATCH was silently dropped and the toggle was completely non-functional. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
This commit is contained in:
@@ -145,6 +145,7 @@ _AUTH_EXEMPT_PATHS = {"/login", "/auth/mode", "/auth/logout", "/api/instance-inf
|
|||||||
_STATIC_EXTENSIONS = {
|
_STATIC_EXTENSIONS = {
|
||||||
".css",
|
".css",
|
||||||
".js",
|
".js",
|
||||||
|
".json",
|
||||||
".svg",
|
".svg",
|
||||||
".png",
|
".png",
|
||||||
".ico",
|
".ico",
|
||||||
|
|||||||
@@ -1720,7 +1720,7 @@ function openSettings() {
|
|||||||
// Auto-open
|
// Auto-open
|
||||||
const autoOpenEl = $('setting-auto-open');
|
const autoOpenEl = $('setting-auto-open');
|
||||||
if (autoOpenEl) {
|
if (autoOpenEl) {
|
||||||
autoOpenEl.checked = ss && ss.auto_open !== undefined ? !!ss.auto_open : true;
|
autoOpenEl.checked = ss && ss.auto_open_created !== undefined ? !!ss.auto_open_created : true;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Device name
|
// Device name
|
||||||
@@ -2288,7 +2288,7 @@ function bindStaticEventListeners() {
|
|||||||
});
|
});
|
||||||
on($('setting-auto-open'), 'change', function() {
|
on($('setting-auto-open'), 'change', function() {
|
||||||
var el = $('setting-auto-open');
|
var el = $('setting-auto-open');
|
||||||
if (el) patchServerSetting('auto_open', el.checked);
|
if (el) patchServerSetting('auto_open_created', el.checked);
|
||||||
});
|
});
|
||||||
|
|
||||||
// Hidden sessions — delegated handler on container (checkboxes are dynamic)
|
// Hidden sessions — delegated handler on container (checkboxes are dynamic)
|
||||||
|
|||||||
+102
-19
@@ -18,6 +18,7 @@ import pathlib
|
|||||||
import pwd
|
import pwd
|
||||||
import socket
|
import socket
|
||||||
import ssl
|
import ssl
|
||||||
|
import shutil
|
||||||
import subprocess
|
import subprocess
|
||||||
import sys
|
import sys
|
||||||
import time
|
import time
|
||||||
@@ -376,28 +377,98 @@ async def get_sessions() -> list[dict]:
|
|||||||
|
|
||||||
@app.post("/api/sessions")
|
@app.post("/api/sessions")
|
||||||
async def create_session(payload: CreateSessionPayload) -> dict:
|
async def create_session(payload: CreateSessionPayload) -> dict:
|
||||||
"""Create a new tmux session using the new_session_template from settings.
|
"""Create a new session using the new_session_template from settings.
|
||||||
|
|
||||||
Substitutes {name} in the template with the validated payload name, then
|
Substitutes ``{name}`` in the template with the validated payload name,
|
||||||
runs the command as a fire-and-forget subprocess (stdout/stderr discarded).
|
runs the command as an async subprocess, and waits up to 30 seconds for
|
||||||
|
it to finish. Returns ``{name, ok: True}`` on success or
|
||||||
|
``{name, ok: False, error: ...}`` with HTTP 500 on failure so that the
|
||||||
|
frontend can surface actionable errors instead of silently timing out.
|
||||||
|
|
||||||
Returns {name: name} regardless of outcome.
|
Some session commands (e.g. ``amplifier-workspace``) create the tmux
|
||||||
Raises HTTP 422 if name is empty or whitespace (handled by Pydantic).
|
session and then attempt to *attach* to it, which requires a TTY. When
|
||||||
|
launched from muxplex (no TTY available) the attach step fails with a
|
||||||
|
non-zero exit code even though the session was successfully created. To
|
||||||
|
handle this, when the command exits non-zero we check whether a tmux
|
||||||
|
session with the requested name now exists -- if it does, we treat it as
|
||||||
|
a success.
|
||||||
"""
|
"""
|
||||||
name = payload.name
|
name = payload.name
|
||||||
settings = load_settings()
|
settings = load_settings()
|
||||||
command = settings["new_session_template"].replace("{name}", name)
|
template = settings["new_session_template"]
|
||||||
|
|
||||||
|
# Pre-flight: check that the base command is on PATH.
|
||||||
|
base_cmd = template.split()[0] if template.strip() else ""
|
||||||
|
if base_cmd and not shutil.which(base_cmd):
|
||||||
|
_log.error(
|
||||||
|
"Session command binary not found on PATH: %r (PATH=%s)",
|
||||||
|
base_cmd,
|
||||||
|
os.environ.get("PATH", ""),
|
||||||
|
)
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=500,
|
||||||
|
detail=f"Command not found: {base_cmd}. "
|
||||||
|
"Ensure it is installed and in the server's PATH.",
|
||||||
|
)
|
||||||
|
|
||||||
|
command = template.replace("{name}", name)
|
||||||
_log.info("Creating session '%s' with command: %s", name, command)
|
_log.info("Creating session '%s' with command: %s", name, command)
|
||||||
try:
|
try:
|
||||||
subprocess.Popen(
|
proc = await asyncio.create_subprocess_shell(
|
||||||
command,
|
command,
|
||||||
shell=True,
|
stdout=asyncio.subprocess.PIPE,
|
||||||
stdout=subprocess.DEVNULL,
|
stderr=asyncio.subprocess.PIPE,
|
||||||
stderr=subprocess.DEVNULL,
|
|
||||||
)
|
)
|
||||||
except Exception:
|
stdout_bytes, stderr_bytes = await asyncio.wait_for(
|
||||||
_log.warning("Failed to launch new-session command: %r", command)
|
proc.communicate(), timeout=30
|
||||||
return {"name": name}
|
)
|
||||||
|
if proc.returncode != 0:
|
||||||
|
stderr_text = stderr_bytes.decode("utf-8", errors="replace").strip()
|
||||||
|
# Some commands (amplifier-workspace) create the session then
|
||||||
|
# try to attach (which fails without a TTY). If the session
|
||||||
|
# exists despite the non-zero exit, treat it as success.
|
||||||
|
sessions = await enumerate_sessions()
|
||||||
|
if name in sessions:
|
||||||
|
_log.info(
|
||||||
|
"Session command exited %d but session '%s' exists -- "
|
||||||
|
"treating as success (likely a TTY-attach failure)",
|
||||||
|
proc.returncode,
|
||||||
|
name,
|
||||||
|
)
|
||||||
|
else:
|
||||||
|
_log.warning(
|
||||||
|
"Session command exited %d: %s (stderr: %s)",
|
||||||
|
proc.returncode,
|
||||||
|
command,
|
||||||
|
stderr_text,
|
||||||
|
)
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=500,
|
||||||
|
detail=(
|
||||||
|
f"Session command failed (exit {proc.returncode}): "
|
||||||
|
f"{stderr_text}"
|
||||||
|
)
|
||||||
|
if stderr_text
|
||||||
|
else f"Session command failed with exit code {proc.returncode}",
|
||||||
|
)
|
||||||
|
except asyncio.TimeoutError:
|
||||||
|
_log.info(
|
||||||
|
"Session command still running after 30s (may be long-lived): %s",
|
||||||
|
command,
|
||||||
|
)
|
||||||
|
# Long-running session commands (e.g. amplifier-workspace that
|
||||||
|
# spawns background processes) may outlive the 30s window. This is
|
||||||
|
# not necessarily an error -- return success and let the frontend
|
||||||
|
# poll for the session to appear.
|
||||||
|
except HTTPException:
|
||||||
|
raise
|
||||||
|
except Exception as exc:
|
||||||
|
_log.warning("Failed to launch session command %r: %s", command, exc)
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=500,
|
||||||
|
detail=f"Failed to launch command: {exc}",
|
||||||
|
)
|
||||||
|
return {"name": name, "ok": True}
|
||||||
|
|
||||||
|
|
||||||
@app.post("/api/sessions/{name}/connect")
|
@app.post("/api/sessions/{name}/connect")
|
||||||
@@ -584,9 +655,19 @@ async def get_settings() -> dict:
|
|||||||
|
|
||||||
@app.patch("/api/settings")
|
@app.patch("/api/settings")
|
||||||
async def update_settings(request: Request) -> dict:
|
async def update_settings(request: Request) -> dict:
|
||||||
"""Merge known keys from the request body into settings and return updated settings."""
|
"""Merge known keys from the request body into settings and return updated settings.
|
||||||
|
|
||||||
|
The response is redacted in the same way as ``GET /api/settings`` so that
|
||||||
|
sensitive keys are never leaked to the browser.
|
||||||
|
"""
|
||||||
body = await request.json()
|
body = await request.json()
|
||||||
return patch_settings(body)
|
updated = patch_settings(body)
|
||||||
|
result = copy.deepcopy(updated)
|
||||||
|
result["federation_key"] = ""
|
||||||
|
for inst in result.get("remote_instances", []):
|
||||||
|
if "key" in inst:
|
||||||
|
inst["key"] = ""
|
||||||
|
return result
|
||||||
|
|
||||||
|
|
||||||
@app.get("/api/instance-info")
|
@app.get("/api/instance-info")
|
||||||
@@ -785,7 +866,9 @@ async def federation_terminal_ws_proxy(websocket: WebSocket, remote_id: int) ->
|
|||||||
async with websockets.connect(
|
async with websockets.connect(
|
||||||
ws_url,
|
ws_url,
|
||||||
subprotocols=[Subprotocol("tty")],
|
subprotocols=[Subprotocol("tty")],
|
||||||
additional_headers={"Authorization": f"Bearer {remote_key}"} if remote_key else {},
|
additional_headers={"Authorization": f"Bearer {remote_key}"}
|
||||||
|
if remote_key
|
||||||
|
else {},
|
||||||
ssl=ssl_context,
|
ssl=ssl_context,
|
||||||
) as remote_ws:
|
) as remote_ws:
|
||||||
|
|
||||||
@@ -1065,7 +1148,7 @@ async def federation_connect(
|
|||||||
_log.warning("federation_connect: remote %s unreachable: %s", remote_url, exc)
|
_log.warning("federation_connect: remote %s unreachable: %s", remote_url, exc)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=503,
|
status_code=503,
|
||||||
detail=f"Remote unreachable: {remote_url}",
|
detail=f"Remote unreachable: {remote_url} ({type(exc).__name__}: {exc})",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -1113,7 +1196,7 @@ async def federation_bell_clear(
|
|||||||
)
|
)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=503,
|
status_code=503,
|
||||||
detail=f"Remote unreachable: {remote_url}",
|
detail=f"Remote unreachable: {remote_url} ({type(exc).__name__}: {exc})",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -1161,7 +1244,7 @@ async def federation_create_session(
|
|||||||
)
|
)
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=503,
|
status_code=503,
|
||||||
detail=f"Remote unreachable: {remote_url}",
|
detail=f"Remote unreachable: {remote_url} ({type(exc).__name__}: {exc})",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+1
-1
@@ -85,7 +85,7 @@ async def enumerate_sessions() -> list[str]:
|
|||||||
"""
|
"""
|
||||||
try:
|
try:
|
||||||
output = await run_tmux("list-sessions", "-F", "#{session_name}")
|
output = await run_tmux("list-sessions", "-F", "#{session_name}")
|
||||||
except RuntimeError:
|
except (RuntimeError, FileNotFoundError):
|
||||||
return []
|
return []
|
||||||
|
|
||||||
names = [line.strip() for line in output.splitlines() if line.strip()]
|
names = [line.strip() for line in output.splitlines() if line.strip()]
|
||||||
|
|||||||
+25
-9
@@ -83,13 +83,18 @@ def patch_settings(patch: dict) -> dict:
|
|||||||
"""
|
"""
|
||||||
current = load_settings()
|
current = load_settings()
|
||||||
|
|
||||||
# Snapshot existing remote keys by URL *before* applying the patch so we
|
# Snapshot existing remote keys by URL *and* by position *before* applying
|
||||||
# can restore them if the patch contains redacted (empty) key values.
|
# the patch so we can restore them if the patch contains redacted (empty)
|
||||||
existing_remote_keys: dict[str, str] = {
|
# key values. The URL-based lookup handles the common case (name-only edits).
|
||||||
r["url"]: r.get("key", "")
|
# The position-based fallback handles URL edits (e.g. http -> https) where
|
||||||
for r in current.get("remote_instances", [])
|
# the URL changes but the remote identity is the same.
|
||||||
if r.get("url")
|
existing_remotes = current.get("remote_instances", [])
|
||||||
|
existing_remote_keys_by_url: dict[str, str] = {
|
||||||
|
r["url"]: r.get("key", "") for r in existing_remotes if r.get("url")
|
||||||
}
|
}
|
||||||
|
existing_remote_keys_by_index: list[str] = [
|
||||||
|
r.get("key", "") for r in existing_remotes
|
||||||
|
]
|
||||||
|
|
||||||
for key in DEFAULT_SETTINGS:
|
for key in DEFAULT_SETTINGS:
|
||||||
if key in patch:
|
if key in patch:
|
||||||
@@ -97,10 +102,21 @@ def patch_settings(patch: dict) -> dict:
|
|||||||
|
|
||||||
# Restore keys that were stripped by redaction.
|
# Restore keys that were stripped by redaction.
|
||||||
if "remote_instances" in patch:
|
if "remote_instances" in patch:
|
||||||
for remote in current["remote_instances"]:
|
for i, remote in enumerate(current["remote_instances"]):
|
||||||
|
if remote.get("key"):
|
||||||
|
# Non-empty key in the patch = intentional key rotation, keep it.
|
||||||
|
continue
|
||||||
url = remote.get("url", "")
|
url = remote.get("url", "")
|
||||||
if not remote.get("key") and url in existing_remote_keys:
|
if url in existing_remote_keys_by_url:
|
||||||
remote["key"] = existing_remote_keys[url]
|
# URL unchanged -- restore by exact URL match.
|
||||||
|
remote["key"] = existing_remote_keys_by_url[url]
|
||||||
|
elif (
|
||||||
|
i < len(existing_remote_keys_by_index)
|
||||||
|
and existing_remote_keys_by_index[i]
|
||||||
|
):
|
||||||
|
# URL changed (e.g. http -> https) but position is the same --
|
||||||
|
# restore by index so editing a URL doesn't erase the key.
|
||||||
|
remote["key"] = existing_remote_keys_by_index[i]
|
||||||
|
|
||||||
save_settings(current)
|
save_settings(current)
|
||||||
return current
|
return current
|
||||||
|
|||||||
Reference in New Issue
Block a user