PR Review: security+perf: harden inputs, fix command injection, optimize DB access

Executive Summary

Affected Areas: hooks/mempal_save_hook.sh, mempalace/config.py, mempalace/convo_miner.py, mempalace/knowledge_graph.py, mempalace/mcp_server.py, mempalace/miner.py, mempalace/palace.py (new), pyproject.toml

Business Impact: Security hardening protects against path traversal and command injection in mining and MCP write operations. Performance improvements (singleton client, metadata cache, WAL journal mode) reduce DB overhead during interactive MCP usage.

Flow Changes: Miners now import shared SKIP_DIRS, get_collection, and file_already_mined from a new palace.py module instead of defining their own copies. MCP server adds write-ahead logging before all write operations and caches metadata scans with a 30s TTL. Knowledge graph switches to persistent connections with WAL journal mode.

Ratings

PR Health

• Has clear description
• References ticket/issue (if applicable) — no issue linked
• Appropriate size (or justified if large)
• Has relevant tests — no tests added for new sanitize_name(), sanitize_content(), palace.py, or WAL logging

High Priority Issues

(Must fix before merge)

🐛 #1: STOP_HOOK_ACTIVE not sanitized — command injection via eval

Location: hooks/mempal_save_hook.sh (new lines in eval block) | Confidence: ✅ HIGH

The PR claims to fix command injection by adding a safe() sanitizer lambda. SESSION_ID and TRANSCRIPT_PATH are passed through safe(), but STOP_HOOK_ACTIVE is interpolated directly into the eval'd output without sanitization. A crafted JSON input with "stop_hook_active": "true\"; rm -rf /tmp; echo \"" would break out of the shell double-quotes and execute arbitrary commands when eval runs the output.

This is a command injection vulnerability in a PR whose purpose is to fix command injection.

  safe = lambda s: re.sub(r'[^a-zA-Z0-9_/.\\-~]', '', str(s))
  print(f'SESSION_ID=\\\"{safe(sid)}\\\"')
- print(f'STOP_HOOK_ACTIVE=\\\"{sha}\\\"')
+ print(f'STOP_HOOK_ACTIVE=\\\"{safe(sha)}\\\"')
  print(f'TRANSCRIPT_PATH=\\\"{safe(tp)}\\\"')

🏗️ #2: SKIP_DIRS consolidation silently drops entries

Location: mempalace/palace.py:10-22 | Confidence: ✅ HIGH

The new palace.py consolidates SKIP_DIRS from miner.py and convo_miner.py, but the version in convo_miner.py on main includes "tool-results" and "memory" entries that are not present in palace.py. After this PR, conversation mining will no longer skip those directories, potentially mining tool output and memory files that were intentionally excluded.

  SKIP_DIRS = {
      ".git",
      "node_modules",
      "__pycache__",
      ".venv",
      "venv",
      "env",
      "dist",
      "build",
      ".next",
      "coverage",
      ".mempalace",
+     "tool-results",
+     "memory",
  }

Additionally, room_detector_local.py and entity_detector.py still maintain their own SKIP_DIRS copies — the consolidation is incomplete.

Medium Priority Issues

(Should fix, not blocking)

🎨 #3: _SAFE_NAME_RE compiled but never used

Location: mempalace/config.py:16 | Confidence: ✅ HIGH

A regex _SAFE_NAME_RE is compiled at module level but sanitize_name() never references it. The function only does manual checks for .., /, \\, and null bytes — which means special characters like ;, ', ", $, and backticks all pass validation. Either apply the regex or remove the dead code.

  def sanitize_name(value: str, field_name: str = "name") -> str:
      if not isinstance(value, str) or not value.strip():
          raise ValueError(f"{field_name} must be a non-empty string")
      value = value.strip()
      if len(value) > MAX_NAME_LENGTH:
          raise ValueError(f"{field_name} exceeds maximum length of {MAX_NAME_LENGTH} characters")
+     if not _SAFE_NAME_RE.match(value):
+         raise ValueError(f"{field_name} contains invalid characters")
-     if ".." in value or "/" in value or "\\" in value:
-         raise ValueError(f"{field_name} contains invalid path characters")
-     if "\x00" in value:
-         raise ValueError(f"{field_name} contains null bytes")
      return value

🚨 #4: Module-level _WAL_DIR.mkdir() creates directories at import time

Location: mempalace/mcp_server.py:48-49 | Confidence: ✅ HIGH

_WAL_DIR.mkdir(parents=True, exist_ok=True) executes at module import time, not when the MCP server starts. Any code that imports from mcp_server.py — including the test suite — will create ~/.mempalace/wal/ in the real home directory. This causes test environment pollution and unexpected filesystem side effects.

- _WAL_DIR = Path(os.path.expanduser("~/.mempalace/wal"))
- _WAL_DIR.mkdir(parents=True, exist_ok=True)
- _WAL_FILE = _WAL_DIR / "write_log.jsonl"
+ _WAL_DIR = Path(os.path.expanduser("~/.mempalace/wal"))
+ _WAL_FILE = _WAL_DIR / "write_log.jsonl"

  def _wal_log(operation: str, params: dict, result: dict = None):
+     _WAL_DIR.mkdir(parents=True, exist_ok=True)
      entry = { ... }

🔄 #5: knowledge_graph.py persistent connection risks with __del__ cleanup

Location: mempalace/knowledge_graph.py:98-103 | Confidence: ⚠️ MED

Relying on __del__ for connection cleanup is unreliable in Python — it may never be called, or may be called when the interpreter is shutting down (causing errors). The close() method exists but is never called by any code in this PR. Consider making KnowledgeGraph a context manager (__enter__/__exit__) so callers can use with blocks, or call close() explicitly in the callers.

+ def __enter__(self):
+     return self
+
+ def __exit__(self, *exc):
+     self.close()
+
  def __del__(self):
      self.close()

Low Priority Issues

(Nice to have)

⚡ #6: WAL file grows unbounded

Location: mempalace/mcp_server.py:56-63 | Confidence: ✅ HIGH

_wal_log() appends to write_log.jsonl without any rotation or size limits. Over months of active MCP usage, this file could grow to hundreds of MB. Consider rotating by date or adding a size cap.

🎨 #7: No tests for new security-critical functions

Location: mempalace/config.py, mempalace/palace.py | Confidence: ✅ HIGH

sanitize_name(), sanitize_content(), and the shared get_collection()/file_already_mined() in palace.py are new security-critical functions with no test coverage. Given this is a security audit PR, adversarial test cases (path traversal, null bytes, oversized inputs, unicode edge cases) would strengthen confidence in the validation logic.

Created by Octocode MCP https://octocode.ai 🔍🐙

@anthonyonazure pls rebase

@anthonyonazure rebase pls

hey @anthonyonazure — thanks for the thorough security audit. we've cherry-picked your changes into #387 with lint fixes applied and credited you as co-author. appreciate the contribution!

hey @anthonyonazure — thanks for the thorough security audit. we've cherry-picked your changes into #387 with lint fixes applied and credited you as co-author in the commit. appreciate the contribution!