Skip to content

Commit ce430ec

Browse files
committed
fix(mcp-proxy): harden approval server threading
Use daemon request threads for the loopback approval server, validate Content-Length before reading POST bodies, and apply a per-request socket timeout for slow local clients. Document the one-proxy-per-IDE deployment pattern and add harness-level multi-instance tests for distinct approval ports, independent evidence stores, and decision isolation. This intentionally avoids single-proxy multi-IDE routing, which is out of v0.1 scope. Implemented with assistance from Codex.
1 parent 399333d commit ce430ec

4 files changed

Lines changed: 432 additions & 4 deletions

File tree

agentveil_mcp_proxy/approval/server.py

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
from urllib.parse import parse_qs
1616

1717

18+
MAX_POST_BODY_BYTES = 8192
19+
REQUEST_SOCKET_TIMEOUT_SECONDS = 5.0
1820
SECURITY_HEADERS = {
1921
"Referrer-Policy": "no-referrer",
2022
"Cache-Control": "no-store, no-cache, must-revalidate, max-age=0",
@@ -29,6 +31,10 @@
2931
COOKIE_NAME = "avp_approval_session"
3032

3133

34+
class _DaemonThreadingHTTPServer(ThreadingHTTPServer):
35+
daemon_threads = True
36+
37+
3238
class ApprovalServerError(RuntimeError):
3339
"""Raised when the local approval server cannot operate safely."""
3440

@@ -84,7 +90,7 @@ def __init__(self, *, host: str = "127.0.0.1", port: int = 0):
8490
self._decisions: dict[str, ApprovalServerDecision] = {}
8591
self._decision_events: dict[str, threading.Event] = {}
8692
self._terminal_requests: set[str] = set()
87-
self._httpd: ThreadingHTTPServer | None = None
93+
self._httpd: _DaemonThreadingHTTPServer | None = None
8894
self._thread: threading.Thread | None = None
8995

9096
@property
@@ -118,7 +124,7 @@ def start(self) -> None:
118124
class Handler(_ApprovalRequestHandler):
119125
server_owner = owner
120126

121-
self._httpd = ThreadingHTTPServer((self.host, self.port), Handler)
127+
self._httpd = _DaemonThreadingHTTPServer((self.host, self.port), Handler)
122128
self.host, self.port = self._httpd.server_address[:2]
123129
self._thread = threading.Thread(
124130
target=self._httpd.serve_forever,
@@ -244,6 +250,10 @@ def _valid_cookie(self, raw_cookie: str | None) -> bool:
244250
class _ApprovalRequestHandler(BaseHTTPRequestHandler):
245251
server_owner: ApprovalServer
246252

253+
def setup(self) -> None:
254+
super().setup()
255+
self.request.settimeout(REQUEST_SOCKET_TIMEOUT_SECONDS)
256+
247257
def log_message(self, _format: str, *_args: Any) -> None:
248258
return
249259

@@ -283,8 +293,16 @@ def do_POST(self) -> None:
283293
if not self.server_owner._valid_cookie(self.headers.get("Cookie")):
284294
self._send_text(HTTPStatus.FORBIDDEN, "forbidden")
285295
return
286-
length = int(self.headers.get("Content-Length", "0") or "0")
287-
body = self.rfile.read(min(length, 8192)).decode("utf-8", errors="replace")
296+
raw_length = self.headers.get("Content-Length", "0") or "0"
297+
try:
298+
length = int(raw_length)
299+
except ValueError:
300+
self._send_text(HTTPStatus.BAD_REQUEST, "invalid content length")
301+
return
302+
if length < 0 or length > MAX_POST_BODY_BYTES:
303+
self._send_text(HTTPStatus.BAD_REQUEST, "invalid content length")
304+
return
305+
body = self.rfile.read(length).decode("utf-8", errors="replace")
288306
form = parse_qs(body, keep_blank_values=True)
289307
csrf = (form.get("csrf_token") or [""])[0]
290308
if not hmac.compare_digest(csrf, prompt.csrf_token):

docs/MCP_PROXY_OPERATIONS.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,37 @@ The proxy writes the pending approval record before it renders the approval page
115115
or sends notifications. Approval, denial, and timeout decisions are written back
116116
to local evidence before the proxy acts on them.
117117

118+
## Multi-IDE Deployment
119+
120+
For developers running multiple LLM IDE clients, the canonical pattern is one
121+
MCP proxy per IDE.
122+
123+
Each `agentveil-mcp-proxy run` invocation creates an independent process with:
124+
125+
- A distinct approval server port, bound to `127.0.0.1` on a random unused port
126+
- An independent evidence database, scoped by `--home` or `AVP_HOME`
127+
- An independent Runtime Gate session, identity, and control grant
128+
- Independent circuit breaker state
129+
130+
This process-level isolation avoids shared approval state between proxy
131+
instances on the same host. Each IDE's MCP server configuration should point at
132+
its own proxy command.
133+
134+
Example: two IDEs on one machine
135+
136+
```bash
137+
AVP_HOME="$HOME/.avp-claude" agentveil-mcp-proxy run
138+
AVP_HOME="$HOME/.avp-cursor" agentveil-mcp-proxy run
139+
```
140+
141+
Each proxy has its own home tree containing its identity, control grant, and
142+
`evidence.sqlite`. Decisions in one proxy do not authorize or deny requests in
143+
the other.
144+
145+
For centralized policy enforcement across multiple developers, enforce policy
146+
in the AVP backend rather than multiplexing multiple IDE clients through one
147+
local proxy process.
148+
118149
## Headless Approval Mode
119150

120151
For CI or scheduled jobs, run with deny-by-default headless behavior:

tests/test_mcp_proxy_approval.py

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,17 @@
99
from pathlib import Path
1010
import re
1111
import signal
12+
import socket
1213
import sys
1314
import threading
1415
import time
1516
from typing import Any
17+
from urllib.parse import urlencode, urlsplit
1618

1719
import httpx
1820
import pytest
1921

22+
import agentveil_mcp_proxy.approval.server as approval_server_module
2023
import agentveil_mcp_proxy.cli as proxy_cli
2124
from agentveil_mcp_proxy.approval import (
2225
ApprovalFlowError,
@@ -182,6 +185,14 @@ def _get_csrf(client: httpx.Client, url: str) -> str:
182185
return match.group(1)
183186

184187

188+
def _get_csrf_and_cookie(client: httpx.Client, url: str) -> tuple[str, str]:
189+
response = client.get(url)
190+
assert response.status_code == 200
191+
match = TOKEN_RE.search(response.text)
192+
assert match
193+
return match.group(1), response.headers["Set-Cookie"].split(";", 1)[0]
194+
195+
185196
def _post_decision(client: httpx.Client, url: str, *, decision: str, csrf: str, scope: str = "exact"):
186197
return client.post(url, data={
187198
"decision": decision,
@@ -225,6 +236,48 @@ def _request_and_post(
225236
return result_box["outcome"], prompt, response
226237

227238

239+
def _raw_http_request(host: str, port: int, request: str, *, timeout: float = 2.0) -> bytes:
240+
with socket.create_connection((host, port), timeout=timeout) as sock:
241+
sock.settimeout(timeout)
242+
sock.sendall(request.encode("utf-8"))
243+
chunks = []
244+
while True:
245+
chunk = sock.recv(4096)
246+
if not chunk:
247+
break
248+
chunks.append(chunk)
249+
return b"".join(chunks)
250+
251+
252+
def _raw_post(
253+
server: ApprovalServer,
254+
url: str,
255+
*,
256+
content_length: str,
257+
body: str = "",
258+
cookie: str | None = None,
259+
) -> bytes:
260+
path = urlsplit(url).path
261+
headers = [
262+
f"POST {path} HTTP/1.1",
263+
f"Host: {server.host}:{server.port}",
264+
"Content-Type: application/x-www-form-urlencoded",
265+
f"Content-Length: {content_length}",
266+
"Connection: close",
267+
]
268+
if cookie is not None:
269+
headers.append(f"Cookie: {cookie}")
270+
request = "\r\n".join(headers) + "\r\n\r\n" + body
271+
return _raw_http_request(server.host, server.port, request)
272+
273+
274+
def _assert_status(raw_response: bytes, status_code: int) -> None:
275+
status_line = raw_response.split(b"\r\n", 1)[0].decode("ascii", errors="replace")
276+
assert status_line.startswith(f"HTTP/1.0 {status_code} ") or status_line.startswith(
277+
f"HTTP/1.1 {status_code} "
278+
), raw_response.decode("utf-8", errors="replace")
279+
280+
228281
def test_approval_server_binds_only_to_127_0_0_1():
229282
server = ApprovalServer()
230283
server.start()
@@ -238,6 +291,99 @@ def test_approval_server_binds_only_to_127_0_0_1():
238291
ApprovalServer(host="0.0.0.0")
239292

240293

294+
def test_approval_server_request_threads_are_daemon(monkeypatch):
295+
seen: dict[str, bool] = {}
296+
original_do_get = approval_server_module._ApprovalRequestHandler.do_GET
297+
298+
def recording_do_get(self):
299+
seen["daemon"] = threading.current_thread().daemon
300+
return original_do_get(self)
301+
302+
monkeypatch.setattr(approval_server_module._ApprovalRequestHandler, "do_GET", recording_do_get)
303+
server = ApprovalServer()
304+
server.start()
305+
try:
306+
assert server._httpd is not None
307+
assert server._httpd.daemon_threads is True
308+
url = server.register(_prompt())
309+
response = httpx.get(url)
310+
assert response.status_code == 200
311+
assert seen["daemon"] is True
312+
finally:
313+
server.stop()
314+
315+
316+
def _assert_invalid_content_length_rejected(content_length: str) -> None:
317+
server = ApprovalServer()
318+
server.start()
319+
try:
320+
url = server.register(_prompt())
321+
with httpx.Client() as client:
322+
_csrf, cookie = _get_csrf_and_cookie(client, url)
323+
response = _raw_post(server, url, content_length=content_length, cookie=cookie)
324+
_assert_status(response, 400)
325+
assert b"invalid content length" in response
326+
finally:
327+
server.stop()
328+
329+
330+
def test_post_with_non_numeric_content_length_returns_400():
331+
_assert_invalid_content_length_rejected("abc")
332+
333+
334+
def test_post_with_negative_content_length_returns_400():
335+
_assert_invalid_content_length_rejected("-100")
336+
337+
338+
def test_post_with_oversized_content_length_returns_400():
339+
_assert_invalid_content_length_rejected("99999")
340+
341+
342+
def test_post_with_valid_content_length_succeeds():
343+
server = ApprovalServer()
344+
server.start()
345+
try:
346+
url = server.register(_prompt())
347+
with httpx.Client() as client:
348+
csrf, cookie = _get_csrf_and_cookie(client, url)
349+
body = urlencode({
350+
"decision": "approve",
351+
"csrf_token": csrf,
352+
"approval_scope": "exact",
353+
})
354+
response = _raw_post(
355+
server,
356+
url,
357+
content_length=str(len(body.encode("utf-8"))),
358+
body=body,
359+
cookie=cookie,
360+
)
361+
_assert_status(response, 200)
362+
decision = server.wait_for_decision("req-1", timeout=0.1)
363+
assert decision is not None
364+
assert decision.decision == "approve"
365+
finally:
366+
server.stop()
367+
368+
369+
def test_slow_client_request_socket_timeout(monkeypatch):
370+
monkeypatch.setattr(approval_server_module, "REQUEST_SOCKET_TIMEOUT_SECONDS", 0.25)
371+
server = ApprovalServer()
372+
server.start()
373+
try:
374+
with socket.create_connection((server.host, server.port), timeout=2.0) as sock:
375+
sock.settimeout(2.0)
376+
sock.sendall(b"GET /approval/")
377+
time.sleep(0.6)
378+
try:
379+
data = sock.recv(1)
380+
except (ConnectionResetError, TimeoutError, socket.timeout):
381+
data = b""
382+
assert data == b""
383+
finally:
384+
server.stop()
385+
386+
241387
def test_post_without_token_returns_403():
242388
server = ApprovalServer()
243389
server.start()

0 commit comments

Comments
 (0)