fix: enforce symlink containment in file discovery and resolution#596
fix: enforce symlink containment in file discovery and resolution#596danielmeppiel merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds symlink containment enforcement across multiple file discovery and prompt resolution entry points so that files which resolve outside the intended base directory are rejected and cannot be read/compiled/deployed.
Changes:
- Enforced resolved-path containment via
ensure_path_within()in primitive discovery and integrator file discovery. - Added containment checks to prompt auto-discovery and prompt file resolution.
- Added a new unit test module covering symlink containment across the touched subsystems.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_symlink_containment.py | Adds regression tests validating that external-target symlinks are rejected by discovery/resolution. |
| src/apm_cli/primitives/discovery.py | Filters discovered primitive files by resolved-path containment within base_dir. |
| src/apm_cli/integration/base_integrator.py | Rejects glob-discovered package files whose resolved paths escape the package directory. |
| src/apm_cli/integration/hook_integrator.py | Rejects hook JSON files whose resolved paths escape the package directory. |
| src/apm_cli/core/script_runner.py | Filters discovered prompts by containment; rejects prompt resolution that escapes the project root. |
Copilot's findings
Comments suppressed due to low confidence (3)
tests/unit/test_symlink_containment.py:99
- Symlink creation here should be guarded for platforms/environments that do not support creating symlinks. Catch OSError from symlink_to() and skip the test (self.skipTest) to avoid cross-platform failures.
instructions_dir = self.project / ".github" / "instructions"
instructions_dir.mkdir(parents=True)
symlink = instructions_dir / "evil.instructions.md"
symlink.symlink_to(self.secret)
assert symlink.exists()
tests/unit/test_symlink_containment.py:137
- Symlink creation in this test is not portable: symlink_to() can raise OSError on Windows without elevated privileges. Add a try/except around symlink_to() and skip when symlinks are unavailable.
agents_dir = self.pkg / ".apm" / "agents"
agents_dir.mkdir(parents=True)
symlink = agents_dir / "evil.agent.md"
symlink.symlink_to(self.secret)
tests/unit/test_symlink_containment.py:172
- Guard this symlink_to() with try/except OSError and skip the test if symlinks cannot be created on the current platform. Otherwise this will fail on Windows runners without symlink support.
hooks_dir = self.pkg / ".apm" / "hooks"
hooks_dir.mkdir(parents=True)
symlink = hooks_dir / "evil.json"
symlink.symlink_to(self.secret)
- Files reviewed: 5/5 changed files
- Comments generated: 4
| prompts_dir = self.project / ".apm" / "prompts" | ||
| prompts_dir.mkdir(parents=True) | ||
| symlink = prompts_dir / "evil.prompt.md" | ||
| symlink.symlink_to(self.secret) | ||
| assert symlink.exists() # Symlink target is readable |
There was a problem hiding this comment.
These tests create symlinks unconditionally. On Windows (and some CI environments), symlink creation can raise OSError (e.g. missing privileges / Developer Mode), which will fail the suite post-merge. Wrap symlink creation in try/except OSError and skip the test (e.g. self.skipTest) when symlinks are not supported.
This issue also appears in the following locations of the same file:
- line 95
- line 133
- line 168
src/apm_cli/core/script_runner.py
Outdated
| raise FileNotFoundError( | ||
| f"Prompt file '{prompt_file}' is a symlink that resolves " | ||
| f"outside the project directory. This is not allowed." |
There was a problem hiding this comment.
The FileNotFoundError message claims the path "is a symlink" when ensure_path_within() will also reject non-symlink paths that simply resolve outside the project (e.g. absolute paths or .. traversal that lands outside). Consider making the message accurate (e.g. "resolves outside the project directory") and, if needed, mention symlinks separately only when prompt_path.is_symlink() is true.
| raise FileNotFoundError( | |
| f"Prompt file '{prompt_file}' is a symlink that resolves " | |
| f"outside the project directory. This is not allowed." | |
| if prompt_path.is_symlink(): | |
| raise FileNotFoundError( | |
| f"Prompt file '{prompt_file}' is a symlink that resolves " | |
| f"outside the project directory. This is not allowed." | |
| ) | |
| raise FileNotFoundError( | |
| f"Prompt file '{prompt_file}' resolves outside the project " | |
| f"directory. This is not allowed." |
| def find_primitive_files(base_dir: str, patterns: List[str]) -> List[Path]: | ||
| """Find primitive files matching the given patterns. | ||
|
|
||
| Rejects symlinks that resolve outside the base directory to prevent | ||
| symlink traversal attacks from malicious packages. | ||
|
|
There was a problem hiding this comment.
Docs drift: docs/src/content/docs/enterprise/security.md currently states "Symlinks are never followed" and references rejecting symlinks via not .is_symlink(), but this function now permits symlinks as long as the resolved target stays within base_dir. Please update the security docs to describe the actual policy (allow contained symlinks; reject paths that resolve outside).
| import shutil | ||
| import unittest | ||
| from pathlib import Path | ||
| from unittest.mock import patch |
There was a problem hiding this comment.
Unused import: patch is imported but never used. Please remove it to keep the test module clean.
| from unittest.mock import patch |
Reject symlinked primitive files outright via is_symlink() checks at all
file discovery and resolution choke points. This matches the documented
security policy ('symlinks are never followed') and prevents symlink-
based traversal attacks from malicious packages.
Protected paths:
- PromptCompiler._resolve_prompt_file (prompt compilation/preview)
- ScriptRunner._discover_prompt_file (prompt discovery)
- find_primitive_files (instruction/agent/context discovery)
- BaseIntegrator.find_files_by_glob (integrator file discovery)
- HookIntegrator.find_hook_files (hook JSON discovery)
- 5 new security tests with Windows symlink portability guards
- Updated security docs to reflect full symlink rejection coverage
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3cf696b to
7160659
Compare
Summary
Adds symlink boundary validation at all file discovery and resolution choke points. Files that are symlinks resolving outside their expected base directory are now silently rejected during discovery, preventing them from being read, compiled, or deployed.
Changes
Uses the existing
ensure_path_within()utility (which resolves symlinks before checking containment) at 5 choke points:PromptCompiler._resolve_prompt_file-- prompt compilation and previewScriptRunner._discover_prompt_file-- prompt auto-discoveryfind_primitive_files-- instruction, agent, context, and memory file discoveryBaseIntegrator.find_files_by_glob-- integrator-level file discovery (agents, instructions, prompts, skills)HookIntegrator.find_hook_files-- hook JSON file discoveryTesting
test_symlink_containment.pyvalidate rejection across all subsystems