Reject malformed wheel record paths - #14257
truemagic-coder wants to merge 4 commits into
Conversation
|
Hi @truemagic-coder, thanks for your contribution! Could you walk me through your changes? I'd like to understand the reasoning behind your approach. |
|
Hi @sepehr-rs, you are welcome! I saw this as two trust boundaries: install and uninstall. On install, I added separate uninstall checks because malformed I kept symlinks valid. Removing a symlink is very different from treating the directory it points to as package-owned. The tests follow the same split. Install tests prove bad metadata is rejected before writes. Uninstall tests prove bad metadata cannot remove unrelated files. Unit tests cover root paths, absolute paths, directories, and normal entries. The goal of the PR is to stop malformed |
sepehr-rs
left a comment
There was a problem hiding this comment.
Thanks for the effort you put into this!
This is heading in the right direction, but while testing this locally, I found that the uninstall guard only rejects RECORD entries that resolve exactly to the install root or to an actual directory. It doesn't reject entries that traverse outside location while still resolving to a regular file, e.g. ../../../document.txt,,.
Since this file is still inside the virtual environment, it gets deleted. Given the discussions in #10118, I think pip should guard against this case as well.
Here's the reproduction I used. Run this in a virtual environment, not against your system's Python:
pip install six
SITE=$(python -c "import site;print(site.getsitepackages()[0])")
RECORD=$(ls "$SITE"/six-*.dist-info/RECORD)
echo do-not-delete > "$VIRTUAL_ENV/document.txt"
echo '../../../document.txt,,' >> "$RECORD"
pip uninstall six -y
ls "$VIRTUAL_ENV/document.txt" # file is gone
That traversal case is real, but after looking through the existing work on #13873 / #13871 / #13874, I think it is a separate problem from #10118. Generic uninstall containment has already run into legitimate RECORD paths outside site-packages, including scripts and custom install schemes. I’d prefer to keep this PR scoped to preventing the installation root itself from becoming package-owned. I will address your other code comments on their own. Thanks! |
a436aba to
a650ad1
Compare
What does this PR do?
Fixes #10118.
A malformed wheel RECORD entry such as ./ can cause pip to treat the installation root as package-owned.
A later update, reinstall, or uninstall can then remove unrelated files from site-packages.
This PR hardens RECORD path handling.
On install:
empty paths are rejected;
absolute wheel paths are rejected;
paths that resolve to the installation root are rejected;
validation happens before installation file operations begin.
On uninstall:
entries that resolve to the installation root are rejected;
entries that name real directories are rejected;
valid relative file paths continue to work;
valid absolute installed RECORD file paths continue to work;
symlinks continue to identify the symlink itself rather than the directory they point to.
Absolute paths are valid in an installed RECORD. This includes Windows cases where pip cannot make a path relative because the file and lib_dir are on different logical disks.
The tests cover these cases directly.
Verification
The exact submitted commit was independently verified with FalseGreen against a frozen verification contract.
The verification covers:
relative and absolute paths resolving to the installation root;
relative and absolute real-directory entries;
valid relative file entries;
valid absolute installed RECORD file entries;
Windows different-logical-disk path conversion;
relative and absolute symlink-directory entries;
the original malformed ./ install and uninstall regression.
6/6 verification criteria passed.
Verified source:
a650ad1
The verification is scoped to these criteria. It does not claim broader correctness of pip.
PR Checklist:
Assisted-by: Codex