From 0e2dc53e6eefcdc63e3ac47015fab6acfdb949d7 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 28 Feb 2022 15:37:08 -0500 Subject: [PATCH] pip_audit, test: allow editable packages to be skipped (#244) * pip_audit, test: allow editable packages to be skipped * README: update `--help` * pip_audit, test: skip editable with requirements sources * test: silence mypy * CHANGELOG: record changes --- CHANGELOG.md | 6 ++++ README.md | 3 ++ pip_audit/_cli.py | 19 +++++++--- pip_audit/_dependency_source/pip.py | 34 ++++++++++++------ pip_audit/_dependency_source/requirement.py | 29 +++++++++------ .../resolvelib/resolvelib.py | 19 +++++++++- setup.py | 2 +- test/dependency_source/test_pip.py | 35 +++++++++++++++++++ test/dependency_source/test_resolvelib.py | 10 ++++++ 9 files changed, 130 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45c18145..f9a0d87a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ All versions prior to 0.0.9 are untracked. ## [Unreleased] - ReleaseDate +### Added + +* CLI: The `--skip-editable` flag has been added, allowing users to skip local + packages or parsed requirements (via `-r`) that are marked as editable + ([#244](https://github.com/trailofbits/pip-audit/pull/244)) + ## [2.0.0] - 2022-02-18 ### Added diff --git a/README.md b/README.md index b4d3f1f0..214734f9 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ usage: pip-audit [-h] [-V] [-l] [-r REQUIREMENTS] [-f FORMAT] [-s SERVICE] [--progress-spinner {on,off}] [--timeout TIMEOUT] [--path PATHS] [-v] [--fix] [--require-hashes] [--index-url INDEX_URL] [--extra-index-url EXTRA_INDEX_URLS] + [--skip-editable] audit the Python environment for dependencies with known vulnerabilities @@ -129,6 +130,8 @@ optional arguments: extra URLs of package indexes to use in addition to `--index-url`; should follow the same rules as `--index-url` (default: []) + --skip-editable don't audit packages that are marked as editable + (default: False) ``` diff --git a/pip_audit/_cli.py b/pip_audit/_cli.py index e53c3d37..0ec9c9ae 100644 --- a/pip_audit/_cli.py +++ b/pip_audit/_cli.py @@ -262,6 +262,11 @@ def _parser() -> argparse.ArgumentParser: help="extra URLs of package indexes to use in addition to `--index-url`; should follow the " "same rules as `--index-url`", ) + parser.add_argument( + "--skip-editable", + action="store_true", + help="don't audit packages that are marked as editable", + ) return parser @@ -304,14 +309,20 @@ def audit() -> None: if args.requirements is not None: index_urls = [args.index_url] + args.extra_index_urls req_files: List[Path] = [Path(req.name) for req in args.requirements] + # TODO: This is a leaky abstraction; we should construct the ResolveLibResolver + # within the RequirementSource instead of in-line here. source = RequirementSource( req_files, - ResolveLibResolver(index_urls, args.timeout, args.cache_dir, state), - args.require_hashes, - state, + ResolveLibResolver( + index_urls, args.timeout, args.cache_dir, args.skip_editable, state + ), + require_hashes=args.require_hashes, + state=state, ) else: - source = PipSource(local=args.local, paths=args.paths, state=state) + source = PipSource( + local=args.local, paths=args.paths, skip_editable=args.skip_editable, state=state + ) # `--dry-run` only affects the auditor if `--fix` is also not supplied, # since the combination of `--dry-run` and `--fix` implies that the user diff --git a/pip_audit/_dependency_source/pip.py b/pip_audit/_dependency_source/pip.py index 1486837e..20bd578c 100644 --- a/pip_audit/_dependency_source/pip.py +++ b/pip_audit/_dependency_source/pip.py @@ -37,7 +37,12 @@ class PipSource(DependencySource): """ def __init__( - self, *, local: bool = False, paths: Sequence[Path] = [], state: AuditState = AuditState() + self, + *, + local: bool = False, + paths: Sequence[Path] = [], + skip_editable: bool = False, + state: AuditState = AuditState(), ) -> None: """ Create a new `PipSource`. @@ -49,10 +54,14 @@ def __init__( list is empty, the `DependencySource` will query the current Python environment. + `skip_editable` controls whether dependencies marked as "editable" are skipped. + By default, editable dependencies are not skipped. + `state` is an `AuditState` to use for state callbacks. """ self._local = local self._paths = paths + self._skip_editable = skip_editable self.state = state if _PIP_VERSION < _MINIMUM_RELIABLE_PIP_VERSION: @@ -76,16 +85,21 @@ def collect(self) -> Iterator[Dependency]: local=self._local, paths=list(self._paths) ).items(): dep: Dependency - try: - dep = ResolvedDependency(name=dist.name, version=Version(str(dist.version))) - self.state.update_state(f"Collecting {dep.name} ({dep.version})") - except InvalidVersion: - skip_reason = ( - "Package has invalid version and could not be audited: " - f"{dist.name} ({dist.version})" + if dist.editable and self._skip_editable: + dep = SkippedDependency( + name=dist.name, skip_reason="distribution marked as editable" ) - logger.debug(skip_reason) - dep = SkippedDependency(name=dist.name, skip_reason=skip_reason) + else: + try: + dep = ResolvedDependency(name=dist.name, version=Version(str(dist.version))) + self.state.update_state(f"Collecting {dep.name} ({dep.version})") + except InvalidVersion: + skip_reason = ( + "Package has invalid version and could not be audited: " + f"{dist.name} ({dist.version})" + ) + logger.debug(skip_reason) + dep = SkippedDependency(name=dist.name, skip_reason=skip_reason) yield dep except Exception as e: raise PipSourceError("failed to list installed distributions") from e diff --git a/pip_audit/_dependency_source/requirement.py b/pip_audit/_dependency_source/requirement.py index 4387ab2a..3df6e57f 100644 --- a/pip_audit/_dependency_source/requirement.py +++ b/pip_audit/_dependency_source/requirement.py @@ -14,8 +14,8 @@ from packaging.requirements import Requirement from packaging.specifiers import SpecifierSet from packaging.version import Version +from pip_api import Requirement as ParsedRequirement from pip_api import parse_requirements -from pip_api._parse_requirements import Requirement as ParsedRequirement from pip_api._parse_requirements import UnparsedRequirement from pip_api.exceptions import PipError @@ -45,6 +45,7 @@ def __init__( self, filenames: List[Path], resolver: DependencyResolver, + *, require_hashes: bool = False, state: AuditState = AuditState(), ) -> None: @@ -55,11 +56,14 @@ def __init__( `resolver` is the `DependencyResolver` instance to use. + `require_hashes` controls the hash policy: if `True`, dependency collection + will fail unless all requirements include hashes. + `state` is an `AuditState` to use for state callbacks. """ - self.filenames = filenames - self.resolver = resolver - self.require_hashes = require_hashes + self._filenames = filenames + self._resolver = resolver + self._require_hashes = require_hashes self.state = state def collect(self) -> Iterator[Dependency]: @@ -69,7 +73,7 @@ def collect(self) -> Iterator[Dependency]: Raises a `RequirementSourceError` on any errors. """ collected: Set[Dependency] = set() - for filename in self.filenames: + for filename in self._filenames: try: reqs = parse_requirements(filename=filename) except PipError as pe: @@ -82,7 +86,7 @@ def collect(self) -> Iterator[Dependency]: # # If at least one requirement has a hash, it implies that we require hashes for all # requirements - if self.require_hashes or any( + if self._require_hashes or any( isinstance(req, ParsedRequirement) and req.hashes for req in reqs.values() ): yield from self._collect_hashed_deps(iter(reqs.values())) @@ -91,7 +95,7 @@ def collect(self) -> Iterator[Dependency]: # Invoke the dependency resolver to turn requirements into dependencies req_values: List[Requirement] = [Requirement(str(req)) for req in reqs.values()] try: - for _, deps in self.resolver.resolve_all(iter(req_values)): + for _, deps in self._resolver.resolve_all(iter(req_values)): for dep in deps: # Don't allow duplicate dependencies to be returned if dep in collected: @@ -117,15 +121,15 @@ def fix(self, fix_version: ResolvedFixVersion) -> None: # Make temporary copies of the existing requirements files. If anything goes wrong, we # want to copy them back into place and undo any partial application of the fix. tmp_files: List[IO[str]] = [ - stack.enter_context(NamedTemporaryFile(mode="w")) for _ in self.filenames + stack.enter_context(NamedTemporaryFile(mode="w")) for _ in self._filenames ] - for (filename, tmp_file) in zip(self.filenames, tmp_files): + for (filename, tmp_file) in zip(self._filenames, tmp_files): with filename.open("r") as f: shutil.copyfileobj(f, tmp_file) try: # Now fix the files inplace - for filename in self.filenames: + for filename in self._filenames: self.state.update_state( f"Fixing dependency {fix_version.dep.name} ({fix_version.dep.version} => " f"{fix_version.version})" @@ -162,7 +166,7 @@ def _fix_file(self, filename: Path, fix_version: ResolvedFixVersion) -> None: f.write(str(req) + os.linesep) def _recover_files(self, tmp_files: List[IO[str]]) -> None: - for (filename, tmp_file) in zip(self.filenames, tmp_files): + for (filename, tmp_file) in zip(self._filenames, tmp_files): try: os.replace(tmp_file.name, filename) # We need to tinker with the internals to prevent the file wrapper from attempting @@ -177,6 +181,9 @@ def _recover_files(self, tmp_files: List[IO[str]]) -> None: def _collect_hashed_deps( self, reqs: Iterator[Union[ParsedRequirement, UnparsedRequirement]] ) -> Iterator[Dependency]: + # NOTE: Editable and hashed requirements are incompatible by definition, so + # we don't bother checking whether the user has asked us to skip editable requirements + # when we're doing hashed requirement collection. for req in reqs: req = cast(ParsedRequirement, req) if not req.hashes: diff --git a/pip_audit/_dependency_source/resolvelib/resolvelib.py b/pip_audit/_dependency_source/resolvelib/resolvelib.py index ee41ff10..b719c227 100644 --- a/pip_audit/_dependency_source/resolvelib/resolvelib.py +++ b/pip_audit/_dependency_source/resolvelib/resolvelib.py @@ -5,9 +5,10 @@ import logging from pathlib import Path -from typing import List, Optional +from typing import List, Optional, cast from packaging.requirements import Requirement +from pip_api import Requirement as ParsedRequirement from requests.exceptions import HTTPError from resolvelib import BaseReporter, Resolver @@ -33,6 +34,7 @@ def __init__( index_urls: List[str] = [PYPI_URL], timeout: Optional[int] = None, cache_dir: Optional[Path] = None, + skip_editable: bool = False, state: AuditState = AuditState(), ) -> None: """ @@ -41,16 +43,31 @@ def __init__( `timeout` and `cache_dir` are optional arguments for HTTP timeouts and caching, respectively. + `skip_editable` controls whether requirements marked as "editable" are skipped. + By default, editable requirements are not skipped. + `state` is an `AuditState` to use for state callbacks. """ self.provider = PyPIProvider(index_urls, timeout, cache_dir, state) self.reporter = BaseReporter() self.resolver: Resolver = Resolver(self.provider, self.reporter) + self._skip_editable = skip_editable def resolve(self, req: Requirement) -> List[Dependency]: """ Resolve the given `Requirement` into a `Dependency` list. """ + + # HACK: `resolve` takes both `packaging.Requirement` and `pip_api.Requirement`, + # since the latter is a subclass. But only the latter knows whether the + # requirement is editable, so we need to check for it here. + if isinstance(req, ParsedRequirement): + req = cast(ParsedRequirement, req) + if req.editable and self._skip_editable: + return [ + SkippedDependency(name=req.name, skip_reason="requirement marked as editable") + ] + deps: List[Dependency] = [] try: result = self.resolver.resolve([req]) diff --git a/setup.py b/setup.py index 7b698c87..7f6626cb 100644 --- a/setup.py +++ b/setup.py @@ -28,7 +28,7 @@ platforms="any", python_requires=">=3.7", install_requires=[ - "pip-api>=0.0.27", + "pip-api>=0.0.28", "packaging>=21.0.0", "progress>=1.6", "resolvelib>=0.8.0", diff --git a/test/dependency_source/test_pip.py b/test/dependency_source/test_pip.py index c8506ab8..55543b08 100644 --- a/test/dependency_source/test_pip.py +++ b/test/dependency_source/test_pip.py @@ -58,6 +58,7 @@ def test_pip_source_invalid_version(monkeypatch): class MockDistribution: name: str version: str + editable: bool = False # Return a distribution with a version that doesn't conform to PEP 440. # We should log a debug message and skip it. @@ -87,6 +88,40 @@ def mock_installed_distributions( assert ResolvedDependency(name="pip-api", version=Version("1.0")) in specs +def test_pip_source_skips_editable(monkeypatch): + source = pip.PipSource(skip_editable=True) + + @dataclass(frozen=True) + class MockDistribution: + name: str + version: str + editable: bool = False + + # Return a distribution with a version that doesn't conform to PEP 440. + # We should log a debug message and skip it. + def mock_installed_distributions( + local: bool, paths: List[os.PathLike] + ) -> Dict[str, MockDistribution]: + return { + "pytest": MockDistribution("pytest", "0.1"), + "pip-audit": MockDistribution("pip-audit", "2.0.0", True), + "pip-api": MockDistribution("pip-api", "1.0"), + } + + monkeypatch.setattr(pip_api, "installed_distributions", mock_installed_distributions) + + specs = list(source.collect()) + assert ResolvedDependency(name="pytest", version=Version("0.1")) in specs + assert ( + SkippedDependency( + name="pip-audit", + skip_reason="distribution marked as editable", + ) + in specs + ) + assert ResolvedDependency(name="pip-api", version=Version("1.0")) in specs + + def test_pip_source_fix(monkeypatch): source = pip.PipSource() diff --git a/test/dependency_source/test_resolvelib.py b/test/dependency_source/test_resolvelib.py index 0b4b5d98..5ac27b76 100644 --- a/test/dependency_source/test_resolvelib.py +++ b/test/dependency_source/test_resolvelib.py @@ -5,6 +5,7 @@ import requests from packaging.requirements import Requirement from packaging.version import Version +from pip_api import Requirement as ParsedRequirement from requests.exceptions import HTTPError from resolvelib.resolvers import InconsistentCandidate, ResolutionImpossible @@ -400,3 +401,12 @@ def get_multiple_index_package_mock(url): resolved_deps = dict(resolver.resolve_all(iter([req]))) assert req in resolved_deps assert resolved_deps[req] == [ResolvedDependency("flask", Version("0.5"))] + + +def test_resolvelib_skip_editable(): + resolver = resolvelib.ResolveLibResolver(skip_editable=True) + req = ParsedRequirement("foo==1.0.0", editable=True, filename="stub", lineno=1) + + deps = resolver.resolve(req) # type: ignore + assert len(deps) == 1 + assert deps[0] == SkippedDependency(name="foo", skip_reason="requirement marked as editable")