Add multi-terminal support with dynamic TTYD management

[axisrow]

Summary

• Add TTYDManager class to dynamically spawn/kill TTYD processes with automatic port reuse
• New REST API endpoints: GET/POST /terminals, DELETE /terminals/<id> with CSRF protection
• Replace static "Terminal" button in dashboard with dynamic JS-powered terminal list
• Each terminal gets its own TTYD process + tmux session (/ttyd1, /ttyd2, etc.)
• Users can create terminals (opens in new tab) and delete them (× button with confirmation)
• Move TTYD lifecycle management from entrypoint.sh to ttyd_proxy.py
• Update CLAUDE.md with CI/CD, test structure, and HTML template documentation
• Fix outdated "No automated tests" in AGENTS.md

Test plan

• Run python -m pytest tests/ — all 93 tests pass
• Build Docker image and verify proxy starts with auto-created ttyd1
• Login and verify dashboard shows ttyd1 + "New terminal" button
• Click "+ New terminal" — creates ttyd2, opens in new tab
• Click × on ttyd1 — terminal deleted, removed from list
• Verify Tab completion works in terminal
• Verify GET /health returns terminal count
• Verify CSRF protection on POST/DELETE endpoints

🤖 Generated with Claude Code

[axisrow]

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

[claude]

Claude finished @axisrow's task in 2m 13s —— View job

---

Code Review

• Read changed files
• Analyze for bugs, security vulnerabilities, logical errors, data loss risks, performance problems
• Post review findings

---

Critical Issues

1. Zombie process leak in delete_terminal — app/ttyd_proxy.py:140-144

After process.wait(timeout=5) raises TimeoutExpired, the code calls process.kill() but never calls process.wait() afterward. A SIGKILL'd process that has no wait() call becomes a zombie and holds a slot in the process table indefinitely. Over time, with many terminal creates/deletes, this accumulates. Fix: call process.wait() after process.kill().

except (subprocess.TimeoutExpired, OSError):
    try:
        process.kill()
        process.wait()  # <-- missing: reap the zombie after kill
    except OSError:
        pass

Fix this →

---

2. Lock held during blocking subprocess calls — app/ttyd_proxy.py:185-187 and 199-202

Both list_terminals and get_terminal call _cleanup_dead() inside with self.lock:. _cleanup_dead runs subprocess.run(["runuser", ..., "tmux", "kill-session", ...], timeout=5) — a blocking call that can take up to 5 seconds. This means:

• Any dead terminal causes the lock to be held for up to 5 seconds.
• All concurrent operations (create_terminal, delete_terminal, other list_terminals calls) block for that entire time.
• list_terminals is called from the unauthenticated /health endpoint, so any client can trigger this stall without authentication.

Fix: collect dead terminals inside the lock, release the lock, then run cleanup outside it.

Fix this →

---

3. Race: terminal returned to client before TTYD is ready — app/ttyd_proxy.py:~660

handle_terminals_create calls ttyd_manager.create_terminal() with the default wait=False. The API immediately returns {"id": N} and the browser opens /ttydN. But TTYD hasn't bound to its port yet — the first WebSocket connection from the new tab will get a connection-refused error. There is no retry or readiness signal on the client side.

wait=True was intentionally left for startup only (to avoid blocking the server's request loop), but without it the UX is broken on first connect. Consider a lightweight HTTP readiness poll in the JS before opening the tab, or a server-side retry in the proxy handler.

---

4. No terminal ownership enforcement — app/ttyd_proxy.py:~672-680

DELETE /terminals/<id> only checks that the caller is authenticated — it does not verify ownership. Any authenticated user can delete any other user's terminal by guessing the sequential integer ID. In a multi-user deployment (multiple SSH/web users sharing one container) this is a real access control flaw: user A can kill user B's running session.

Fix this →

---

5. Session token embeds TTYD_BASE_PORT but port-matching check was removed — app/ttyd_proxy.py:~780

In the old code, handle_ttyd_proxy validated that the session token's embedded port matched TTYD_TTYD_PORT, which prevented replaying a stolen session against a different backend. That check is now gone. The session token still encodes a port (always TTYD_BASE_PORT), but the proxy now uses the URL path's terminal_id to look up the actual port — the token's port field is ignored entirely. This is not a new vulnerability per se, but it leaves the session token carrying a field that serves no security purpose, which is confusing and could be a latent bug if the port field is accidentally relied upon again in the future.

---

Minor Issues

• handle_health is unauthenticated and triggers dead-terminal cleanup (via list_terminals): as noted above, this is exploitable to cause stalls. Consider caching the terminal count or returning it without triggering cleanup from an unauthenticated path.
• CSRF token expiry (10 min default) vs. long-lived dashboard sessions: if a user keeps the dashboard open for >10 minutes, the CSRF token will expire and + New terminal / delete will silently fail (returns 403 JSON). The frontend doesn't surface this error — createTerminal and deleteTerminal only console.log failures.
• /health endpoint change is a breaking API change: the old response was {"status": "ok", "ttyd": "running"}. The new response has "ttyd": "no terminals" (not "running") when no terminals are active, and adds "terminals": N. Any monitoring scripts checking for "ttyd": "running" will break.

---

[axisrow]

Triage of review comments

Comment 1 — Zombie process leak in delete_terminal

Verdict: Partially valid. process.wait(timeout=5) exists at line 139 after terminate(). However, when TimeoutExpired occurs and process.kill() is called at line 142, there's no subsequent wait(). Will fix by adding process.wait() after kill().

Comment 2 — Lock held during blocking subprocess calls

Verdict: FIX. Confirmed: _cleanup_dead() runs subprocess.run(timeout=5) inside the lock in both list_terminals() and get_terminal(). Will move cleanup outside the lock.

Comment 3 — Terminal returned before TTYD is ready

Verdict: Intentional design decision. Non-blocking creation was chosen deliberately to avoid blocking the single-threaded HTTP server for up to 15 seconds. TTYD typically starts in 1-2 seconds. The iframe/WebSocket will reconnect automatically.

Comment 4 — No terminal ownership enforcement

Verdict: By design. User explicitly specified "Общие для всех пользователей" (shared terminals for all users). This is not a bug — it's a requirement.

Comment 5 — Session token port field

Verdict: Irrelevant. Reviewer noted this is "not a new vulnerability." Already discussed in previous review cycle. Retained for backward token format compatibility.

Minor: /health triggers cleanup under lock

Will fix as part of Comment 2 — moving cleanup outside the lock.

Minor: CSRF token expiry

Skip. CSRF token is re-issued on every dashboard page load. A user who leaves the page open >10min without action can simply refresh.

Minor: /health breaking change

Intentional. Multi-terminal support requires updated health response format.

[axisrow]

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

[claude]

Claude finished @axisrow's task in 3m 4s —— View job

---

Code Review

• Read changed files
• Analyze for bugs, security vulnerabilities, logical errors, data loss risks, performance problems
• Post review findings

---

Summary

Both critical fixes from the previous review were correctly applied. The remaining items are either acknowledged design decisions or minor issues. No new critical bugs were introduced.

---

Previous Critical Issues — Status

1. Zombie process leak ✅ Fixed — app/ttyd_proxy.py:140-144

process.wait(timeout=3) is now called after process.kill(). The zombie reap path is correct.

2. Lock held during blocking subprocess ✅ Fixed — app/ttyd_proxy.py:196-198 and 214-217

Both list_terminals and get_terminal now collect dead terminals inside the lock, remove them from the dict inside the lock, then call _cleanup_dead() after releasing the lock. The fix is structurally correct.

---

Remaining Issues

1. Unauthenticated /health still triggers dead-terminal cleanup subprocesses

handle_health calls list_terminals() which, when dead terminals exist, runs subprocess.run(["runuser", ..., "tmux", "kill-session", ...]) outside the lock. The subprocess call is now safe from the locking perspective, but it's still a blocking operation that any unauthenticated caller can trigger. The key mitigating factor: since dead terminals are removed from the dict inside the lock before cleanup runs, any subsequent /health call will find the terminal already gone and won't re-trigger cleanup. So the worst case is one subprocess call per dead terminal, not one per request. This is acceptable.

2. Silent CSRF expiry breaks terminal create/delete with no user feedback — app/index.html:164-165,177-178

When the CSRF token expires (default 10 min), POST /terminals and DELETE /terminals/<id> return 403 {"error": "Invalid CSRF token"}. The JS catch only calls console.log — the user sees the button re-enable with no indication of failure. The user's terminal create or delete silently does nothing.

.catch(function(err) {
    console.log('Failed to create terminal:', err);  // user sees nothing
})

This is exploitable to confuse users: a session can be active for >10 minutes without a page refresh, causing all mutation operations to silently fail. Consider surfacing the error in the UI (e.g., setting btn.textContent = 'Error — refresh page') or re-issuing the CSRF token via a lightweight endpoint.

Fix this →

3. loadTerminals() fetch errors are silent on the dashboard — app/index.html:147-149

If GET /terminals fails (session expired, network error), the terminal list simply stays empty with no message. Users may think they have no terminals when in fact the fetch failed.

4. No auto-recreation of the initial terminal if it dies

main() creates ttyd1 at startup via create_terminal(wait=True). If that terminal's process subsequently dies (crash, OOM), the dashboard will show an empty list with no terminals until the user manually clicks "+ New terminal". There is no watchdog or recreation logic. This is a silent operational gap: the container appears healthy (/health returns {"status": "ok", "ttyd": "no terminals"}), but the web terminal is effectively gone until user intervention.

---

Design Decisions Acknowledged (not flagged)

• Terminal returned before TTYD ready (wait=False on API create): Accepted. TTYD typically binds in <2s; a 502 on first load is recoverable.
• No terminal ownership enforcement: Confirmed intentional shared-terminal design.
• Session token port field unused: Retained for token format compatibility; acknowledged as dead field.

---

[axisrow]

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

[claude]

Claude finished @axisrow's task in 3m 0s —— View job

---

Code Review

• Read changed files
• Analyze for bugs, security vulnerabilities, logical errors, data loss risks, performance problems
• Post review findings

---

Previous Critical Issues — Status

1. Zombie process leak ✅ Fixed

process.wait(timeout=3) after process.kill() — confirmed correct.

2. Lock held during blocking subprocess ✅ Fixed

Dead-terminal cleanup now runs outside the lock in both list_terminals and get_terminal — confirmed correct.

---

Remaining Critical Issue

handleAuthError conflates CSRF expiry with session expiry — forced re-authentication — app/index.html:119-125

function handleAuthError(r) {
  if (r.status === 401 || r.status === 403) {
    window.location.href = '/login';
    return true;
  }
  return false;
}

_check_csrf returns 403 for CSRF failures ("Invalid CSRF token"). _check_auth also returns 403 for invalid/missing users. handleAuthError treats both the same: any 403 → redirect to /login.

Consequence: The default CSRF_TOKEN_TTL=600 (10 min) means any user who leaves the dashboard open for 10+ minutes and then tries to create or delete a terminal gets silently redirected to the login page — even though their session cookie is still valid (default SESSION_TIMEOUT=604800, 1 week). The login page (handle_login_page) does not detect the existing valid session and re-issue a CSRF token; the user must re-authenticate fully.

This is a regression from the cycle-2 state where the same scenario caused silent JS console failures (console.log only). Now it causes an unexpected logout.

The fix is to differentiate CSRF failures from auth failures. Two clean approaches:

Option A — Use a distinct HTTP status for CSRF errors on the server side (e.g. 422 or a custom 419), so handleAuthError only handles 401/403:

# In _check_csrf, return 419 instead of 403
self.send_json(419, {"error": "CSRF token expired — please refresh the page"})

function handleAuthError(r) {
  if (r.status === 419) {
    // CSRF expired, not a session issue — reload to get a fresh token
    window.location.reload();
    return true;
  }
  if (r.status === 401 || r.status === 403) {
    window.location.href = '/login';
    return true;
  }
  return false;
}

Option B — Keep 403 but parse the response body to distinguish:

.then(function(r) {
  if (r.status === 403) {
    return r.json().then(function(body) {
      if (body.error && body.error.includes('CSRF')) {
        window.location.reload();  // re-issue CSRF via fresh page load
      } else {
        window.location.href = '/login';
      }
    });
  }
  if (r.status === 401) { window.location.href = '/login'; return; }
  return r.json();
})

Fix this →

---

Minor Issues (no action required)

• deleteTerminal JS catch is silent (console.log only) — if the DELETE request fails for a non-auth reason (network error, 500), the user sees the delete button re-enable with no indication of failure. Low severity since loadTerminals() refreshes the list and the terminal reappears.

• No auto-recreation of ttyd1 if it dies — previously flagged in cycle 2. If the initial TTYD process crashes after startup, the dashboard shows an empty list. The container remains healthy (/health returns {"status": "ok", "ttyd": "no terminals"}). Still an operational gap — users may not realize the web terminal is gone until manually creating one.

---

Design Decisions Acknowledged

• wait=False on API-created terminals: accepted.
• No terminal ownership enforcement: confirmed intentional shared-terminal design.
• Session token port field unused: retained for token format compatibility.

[axisrow]

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

[claude]

Claude finished @axisrow's task in 2m 24s —— View job

---

Code Review

• Read changed files
• Analyze for bugs, security vulnerabilities, logical errors, data loss risks, performance problems
• Post review findings

---

Summary

The critical issue from cycle 3 (handleAuthError conflating CSRF expiry with session expiry) was correctly fixed. No new critical bugs were introduced.

---

Previous Critical Issues — Status

1. Zombie process leak ✅ Fixed

process.wait(timeout=3) after process.kill() — confirmed correct.

2. Lock held during blocking subprocess ✅ Fixed

Dead-terminal cleanup now runs outside the lock in both list_terminals and get_terminal — confirmed correct.

3. CSRF/auth error conflation ✅ Fixed — app/ttyd_proxy.py:662-672, app/index.html:119-129

_check_csrf now returns 419 for both missing and expired CSRF tokens. handleAuthError correctly handles 419 with window.location.reload() and reserves 401/403 for actual session failures. The fix is structurally correct.

---

Minor Observation (no action required)

Login CSRF failures still return 403, not 419 — app/ttyd_proxy.py:752-757

The login handler's manual CSRF check returns 403 {"error": "Invalid CSRF token"} rather than 419. This is inconsistent with _check_csrf() but not a bug: login.html displays data.error for any non-ok response, so users see the error message. The scenario (spending >10 min on the login page without submitting) is extremely rare. No action needed.

---

Previously Acknowledged Design Decisions — No Change

• wait=False on API-created terminals: accepted.
• No terminal ownership enforcement: confirmed intentional shared-terminal design.
• Silent deleteTerminal catch (console.log only for network errors): loadTerminals() refreshes the list regardless; terminal reappears if delete failed.
• No auto-recreation of ttyd1 if initial TTYD dies: container appears healthy but web terminal is gone until user manually creates one — acknowledged operational gap.

---

This PR is ready to merge.