Skip to content

fix: harden hooks, MCP server, and config (security audit) - #893

Open
brodsky754 wants to merge 1 commit into
MemPalace:developfrom
brodsky754:security/audit-fixes-2026-04
Open

fix: harden hooks, MCP server, and config (security audit)#893
brodsky754 wants to merge 1 commit into
MemPalace:developfrom
brodsky754:security/audit-fixes-2026-04

Conversation

@brodsky754

Copy link
Copy Markdown

Summary

  • hooks_cli: Add _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 via os.path.realpath()
  • mcp_server: Replace str(e) with "Internal error" in 12 except Exception handlers to prevent leaking file paths/internal state to MCP clients (keeps str(e) in ValueError handlers for user-facing validation)
  • config: Create config.json atomically with os.open(O_CREAT|O_EXCL, 0o600) to eliminate TOCTOU permission window; write people_map.json with 0o600 permissions (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

  • All 864 baseline tests pass (0 regressions)
  • Updated 4 test fixtures to create .git marker dirs for new _is_valid_mempal_dir() validation
  • Verified via background agent that all fixes are correct and complete

🤖 Generated with Claude Code

@igorls igorls added security Security related bug Something isn't working labels Apr 14, 2026
@bensig

bensig commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

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!

@igorls

igorls commented May 8, 2026

Copy link
Copy Markdown
Member

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)

@igorls igorls added the needs-rebase PR has merge conflicts with develop and needs rebase label May 8, 2026
config.py: write config.json and people_map.json via os.open(..., 0o600)
— O_EXCL on first create, O_TRUNC on overwrite — so the files are
owner-only from creation, closing the brief world-readable window
between open() and chmod(). Covers all five config / people-map write paths.

mcp_server.py: stop returning raw exception text (str(e)) to clients from
the internal-error (`except Exception`) handlers, which leaked backend /
path / stack details; return a generic "Internal error" and log the real
exception server-side via logger.exception(). Deliberate ValueError
validation messages and tool_add_drawer's actionable readback-failure
RuntimeError are still surfaced — callers rely on them and they expose
nothing internal.

Rebased onto develop. The original hooks_cli.py changes are dropped:
develop already validates transcript paths (_validate_transcript_path) and
resolves MEMPAL_DIR before mining, making them redundant.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@brodsky754
brodsky754 force-pushed the security/audit-fixes-2026-04 branch from 01b0198 to 56768a9 Compare June 8, 2026 20:17
@brodsky754

Copy link
Copy Markdown
Author

Rebased onto current develop.

@bensig#863 landed in develop on Apr 15, so the precompact fix is already in the base; no special ordering needed. I've also dropped the hooks_cli.py changes entirely, since develop now covers them — _validate_transcript_path() does the same ..-rejection + resolve (plus a .jsonl/.json allowlist), and _get_mine_targets() already resolves MEMPAL_DIR before mining. That removes the overlap you flagged.

What's left:

  • config.pyconfig.json / people_map.json are now created atomically with os.open(..., 0o600) (O_EXCL on first create, O_TRUNC on overwrite), so there's no window where they're world-readable between open() and chmod(). Applied to all five write paths.
  • mcp_server.py — the internal-error (except Exception) handlers no longer return raw str(e); they return a generic "Internal error" and log the real exception via logger.exception(). Deliberate ValueError validation messages and tool_add_drawer's actionable readback-failure message are unchanged.

ruff check, ruff format --check, and the full test suite (2278 passed) are green locally.

@igorls

igorls commented Aug 15, 2026

Copy link
Copy Markdown
Member

Thanks for this contribution, and apologies for the slow turnaround.

develop has moved a fair way since this was opened and the branch no longer merges cleanly. If you're still interested in landing it, could you rebase onto current develop? Once it merges cleanly and CI is green I'll get it reviewed for the 3.8.0 cycle.

If you'd rather not pick it back up, no problem at all — just say so and I'll close it out, and thanks either way for taking the time to send it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working needs-rebase PR has merge conflicts with develop and needs rebase security Security related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants