Tree: pass filter='fully_trusted' to tarfile.extractall() (PEP 706)#617
Conversation
| self._tree_rcopy_file(NODE_FOREIGN) | ||
|
|
||
| def test_tree_rcopy_no_tarfile_warning(self): | ||
| """test tree rcopy: no tarfile warning leaks (PEP 706)""" |
There was a problem hiding this comment.
I was thinking: ok we need this test only to check this is fine on old Python versions. So, at some point, we can get rid of it. How would we know?
We need to clarify this is a compat check, like (compat for <Py3.12) or something similar
There was a problem hiding this comment.
It's more for Python 3.12-3.13 to make sure they don't emit a warning. I will add a comment to clarify this
There was a problem hiding this comment.
For 3.14+, the current code is broken and other tests would fail.
|
|
||
|
|
||
| # PEP 706: pass filter='tar' when supported (Python 3.7.17+/3.12+) | ||
| _TAR_EXTRACT_KWARGS = {'filter': 'tar'} if hasattr(tarfile, 'data_filter') \ |
There was a problem hiding this comment.
how much did you check that tar is the default we need? (i did not verify myself)
because it looks like the default is None
There was a problem hiding this comment.
I checked and the default (None or filter='data') would change the behavior by stripping setuid flags, etc. Not really what we want for a tool used by sysadmins I guess. The current behavior is closer to filter='tar', which preserves attributes. As the archiving is kind of trusted (a remote clustershell instance is doing it), I think it's fine. We also add additional protection already with --transform in the command I believe. The legacy behavior is really filter='fully_trusted' but it is indeed risky and I tried to find a reason we would need it but I don't see any. So TL;DR; filter='tar' is a little bit safer than now and should not impact the behavior.
There was a problem hiding this comment.
I'm fine using tar to ignore the absolute path in tar, but the group bits will be a problem:
Clear high mode bits (setuid, setgid, sticky) and group/other write bits (S_IWGRP | S_IWOTH).
Write bits for group or others is a problem I think. I assume we do not have a test checking we preserve stuff like 0664 or 0666 in the test, but we probably should add one, and that would mean moving to fully_trusted?
25fc181 to
f4ac431
Compare
|
|
||
|
|
||
| # PEP 706: pass filter='tar' when supported (Python 3.7.17+/3.12+) | ||
| _TAR_EXTRACT_KWARGS = {'filter': 'tar'} if hasattr(tarfile, 'data_filter') \ |
There was a problem hiding this comment.
I'm fine using tar to ignore the absolute path in tar, but the group bits will be a problem:
Clear high mode bits (setuid, setgid, sticky) and group/other write bits (S_IWGRP | S_IWOTH).
Write bits for group or others is a problem I think. I assume we do not have a test checking we preserve stuff like 0664 or 0666 in the test, but we probably should add one, and that would mean moving to fully_trusted?
| self._tree_rcopy_file(NODE_FOREIGN) | ||
|
|
||
| def test_tree_rcopy_no_tarfile_warning(self): | ||
| """test tree rcopy: no tarfile warning leaks (PEP 706)""" |
There was a problem hiding this comment.
(nitpick) add (<3.14 compat) in the test name
The bare extractall() call warns on Python 3.12+ and will raise on 3.14. Use 'fully_trusted' to preserve current behavior: tarballs are built by trusted ClusterShell gateways; 'tar' and 'data' would strip S_IWGRP|S_IWOTH, downgrading modes like 0664 to 0644. Signed-off-by: Stephane Thiell <sthiell@stanford.edu>
f4ac431 to
c4bcb86
Compare
The bare extractall() call emits a DeprecationWarning on Python 3.12+ and will raise TarError on 3.14. Feature-detect via tarfile.data_filter so older Python versions keep the legacy behavior.