Skip to content

fix(memory): reject absolute and escaping memory names in get_memory_file_path#1665

Open
AmirF194 wants to merge 1 commit into
oraios:mainfrom
AmirF194:fix/memory-path-absolute-escape
Open

fix(memory): reject absolute and escaping memory names in get_memory_file_path#1665
AmirF194 wants to merge 1 commit into
oraios:mainfrom
AmirF194:fix/memory-path-absolute-escape

Conversation

@AmirF194

@AmirF194 AmirF194 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What

MemoryManager.get_memory_file_path guards against .. segments "for security reasons", but not against absolute memory names. Because it builds the path with base_dir / "/".join(parts[:-1]), an absolute name discards the base directory: Path(project_memory_dir) / "/etc/cron.d" resolves to /etc/cron.d, escaping the .serena/memories sandbox. Since save_memory / load_memory / delete_memory / move_memory all route through this method, a crafted (e.g. prompt-injected) memory name could read, write, or delete arbitrary *.md files outside the sandbox. Both the project and global/ branches were affected.

Reproduced on current main: get_memory_file_path("/etc/cron.d/backdoor") returns /etc/cron.d/backdoor.md and even attempts to mkdir that directory (only stopped by OS permissions), while the existing control get_memory_file_path("../../etc/x") correctly raises.

Fix

  • Reject absolute names and empty path segments up front, alongside the existing .. check (os.path.isabs(name) or "" in parts). This runs on the full name, so it covers both branches before any directory is created.
  • Route both branches through a new _resolve_memory_path helper that adds a lexical containment backstop (os.path.normpath + is_relative_to, before any mkdir) enforcing the invariant that every returned path stays inside its base memories directory.

The containment check is deliberately lexical rather than Path.resolve()-based: serena intentionally supports directory symlinks inside memories/ (e.g. a monorepo symlinking each submodule's memory dir), and resolve() would follow those symlinks out of the base and wrongly reject them. The existing TestListMemoriesFollowsSymlinks behavior is preserved.

Verification

  • Added TestGetMemoryFilePathContainment (offline): absolute project name, absolute system path, absolute global name, .. still rejected, plus containment assertions that accepted names resolve inside their base dir.
  • The three absolute-rejection tests fail on main and pass on this branch; full test/serena/test_memories_manager.py is 112 passed (including the symlink feature test).
  • uv run poe lint (ruff format + check) and uv run poe type-check (ty) are clean.

Checklist

  • This PR follows the guidelines in CONTRIBUTING.md regarding the scope of PRs (single logical change).
  • For changes that add features or fix problems, I have added an entry to CHANGELOG.md.

Implementation done with AI assistance.

@opcode81 opcode81 force-pushed the fix/memory-path-absolute-escape branch from a497adc to f4e81bd Compare July 10, 2026 09:57
@opcode81 opcode81 force-pushed the fix/memory-path-absolute-escape branch from f4e81bd to 3ab9ff3 Compare July 10, 2026 09:59
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.

1 participant