fix(security): H1 — replace MD5 with SHA-256 in config/skill watchers
Both watcher.py (ConfigWatcher) and skill_loader/watcher.py (SkillsWatcher) used hashlib.md5() for file-integrity change detection. MD5 is collision-prone: a crafted config file could produce the same hash as a benign one, silently suppressing the hot-reload callback and preventing agents from picking up legitimate config changes. Replace hashlib.md5 → hashlib.sha256 in both _hash_file() methods. Update docstrings, comments, and the type-annotation comment (rel_path → md5 hex → sha256 hex). Test update: test_skills_watcher.py — rename helper _md5 → _sha256, update the hash-length assertion from 32 (MD5) to 64 (SHA-256), and rename the test from test_hash_file_returns_md5_for_existing_file to test_hash_file_returns_sha256_for_existing_file. All 25 watcher tests pass. Note: H2 (a2a_client.py timeout=None) was already fixed in Cycle 5 (timeout=httpx.Timeout(connect=30.0, read=300.0, ...)) — confirmed by code review before opening this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
9ec566ad3d
commit
7f3274391e
@ -7,7 +7,7 @@ Architecture
|
||||
------------
|
||||
``SkillsWatcher`` runs as a background asyncio task alongside the agent. It
|
||||
polls the skill directories every ``POLL_INTERVAL`` seconds (default 3 s),
|
||||
computes MD5 hashes of every file, and fires ``_reload_skill()`` when any
|
||||
computes SHA-256 hashes of every file, and fires ``_reload_skill()`` when any
|
||||
file inside a skill's folder changes.
|
||||
|
||||
``_reload_skill()`` calls ``load_skills()`` from ``skills.loader`` for the
|
||||
@ -71,7 +71,7 @@ class SkillsWatcher:
|
||||
self.config_path = config_path
|
||||
self.skill_names = list(skill_names)
|
||||
self.on_reload = on_reload
|
||||
self._hashes: dict[str, str] = {} # rel_path → md5 hex
|
||||
self._hashes: dict[str, str] = {} # rel_path → sha256 hex
|
||||
self._running = False
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
@ -103,12 +103,13 @@ class SkillsWatcher:
|
||||
|
||||
def _hash_file(self, path: Path) -> str:
|
||||
try:
|
||||
return hashlib.md5(path.read_bytes()).hexdigest()
|
||||
# H1: SHA-256 replaces MD5 for file-integrity change detection.
|
||||
return hashlib.sha256(path.read_bytes()).hexdigest()
|
||||
except OSError:
|
||||
return ""
|
||||
|
||||
def _scan(self) -> dict[str, str]:
|
||||
"""Return {relative_path: md5} for every file in watched skill dirs."""
|
||||
"""Return {relative_path: sha256} for every file in watched skill dirs."""
|
||||
hashes: dict[str, str] = {}
|
||||
root = self._skills_root()
|
||||
for skill_name in self.skill_names:
|
||||
|
||||
@ -40,8 +40,8 @@ def _write(path: Path, content: str) -> None:
|
||||
path.write_text(content, encoding="utf-8")
|
||||
|
||||
|
||||
def _md5(content: str) -> str:
|
||||
return hashlib.md5(content.encode()).hexdigest()
|
||||
def _sha256(content: str) -> str:
|
||||
return hashlib.sha256(content.encode()).hexdigest()
|
||||
|
||||
|
||||
# ============================================================================
|
||||
@ -149,7 +149,7 @@ class TestChangedSkills:
|
||||
_write(tmp_path / "skills" / "unwatched" / "SKILL.md", "v2")
|
||||
# Also add path for unwatched to new_hashes manually (shouldn't matter)
|
||||
new_hashes = w._scan()
|
||||
new_hashes["unwatched/SKILL.md"] = _md5("v2")
|
||||
new_hashes["unwatched/SKILL.md"] = _sha256("v2")
|
||||
|
||||
changed = w._changed_skills(new_hashes)
|
||||
assert "unwatched" not in changed
|
||||
@ -351,13 +351,13 @@ class TestHashFile:
|
||||
result = w._hash_file(missing)
|
||||
assert result == ""
|
||||
|
||||
def test_hash_file_returns_md5_for_existing_file(self, tmp_path):
|
||||
"""_hash_file returns a non-empty hex digest for a readable file."""
|
||||
def test_hash_file_returns_sha256_for_existing_file(self, tmp_path):
|
||||
"""_hash_file returns a non-empty SHA-256 hex digest for a readable file."""
|
||||
f = tmp_path / "real_file.txt"
|
||||
f.write_text("hello")
|
||||
w = SkillsWatcher(str(tmp_path), [])
|
||||
result = w._hash_file(f)
|
||||
assert len(result) == 32 # MD5 hex digest length
|
||||
assert len(result) == 64 # SHA-256 hex digest length
|
||||
assert result != ""
|
||||
|
||||
|
||||
|
||||
@ -37,7 +37,11 @@ class ConfigWatcher:
|
||||
|
||||
def _hash_file(self, path: str) -> str:
|
||||
try:
|
||||
return hashlib.md5(Path(path).read_bytes()).hexdigest()
|
||||
# H1: SHA-256 replaces MD5 for file-integrity change detection.
|
||||
# MD5 is collision-prone; using SHA-256 prevents a crafted config
|
||||
# file from producing the same hash as a benign one, which would
|
||||
# silently suppress the hot-reload callback.
|
||||
return hashlib.sha256(Path(path).read_bytes()).hexdigest()
|
||||
except (OSError, IOError):
|
||||
return ""
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user