fix(deps): harden local-dep display against Windows repo_url leak and over-eager hash stripping#2175
Closed
danielmeppiel wants to merge 1 commit into
Closed
fix(deps): harden local-dep display against Windows repo_url leak and over-eager hash stripping#2175danielmeppiel wants to merge 1 commit into
danielmeppiel wants to merge 1 commit into
Conversation
… over-eager hash stripping Two verified follow-ups to the v0.25.0 local-dependency display hardening (PR #2173), both leaking or corrupting the user-facing local-dep identity. 1) Windows-path repo_url pollution. A Windows-form declared path parsed on POSIX (`C:\packages\pkg`) leaves the drive and backslashes embedded in `repo_url` (`_local/C:\packages\pkg`). `_dep_display_name` fell back to that polluted `repo_url` for absolute paths, leaking host structure. Derive a hash-free `_local/<basename>` from `local_path` instead, via a new `_local_basename` helper that routes drive/UNC/backslash paths through PureWindowsPath and everything else through PurePosixPath. Relative paths stay verbatim; a missing `local_path` still falls back to `repo_url`. 2) Over-eager orphan hash stripping. `_resolve_scope_deps` stripped the hash segment from every scanned `_local/<12hex>/pkg` name, corrupting a legitimately declared/mapped slot into `_local/pkg` and masking it. Gate the strip on a CONFIRMED orphan (no lockfile mapping): declared/mapped slots keep their logical name verbatim. Orphan detection still runs on the raw physical identity. TDD: added a real parse -> LockedDependency -> display regression and a declared-hashed-slot preservation test (both RED before the fix). Existing true-orphan and read-failure guards stay green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4c5a1ddc-ce71-4a10-b527-8796df88975e
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens apm deps list / apm deps tree local-dependency display to avoid leaking Windows-path artifacts (drive letters/backslashes) into user-facing output and to prevent incorrectly stripping hash-like segments from declared local dependency identities.
Changes:
- Add
_local_basename()and update_dep_display_name()so absolute local paths render as_local/<basename>(derived fromlocal_path) instead of falling back to potentially pollutedrepo_url. - Refine
_resolve_scope_deps()to strip_local/<12hex>/...hash segments only for confirmed orphan slots (no lockfile mapping), preserving legitimately hashed logical keys. - Add unit regressions covering Windows-path
repo_urlpollution and declared hashed-slot preservation.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/commands/deps/cli.py |
Implements cross-platform local basename derivation and gates hash stripping to confirmed orphans during deps scan/display. |
tests/unit/commands/test_deps_cli_helpers.py |
Adds regression tests for Windows-path pollution and for preserving declared hashed local slot identities. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Two verified pre-merge follow-ups to the v0.25.0 local-dependency display hardening (PR #2173, now merged). Both leaked or corrupted the user-facing local-dependency identity in
apm deps list/treeoutput. Fixed TDD-first; no behavior change for the common relative-path case.Problem (WHY)
PR #2173 hardened local-dep display but two edge cases still leak/corrupt identity:
Windows-path
repo_urlpollution. A Windows-form declared path parsed on POSIX (C:\packages\pkg) leaves the drive letter and backslashes embedded in the parsedrepo_url(_local/C:\packages\pkg), becausePurePosixPath("C:\\...").nametreats backslashes as ordinary characters._dep_display_namefell back to that pollutedrepo_urlfor absolute paths, leaking host filesystem structure into user-facing output.Over-eager orphan hash stripping.
_resolve_scope_depsstripped the hash segment from every scanned_local/<12hex>/pkgslot name. A legitimately declared/mapped slot whose logical key happens to look hashed was corrupted into_local/pkgand masked, instead of being reported under its true logical identity.Approach (WHAT)
Derive a hash-free
_local/<basename>fromlocal_pathfor absolute declared paths, via a new_local_basenamehelper that routes drive/UNC/backslash paths throughPureWindowsPathand everything else throughPurePosixPath. Relativelocal_pathstays verbatim; a missinglocal_pathstill falls back torepo_url.get_unique_keyis still never called for local deps.Gate the hash-strip in the scan loop on a confirmed orphan (a slot with no lockfile mapping to a logical key). Declared/mapped slots keep their logical name verbatim. Orphan detection still runs on the raw physical identity, so detection accuracy is unchanged.
Implementation (HOW)
src/apm_cli/commands/deps/cli.py: add_local_basename; rework_dep_display_namelocal branch; computeis_orphanedbefore the scantryand apply_logical_local_displayonly when orphaned.tests/unit/commands/test_deps_cli_helpers.py: realDependencyReference.parse(r"C:\packages\pkg") -> LockedDependency -> _dep_display_nameregression (asserts no drive/backslashes); declared-hashed-slot preservation test (asserts_local/<12hex>/pkgis preserved, not orphaned). Both were RED before the fix.Trade-offs
Detection intentionally uses
PureWindowsPathheuristics (backslash or drive) rather than importing the heavier path resolver -- keeps this a bounded release patch with no new cross-cutting abstraction.Validation evidence
test_deps_cli_helpers.pytest_transitive_chain_e2e.py(binary shim)How to test
Does not merge or move any tags.