Merge pull request #1665 from AmirF194/fix/memory-path-absolute-escape

fix(memory): reject absolute and escaping memory names in get_memory_file_path

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`

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

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")