diff --git a/CHANGELOG.md b/CHANGELOG.md index eea25f037..e8821560f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,7 +160,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). relative globs also match absolute paths. Added BDD regression tests in `execute_phase_context_assembler_coverage.feature` and `project_context_phase_analysis.feature`. - - **LSP transport header injection vulnerability (#7112)**: Replaced ``errors="replace"`` with +- **LSP transport header injection vulnerability (#7112)**: Replaced ``errors="replace"`` with ``errors="strict"`` in the ``_read_one_message()`` method of ``src/cleveragents/lsp/transport.py``, so that non-ASCII bytes in message headers raise a clearly-typed :class:`~cleveragents.lsp.errors.LspError` instead of silently replacing the diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index ceeb27aa4..19be09580 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -2,7 +2,6 @@ * Aditya Chhabra * Brent E. Edwards -* HAL 9000 has contributed the LSP transport header injection security fix (#7112): replaced non-strict ASCII decoding in the LSP stdio transport's ``_read_one_message()`` method to prevent message boundary manipulation via encoding attacks, with comprehensive BDD test coverage for the vulnerability scenario. * HAL 9000 * Hamza Khyari * Jeffrey Phillips Freeman @@ -26,6 +25,7 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the decision recording hook for the Strategize phase (issue #8522): captures every decision point with question, chosen option, alternatives, confidence, rationale, and full context snapshot for replay and correction. * HAL 9000 has contributed automated specification maintenance, documentation updates, and bot-driven PR authorship. * This project was made possible thanks to considerable donation of time, money, and resources by CleverThis, Inc. +* HAL 9000 has contributed the LSP transport header injection security fix (#7112): replaced non-strict ASCII decoding in the LSP stdio transport's ``_read_one_message()`` method with `errors="strict"`, added a printable-ASCII guard rejecting characters outside 0x20–0x7E, and added comprehensive BDD test coverage for all three header injection vulnerability scenarios. * 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 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. diff --git a/features/lsp_header_injection_security.feature b/features/lsp_header_injection_security.feature index c9c2d6b6b..9b71c5cd0 100644 --- a/features/lsp_header_injection_security.feature +++ b/features/lsp_header_injection_security.feature @@ -1,4 +1,6 @@ -@tdd_issue @tdd_issue_7112 Feature: LSP transport header injection vulnerability (issue #7112) +@tdd_issue +@tdd_issue_7112 +Feature: LSP transport header injection vulnerability (issue #7112) The _read_one_message() method in the LSP stdio transport must strictly enforce ASCII-only headers. Non-ASCII bytes silently replaced by the @@ -11,7 +13,7 @@ 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 +@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 @@ -19,7 +21,7 @@ @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 + When _read_one_message() is invoked Then it should raise an LspError exception And the error message must contain "non-ASCII" @@ -27,6 +29,6 @@ Given a Transport mock with BytesIO stream containing b"Content-Length: 46\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 "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 index 951e54159..e7a12b587 100644 --- a/features/steps/lsp_header_injection_security_steps.py +++ b/features/steps/lsp_header_injection_security_steps.py @@ -16,20 +16,22 @@ from unittest.mock import MagicMock, patch from behave import given, then, when +from cleveragents.lsp.errors import LspError from cleveragents.lsp.transport import StdioTransport # ── Given steps ────────────────────────────────────────────────────────── -@given("a Transport mock with BytesIO stream containing {raw_headers:r}") -def step_transport_with_streams(context: Any, raw_headers: bytes) -> None: +@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(raw_headers) + context.stream = BytesIO(data) transport._process.stdout = context.stream context.transport = transport context.mock_process = transport._process @@ -47,11 +49,14 @@ def step_invoke_read_message(context: Any) -> None: unblocking ``stdout.readline()`` and driving the header-reading loop with the data already sitting in the ``BytesIO`` buffer. """ - - def _patched_select(readable, *_args: Any, timeout: float | None = None) -> list[Any]: + def _patched_select( + readable: list[Any], + *_args: Any, + **_kwargs: Any, + ) -> tuple[list[Any], list[Any], list[Any]]: if readable: - return [readable[0]] # Mark as ready - return [] # Timeout + return ([readable[0]], [], []) # Proper 3-tuple per select.select contract + return ([], [], []) # Timeout path (also a proper 3-tuple) with patch.object(select, "select", side_effect=_patched_select): context.raised_error = None @@ -67,7 +72,6 @@ def step_invoke_read_message(context: Any) -> None: @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" - from cleveragents.lsp.errors import LspError assert isinstance( context.raised_error, (LspError, UnicodeDecodeError) ), f"Expected LspError or UnicodeDecodeError, got {type(context.raised_error).__name__}" @@ -106,11 +110,6 @@ def step_result_has_result(context: Any) -> None: assert context.read_result.get("result") == "success" -@then("no error should be raised") -def step_no_error(context: Any) -> None: - assert context.raised_error is None - - @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" diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index e457735ad..41063d16f 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -114,15 +114,11 @@ class StdioTransport: cwd=self._cwd, ) except FileNotFoundError as exc: - 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: - from cleveragents.lsp.errors import LspError - raise LspError( f"Failed to start LSP server: {exc}", details={"command": self._command, "error": str(exc)}, @@ -224,7 +220,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