Repository navigation
refactor(util): clear read-only in rmfile with stat.S_IWUSR - #2328
EliahKagan merged 2 commits into
Conversation
`git.util.rmfile` exists so that a read-only file can be deleted on Windows, where `os.remove` fails on files with the read-only attribute. On Windows it calls `os.chmod` to clear that attribute before removing the file. Nothing tested this. Before this change, the only callers of `rmfile` were `LockFile._release_lock` and a lock-file helper in `test/test_repo.py`, and no test called either of them on a read-only file. Add `TestRmfile.test_deletes_readonly_file`, alongside the analogous `TestRmtree.test_deletes_dir_with_readonly_files`. Its initial state comes from a new fixture script, `tests/fixtures/rmfile.sh`. The script creates a file named `readonly` and removes its write permissions with `chmod a-w`, as `tests/fixtures/cleanup.sh` does for its read-only case. On Windows this should give the file the read-only attribute. The test uses a copy of the cached fixture output, which keeps the file's mode, so it needs no `execute=True`. The script is separate from `tests/fixtures/rmtree.sh`, whose cases are keyed by `TestRmtree` test names through the `tmp_path` fixture in `test/test_util.py`, and which that fixture runs in place for all but one of them. `rmfile` silently does nothing if the path is not a file, so the test would pass vacuously if the fixture made no file. It therefore first checks that the file exists and is not owner-writable, and only then that `rmfile` removes it. On Windows, `os.stat` reports the read-only attribute as the absence of write bits, so this also confirms that the fixture produced the condition the test is about, rather than assuming it. The test runs on every platform, but it is only meaningful on native Windows. Unlike the `rmtree` test, it is not skipped on Cygwin. That skip's reason, that Cygwin can't set the needed permissions, hasn't been checked for this test. On Cygwin `sys.platform` is `"cygwin"`, so `rmfile` uses no special handling there, and the test only checks that a read-only file can be removed, as on Unix. This adds coverage ahead of changing the mode `rmfile` passes to `os.chmod`: the test passes with the current `0o777`, and on Windows it would fail if the mode lacked the owner-write bit. Validation: on macOS with Python 3.12, the four `TestRmtree` and `TestRmfile` tests pass, the copied fixture file has mode `0o444`, and the precondition fails when the path does not exist. `shellcheck` passes on the new script. Neither Windows nor Cygwin was run locally. The `import-cygwin` workflow does not run the test suite, so Cygwin is not tested in CI either. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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>
There was a problem hiding this comment.
I've read through these changes, commit messages, and the PR description carefully, after iterating on them with the model (Claude Opus 5.5), and they look good to me. I plan to merge once CI passes. Several subtleties:
-
As noted, I switched CodeQL in this repository from using the default query suite to the extended suite. Because we are currently on the default setup rather than the advanced setup, this does not have an associated code change; it's in the Advanced Security settings.
The extended suite includes queries for Python code like the one here, which was arguably more of a style concern in this case, yet which often does indicate a serious security bug. But it also includes more queries for GitHub Actions, which was my main motivation for enabling it. It is not responsible for new false positives (unless you consider the style nit fixed in this PR as a false positive). Everything that has been marked as a false positive here or that looks like it plausibly could be a false positive is from before that change.
The default versus extended query suite is a conceptually separate and mostly practically separate choice from the default versus advanced setup/configuration. These two dichotomies should not be confused. I have not at this time made a change to which setup/configuration we use. I plan to do so sometime soon--but in a way that respects both the intent and good effects of the recent switch from the advanced setup to the default setup, as I understand them. More on this below, since it relates to ordering and validation.
-
Claude rightly wrote:
Not verified: that alert #56 closes. CodeQL default setup doesn't analyze PRs from forks. I (Claude) expect the alert to close as fixed on the first analysis of
mainafter merging, since the identical call inrmtreeis not flagged.CodeQL was run from an advanced, i.e. workflow-based, configuration until it was changed to a default, i.e. dynamic job, based configuration in mid-September. I think that change, as it was done, was overall improvement, because it improved on some outdated aspects of the workflow, including removing some exclusions that are present in the workflow to scan more of the project. (As of this writing, those exclusions are present in the workflow, but the workflow is not what gets used in this repo.)
However, it would be better to have the best of both worlds--an advanced configuration that uses good current configuration choices--becuase, as seen here, the default configuration can't run on PRs originating from outside the repository. This is an underdiscussed limitation and I have come to consider it a compelling reason to prefer the advanced CodeQL configuration anytime it is manageable, aside from repositories that cannot get or are unlikely ever to get PRs from forks.
In practice, the default configuration also does not run in Dependabot PRs, even though those are not associated with any other repository. The checks are created, but they stay gray and accurately report that they can't actually scan anything. See, for example, this run on #2312.
I plan to make that change--i.e., to using the advanced configuration but not with the problems that it had before the mid-September switch back to the default configuration. But I intentionally did this PR first, rather than in the other order or together, to double-check and demonstrate that we indeed do not get CodeQL under these circumstances. This is mostly about demonstration, since it also hasn't run it my other recent PRs.
I may not do the configuration-type change and associated CodeQL workflow rewrite immediately, since I have some other stuff to take care of right now.
-
The second commit says no test could distinguish the old and new way because they have the same behavior on Windows which is where the modified statement runs. That's mostly true, but Codex (running GPT-6.1 Sol) rightly pointed out--when I had it do an additional review of the changes and all claims in the description and commit messages--that it is possible to use audit hooks in a test to see the exact values passed to
os.chmod.I didn't think that required changing the commit message (and neither did Codex), but I mention it here anyway. The reason I'm comfortable with the commit message as written is that I think what it is really expressing and will be understood to mean is that there is no behavioral difference in the effect on disk, or in the effect that Python attempts to produce on disk, between the two.
Note
AI-generated PR: This pull request, its commits and this description were written by an AI agent (Claude Code, using Claude Opus 5.5) acting on behalf of Eliah Kagan through Eliah's GitHub account.
CodeQL default setup for this repository was switched (by Eliah) from the
defaultquery suite toextended. The first extended analysis ofmain(at 38992bc) raised exactly one new alert, #56. It is a high-severitypy/overly-permissive-file("Overly permissive mask in chmod sets file to world writable") on theos.chmod(path, 0o777)ingit.util.rmfile.That call only runs on Windows, where it clears the read-only attribute so that
os.removecan delete the file. There,os.chmodignores every bit except0o200(stat.S_IWUSR). So0o777was never more permissive in practice, but it says more than the code means. This PR passesstat.S_IWUSRinstead, as thermtreeerror handler just above already does. CodeQL doesn't flag that call. Behavior is unchanged on every platform. The commit messages give the details, including where CPython implements this.Nothing tested
rmfileon a read-only file, so this also addsTestRmfile.test_deletes_readonly_file. It checks that its fixture really made a non-writable file before checking thatrmfiledeletes it. On Windows, it would fail ifrmfilepassed a mode without the owner-write bit.Validation:
test (windows, 3.14)job of this run passed, and its log showsTestRmfile::test_deletes_readonly_file PASSED. The run tested f62223c, which has the same tree as this PR's head; only the commit messages have changed since. When this PR was opened, the run's other Windows test jobs were still in progress, and no job in the run had failed.TestRmtreeandTestRmfiletests pass both with and without the change.ruffandshellcheckpass.import-cygwinworkflow does not run the test suite.mainafter merging, since the identical call inrmtreeis not flagged.🤖 Generated with Claude Code