Back up on scrub #1660
Back up on scrub #1660
Conversation
sergiomb2
commented
Nov 26, 2025
sergiomb2
commented
- make mock --scrub=all correctly back up build directory
- Use 'mv' instead of 'cp' for backing up builds
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the backup logic by moving it from backend.py to buildroot.py and changing the operation from copy (cp) to move (mv). This is a good refactoring as the logic is now closer to where the buildroot is actually deleted. However, I've found a critical security vulnerability related to command injection when moving the RPM files. The way the mv command is constructed is unsafe and could allow for arbitrary code execution if a filename contains special shell characters. I've provided a suggestion to fix this using Python's shutil module, which is the safer and more idiomatic approach. I also found a minor style issue where a multi-line string is used for a comment inside a method, which should be a regular hash-prefixed comment. Please address the critical security issue.
sergiomb2
force-pushed
the
back_up_on_scrub
branch
2 times, most recently
from
57d76c8 to
2f02a9d
Compare
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
2f02a9d to
79b7a3f
Compare
xsuchy
left a comment
xsuchy
left a comment
There was a problem hiding this comment.
LGTM
Can you add release entry as described in https://rpm-software-management.github.io/mock/Release-Notes-New-Entry please?
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
79b7a3f to
1149de1
Compare
|
Done. Let me know if I can improve anything else. |
|
I found a bug "shutil.Error: Destination path '/var/lib/mock/backup/fedora-43-x86_64/javapackages-compat-6.4.1-5.fc43.noarch.rpm' already exists" |
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
1149de1 to
cf5d223
Compare
sergiomb2
force-pushed
the
back_up_on_scrub
branch
2 times, most recently
from
daf4800 to
3089847
Compare
|
Can you please rebase on top of |
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
3089847 to
45d15c3
Compare
|
Lint Python issues / python-lint-job (pull_request)Failing after 46s I'm working on another solution |
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
45d15c3 to
1b583a7
Compare
sergiomb2
force-pushed
the
back_up_on_scrub
branch
2 times, most recently
from
2f9cdbf to
3879063
Compare
|
/packit test |
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
3879063 to
01061cb
Compare
praiskup
left a comment
praiskup
left a comment
There was a problem hiding this comment.
Sorry for the delay. Otherwise looks good, thank you for the fix.
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
01061cb to
9503dd4
Compare
…buildroot. Fixes issue rpm-software-management#1639. The backup process now uses `mv` semantics instead of `cp`, avoiding file duplication, preserving timestamps, and improving performance. Gemini Code Assist flagged the use of `util.run` as unsafe, so it was replaced with `os.replace` to safely overwrite existing files. Improve logs and comments in backup_build_results for clarity.
sergiomb2
force-pushed
the
back_up_on_scrub
branch
from
9503dd4 to
cb07a9a
Compare
3 participants
