Codex/fix issues from codex review

clawteam/board/server.py

@@ -4,6 +4,7 @@
 
 import ipaddress
 import json
+import socket
 import threading
 import time
 import urllib.error
@@ -16,81 +17,9 @@
 from clawteam.board.collector import BoardCollector
 
 _STATIC_DIR = Path(__file__).parent / "static"
-_ALLOWED_PROXY_HOSTS = {
-    "api.github.com",
-    "github.com",
-    "raw.githubusercontent.com",
-}
-
-
-class _NoRedirectHandler(urllib.request.HTTPRedirectHandler):
-    """Reject redirects for proxied fetches."""
-
-    def redirect_request(self, req, fp, code, msg, headers, newurl):
-        raise urllib.error.HTTPError(newurl, code, msg, headers, fp)
-
-
-def _is_blocked_hostname(hostname: str) -> bool:
-    host = hostname.strip().lower()
-    if host in {"localhost"}:
-        return True
-    try:
-        ip = ipaddress.ip_address(host)
-    except ValueError:
-        return False
-    return (
-        ip.is_loopback
-        or ip.is_private
-        or ip.is_link_local
-        or ip.is_multicast
-        or ip.is_reserved
-    )
-
-
-def _normalize_proxy_target(target_url: str) -> str:
-    parsed = urlparse(target_url)
-    if parsed.scheme != "https":
-        raise ValueError("Proxy only allows https URLs")
-
-    hostname = (parsed.hostname or "").lower()
-    if not hostname:
-        raise ValueError("Proxy URL must include a hostname")
-    if _is_blocked_hostname(hostname):
-        raise ValueError("Proxy target is not allowed")
-
-    if hostname == "github.com":
-        parts = [p for p in parsed.path.split("/") if p]
-        if len(parts) == 2:
-            return f"https://api.github.com/repos/{parts[0]}/{parts[1]}/readme"
-        return target_url.replace("github.com", "raw.githubusercontent.com").replace("/blob/", "/")
-
-    if hostname not in _ALLOWED_PROXY_HOSTS:
-        raise ValueError("Proxy only allows GitHub-hosted content")
-
-    return target_url
-
-
-def _fetch_proxy_content(target_url: str) -> bytes:
-    normalized = _normalize_proxy_target(target_url)
-    opener = urllib.request.build_opener(_NoRedirectHandler)
-    req = urllib.request.Request(normalized, headers={"User-Agent": "ClawTeam-Server"})
-    with opener.open(req, timeout=10) as resp:
-        final_url = resp.geturl()
-        _normalize_proxy_target(final_url)
-        body = resp.read()
-
-    if normalized.startswith("https://api.github.com/repos/") and final_url == normalized:
-        payload = json.loads(body.decode("utf-8"))
-        download_url = payload.get("download_url")
-        if not download_url:
-            raise ValueError("GitHub README proxy target has no downloadable content")
-        normalized = _normalize_proxy_target(download_url)
-        req = urllib.request.Request(normalized, headers={"User-Agent": "ClawTeam-Server"})
-        with opener.open(req, timeout=10) as resp:
-            _normalize_proxy_target(resp.geturl())
-            return resp.read()
-
-    return body
+_PROXY_TIMEOUT_SECONDS = 10
+_PROXY_MAX_BYTES = 2 * 1024 * 1024
+_PROXY_CHUNK_SIZE = 64 * 1024
 
 
 @dataclass
@@ -124,6 +53,9 @@ class BoardHandler(BaseHTTPRequestHandler):
     default_team: str = ""
     interval: float = 2.0
     team_cache: TeamSnapshotCache
+    proxy_timeout_seconds: float = _PROXY_TIMEOUT_SECONDS
+    proxy_max_bytes: int = _PROXY_MAX_BYTES
+    proxy_chunk_size: int = _PROXY_CHUNK_SIZE
 
     def do_GET(self):
         path = self.path.split("?")[0]
@@ -150,16 +82,7 @@ def do_GET(self):
             if not target_url:
                 self.send_error(400, "URL required")
                 return
-            try:
-                content = _fetch_proxy_content(target_url)
-                self.send_response(200)
-                self.send_header("Content-Type", "text/plain; charset=utf-8")
-                self.end_headers()
-                self.wfile.write(content)
-            except ValueError as e:
-                self.send_error(403, str(e))
-            except Exception as e:
-                self.send_error(500, str(e))
+            self._serve_proxy(target_url)
         else:
             self.send_error(404)
 
@@ -242,6 +165,82 @@ def _serve_sse(self, team_name: str):
         except (BrokenPipeError, ConnectionResetError, OSError):
             pass
 
+    def _resolve_proxy_url(self, target_url: str) -> str:
+        # If github URL, convert to api.github.com/repos/.../readme
+        if "github.com" in target_url and "raw.githubusercontent.com" not in target_url:
+            parsed = urlparse(target_url)
+            parts = [p for p in parsed.path.split("/") if p]
+            if len(parts) == 2:
+                api_url = f"https://api.github.com/repos/{parts[0]}/{parts[1]}/readme"
+                req = urllib.request.Request(api_url, headers={"User-Agent": "ClawTeam-Server"})
+                with urllib.request.urlopen(req, timeout=self.proxy_timeout_seconds) as resp:
+                    data = json.loads(resp.read().decode())
+                    return data.get("download_url", target_url)
+
+            return target_url.replace("github.com", "raw.githubusercontent.com").replace("/blob/", "/")
+
+        return target_url
+
+    def _serve_proxy(self, target_url: str):
+        try:
+            resolved_url = self._resolve_proxy_url(target_url)
+            req = urllib.request.Request(resolved_url, headers={"User-Agent": "ClawTeam-Server"})
+            with urllib.request.urlopen(req, timeout=self.proxy_timeout_seconds) as resp:
+                content_length_header = resp.headers.get("Content-Length")
+                if content_length_header is not None:
+                    try:
+                        content_length = int(content_length_header)
+                    except ValueError:
+                        content_length = None
+                    if content_length is not None and content_length > self.proxy_max_bytes:
+                        self.send_error(413, "Response too large")
+                        return
+                else:
+                    content_length = None
+
+                if content_length is None:
+                    buffered = bytearray()
+                    while True:
+                        chunk = resp.read(self.proxy_chunk_size)
+                        if not chunk:
+                            break
+                        buffered.extend(chunk)
+                        if len(buffered) > self.proxy_max_bytes:
+                            self.send_error(413, "Response too large")
+                            return
+
+                    self.send_response(200)
+                    self.send_header("Content-Type", "text/plain; charset=utf-8")
+                    self.send_header("Access-Control-Allow-Origin", "*")
+                    self.send_header("Content-Length", str(len(buffered)))
+                    self.end_headers()
+                    self.wfile.write(buffered)
+                    return
+
+                self.send_response(200)
+                self.send_header("Content-Type", "text/plain; charset=utf-8")
+                self.send_header("Access-Control-Allow-Origin", "*")
+                if content_length is not None:
+                    self.send_header("Content-Length", str(content_length))
+                self.end_headers()
+
+                while True:
+                    chunk = resp.read(self.proxy_chunk_size)
+                    if not chunk:
+                        break
+                    self.wfile.write(chunk)
+        except TimeoutError:
+            self.send_error(504, "Proxy request timed out")
+        except socket.timeout:
+            self.send_error(504, "Proxy request timed out")
+        except urllib.error.URLError as e:
+            if isinstance(e.reason, (TimeoutError, socket.timeout)):
+                self.send_error(504, "Proxy request timed out")
+            else:
+                self.send_error(502, str(e))
+        except Exception as e:
+            self.send_error(500, str(e))
+
     def log_message(self, format, *args):
         # Suppress default stderr logging for SSE connections
         first = str(args[0]) if args else ""

tests/test_board.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import io
+import socket
 from pathlib import Path
 
 import pytest
@@ -286,54 +287,138 @@ def get(self, team_name, loader):
     assert calls["count"] == 1
 
 
-def test_proxy_rejects_non_github_targets():
-    with pytest.raises(ValueError, match="GitHub-hosted"):
-        _normalize_proxy_target("https://example.com/secret")
+def test_serve_proxy_returns_504_on_timeout(monkeypatch):
+    handler = object.__new__(BoardHandler)
+    handler.proxy_timeout_seconds = 0.01
+    handler.proxy_max_bytes = 2 * 1024 * 1024
+    handler.proxy_chunk_size = 1024
+    handler.wfile = io.BytesIO()
 
+    captured = {}
+    handler.send_error = lambda code, msg=None: captured.setdefault("error", (code, msg))
+    handler.send_response = lambda code: captured.setdefault("status", code)
+    handler.send_header = lambda name, value: None
+    handler.end_headers = lambda: None
 
-def test_proxy_rejects_localhost_targets():
-    with pytest.raises(ValueError, match="not allowed"):
-        _normalize_proxy_target("https://127.0.0.1/admin")
+    def fake_urlopen(req, timeout=None):
+        raise socket.timeout("slow upstream")
 
+    monkeypatch.setattr("clawteam.board.server.urllib.request.urlopen", fake_urlopen)
 
-def test_proxy_fetches_allowed_github_content(monkeypatch):
-    seen = {}
+    handler._serve_proxy("https://example.com/slow.txt")
+
+    assert captured["error"] == (504, "Proxy request timed out")
+
+
+def test_serve_proxy_returns_413_for_oversized_content_length(monkeypatch):
+    handler = object.__new__(BoardHandler)
+    handler.proxy_timeout_seconds = 1
+    handler.proxy_max_bytes = 1024
+    handler.proxy_chunk_size = 256
+    handler.wfile = io.BytesIO()
+
+    captured = {}
+    handler.send_error = lambda code, msg=None: captured.setdefault("error", (code, msg))
+    handler.send_response = lambda code: captured.setdefault("status", code)
+    handler.send_header = lambda name, value: None
+    handler.end_headers = lambda: None
 
     class FakeResponse:
-        def __init__(self, url: str, payload: bytes):
-            self._url = url
-            self._payload = payload
+        def __init__(self):
+            self.headers = {"Content-Length": "2048"}
+
+        def __enter__(self):
+            return self
 
-        def geturl(self):
-            return self._url
+        def __exit__(self, exc_type, exc, tb):
+            return False
 
-        def read(self):
-            return self._payload
+        def read(self, size=-1):
+            return b""
+
+    monkeypatch.setattr(
+        "clawteam.board.server.urllib.request.urlopen",
+        lambda req, timeout=None: FakeResponse(),
+    )
+
+    handler._serve_proxy("https://example.com/large.txt")
+
+    assert captured["error"] == (413, "Response too large")
+
+
+def test_serve_proxy_streams_chunks_without_content_length(monkeypatch):
+    handler = object.__new__(BoardHandler)
+    handler.proxy_timeout_seconds = 1
+    handler.proxy_max_bytes = 4096
+    handler.proxy_chunk_size = 4
+    handler.wfile = io.BytesIO()
+
+    headers = []
+    status = {}
+    handler.send_error = lambda code, msg=None: (_ for _ in ()).throw(AssertionError((code, msg)))
+    handler.send_response = lambda code: status.setdefault("code", code)
+    handler.send_header = lambda name, value: headers.append((name, value))
+    handler.end_headers = lambda: None
+
+    class FakeResponse:
+        def __init__(self):
+            self.headers = {}
+            self._chunks = [b"abcd", b"ef", b""]
 
         def __enter__(self):
             return self
 
         def __exit__(self, exc_type, exc, tb):
             return False
 
-    class FakeOpener:
-        def open(self, req, timeout=10):
-            seen["url"] = req.full_url
-            return FakeResponse(req.full_url, b"ok")
+        def read(self, size=-1):
+            return self._chunks.pop(0)
+
+    monkeypatch.setattr(
+        "clawteam.board.server.urllib.request.urlopen",
+        lambda req, timeout=None: FakeResponse(),
+    )
 
-    monkeypatch.setattr("clawteam.board.server.urllib.request.build_opener", lambda *_: FakeOpener())
+    handler._serve_proxy("https://example.com/chunked.txt")
 
-    assert _fetch_proxy_content("https://raw.githubusercontent.com/org/repo/main/README.md") == b"ok"
-    assert seen["url"] == "https://raw.githubusercontent.com/org/repo/main/README.md"
+    assert status["code"] == 200
+    assert ("Content-Length", "6") in headers
+    assert handler.wfile.getvalue() == b"abcdef"
 
 
-def test_board_ui_escapes_attacker_controlled_fields():
-    html = Path("clawteam/board/static/index.html").read_text(encoding="utf-8")
+def test_serve_proxy_returns_413_for_oversized_chunked_response(monkeypatch):
+    handler = object.__new__(BoardHandler)
+    handler.proxy_timeout_seconds = 1
+    handler.proxy_max_bytes = 5
+    handler.proxy_chunk_size = 4
+    handler.wfile = io.BytesIO()
+
+    captured = {"statuses": []}
+    handler.send_error = lambda code, msg=None: captured.setdefault("error", (code, msg))
+    handler.send_response = lambda code: captured["statuses"].append(code)
+    handler.send_header = lambda name, value: None
+    handler.end_headers = lambda: None
+
+    class FakeResponse:
+        def __init__(self):
+            self.headers = {}
+            self._chunks = [b"abcd", b"ef", b""]
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, exc_type, exc, tb):
+            return False
+
+        def read(self, size=-1):
+            return self._chunks.pop(0)
+
+    monkeypatch.setattr(
+        "clawteam.board.server.urllib.request.urlopen",
+        lambda req, timeout=None: FakeResponse(),
+    )
+
+    handler._serve_proxy("https://example.com/chunked-large.txt")
 
-    assert "escapeHtml(m.name)" in html
-    assert "escapeHtml(m.agentType || 'Agent')" in html
-    assert "escapeHtml(m.fromLabel || m.from || 'SYS')" in html
-    assert "escapeHtml(m.toLabel || m.to || 'ALL')" in html
-    assert "escapeHtml(t.owner || 'Unassigned')" in html
-    assert "t.blockedBy.map(v => escapeHtml(v)).join(', ')" in html
-    assert "option.textContent =" in html
+    assert captured["error"] == (413, "Response too large")
+    assert captured["statuses"] == []