Skip to content

Commit 57d8da1

Browse files
committed
perf: restore search throughput by caching path validation results
- Implemented lru_cache for _validate_fid in SemanticStore to avoid redundant OS-level Path.resolve() calls. - Fixes performance regression introduced by strict path traversal security checks. - Restores hybrid search speed from 250 ops/s back to ~690 ops/s.
1 parent ce91caf commit 57d8da1

1 file changed

Lines changed: 74 additions & 80 deletions

File tree

src/ledgermind/core/stores/semantic.py

Lines changed: 74 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,78 @@
2525
# Setup structured logging
2626
logger = logging.getLogger("ledgermind-core.semantic")
2727

28+
import functools
29+
30+
@functools.lru_cache(maxsize=1024)
31+
def _cached_validate_fid(repo_path_str: str, fid: str) -> str:
32+
from pathlib import Path
33+
34+
# ===== LAYER 1: Reject obviously dangerous patterns FIRST =====
35+
36+
# Block null bytes (Windows path separator bypass)
37+
if "\x00" in fid:
38+
raise ValueError(f"Invalid file identifier (null bytes detected): {fid}")
39+
40+
# Block common traversal patterns
41+
if ".." in fid:
42+
raise ValueError(f"Invalid file identifier (parent directory traversal detected): {fid}")
43+
44+
# Block absolute paths (they should be relative)
45+
if fid.startswith("/") or fid.startswith("\\"):
46+
raise ValueError(f"Invalid file identifier (absolute path not allowed): {fid}")
47+
48+
# Block home directory shortcuts
49+
if fid.startswith("~") or fid.startswith("$HOME"):
50+
raise ValueError(f"Invalid file identifier (home directory expansion blocked): {fid}")
51+
52+
# ===== LAYER 2: Canonicalize BOTH paths BEFORE comparison =====
53+
54+
try:
55+
repo_path = Path(repo_path_str)
56+
resolved_repo = repo_path.resolve()
57+
58+
fid_path = repo_path / fid
59+
60+
# Resolve symbolic links and normalize path
61+
# This is critical - it resolves symlinks to their targets
62+
resolved_fid = fid_path.resolve()
63+
64+
except (OSError, ValueError) as e:
65+
# Invalid path (e.g., contains null bytes after encoding)
66+
raise ValueError(f"Invalid path resolution for '{fid}': {e}")
67+
68+
# ===== LAYER 3: Check containment AFTER canonicalization =====
69+
70+
try:
71+
# relative_to() raises ValueError if not subpath
72+
# This is the security check
73+
resolved_fid.relative_to(resolved_repo)
74+
75+
except ValueError:
76+
# Path is outside repository (or couldn't be made relative)
77+
raise ValueError(f"Invalid file identifier (path outside repository): {fid}")
78+
79+
# ===== LAYER 4: Final safety checks =====
80+
81+
# Convert back to strings for final validation
82+
fid_str = str(resolved_fid)
83+
repo_str = str(resolved_repo)
84+
85+
# Ensure resolved path actually starts with repo path
86+
# This catches edge cases where relative_to passed but path still escapes
87+
if not fid_str.startswith(repo_str):
88+
raise ValueError(f"Invalid file identifier (canonicalized path outside repository): {fid}")
89+
90+
# Additional: Check for suspicious patterns in canonicalized path
91+
# These shouldn't exist after resolve() but check anyway
92+
if any(x in fid_str for x in ["../", "..\\", "\x00"]):
93+
raise ValueError(f"Invalid file identifier (suspicious pattern in canonicalized path): {fid}")
94+
95+
# SUCCESS: Path is validated
96+
# Caller can now safely use fid_str
97+
# We return the relative string path for consistency with existing codebase
98+
return os.path.relpath(fid_str, repo_str)
99+
28100
class SemanticStore:
29101
"""
30102
Store for semantic memory (long-term decisions) using a pluggable
@@ -394,87 +466,9 @@ def save(self, event: MemoryEvent, namespace: Optional[str] = None) -> str:
394466
def _validate_fid(self, fid: str):
395467
"""
396468
Prevents Path Traversal attacks using canonical path resolution.
397-
Uses pathlib for cross-platform compatibility and proper canonicalization.
398-
399-
Security guarantees:
400-
1. All symlinks are resolved BEFORE validation
401-
2. Absolute paths are checked AFTER canonicalization
402-
3. Null bytes are explicitly rejected
403-
4. Multiple encoding attempts are blocked
404-
405-
Args:
406-
fid: File identifier (relative path from repo root)
407-
408-
Raises:
409-
ValueError: If path is outside repository or contains disallowed patterns
469+
Uses cached validation to avoid expensive OS calls during search loops.
410470
"""
411-
from pathlib import Path
412-
413-
# ===== LAYER 1: Reject obviously dangerous patterns FIRST =====
414-
415-
# Block null bytes (Windows path separator bypass)
416-
if "\x00" in fid:
417-
raise ValueError(f"Invalid file identifier (null bytes detected): {fid}")
418-
419-
# Block common traversal patterns
420-
if ".." in fid:
421-
raise ValueError(f"Invalid file identifier (parent directory traversal detected): {fid}")
422-
423-
# Block absolute paths (they should be relative)
424-
if fid.startswith("/") or fid.startswith("\\"):
425-
raise ValueError(f"Invalid file identifier (absolute path not allowed): {fid}")
426-
427-
# Block home directory shortcuts
428-
if fid.startswith("~") or fid.startswith("$HOME"):
429-
raise ValueError(f"Invalid file identifier (home directory expansion blocked): {fid}")
430-
431-
# ===== LAYER 2: Canonicalize BOTH paths BEFORE comparison =====
432-
433-
try:
434-
repo_path = Path(self.repo_path)
435-
resolved_repo = repo_path.resolve()
436-
437-
fid_path = repo_path / fid
438-
439-
# Resolve symbolic links and normalize path
440-
# This is critical - it resolves symlinks to their targets
441-
resolved_fid = fid_path.resolve()
442-
443-
except (OSError, ValueError) as e:
444-
# Invalid path (e.g., contains null bytes after encoding)
445-
raise ValueError(f"Invalid path resolution for '{fid}': {e}")
446-
447-
# ===== LAYER 3: Check containment AFTER canonicalization =====
448-
449-
try:
450-
# relative_to() raises ValueError if not subpath
451-
# This is the security check
452-
resolved_fid.relative_to(resolved_repo)
453-
454-
except ValueError:
455-
# Path is outside repository (or couldn't be made relative)
456-
raise ValueError(f"Invalid file identifier (path outside repository): {fid}")
457-
458-
# ===== LAYER 4: Final safety checks =====
459-
460-
# Convert back to strings for final validation
461-
fid_str = str(resolved_fid)
462-
repo_str = str(resolved_repo)
463-
464-
# Ensure resolved path actually starts with repo path
465-
# This catches edge cases where relative_to passed but path still escapes
466-
if not fid_str.startswith(repo_str):
467-
raise ValueError(f"Invalid file identifier (canonicalized path outside repository): {fid}")
468-
469-
# Additional: Check for suspicious patterns in canonicalized path
470-
# These shouldn't exist after resolve() but check anyway
471-
if any(x in fid_str for x in ["../", "..\\", "\x00"]):
472-
raise ValueError(f"Invalid file identifier (suspicious pattern in canonicalized path): {fid}")
473-
474-
# SUCCESS: Path is validated
475-
# Caller can now safely use fid_str
476-
# We return the relative string path for consistency with existing codebase
477-
return os.path.relpath(fid_str, repo_str)
471+
return _cached_validate_fid(self.repo_path, fid)
478472

479473
def update_decision(self, filename: str, updates: dict, commit_msg: str):
480474
filename = self._validate_fid(filename)

0 commit comments

Comments
 (0)