Check symlink target in tar extraction fallback for Pythons without data_filter
#13538
Conversation
|
@sethmlarson thanks for the PR, do you know why Python 3.9 is failing (at least for MacOS)? |
|
Here's the relevant issue & PR that added the logic you are removing #288 / #293. Old pip workarounds are usually in there for a reason, and I don't see much documentation from Python on how That said, given it's calling a non-public function it's probably for the best. |
sethmlarson
changed the title
tarfile.extractfile() handle symlinks, even without data_filtertarfile.extractfile() handle symlinks, even without data_filter
|
@notatallshaw I'll figure out CI, I should have opened this one as a draft but unfortunately there's no going back once it's opened. Will ping again once we get it figured out. |
|
@sethmlarson don't worry about draft / not draft status, we typically will only merge with an independent maintainer approval and all tests passing. |
sethmlarson
force-pushed
the
symlink-handling
branch
from
264185d to
3be3682
Compare
sethmlarson
force-pushed
the
symlink-handling
branch
from
3be3682 to
2f2b84e
Compare
sethmlarson
changed the title
tarfile.extractfile() handle symlinks, even without data_filterdata_filter
|
@notatallshaw Okay, now this PR should be ready for review! |
|
@sethmlarson I'm sure if I dug into the unit tests I'd figure it out, but I would appreciate an explanation of why this is necessary or what benefits doing this check brings. I'm not a huge fan of using private functions (especially with how well that worked out with Black). |
|
As @ichard26 says, I'm a bit confused, this PR started off by removing code, and the news entry still seems to reflect that, but now it just adds an extra private function call. |
| reason="tarfile filters (PEP-721) must be absent", | ||
| ) | ||
| def test_unpack_tar_data_filter_bad_links_parent_dir(self) -> None: | ||
| evil_link = "../../../evil" |
There was a problem hiding this comment.
When these test fails it leaves a file at /tmp/evil which is not cleaned up. Which causes subsequent runs to fail even if they are good.
Can you do something to handle this? Maybe extract to a sub directory of this temporary directory?
| reason="tarfile filters (PEP-721) not available", | ||
| ) | ||
| def test_unpack_tar_no_data_filter_bad_links_parent_dir(self) -> None: | ||
| evil_link = "../../../evil" |
Closes #13537