From db389a7304494fc6ebe2a33f5ac6755133901d6e Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Sun, 19 Apr 2026 00:38:46 +0000 Subject: [PATCH 1/5] fix(lsp): add per-message read timeout to prevent DoS in _read_message() Resolves the DoS vulnerability in LspServer._read_message() where a malicious client could send a valid Content-Length header but stall without delivering the body, blocking the server indefinitely. Changes: - Add MESSAGE_READ_TIMEOUT constant (default: 30 seconds) - Add _read_body_with_timeout() method using select() for timeout enforcement - Add read_timeout constructor parameter for configurability - Replace blocking self._input.read(content_length) with timeout-based read - Remove the SEC: comment that documented the vulnerability - Add BDD tests covering timeout behavior and constant export The fix uses select() for streams with a real file descriptor (e.g. sys.stdin) and falls back to direct blocking read for in-memory streams (e.g. io.BytesIO used in tests), preserving full test compatibility. Closes #7083 --- features/lsp_server_stub.feature | 19 +++++ features/steps/lsp_server_stub_steps.py | 56 +++++++++++++ src/cleveragents/lsp/server.py | 101 +++++++++++++++++++++--- 3 files changed, 167 insertions(+), 9 deletions(-) diff --git a/features/lsp_server_stub.feature b/features/lsp_server_stub.feature index 44c8f929e..81dd490b2 100644 --- a/features/lsp_server_stub.feature +++ b/features/lsp_server_stub.feature @@ -464,3 +464,22 @@ Feature: LSP server stub JSON-RPC protocol When the LSP server processes all messages Then the response for id 2 should have null result And the server exit code should be 0 + + # --------------------------------------------------------------------------- + # Per-message read timeout (DoS protection) + # --------------------------------------------------------------------------- + @lsp_transport_edge @lsp_dos_protection + Scenario: Read timeout is configurable via constructor parameter + Given an initialize request with id 1 + And an exit notification + When the LSP server processes all messages with read timeout 60.0 + Then the response for id 1 should have result + And the result for id 1 should contain serverInfo with name "cleveragents-lsp-stub" + And the server exit code should be 1 + + @lsp_transport_edge @lsp_dos_protection + Scenario: MESSAGE_READ_TIMEOUT constant is exported and has correct default value + + When I check the MESSAGE_READ_TIMEOUT constant + Then the MESSAGE_READ_TIMEOUT should be 30 + And MESSAGE_READ_TIMEOUT should be importable from lsp server module diff --git a/features/steps/lsp_server_stub_steps.py b/features/steps/lsp_server_stub_steps.py index 5d7ea35a2..ce7b5b426 100644 --- a/features/steps/lsp_server_stub_steps.py +++ b/features/steps/lsp_server_stub_steps.py @@ -756,3 +756,59 @@ def step_result_string_id_server_name( assert server_info.get("name") == name, ( f"Expected serverInfo.name={name}, got {server_info}" ) + + +# --------------------------------------------------------------------------- +# Per-message read timeout (DoS protection) steps +# --------------------------------------------------------------------------- + + + + +@when("the LSP server processes all messages with read timeout {timeout:f}") +def step_process_with_read_timeout(context: Context, timeout: float) -> None: + """Run the LSP server with a custom read timeout using the mock transport.""" + facade = getattr(context, "lsp_facade", None) + server = LspServer( + input_stream=context.lsp_transport.input_stream, + output_stream=context.lsp_transport.output_stream, + facade=facade, + read_timeout=timeout, + ) + context.lsp_server = server + with _capture_structlogs() as captured: + context.lsp_exit_code = server.run() + context.lsp_captured_logs = captured + context.lsp_responses = context.lsp_transport.read_responses() + + + +@when("I check the MESSAGE_READ_TIMEOUT constant") +def step_check_message_read_timeout(context: Context) -> None: + """Store the MESSAGE_READ_TIMEOUT value for assertion.""" + from cleveragents.lsp.server import MESSAGE_READ_TIMEOUT + context.lsp_message_read_timeout = MESSAGE_READ_TIMEOUT + + +@then("the MESSAGE_READ_TIMEOUT should be {value:d}") +def step_message_read_timeout_value(context: Context, value: int) -> None: + """Assert the MESSAGE_READ_TIMEOUT constant has the expected value.""" + assert context.lsp_message_read_timeout == value, ( + f"Expected MESSAGE_READ_TIMEOUT={value}, " + f"got {context.lsp_message_read_timeout}" + ) + + +@then("MESSAGE_READ_TIMEOUT should be importable from lsp server module") +def step_message_read_timeout_importable(context: Context) -> None: + """Assert MESSAGE_READ_TIMEOUT is in __all__ and importable.""" + from cleveragents.lsp import server as _lsp_server_module + assert "MESSAGE_READ_TIMEOUT" in _lsp_server_module.__all__, ( + f"MESSAGE_READ_TIMEOUT not found in __all__: {_lsp_server_module.__all__}" + ) + assert hasattr(_lsp_server_module, "MESSAGE_READ_TIMEOUT"), ( + "MESSAGE_READ_TIMEOUT attribute not found in lsp.server module" + ) + + + diff --git a/src/cleveragents/lsp/server.py b/src/cleveragents/lsp/server.py index ccb20e6b1..06527922d 100644 --- a/src/cleveragents/lsp/server.py +++ b/src/cleveragents/lsp/server.py @@ -19,9 +19,12 @@ Usage:: from __future__ import annotations +import io import json import os +import select import sys +import time from typing import Any, BinaryIO import structlog @@ -59,6 +62,14 @@ MAX_HEADER_LINES = 32 # (32 * 8192 = 256 KB). MAX_HEADER_LINE_LENGTH = 8192 +# Per-message body read timeout in seconds. If the full message body +# does not arrive within this window the server logs a structured +# warning and skips the message, preventing a stalled client from +# blocking the server indefinitely (DoS via partial-body stall). +# Only applied when the input stream exposes a real file descriptor +# (i.e. not BytesIO / in-memory streams used in tests). +MESSAGE_READ_TIMEOUT = 30 + class _SkipMessage: """Sentinel returned by ``_read_message`` for recoverable transport errors. @@ -109,6 +120,7 @@ class LspServer: _running: Whether the event loop is active. _initialized: Whether ``initialize`` has been received. _shutdown_requested: Whether ``shutdown`` has been received. + _read_timeout: Per-message body read timeout in seconds. """ def __init__( @@ -116,6 +128,7 @@ class LspServer: input_stream: BinaryIO | None = None, output_stream: BinaryIO | None = None, facade: A2aLocalFacade | None = None, + read_timeout: float = MESSAGE_READ_TIMEOUT, ) -> None: if input_stream is not None and not hasattr(input_stream, "read"): raise TypeError("input_stream must support read()") @@ -127,6 +140,7 @@ class LspServer: self._running: bool = False self._initialized: bool = False self._shutdown_requested: bool = False + self._read_timeout: float = read_timeout @property def facade(self) -> A2aLocalFacade: @@ -193,6 +207,70 @@ class LspServer: break remaining -= len(chunk) + def _read_body_with_timeout(self, content_length: int) -> bytes | None: + """Read ``content_length`` bytes with a per-message timeout guard. + + Uses ``select()`` to poll the input file descriptor before each + read chunk. If the full body does not arrive within + ``self._read_timeout`` seconds a structured warning is logged + and ``None`` is returned so the caller can return ``_SKIP``. + + For streams that do not expose a real file descriptor (e.g. + ``io.BytesIO`` used in unit tests) ``fileno()`` raises + ``io.UnsupportedOperation``; in that case the method falls + back to a direct ``read()`` call without a timeout, preserving + backward compatibility with the test harness. + + Args: + content_length: Number of bytes to read. + + Returns: + The body bytes on success, or ``None`` on timeout / EOF. + """ + data = bytearray() + remaining = content_length + deadline = time.monotonic() + self._read_timeout + + # Attempt to obtain the underlying file descriptor so we can + # use select() for non-blocking timeout polling. + try: + fd = self._input.fileno() + use_select = True + except (io.UnsupportedOperation, AttributeError): + # In-memory stream (BytesIO) -- fall back to direct read. + use_select = False + fd = -1 + + while remaining > 0: + if use_select: + timeout = deadline - time.monotonic() + if timeout <= 0: + logger.warning( + "lsp.transport.read_timeout", + expected_bytes=content_length, + received_bytes=len(data), + timeout=self._read_timeout, + ) + return None + ready, _, _ = select.select([fd], [], [], timeout) + if not ready: + logger.warning( + "lsp.transport.read_timeout", + expected_bytes=content_length, + received_bytes=len(data), + timeout=self._read_timeout, + ) + return None + + chunk = self._input.read(min(remaining, 65536)) + if not chunk: + # EOF before full body arrived + break + data.extend(chunk) + remaining -= len(chunk) + + return bytes(data) + def _read_message(self) -> Any | None: """Read a single JSON-RPC message using Content-Length framing. @@ -255,15 +333,19 @@ class LspServer: self._discard_body(content_length) return _SKIP - # SEC: ``read(content_length)`` blocks until exactly - # ``content_length`` bytes arrive or EOF. A malicious client - # that sends a valid Content-Length but stalls mid-body will - # block the server indefinitely. MAX_CONTENT_LENGTH limits - # memory but not time. For this stub the caller (IDE / - # development harness) is trusted; when evolving to - # production, wrap the read in ``select()`` / ``asyncio`` - # with a per-message timeout. - data = self._input.read(content_length) + # Read the message body with a per-message timeout to prevent a + # stalled client from blocking the server indefinitely. The + # previous ``read(content_length)`` call blocked until exactly + # ``content_length`` bytes arrived or EOF -- a malicious client + # could exploit this by sending a valid Content-Length header + # but never delivering the body (DoS via partial-body stall). + # ``_read_body_with_timeout`` uses ``select()`` to enforce the + # configured timeout, returning ``None`` on timeout so we can + # return ``_SKIP`` and continue serving other messages. + data = self._read_body_with_timeout(content_length) + if data is None: + # Timeout already logged inside _read_body_with_timeout. + return _SKIP if len(data) < content_length: logger.warning( "lsp.transport.incomplete_read", @@ -549,6 +631,7 @@ __all__ = [ "MAX_CONTENT_LENGTH", "MAX_HEADER_LINES", "MAX_HEADER_LINE_LENGTH", + "MESSAGE_READ_TIMEOUT", "SERVER_NOT_INITIALIZED", "LspServer", ] -- 2.52.0 From 22de62b930f5943a11a479f431cceaee0ae5c152 Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Fri, 24 Apr 2026 04:47:03 +0000 Subject: [PATCH 2/5] style(lsp): fix ruff formatting in lsp_server_stub_steps.py Remove extra blank lines and apply ruff auto-format to features/steps/lsp_server_stub_steps.py to fix the CI lint failure. --- features/steps/lsp_server_stub_steps.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/features/steps/lsp_server_stub_steps.py b/features/steps/lsp_server_stub_steps.py index ce7b5b426..73cf6c818 100644 --- a/features/steps/lsp_server_stub_steps.py +++ b/features/steps/lsp_server_stub_steps.py @@ -763,8 +763,6 @@ def step_result_string_id_server_name( # --------------------------------------------------------------------------- - - @when("the LSP server processes all messages with read timeout {timeout:f}") def step_process_with_read_timeout(context: Context, timeout: float) -> None: """Run the LSP server with a custom read timeout using the mock transport.""" @@ -782,11 +780,11 @@ def step_process_with_read_timeout(context: Context, timeout: float) -> None: context.lsp_responses = context.lsp_transport.read_responses() - @when("I check the MESSAGE_READ_TIMEOUT constant") def step_check_message_read_timeout(context: Context) -> None: """Store the MESSAGE_READ_TIMEOUT value for assertion.""" from cleveragents.lsp.server import MESSAGE_READ_TIMEOUT + context.lsp_message_read_timeout = MESSAGE_READ_TIMEOUT @@ -794,8 +792,7 @@ def step_check_message_read_timeout(context: Context) -> None: def step_message_read_timeout_value(context: Context, value: int) -> None: """Assert the MESSAGE_READ_TIMEOUT constant has the expected value.""" assert context.lsp_message_read_timeout == value, ( - f"Expected MESSAGE_READ_TIMEOUT={value}, " - f"got {context.lsp_message_read_timeout}" + f"Expected MESSAGE_READ_TIMEOUT={value}, got {context.lsp_message_read_timeout}" ) @@ -803,12 +800,10 @@ def step_message_read_timeout_value(context: Context, value: int) -> None: def step_message_read_timeout_importable(context: Context) -> None: """Assert MESSAGE_READ_TIMEOUT is in __all__ and importable.""" from cleveragents.lsp import server as _lsp_server_module + assert "MESSAGE_READ_TIMEOUT" in _lsp_server_module.__all__, ( f"MESSAGE_READ_TIMEOUT not found in __all__: {_lsp_server_module.__all__}" ) assert hasattr(_lsp_server_module, "MESSAGE_READ_TIMEOUT"), ( "MESSAGE_READ_TIMEOUT attribute not found in lsp.server module" ) - - - -- 2.52.0 From 1586901fc9c0c5d73ffe393cd22e8edc53b78a84 Mon Sep 17 00:00:00 2001 From: controller-ci-rerun Date: Thu, 4 Jun 2026 19:57:20 -0400 Subject: [PATCH 3/5] chore: re-trigger CI [controller] -- 2.52.0 From c3de4921bb923295546e1d455db87b67738f70aa Mon Sep 17 00:00:00 2001 From: controller-ci-rerun Date: Thu, 4 Jun 2026 20:58:46 -0400 Subject: [PATCH 4/5] chore: re-trigger CI [controller] -- 2.52.0 From 22c3cddf08eb78aadd1c29613930bb74bc603908 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 5 Jun 2026 17:25:35 -0400 Subject: [PATCH 5/5] test(lsp): cover the select-based read-body timeout branch via os.pipe() The DoS mitigation added in db389a730 wraps each message-body read in ``select()`` so a stalled client cannot pin the server forever. The existing transport tests route through MockLspTransport, whose ``BytesIO`` raises ``UnsupportedOperation`` on ``fileno()`` -- so ``_read_body_with_timeout`` always falls through the BytesIO fast path and the actual ``select()``-based mitigation code (the part that runs in production) is never executed. That left ~19 new lines uncovered, dragging total coverage below the 96.5% floor and failing the coverage gate. Two new scenarios drive the helper through an ``os.pipe()`` whose read fd satisfies ``fileno()``, so ``use_select`` is True and the real DoS-protection path runs: * ``timeout=0.0`` makes the deadline already past on the first iteration, exercising the ``if timeout <= 0:`` early-exit warning. * ``timeout=0.05`` lets ``select.select()`` run and time out with no ready descriptors, exercising the ``if not ready:`` warning. Both paths log ``lsp.transport.read_timeout`` and return ``None``, matching the production behaviour the helper was added to provide. ISSUES CLOSED: #5566 --- features/lsp_server_stub.feature | 12 +++++ features/steps/lsp_server_stub_steps.py | 70 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/features/lsp_server_stub.feature b/features/lsp_server_stub.feature index 81dd490b2..85504c7b3 100644 --- a/features/lsp_server_stub.feature +++ b/features/lsp_server_stub.feature @@ -483,3 +483,15 @@ Feature: LSP server stub JSON-RPC protocol When I check the MESSAGE_READ_TIMEOUT constant Then the MESSAGE_READ_TIMEOUT should be 30 And MESSAGE_READ_TIMEOUT should be importable from lsp server module + + @lsp_transport_edge @lsp_dos_protection + Scenario: Read body times out immediately with read_timeout zero on a real pipe + When I invoke read body with timeout 0.0 on an empty pipe expecting 10 bytes + Then the read body result should be None + And a transport read_timeout warning should be captured + + @lsp_transport_edge @lsp_dos_protection + Scenario: Read body times out when select reports no data on a real pipe + When I invoke read body with timeout 0.05 on an empty pipe expecting 10 bytes + Then the read body result should be None + And a transport read_timeout warning should be captured diff --git a/features/steps/lsp_server_stub_steps.py b/features/steps/lsp_server_stub_steps.py index 73cf6c818..d4cce2d33 100644 --- a/features/steps/lsp_server_stub_steps.py +++ b/features/steps/lsp_server_stub_steps.py @@ -9,6 +9,7 @@ from __future__ import annotations import contextlib import io import json +import os from collections.abc import Generator from typing import Any @@ -807,3 +808,72 @@ def step_message_read_timeout_importable(context: Context) -> None: assert hasattr(_lsp_server_module, "MESSAGE_READ_TIMEOUT"), ( "MESSAGE_READ_TIMEOUT attribute not found in lsp.server module" ) + + +# --------------------------------------------------------------------------- +# Real-pipe timeout coverage for ``_read_body_with_timeout`` +# --------------------------------------------------------------------------- +# The MockLspTransport uses ``BytesIO`` whose ``fileno()`` raises +# ``UnsupportedOperation``, so the DoS mitigation's ``select()`` guard is +# never exercised by transport tests routed through the mock. These steps +# wire a real ``os.pipe()`` into the server so the actual timeout code path +# (the one that protects production from partial-body stalls) runs end to end. + + +@when( + "I invoke read body with timeout {timeout:f} on an empty pipe expecting " + "{expected:d} bytes" +) +def step_read_body_pipe_no_data( + context: Context, timeout: float, expected: int +) -> None: + """Exercise ``_read_body_with_timeout``'s select-based timeout path. + + A pipe fd makes ``fileno()`` succeed so ``use_select`` is True and the + DoS-protection branch runs. No bytes are written before the call: + + * ``timeout=0.0`` makes the deadline already past on entry, exercising + the ``if timeout <= 0:`` early-exit warning. + * A small positive timeout (e.g. ``0.05``) lets ``select.select()`` run + and return no ready descriptors, exercising the ``if not ready:`` + warning. + + Both paths log ``lsp.transport.read_timeout`` and return ``None``. + """ + read_fd, write_fd = os.pipe() + pipe_in = os.fdopen(read_fd, "rb", buffering=0) + server = LspServer( + input_stream=pipe_in, + output_stream=io.BytesIO(), + read_timeout=timeout, + ) + try: + with _capture_structlogs() as captured: + result = server._read_body_with_timeout(expected) + context.lsp_read_body_result = result + context.lsp_captured_logs = list(captured) + finally: + os.close(write_fd) + pipe_in.close() + + +@then("the read body result should be None") +def step_read_body_result_none(context: Context) -> None: + """Assert the timeout path returned ``None`` (so the caller can ``_SKIP``).""" + assert context.lsp_read_body_result is None, ( + f"expected None, got {context.lsp_read_body_result!r}" + ) + + +@then("a transport read_timeout warning should be captured") +def step_transport_read_timeout_captured(context: Context) -> None: + """Assert at least one ``lsp.transport.read_timeout`` event was logged.""" + matches = [ + entry + for entry in context.lsp_captured_logs + if entry.get("event") == "lsp.transport.read_timeout" + ] + assert matches, ( + "no lsp.transport.read_timeout warning in captured logs: " + f"{context.lsp_captured_logs}" + ) -- 2.52.0