diff --git a/src/metaseed/specs/persistence.py b/src/metaseed/specs/persistence.py index 170fd1c..c0f2abd 100644 --- a/src/metaseed/specs/persistence.py +++ b/src/metaseed/specs/persistence.py @@ -12,16 +12,39 @@ import shutil from collections.abc import Callable +from pathlib import Path from typing import TYPE_CHECKING, Any from metaseed.specs.builder import SpecBuilder if TYPE_CHECKING: - from pathlib import Path - from metaseed.specs.schema import ProfileSpec +def _specs_subpath(*parts: str) -> Path: + """Resolve a path under the custom-specs dir, rejecting escapes. + + ``name`` and ``version`` components flow from user/model input into + filesystem paths (and, on delete, into ``shutil.rmtree``). A component like + ``../../.config/autostart`` or an absolute path would otherwise escape the + specs directory. Resolve the full path and require it to stay within the + (resolved) base; also reject empty components. + + Raises: + ValueError: If any component is empty or the result escapes the base. + """ + base = get_custom_specs_dir() + for part in parts: + if not part or not str(part).strip(): + raise ValueError("path component cannot be empty") + candidate = base.joinpath(*parts).resolve() + if not candidate.is_relative_to(base.resolve()): + raise ValueError( + f"path component escapes the specs directory: {parts!r}" + ) + return candidate + + def get_custom_specs_dir() -> Path: """Return the directory for user-created specs (created if needed). @@ -65,7 +88,7 @@ def save_spec(spec: ProfileSpec, name: str | None = None) -> Path: f"Please choose a different name." ) - version_dir = get_custom_specs_dir() / safe_name / spec.version + version_dir = _specs_subpath(safe_name, spec.version) version_dir.mkdir(parents=True, exist_ok=True) profile_path = version_dir / "profile.yaml" @@ -149,12 +172,13 @@ def delete_user_spec(name: str, version: str | None = None) -> bool: if not loader.is_user_defined(name): raise ValueError(f"Cannot delete built-in specification: {name}") - profile_dir = get_custom_specs_dir() / name + # Containment-check before any rmtree: name/version must not escape the base. + profile_dir = _specs_subpath(name) if not profile_dir.exists(): return False if version: - version_dir = profile_dir / version + version_dir = _specs_subpath(name, version) if not version_dir.exists(): return False shutil.rmtree(version_dir) diff --git a/tests/test_specs/test_persistence_security.py b/tests/test_specs/test_persistence_security.py new file mode 100644 index 0000000..cf84d45 --- /dev/null +++ b/tests/test_specs/test_persistence_security.py @@ -0,0 +1,102 @@ +"""Security regression tests: spec name/version must not escape the specs dir. + +``save_spec`` interpolated ``spec.version`` and ``delete_user_spec`` interpolated +``name``/``version`` into filesystem paths without sanitization — a crafted value +(``../../.config/autostart``, an absolute path) escaped the custom-specs directory +(a chosen-directory write, and a destructive ``rmtree`` on delete). The fix routes +both through ``_specs_subpath`` (resolve + containment check). +""" + +from __future__ import annotations + +import pytest + +from metaseed.specs import persistence +from metaseed.specs.loader import SpecLoader + + +@pytest.fixture +def specs_base(tmp_path, monkeypatch): + base = tmp_path / "custom_specs" + base.mkdir() + monkeypatch.setattr(persistence, "get_custom_specs_dir", lambda: base) + return base + + +def _spec(): + spec = SpecLoader(profile="miappe").load_profile(version="1.2", profile="miappe") + spec.name = "mytestprofile" + return spec + + +class TestSaveSpecTraversal: + def test_relative_traversal_version_blocked(self, specs_base, tmp_path): + spec = _spec() + spec.version = "../../outside_evil" + with pytest.raises(ValueError): + persistence.save_spec(spec, name="mytestprofile") + assert not (tmp_path / "outside_evil").exists() + + def test_absolute_version_blocked(self, specs_base, tmp_path): + spec = _spec() + # An absolute path outside the specs base must be rejected. + spec.version = str(tmp_path / "evil_abs") + with pytest.raises(ValueError): + persistence.save_spec(spec, name="mytestprofile") + + def test_empty_version_blocked(self, specs_base): + spec = _spec() + spec.version = " " + with pytest.raises(ValueError): + persistence.save_spec(spec, name="mytestprofile") + + def test_valid_version_saves_within_base(self, specs_base): + spec = _spec() + spec.version = "1.0" + path = persistence.save_spec(spec, name="mytestprofile") + assert path.is_relative_to(specs_base) + assert path.name == "profile.yaml" + assert path.read_text(encoding="utf-8") + + +class TestDeleteSpecTraversal: + def test_version_traversal_does_not_rmtree_outside( + self, specs_base, tmp_path, monkeypatch + ): + # A sibling dir outside the base that must survive. + victim = tmp_path / "victim" + victim.mkdir() + (victim / "keep.txt").write_text("x") + # Create the real profile dir so delete reaches the version check. + spec = _spec() + spec.version = "1.0" + persistence.save_spec(spec, name="mytestprofile") + # Force the user-defined gate so the containment guard is what fires. + monkeypatch.setattr( + "metaseed.specs.loader.SpecLoader.is_user_defined", lambda self, name: True + ) + with pytest.raises(ValueError): + persistence.delete_user_spec("mytestprofile", "../../victim") + assert victim.exists() and (victim / "keep.txt").exists() + + def test_name_traversal_does_not_rmtree_outside( + self, specs_base, tmp_path, monkeypatch + ): + victim = tmp_path / "victim2" + victim.mkdir() + (victim / "keep.txt").write_text("x") + monkeypatch.setattr( + "metaseed.specs.loader.SpecLoader.is_user_defined", lambda self, name: True + ) + with pytest.raises(ValueError): + persistence.delete_user_spec("../../victim2") + assert victim.exists() and (victim / "keep.txt").exists() + + def test_valid_delete_roundtrip(self, specs_base, monkeypatch): + spec = _spec() + spec.version = "1.0" + persistence.save_spec(spec, name="mytestprofile") + monkeypatch.setattr( + "metaseed.specs.loader.SpecLoader.is_user_defined", lambda self, name: True + ) + assert persistence.delete_user_spec("mytestprofile", "1.0") is True