fix: sanitize SESSION_ID in save hook to prevent path traversal#141
fix: sanitize SESSION_ID in save hook to prevent path traversal#141bensig merged 1 commit intoMemPalace:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the Claude/Codex “Stop” save hook against path traversal by sanitizing SESSION_ID before it’s used in state file paths, and also includes broader project maintenance updates (tests + packaging configuration).
Changes:
- Sanitize
SESSION_IDinhooks/mempal_save_hook.shto prevent path traversal when writing per-session state. - Expand/introduce a substantial pytest suite (MCP server tools, search API, KG, AAAK dialect) with shared fixtures.
- Update packaging metadata/tooling (move to hatchling; remove
requirements.txt; update version metadata).
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
hooks/mempal_save_hook.sh |
Sanitizes SESSION_ID before using it in $STATE_DIR filenames. |
mempalace/__init__.py |
Bumps package __version__ to 3.0.0. |
pyproject.toml |
Updates build backend to hatchling; adjusts dev dependency configuration; adds ruff/pytest config. |
requirements.txt |
Removes legacy requirements file. |
tests/conftest.py |
Adds isolated HOME/palace/KG fixtures and seeded test data. |
tests/test_mcp_server.py |
Adds tool/dispatch tests for MCP server handlers. |
tests/test_searcher.py |
Adds tests for the programmatic search_memories API. |
tests/test_knowledge_graph.py |
Adds tests for temporal KG CRUD/queries/timeline/stats. |
tests/test_dialect.py |
Adds tests for AAAK dialect compression/encoding/decoding/stats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """MemPalace — Give your AI a memory. No API key required.""" | ||
|
|
||
| __version__ = "2.0.0" | ||
| __version__ = "3.0.0" | ||
|
|
||
| from .cli import main |
There was a problem hiding this comment.
__version__ was bumped to 3.0.0 here, but the MCP server still reports serverInfo.version as 2.0.0 in mempalace/mcp_server.py's initialize response. This creates a user-visible version mismatch; consider sourcing the MCP server version from mempalace.__version__ (or otherwise updating it) so they stay in sync.
| [dependency-groups] | ||
| dev = ["pytest>=7.0", "ruff>=0.4.0"] | ||
|
|
There was a problem hiding this comment.
dev dependencies are defined twice ([project.optional-dependencies].dev and [dependency-groups].dev). Keeping two lists in sync is error-prone; consider making one the single source of truth (e.g., keep only dependency-groups for uv, or only optional-dependencies if you need pip install .[dev]).
| [dependency-groups] | |
| dev = ["pytest>=7.0", "ruff>=0.4.0"] |
| # Parse fields from Claude Code's JSON | ||
| SESSION_ID=$(echo "$INPUT" | python3 -c "import sys,json; print(json.load(sys.stdin).get('session_id','unknown'))" 2>/dev/null) | ||
| # Sanitize SESSION_ID to prevent path traversal (only allow alnum, dash, underscore) | ||
| SESSION_ID=$(echo "$SESSION_ID" | tr -cd 'a-zA-Z0-9_-') |
There was a problem hiding this comment.
For robust sanitization, consider avoiding echo (which can have implementation-defined behavior) and forcing a stable locale for the tr ranges. Using printf '%s' "$SESSION_ID" and LC_ALL=C tr -cd ... makes the sanitization behavior more predictable across environments.
| SESSION_ID=$(echo "$SESSION_ID" | tr -cd 'a-zA-Z0-9_-') | |
| SESSION_ID=$(printf '%s' "$SESSION_ID" | LC_ALL=C tr -cd 'a-zA-Z0-9_-') |
|
|
||
| # Parse fields from Claude Code's JSON | ||
| SESSION_ID=$(echo "$INPUT" | python3 -c "import sys,json; print(json.load(sys.stdin).get('session_id','unknown'))" 2>/dev/null) | ||
| # Sanitize SESSION_ID to prevent path traversal (only allow alnum, dash, underscore) | ||
| SESSION_ID=$(echo "$SESSION_ID" | tr -cd 'a-zA-Z0-9_-') | ||
| [ -z "$SESSION_ID" ] && SESSION_ID="unknown" |
There was a problem hiding this comment.
The PR title/description focus on sanitizing SESSION_ID in the save hook, but this change set also introduces a large test suite expansion and packaging/tooling changes (pyproject build backend + requirements removal). Consider splitting those into separate PRs or updating the PR description to explicitly scope/rationalize the extra changes, since they increase review and release risk beyond the security fix.
|
Hey Igor — CI is failing, looks like it needs a rebase on main (we've merged a bunch of changes recently). Can you rebase and push? |
The save hook uses SESSION_ID in file paths (state_dir/). A crafted session_id value like '../../etc/cron.d/evil' could write state files outside the intended directory. Strip everything except [a-zA-Z0-9_-] from SESSION_ID, defaulting to 'unknown' if empty after sanitization. Finding: MemPalace#4 (HIGH — path traversal via SESSION_ID) Includes test infrastructure from PR MemPalace#131. 92 tests pass.
igorls
force-pushed
the
fix/hook-security
branch
from
2e965a3 to
50239d4
Compare
Problem
The save hook (
hooks/mempal_save_hook.sh) usesSESSION_IDfrom JSON input directly in file paths:STATE_FILE="$STATE_DIR/$SESSION_ID"A crafted
session_idvalue like../../etc/cron.d/evilwould write state files outside~/.mempalace/hook_state/.Finding: #4 (HIGH — path traversal via SESSION_ID)
Fix
92 tests pass (includes test infrastructure from PR #131).