Repository navigation
Commit 53f3c62
refactor(util): clear read-only in
On Windows, `git.util.rmfile` calls `os.chmod(path, 0o777)` before
`os.remove` so that read-only files can be deleted. With the CodeQL
`security-extended` suite now enabled for this repository, that call is
reported as `py/overly-permissive-file` ("Overly permissive mask in
chmod sets file to world writable"), at high severity.
The finding is a false positive in effect. On Windows, `os.chmod` can
only set or clear the read-only attribute, and the Python documentation
says "All other bits are ignored". CPython's implementation confirms
this. Despite its name, `Modules/posixmodule.c` is also the Windows
implementation: `os` takes its functions from a built-in module that is
`posix` on Unix and `nt` on Windows, and both are compiled from that
file, with the Windows code under `MS_WINDOWS`. There, `chmod` tests
only `mode & _S_IWRITE`, clearing `FILE_ATTRIBUTE_READONLY` when that
bit is set and setting it otherwise. This is the same in 3.8, the
oldest Python GitPython supports, as in `main`, including the newer
handle-based path. `_S_IWRITE` is `0o200`, the same value as
`stat.S_IWUSR` (and `stat.S_IWRITE`). The call is only reached when
`sys.platform == "win32"`, so every bit in `0o777` except `0o200` was
always ignored.
Pass `stat.S_IWUSR` instead. On Windows this behaves identically: it
clears the read-only attribute and does nothing else. It states the
intent exactly, and it matches the `os.chmod(path, stat.S_IWUSR)`
already used for the same purpose by the `rmtree` error handler a few
lines above, which CodeQL does not flag.
Behavior is unchanged on all platforms, so no test can fail before this
change and pass after it. `TestRmfile.test_deletes_readonly_file`
covers the behavior being preserved, and on Windows would fail if the
mode lacked the owner-write bit.
Validation: on macOS with Python 3.12, the `TestRmtree` and `TestRmfile`
tests pass both before and after this change, and `ruff check` and
`ruff format --check` pass. Windows, where the changed line actually
runs, was not run locally and is left to CI.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>rmfile with stat.S_IWUSR
1 parent d74c104 commit 53f3c62
1 file changed
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
211 | 211 | | |
212 | 212 | | |
213 | 213 | | |
214 | | - | |
| 214 | + | |
215 | 215 | | |
216 | 216 | | |
217 | 217 | | |
| |||
0 commit comments