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