Harden Artifactory zip extraction#1948
Conversation
fallintoplace
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
This PR hardens Artifactory VCS ZIP extraction by routing both multi-entry and single-file archives through the shared safe_extract_zip() helper, preserving root-prefix stripping while failing closed on unsafe entries (e.g., path traversal) and enforcing the shared uncompressed-size limits.
Changes:
- Added a
member_name_transformhook tosafe_extract_zip()to support root-prefix stripping while keeping centralized ZIP safety checks. - Refactored Artifactory archive extraction in
DownloadDelegateto use the shared safe extractor and surface unsafe-archive errors consistently. - Added/updated unit + hermetic integration coverage for traversal rejection and uncompressed-size limit enforcement.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/utils/archive.py |
Extends safe_extract_zip() with a member-name transform hook so callers can strip prefixes without bypassing safety checks. |
src/apm_cli/deps/download_strategies.py |
Introduces _extract_artifactory_zip() and routes Artifactory ZIP extraction through safe_extract_zip() with fail-closed handling for unsafe members. |
tests/unit/test_artifactory_support.py |
Updates Artifactory edge-case tests to assert traversal is rejected and adds a regression for uncompressed-size limits. |
tests/unit/deps/test_download_strategies_selection.py |
Adds unit coverage for single-file traversal rejection and uncompressed-size limit failures in Artifactory downloads. |
tests/unit/deps/test_download_strategies_phase3.py |
Mirrors the selection test coverage for the phase3 suite (same new Artifactory ZIP safety regressions). |
tests/integration/test_artifactory_install_e2e.py |
Updates the hermetic Artifactory stub extraction logic to use safe_extract_zip() and match production behavior. |
Comments suppressed due to low confidence (1)
src/apm_cli/utils/archive.py:201
- safe_extract_zip gained
member_name_transform, but the docstring does not describe what the hook does or that the returnedextractedlist now contains transformed names. This makes the new API harder to use correctly.
"""Safely stream-extract *zf* under *dest_root* with zip-bomb limits.
The uncompressed-size limit is enforced against bytes actually read from
each entry, not against attacker-controlled ZipInfo.file_size metadata.
"""
| headers["Authorization"] = f"Bearer {self._host.artifactory_token}" | ||
| return headers | ||
|
|
||
| def _extract_artifactory_zip(self, zf, target_path: Path, url: str) -> None: |
| target = self.temp_dir / "pkg" | ||
| monkeypatch.setitem(safe_extract_zip.__kwdefaults__, "max_uncompressed", 1024) |
| d, _ = self._delegate([resp]) | ||
| monkeypatch.setitem(safe_extract_zip.__kwdefaults__, "max_uncompressed", 1024) |
| d, _ = self._delegate([resp]) | ||
| monkeypatch.setitem(safe_extract_zip.__kwdefaults__, "max_uncompressed", 1024) |
Adds the required integration regression for malicious Artifactory ZIP traversal through the real downloader path, records the security change in the changelog, and preserves response-close failures in debug diagnostics. Addresses shepherd panel follow-ups for PR microsoft#1948. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the required integration regression for malicious Artifactory ZIP traversal through the real downloader path, records the security change in the changelog, and preserves response-close failures in debug diagnostics. Addresses shepherd panel follow-ups for PR microsoft#1948. apm-spec-waiver: Internal archive extraction security hardening does not extend the OpenAPM package specification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
fix/artifactory-safe-zip-extraction
branch
from
72cf249 to
40c1704
Compare
Empty commit to retrigger upstream pull_request checks after the maintainer force-with-lease update. No code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Documents that the malicious-zip integration regression uses the production DownloadDelegate against local HTTP, keeping the test's coverage tier clear for reviewers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…zip-extraction # Conflicts: # CHANGELOG.md
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | The final shape keeps Artifactory extraction behind the shared archive utility without adding a parallel extraction policy. |
| CLI Logging Expert | 0 | 0 | 0 | No user-facing CLI output changes; debug-only close diagnostics are appropriately quiet. |
| DevX UX Expert | 0 | 0 | 0 | Install failure remains fail-closed and localized to archive validation errors. |
| Supply Chain Security Expert | 0 | 0 | 0 | Zip-slip and unsafe extraction risk are closed on the Artifactory path; no remaining in-scope security reservations. |
| OSS Growth Hacker | 0 | 0 | 0 | Security hardening is recorded in the changelog with no extra user burden. |
| Doc Writer | 0 | 0 | 0 | Changelog and docstring updates are accurate and concise. |
| Test Coverage Expert | 0 | 0 | 0 | The required integration regression trap exists and mutation-proves the guard. |
| Performance Expert | 0 | 0 | 0 | No new hot-path concern from the folded changes. |
B = blocking-severity findings, R = recommended, N = nits. Counts are signal strength, not gates. The maintainer ships.
Recommendation
Ship now from the advisory panel lens. The remaining note about buffering Artifactory archives is broader performance work and is not part of this ZIP safety fix.
Reservations carried from strategic-alignment
- Supply-chain-security: zip-slip fully closed through
safe_extract_zip; close-failure debug logging and returned-name semantics were verified/addressed by40c170496. - Test-coverage: regression traps are present; the new integration test proves the real Artifactory install/extract path rejects traversal archives.
Folded in this run
- (panel) Added integration coverage for malicious Artifactory ZIP rejection through the real downloader/extract path -- resolved in
40c170496. - (panel) Added mutation-break proof for the extraction guard -- resolved in
40c170496. - (panel) Added debug logging for response close failures instead of silently suppressing them -- resolved in
40c170496. - (panel) Documented
member_name_transformand returned extracted-name semantics -- resolved in40c170496. - (panel) Added the security changelog entry -- resolved in
40c170496. - (copilot) Clarified the safe extraction transform docstring signal -- resolved in
40c170496.
Copilot signals reviewed
src/apm_cli/utils/archive.py-- LEGIT: low-risk doc clarity request formember_name_transform; folded in40c170496.
Deferred (out-of-scope follow-ups)
These items were surfaced by the panel but cross the stated scope of this PR. Each one suggests a separate follow-up.
- (panel) Avoid buffering Artifactory archives before ZIP validation -- scope boundary: changes download/cache performance architecture beyond the ZIP traversal hardening in this PR.
Regression-trap evidence (mutation-break gate)
tests/integration/test_artifactory_install_e2e.py::test_malicious_zip_traversal_rejected_through_real_downloader-- deleted_zip_member_target(...)guard enforcement insafe_extract_zip; 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 passed. Full local lint mirror also passed: ruff check, ruff format --check, pylint R0801, and scripts/lint-auth-signals.sh.
CI
Fresh GitHub checks are green on 1e3da97592c52a594e7b62dfd1e3ab52d7a2df66: CI, Merge Gate, Spec conformance, NOTICE Drift Check, CodeQL, and CLA all pass. Local integration evidence: uv run --extra dev pytest tests/integration -q -> 10114 passed, 242 skipped, 16 deselected, 2 xfailed in 270.33s (after 1 CI fix iteration).
Mergeability status
Captured from gh pr view 1948 --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 |
|---|---|---|---|---|---|---|---|---|---|---|
| #1948 | 1e3da97 |
ship_now | 2 | 6 | 1 | 1 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
2 outer iteration(s); 1 Copilot round(s). Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No authentication or token-resolution surface changed.
Doc Writer
No findings.
Test Coverage Expert
No findings.
Performance Expert
- [recommended] Artifactory archive buffering remains broader performance work outside this PR's safety scope.
The PR is about closing unsafe ZIP extraction; changing transport/cache architecture should be handled separately.
This panel is advisory. Re-apply the panel-review label after addressing feedback to re-run.
danielmeppiel
removed
the
panel-review
Summary
Why
Single-file Artifactory ZIP downloads still used
ZipFile.extractall(), which bypassed the shared archive guards. A crafted archive could use path traversal to write outside the target directory.Validation
uv run --extra dev pytest tests/unit/utils/test_archive.py tests/unit/deps/test_download_strategies_phase3.py tests/unit/deps/test_download_strategies_selection.py tests/unit/test_artifactory_support.py -quv run --extra dev pytest tests/integration/test_artifactory_install_e2e.py -quv run --extra dev ruff check src/apm_cli/utils/archive.py src/apm_cli/deps/download_strategies.py tests/integration/test_artifactory_install_e2e.py tests/unit/deps/test_download_strategies_phase3.py tests/unit/deps/test_download_strategies_selection.py tests/unit/test_artifactory_support.pyapm-spec-waiver: Internal archive extraction security hardening does not extend the OpenAPM package specification.