diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ac7d1a1..78703962 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,8 @@ Status of the `main` branch. Changes prior to the next official version change w * Memories: - Make memory iteration follow symbolic links + - Fix: a memory name that was absolute (e.g. `/etc/cron.d/backdoor`) or contained empty path + segments could write outside the `memories` folder. * Dependencies: - Add dependency `oslex` diff --git a/src/serena/memories/memory_manager.py b/src/serena/memories/memory_manager.py index 370e3572..daa03725 100644 --- a/src/serena/memories/memory_manager.py +++ b/src/serena/memories/memory_manager.py @@ -147,11 +147,41 @@ class MemoryManager: # use a callable replacement to avoid backreference interpretation of characters in ref_new return re.subn(pattern, lambda _m: ref_new, content) + def _resolve_memory_path(self, base_dir: Path, parts: Sequence[str]) -> Path: + """ + Builds the ``*.md`` path for ``parts`` under ``base_dir``, creating any parent + subdirectories, and guarantees the result stays inside ``base_dir``. + + The containment check is a defense-in-depth backstop for :meth:`get_memory_file_path`'s + up-front segment validation: even if a crafted name slipped through, the built path must + never escape the memories sandbox (which would let an agent read/write/delete arbitrary + files). The check runs *before* any directory is created, so a rejected name cannot leave + stray directories behind either. It is deliberately *lexical* (``normpath``, no symlink + resolution): directory symlinks placed inside the memories folder are a supported way to + share memories (e.g. a monorepo symlinking each submodule's memory dir), and those must + keep resolving to their targets at I/O time. + """ + filename = f"{parts[-1]}.md" + subdir = base_dir if len(parts) == 1 else base_dir.joinpath(*parts[:-1]) + candidate = subdir / filename + base_norm = Path(os.path.normpath(base_dir)) + if not Path(os.path.normpath(candidate)).is_relative_to(base_norm): + raise ValueError(f"Memory name resolves outside the memories directory. Got: {'/'.join(parts)}") + subdir.mkdir(parents=True, exist_ok=True) + return candidate + def get_memory_file_path(self, name: str) -> Path: name = self._sanitize_name(name) parts = name.split("/") + if ".." in parts: - raise ValueError(f"Memory name cannot contain '..' segments for security reasons. Got: {name}") + raise ValueError(f"Memory name cannot contain '..' segments. Got: {name}") + + # Reject absolute names and empty path segments: pathlib discards the base directory when + # joined with an absolute path (e.g. "/etc/cron.d/backdoor" would reset to "/etc/cron.d"), + # letting a memory name escape the sandbox. A leading "/" produces an empty first segment. + if os.path.isabs(name) or "" in parts: + raise ValueError(f"Memory name cannot be absolute or contain empty path segments. Got: {name}") if self._is_global(name): if name == self.GLOBAL_TOPIC: @@ -160,26 +190,11 @@ class MemoryManager: ) # Strip "global/" prefix and resolve against global dir sub_name = name[len(self.GLOBAL_TOPIC) + 1 :] - parts = sub_name.split("/") - filename = f"{parts[-1]}.md" - if len(parts) > 1: - subdir = self._global_memory_dir / "/".join(parts[:-1]) - subdir.mkdir(parents=True, exist_ok=True) - return subdir / filename - return self._global_memory_dir / filename + return self._resolve_memory_path(self._global_memory_dir, sub_name.split("/")) # Project-local memory assert self._project_memory_dir is not None, "Project dir was not passed at initialization" - - filename = f"{parts[-1]}.md" - - if len(parts) > 1: - # Create subdirectory path - subdir = self._project_memory_dir / "/".join(parts[:-1]) - subdir.mkdir(parents=True, exist_ok=True) - return subdir / filename - - return self._project_memory_dir / filename + return self._resolve_memory_path(self._project_memory_dir, parts) def _check_write_access(self, name: str, is_tool_context: bool) -> None: # in tool context, memories can be read-only diff --git a/test/serena/test_memories_manager.py b/test/serena/test_memories_manager.py index 79be6832..158ab379 100644 --- a/test/serena/test_memories_manager.py +++ b/test/serena/test_memories_manager.py @@ -303,6 +303,52 @@ class TestListMemoriesFollowsSymlinks: assert manager.load_memory("submodule/nested/sub_b") == "# b" +class TestGetMemoryFilePathContainment: + """Regression: :meth:`MemoryManager.get_memory_file_path` must never return a path outside the + memories sandbox. An *absolute* memory name (e.g. ``/etc/cron.d/backdoor``) previously caused + ``pathlib`` to discard the base directory and reset to the absolute path, escaping + ``.serena/memories``. Since save/load/delete/move all route through this method, that let an + agent read, write, or delete arbitrary ``*.md`` files outside the sandbox. The guard must also + hold for the ``global/...`` branch, and the existing ``..`` rejection must keep working. + """ + + def test_absolute_project_memory_name_escapes_sandbox_is_rejected(self, fs_manager: MemoryManager, tmp_path) -> None: + # An absolute name pointing outside the memories dir. On the buggy code this silently + # returned a path under ``escape_target`` (outside ``memories``); the fix rejects it. + escape = str(tmp_path / "escape_target" / "backdoor") + with pytest.raises(ValueError): + fs_manager.get_memory_file_path(escape) + + def test_absolute_project_memory_name_system_path_is_rejected(self, fs_manager: MemoryManager) -> None: + with pytest.raises(ValueError): + fs_manager.get_memory_file_path("/etc/cron.d/backdoor") + + def test_absolute_global_memory_name_is_rejected(self, fs_manager: MemoryManager, tmp_path, monkeypatch) -> None: + # "global//etc/..." routes through the global branch; the empty segment / absolute sub-name + # must be rejected there too. Redirect the global dir so the test never touches the real one. + monkeypatch.setattr(fs_manager, "_global_memory_dir", tmp_path / "global") + with pytest.raises(ValueError): + fs_manager.get_memory_file_path("global//etc/cron.d/backdoor") + + def test_dotdot_segment_still_rejected(self, fs_manager: MemoryManager) -> None: + # the pre-existing guard must keep working + with pytest.raises(ValueError): + fs_manager.get_memory_file_path("../../etc/passwd") + + @pytest.mark.parametrize("name", ["notes", "topic/notes", "a/b/c/deep", "mem:topic/foo.md"]) + def test_accepted_project_names_resolve_inside_memories_dir(self, fs_manager: MemoryManager, name: str) -> None: + assert fs_manager._project_memory_dir is not None + path = fs_manager.get_memory_file_path(name) + assert path.resolve().is_relative_to(fs_manager._project_memory_dir.resolve()) + + def test_accepted_global_name_resolves_inside_global_dir(self, fs_manager: MemoryManager, tmp_path, monkeypatch) -> None: + global_dir = tmp_path / "global" + global_dir.mkdir() + monkeypatch.setattr(fs_manager, "_global_memory_dir", global_dir) + path = fs_manager.get_memory_file_path("global/topic/notes") + assert path.resolve().is_relative_to(global_dir.resolve()) + + class TestValidateReferentialIntegrity: def test_clean_report_when_all_references_resolve(self, fs_manager: MemoryManager) -> None: _write(fs_manager, "auth", "# auth notes")