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

@@ -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

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)
 ```
 <!-- @end-pip-audit-help@ -->
 

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

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

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:

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])

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",

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()
 

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")