The --fix flag doesn't work for dependencies of top-level requirements

[tetsuo-cpp]

I'm still working on a repro for this but just from reading the code, I'm pretty sure that this isn't correct. When running --fix on the current environment it works like this:

• Figure out what version to upgrade the broken dependency to.
• Upgrade it and ignore any other package that uses that dependency. If we have packages that don't work properly with the upgraded dependency, too bad.

I think this is reasonable for an automated fix.
However, for requirements files our approach doesn't work well. We're currently doing:

• Figure out what version to upgrade the broken dependency to.
• Scan the requirements file and look for a matching dependency and update it.

However, transitive dependencies won't necessarily appear in the requirements file unless the requirements have pinned hashes. So in this case, the fix won't be applied to the requirements file. I think we have a few options here:

1. Rewrite the --fix logic to work differently for requirements files. We'll need something that identifies which top-level requirement a dependency belongs to and then upgrade the top-level requirement, check whether it fixed the vulnerability, rinse, repeat.
2. Make --fix write a new line to the requirements file explicitly listing the fixed dependency if it isn't already there.
3. Only allow --fix in combination with --require-hashes or --no-deps since we know we'll have the full set of dependencies in the requirements file.

At the moment, I'm trying to find a package that has a vulnerability in its dependency so I can actually test this out.

[tetsuo-cpp]

@woodruffw @di
This one is pretty important I think. Could I get your thoughts on this? I'll take a stab at this once we've decided on a plan.

I think 1) probably isn't practical to implement but 2) or 3) seem possible. I'm leaning towards 2) personally. Of course, I'm open to suggestions if there's an idea I've missed.

[woodruffw]

Yeah, this is an interesting problem.

IMO, (1) is both impractical and slightly wrong: a vulnerability in a subdependency might be completely transient and fixed on the next installation (by pip's resolver selecting the newer version); attempting to upgrade its "parent" might cause an entire cascade that completely breaks the application potentially without actually fixing the vulnerability.

I'm also not keen on (3) personally, since it drastically limits the usefulness of --fix -- users really should be pinning their dependencies, but it's just not common enough for the restriction to be worth it in pip-audit, IMO.

So yeah, I think (2) is reasonable. Some thoughts:

1. It might make sense to add a comment explicitly stating that the fixed subdependency has been added, since users will probably find additions to their dependencies confusing otherwise. Something similar what pip-compile does, where they add a # via foo-dep comment after each transitive dependency.
2. We should probably emit warnings when this happens.

Finally, another fourth option: we could do nothing. There's an argument to be made that, in the absence of a fixed dependency tree, pip-audit shouldn't assert that dependencies will continue to exist across multiple runs. (2) does that -- it assumes that a subdependency should be saved to the requirements file, which might break the file across multiple platforms (or simply cease to be required, if the subdependency was a sub-sub-dependency that was only needed by a specific point release of a subdependency).

Under this last option, IMO we should simply warn (perhaps with big red text) that a subdependency is vulnerable, and not attempt to fix it by patching it into the requirements input.

[tetsuo-cpp]

Finally, another fourth option: we could do nothing. There's an argument to be made that, in the absence of a fixed dependency tree, pip-audit shouldn't assert that dependencies will continue to exist across multiple runs. (2) does that -- it assumes that a subdependency should be saved to the requirements file, which might break the file across multiple platforms (or simply cease to be required, if the subdependency was a sub-sub-dependency that was only needed by a specific point release of a subdependency).

I'm still trying to digest this part. Is the scenario you're talking about where we have a requirement with a conditional dependency (or even just a requirement with a marker)? If we patch a sub dependency, it gets explicitly listed in the requirements file and therefore, starts getting installed in situations where it normally wouldn't?

[woodruffw]

Sort of — I’m imagining a situation where the user specifies version N of a package, which has a sub dependency with version Q. Q has a vulnerability, so we upgrade it to Q’. The user then later manually (or automatically) upgrades N to N’, which happens to remove Q from its dependency set. With that, the user would be left with an “orphaned” dependency on Q’ in their requirements, since it’s no longer a subdependency. Sent from mobile. Please excuse my brevity.

…

On Jun 12, 2022, at 8:49 AM, Alex Cameron ***@***.***> wrote:  Finally, another fourth option: we could do nothing. There's an argument to be made that, in the absence of a fixed dependency tree, pip-audit shouldn't assert that dependencies will continue to exist across multiple runs. (2) does that -- it assumes that a subdependency should be saved to the requirements file, which might break the file across multiple platforms (or simply cease to be required, if the subdependency was a sub-sub-dependency that was only needed by a specific point release of a subdependency). I'm still trying to digest this part. Is the scenario you're talking about where we have a requirement with a conditional dependency (or even just a requirement with a marker)? If we patch a sub dependency, it gets explicitly listed in the requirements file and therefore, gets installed in situations where it normally wouldn't? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[woodruffw]

Forgot to mention: if we end up in a state like #291 (comment) where Q' is now an orphaned dependency, it might make sense to offer the user some advice and suggest that they remove it (or even do so automatically).

We could possibly do that by tracking our own comments (added during --fix) and making sure that any associated deps appear nowhere else in the tree, implying that they've been orphaned.