Security: Path traversal/arbitrary file write risk in replay cache key handling#999
Conversation
`_path_for` builds file paths directly from `key` (`self.root / key[:2] / f"{key}.json.gz"`) without validating allowed characters. If `load()`/`save()` are called with attacker-controlled keys, `..` or path separators can escape the cache directory, enabling unintended file read/write locations and potential overwrite of arbitrary files.
Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
📝 WalkthroughWalkthroughReplay cache key handling now includes validation to ensure keys match a 64-character lowercase hex format and that resolved file paths remain contained within the cache directory. Invalid keys or paths that escape the cache root raise Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)lumibot/components/agents/replay_cache.py************* Module pylintrc ... [truncated 2543 characters] ... t.components.agents.replay_cache", Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lumibot/components/agents/replay_cache.py (1)
53-77:⚠️ Potential issue | 🟠 MajorAdd regression tests, explanatory comment, and documentation for this security fix.
The defense-in-depth approach (strict regex + post-resolution
relative_tocontainment check) correctly mitigates path traversal risk. The regex^[a-f0-9]{64}$matches the lowercase output ofhashlib.sha256(...).hexdigest()fromcompute_key, so legitimate callers are unaffected.However, per coding guidelines, this behavioral change requires:
Regression tests in the same commit exercising malicious keys (
"../etc/passwd","a"*63, uppercase hex, embedded/, NUL byte) and assertingValueError("Invalid replay cache key"), plus a happy-path test for a valid 64-char hex key.Inline comment explaining the why and invariant:
Suggested invariant comment
def _path_for(self, key: str) -> Path: + # Invariant: ``key`` must be a SHA-256 hex digest produced by ``compute_key``. + # The regex rejects path separators and ``..`` segments; the resolved-path + # containment check is defense-in-depth against symlinked roots or future + # regex relaxation. if not _KEY_RE.fullmatch(key): raise ValueError("Invalid replay cache key")
- Documentation update if cache key format is part of any public contract (e.g., docs/ or docstrings).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lumibot/components/agents/replay_cache.py` around lines 53 - 77, The change tightens AgentReplayCache key validation (see _KEY_RE and _path_for) to prevent path traversal, so add regression tests that call AgentReplayCache._path_for (or compute_key where appropriate) asserting ValueError("Invalid replay cache key") for malicious inputs ("../etc/passwd", "a"*63, uppercase hex, strings with "/", NUL byte) and a happy-path test using a 64-char lowercase hex produced by compute_key; add a short inline comment above _KEY_RE/_path_for explaining the defense-in-depth invariant (regex + post-resolution containment check against self.root) and why legitimate keys from compute_key remain valid; and if the key format is part of any public API surface, update the relevant docstring or docs to state the 64-char lowercase hex requirement.
🧹 Nitpick comments (1)
lumibot/components/agents/replay_cache.py (1)
67-77: Optional: cacheself.root.resolve()once in__init__.
self.rootis fixed after construction, soresolved_root = self.root.resolve()recomputes the same value on every_path_forcall (which is on everyload/save). Resolving once in__init__and reusing it slightly reduces per-call syscalls and makes intent clearer. Non-blocking.♻️ Proposed refactor
def __init__(self) -> None: self.root = _cache_root() / "agent_runtime" / "replay" self.root.mkdir(parents=True, exist_ok=True) + self._resolved_root = self.root.resolve() self.remote_cache = get_backtest_cache() @@ def _path_for(self, key: str) -> Path: if not _KEY_RE.fullmatch(key): raise ValueError("Invalid replay cache key") path = self.root / key[:2] / f"{key}.json.gz" - resolved_root = self.root.resolve() resolved_path = path.resolve() try: - resolved_path.relative_to(resolved_root) + resolved_path.relative_to(self._resolved_root) except ValueError as exc: raise ValueError("Invalid replay cache key") from exc return resolved_path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lumibot/components/agents/replay_cache.py` around lines 67 - 77, Cache the resolved root path once in the constructor and reuse it in _path_for to avoid repeated filesystem resolves: add an attribute (e.g., self._resolved_root = self.root.resolve()) in __init__ and then update _path_for to use that cached self._resolved_root instead of calling self.root.resolve() each time, while keeping the same validation logic (KEY_RE check, building path via self.root / key[:2] / f"{key}.json.gz", resolving the candidate path and using candidate.relative_to(self._resolved_root) to detect escapes and raise ValueError as before).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lumibot/components/agents/replay_cache.py`:
- Around line 53-77: The change tightens AgentReplayCache key validation (see
_KEY_RE and _path_for) to prevent path traversal, so add regression tests that
call AgentReplayCache._path_for (or compute_key where appropriate) asserting
ValueError("Invalid replay cache key") for malicious inputs ("../etc/passwd",
"a"*63, uppercase hex, strings with "/", NUL byte) and a happy-path test using a
64-char lowercase hex produced by compute_key; add a short inline comment above
_KEY_RE/_path_for explaining the defense-in-depth invariant (regex +
post-resolution containment check against self.root) and why legitimate keys
from compute_key remain valid; and if the key format is part of any public API
surface, update the relevant docstring or docs to state the 64-char lowercase
hex requirement.
---
Nitpick comments:
In `@lumibot/components/agents/replay_cache.py`:
- Around line 67-77: Cache the resolved root path once in the constructor and
reuse it in _path_for to avoid repeated filesystem resolves: add an attribute
(e.g., self._resolved_root = self.root.resolve()) in __init__ and then update
_path_for to use that cached self._resolved_root instead of calling
self.root.resolve() each time, while keeping the same validation logic (KEY_RE
check, building path via self.root / key[:2] / f"{key}.json.gz", resolving the
candidate path and using candidate.relative_to(self._resolved_root) to detect
escapes and raise ValueError as before).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de6f8765-824f-4ecf-9666-fc804f1987fa
📒 Files selected for processing (1)
lumibot/components/agents/replay_cache.py
1 participant
Summary
Security: Path traversal/arbitrary file write risk in replay cache key handling
Problem
Severity:
High| File:lumibot/components/agents/replay_cache.py:L58_path_forbuilds file paths directly fromkey(self.root / key[:2] / f"{key}.json.gz") without validating allowed characters. Ifload()/save()are called with attacker-controlled keys,..or path separators can escape the cache directory, enabling unintended file read/write locations and potential overwrite of arbitrary files.Solution
Validate
keystrictly (e.g.,^[a-f0-9]{64}$for SHA-256 hex), reject any path separators/dots, and enforce a post-resolution check (resolved_path.is_relative_to(self.root.resolve())) before any file operation.Changes
lumibot/components/agents/replay_cache.py(modified)Summary by CodeRabbit