-
-
Notifications
You must be signed in to change notification settings - Fork 611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove extras from compiled file #1608
Remove extras from compiled file #1608
Conversation
IMO the extras should be preserved, as they are the only way within a single file to guarantee any order of installation, which may have significant effects on a given package's setup procedure. If someone replaces a versioned PyPI package with a modification (and the user isn't hash checking it), then there's no need for the attacker to put the malicious code in a dependency, which in a sense makes it more visible than including it directly in the replaced package. |
@AndydeCleyre About your second point, It makes sense that in reality, the attacker would include the change in the code and not as an extra. It doesn't change the fact that it may install unexpected extras (especially if we allow the extras to be included in direct reference of a VCS). Although in theory, a package should be pinned and the content should not change. About your first point, I don't know well enough how that works to comment. Are you saying that |
For If those groups specify
You can find some context for a good example in the case of |
@AndydeCleyre Thank you for your explanation! |
Currently, we have direct references without extras and pinned packages with extras in requirements.txt, which looks wrong and should be synced in some single way. Intuitively I still think that we should omit the extras, but I'd like to see a proper discussion in a separate issue with "pros and cons" and examples from other tools for each method. Who would like to open an issue? |
@atugushev I could work on that Saturday unless @AndydeCleyre you'd like to do it? |
Please go ahead, thanks! |
I will try to run all the tools with gdal and numpy (with and without extras) which are mentioned in #992 and see what happens. |
I will be closing this because we did not have a bug and an agreed behavior yet. Also the already existing |
This PR removes the extras from the compiled files.
Please note that this is a proposal that needs discussion.
Here's a comparative table of other tools:
In my opinion the compiled files should not include extras.
Let's take an example:
Given a
requirements.in
containing typer with all extras:When doing
pip-sync
, the compiled file is:Then
pip freeze
shows:Now imagine that
typer[all]
did not containcolorama
(which is an extra dependency) at the time we ranpip-compile
.The compiled file would look like that:
Let's save the change in the compiled file and remove all installed packages in the venv.
Then let's do
pip-sync
andpip freeze
again. Here's the output:As you can see, while
colorama
was not explicitly defined in the compiled files, it has been installed because of the extra specified in the compiled file.I do not know if it's possible to replace a package in PyPI with the same version.
If it is, then an attacker could inject an extra dependency and users would install it without realizing it. Personally, when my list of installed packages is huge, I don't check one by one all of them, I trust
pip
orpip-sync
to install what's defined in the compiled file and nothing else. That's why I think this change makes sense.Links mentioning this change
Contributor checklist
Maintainer checklist
backwards incompatible
,feature
,enhancement
,deprecation
,bug
,dependency
,docs
orskip-changelog
as they determine changelog listing.