fix(install): dereference in-package symlinks on local install (#1668)

[danielmeppiel]

fix(install): dereference in-package symlinks on local install

TL;DR

Local-path apm install now materializes in-package symlinks as real files,
matching the behavior of remote install. A symlink whose resolved target escapes
the package root causes a hard-fail (PathTraversalError), never a silent drop.
All five mandatory security guardrails from the maintainer brief are implemented.

Closes #1668

Problem (WHY)

"A local-path apm install <dir> silently drops skill files that are
symlinks; a remote apm install owner/repo of the same package keeps them."
-- Issue #1668

The divergence had two root causes:

• _copy_local_package called shutil.copytree(..., symlinks=True), preserving
  every symlink in apm_modules/ rather than materializing its content.
• The downstream deploy filter ignore_non_content (security/gate.py) excluded
  all symlinks from the copy into target directories -- including in-package
  ones that were safe to follow.
• Result: both operations reported success while the referenced files vanished.

The old code also carried an explicit SECURITY comment acknowledging an open
TOCTOU window and deferring the fix -- this PR closes that window.

Approach (WHAT)

Layer	Change
local_content.py	Replace bulk copytree(symlinks=True) with _copy_tree_dereferencing_validated()
New helper	Atomic per-symlink: resolve(strict=True) -> ensure_path_within() -> copy2
Escaping symlinks	Raise PathTraversalError immediately; no warn-and-skip, no partial copy
ignore_non_content (gate.py)	Unchanged -- symlinks no longer reach deployment because they are dereferenced at staging
Security docs	Threat-model note added to docs/enterprise/security.md

Implementation (HOW)

src/apm_cli/install/phases/local_content.py

Added _copy_tree_dereferencing_validated(src, dst, pkg_root): walks the source
tree recursively. For each symlink entry it atomically calls
entry.resolve(strict=True), passes the result to ensure_path_within(resolved, pkg_root) (the #596 containment helper), then shutil.copy2(resolved, dst_entry)
for files or recurses for directories. Any symlink escaping pkg_root raises
PathTraversalError with a human-readable message and aborts the entire copy.
Replaced the shutil.copytree(symlinks=True) call and the accompanying
unresolved TOCTOU note with a single call to the new helper.

tests/unit/install/test_local_content_symlink_deref.py (new)

Six typed-coverage tests written as a failing regression trap before the fix:

• TestInPackageSymlinkIsDerefed -- in-package symlink materialized as real file
• TestEscapingSymlinkHardFails -- escaping symlink raises PathTraversalError; no partial copy
• TestNonSymlinkFilesUnchanged -- non-symlink baseline unchanged

tests/unit/test_local_deps.py

Updated test_copy_preserves_symlinks_without_following to assert the new
correct behavior (hard-fail) instead of the old incorrect behavior
(preserved-as-symlink with silent downstream drop).

docs/src/content/docs/enterprise/security.md

Added "Local-install symlink dereference and containment guarantee" subsection
under Symlink handling, documenting all four security properties.

Diagrams

flowchart TD
    A[_copy_local_package called] --> B[_copy_tree_dereferencing_validated]
    B --> C{entry type?}
    C -- regular file --> D[shutil.copy2 to dst]
    C -- directory --> E[recurse]
    C -- symlink --> F[resolve strict=True]
    F --> G{resolved within pkg_root?}
    G -- yes --> H[copy2 resolved content to dst]
    G -- no --> I[raise PathTraversalError<br/>install aborted]
    D --> J[continue to next entry]
    H --> J
    E --> J

Loading

sequenceDiagram
    participant Caller as _copy_local_package
    participant Helper as _copy_tree_dereferencing_validated
    participant PW as ensure_path_within
    participant FS as shutil.copy2

    Caller->>Helper: src=local, dst=install_path, pkg_root=local
    loop for each entry
        Helper->>Helper: entry.is_symlink()?
        alt is symlink
            Helper->>Helper: resolve(strict=True)
            Helper->>PW: ensure_path_within(resolved, pkg_root)
            alt escapes pkg_root
                PW-->>Helper: PathTraversalError
                Helper-->>Caller: PathTraversalError (install aborted)
            else within pkg_root
                Helper->>FS: copy2(resolved, dst_entry)
            end
        else regular file
            Helper->>FS: copy2(entry, dst_entry)
        end
    end
    Helper-->>Caller: return install_path

Loading

Trade-offs

• Behavior change on escaping symlinks. Old: symlink preserved silently in
  apm_modules/, downstream filter drops it without warning. New: hard-fail.
  Authors who had escaping symlinks (even unintentionally) now see an error
  rather than silent data loss -- this is strictly safer.
• No ignore_non_content change. By fixing the staging phase, the deploy
  filter naturally handles in-package content as regular files. Keeping the
  filter intact preserves its protection for any residual symlinks from other
  code paths.
• copystat on symlinks. Calling shutil.copystat(entry, dst_entry) after
  the dereference preserves the link's mtime, not the target's -- acceptable for
  a package staging copy where timestamps are not semantically significant.

Benefits

1. Local install output is consistent with remote install for packages that use
   in-package symlinks to share reference content (e.g. shared-contract.md).
2. Path-traversal via crafted symlinks is now a hard install failure rather than
   a silent no-op, closing a latent attack surface.
3. The TOCTOU window documented in the old SECURITY comment is resolved: resolve,
   validate, and copy are now a single atomic sequence per file.
4. Security posture is documented in the enterprise security page with a complete
   threat model.

Validation evidence

$ uv run --extra dev pytest tests/unit/install/test_local_content_symlink_deref.py tests/unit/test_local_deps.py::TestCopyLocalPackage -v

tests/unit/install/test_local_content_symlink_deref.py::TestInPackageSymlinkIsDerefed::test_in_package_symlink_materialized_as_real_file PASSED
tests/unit/install/test_local_content_symlink_deref.py::TestInPackageSymlinkIsDerefed::test_in_package_nested_symlink_content_preserved PASSED
tests/unit/install/test_local_content_symlink_deref.py::TestEscapingSymlinkHardFails::test_symlink_escaping_package_root_raises PASSED
tests/unit/install/test_local_content_symlink_deref.py::TestEscapingSymlinkHardFails::test_escaping_install_path_is_not_partially_created PASSED
tests/unit/install/test_local_content_symlink_deref.py::TestNonSymlinkFilesUnchanged::test_regular_files_copied_intact PASSED
tests/unit/install/test_local_content_symlink_deref.py::TestNonSymlinkFilesUnchanged::test_install_path_returned_on_success PASSED
tests/unit/test_local_deps.py::TestCopyLocalPackage::test_copy_preserves_symlinks_without_following PASSED
13 passed in 0.99s

$ uv run --extra dev pytest tests/unit/install/ tests/unit/test_local_deps.py tests/unit/test_security_gate.py tests/unit/test_symlink_containment.py -q
1662 passed, 3 subtests passed in 12.17s

$ uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/
All checks passed! 1225 files already formatted

$ uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/
Your code has been rated at 10.00/10

$ bash scripts/lint-auth-signals.sh
[+] auth-signal lint clean

Scenario Evidence

Scenario	Test	APM Principle
In-package symlink materialized as real file	TestInPackageSymlinkIsDerefed::test_in_package_symlink_materialized_as_real_file	Parity between local and remote install
Escaping symlink causes hard-fail	TestEscapingSymlinkHardFails::test_symlink_escaping_package_root_raises	Fail-closed on security surface
No partial copy left on escaping symlink	TestEscapingSymlinkHardFails::test_escaping_install_path_is_not_partially_created	Atomicity
Non-symlink files unchanged	TestNonSymlinkFilesUnchanged::test_regular_files_copied_intact	Backward compatibility

How to test

1. Create a local APM package with an in-package symlink:

   mkdir -p /tmp/symlink-demo/.apm/skills/demo/references
   echo "# Shared" > /tmp/symlink-demo/.apm/shared/shared.md
   mkdir -p /tmp/symlink-demo/.apm/shared
   echo "# Shared" > /tmp/symlink-demo/.apm/shared/shared.md
   ln -s ../../../shared/shared.md /tmp/symlink-demo/.apm/skills/demo/references/shared.md
   echo "name: demo\nversion: 1.0.0" > /tmp/symlink-demo/apm.yml

2. Run apm install /tmp/symlink-demo --target claude and verify
   .claude/skills/demo/references/shared.md is a regular file with correct content.
3. Run pytest tests/unit/install/test_local_content_symlink_deref.py -v --
   all 6 tests must pass.
4. Create a package with an escaping symlink (ln -s /etc/hosts /tmp/symlink-demo/evil),
   run apm install /tmp/symlink-demo and confirm the install fails with a clear
   PathTraversalError message naming the offending symlink.

[Copilot]

Pull request overview

This PR makes local-path apm install <dir> stage in-package symlinks as real files (matching remote install behavior) and hard-fails on symlinks whose resolved targets escape the package root, with accompanying unit-test coverage and security documentation updates.

Changes:

• Replaced local staging copytree(..., symlinks=True) behavior with a validated, symlink-dereferencing copy routine.
• Added regression/security tests covering in-package symlink materialization and escaping-symlink failure behavior.
• Updated enterprise security docs to describe the local-install containment guarantee.

Show a summary per file

File	Description
src/apm_cli/install/phases/local_content.py	Adds validated dereferencing copy helper and switches local staging to use it.
tests/unit/install/test_local_content_symlink_deref.py	New regression/security tests for local install symlink dereference and escape handling.
tests/unit/test_local_deps.py	Updates existing test expectation to assert hard-fail on escaping symlink.
docs/src/content/docs/enterprise/security.md	Documents the local-install symlink dereference + containment posture.

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 3

[danielmeppiel]

APM Review Panel: ship_with_followups

Security guarantee holds: per-file resolve->guard->copy pattern is TOCTOU-safe, escaping symlinks hard-fail, regression tests trap removal. Ship with 4 high-signal followups.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All six orchestrator reservations are satisfied. The per-file resolve(strict=True)->ensure_path_within->copy2 pattern is confirmed TOCTOU-safe by both python-architect and supply-chain-security-expert: the resolved absolute path is computed once and used directly by copy2, eliminating the rename-after-check window that plagues bulk copytree approaches. Escaping symlinks hard-fail with PathTraversalError -- never warn-and-skip -- matching the containment contract. Only in-package symlinks are dereferenced; external targets trigger the guard. The security tests are confirmed regression-trap: removing ensure_path_within would let escaping files through.

The two substantive gaps -- circular directory-symlink recursion and partial-copy cleanup -- were independently flagged by both python-architect and supply-chain-security-expert, and have been folded into commit f301b57. OS ELOOP catches simple cycles but the platform limit varies (40 on Linux, lower on macOS); the explicit visited-set guard is deterministic, testable, and cheap. Three additional test classes (TestBrokenSymlink, TestSymlinkToDirectory, TestCircularSymlink) bring the test file to 9 tests across 6 classes.

Aligned with: Secure by default -- hard-fail on escaping symlinks, containment per-file, no opt-in flag needed. Pragmatic as npm -- matches npm/pip precedent for local path-escape hard-fails; closes the footgun for monorepo package authors using in-package symlinks for shared config.

Growth signal. Story-shaped for enterprise trust signaling: 'Local installs now safely dereference in-package symlinks, closing a path-traversal vector and matching remote-install behavior.' Worth leading next release notes with this fix as evidence of supply-chain hygiene.

Reservations carried from strategic-alignment:

• "security-surface (path traversal): confirm TOCTOU-safe atomic per-file resolve(strict=True)->ensure_path_within->copy2 (NOT a bulk copytree)" -- addressed: confirmed per-file pattern; all panelists converged.
• "a symlink whose resolved target escapes the package root HARD-FAILS with PathTraversalError (never warn-and-skip)" -- confirmed: both code and tests enforce this; security test would fail without the guard.
• "ONLY in-package symlinks are dereferenced (no following external/arbitrary symlinks)" -- confirmed: ensure_path_within check fires before any copy.
• "the docs threat-model/containment note is present and accurate" -- addressed: doc accuracy fixes folded (intro contradiction fixed, 'atomically' softened to 'per-file', cross-link added).
• "the regression+security tests actually fail without the guard" -- confirmed: mutation-break gate passed (removing ensure_path_within causes TestEscapingSymlinkHardFails to fail).

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Architecturally sound private helper; per-file resolve->guard->copy is TOCTOU-safe.
CLI Logging Expert	0	1	1	[x] prefix in exception body causes double-prefix rendering; folded.
DevX UX Expert	0	0	2	Correct fix; local/remote install parity is right. No UX regressions.
Supply Chain Security Expert	0	4	0	Containment validated per-symlink; TOCTOU acceptable; cycle guard and cleanup recommended.
OSS Growth Hacker	0	0	0	Strong trust-building PR; enterprise-grade supply-chain hygiene signal.
Auth Expert	--	--	--	Inactive: PR touches only local file copy logic; no auth surfaces changed.
Doc Writer	0	5	2	Accuracy fixes needed; majority folded.
Test Coverage Expert	0	1	1	Security regression trap confirmed; broken-symlink and dir-symlink tests added.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Supply Chain Security Expert] Add visited-set cycle guard for recursive directory symlink traversal -- OS ELOOP is platform-dependent (40 hops Linux, fewer macOS). Explicit visited-set is O(1), deterministic, and testable. Both security and architecture panelists converged on this independently. FOLDED in f301b57.

2. [Test Coverage Expert] Add broken/dangling symlink error path test (OSError -> PathTraversalError) -- outcome:missing on secure-by-default surface. The OSError catch-and-wrap branch is user-visible but untested. FOLDED in f301b57.

3. [Supply Chain Security Expert] Wrap _copy_tree_dereferencing_validated in try/except PathTraversalError with safe_rmtree cleanup -- Partial-copy dangling content on malicious packages is a real attack surface. FOLDED in f301b57.

4. [Doc Writer] Fix section intro contradiction and soften 'resolved atomically' overclaim in security.md -- Security docs that contradict implementation erode enterprise audit trust. FOLDED in f301b57.

5. [CLI Logging Expert] Remove [x] prefix from PathTraversalError message body to avoid double-prefix rendering -- Double [x][x] in terminal output looks broken and undermines CLI polish. FOLDED in f301b57.

Folded in this run (commit f301b57)

• Circular symlink visited-set guard (python-architect + supply-chain-security-expert)
• Partial-copy cleanup on PathTraversalError (supply-chain-security-expert)
• copystat(resolved, ...) instead of copystat(entry, ...) for symlink entries (python-architect)
• prefix removed from PathTraversalError messages (cli-logging-expert)
• Escaping-symlink error now includes fix suggestion (cli-logging-expert)
• Broken symlink test: TestBrokenSymlink class (test-coverage-expert)
• Symlink-to-directory test: TestSymlinkToDirectory class (test-coverage-expert)
• Circular symlink test: TestCircularSymlink class (test-coverage-expert)
• docs/enterprise/security.md: intro contradiction fixed, 'atomically' -> 'per-file', duplicate sentence removed, visited-set item added, cross-link to Path traversal prevention (doc-writer)
• CHANGELOG [Unreleased] Fixed entry for [BUG] Local-path install drops symlinked skill references; remote install keeps them #1668 (doc-writer)

Copilot signals reviewed

No Copilot inline comments found on this PR (copilot_rounds: 1, copilot_drained: true).

Deferred

• Integration test through real install pipeline (supply-chain-security-expert): scope-crossing -- would require tests/integration/ infrastructure not in this PR's scope.
• Cross-link style/issue citation cleanup in security.md (doc-writer): style convention not caused by this change.
• PermissionError handling in iterdir (python-architect): pre-existing pattern not introduced by this PR.

Lint evidence

$ uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/
All checks passed!
1225 files already formatted

$ uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/
Your code has been rated at 10.00/10

$ bash scripts/lint-auth-signals.sh
[+] auth-signal lint clean

CI evidence (post-fold push)

CI GREEN on commit f301b57: all 15 checks passed.

Mergeability

#PR	sha	CEO stance	iterations	folds	deferrals	copilot rounds	ci_status	mergeable	merge_state	notes
#1676	f301b57	ship_with_followups	1	10	3	1	pending	MERGEABLE	BLOCKED	awaiting CI + human review