fix: harden hooks, MCP server, and config (security audit)#893
Open
brodsky754 wants to merge 1 commit intoMemPalace:developfrom
Open
fix: harden hooks, MCP server, and config (security audit)#893brodsky754 wants to merge 1 commit intoMemPalace:developfrom
brodsky754 wants to merge 1 commit intoMemPalace:developfrom
Conversation
… injection, and info leakage Security audit identified 3 High and 3 Medium findings. This commit addresses them with minimal, targeted fixes while preserving all 864 baseline tests. High fixes: - hooks_cli: validate transcript_path rejects ".." traversal before reading - hooks_cli: validate MEMPAL_DIR contains project markers before subprocess spawn, canonicalize paths via os.path.realpath() Medium fixes: - mcp_server: replace str(e) with "Internal error" in Exception handlers (12 sites) to prevent leaking file paths and internal state to MCP clients - config: create config.json atomically with os.open(O_CREAT|O_EXCL, 0o600) to eliminate TOCTOU permission window - config: write people_map.json with 0o600 permissions (contains PII) Co-Authored-By: Claude Opus 4.6 <[email protected]>
brodsky754
requested review from
bensig,
igorls and
milla-jovovich
as code owners
Collaborator
|
hey @brodsky754 — this conflicts with develop. also pls rebase onto #863 (precompact fix) first since both touch hooks_cli.py — merging in the wrong order would regress the compaction fix. thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_is_safe_transcript_path()to reject..traversal in transcript reads; add_is_valid_mempal_dir()to validate MEMPAL_DIR contains project markers before subprocess spawn; canonicalize all paths viaos.path.realpath()str(e)with"Internal error"in 12except Exceptionhandlers to prevent leaking file paths/internal state to MCP clients (keepsstr(e)inValueErrorhandlers for user-facing validation)config.jsonatomically withos.open(O_CREAT|O_EXCL, 0o600)to eliminate TOCTOU permission window; writepeople_map.jsonwith0o600permissions (contains PII)Context
Full security audit of the codebase found 0 Critical, 3 High, 6 Medium, 6 Low findings. This PR addresses all 3 High and 3 trivial Medium fixes. Remaining Medium/Low findings are documented in the audit report (informational only).
Test plan
.gitmarker dirs for new_is_valid_mempal_dir()validation🤖 Generated with Claude Code