Fix critical bugs in ttyd_proxy.py

[axisrow]

Summary

• RateLimiter memory leak (CRITICAL): attempts dict keys (IP addresses) were never purged, causing unbounded memory growth. Added periodic cleanup of stale keys every 100 calls.
• UnicodeDecodeError crash (HIGH): Invalid UTF-8 in POST body crashed the login handler without responding. Now catches the exception and returns 400.
• Info disclosure in 502 responses (MEDIUM): str(exc) leaked internal details (IPs, ports, OS errors) to clients. Removed detail field, errors logged to stderr instead.

Test plan

• All 93 existing tests pass
• Manual: send POST with invalid UTF-8 body to /login, verify 400 response
• Manual: simulate TTYD down, verify 502 response has no detail field
• Manual: long-running instance with many unique IPs — verify memory stays bounded

🤖 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 encountered an error —— View job

---

I'll analyze this and get back to you.

[axisrow]

@claude review this PR. Focus only on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems.

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[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 1m 49s —— View job

---

PR Review

• Gather context
• Read changed file
• Analyze changes
• Post review

---

Summary

All three fixes are correct. No critical issues introduced by this PR.

---

Fix 1: RateLimiter memory leak (app/ttyd_proxy.py:78-87)

Correct. The cleanup logic is sound:

• Runs under the existing lock — thread-safe.
• all(current_time - t >= self.window_seconds for t in timestamps) correctly identifies keys whose entire history is outside the rate window. Empty-list case (all() on [] → True) is not a problem: after every successful is_allowed() call, the key gets a timestamp appended, so keys never remain empty between calls.
• Cleanup runs before per-key filtering, which is the right order (avoids a TOCTOU gap).

No issues.

---

Fix 2: UnicodeDecodeError crash (app/ttyd_proxy.py:453-457)

Correct. The exception is caught in the right place, the client gets a proper 400, and the handler returns cleanly. No issues.

---

Fix 3: Info disclosure in 502 responses (app/ttyd_proxy.py:865-867, 1028-1030)

Correct. Internal error details are removed from client responses and redirected to stderr.

One pre-existing bug worth noting (not introduced by this PR): the second except OSError block at line 1028-1030 wraps code that includes send_response(), send_header(), and end_headers(). If an OSError occurs after those calls have already written partial response headers to the socket (e.g., during wfile.write(data)), the subsequent send_json(502, ...) call will attempt to write a second HTTP response on an already-started stream, corrupting it. The send_json call will silently fail or produce garbage. Since this is pre-existing and outside the scope of this PR, it's not a blocker — but worth a follow-up fix. Fix this →

---

Verdict: LGTM. All three fixes are correct and well-scoped. The pre-existing double-response issue on late OSError is the only thing to watch, but it's not introduced here.

[axisrow]

@claude review this PR. Focus only on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems.

[claude]

Claude finished @axisrow's task in 1m 15s —— View job

---

PR Review

• Gather context
• Read changed file
• Analyze changes
• Post review

---

Summary

All three fixes are correct. No critical issues introduced by this PR.

---

Fix 1: RateLimiter memory leak (app/ttyd_proxy.py:78-87)

Correct. The cleanup logic is sound:

• Runs under the existing lock — thread-safe.
• all(current_time - t >= self.window_seconds for t in timestamps) correctly identifies keys whose entire history is outside the rate window. The empty-list edge case (all() on [] → True) is harmless: keys with empty timestamp lists would be deleted, but defaultdict(list) re-creates them on next access anyway, and the new attempt is then appended.
• Cleanup runs before per-key filtering, so if the current request's key is stale and gets deleted, defaultdict recreates it as empty — the request is correctly allowed and the new timestamp recorded.

No issues.

---

Fix 2: UnicodeDecodeError crash (app/ttyd_proxy.py:456-460)

Correct. Exception caught in the right place, client gets a proper 400, handler returns cleanly. No issues.

---

Fix 3: Info disclosure in 502 responses (app/ttyd_proxy.py:865-867, 1028-1030)

Correct. Internal details removed from client responses and redirected to stderr.

One pre-existing bug (not introduced by this PR): the second except OSError block at line 1028-1030 wraps code that includes send_response(), send_header(), and end_headers(). If an OSError fires during self.wfile.write(data) at line 1027 — i.e., after headers are already written — the subsequent send_json(502, ...) call will attempt to write a second HTTP response on an already-started stream, corrupting it. send_json will silently fail or produce garbage. Pre-existing and out of scope here, but worth a follow-up. Fix this →

---

Verdict: LGTM. All three fixes are correct and well-scoped.