Skip to content

fix(#37,#38-#43,#42): narrow LLM write guard, expanduser paths, Copilot absolute paths#53

Merged
FZ2000 merged 1 commit intomainfrom
fix/security-llm-memory-path-37
Mar 8, 2026
Merged

fix(#37,#38-#43,#42): narrow LLM write guard, expanduser paths, Copilot absolute paths#53
FZ2000 merged 1 commit intomainfrom
fix/security-llm-memory-path-37

Conversation

@FZ2000
Copy link
Owner

@FZ2000 FZ2000 commented Mar 8, 2026

Fixes #37, relates to #38, #39, #40, #41, #42, #43

#37 — LLM memory write MEMORY_ALLOWED_BASE too broad

The base guard now runs at the START of apply_memory_via_llm() (before any LLM call). If a subclass sets MEMORY_SCHEMA but forgets MEMORY_ALLOWED_BASE, it gets a RuntimeError immediately rather than silently defaulting to the entire home directory. This prevents accidental whole-home write access from new appliers that copy the base pattern without setting the guard.

#38-#43 — Tool memory files not written after sync

Root cause: LLM-generated paths using tilde notation (e.g. /.claude/CLAUDE.md) were not being expanded before the security check. Path('/.claude/CLAUDE.md').resolve() on macOS resolves relative to CWD, not the user home, causing the guard to reject valid paths. Fixed by adding Path.expanduser() before .resolve() in the path validation logic.

#42 — GitHub Copilot applier uses relative CWD paths

Module-level COPILOT_INSTRUCTIONS and VSCODE_MCP_JSON constants were relative Paths. Now replaced with accessor functions (_copilot_instructions(), _vscode_mcp_json(), _copilot_instructions_dir()) evaluated at call-time using Path.cwd().resolve(), ensuring stable absolute paths.

Tests

4 new tests in test_security_llm_memory_path.py:

  • RuntimeError when MEMORY_ALLOWED_BASE is None
  • No error when MEMORY_ALLOWED_BASE is properly set
  • Path outside allowed base is rejected with a warning
  • Tilde paths inside allowed base are accepted

…ot absolute paths

- #37: Move MEMORY_ALLOWED_BASE=None guard to the start of
  apply_memory_via_llm() — before any LLM call — so misconfigured
  appliers fail immediately rather than after a network round-trip.
  Raises RuntimeError so callers cannot silently swallow the problem.
- #38-#43: Add Path.expanduser() before .resolve() in the security
  check so LLM-generated paths using '~/' notation are correctly
  expanded and compared against the allowed base directory. Without
  this, paths like '~/.claude/CLAUDE.md' would resolve relative to
  CWD and be rejected even when they should be accepted.
- #42: Replace module-level relative Path constants in CopilotApplier
  (COPILOT_INSTRUCTIONS, VSCODE_MCP_JSON) with accessor functions
  (_copilot_instructions(), _vscode_mcp_json()) that are evaluated at
  call-time using Path.cwd().resolve(), ensuring a stable absolute path
  regardless of working directory changes between collection and apply.

Tests: 4 new tests in test_security_llm_memory_path.py covering the
RuntimeError guard, the expanduser acceptance/rejection paths.
@FZ2000 FZ2000 merged commit 26bc021 into main Mar 8, 2026
5 checks passed
@FZ2000 FZ2000 deleted the fix/security-llm-memory-path-37 branch March 8, 2026 02:18
forge-fz2000 added a commit that referenced this pull request Mar 9, 2026
… diff

Removed incorrectly listed items that are NOT in main:
- apc skill remove / apc unsync (PR #72 was merged into a feature branch,
  not main)
- Windsurf/Copilot native sync support (same)
- --target/-t removal from apc install (same)

Added missing items that ARE in main since v0.1.1:
- Security fixes: input validation, chmod 600 MCP configs, scrub secrets
  from export (#27,#28,#30 via #50; #32,#35 via #52)
- Bug fixes: LLM write guard, expanduser paths, Copilot absolute paths
  (#37,#38-#43,#42 via #53; #36,#45 via #54)
- Fix: ~/.apc/skills/ always created after apc install
- Fix: --version reads from importlib.metadata
- Docs: README shell completion, CLI basics (#23,#26 via #67)
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.

[SECURITY] LLM memory write allowed base is too broad — can overwrite non-memory tool config files

1 participant