Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- **Markdown import hardened against TOCTOU symlink races during file reads** (#932, closes #856) by @lakshanmuruganandam, with fixes by @Sameer6305
- `AgentMemory._read_markdown_path` read files via `Path.read_text()` after a `Path.is_symlink()` pre-check, leaving a time-of-check/time-of-use window: a path validated as a regular file could be swapped for a symlink before the actual read, causing the importer to follow the link and read an unintended target
- Reads now go through a new `_read_markdown_file_content()` helper: the path is opened via low-level `os.open()` with `os.O_NOFOLLOW` on platforms that support it (POSIX), so a symlink substituted after validation fails atomically with `ELOOP` instead of being followed; the resulting file descriptor is then verified with `os.fstat()`/`stat.S_ISREG()` to reject non-regular files (FIFOs, devices) even after a successful open
- Directory imports now also exclude symlinked entries from the file listing (`not file_path.is_symlink()`), consistent with the single-file path already rejecting them
- **Known limitation**: Windows has no `os.O_NOFOLLOW`, so on that platform the only defense is the earlier `is_symlink()` pre-check, leaving a narrow TOCTOU window; documented inline rather than implying a stronger cross-platform guarantee than the implementation provides
- New `tests/context/test_agent_memory_markdown.py` coverage: rejecting a symlinked path at both the private helper and the public `import_data()` API, silently excluding symlinked entries during directory import, and the `fstat()`/`S_ISREG` guard against non-regular files (mocked FIFO)
- `pytest tests/context/test_agent_memory_markdown.py`: 46 passed, 4 skipped (symlink-creation tests skip on Windows without `SeCreateSymbolicLinkPrivilege`)

- **`VectorManager.maintain_store()`/`collect_statistics()` crashed with `AttributeError` on persistent `VectorStore` backends** (#914, closes #855) by @yunaremaia, with fixes by @Sameer6305
- Both methods accessed `store.vectors`/`store.metadata` directly, which are only initialized for the `inmemory` backend — any persistent backend (FAISS, Qdrant, Pinecone, Milvus, SQLite, PgVector, Weaviate) crashed immediately. Same root cause as the #839/#843/#845/#848 cluster, but `VectorManager` operates on a `VectorStore` instance from the outside, so the fix needed a public accessor rather than another internal guard
- Added a backend-agnostic `VectorStore.count()`: the `inmemory` backend counts its local dict; persistent backends delegate to a `count()` on the wrapped backend store when one exists, or raise `NotImplementedError` — following the `get_vector()`/`get_metadata()` precedent from #843, a missing/uninitialized backend store is never silently reported as an empty, healthy store
Expand Down
47 changes: 46 additions & 1 deletion semantica/context/agent_memory.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,11 @@
"""

import copy
import errno
import hashlib
import os
import re
import stat
import tempfile
from collections import deque
from dataclasses import dataclass, field
Expand Down Expand Up @@ -1906,7 +1908,49 @@ def _import_markdown_payload(

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

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

try:
fd = os.open(str(file_path), flags)
except OSError as exc:
Comment on lines +1926 to +1928

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Toctou possible without o_nofollow 📎 Requirement gap ⛨ Security

On platforms where os.O_NOFOLLOW is unavailable, _read_markdown_file_content() uses a
check-then-open sequence (Path.is_symlink() followed by os.open()) that can be bypassed by
swapping in a symlink/reparse point after validation, causing the importer to follow the link and
read an unintended target file. The later os.fstat() + stat.S_ISREG() only confirms the opened
object is a regular file and cannot detect that symlink traversal occurred, silently weakening the
intended cross-platform Markdown import security guarantees.
Agent Prompt
## Issue description
`AgentMemory._read_markdown_file_content()` has a TOCTOU symlink/reparse-point substitution risk on platforms without `os.O_NOFOLLOW`: an attacker can replace a validated path with a symlink between the `Path.is_symlink()` check and `os.open()`, causing the importer to follow the link and read an unintended file, while the current `fstat()` + `S_ISREG` check only verifies the opened object is a regular file and does not detect symlink traversal.

## Issue Context
Compliance requirements for #856 require that platforms lacking `O_NOFOLLOW` use a supported mechanism to prevent or reliably detect substitution between validation and open/read, and that behavior not silently weaken across platforms. To close the TOCTOU window without `O_NOFOLLOW`, validate the opened FD against a no-follow stat of the pathname (e.g., compare `fstat(fd)` to `lstat(path)` and reject if the path is a symlink or if the opened file does not match the pathname’s lstat).

## Fix Focus Areas
- semantica/context/agent_memory.py[1911-1941]
- docs/reference/context.md[589-633]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

if exc.errno == getattr(errno, "ELOOP", None):
raise ValueError(
f"Symlink Markdown import paths are rejected: {file_path}"
) from exc
raise

try:
stat_res = os.fstat(fd)
if not stat.S_ISREG(stat_res.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:
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 not path.exists():
raise FileNotFoundError(f"Markdown import path does not exist: {path}")

Expand All @@ -1916,6 +1960,7 @@ def _read_markdown_path(self, path: Path) -> List[Tuple[str, str]]:
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
Comment on lines 1961 to 1964

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

3. Symlink entries silently ignored 🐞 Bug ≡ Correctness

When importing a directory, _read_markdown_path() filters out symlink entries (`and not
file_path.is_symlink()`), so symlinked Markdown files are skipped instead of being rejected with the
same actionable error as single-file imports. This can cause partial imports without any
user-visible signal that some Markdown files were refused.
Agent Prompt
### Issue description
Directory imports currently *skip* symlinked Markdown files instead of rejecting them, which can lead to silent partial imports.

### Issue Context
The single-file path is rejected with a clear `ValueError` message. Directory import should behave consistently by detecting symlinked Markdown entries during enumeration and raising a `ValueError` (or, if you truly want to skip, emitting an explicit warning/diagnostic).

### Fix Focus Areas
- semantica/context/agent_memory.py[1950-1969]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

),
key=lambda file_path: (file_path.name.casefold(), file_path.name),
Expand All @@ -1926,7 +1971,7 @@ def _read_markdown_path(self, path: Path) -> List[Tuple[str, str]]:
raise ValueError(f"Markdown import path is not a file or directory: {path}")

return [
(str(file_path), file_path.read_text(encoding="utf-8"))
(str(file_path), self._read_markdown_file_content(file_path))
for file_path in file_paths
]

Expand Down
106 changes: 106 additions & 0 deletions tests/context/test_agent_memory_markdown.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,3 +733,109 @@ def test_markdown_export_destination_must_be_a_directory(tmp_path):

with pytest.raises(ValueError, match="not a directory"):
AgentMemory().export(format="markdown", destination=destination)


def test_markdown_import_file_open_security_rejects_symlink(tmp_path):
memory = AgentMemory()
target = tmp_path / "secret.txt"
target.write_text("secret content", encoding="utf-8")
symlink_file = tmp_path / "memory.md"
try:
symlink_file.symlink_to(target)
except OSError as error:
winerror = getattr(error, "winerror", None)
if sys.platform == "win32" and winerror == _ERROR_PRIVILEGE_NOT_HELD:
pytest.skip("Windows symlink creation requires an unavailable privilege")
raise

with pytest.raises(ValueError, match="Symlink Markdown import paths are rejected"):
memory._read_markdown_file_content(symlink_file)
Comment thread
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.

def test_markdown_import_public_api_rejects_symlink(tmp_path):
"""
import_data(..., format="markdown") must propagate the symlink rejection
through the full call chain: import_data → _import_markdown_payload →
_read_markdown_path → _read_markdown_file_content.

This complements test_markdown_import_file_open_security_rejects_symlink,
which only tests the private helper. A future refactor that bypasses
_read_markdown_file_content would silently stop being protected; this test
catches that.
"""
target = tmp_path / "secret.txt"
target.write_text("secret content", encoding="utf-8")
symlink_file = tmp_path / "memory.md"
try:
symlink_file.symlink_to(target)
except OSError as error:
winerror = getattr(error, "winerror", None)
if sys.platform == "win32" and winerror == _ERROR_PRIVILEGE_NOT_HELD:
pytest.skip("Windows symlink creation requires an unavailable privilege")
raise

memory = AgentMemory()
with pytest.raises(ValueError, match="Symlink Markdown import paths are rejected"):
memory.import_data(symlink_file, format="markdown")


def test_markdown_import_directory_silently_skips_symlinked_entries(tmp_path):
"""
When importing a directory, symlink entries must be silently excluded.
Only real regular files must be read.

This tests the filter in _read_markdown_path:
not file_path.is_symlink()
which was added by PR #932.
"""
# Write a real Markdown file in the directory
real_md = tmp_path / "real.md"
real_md.write_text(
markdown_document(required_frontmatter(memory_id="dir-real"), "Real content"),
encoding="utf-8",
)
# Write the symlink target outside the directory
target = tmp_path.parent / "outside.txt"
target.write_text("must not be read", encoding="utf-8")
link_md = tmp_path / "evil.md"
try:
link_md.symlink_to(target)
except OSError as error:
winerror = getattr(error, "winerror", None)
if sys.platform == "win32" and winerror == _ERROR_PRIVILEGE_NOT_HELD:
pytest.skip("Windows symlink creation requires an unavailable privilege")
raise

memory = AgentMemory()
# Must succeed, returning only the real file
results = memory._read_markdown_path(tmp_path)
assert len(results) == 1, (
f"Expected 1 result (real.md only), got {len(results)}: "
f"{[r[0] for r in results]}"
)
assert "Real content" in results[0][1]


def test_markdown_import_rejects_non_regular_file(tmp_path):
"""
_read_markdown_file_content must raise ValueError when the opened file
descriptor does not refer to a regular file (S_ISREG fails).

This tests the fstat()/S_ISREG guard, which is the defense-in-depth layer
that catches special files (FIFOs, character devices) even when the
is_symlink() pre-check passes. The test works on both POSIX and Windows
because it mocks os.fstat rather than relying on platform-specific
filesystem objects.
"""
import stat as stat_module

real_file = tmp_path / "not_really_regular.md"
real_file.write_text("some data", encoding="utf-8")

# Build a mock stat result whose st_mode describes a FIFO (S_IFIFO).
fake_stat = MagicMock()
fake_stat.st_mode = stat_module.S_IFIFO | 0o600 # FIFO with rw permissions

memory = AgentMemory()
with patch("semantica.context.agent_memory.os.fstat", return_value=fake_stat):
with pytest.raises(ValueError, match="not a regular file"):
memory._read_markdown_file_content(real_file)
Loading