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
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
OK, However, I was tryign to figure out how to properly test this. |
|
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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb 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.
I think it might be better to actually use Activate.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.
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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
| 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.
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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb 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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb 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 Python require a NEWS entry. Add one using the blurb_it web app or the blurb 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
Outdated
Show resolved
Hide resolved
Co-authored-by: Miro Hrončok <miro@hroncok.cz>
|
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.
Perhaps move this to the toplevel imports. All the other unconditional imports are there, at the beginning of the file.
|
Thanks. I like the test, and the change fixes the reported problem. |
|
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>
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.