Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.2k
gh-145417: Do not preserve SELinux context when copying venv scripts#145454
gh-145417: Do not preserve SELinux context when copying venv scripts#145454Shrey-N wants to merge 17 commits intopython:mainfrom
Conversation
python-cla-botbot commentedMar 3, 2026 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
hroncok left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This way, I believe the mode is not copied, and the suggested change needs to be done as well.
hroncok commentedMar 3, 2026
OK, However, I was tryign to figure out how to properly test this. |
Shrey-N commentedMar 3, 2026
Apologies@hroncok I completely missed that you were planning to handle this submission yourself, I am a new contributor, and I was eager to help out and learn about the workflow... Regarding the tests I think your idea of asserting the mtime is a nice approach, since shutil.copy should update the timestamp while copy2 would preserve it. I am happy to either close this PR, or leave it here if it is a useful starting point. Thank you for your patience I am looking forward to this learning process! |
Add a test to verify mtime behavior during script installation.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Shrey-N commentedMar 3, 2026
I went ahead and added a test case into this PR using the mtime approach, It should cover the verification for metadata prevention now. |
Address linting failures in the new test_install_scripts_mtime by removing redundant imports and fixing trailing whitespace in the docstrings.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
hroncok left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it might be better to actually useActivate.ps1 in the test. Something like this:
- check the mtime of the
Activate.ps1file in the stdlib - if the current time is the same (unlikely, but possible), sleep 1 second
- create a virtual environment
- assert the
Activate.ps1file in the venv mtime differs from 1. - assert the mode of both
Activate.ps1s match (asserting this has not regressed) - assert the
Activate.ps1contents match, so we know we are testing with a proper file (this assert will fail if the file later becomes a template, which serves as a protection against a no longer functional test)
Lib/test/test_venv.py Outdated
| """ | ||
| Test that install_scripts does not preserve mtime when copying scripts. | ||
| Using mtime serves as a proxy to verify that shutil.copy2 (and thus | ||
| SELinux bin_t contexts) is not being used during script installation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's useful to link the issue here so people reading this know why it matters.
Update the mtime test case based on feedback
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Shrey-N commentedMar 3, 2026
Thank you for the detailed feedback@hroncok ! I have updated the test case to use the Activate.ps1 from the stdlib and followed your plan, including the mode and content assertions. I also added the issue link to the docstring. Let me know if everything looks good now! |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Uh oh!
There was an error while loading.Please reload this page.
| self.assertNotIn(b'__VENV_PYTHON__', src_data, | ||
| "Test assumes Activate.ps1 is a static file, not a template") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion: Move this before the equality check. It's unlikely the files will be identical when this happens, and this assertion is more meaningful than the previous one.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Co-authored-by: Miro Hrončok <miro@hroncok.cz>
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Shrey-N commentedMar 3, 2026
I fixed the order of the checks so the template protection happens first I think that would make sense if something fails. I also applied your suggestion for the docstring, I have updated the branch too! Thanks for the help and the mentorship! Let me know if everything looks good to you.@hroncok |
Removed unnecessary blank lines in the test case.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Added a temporal check to ensure the source file is not modified during the test. Enhanced assertions to protect against future changes in the Activate.ps1 file's structure.
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Misc/NEWS.d/next/Library/2026-03-03-11-49-44.gh-issue-145417.m_HxIL.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Miro Hrončok <miro@hroncok.cz>
Shrey-N commentedMar 3, 2026
Accepted! I have updated the branch and everything is passing now. Thanks again for the help and the mentorship throughout this I really appreciate the guidance!@hroncok |
Lib/test/test_venv.py Outdated
| incorrectly copying e.g. SELinux bin_t context. | ||
| See gh-145417. | ||
| """ | ||
| import time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps move this to the toplevel imports. All the other unconditional imports are there, at the beginning of the file.
hroncok commentedMar 3, 2026
Thanks. I like the test, and the change fixes the reported problem. |
Uh oh!
There was an error while loading.Please reload this page.
Shrey-N commentedMar 3, 2026
I have moved the import time to the top level as you suggested. Everything should be good now! Thank you so much for the detailed review and for all the guidance today it means a lot to me!@hroncok |
Co-authored-by: Miro Hrončok <miro@hroncok.cz>
Uh oh!
There was an error while loading.Please reload this page.
Fixes#145417
When copying scripts in
venvthat don't need variables replaced (likeActivate.ps1),shutil.copy2was incorrectly preserving extended system metadata, including thebin_tSELinux context from the system template directory.This changes
shutil.copy2toshutil.copy, which aligns the behavior with the fallbackelseblock (which usesshutil.copymode).This ensures that only the file contents and permission mode are copied, while safely inheriting the SELinux context of the destination project directory.