Fix sdist extraction on older Python versions - #11037
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If the owner-name workaround is wrong, extraction could fail or create files with an unintended owner or group, potentially affecting access to the extracted sdist. Reverting fixes future extractions, but already-created files and their ownership would remain until the archive is re-extracted or the metadata is corrected.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
907013b to
f89bbd6
Compare
radoering
left a comment
radoering
left a comment
There was a problem hiding this comment.
There are more spots with the pattern "try - except KeyError" and running the repro with Poetry from your branch still fails:
TypeError
uid should be integer, not NoneType
(Just replace poetry==2.4.2 with git+https://github.com/fredrikblau/poetry.git@fix/older-python-sdist-extraction in the repro.)
|
Handled the missing-ownership case from the repro. The fallback now always sets uid/gid to -1, and the regression test simulates omitted fields.
|
0114526 to
90b280f
Compare
|
Fixed the mypy failure in the regression setup by using object.setattr to simulate missing ownership fields. Focused tests: 50 passed. The hosted Windows pytest failure is unrelated PermissionError in test_env.py. |
Python versions without a working tarfile data filter fall back to the filter copied from CPython, which clears ownership by setting uid, gid, uname and gname to None. TarFile.chown() in those same versions cannot cope with that: it passes the ids to os.chown() and resolves the names with pwd/grp, catching only KeyError. Extracting an sdist therefore failed with "uid should be integer, not NoneType", or a TypeError from the name lookup when the archive itself carried no owner names. Clear ownership with values those versions accept instead: -1 for the ids, which os.chown() reads as "leave unchanged", and "" for the names, which fails the lookup with KeyError and falls back to the ids. Fixes: python-poetry#11036
a31537b to
f69a507
Compare
Python versions without a working tarfile data filter fall back to the filter copied from CPython, which clears ownership by setting uid, gid, uname and gname to None. TarFile.chown() in those same versions cannot cope with that: it passes the ids to os.chown() and resolves the names with pwd/grp, catching only KeyError. Extracting an sdist therefore failed with "uid should be integer, not NoneType", or a TypeError from the name lookup when the archive itself carried no owner names. Clear ownership with values those versions accept instead: -1 for the ids, which os.chown() reads as "leave unchanged", and "" for the names, which fails the lookup with KeyError and falls back to the ids. (cherry picked from commit 98df56f)
3 participants
Summary
The tar security fallback clears ownership names with
None. Python versions before 3.10.12 and 3.11.4 pass those values togrp.getgrnam()/pwd.getpwnam(), causing root sdist installs to fail.Use empty ownership names instead, which preserves the ownership-stripping behavior without triggering the older tarfile bug. Added a regression test that simulates the affected tarfile implementation.
Fixes #11036.
Tests
pytest tests/utils/test_helpers.py -q(50 passed)pytest(2,935 passed, 28 skipped; 6 collection errors due missingdeepdiffbefore installation, and one unrelated Python 3.14 platform-tag expectation)ruff check tests/utils/test_helpers.py(passed; existingPLC0206remains insrc/poetry/utils/helpers.py)