diff --git a/CHANGELOG.md b/CHANGELOG.md index 481a64af..88795f46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -402,6 +402,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Markdown import followed filesystem links even though Markdown export already refused to overwrite them** (#851, follow-up to #765, #786) by @SaurabhScripts + - `AgentMemory._read_markdown_path()` now rejects symlink files, broken symlinks, symlinked directories, Windows directory junctions, and other Windows reparse points supplied directly; linked entries discovered inside an otherwise valid directory are safely skipped, preserving the current directory-import contract + - `_read_markdown_file_content()` re-checks the file and parent directory immediately before and after opening, uses `O_NOFOLLOW` where available, and verifies the resulting descriptor is a regular file via `fstat`/`S_ISREG`, so link swaps are rejected rather than silently followed + - Junction detection uses `os.path.isjunction()` where available and falls back to the Windows reparse-point file attribute on older Python versions; export applies the same link check before replacing a Markdown file + - Documented the import restriction in `docs/reference/context.md`; added 11 tests to `tests/context/test_agent_memory_markdown.py` covering file/directory/broken-symlink rejection, simulated open races, mocked and real Windows junctions, and the reparse-point fallback + - Any additional review follow-up commits land in this same PR/entry rather than as a separate changelog item + - **`PipelineWithProvenance` raised `ModuleNotFoundError` on import and `AttributeError` on `.run()`** (#858, closes #858) by @Karunasagar12 - `from .pipeline import Pipeline` failed because `semantica/pipeline/pipeline.py` does not exist; corrected to `from .pipeline_builder import Pipeline` - `.run()` called `self._pipeline.run()` on the `Pipeline` dataclass, which has no such method; replaced with `self._engine.execute_pipeline(self._pipeline, ...)` delegating to `ExecutionEngine` diff --git a/docs/reference/context.md b/docs/reference/context.md index 18950e9e..02ac9afc 100644 --- a/docs/reference/context.md +++ b/docs/reference/context.md @@ -625,8 +625,10 @@ malformed or duplicate fields before changing memory, and re-importing unchanged files is idempotent. Memory-local `entities` and `relationships` are preserved as provenance but are not applied to `ContextGraph` by Markdown import. Use a dedicated export directory: matching files are overwritten, but unrelated or stale Markdown -files are not deleted automatically. Export refuses to overwrite symbolic links and -uses atomic file replacement. Timestamp offsets are preserved in Markdown and +files are not deleted automatically. Export refuses to overwrite filesystem links and +uses atomic file replacement; import also refuses symlinks, Windows directory +junctions, and other Windows reparse points. +Timestamp offsets are preserved in Markdown and normalized to UTC only for comparisons, so aware and local-naive records can be queried together safely. Vector-store writes are deferred until the in-memory import commits; adapter synchronization remains best-effort and logs failures. diff --git a/semantica/context/_markdown_filesystem.py b/semantica/context/_markdown_filesystem.py new file mode 100644 index 00000000..70872f85 --- /dev/null +++ b/semantica/context/_markdown_filesystem.py @@ -0,0 +1,32 @@ +"""Filesystem safety helpers for human-editable Markdown persistence.""" + +import os +import stat +from pathlib import Path +from typing import Optional + + +def is_filesystem_link(path: Path) -> bool: + """Return whether *path* is a symlink, junction, or Windows reparse point.""" + if path.is_symlink(): + return True + + isjunction = getattr(os.path, "isjunction", None) + if isjunction is not None and isjunction(path): + return True + + try: + attributes = getattr(os.lstat(path), "st_file_attributes", 0) + except (FileNotFoundError, NotADirectoryError): + return False + + reparse_point = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0x400) + return bool(attributes & reparse_point) + + +def find_filesystem_link(path: Path) -> Optional[Path]: + """Return the first linked component in *path*, including its ancestors.""" + for candidate in (path, *path.parents): + if is_filesystem_link(candidate): + return candidate + return None diff --git a/semantica/context/agent_memory.py b/semantica/context/agent_memory.py index 38dd122c..def79ee3 100644 --- a/semantica/context/agent_memory.py +++ b/semantica/context/agent_memory.py @@ -77,6 +77,7 @@ import yaml from ..utils.logging import get_logger from ..utils.progress_tracker import get_progress_tracker from ..utils.types import EntityDict, RelationshipDict +from ._markdown_filesystem import find_filesystem_link class _UniqueKeySafeLoader(yaml.SafeLoader): @@ -1742,9 +1743,10 @@ class AgentMemory: @staticmethod def _write_markdown_file(file_path: Path, document: str) -> None: """Atomically replace a Markdown file without following output symlinks.""" - if file_path.is_symlink(): + if find_filesystem_link(file_path) is not None: raise ValueError( - f"Refusing to overwrite Markdown symbolic link: {file_path}" + "Refusing to overwrite Markdown symbolic link or junction: " + f"{file_path}" ) temporary_path = None @@ -1867,7 +1869,8 @@ class AgentMemory: if "\n" not in data and "\r" not in data: candidate = Path(data) try: - candidate_exists = candidate.exists() + candidate_is_link = find_filesystem_link(candidate) is not None + candidate_exists = candidate_is_link or candidate.exists() except OSError as exc: error_message = ( "Failed to inspect possible Markdown import " @@ -1909,62 +1912,71 @@ class AgentMemory: return memories def _read_markdown_file_content(self, file_path: Path) -> str: - if file_path.is_symlink(): - raise ValueError(f"Symlink Markdown import paths are rejected: {file_path}") + if find_filesystem_link(file_path) is not None: + raise ValueError( + "Symlink Markdown import paths are rejected; symbolic links and " + f"junctions are unsafe: {file_path}" + ) flags = os.O_RDONLY - if hasattr(os, "O_NOFOLLOW"): - # On POSIX, O_NOFOLLOW makes os.open() fail with ELOOP if the - # final path component is a symlink, atomically closing the TOCTOU - # window between the is_symlink() check above and the open call. - # On Windows, O_NOFOLLOW is not available; the is_symlink() pre-check - # above is the only symlink defense and remains vulnerable to a narrow - # race. The fstat()/S_ISREG guard below still rejects special files - # (FIFOs, devices) on both platforms. - flags |= os.O_NOFOLLOW + nofollow_flag = getattr(os, "O_NOFOLLOW", 0) + flags |= nofollow_flag try: fd = os.open(str(file_path), flags) except OSError as exc: - if exc.errno == getattr(errno, "ELOOP", None): + if ( + (nofollow_flag and exc.errno == errno.ELOOP) + or find_filesystem_link(file_path) is not None + ): raise ValueError( - f"Symlink Markdown import paths are rejected: {file_path}" + "Symlink Markdown import paths are rejected; symbolic links " + f"and junctions are unsafe: {file_path}" ) from exc raise try: - stat_res = os.fstat(fd) - if not stat.S_ISREG(stat_res.st_mode): + if find_filesystem_link(file_path) is not None: + raise ValueError( + "Symlink Markdown import paths are rejected; symbolic links " + f"and junctions are unsafe: {file_path}" + ) + if not stat.S_ISREG(os.fstat(fd).st_mode): raise ValueError( f"Markdown import path is not a regular file: {file_path}" ) - with open(fd, "r", encoding="utf-8", closefd=True) as f: - return f.read() - except Exception: - try: + with os.fdopen(fd, mode="r", encoding="utf-8") as source: + fd = -1 + return source.read() + finally: + if fd >= 0: os.close(fd) - except OSError: - pass - raise def _read_markdown_path(self, path: Path) -> List[Tuple[str, str]]: - if path.is_symlink(): - raise ValueError(f"Symlink Markdown import paths are rejected: {path}") + if find_filesystem_link(path) is not None: + raise ValueError( + "Symlink Markdown import paths are rejected; symbolic links and " + f"junctions are unsafe: {path}" + ) if not path.exists(): raise FileNotFoundError(f"Markdown import path does not exist: {path}") if path.is_dir(): - file_paths = sorted( - ( - file_path - for file_path in path.iterdir() - if file_path.is_file() - and not file_path.is_symlink() - and file_path.suffix.lower() in self._MARKDOWN_EXTENSIONS - ), - key=lambda file_path: (file_path.name.casefold(), file_path.name), - ) + file_paths = [] + for file_path in path.iterdir(): + if file_path.suffix.lower() not in self._MARKDOWN_EXTENSIONS: + continue + if find_filesystem_link(file_path) is not None: + continue + if file_path.is_file(): + file_paths.append(file_path) + if find_filesystem_link(path) is not None: + raise ValueError( + "Symlink Markdown import paths are rejected; symbolic links " + f"and junctions are unsafe: {path}" + ) + file_paths.sort(key=lambda item: (item.name.casefold(), item.name)) elif path.is_file(): file_paths = [path] else: diff --git a/tests/context/test_agent_memory_markdown.py b/tests/context/test_agent_memory_markdown.py index bf1ca128..c1f71301 100644 --- a/tests/context/test_agent_memory_markdown.py +++ b/tests/context/test_agent_memory_markdown.py @@ -1,7 +1,11 @@ import errno +import os +import stat +import subprocess import sys from copy import deepcopy from datetime import datetime, timedelta, timezone +from pathlib import Path from unittest.mock import MagicMock, patch import pytest @@ -56,6 +60,25 @@ def markdown_document(frontmatter, body=""): return f"---\n{yaml_text}---\n\n{body}" +def _require_symlink_support(tmp_path): + """Skip the test if this environment cannot create symbolic links. + + Symlink creation can be unavailable even on POSIX (e.g. restricted + containers) and commonly requires elevated privilege or Developer Mode + on Windows. Probe actual capability instead of assuming based on + platform, so these tests still run wherever symlinks genuinely work. + """ + probe_target = tmp_path / ".symlink_probe_target" + probe_link = tmp_path / ".symlink_probe_link" + probe_target.write_text("", encoding="utf-8") + try: + probe_link.symlink_to(probe_target) + except OSError as exc: + pytest.skip(f"environment cannot create symbolic links: {exc}") + probe_link.unlink() + probe_target.unlink() + + def required_frontmatter(memory_id="mem_test", **overrides): frontmatter = { "id": memory_id, @@ -707,6 +730,151 @@ def test_markdown_string_path_inspection_errors_are_actionable(): assert exc_info.value.__cause__ is original_error +@pytest.mark.parametrize("use_string_path", [False, True]) +def test_markdown_import_rejects_symlinked_file(tmp_path, use_string_path): + _require_symlink_support(tmp_path) + outside = tmp_path / "outside.md" + outside.write_text( + markdown_document(required_frontmatter(), "Do not import"), + encoding="utf-8", + ) + source = tmp_path / "memory.md" + source.symlink_to(outside) + payload = str(source) if use_string_path else source + memory = AgentMemory() + + with pytest.raises(ValueError, match="symbolic link"): + memory.import_data(payload, format="markdown") + + assert memory.count() == 0 + + +@pytest.mark.parametrize("use_string_path", [False, True]) +def test_markdown_import_rejects_broken_symlink(tmp_path, use_string_path): + _require_symlink_support(tmp_path) + source = tmp_path / "missing-memory.md" + source.symlink_to(tmp_path / "missing-target.md") + payload = str(source) if use_string_path else source + memory = AgentMemory() + + with pytest.raises(ValueError, match="symbolic link"): + memory.import_data(payload, format="markdown") + + assert memory.count() == 0 + + +@pytest.mark.parametrize("use_string_path", [False, True]) +def test_markdown_import_rejects_symlinked_directory(tmp_path, use_string_path): + _require_symlink_support(tmp_path) + outside = tmp_path / "outside" + outside.mkdir() + (outside / "memory.md").write_text( + markdown_document(required_frontmatter(), "Do not import"), + encoding="utf-8", + ) + source = tmp_path / "memory-export" + source.symlink_to(outside, target_is_directory=True) + payload = str(source) if use_string_path else source + memory = AgentMemory() + + with pytest.raises(ValueError, match="symbolic link"): + memory.import_data(payload, format="markdown") + + assert memory.count() == 0 + + +def test_markdown_import_skips_symlinked_file_in_directory(tmp_path): + _require_symlink_support(tmp_path) + outside = tmp_path / "outside.md" + outside.write_text( + markdown_document(required_frontmatter(), "Do not import"), + encoding="utf-8", + ) + source = tmp_path / "memory-export" + source.mkdir() + (source / "memory.md").symlink_to(outside) + memory = AgentMemory() + + assert memory.import_data(source, format="markdown") == 0 + assert memory.count() == 0 + + +@pytest.mark.skipif(not hasattr(os, "O_NOFOLLOW"), reason="requires O_NOFOLLOW") +def test_markdown_import_does_not_follow_symlink_raced_before_open(tmp_path): + _require_symlink_support(tmp_path) + outside = tmp_path / "outside.md" + outside.write_text( + markdown_document(required_frontmatter(), "Do not import"), + encoding="utf-8", + ) + source = tmp_path / "memory.md" + source.symlink_to(outside) + + with patch.object(Path, "is_symlink", return_value=False): + with pytest.raises(ValueError, match="symbolic link"): + AgentMemory()._read_markdown_file_content(source) + + +def test_markdown_import_rejects_windows_junction(tmp_path, monkeypatch): + source = tmp_path / "junction" + source.mkdir() + (source / "memory.md").write_text("not read", encoding="utf-8") + + monkeypatch.setattr( + os.path, + "isjunction", + lambda candidate: Path(candidate) == source, + raising=False, + ) + + with pytest.raises(ValueError, match="junction"): + AgentMemory().import_data(source, format="markdown") + + +def test_markdown_import_rejects_windows_reparse_point_fallback(tmp_path, monkeypatch): + source = tmp_path / "reparse-point" + source.mkdir() + real_lstat = os.lstat + + class ReparseStat: + st_file_attributes = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0x400) + + monkeypatch.delattr(os.path, "isjunction", raising=False) + monkeypatch.setattr(Path, "is_symlink", lambda self: False) + monkeypatch.setattr( + os, + "lstat", + lambda candidate: ( + ReparseStat() if Path(candidate) == source else real_lstat(candidate) + ), + ) + + with pytest.raises(ValueError, match="junction"): + AgentMemory().import_data(source, format="markdown") + + +@pytest.mark.skipif(os.name != "nt", reason="requires Windows junctions") +def test_markdown_import_rejects_real_windows_junction(tmp_path): + outside = tmp_path / "outside" + outside.mkdir() + (outside / "memory.md").write_text("not read", encoding="utf-8") + source = tmp_path / "junction" + result = subprocess.run( + ["cmd.exe", "/c", "mklink", "/J", str(source), str(outside)], + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0: + pytest.skip(f"could not create Windows junction: {result.stderr}") + + try: + with pytest.raises(ValueError, match="junction"): + AgentMemory().import_data(source, format="markdown") + finally: + os.rmdir(source) + + def test_legacy_dict_import_behavior_is_unchanged(): memory = AgentMemory() data = { @@ -751,6 +919,7 @@ def test_markdown_import_file_open_security_rejects_symlink(tmp_path): with pytest.raises(ValueError, match="Symlink Markdown import paths are rejected"): memory._read_markdown_file_content(symlink_file) + def test_markdown_import_public_api_rejects_symlink(tmp_path): """ import_data(..., format="markdown") must propagate the symlink rejection