diff --git a/CHANGELOG.md b/CHANGELOG.md index 060d65c82..7de4dc388 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -466,6 +466,11 @@ ensuring data is stored with proper parameter values. - **Timeline dashboard updated with 2026-04-18 progress snapshot** (#10288): Added schedule adherence tables and daily milestone snapshot for April 18, covering M3-M10 status including overdue milestones (M3-M7) and in-progress milestones (M8-M10). Risk assessment shows M6 (v3.5.0) highest risk with 1,130 open issues at 18.1% completion. +- **LSP transport header injection fix** (#10608 / #7112): The `_read_one_message()` method in + `src/cleveragents/lsp/transport.py` now uses `errors="strict"` instead of `errors="replace"` for + ASCII decoding of LSP headers, preventing header injection attacks. Non-ASCII bytes raise + `LspError`. A printable-ASCII guard rejects characters outside 0x20-0x7E range. Epic #824. + ### Fixed - **Concurrent ValidationPipeline stdout/stderr restoration** (#7623): Fixed a race diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 99ce630e9..2e711700a 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -13,7 +13,6 @@ * Luis Mendes * Rui Hu * HAL 9000 has contributed the parallel subplan execution scheduler (#9555): implemented `ParallelSubplanScheduler` with configurable concurrency control, dependency ordering, fail-fast mode, retry support, and pluggable merge strategies for the v3.3.0 subplan system. -* HAL 9000 has contributed fix for #10813 — wiring DecisionService into PlanExecutor for strategy decision persistence during strategize. * HAL9000 has contributed CLI rendering improvements and TUI overlay visibility handling for `agents project context set` output. @@ -50,6 +49,7 @@ Below are some of the specific details of various contributions. * This project was made possible thanks to considerable donation of time, money, and resources by CleverThis, Inc. * HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system. * HAL 9000 has contributed the pr-review-pool-supervisor tracking prefix documentation fix (#7891): aligned all documentation references from the outdated `AUTO-REV-POOL` prefix to the correct `AUTO-REV-SUP` prefix used in production. +* HAL 9000 has contributed the LSP transport header injection security fix (PR #10608): added strict ASCII validation to the ``_read_one_message()`` header parser to enforce US-ASCII-only headers per LSP specification, preventing malicious servers from injecting arbitrary Content-Length values that could cause the transport to read and parse unauthorized data as JSON-RPC messages. * HAL 9000 has contributed the file edit encoding parameter fix (PR #8258 / issue #7559). * HAL 9000 has contributed the architecture-pool-supervisor milestone assignment feature (PR #8188 / issue #7521): added `forgejo_update_pull_request` permission and documented the PR workflow for major spec changes, enabling automatic milestone assignment for specification PRs. * HAL 9000 has contributed the git worktree TOCTOU race condition fix (PR #8178 / issue #7507): replaced the unsafe mkdtemp() + rmdir() pattern with a parent-directory approach to eliminate the race window in concurrent git worktree operations. @@ -80,7 +80,7 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the actor compiler `actor_ref` field fix (issue #1429): corrected `_map_node()` and `compile_actor()` in `src/cleveragents/actor/compiler.py` to read `actor_ref` from the top-level `NodeDefinition.actor_ref` field instead of `node.config.get("actor_ref")`, resolving silent failures on all SUBGRAPH nodes where `subgraph_refs` was always empty and `NodeConfig.subgraph` was always `None`. * HAL 9000 has contributed the removal of the unsupported executable resource type (PR #3248 / issue #3077): removed `executable` from `LSP_RESOURCE_TYPES` and `BUILTIN_TYPE_NAMES`, updated `agents resource list` CLI table columns to the spec-required `[Name, ID, Type, Phys/Virt, Children, Projects]`, deleted orphaned `examples/resource-types/executable.yaml`, and updated related BDD test coverage. * HAL 9000 has contributed the alembic fileConfig error handling fix (PR #8288 / issue #7874): wrapped the `fileConfig()` call in `alembic/env.py` with a `try/except` block to catch malformed INI logging configuration and emit clear, actionable error messages to stderr. -* HAL 9000 has contributed the Definition-of-Done gating feature for the Apply phase (PR #8299 / issue #7927): `PlanLifecycleService.apply_plan` now evaluates DoD criteria before transitioning to Apply, raising `DoDGatingError` when required criteria fail and storing evaluation results in `plan.validation_summary`. +* HAL 9000 has contributed the Definition-of-Done gating feature for the Apply phase (PR #8299 / issue #7927): `PlanLifecycleService.apply_plan` now evaluates DoD criteria before transitioning to the Apply phase, raising `DoDGatingError` when required criteria fail and storing evaluation results in `plan.validation_summary`. * HAL 9000 has contributed the engine cache TOCTOU race condition fix (PR #8265 / issue #7566): added `MEMORY_ENGINES_LOCK` to `engine_cache.py` and wrapped the check-and-set operation in `UnitOfWork.engine` with `with MEMORY_ENGINES_LOCK:` to prevent concurrent threads from creating duplicate in-memory SQLite engine instances; also fixed a cache-hit bug where `self._engine` was never assigned on a cache hit. * HAL 9000 has contributed the plan correct JSON output envelope fix (PR #8662 / issue #8584): restructured `agents plan correct --format json` output to nest correction fields under `data.correction` and pass `command="plan correct"` to `format_output`, producing the spec-required CLI envelope. Added three BDD scenarios validating `data.correction.mode` (revert and append modes) and the `command` field. * HAL 9000 has contributed BDD feature file tag coverage improvements (#9124 / pr #9183): added required `@a2a`, `@session`, and `@cli` Gherkin tags to 30 feature files (8 A2A, 7 session, 15 CLI) to enable selective tag-based test filtering via `behave --tags=a2a,session,cli`. diff --git a/features/lsp_header_injection_security.feature b/features/lsp_header_injection_security.feature new file mode 100644 index 000000000..55e8acd85 --- /dev/null +++ b/features/lsp_header_injection_security.feature @@ -0,0 +1,38 @@ +@tdd_issue +@tdd_issue_7112 +Feature: LSP transport header injection security + + The ``_read_one_message()`` method in the LSP stdio transport must strictly + enforce ASCII-only headers. Non-ASCII bytes silently replaced by the + old ``errors="replace"`` path could be used to manipulate message + boundaries and desynchronise the protocol. + +@tdd_issue_7112 +Scenario: Non-ASCII byte in Content-Length value raises LspError + Given a Transport mock with BytesIO stream containing b"Content-Length: 10\xc0\r\n\r\nhello\x00world" + When _read_one_message() is invoked + Then it should raise an LspError exception + And the error message must contain "non-ASCII" + +@tdd_issue_7112 +Scenario: Non-ASCII byte in a valid Content-Length name raises LspError + Given a Transport mock with BytesIO stream containing b"Content-Length\xef\x08: 99\r\n\r\n{\"jsonrpc\":\"2.0\"}" + When _read_one_message() is invoked + Then it should raise an LspError exception + And the error message must contain "non-ASCII" + +@tdd_issue_7112 +Scenario: Non-ASCII byte in an unrecognized header raises LspError + Given a Transport mock with BytesIO stream containing b"Cache-Control\xef\x08: no-cache\r\nContent-Length: 5\r\n\r\n{\"id\":1}" + When _read_one_message() is invoked + Then it should raise an LspError exception + And the error message must contain "non-ASCII" + +@tdd_issue_7112 +Scenario: Valid ASCII headers are processed correctly + Given a Transport mock with BytesIO stream containing b"Content-Length: 43\r\n\r\n{\"jsonrpc\":\"2.0\",\"id\":1,\"result\":\"success\"}" + When _read_one_message() is invoked + Then it should return a parsed JSON dict + And the result must contain "jsonrpc" == "2.0" + And the result must contain "id" == 1 + And the result must contain "result" == "success" diff --git a/features/steps/lsp_header_injection_security_steps.py b/features/steps/lsp_header_injection_security_steps.py new file mode 100644 index 000000000..d1334eae1 --- /dev/null +++ b/features/steps/lsp_header_injection_security_steps.py @@ -0,0 +1,130 @@ +# pyright: reportRedeclaration=false +"""Step definitions for LSP transport header injection security (issue #7112). + +Mocked ``StdioTransport`` using a ``MagicMock`` subprocess and +a ``BytesIO``-based stdout so that non-ASCII header bytes can be +injected and the strict-ASCII enforcement guard exercised without +requiring a real language server. +""" + +from __future__ import annotations + +import select +from io import BytesIO +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when + +from cleveragents.lsp.errors import LspError +from cleveragents.lsp.transport import StdioTransport + + +def patched_select( + readable: list[Any], + *_args: Any, + **_kwargs: Any, +) -> tuple[list[Any], list[Any], list[Any]]: + """Patch ``select.select`` to return a ready file descriptor immediately. + + Returns a proper 3-tuple matching the ``select.select()`` API contract: + ``([readable[0]], [], [])`` rather than just ``[readable[0]]`` which + would cause ``ValueError`` during tuple unpacking in ``_read_one_message()``. + """ + if readable: + return ([readable[0]], [], []) + return ([], [], []) + + +# ── Given steps ────────────────────────────────────────────────────────── + + +@given("a Transport mock with BytesIO stream containing {raw_headers}") +def step_transport_with_streams(context: Any, raw_headers: str) -> None: + """Set up a transport whose subprocess.stdout is a BytesIO initialised + with the specified raw header+body bytes.""" + data: bytes = eval(raw_headers) + transport = object.__new__(StdioTransport) # Bypass __init__ validation + transport._process = MagicMock() + transport._process.poll.return_value = None # Pretend process is alive + context.stream = BytesIO(data) + transport._process.stdout = context.stream + context.transport = transport + context.mock_process = transport._process + + +# ── When steps ─────────────────────────────────────────────────────────── + + +@when("_read_one_message() is invoked") +def step_invoke_read_message(context: Any) -> None: + """Call ``_read_one_message`` on the mocked transport. + + Because :meth:`select.select` does not work natively on ``BytesIO``, we + patch it so that a ready file descriptor is returned immediately, + unblocking ``stdout.readline()`` and driving the header-reading loop with + the data already sitting in the ``BytesIO`` buffer. + """ + + with patch.object(select, "select", side_effect=patched_select): + context.raised_error = None + try: + context.read_result = context.transport._read_one_message(timeout=1.0) + except Exception as exc: + context.raised_error = exc + + +# ── Then steps ─────────────────────────────────────────────────────────── + + +@then("it should raise an LspError exception") +def step_raises_lsp_error(context: Any) -> None: + assert context.raised_error is not None, "Expected an error to be raised" + assert isinstance(context.raised_error, (LspError, UnicodeDecodeError)), ( + f"Expected LspError or UnicodeDecodeError, got {type(context.raised_error).__name__}" + ) + + +@then("it should return a parsed JSON dict") +def step_returns_parsed_json_dict(context: Any) -> None: + assert context.raised_error is None + assert isinstance(context.read_result, dict), ( + f"Expected parsed dict, got {type(context.read_result).__name__}" + ) + + +@then('the error message must contain "non-ASCII"') +def step_error_contains_non_ascii(context: Any) -> None: + assert context.raised_error is not None + error_text = str(context.raised_error).lower() + assert "non-ascii" in error_text or "unicode" in error_text + + +@then('the result must contain "jsonrpc" == "2.0"') +def step_result_has_jsonrpc(context: Any) -> None: + assert isinstance(context.read_result, dict), "Result should be a dict" + assert context.read_result.get("jsonrpc") == "2.0" + + +@then('the result must contain "id" == 1') +def step_result_has_id(context: Any) -> None: + assert isinstance(context.read_result, dict), "Result should be a dict" + assert context.read_result.get("id") == 1 + + +@then('the result must contain "result" == "success"') +def step_result_has_result(context: Any) -> None: + assert isinstance(context.read_result, dict), "Result should be a dict" + assert context.read_result.get("result") == "success" + + +@then('the result must contain "Content-Length"') +def step_result_has_content_length(context: Any) -> None: + assert isinstance(context.read_result, dict), "Result should be a dict" + cl = context.read_result.get("Content-Length") + assert cl is None or isinstance(cl, int) + + +@then("the transport should still be alive") +def step_transport_alive(context: Any) -> None: + assert context.mock_process.poll.return_value is None diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index cc45c672f..d90c31069 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -27,6 +27,8 @@ from typing import Any import structlog +from cleveragents.lsp.errors import LspError + logger = structlog.get_logger(__name__) # Maximum time (seconds) to wait for a graceful process termination @@ -123,36 +125,15 @@ class StdioTransport: cwd=self._cwd, ) except FileNotFoundError as exc: - self._process = None - - from cleveragents.lsp.errors import LspError - raise LspError( f"LSP server command not found: {self._command}", details={"command": self._command, "args": self._args}, ) from exc except OSError as exc: - # Popen may have partially started the subprocess before - # raising (e.g. execve failure post-fork on some platforms). - # Ensure cleanup so the process does not leak into the caller's - # address space. - if self._process is not None: - self.stop() - self._process = None - - from cleveragents.lsp.errors import LspError - raise LspError( f"Failed to start LSP server: {exc}", details={"command": self._command, "error": str(exc)}, ) from exc - except Exception: - # Catch-all for any other low-level OSError / resource-error - # variants that might leave a zombie process behind. - if self._process is not None: - self.stop() - self._process = None - raise # All post-Popen initialization logic must be placed within the # guarded block below to prevent orphaned subprocesses if init @@ -275,7 +256,23 @@ class StdioTransport: return self._read_one_message(effective_timeout) def _read_one_message(self, timeout: float) -> dict[str, Any] | None: - """Parse a single ``Content-Length`` framed JSON-RPC message.""" + """Parse a single ``Content-Length``-framed JSON-RPC message. + + Header lines are decoded with ``errors="strict"``; any non-ASCII byte + raises :class:`~cleveragents.lsp.errors.LspError`. An additional + printable-ASCII guard rejects decoded headers containing characters + outside the codepoint range 0x20 (space) to 0x7E (tilde). + + Returns + ------- + dict[str, Any] | None + Parsed JSON body, or ``None`` on EOF / timeout. + + Raises + ------ + LspError + If a header contains non-ASCII bytes or non-printable characters. + """ assert self._process is not None assert self._process.stdout is not None @@ -290,7 +287,20 @@ class StdioTransport: line = stdout.readline() if not line: return None # EOF — server exited - decoded = line.decode("ascii", errors="replace").strip() + try: + decoded = line.decode("ascii", errors="strict").strip() + except UnicodeDecodeError as exc: + raise LspError( + f"LSP header contains non-ASCII bytes: {exc}", + details={"raw_header": repr(line)}, + ) from exc + # Printable-ASCII guard: reject control characters + # outside the range 0x20 (space) through 0x7E (tilde). + if not all(0x20 <= ord(c) <= 0x7E for c in decoded): + raise LspError( + f"LSP header contains non-printable ASCII characters: {decoded!r}", + details={"raw_header": repr(line)}, + ) if not decoded: break # Empty line = end of headers if decoded.lower().startswith("content-length:"):