Skip to content

fix: sanitize save hook transcript and session inputs - #62

Closed
PikkonMG wants to merge 1 commit into
MemPalace:developfrom
PikkonMG:fix/save-hook-input-sanitization
Closed

fix: sanitize save hook transcript and session inputs#62
PikkonMG wants to merge 1 commit into
MemPalace:developfrom
PikkonMG:fix/save-hook-input-sanitization

Conversation

@PikkonMG

@PikkonMG PikkonMG commented Apr 7, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes two security issues in hooks/mempal_save_hook.sh:

  • Stops embedding transcript_path directly into inline Python source
  • Sanitizes session_id before using it in the hook state filename

This keeps the existing autosave behavior the same while making hook input handling safer.

How to test

source .venv/bin/activate
pytest tests/ -v
bash -n hooks/mempal_save_hook.sh

##Manual validation:

Under-threshold run returns {}
Threshold run returns decision: block
stop_hook_active=true still bypasses correctly
Quoted transcript paths no longer break execution
Traversal-style session_id values stay contained in ~/.mempalace/hook_state

## Checklist
- [x] Tests pass (`pytest tests/ -v`)
- [x] Hook syntax validated (`bash -n hooks/mempal_save_hook.sh`)
- [x] No new dependencies added
- [ ] Linter passes (`ruff check .`)

@bensig

bensig commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Nice work on the SESSION_ID sanitization — that's the piece we still needed (we fixed the TRANSCRIPT_PATH injection in #114 but missed SESSION_ID).

However this PR now conflicts with our merged changes to the hook. Could you rebase on main and keep just the SESSION_ID sanitization + the single-parse JSON approach? The TRANSCRIPT_PATH sys.argv fix is already in.

@PikkonMG

PikkonMG commented Apr 7, 2026

Copy link
Copy Markdown
Author

Rebased on main and trimmed the branch to keep just the SESSION_ID sanitization plus the single-parse JSON approach. Re-ran tests and hook syntax validation.

Validation:

  • pytest tests/ -v27 passed
  • bash -n hooks/mempal_save_hook.sh

@PikkonMG

PikkonMG commented Apr 9, 2026

Copy link
Copy Markdown
Author

I rebased on the latest main and kept the SESSION_ID sanitization plus the single-parse JSON approach, while preserving the current upstream TRANSCRIPT_PATH handling.

I think this version is safer because it keeps the parsed hook fields as data instead of using eval to execute generated shell assignments. It still parses the input once, but reduces the risk of turning parsed values back into shell code. The SESSION_ID is also normalized before it is used in the hook state filename.

Validation:

pytest tests/ -v → 534 passed, 106 deselected
bash -n hooks/mempal_save_hook.sh

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔧 Review of #62fix: sanitize save hook transcript and session inputs

Scope: +32/−14 · 1 file(s)

  • hooks/mempal_save_hook.sh (modified: +32/−14)

Related PRs

  • Note: Related to #589 (JoeProAI's session sanitization) — verify no overlap

Suggestions

  • 💡 No tests included — consider adding coverage for the new code paths
  • 📋 PR checklist: 3/4 completed — 1 item(s) still unchecked

🟢 Approved — clean, well-structured PR. Good work @PikkonMG!


🏛️ Reviewed by MemPalace-AGI · Autonomous research system with perfect memory · Showcase: Truth Palace of Atlantis

@bensig
bensig changed the base branch from main to develop April 11, 2026 22:23
@bensig

bensig commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Closing — this is superseded by recently merged PRs to develop. Thank you for the contribution!

@bensig bensig closed this Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants