pip, _fix: Implement `--fix` for `PipSource` #212

tetsuo-cpp · 2022-01-12T06:35:39Z

Closes #82

tetsuo-cpp · 2022-01-12T06:44:58Z

This is a proof-of-concept. There's a bit of cleanup and UX to figure out like how to notify users what upgrades took place. But this is the general idea I'm going for. It upgrades Flask 0.5 to Flask 1.0 successfully.

The main deviation from what we discussed earlier is that instead of doing pip install --upgrade, I have some code to "resolve" the fix version. I do that by looking at the fix versions of all the vulnerabilities, choosing the latest one (that includes all the fixes), then checking it against the vulnerability service. If there are vulnerabilities in that version, I repeat until I either find a clean version with no vulnerabilities or I find a vulnerability with no fix version (in which case I'll fail the --fix).

My reasoning behind this is that the latest version of a package can have a reported vulnerability so blindly doing pip install --upgrade doesn't seem to be the right thing to do given that we have fix version information at our disposal. Also generally speaking, the more versions we jump, the more likely the user is going to experience API breakages. So I think we should upgrade the minimum distance possible without introducing new known vulnerabilities.

@woodruffw and @di, what do you think about this approach?

pip_audit/_dependency_source/pip.py

pip_audit/_fix.py

woodruffw · 2022-01-12T13:50:07Z

My reasoning behind this is that the latest version of a package can have a reported vulnerability so blindly doing pip install --upgrade doesn't seem to be the right thing to do given that we have fix version information at our disposal. Also generally speaking, the more versions we jump, the more likely the user is going to experience API breakages. So I think we should upgrade the minimum distance possible without introducing new known vulnerabilities.

This reasoning makes sense to me, and the underlying algorithm makes sense to me as well!

pip_audit/_dependency_source/interface.py

woodruffw · 2022-01-12T14:05:31Z

Overall structure looks good to me! In terms of notifying users, I was thinking that we'd amend our normal output.

Without --fix:

$ pip-audit
Found 12 known vulnerabilities in 18 packages
<output depending on --format>

With --fix:

$ pip-audit --fix
Found 12 known vulnerabilities in 18 packages and fixed 9 vulnerabilities in 4 packages
<output depending on --format>

In particular, both our columnar and JSON formats should support emitting a summary of the fixes performed (and those skipped, including the reason the fix failed).

How does that sound?

di · 2022-01-12T18:12:23Z

I agree we should apply the "minimum" fix here.

Let's think about exit codes as well: should pip-audit --fix fail if it can't fix all vulnerabilities? IMO yes.

tetsuo-cpp · 2022-01-13T03:16:40Z

@woodruffw

In particular, both our columnar and JSON formats should support emitting a summary of the fixes performed (and those skipped, including the reason the fix failed).

Yep, that's a good plan. In the interest of not letting this PR accumulate too much code, I'll make separate issues for these and follow up.

@di

Let's think about exit codes as well: should pip-audit --fix fail if it can't fix all vulnerabilities? IMO yes.

Yeah I agree with this. I think the exit codes should work differently with --fix. So if you run with --fix, successfully fixing all vulnerabilities should exit with 0 (whereas without --fix would exit with 1 since we detected vulnerabilities to begin with). And as you suggested, if we can't fix them all, we should exit with 1.

pip_audit/_cli.py

pip_audit/_fix.py

tetsuo-cpp · 2022-01-13T12:17:34Z

The CI breakage is due to #213

woodruffw · 2022-01-13T15:48:36Z

pip_audit/_dependency_source/pip.py

+                fix_cmd, check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
+            )
+        except subprocess.CalledProcessError as cpe:
+            raise PipSourceError(


Nitpick: we might want to consider creating a specialized DependencyFixError that each dependency source in turn specializes.

So the hierarchy would be: DependencySourceError -> DependencyFixError -> PipFixError | RequirementFixError.

woodruffw · 2022-01-13T15:52:56Z

LGTM overall! I'm going to make the exception refactor I mentioned in #212 (review) and then pull the trigger on this.

### Added * CLI: The `--fix` flag has been added, allowing users to attempt to automatically upgrade any vulnerable dependencies to the first safe version available ([#212](pypa/pip-audit#212), [#222](pypa/pip-audit#222)) * CLI: The combination of `--fix` and `--dry-run` is now supported, causing `pip-audit` to perform the auditing step but not any resulting fix steps ([#223](pypa/pip-audit#223)) * CLI: The `--require-hashes` flag has been added which can be used in conjunction with `-r` to check that all requirements in the file have an associated hash ([#229](pypa/pip-audit#229)) * CLI: The `--index-url` flag has been added, allowing users to use custom package indices when running with the `-r` flag ([#238](pypa/pip-audit#238)) * CLI: The `--extra-index-url` flag has been added, allowing users to use multiple package indices when running with the `-r` flag ([#238](pypa/pip-audit#238)) ### Changed * `pip-audit`'s minimum Python version is now 3.7. * CLI: The default output format is now correctly pluralized ([#221](pypa/pip-audit#221)) * Output formats: The SBOM output formats (`--format=cyclonedx-xml` and `--format=cyclonedx-json`) now use CycloneDX [Schema 1.4](https://cyclonedx.org/docs/1.4/xml/) ([#216](pypa/pip-audit#216)) * Vulnerability sources: When using PyPI as a vulnerability service, any hashes provided in a requirements file are checked against those reported by PyPI ([#229](pypa/pip-audit#229)) * Vulnerability sources: `pip-audit` now uniques each result based on its alias set, reducing the amount of duplicate information in the default columnar output format ([#232](pypa/pip-audit#232)) * CLI: `pip-audit` now prints its output more frequently, including when there are no discovered vulnerabilities but packages were skipped. Similarly, "manifest" output formats (JSON, CycloneDX) are now emitted unconditionally ([#240](pypa/pip-audit#240)) ### Fixed * CLI: A regression causing excess output during `pip audit -r` was fixed ([#226](pypa/pip-audit#226))

tetsuo-cpp added 3 commits January 12, 2022 16:06

_cli, _fix: Implement resolving of fix versions

1c9ae04

pip: Implement fix

82d6441

_cli: Minor fixes

4000b2b

tetsuo-cpp marked this pull request as draft January 12, 2022 06:35

tetsuo-cpp commented Jan 12, 2022

View reviewed changes

pip_audit/_dependency_source/pip.py Outdated Show resolved Hide resolved

tetsuo-cpp commented Jan 12, 2022

View reviewed changes

pip_audit/_fix.py Outdated Show resolved Hide resolved

_dependency_source: Make fix an abstract method

a9bcb0c

woodruffw self-requested a review January 12, 2022 13:50

woodruffw reviewed Jan 12, 2022

View reviewed changes

pip_audit/_dependency_source/interface.py Outdated Show resolved Hide resolved

tetsuo-cpp added 3 commits January 13, 2022 12:19

pip, _fix: Use more appropriate exception types

402783d

_fix: Make error messages consistent

a4898d5

pip_audit: Propagate fix information up to CLI layer

434f890

tetsuo-cpp commented Jan 13, 2022

View reviewed changes

pip_audit/_cli.py Show resolved Hide resolved

tetsuo-cpp commented Jan 13, 2022

View reviewed changes

pip_audit/_fix.py Show resolved Hide resolved

tetsuo-cpp added 7 commits January 13, 2022 17:42

_fix: Add documentation

1e47706

conftest: Get tests passing

6e1a186

test: Fill in coverage for PipSource

889bb0d

test: Unit test fix version resolution

3595eb7

README: Update help text

31d72ec

_cli: Fix summary msg

682a58c

README: Add --fix example to doc

a9b5bf5

tetsuo-cpp marked this pull request as ready for review January 13, 2022 12:17

tetsuo-cpp requested review from woodruffw and di January 13, 2022 12:18

tetsuo-cpp mentioned this pull request Jan 13, 2022

Implement --fix for RequirementSource #214

Closed

1 task

Merge branch 'main' into alex/dep-fix

992595d

woodruffw reviewed Jan 13, 2022

View reviewed changes

woodruffw mentioned this pull request Jan 13, 2022

Implement --fix for PipSource #82

Closed

2 tasks

pip_audit, test: add a separate DependencyFixError hierarchy

0f6b4ad

woodruffw approved these changes Jan 13, 2022

View reviewed changes

CHANGELOG: record changes

64e3e5f

woodruffw merged commit b1363f1 into main Jan 13, 2022

woodruffw deleted the alex/dep-fix branch January 13, 2022 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pip, _fix: Implement `--fix` for `PipSource` #212

pip, _fix: Implement `--fix` for `PipSource` #212

tetsuo-cpp commented Jan 12, 2022 •

edited

Loading

tetsuo-cpp commented Jan 12, 2022 •

edited

Loading

woodruffw commented Jan 12, 2022

woodruffw commented Jan 12, 2022 •

edited by tetsuo-cpp

Loading

di commented Jan 12, 2022

tetsuo-cpp commented Jan 13, 2022 •

edited

Loading

tetsuo-cpp commented Jan 13, 2022

woodruffw Jan 13, 2022

woodruffw commented Jan 13, 2022

pip, _fix: Implement --fix for PipSource #212

pip, _fix: Implement --fix for PipSource #212

Conversation

tetsuo-cpp commented Jan 12, 2022 • edited Loading

tetsuo-cpp commented Jan 12, 2022 • edited Loading

woodruffw commented Jan 12, 2022

woodruffw commented Jan 12, 2022 • edited by tetsuo-cpp Loading

di commented Jan 12, 2022

tetsuo-cpp commented Jan 13, 2022 • edited Loading

tetsuo-cpp commented Jan 13, 2022

woodruffw Jan 13, 2022

Choose a reason for hiding this comment

woodruffw commented Jan 13, 2022

pip, _fix: Implement `--fix` for `PipSource` #212

pip, _fix: Implement `--fix` for `PipSource` #212

tetsuo-cpp commented Jan 12, 2022 •

edited

Loading

tetsuo-cpp commented Jan 12, 2022 •

edited

Loading

woodruffw commented Jan 12, 2022 •

edited by tetsuo-cpp

Loading

tetsuo-cpp commented Jan 13, 2022 •

edited

Loading