From e4998e5066e987b9898e5621bed20d0f12b2be0f Mon Sep 17 00:00:00 2001 From: n1ckyb Date: Sun, 9 Aug 2026 22:08:26 +0100 Subject: [PATCH] fix(lsp): log a workspace-containment refusal as a refusal, not an error Closes #23. A URI resolving outside the workspace root raised a bare ValueError, caught by the blanket `except Exception` in server.py and logged with the same prefix at the same level as an internal fault: WARNING intentumdiff.lsp_server.server: intentumdiff/semanticDiff error: URI '...' resolves to '...' which is outside the workspace root '...' PASSED An operator could not tell "someone asked for a path outside the workspace and we blocked it" from "the diff engine threw". The refusal is a security control, so its audit trail is part of the control. Adds WorkspaceContainmentError, subclassing ValueError so every existing caller and test that catches ValueError keeps working. Raised for both the explicit traversal case and the containment case, handled separately, and the response carries code="workspace_containment" so a client can act on it without string-matching a message. The test now ASSERTS the log line, and asserts the internal-error prefix is absent. Previously nothing checked it: the warning could have been deleted and every test would still have passed. Capturing it also stops the warning printing in full beside a PASSED line, which reads like something went wrong and was ignored. pytest tests/unit/test_lsp_server.py: 24 passed. Co-Authored-By: Claude Opus 5 --- src/intentumdiff/lsp_server/_handlers.py | 15 ++++++++++++-- src/intentumdiff/lsp_server/server.py | 9 ++++++++ tests/unit/test_lsp_server.py | 26 +++++++++++++++++++++--- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/intentumdiff/lsp_server/_handlers.py b/src/intentumdiff/lsp_server/_handlers.py index 9cc72b8..3b413e6 100644 --- a/src/intentumdiff/lsp_server/_handlers.py +++ b/src/intentumdiff/lsp_server/_handlers.py @@ -45,6 +45,17 @@ class _ServerState: # URI helpers # --------------------------------------------------------------------------- +class WorkspaceContainmentError(ValueError): + """A URI resolved outside the workspace root, or used explicit traversal. + + A distinct type because this is a REFUSAL, not a fault: the server did its job. Logged and + reported separately from internal errors so an operator can tell "someone asked for a path + outside the workspace and we blocked it" from "the diff engine threw". + + Subclasses ValueError deliberately - callers that already catch ValueError keep working. + """ + + def uri_to_path(uri: str, workspace_root: Path | None = None) -> Path: """Convert a ``file://`` URI to a :class:`~pathlib.Path`. @@ -57,7 +68,7 @@ def uri_to_path(uri: str, workspace_root: Path | None = None) -> Path: path = Path(unquote(parsed.path)) # Reject explicit traversal components if ".." in path.parts: - raise ValueError(f"Path traversal rejected in URI: {uri!r}") + raise WorkspaceContainmentError(f"Path traversal rejected in URI: {uri!r}") # On Windows, urlparse puts a leading '/' before the drive letter — # strip it so Path('C:/foo') is constructed correctly. if path.parts and path.parts[0] in ("/", "\\") and len(path.parts) > 1: @@ -72,7 +83,7 @@ def uri_to_path(uri: str, workspace_root: Path | None = None) -> Path: try: resolved.relative_to(root_resolved) except ValueError: - raise ValueError( + raise WorkspaceContainmentError( f"URI {uri!r} resolves to {resolved!r} which is outside the " f"workspace root {root_resolved!r}." ) diff --git a/src/intentumdiff/lsp_server/server.py b/src/intentumdiff/lsp_server/server.py index b3a42d2..3deb52b 100644 --- a/src/intentumdiff/lsp_server/server.py +++ b/src/intentumdiff/lsp_server/server.py @@ -36,6 +36,7 @@ _do_diff, schedule_diff, uri_to_path, + WorkspaceContainmentError, ) log = logging.getLogger(__name__) @@ -198,6 +199,14 @@ def _semantic_diff(params: dict) -> dict: return json.loads(diff.model_dump_json()) + except WorkspaceContainmentError as exc: + # A refusal, not a fault. Logged distinctly so an operator can tell "someone asked + # for a path outside the workspace and we blocked it" from "the engine threw" - + # previously both produced the same line at the same level, which made the audit + # trail for a path-traversal attempt indistinguishable from routine noise. + log.warning("intentumdiff/semanticDiff refused (workspace containment): %s", exc) + return {"error": str(exc), "code": "workspace_containment"} + except Exception as exc: # noqa: BLE001 log.warning("intentumdiff/semanticDiff error: %s", exc) return {"error": str(exc)} diff --git a/tests/unit/test_lsp_server.py b/tests/unit/test_lsp_server.py index 0ee0703..29530bc 100644 --- a/tests/unit/test_lsp_server.py +++ b/tests/unit/test_lsp_server.py @@ -13,6 +13,8 @@ from __future__ import annotations +import logging + import sys from pathlib import Path from unittest.mock import patch @@ -330,17 +332,35 @@ def test_non_file_workspace_root_returns_error(self, _srv): assert "error" in result assert "workspace root" in result["error"] - def test_uri_outside_root_returns_containment_error(self, _srv, tmp_path): + def test_uri_outside_root_returns_containment_error(self, _srv, tmp_path, caplog): + """A refusal outside the workspace root, and the audit line that records it. + + The log line is asserted, not merely tolerated. It is part of the control: a server + that silently refuses a traversal attempt tells an operator nothing. Without this + assertion the warning could be deleted and every test would still pass. + + Capturing it also keeps the suite output clean - this warning used to print in full + beside a PASSED line, which reads like something went wrong and was ignored. + """ server, handler = _srv root = tmp_path / "project" root.mkdir() # Both paths are siblings of the workspace root, not inside it outside_old = tmp_path / "evil" / "old.py" outside_new = tmp_path / "evil" / "new.py" - with self._ws_patch(server, root.as_uri()): - result = handler({"oldUri": outside_old.as_uri(), "newUri": outside_new.as_uri()}) + with caplog.at_level(logging.WARNING, logger="intentumdiff.lsp_server.server"): + with self._ws_patch(server, root.as_uri()): + result = handler( + {"oldUri": outside_old.as_uri(), "newUri": outside_new.as_uri()} + ) assert "error" in result assert "outside" in result["error"] + # A distinct code, so a client can act on a refusal without string-matching. + assert result.get("code") == "workspace_containment" + # Logged as a refusal, NOT as an internal error - the two were previously + # indistinguishable at the same level with the same prefix. + assert "refused (workspace containment)" in caplog.text + assert "intentumdiff/semanticDiff error:" not in caplog.text def test_uri_inside_root_passes_containment(self, _srv, tmp_path): server, handler = _srv