From 9f40b5d69a97a8f8c8fa574083111d5239f01cf7 Mon Sep 17 00:00:00 2001 From: John Morrissey <544926+tachyon-beep@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:17:33 +1000 Subject: [PATCH] fix(python): distrust repository-local interpreters --- crates/loomweave-core/src/plugin/host.rs | 18 +-- .../loomweave-core/src/plugin/interpreter.rs | 129 +++--------------- .../loomweave_plugin_python/interpreter.py | 19 +-- plugins/python/tests/test_interpreter.py | 46 +++---- plugins/python/tests/test_pyright_session.py | 13 +- plugins/python/tests/test_server.py | 13 +- 6 files changed, 76 insertions(+), 162 deletions(-) diff --git a/crates/loomweave-core/src/plugin/host.rs b/crates/loomweave-core/src/plugin/host.rs index 2d742137..5745057d 100644 --- a/crates/loomweave-core/src/plugin/host.rs +++ b/crates/loomweave-core/src/plugin/host.rs @@ -1600,11 +1600,11 @@ ontology_version = "0.1.0" std::fs::set_permissions(&venv, std::fs::Permissions::from_mode(0o755)).unwrap(); let empty = |_: &str| -> Option { None }; - // A pyright plugin is handed the project's own `.venv` interpreter. + // Repository-local interpreters are untrusted and are not exported. assert_eq!( exported_interpreter(&pyright_small_rss_manifest(), dir.path(), &empty), - Some(venv.clone()), - "a language-server plugin must be pinned to the project interpreter" + None, + "a language-server plugin must not execute a repository interpreter" ); // A plugin that does not declare the pyright runtime never triggers // discovery: the variable means nothing to it, and exporting it would @@ -1625,18 +1625,14 @@ ontology_version = "0.1.0" None, "an operator override must survive untouched" ); - // ...but an EMPTY value is not an override. Both discoveries treat - // `""` as unset on the override rung (`if override:` in Python, - // `.filter(|v| !v.is_empty())` in Rust), so a bare `is_some()` guard - // here would suppress the export for a variable the plugin then also - // ignores — leaving the child with no interpreter at all, which is the - // launcher-dependent hole this export exists to close. + // An EMPTY value is not an override, and does not make repository + // interpreter discovery safe. let empty_override = |key: &str| -> Option { (key == PYTHON_INTERPRETER_ENV).then(OsString::new) }; assert_eq!( exported_interpreter(&pyright_small_rss_manifest(), dir.path(), &empty_override), - Some(venv.clone()), - "an empty override is unset: the host still exports its own pinned choice" + None, + "an empty override must not permit a repository interpreter" ); // An UNPINNED (bare `PATH`) choice is not exported: presenting a guess // to the plugin as an authoritative pin buys nothing over its own diff --git a/crates/loomweave-core/src/plugin/interpreter.rs b/crates/loomweave-core/src/plugin/interpreter.rs index 81de1d79..2a476d8c 100644 --- a/crates/loomweave-core/src/plugin/interpreter.rs +++ b/crates/loomweave-core/src/plugin/interpreter.rs @@ -37,8 +37,6 @@ pub const PYTHON_INTERPRETER_ENV: &str = "LOOMWEAVE_PYTHON_INTERPRETER"; pub enum InterpreterSource { /// [`PYTHON_INTERPRETER_ENV`] named an executable file. Override, - /// `/.venv/bin/python` — the project's own virtualenv. - DotVenv, /// `$VIRTUAL_ENV/bin/python` — an activated virtualenv. VirtualEnv, /// `$CONDA_PREFIX/bin/python` — an activated conda environment. @@ -60,7 +58,7 @@ pub struct ProjectInterpreter { } impl ProjectInterpreter { - /// Project-owned (override / `.venv` / `VIRTUAL_ENV` / `CONDA_PREFIX`). + /// Explicitly selected (override / `VIRTUAL_ENV` / `CONDA_PREFIX`). #[must_use] pub fn pinned(&self) -> bool { !matches!( @@ -72,7 +70,7 @@ impl ProjectInterpreter { /// Stable string for `plugin_index_meta.resolver_environment`. /// /// An unpinned choice is tagged so it can never compare equal to a pinned - /// path with the same bytes: acquiring a project venv at a location that + /// path with the same bytes: activating an environment whose interpreter /// happened to be first on `PATH` still moves the marker. #[must_use] pub fn fingerprint(&self) -> String { @@ -184,17 +182,13 @@ fn which(name: &str, path_var: Option<&OsString>) -> Option { /// Resolve the project's interpreter in the contract order (module docs). /// `env` abstracts `std::env::var_os` so tests can inject an environment. /// -/// `project_root` MUST be canonicalised by the caller. Discovery joins -/// `.venv/bin/python` onto the root as given and normalises only lexically, so -/// a symlinked root yields a symlinked interpreter path and a different -/// [`ProjectInterpreter::fingerprint`]. `analyze` (which records the -/// fingerprint) and `PluginHost::spawn_unhandshaken` (which exports the -/// interpreter) both canonicalise first; dropping it at either site would skew -/// the marker against the exported interpreter and re-dispatch every run. See -/// `the_root_canonicalisation_at_both_call_sites_is_load_bearing`. +/// `project_root` remains part of the cross-language API but is deliberately +/// not inspected for interpreters. Pyright executes its configured Python, so +/// selecting `/.venv/bin/python` would execute untrusted +/// repository content without an operator trust decision. #[must_use] pub fn discover_project_interpreter( - project_root: &Path, + _project_root: &Path, env: &dyn Fn(&str) -> Option, ) -> ProjectInterpreter { if let Some(raw) = env(PYTHON_INTERPRETER_ENV).filter(|value| !value.is_empty()) { @@ -209,12 +203,6 @@ pub fn discover_project_interpreter( "{PYTHON_INTERPRETER_ENV} is not an executable file; ignoring the override" ); } - if let Some(path) = usable(&project_root.join(".venv/bin/python")) { - return ProjectInterpreter { - path: Some(path), - source: InterpreterSource::DotVenv, - }; - } for (var, source) in [ ("VIRTUAL_ENV", InterpreterSource::VirtualEnv), ("CONDA_PREFIX", InterpreterSource::Conda), @@ -286,53 +274,6 @@ mod tests { assert_eq!(PYTHON_INTERPRETER_ENV, "LOOMWEAVE_PYTHON_INTERPRETER"); } - #[test] - fn the_root_canonicalisation_at_both_call_sites_is_load_bearing() { - // `analyze` computes the fingerprint from its canonicalised - // `project_root`; `PluginHost::spawn_unhandshaken` re-canonicalises the - // root it is handed before running the SAME discovery to decide what to - // export. They agree only because BOTH canonicalise and canonicalise is - // idempotent. - // - // Discovery itself is deliberately NOT root-invariant: it joins - // `.venv/bin/python` onto the root as given and lexically normalises, - // so a symlinked root yields a symlinked interpreter path. That is - // correct for a venv (see the symlink test below) but it means dropping - // the canonicalisation at either call site would silently skew the - // recorded marker against the exported interpreter — an index that - // re-dispatches every run. This test pins the skew so that removal - // fails loudly here rather than quietly in production. - let dir = tempfile::tempdir().unwrap(); - let real_root = dir.path().join("real"); - let venv = make_python(&real_root.join(".venv/bin/python")); - let link_root = dir.path().join("link"); - std::os::unix::fs::symlink(&real_root, &link_root).unwrap(); - let canonical_root = link_root.canonicalize().unwrap(); - - assert_eq!( - discover_project_interpreter(&canonical_root, &env(&HashMap::new())).path, - Some(venv), - "a canonical root finds the project .venv at its real path" - ); - // Idempotence — the property the two call sites actually rely on. - assert_eq!( - discover_project_interpreter(&canonical_root, &env(&HashMap::new())).fingerprint(), - discover_project_interpreter( - &canonical_root.canonicalize().unwrap(), - &env(&HashMap::new()) - ) - .fingerprint(), - "canonicalising twice must not move the fingerprint" - ); - // And the skew a dropped canonicalisation would introduce. - assert_ne!( - discover_project_interpreter(&link_root, &env(&HashMap::new())).fingerprint(), - discover_project_interpreter(&canonical_root, &env(&HashMap::new())).fingerprint(), - "an UNcanonicalised root yields a different fingerprint — which is why both \ - `analyze` and `spawn_unhandshaken` must canonicalise before discovering" - ); - } - /// Sets the process CWD for the duration of a test and restores it on drop /// (including on panic, so a failing assertion cannot leak a bad CWD into /// another test in the same binary). @@ -416,19 +357,6 @@ mod tests { "empty values on every rung must discover nothing — NOT the CWD's python" ); - // An empty override falls through to `.venv` without taking the - // warning path (that branch is for an operator who set a BAD path, not - // for an unset variable), and an empty PATH cannot outrank it. - let dotvenv = make_python(&root.join(".venv/bin/python")); - assert_eq!( - discover_project_interpreter(&root, &env(&all_empty)), - ProjectInterpreter { - path: Some(dotvenv), - source: InterpreterSource::DotVenv - }, - "an empty override falls through to .venv" - ); - // Control: a NON-empty PATH naming a directory with no interpreter // reaches the same `None`, the legitimate way. let empty_dir = dir.path().join("empty"); @@ -441,33 +369,23 @@ mod tests { } #[test] - fn dotvenv_wins_over_virtual_env_and_path() { + fn repository_dotvenv_is_ignored() { let dir = tempfile::tempdir().unwrap(); - let dotvenv = make_python(&dir.path().join(".venv/bin/python")); - let other = make_python(&dir.path().join("elsewhere/bin/python")); - let map = HashMap::from([ - ( - "VIRTUAL_ENV", - dir.path().join("elsewhere").display().to_string(), - ), - ("PATH", other.parent().unwrap().display().to_string()), - ]); + make_python(&dir.path().join(".venv/bin/python")); + let trusted = make_python(&dir.path().join("elsewhere/bin/python")); + let map = HashMap::from([( + "VIRTUAL_ENV", + dir.path().join("elsewhere").display().to_string(), + )]); let found = discover_project_interpreter(dir.path(), &env(&map)); - assert_eq!( - found, - ProjectInterpreter { - path: Some(dotvenv.clone()), - source: InterpreterSource::DotVenv - } - ); - assert!(found.pinned()); - assert_eq!(found.fingerprint(), dotvenv.display().to_string()); + assert_eq!(found.path, Some(trusted)); + assert_eq!(found.source, InterpreterSource::VirtualEnv); } #[test] fn override_wins_and_an_unusable_override_falls_through() { let dir = tempfile::tempdir().unwrap(); - let dotvenv = make_python(&dir.path().join(".venv/bin/python")); + make_python(&dir.path().join(".venv/bin/python")); let custom = make_python(&dir.path().join("custom/python")); let map = HashMap::from([(PYTHON_INTERPRETER_ENV, custom.display().to_string())]); assert_eq!( @@ -479,8 +397,8 @@ mod tests { dir.path().join("nope").display().to_string(), )]); let found = discover_project_interpreter(dir.path(), &env(&map)); - assert_eq!(found.source, InterpreterSource::DotVenv); - assert_eq!(found.path, Some(dotvenv)); + assert_eq!(found.source, InterpreterSource::None); + assert_eq!(found.path, None); } #[test] @@ -544,12 +462,9 @@ mod tests { discover_project_interpreter(dir.path(), &env(&map)).path, Some(real.clone()) ); - let link = dir.path().join(".venv/bin/python"); - fs::create_dir_all(link.parent().unwrap()).unwrap(); - std::os::unix::fs::symlink(&real, &link).unwrap(); + // A repository-local symlink is not considered without an explicit override. let found = discover_project_interpreter(dir.path(), &env(&HashMap::new())); - assert_eq!(found.path, Some(link), "the symlink path, not its target"); - assert_eq!(found.source, InterpreterSource::DotVenv); + assert_eq!(found.source, InterpreterSource::None); } #[test] @@ -558,7 +473,7 @@ mod tests { // separator at construction, so `is_file()` succeeds and the plugin // pins the override. Rust's `metadata`/`access(2)` on the raw // `…/python/` fail with ENOTDIR, so without the strip the HOST would - // fall through to `.venv` (or export nothing) while the PLUGIN pinned + // export nothing while the PLUGIN pinned // the operator's path — the two disagreeing on the same environment, // which is the failure mode this module exists to prevent. The // returned path must also be the stripped, normalised one, byte-equal diff --git a/plugins/python/src/loomweave_plugin_python/interpreter.py b/plugins/python/src/loomweave_plugin_python/interpreter.py index 060afa99..753a57aa 100644 --- a/plugins/python/src/loomweave_plugin_python/interpreter.py +++ b/plugins/python/src/loomweave_plugin_python/interpreter.py @@ -5,9 +5,11 @@ ``python.pythonPath``. Under ``loomweave analyze`` launched from an agent hook that ``python`` is the system interpreter, which cannot import the project's editable install, and every ``tests/`` -> ``src/`` call target came back empty -while the coverage claim still said ``complete``. This module picks the -project's own interpreter deterministically so the answer no longer depends -on who launched the run. +while the coverage claim still said ``complete``. This module picks an +explicitly trusted or activated interpreter deterministically so the answer +no longer depends on who launched the run. Repository-local interpreters are +deliberately not discovered: Pyright executes the selected interpreter, so an +executable committed at ``.venv/bin/python`` is untrusted repository content. The order below is a CROSS-LANGUAGE CONTRACT with ``crates/loomweave-core/src/plugin/interpreter.rs`` (the host runs the same @@ -43,14 +45,14 @@ # must carry the same literal. INTERPRETER_OVERRIDE_ENV: Final = "LOOMWEAVE_PYTHON_INTERPRETER" -InterpreterSource = Literal["override", "dotvenv", "virtual_env", "conda", "path", "none"] +InterpreterSource = Literal["override", "virtual_env", "conda", "path", "none"] _PREFIX_SOURCES: Final[tuple[tuple[str, InterpreterSource], ...]] = ( ("VIRTUAL_ENV", "virtual_env"), ("CONDA_PREFIX", "conda"), ) _PINNED_SOURCES: Final[frozenset[InterpreterSource]] = frozenset( - {"override", "dotvenv", "virtual_env", "conda"}, + {"override", "virtual_env", "conda"}, ) @@ -63,7 +65,7 @@ class ProjectInterpreter: @property def pinned(self) -> bool: - """True when the interpreter is project-owned (not a PATH guess).""" + """True when the interpreter was explicitly selected (not a PATH guess).""" return self.source in _PINNED_SOURCES @@ -78,6 +80,9 @@ def discover_project_interpreter( environ: Mapping[str, str] | None = None, ) -> ProjectInterpreter: """Resolve the project's interpreter in the contract order (see module doc).""" + # Keep the project root in the API because this function is mirrored by + # the host, but never use it to select an executable from the repository. + _ = project_root env = os.environ if environ is None else environ override = env.get(INTERPRETER_OVERRIDE_ENV) if override: @@ -87,8 +92,6 @@ def discover_project_interpreter( f"loomweave-plugin-python: {INTERPRETER_OVERRIDE_ENV}={override!r} is not an " "executable file; ignoring the override and discovering the interpreter\n", ) - if (hit := _usable(Path(project_root) / ".venv" / "bin" / "python")) is not None: - return ProjectInterpreter(path=str(hit), source="dotvenv") for var, source in _PREFIX_SOURCES: prefix = env.get(var) if prefix and (hit := _usable(Path(prefix) / "bin" / "python")) is not None: diff --git a/plugins/python/tests/test_interpreter.py b/plugins/python/tests/test_interpreter.py index 2ade84b7..6d19a0b3 100644 --- a/plugins/python/tests/test_interpreter.py +++ b/plugins/python/tests/test_interpreter.py @@ -25,18 +25,18 @@ def _make_python(path: Path) -> Path: return path -def test_dotvenv_wins_over_virtual_env_and_path(tmp_path: Path) -> None: - dotvenv = _make_python(tmp_path / ".venv" / "bin" / "python") - other = _make_python(tmp_path / "elsewhere" / "bin" / "python") - environ = {"VIRTUAL_ENV": str(other.parent.parent), "PATH": str(other.parent)} +def test_repository_dotvenv_is_ignored(tmp_path: Path) -> None: + _make_python(tmp_path / ".venv" / "bin" / "python") + trusted = _make_python(tmp_path / "elsewhere" / "bin" / "python") - found = discover_project_interpreter(tmp_path, environ) + found = discover_project_interpreter( + tmp_path, {"VIRTUAL_ENV": str(trusted.parent.parent)} + ) - assert found == ProjectInterpreter(path=str(dotvenv), source="dotvenv") + assert found == ProjectInterpreter(path=str(trusted), source="virtual_env") assert found.pinned - -def test_override_env_wins_over_dotvenv(tmp_path: Path) -> None: +def test_override_env_wins_over_other_sources(tmp_path: Path) -> None: _make_python(tmp_path / ".venv" / "bin" / "python") override = _make_python(tmp_path / "custom" / "python") @@ -50,13 +50,12 @@ def test_override_env_wins_over_dotvenv(tmp_path: Path) -> None: def test_unusable_override_is_ignored_and_discovery_continues( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: - dotvenv = _make_python(tmp_path / ".venv" / "bin" / "python") missing = tmp_path / "nope" / "python" found = discover_project_interpreter(tmp_path, {INTERPRETER_OVERRIDE_ENV: str(missing)}) - assert found.source == "dotvenv" - assert found.path == str(dotvenv) + assert found.source == "none" + assert found.path is None assert INTERPRETER_OVERRIDE_ENV in capsys.readouterr().err @@ -97,7 +96,7 @@ def test_nothing_found_is_none_and_unpinned(tmp_path: Path) -> None: assert not found.pinned -def test_non_executable_dotvenv_python_is_not_a_hit(tmp_path: Path) -> None: +def test_non_executable_dotvenv_python_is_ignored(tmp_path: Path) -> None: target = tmp_path / ".venv" / "bin" / "python" target.parent.mkdir(parents=True) target.write_text("", encoding="utf-8") @@ -110,11 +109,11 @@ def test_non_executable_dotvenv_python_is_not_a_hit(tmp_path: Path) -> None: def test_environ_defaults_to_os_environ(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - dotvenv = _make_python(tmp_path / ".venv" / "bin" / "python") + venv = _make_python(tmp_path / "trusted" / "bin" / "python") monkeypatch.delenv(INTERPRETER_OVERRIDE_ENV, raising=False) + monkeypatch.setenv("VIRTUAL_ENV", str(venv.parent.parent)) - assert discover_project_interpreter(tmp_path).path == str(dotvenv) - + assert discover_project_interpreter(tmp_path).path == str(venv) def test_empty_environ_does_not_leak_to_os_environ(tmp_path: Path) -> None: # With an empty environ (no PATH), even if os.environ has a python on PATH, @@ -125,20 +124,15 @@ def test_empty_environ_does_not_leak_to_os_environ(tmp_path: Path) -> None: assert not found.pinned -def test_symlink_paths_are_preserved(tmp_path: Path) -> None: - # Create a real interpreter at base/python3.12 and symlink .venv/bin/python to it. +def test_repository_dotvenv_symlink_is_ignored(tmp_path: Path) -> None: base_python = _make_python(tmp_path / "base" / "python3.12") - venv_bin = tmp_path / ".venv" / "bin" - venv_bin.mkdir(parents=True) - symlink_python = venv_bin / "python" + symlink_python = tmp_path / ".venv" / "bin" / "python" + symlink_python.parent.mkdir(parents=True) symlink_python.symlink_to(base_python) - found = discover_project_interpreter(tmp_path, {}) - - # The result should be the symlink path, not the target. - assert found.path == str(symlink_python) - assert found.source == "dotvenv" - + assert discover_project_interpreter(tmp_path, {}) == ProjectInterpreter( + path=None, source="none" + ) def test_path_with_python_and_python3_in_different_dirs(tmp_path: Path) -> None: # Create python3 in dirA and python in dirB; PATH is dirA:dirB. diff --git a/plugins/python/tests/test_pyright_session.py b/plugins/python/tests/test_pyright_session.py index 9eaee502..1f9ca873 100644 --- a/plugins/python/tests/test_pyright_session.py +++ b/plugins/python/tests/test_pyright_session.py @@ -4492,10 +4492,13 @@ def write_frame(message): with PyrightSession( tmp_path, executable=str(script), - env={"CONFIG_MARKER": str(marker), "LOOMWEAVE_PYTHON_INTERPRETER": ""}, + env={ + "CONFIG_MARKER": str(marker), + "LOOMWEAVE_PYTHON_INTERPRETER": str(fake_python), + }, init_timeout_secs=1.0, ) as session: - assert session.interpreter.source == "dotvenv" + assert session.interpreter.source == "override" result = session.resolve_calls(module, ["python:function:demo.caller"]) python_section = json.loads(marker.read_text()) @@ -4664,10 +4667,10 @@ def test_interpreter_is_announced_once_per_session_not_once_per_spawn( with PyrightSession( tmp_path, executable=str(script), - env={"LOOMWEAVE_PYTHON_INTERPRETER": ""}, + env={"LOOMWEAVE_PYTHON_INTERPRETER": str(fake_python)}, init_timeout_secs=5.0, ) as session: - assert session.interpreter.source == "dotvenv" + assert session.interpreter.source == "override" session.resolve_calls(module, ["python:function:demo.caller"]) first = capsys.readouterr().err # A killed process forces a second, real `_spawn_and_initialize`. @@ -4678,7 +4681,7 @@ def test_interpreter_is_announced_once_per_session_not_once_per_spawn( announcements = [line for line in first.splitlines() if "pyright interpreter" in line] assert len(announcements) == 1, first assert str(fake_python) in announcements[0] - assert "source=dotvenv" in announcements[0] + assert "source=override" in announcements[0] assert "pinned=True" in announcements[0] # Non-vacuity FIRST: without a genuine second spawn the assertion below is # free, and a failure there would point at the wrong thing. diff --git a/plugins/python/tests/test_server.py b/plugins/python/tests/test_server.py index acfb8fe2..5a0cf679 100644 --- a/plugins/python/tests/test_server.py +++ b/plugins/python/tests/test_server.py @@ -931,7 +931,7 @@ def close(self) -> None: assert stats["pyright_init_latency_total_ms"] == 4321 -def test_initialize_discovers_and_advertises_the_project_interpreter( +def test_initialize_does_not_advertise_a_repository_interpreter( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: fake_python = tmp_path / ".venv" / "bin" / "python" @@ -939,6 +939,9 @@ def test_initialize_discovers_and_advertises_the_project_interpreter( fake_python.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") fake_python.chmod(0o755) monkeypatch.delenv("LOOMWEAVE_PYTHON_INTERPRETER", raising=False) + monkeypatch.delenv("VIRTUAL_ENV", raising=False) + monkeypatch.delenv("CONDA_PREFIX", raising=False) + monkeypatch.setenv("PATH", "") state = server_module.ServerState() response = server_module.handle_initialize( @@ -946,12 +949,12 @@ def test_initialize_discovers_and_advertises_the_project_interpreter( ) assert response["capabilities"]["python_interpreter"] == { - "path": str(fake_python.resolve()), - "source": "dotvenv", - "pinned": True, + "path": None, + "source": "none", + "pinned": False, } assert state.interpreter is not None - assert state.interpreter.pinned + assert not state.interpreter.pinned def test_analyze_file_hands_the_discovered_interpreter_to_pyright(