Skip to content

fix: sanitize topic parameter in tool_diary_write#936

Merged
igorls merged 1 commit intoMemPalace:developfrom
shaun0927:fix/diary-topic-sanitize
Apr 26, 2026
Merged

fix: sanitize topic parameter in tool_diary_write#936
igorls merged 1 commit intoMemPalace:developfrom
shaun0927:fix/diary-topic-sanitize

Conversation

@shaun0927
Copy link
Copy Markdown
Contributor

@shaun0927 shaun0927 commented Apr 16, 2026

What does this PR do?

tool_diary_write validates agent_name via sanitize_name() (line 926) and entry via sanitize_content() (line 927), but stores topic raw into ChromaDB metadata at line 967. Any string — including null bytes, path traversal sequences, or megabyte-length payloads — passes through unchecked.

# Before
def tool_diary_write(agent_name: str, entry: str, topic: str = "general"):
    agent_name = sanitize_name(agent_name, "agent_name")  # ✓
    entry = sanitize_content(entry)                         # ✓
    # topic — stored raw at line 967                        # ✗

# After
def tool_diary_write(agent_name: str, entry: str, topic: str = "general"):
    agent_name = sanitize_name(agent_name, "agent_name")  # ✓
    entry = sanitize_content(entry)                         # ✓
    topic = sanitize_name(topic, "topic")                   # ✓ added

This is consistent with the input validation pattern already applied to every other MCP tool parameter (e.g. wing, room, agent_name in tool_add_drawer; subject/predicate/object in tool_kg_add via sanitize_kg_value).

Refs: #934 (Finding 4)

How to test

from mempalace.mcp_server import tool_diary_write

# Should be rejected (null byte)
result = tool_diary_write("test_agent", "hello", topic="bad\x00topic")
assert result["success"] is False

# Should be rejected (too long)
result = tool_diary_write("test_agent", "hello", topic="x" * 200)
assert result["success"] is False

# Should succeed (normal topic)
result = tool_diary_write("test_agent", "hello", topic="daily_standup")
assert result["success"] is True

Checklist

  • ruff check mempalace/mcp_server.py — passed
  • ruff format --check mempalace/mcp_server.py — passed

@shaun0927
fix: sanitize topic parameter in tool_diary_write
5bf8260 
agent_name and entry are validated via sanitize_name/sanitize_content,
but topic is stored raw into ChromaDB metadata. Apply the same
sanitize_name guard to reject null bytes, path traversal, and
oversized payloads.
@igorls igorls added bug Something isn't working area/mcp MCP server and tools labels Apr 16, 2026
@shaun0927 shaun0927 mentioned this pull request Apr 24, 2026
Closed
@shaun0927
Copy link
Copy Markdown
Contributor Author

shaun0927 commented Apr 24, 2026

Gentle ping — this is a 1-line sanitize fix for #1154 (split from #934). Same pattern as the other MCP tool handlers in the file. Happy to rebase if develop has moved under it.

bensig
bensig approved these changes Apr 26, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@bensig bensig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 — exact fix shaun0927 proposed in #1154. Same defensive pattern as wing/room in tool_add_drawer. Diff is +1/-0; the new sanitize_name(topic, "topic") runs alongside the existing agent_name and entry sanitizers in the same try/except, so ValueError handling is unchanged.

Ran the existing test_mcp_server.py suite locally on develop: 64 passed. CI matrix should match.

Ready to merge.

@igorls igorls merged commit 5de5b09 into MemPalace:develop Apr 26, 2026
6 checks passed
jphein added a commit to jphein/mempalace that referenced this pull request Apr 26, 2026
@jphein @claude
fix(mcp_server): restore topic sanitization in tool_diary_write
903441a 
Upstream PR MemPalace#936 (@shaun0927, commit 5bf8260) added
sanitize_name(topic, "topic") to tool_diary_write before metadata
write — null-byte / path-traversal / size guards. The 2026-04-26
upstream/develop sync brought the commit in, but the merge resolution
in our `tool_diary_write` (which had been modified by the
checkpoint-split phases A–D) kept our version and dropped the
sanitize line.

Verified non-mangling for our routing values: sanitize_name passes
"checkpoint", "auto-save", "CHECKPOINT", "general" through unchanged,
so the `if topic in _CHECKPOINT_TOPICS` check below remains correct.

Suite 1395/1395.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Scorpion1221 pushed a commit to Scorpion1221/mempalace that referenced this pull request May 2, 2026
fix(mcp_server): sanitize diary topic input (upstream MemPalace#936)
9835700
Scorpion1221 pushed a commit to Scorpion1221/mempalace that referenced this pull request May 2, 2026
chore(release): bump to 3.3.401
5b14c3b 
Sync with upstream/develop via cherry-pick of 9 critical bugfixes:
- HNSW index bloat prevention (MemPalace#1191)
- SIGSEGV guards on collection reopen (MemPalace#1262, MemPalace#1289)
- blob-seq marker fast-path (MemPalace#1177)
- palace_graph None-metadata guard (MemPalace#1201)
- palace_graph security tunnels (MemPalace#1168)
- hyphenated wing name normalization (MemPalace#1197)
- searcher _tokenize None guard (MemPalace#1198)
- mcp_server diary topic sanitize (MemPalace#936)

Tests: 1459 default + 113 benchmark + 6/7 stress all pass.
(random-kill stress test was failing on dev pre-merge; not regressed.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bensig bensig bensig approved these changes

@milla-jovovich milla-jovovich Awaiting requested review from milla-jovovich milla-jovovich is a code owner

Assignees

No one assigned

Labels

area/mcp MCP server and tools bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shaun0927 @bensig @igorls