fix(install): dereference in-package symlinks on local install (#1668)#1676
Conversation
Local-path install now materializes in-package symlinks as real files, matching the behavior of remote install (which uses robust_copytree with symlinks=False / git checkout semantics). Before this fix, _copy_local_package used shutil.copytree(symlinks=True), which preserved symlinks in apm_modules/. The downstream ignore_non_content filter in security/gate.py then silently dropped every symlink, causing referenced files to go missing without any warning. Changes: - Add _copy_tree_dereferencing_validated() to local_content.py: walks the source tree atomically (resolve -> ensure_path_within -> copy2 per entry). In-package symlinks are materialized as real files. Symlinks whose resolved target escapes the package root raise PathTraversalError immediately (hard-fail, never warn-and-skip). - Remove symlinks=True / stale SECURITY comment from _copy_local_package. - Add regression tests: in-package symlink materialized; escaping symlink hard-fails; non-symlink baseline unchanged. - Update test_local_deps.py: test_copy_preserves_symlinks_without_following now asserts the correct hard-fail behavior instead of the old preserve-as- symlink behavior. - Add threat-model note to docs/enterprise/security.md describing the dereference behavior and containment guarantee. Security guardrails (all from the maintainer-approved brief): 1. ensure_path_within() validates every symlink target before copy. 2. TOCTOU-safe: atomic per-file resolve->validate->copy2 sequence. 3. Escaping symlinks hard-fail (PathTraversalError), never warn-and-skip. 4. Only in-package symlinks dereferenced; external symlinks rejected. 5. Threat-model note added to security docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
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
| # Dereference in-package symlinks atomically (fix for #1668). | ||
| # Each symlink is resolved -> validated within `local` -> copied as a real | ||
| # file, matching the behavior of the remote install path which uses | ||
| # robust_copytree with symlinks=False. Symlinks that escape the package | ||
| # root raise PathTraversalError immediately (hard-fail, not warn-and-skip). | ||
| _copy_tree_dereferencing_validated(local, install_path, local) | ||
| return install_path |
| if resolved.is_dir(): | ||
| _copy_tree_dereferencing_validated(resolved, dst_entry, pkg_root) | ||
| else: | ||
| shutil.copy2(resolved, dst_entry) | ||
| shutil.copystat(entry, dst_entry) |
| def test_escaping_install_path_is_not_partially_created(self, tmp_path: Path) -> None: | ||
| """When an escaping symlink is found the install_path is not left partial. | ||
|
|
||
| The install must either complete fully or fail atomically. A partial | ||
| tree in apm_modules/ could cause subsequent installs to behave | ||
| incorrectly. | ||
| """ |
…anup, and extended tests Folds panel follow-ups on the security surface introduced by #1668: - Add visited-set cycle guard to _copy_tree_dereferencing_validated to detect circular directory-symlink chains deterministically (O(1), independent of OS ELOOP platform limits). Previously, circular in-package dir symlinks would overflow the Python call stack rather than producing a clear PathTraversalError. - Fix copystat calls to use the resolved path (not the symlink entry) as the stat source for symlink entries. The old call was effectively a no-op (os.stat follows symlinks, giving the same result), but was misleading to readers. - Wrap the _copy_tree_dereferencing_validated call in _copy_local_package with a try/except PathTraversalError + safe_rmtree cleanup to guarantee that no partial install_path content remains on failure. The escaping-symlink test already asserted this, but production code was relying on the outer error propagation without cleanup. - Remove [x] prefix from PathTraversalError message bodies. The rendering layer owns the prefix; embedding [x] inside the exception string causes double-prefix output when the caller's diagnostics handler wraps the error. - Add actionable fix hint to the escaping-symlink error message. - Add three new test classes: TestBrokenSymlink (broken/dangling symlink raises PathTraversalError), TestSymlinkToDirectory (in-package symlink-to-dir materializes as real directory tree), TestCircularSymlink (circular dir symlinks detected and aborted). Brings test file to 9 tests across 6 classes. - Fix docs/enterprise/security.md: soften 'Symlinks are never followed' intro to scope it accurately, update 'resolved atomically' to 'resolved per-file' to match the TOCTOU comment in the code, remove duplicate closing sentence, add visited-set item and cross-link to path-traversal-prevention section. - Add CHANGELOG [Unreleased] Fixed entry for #1668. Mutation-break gate: confirmed that removing the ensure_path_within guard causes TestEscapingSymlinkHardFails tests to fail. Addresses panel follow-ups: cycle guard (supply-chain-security-expert + python-architect, converged), partial-copy cleanup (supply-chain-security-expert), broken-symlink test (test-coverage-expert), copystat fix (python-architect), error message prefix (cli-logging-expert), doc accuracy (doc-writer), CHANGELOG entry (doc-writer). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| 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
-
[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.
-
[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.
-
[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.
-
[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.
-
[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 |
… tests Folds the three remaining panel follow-ups deferred on #1676: - Wrap _copy_tree_dereferencing_validated's iterdir() in try/except OSError, raising a clear PathTraversalError on PermissionError/unreadable package directories instead of leaking a bare OSError up the install stack (python-architect follow-up). - Add integration tests through LocalDependencySource.acquire (real _copy_local_package, no mock) pinning in-package symlink dereference and escaping-symlink hard-fail at the install-pipeline boundary (supply-chain-security-expert follow-up). - Add unit regression-trap for the unreadable-directory error path. - Sync docs/enterprise/security.md and CHANGELOG with the new guarantee. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 3 | Sound escaping-symlink guard and circular-detection logic; nit: redundant copystat after copy2 at line 170; _visited per-branch snapshot semantics need a doc comment to prevent future regression. |
| CLI Logging Expert | 0 | 2 | 2 | Error messages are ASCII-safe and precise; two gaps: no explicit PathTraversalError handler in install.py (double-wrap prefix) and helper accepts no logger (blank --verbose trace). |
| DevX UX Expert | 0 | 3 | 2 | PathTraversalError misused for broken symlinks/permission errors; 3 of 4 error messages omit next-action guidance; install.md not updated for the behavior change. |
| Supply Chain Security Expert | 0 | 3 | 2 | Core POSIX path-traversal fix is sound; three gaps remain: NTFS junction bypass on Windows, RecursionError leaks partial install without cleanup, and a narrow TOCTOU window between validate and copy2. |
| OSS Growth Hacker | 0 | 2 | 2 | Strong trust and parity story with one buried lead: CHANGELOG puts the user-impact hook last, and the hard-fail breaking change needs an explicit callout for upgrading users. |
| Doc Writer | 0 | 3 | 4 | Security.md section is accurate; CHANGELOG misses a blank line; implementation-internal language leaks into 2 items; no stale 'never followed' claims found elsewhere. |
| Test Coverage Expert | 0 | 3 | 1 | All 8 unit scenarios confirmed present; primary security path has integration-tier coverage (S7 unverified); broken-symlink and circular-symlink hard-fails lack integration-tier regression traps. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] Tighten partial-cleanup test assertion from conditional to unconditional -- outcome:failed evidence -- the conditional form (
if install_path.exists(): assert not evil.exists()) silently passes when other partial content remains, directly weakening the regression trap for the PR's own atomicity guarantee. One-line change, highest evidence signal in the panel; fold pre-merge or in an immediate same-day follow-up. - [Supply Chain Security Expert] Add depth counter (cap ~128 levels) or widen the except clause to catch
RecursionErrorand route it through the cleanup gate -- outcome:missing on a security-surface promise. A crafted package with 250-plus nested directories reachesRecursionErrorbeforesafe_rmtreeruns, leaving a partialinstall_pathon disk. - [OSS Growth Hacker + DevX UX Expert] Reorder CHANGELOG entry to lead with the user-impact sentence and add an explicit upgrade notice for the escaping-symlink hard-fail behavior change -- three panelists independently flagged this. CI that was green with an out-of-root symlink will go red on upgrade without warning. Minimum fix: lead with "Previously,
apm installsilently dropped symlinked files" and add "find <pkg-path> -type lto audit before upgrading." - [CLI Logging Expert + DevX UX Expert] Add a dedicated
PathTraversalErrorhandler ininstall.pyimmediately after theDirectDependencyErrorhandler, mirroring the existing pattern -- without it,PathTraversalErrorfalls into the genericexcept Exceptionbranch, producing a double-wrapped message and a misleading "--verbose for diagnostics" hint when the helper has no logger. - [DevX UX Expert + Doc Writer] Update
docs/reference/cli/install.mdBehavior section anddocs/producer/pack-a-bundle.mdto document symlink dereference behavior and the three hard-fail cases -- a CLI behavior change not reflected in the reference docs in the same PR leaves the public contract incomplete.
Architecture
classDiagram
direction LR
class local_content:::touched {
<<Module>>
+_copy_local_package(dep_ref, install_path, base_dir) Path
+_copy_tree_dereferencing_validated(src, dst, pkg_root, _visited) None
+_project_has_root_primitives(project_root) bool
+_has_local_apm_content(project_root) bool
}
class path_security {
<<Module>>
+ensure_path_within(path, base_dir) Path
+safe_rmtree(path, base_dir) None
}
class PathTraversalError {
<<ValueError>>
}
class file_ops {
<<Module>>
+robust_copy2(src, dst) Path
+robust_copytree(src, dst) Path
}
class shutil {
<<StdLib>>
+copy2(src, dst) Path
+copystat(src, dst) None
}
class NullCommandLogger {
<<Fallback>>
+error(msg) None
}
local_content ..> path_security : uses ensure_path_within and safe_rmtree
local_content ..> PathTraversalError : raises on escaping or circular symlink
path_security ..> PathTraversalError : raises on containment violation
local_content ..> shutil : calls copy2 and copystat
local_content ..> NullCommandLogger : fallback when logger is None
path_security ..> file_ops : delegates robust_rmtree
note for local_content "Guard-then-recurse: per-branch _visited snapshot -- _visited | resolved, not in-place mutation"
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[apm install local_path] --> B[_copy_local_package local_content.py:178]
B --> C{local.is_dir?}
C -- No --> D[return None: path missing]
C -- Yes --> E{apm.yml or SKILL.md present?}
E -- No --> F[return None: not a valid APM pkg]
E -- Yes --> G{install_path.exists?}
G -- Yes --> H[safe_rmtree install_path]
G -- No --> I[_copy_tree_dereferencing_validated]
H --> I
I --> J[dst.mkdir parents=True]
J --> K[sorted src.iterdir]
K -- OSError --> KE[raise PathTraversalError: cannot read package dir]
K -- OK --> L{entry type?}
L -- is_symlink --> M[entry.resolve strict=True]
M -- OSError --> ME[raise PathTraversalError: broken symlink]
M -- resolved --> N[ensure_path_within resolved pkg_root]
N -- escapes root --> NE[raise PathTraversalError: symlink escapes package root]
N -- within root --> O{resolved.is_dir?}
O -- Yes and in _visited --> P[raise PathTraversalError: circular symlink]
O -- Yes and not in _visited --> Q[recurse with _visited union resolved]
Q --> R[copystat resolved dst_entry dir timestamps]
R --> L
O -- No file --> S[copy2 resolved dst_entry]
S --> T[copystat redundant: copy2 already called it]
T --> L
L -- is_dir --> U[recurse same _visited]
U --> V[copystat dir timestamps]
V --> L
L -- is_file --> W[copy2 entry dst_entry]
W --> L
L -- done --> X[return install_path]
B -- PathTraversalError --> Y{install_path.exists?}
Y -- Yes --> Z[safe_rmtree cleanup partial]
Z --> AA[re-raise PathTraversalError]
Y -- No --> AA
sequenceDiagram
participant CLI as apm install
participant LCP as _copy_local_package
participant CTDV as _copy_tree_dereferencing_validated
participant PS as ensure_path_within
participant FS as Filesystem
CLI->>LCP: dep_ref, install_path, base_dir, project_root, logger
LCP->>FS: local.is_dir, install_path.parent.mkdir
LCP->>FS: safe_rmtree install_path if exists
LCP->>CTDV: src=local, dst=install_path, pkg_root=local, _visited=None
CTDV->>FS: dst.mkdir and sorted src.iterdir
loop for each entry in src
alt entry is symlink
CTDV->>FS: entry.resolve strict=True
Note over CTDV: PathTraversalError if broken target
CTDV->>PS: ensure_path_within resolved pkg_root
Note over PS: PathTraversalError if escapes pkg_root
alt resolved is dir and not in _visited
CTDV->>CTDV: recurse resolved dst_entry pkg_root _visited|resolved
CTDV->>FS: copystat resolved dst_entry dir timestamps
else resolved is dir and in _visited
Note over CTDV: PathTraversalError circular symlink
else resolved is file
CTDV->>FS: copy2 resolved dst_entry
CTDV->>FS: copystat redundant: copy2 already called it
end
else entry is regular directory
CTDV->>CTDV: recurse entry dst_entry pkg_root same _visited
CTDV->>FS: copystat entry dst_entry dir timestamps
else entry is regular file
CTDV->>FS: copy2 entry dst_entry
end
end
CTDV-->>LCP: success or PathTraversalError
alt PathTraversalError raised
LCP->>FS: safe_rmtree install_path cleanup
LCP-->>CLI: raise PathTraversalError
else success
LCP-->>CLI: return install_path
end
Recommendation
Ship with followups. The core security fix is correct, the unit test suite covers all eight scenarios comprehensively, and no panelist returned a blocking finding. Two items the author should address before merge or in an immediate same-day PR: (1) tighten the partial-cleanup test assertion from conditional to unconditional (supply-chain, outcome:failed evidence -- one line, defends the PR's own atomicity claim), and (2) add an explicit upgrade notice to the CHANGELOG for the escaping-symlink hard-fail behavior change (three-panelist convergence, trust-critical for upgrading users). All remaining followups -- the dedicated PathTraversalError handler in install.py, the install.md and pack-a-bundle.md documentation gaps, the RecursionError depth cap, the NTFS junction bypass note in security.md, and the error-type redesign (PathTraversalError vs. a new LocalInstallError) -- are scoped to follow-up issues and do not block the security value this PR delivers.
Full per-persona findings
Python Architect
- [recommended]
_visitedper-branch snapshot semantics are correct but silently breakable by a future maintainer atsrc/apm_cli/install/phases/local_content.py:90
The_visitedset uses_visited | {resolved}(immutable snapshot per branch). This is correct for diamond-topology packages (two symlinks to the same dir are both materialized) while true cycles are still caught. A future maintainer may replace with_visited.add(resolved), breaking diamond-sharing. Minimal fix: add a docstring sentence. "Per-branch immutable snapshot --_visited | {resolved}, not in-place mutation. Do not replace with_visited.add(resolved)-- that breaks diamond-topology packages where two symlinks point to the same directory." - [nit]
shutil.copystatat line 170 is redundant aftershutil.copy2for the symlink-to-file branch atsrc/apm_cli/install/phases/local_content.py:170
shutil.copy2internally callscopystat. The explicitcopystatat line 170 is a no-op second metadata pass. Directorycopystatcalls at lines 167 and 173 are correct and necessary.
Suggested: Remove line 170. Keepcopystatat lines 167 and 173. - [nit] Missing type annotation on
_visitedparameter atsrc/apm_cli/install/phases/local_content.py:90
src,dst,pkg_roothave type hints;_visitedhas none.
Suggested:def _copy_tree_dereferencing_validated(src: Path, dst: Path, pkg_root: Path, _visited: set[Path] | None = None) -> None: - [nit] Raw
shutil.copy2diverges from install-pipeline convention ofrobust_copy2atsrc/apm_cli/install/phases/local_content.py:169
github_downloader.pyandartifactory_orchestrator.pyboth userobust_copy2fromapm_cli.utils.file_ops(reflink clone before fallback). Local same-filesystem copies would benefit from reflink.
Suggested: Importrobust_copy2and replaceshutil.copy2at lines 169 and 175.
CLI Logging Expert
- [recommended]
PathTraversalErrorfalls into genericexcept Exceptionhandler -- message is double-wrapped and the fix hint is buried atsrc/apm_cli/commands/install.py:1590
PathTraversalError(ValueError subclass) is not caught explicitly ininstall.py. It falls through toexcept Exception, producing[x] Error installing dependencies: Symlink ... Local install aborted.-- the actionable fix hint is buried.DirectDependencyErrorandPolicyViolationErrorboth have dedicated handlers;PathTraversalErrordeserves the same since its messages carry fix instructions. The generic handler also emits "Run with --verbose for detailed diagnostics" which is misleading because the helper has no logger.
Suggested: Addexcept PathTraversalError as e: ... logger.error(str(e)); sys.exit(1)immediately after theDirectDependencyErrorhandler at line 1593. Mirror in the second install block near line 1814. - [recommended] Logger not forwarded to
_copy_tree_dereferencing_validated----verboseyields zero per-symlink trace atsrc/apm_cli/install/phases/local_content.py:90
The logger accepted by_copy_local_packageis not passed to the helper. In--verbosemode, users see no per-entry progress. The silent cleanup (safe_rmtree) is also unlogged.
Suggested: Addlogger=Noneparameter to helper, thread through recursive calls, addlogger.verbose_detailcalls on dereference and cleanup. - [nit] Circular and broken-symlink error messages lack fix hints; only escape message has actionable recovery advice at
src/apm_cli/install/phases/local_content.py:141
Suggested: Broken: append "Fix or remove the broken symlink and retry." Circular: append "Remove one of the circular symlinks or replace it with a regular file copy and retry." - [nit] "Local install aborted." suffix is redundant when rendered via the generic handler at
src/apm_cli/install/phases/local_content.py:131
No change needed once a dedicatedPathTraversalErrorhandler is added toinstall.py(follow-up 4 above). If deferred, consider "Aborting install." to reduce verbosity.
DevX UX Expert
- [recommended]
PathTraversalErrorreused for broken symlinks and permission errors conflates security violations with innocent operational failures atsrc/apm_cli/install/phases/local_content.py:141
PathTraversalErroris documented as "Raised when a computed path escapes its expected base directory." A dangling symlink (OSError fromresolve(strict=True)) orPermissionErroroniterdiris neither. The install pipeline labelsPathTraversalErrorcatches as "Path-safety violation". Wrapping a stale symlink in a security-named exception trains CI monitors to treat it as a containment breach.
Suggested: IntroduceLocalInstallError(orPackageContentError) for non-containment failures: broken symlinks, circular chains, unreadable directories. ReservePathTraversalErrorstrictly for the escaping-symlink case. (CEO: deferred to follow-up issue -- cross-cutting refactor with test-migration cost.) - [recommended] Broken symlink, circular symlink, and unreadable directory error messages all omit a concrete next-action step at
src/apm_cli/install/phases/local_content.py:142
Only the escaping-symlink case has a fix hint. The other three errors end without guidance.
Suggested: Broken: "Remove or fix the symlink target, then re-run apm install." Circular: "Remove the circular symlink loop from your package, then re-run apm install." Unreadable dir: "Check filesystem permissions on the directory, then re-run apm install." - [recommended]
docs/reference/cli/install.mdnot updated to document the new symlink dereference behavior or new hard-fail cases atdocs/src/content/docs/reference/cli/install.md:105
The Behavior section covers local-path installs but contains no mention of symlink dereference or the three hard-fail cases.
Suggested: Add Behavior bullet: "Local-path symlink handling. In-package symlinks are dereferenced into regular files... Symlinks whose resolved target escapes the package root, broken symlinks, and circular chains hard-fail with a clear error naming the offending path." - [nit] CHANGELOG entry says circular/unreadable "detected deterministically" but does not say they hard-fail; broken symlinks not mentioned at
CHANGELOG.md
Suggested: Extend: "...hard-fail with a PathTraversalError; broken or dangling symlinks and circular directory-symlink chains also hard-fail with a clear error naming the offending path." - [nit] Missing blank line between the new CHANGELOG bullet and
### Changedsection header atCHANGELOG.md
Supply Chain Security Expert
- [recommended] NTFS junction points bypass
is_symlinkcheck on Windows; traverse without containment guard atsrc/apm_cli/install/phases/local_content.py:171
Path.is_symlink()returns False for NTFS junction points (IO_REPARSE_TAG_MOUNT_POINT). On Windows, a malicious local package with a junction pointing outside would fall into theelif entry.is_dir()branch with noensure_path_withincheck --pkg_rootboundary never applied. Linux and macOS unaffected.
Suggested: Add Windows junction detection guard, or document as known limitation insecurity.md.
Proof (missing at manual-only): No Windows junction test exists --is_symlink()returns False for NTFS junctions so the gap is confirmed by Python lstat semantics. - [recommended]
RecursionErroron deep package trees bypassesPathTraversalErrorcleanup gate, leaving partialinstall_pathatsrc/apm_cli/install/phases/local_content.py:276
Effective max depth beforeRecursionErroris ~200-240 levels. A crafted package with 250-plus nested directories triggers it. Thetry/exceptonly catchesPathTraversalError;RecursionErrorpropagates unhandled, skippingsafe_rmtree. Contradicts the PR's atomicity claim.
Suggested: Widenexceptto catchRecursionErrorand route through cleanup, or add depth counter capped at 128 levels.
Proof (missing at unit): No test verifies thatRecursionErrorduring copy triggers the partial-install cleanup path. - [recommended] TOCTOU window between
ensure_path_withinandcopy2is narrowed but not closed atsrc/apm_cli/install/phases/local_content.py:169
Betweenensure_path_withinandcopy2, a concurrent process could replace the file with an escaping symlink.shutil.copy2follows symlinks by default. Low practical risk for user-controlled local packages but should be documented.
Suggested: Add docstring andsecurity.mdnote that a narrow TOCTOU window remains when the package source directory is concurrently writable. - [nit] Partial-cleanup test asserts conditionally on
install_path.exists()instead of asserting unconditionally attests/unit/install/test_local_content_symlink_deref.py:209
Test usesif install_path.exists(): assert not evil.exists()which silently passes if other partial content remains.
Suggested: Replace withassert not install_path.exists()
Proof (failed at unit):TestEscapingSymlinkHardFails::test_escaping_install_path_is_not_partially_created-- conditional form weakens the atomicity regression trap. - [nit]
shutil.copy2already callscopystatinternally; extracopystaton file symlinks at line 170 is a redundant syscall atsrc/apm_cli/install/phases/local_content.py:170
Suggested: Remove line 170 (file-symlink branch only; keep directorycopystatat lines 167 and 173).
OSS Growth Hacker
- [recommended] CHANGELOG buries the user-impact lead; silent data loss should open the sentence, not close it at
CHANGELOG.md
Package authors who hit this bug spent time debugging why their shared-contract.md was missing. The recognition trigger -- "Previously, in-package symlinks were silently dropped" -- is the last clause. Moving it first transforms this from a maintainer note into a user-resonant fix story.
Suggested: Lead with: "Previously,apm install <local-path>silently dropped symlinked files during deployment. The fix materializes in-package symlinks as real files so local and remote installs produce consistent output..." - [recommended] Escaping-symlink hard-fail is an implicit behavior change that needs an upgrade notice in CHANGELOG at
CHANGELOG.md
Before: escaping symlinks silently dropped/preserved. After: hard-fail. Any user with an out-of-root symlink (even accidentally) will see a newPathTraversalErroron upgrade. CI that was green will go red.
Suggested: Add: "If your local package contains symlinks pointing outside the package root that previously installed without error, those installs will now fail; audit withfind <pkg-path> -type land replace escaping symlinks with real file copies before upgrading." - [nit]
security.mdcontainment guarantee is an enterprise evaluator asset worth a release-note call-out, not just inline prose atdocs/src/content/docs/enterprise/security.md - [nit]
first-package.mdteaches package authoring but never mentions that in-package symlinks to shared content are a supported pattern
Suggested: Add tip after skill-authoring section about using in-package symlinks for shared content, with a link to the symlink handling section ofsecurity.md.
Auth Expert -- inactive
The six changed files (local_content.py, test_local_content_symlink_deref.py, test_local_deps.py, test_install_sources_classification.py, CHANGELOG.md, enterprise/security.md) concern only local-filesystem symlink dereferencing during package staging and carry zero contact with AuthResolver, token management, credential resolution, host classification, or any remote-auth surface.
Doc Writer
- [recommended] New subsection adds ~300 words with no compensating cuts at
docs/src/content/docs/enterprise/security.md:240
PROSE non-bloat: if word count increases, something must shrink or the trade-off must be documented. The PR does not document the delta.
Suggested: Consolidate items 3+4 (same rule stated twice from opposite directions) and items 5+6 into single sentences each, saving ~80 words. - [recommended]
PathTraversalErroruser-visible message not shown; developers hitting the error cannot cross-reference docs atdocs/src/content/docs/enterprise/security.md:257
The section guarantees a "human-readable message naming the offending link" but never shows it. A developer encountering the error mid-install cannot confirm they are seeing the expected signal.
Suggested: Add a code block after item 3 showing the actual error message format. - [recommended] Producer guide gap: package authors using symlinks have no page explaining local-install dereference behavior
Suggested: Add note indocs/producer/pack-a-bundle.md: "When a consumer installs your package from a local path, APM dereferences in-package symlinks into regular files. Symlinks whose resolved target escapes the package root abort the install. See Symlink handling." - [nit] CHANGELOG missing blank line between last Fixed entry and
### Changedheading atCHANGELOG.md:24 - [nit] Item 4 contains a redundant second sentence: "External symlinks are never followed." at
docs/src/content/docs/enterprise/security.md:260
Item 4 already states "Only symlinks that resolve within the package root are dereferenced." The negative restatement adds no information. - [nit] Items 5 and 6 use implementation-internal language ("visited-set guard", "OS-level ELOOP limits", "bare OS error up the install stack") at
docs/src/content/docs/enterprise/security.md:261
Suggested: Item 5: "Circular directory-symlink chains fail the install with a PathTraversalError." Item 6: "An unreadable package directory fails the install with a PathTraversalError rather than a raw OS error." - [nit] Broken-target symlink case is a hard-fail but not covered by any of the 6 documented properties at
docs/src/content/docs/enterprise/security.md:252
Suggested: Extend item 1 or add item 7: "A symlink whose target cannot be resolved (broken link) also hard-fails the install with a PathTraversalError."
Test Coverage Expert
- [nit] Regression trap
test_copy_preserves_symlinks_without_followingcorrectly asserts hard-fail, not preserved-as-symlink attests/unit/test_local_deps.py:586
Verified at lines 586-624. The test correctly creates an escaping symlink and assertsPathTraversalErroris raised. Thematch=r'(?i)escape|outside|traversal'pattern matches the new error message. Regression trap quality is correct.
Proof (passed at unit):TestCopyLocalPackage::test_copy_preserves_symlinks_without_following--with pytest.raises(PathTraversalError, match=r'(?i)escape|outside|traversal'): _copy_local_package(...) - [recommended] Integration-tier tests for
acquire()boundary are structurally correct but S7-unverifiable: pytest not available in review env attests/integration/test_install_sources_classification.py:578
TestLocalInstallSymlinkDerefThroughPipelinedoes NOT mock_copy_local_package-- onlydetect_package_typeis mocked, which is correct. Structurally exercises the full pipeline. Cannot certify with run evidence from this environment.
Suggested: Runpytest tests/integration/test_install_sources_classification.py::TestLocalInstallSymlinkDerefThroughPipeline -vto certify before merge.
Proof (unknown at integration-with-fixtures):test_in_package_symlink_materialized_as_real_file--assert staged_link.is_file() and not staged_link.is_symlink() - [recommended] Broken (dangling) symlink hard-fail path is unit-only; missing integration-tier regression trap at
tests/integration/test_install_sources_classification.py
The broken-symlink branch is a distinct code path from the escaping-symlink branch. The existing integration test would not catch a regression in the broken-symlink path.
Suggested: Addtest_broken_symlink_hard_fails_through_pipelinetoTestLocalInstallSymlinkDerefThroughPipeline.
Proof (missing at integration-with-fixtures):TestLocalInstallSymlinkDerefThroughPipeline::test_broken_symlink_hard_fails_through_pipelinedoes not exist --with pytest.raises(PathTraversalError, match=r'(?i)(broken|unresolvable)'): source.acquire() - [recommended] Circular directory-symlink detection is unit-only; visited-set guard has no integration-tier regression trap at
tests/integration/test_install_sources_classification.py
If_visitedtracking regressed, the install would recurse deeply before hitting OS ELOOP. No integration test covers this path.
Suggested: Addtest_circular_symlink_hard_fails_through_pipelinetoTestLocalInstallSymlinkDerefThroughPipeline.
Proof (missing at integration-with-fixtures):TestLocalInstallSymlinkDerefThroughPipeline::test_circular_symlink_hard_fails_through_pipelinedoes not exist --with pytest.raises(PathTraversalError, match=r'(?i)(circular|visited)'): source.acquire()
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pypi.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #1676 · sonnet46 18.4M · ◷
github-actions
Bot
removed
the
panel-review
Tighten the local symlink dereference helper API, remove redundant metadata copying, improve remediation text, and align the security docs with the observable local/remote install guarantee surfaced by the panel review for PR #1676. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Recursive copy boundary is sound; folded helper API and redundant metadata-copy cleanup. |
| CLI Logging Expert | 0 | 0 | 3 | Error text is actionable; folded remediation hints for broken, circular, and unreadable paths. |
| DevX UX Expert | 0 | 0 | 3 | Install behavior now matches user expectations; folded changelog formatting. |
| Supply Chain Security Expert | 0 | 0 | 2 | Containment is fail-closed; residual local TOCTOU is not a new risk for this trusted local source model. |
| OSS Growth Hacker | 0 | 0 | 3 | The changelog and security docs now lead with the user-visible install parity and trust signal. |
| Doc Writer | 0 | 1 | 3 | Folded the inaccurate remote-git-checkout wording and documented broken-symlink failure. |
| Test Coverage Expert | 0 | 0 | 0 | Regression and security tests exist at unit and integration-with-fixtures tiers. |
Recommendation
Ship after maintainer review. No in-scope panel or Copilot follow-up remains open after the folded commit, and the latest pushed SHA is CI-green.
Folded in this run
- (panel) Made
_copy_tree_dereferencing_validatedkeep recursive_visitedstate keyword-only and removed redundantcopystataftercopy2for symlinked files -- resolved in9962765f. - (panel) Added remediation hints for broken symlink, circular symlink, and unreadable directory failures -- resolved in
9962765f. - (panel) Corrected security docs to describe observable local/remote output parity rather than git checkout mechanics -- resolved in
9962765f. - (panel) Documented broken/unresolvable symlink hard-fail behavior and fixed markdown spacing in the security page and changelog -- resolved in
9962765f.
Copilot signals reviewed
No copilot-pull-request-reviewer[bot] inline comments were present in the pull request comments API after two fetch rounds; the summary-only review was reviewed.
Regression-trap evidence (mutation-break gate)
tests/unit/install/test_local_content_symlink_deref.py::TestInPackageSymlinkIsDerefed::test_in_package_symlink_materialized_as_real_file-- replaced the dereferencing copy path withshutil.copytree(..., symlinks=True); test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both silent. Full local lint mirror also passed pylint R0801 and scripts/lint-auth-signals.sh.
CI
All PR checks were green on 9962765f4d8b5309b5e30c99bb14290ddb887093 after 0 CI fix iterations, including Lint, Build & Test Shard 1/2, Coverage Combine, CodeQL, APM Self-Check, PR Binary Smoke, Spec conformance gate, NOTICE Drift Check, and license/cla.
Mergeability status
Captured from gh pr view 1676 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1676 | 9962765 |
ship_now | 1 | 4 | 0 | 2 | green | MERGEABLE | BLOCKED | awaiting maintainer review |
Convergence
1 outer iteration; 2 Copilot rounds. Final panel stance: ship_now.
Ready for maintainer review.
…arency) Sync main commits #1700, #1676, #1694, #1710. The #1700 feature (surface installed hook actions during install) wove display-payload tracking through code my refactor extracted into sibling helpers; resolve two conflicts by keeping the extracted structure and porting #1700 semantics: - services.py: accept the extracted _log_per_kind_results() call; move _log_hook_display_payloads into services_integrate.py (avoids circular import) and re-export it from services.py; emit hook summaries inside _log_per_kind_results. - hook_integrator._integrate_merged_hooks: accept extracted _merge_hook_file_entries / _write_merged_config; thread per-file display data out via a new optional capture_entries kwarg on _merge_hook_file_entries; build display payloads after _write_merged_config finalizes (post _apm_source strip). - Keep hook_integrator.py <=800 by relocating _iter_hook_entries / _summarize_command / _build_display_payload to hook_transforms.py and _parse_hook_json to hook_merge.py as thin delegators. Shadow gate green: ruff, ruff format, pylint R0801 10.00/10 (EXIT=0), auth-signals, import smoke, 3178 tests pass. All files within 800-line guard (hook_integrator.py 799, hook_merge.py 766, hook_transforms.py 601, services.py 690, services_integrate.py 307). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fix(install): dereference in-package symlinks on local install
TL;DR
Local-path
apm installnow 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)
The divergence had two root causes:
_copy_local_packagecalledshutil.copytree(..., symlinks=True), preservingevery symlink in
apm_modules/rather than materializing its content.ignore_non_content(security/gate.py) excludedall symlinks from the copy into target directories -- including in-package
ones that were safe to follow.
The old code also carried an explicit
SECURITYcomment acknowledging an openTOCTOU window and deferring the fix -- this PR closes that window.
Approach (WHAT)
local_content.pycopytree(symlinks=True)with_copy_tree_dereferencing_validated()resolve(strict=True)->ensure_path_within()->copy2PathTraversalErrorimmediately; no warn-and-skip, no partial copyignore_non_content(gate.py)docs/enterprise/security.mdImplementation (HOW)
src/apm_cli/install/phases/local_content.pyAdded
_copy_tree_dereferencing_validated(src, dst, pkg_root): walks the sourcetree recursively. For each symlink entry it atomically calls
entry.resolve(strict=True), passes the result toensure_path_within(resolved, pkg_root)(the #596 containment helper), thenshutil.copy2(resolved, dst_entry)for files or recurses for directories. Any symlink escaping
pkg_rootraisesPathTraversalErrorwith a human-readable message and aborts the entire copy.Replaced the
shutil.copytree(symlinks=True)call and the accompanyingunresolved 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 fileTestEscapingSymlinkHardFails-- escaping symlink raisesPathTraversalError; no partial copyTestNonSymlinkFilesUnchanged-- non-symlink baseline unchangedtests/unit/test_local_deps.pyUpdated
test_copy_preserves_symlinks_without_followingto assert the newcorrect behavior (hard-fail) instead of the old incorrect behavior
(preserved-as-symlink with silent downstream drop).
docs/src/content/docs/enterprise/security.mdAdded "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 --> JsequenceDiagram 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_pathTrade-offs
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.
ignore_non_contentchange. By fixing the staging phase, the deployfilter naturally handles in-package content as regular files. Keeping the
filter intact preserves its protection for any residual symlinks from other
code paths.
copystaton symlinks. Callingshutil.copystat(entry, dst_entry)afterthe dereference preserves the link's mtime, not the target's -- acceptable for
a package staging copy where timestamps are not semantically significant.
Benefits
in-package symlinks to share reference content (e.g. shared-contract.md).
a silent no-op, closing a latent attack surface.
validate, and copy are now a single atomic sequence per file.
threat model.
Validation evidence
Scenario Evidence
TestInPackageSymlinkIsDerefed::test_in_package_symlink_materialized_as_real_fileTestEscapingSymlinkHardFails::test_symlink_escaping_package_root_raisesTestEscapingSymlinkHardFails::test_escaping_install_path_is_not_partially_createdTestNonSymlinkFilesUnchanged::test_regular_files_copied_intactHow to test
apm install /tmp/symlink-demo --target claudeand verify.claude/skills/demo/references/shared.mdis a regular file with correct content.pytest tests/unit/install/test_local_content_symlink_deref.py -v--all 6 tests must pass.
ln -s /etc/hosts /tmp/symlink-demo/evil),run
apm install /tmp/symlink-demoand confirm the install fails with a clearPathTraversalErrormessage naming the offending symlink.