fix(policy): resolve project_root before path-traversal check

[qorexdevs]

Description

_get_cache_dir passes an unresolved project_root to ensure_path_within, which then resolves candidate and base_dir independently. On Windows, Path.resolve() on an existing path expands 8.3 short names (e.g. RUNNER~1 -> runneradmin) and may add the \?\ extended-length prefix, but on a not-yet-existing path it keeps the original form. When candidate doesn't exist yet (first writer in a concurrent scenario), the two sides resolve to different forms and is_relative_to fails with PathTraversalError.

Two changes:

• discovery.py: resolve project_root once at the top of _get_cache_dir so the candidate path inherits the long-name form
• path_security.py: strip the \?\ extended-length prefix in ensure_path_within before comparison, since Windows resolve() adds it inconsistently

Fixes #886

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass (472 policy unit tests green)
• Added regression test (test_unresolved_project_root_does_not_raise) that passes a symlink-indirect project root through _get_cache_dir

[qorexdevs]

@microsoft-github-policy-service agree

[github-actions]

APM Review Panel Verdict

Disposition: APPROVE (with one required pre-merge action: CHANGELOG entry)

---

Per-persona findings

Python Architect:

This is a routine bug fix (one new private helper, one early-resolve call). The module is purely procedural; the class diagram below shows module boundary and function roles.

OO / class diagram

classDiagram
    direction TD
    class path_security {
        <<Module>>
        +validate_path_segments(path_str, context, reject_empty, allow_current_dir)
        +ensure_path_within(path, base_dir) Path
        +safe_rmtree(path, base_dir)
        -_strip_extended_prefix(p) Path
    }
    class PathTraversalError {
        <<Exception>>
    }
    class discovery_get_cache_dir {
        <<CallerFunction>>
        +_get_cache_dir(project_root) Path
    }
    path_security ..> PathTraversalError : raises
    discovery_get_cache_dir ..> path_security : calls ensure_path_within
    note for path_security "_strip_extended_prefix: new private helper\nensure_path_within: containment guard\nboth modified in this PR"
    note for discovery_get_cache_dir "project_root = project_root.resolve()\nadded early (#886)"
    class path_security:::touched
    class discovery_get_cache_dir:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

Execution flow diagram

flowchart TD
    A["_get_cache_dir(project_root)"] --> B["[FS] project_root = project_root.resolve() -- NEW #886"]
    B --> C["base = project_root / apm_modules"]
    C --> D["candidate = base / POLICY_CACHE_DIR"]
    D --> E["ensure_path_within(candidate, project_root)"]
    E --> F["[FS] resolved = path.resolve()"]
    E --> G["[FS] resolved_base = base_dir.resolve()"]
    F --> H["_strip_extended_prefix(resolved) -- NEW #886"]
    G --> I["_strip_extended_prefix(resolved_base) -- NEW #886"]
    H --> J["resolved.is_relative_to(resolved_base)"]
    I --> J
    J -->|False| K["raise PathTraversalError"]
    J -->|True| L["return resolved -- prefix-stripped on Windows"]
    L --> M["return candidate"]

Loading

Design patterns

• Used in this PR: Pure helper (guard normalizer) -- _strip_extended_prefix is a side-effect-free normalization function used as a pre-condition inside ensure_path_within. No OO pattern needed at this scope.
• Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope.

Structural note (follow-up, not blocker): ensure_path_within now returns a prefix-stripped path on Windows. If any call site uses the returned Path for a long-path-sensitive operation (>260 chars), stripping \\?\ could silently break it. Current callers (_get_cache_dir, safe_rmtree) discard the return value, so there is no immediate issue -- but worth auditing all call sites for long-path awareness.

---

CLI Logging Expert: No changes to CLI output, CommandLogger, DiagnosticCollector, _rich_* helpers, or STATUS_SYMBOLS. The fix is entirely below the output layer. No logging anti-patterns introduced. Compliant.

---

DevX UX Expert: No CLI surface changes. The Windows path normalization fix is transparent to users -- no behavioral change is visible at the command surface. PathTraversalError messages are unchanged and already name what failed. No new flags, no command shape changes. Compliant.

---

Supply Chain Security Expert: path_security.py is listed as an explicit chokepoint -- this warrants close scrutiny.

1. Symmetric stripping: _strip_extended_prefix is applied to BOTH resolved AND resolved_base. Asymmetric application would create bypass potential; the symmetric form here is correct. [+]
2. Stripping after resolve(): The prefix is stripped after path.resolve() has followed symlinks. Symlink traversal attacks remain caught. [+]
3. \\?\ prefix semantics: \\?\C:\foo and C:\foo refer to the same physical location on Windows. Stripping after resolution does not weaken containment. [+]
4. Direction of fix: The change addresses a false-negative (path inside bounds rejected as outside bounds due to prefix mismatch), not a false-positive (outside-bounds path accepted). Correctness is tightened, not loosened. [+]
5. UNC form gap (pre-existing, not introduced here): \\?\UNC\server\share is not stripped. Unlikely for apm_modules/.policy-cache but worth noting.

No security regression. Containment is preserved. Implementation is correct.

---

Auth Expert: Not activated -- the changed files (src/apm_cli/utils/path_security.py, src/apm_cli/policy/discovery.py) contain only Windows path normalization logic with no authentication, token management, host classification, or credential resolution surface.

---

OSS Growth Hacker: Internal infrastructure fix -- no direct conversion surface (README, quickstart) affected. Windows compatibility reliability is a quiet enterprise trust signal for Windows-heavy teams. No story angle strong enough to stand alone; if two or more Windows fixes cluster in the same release, worth batching under a "works everywhere" release note. Side-channel to CEO: tagging this in the next release narrative compounds the "Windows reliability" positioning advantage at low cost. No WIP/growth-strategy.md update required.

---

CEO arbitration

All specialists agree -- no disagreements to arbitrate. The change is a narrow, well-targeted fix for a Windows-specific path comparison failure caused by inconsistent \\?\ prefix presence from Path.resolve(). The fix is correct (symmetric stripping, stripping after symlink resolution, early normalize in _get_cache_dir), the tests are appropriate (symlink regression + updated expectation), and no security or UX surface is disturbed.

The one gap is structural, not behavioral: the PR carries no CHANGELOG.md entry. Repo convention is unambiguous -- every merged PR with code changes needs a changelog entry. This is a one-liner fix and does not warrant blocking the merge, but it must be added before landing. APPROVE once the CHANGELOG entry is in.

---

Required actions before merge

1. Add a CHANGELOG.md entry under ## [Unreleased] > ### Fixed for this fix. Suggested line: `path_security.ensure_path_within` and `_get_cache_dir`: fix false `PathTraversalError` on Windows when `Path.resolve()` inconsistently adds the `\\?\` extended-length prefix (#886)

---

Optional follow-ups

• Audit all call sites of ensure_path_within for long-path sensitivity (>260 chars on Windows). The function now returns a prefix-stripped path; callers that pass the return value to further filesystem operations could be affected if paths exceed the traditional MAX_PATH limit.
• Add a test case for the \\?\UNC\ form (network long-path shares) in test_pr_832_findings.py to document the current behavior boundary.

Generated by PR Review Panel for issue #895 · ● 655.1K · ◷