From fb162d516f87c914dadee92613a2624a143f8819 Mon Sep 17 00:00:00 2001 From: Mohammed Alkindi Date: Wed, 19 Aug 2026 00:13:49 +0400 Subject: [PATCH 1/2] fix: don't leak or mask errors when rewrite cleanup fails on Windows When set_key or unset_key fails after rewrite() has restored the original file mode, the temporary file has already been chmod'ed to that mode. On Windows a mode without the owner-write bit sets the read-only attribute, so the os.unlink in the cleanup path fails too. The temporary file is left next to the .env file and the cleanup error replaces the real one in the traceback. Reset the mode and retry before giving up, and never let a cleanup failure propagate in place of the error that caused the rewrite to be abandoned. --- src/dotenv/main.py | 23 +++++++++++++++++++++-- tests/test_main.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/dotenv/main.py b/src/dotenv/main.py index 3123690a..f60d4d3b 100644 --- a/src/dotenv/main.py +++ b/src/dotenv/main.py @@ -135,6 +135,25 @@ def get_key( return DotEnv(dotenv_path, verbose=True, encoding=encoding).get(key_to_get) +def _discard_temp_file(path: pathlib.Path) -> None: + """ + Delete `rewrite`'s temporary file, ignoring any failure to do so. + + This runs while another exception is propagating, so it must not raise: + that error is the one worth reporting. On Windows, a file whose mode has + no owner-write bit carries the read-only attribute and can't be unlinked, + so the mode is reset before a second attempt. + """ + try: + path.unlink(missing_ok=True) + except OSError: + try: + path.chmod(stat.S_IWRITE | stat.S_IREAD) + path.unlink(missing_ok=True) + except OSError: + logger.warning("python-dotenv could not remove the temporary file %s", path) + + @contextmanager def rewrite( path: StrPath, @@ -183,10 +202,10 @@ def rewrite( os.replace(dest_path, path) except BaseException: - dest_path.unlink(missing_ok=True) + _discard_temp_file(dest_path) raise else: - dest_path.unlink(missing_ok=True) + _discard_temp_file(dest_path) raise error from None diff --git a/tests/test_main.py b/tests/test_main.py index 6f9d4c5c..ce83a657 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,6 +1,7 @@ import io import logging import os +import pathlib import stat import subprocess import sys @@ -195,6 +196,44 @@ def test_set_key_permission_error(dotenv_path): assert dotenv_path.read_text() == "" +@pytest.mark.skipif( + sys.platform != "win32" and os.geteuid() == 0, + reason="Root user can access files even with 000 permissions.", +) +def test_set_key_permission_error_leaves_no_temp_file(dotenv_path): + if sys.platform == "win32": + # On Windows, make file read-only + dotenv_path.chmod(stat.S_IREAD) + else: + # On Unix, remove all permissions + dotenv_path.chmod(0o000) + + try: + with pytest.raises(PermissionError): + dotenv.set_key(dotenv_path, "a", "b") + + assert list(dotenv_path.parent.glob(".tmp_*")) == [] + finally: + # Restore permissions + if sys.platform == "win32": + dotenv_path.chmod(stat.S_IWRITE | stat.S_IREAD) + else: + dotenv_path.chmod(0o600) + + +def test_rewrite_reports_original_error_when_cleanup_fails(dotenv_path): + replace_error = OSError("replace failed") + + with mock.patch("dotenv.main.os.replace", side_effect=replace_error): + with mock.patch.object( + pathlib.Path, "unlink", side_effect=OSError("unlink failed") + ): + with pytest.raises(OSError) as excinfo: + dotenv.set_key(dotenv_path, "a", "b") + + assert excinfo.value is replace_error + + def test_get_key_no_file(tmp_path): nx_path = tmp_path / "nx" logger = logging.getLogger("dotenv.main") From d04ea348f45f2195b3029be438cf0b8ec726e5f3 Mon Sep 17 00:00:00 2001 From: Saurabh Kumar Date: Thu, 1 Oct 2026 11:59:34 +0530 Subject: [PATCH 2/2] test: cover temp file cleanup on any OS and the cleanup warning Simulate Windows read-only semantics so the chmod-and-retry path is exercised on every platform, for both set_key and unset_key, and assert the warning logged when the temporary file can't be removed. Add a CHANGELOG entry. --- CHANGELOG.md | 3 +++ tests/test_main.py | 49 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea81ecc7..eb54e1cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Fix a package build deprecation warning caused by a non-string `license` value in `pyproject.toml` by [@kurtmckee] in [#648] - `set_key`, `unset_key` and the `dotenv set`/`unset` commands now name the `.env` path instead of an internal temporary file when its directory is missing or not writable, and the CLI prints a short error and exits with code 2 instead of a traceback by [@jamalkamaladdin] in [#711] +- `set_key` and `unset_key` no longer leave a `.tmp_*` file behind on Windows when writing a read-only `.env` fails, and the error raised is the one from the failed write rather than from cleaning up the temporary file by [@MohammedAlkindi] in [#686] ## [1.2.4] - 2026-10-01 @@ -458,6 +459,7 @@ os.PathLike]` instead of just `os.PathLike` (#347 by [@bbc2]). [#648]: https://github.com/theskumar/python-dotenv/pull/648 [#663]: https://github.com/theskumar/python-dotenv/pull/663 [#680]: https://github.com/theskumar/python-dotenv/pull/680 +[#686]: https://github.com/theskumar/python-dotenv/pull/686 [#698]: https://github.com/theskumar/python-dotenv/pull/698 [#700]: https://github.com/theskumar/python-dotenv/pull/700 [#711]: https://github.com/theskumar/python-dotenv/pull/711 @@ -468,6 +470,7 @@ os.PathLike]` instead of just `os.PathLike` (#347 by [@bbc2]). [@23f3001135]: https://github.com/23f3001135 [@EpicWink]: https://github.com/EpicWink [@Flimm]: https://github.com/Flimm +[@MohammedAlkindi]: https://github.com/MohammedAlkindi [@Nicals]: https://github.com/Nicals [@Nougat-Waffle]: https://github.com/Nougat-Waffle [@Qwerty-133]: https://github.com/Qwerty-133 diff --git a/tests/test_main.py b/tests/test_main.py index d1390127..27c5b5fd 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -221,7 +221,7 @@ def test_set_key_permission_error_leaves_no_temp_file(dotenv_path): dotenv_path.chmod(0o600) -def test_rewrite_reports_original_error_when_cleanup_fails(dotenv_path): +def test_rewrite_reports_original_error_when_cleanup_fails(dotenv_path, caplog): replace_error = OSError("replace failed") with mock.patch("dotenv.main.os.replace", side_effect=replace_error): @@ -232,6 +232,10 @@ def test_rewrite_reports_original_error_when_cleanup_fails(dotenv_path): dotenv.set_key(dotenv_path, "a", "b") assert excinfo.value is replace_error + [temp_file] = dotenv_path.parent.glob(".tmp_*") + assert caplog.messages == [ + f"python-dotenv could not remove the temporary file {temp_file}" + ] def test_set_key_missing_directory(tmp_path): @@ -266,6 +270,49 @@ def test_set_key_read_only_directory(tmp_path): assert list(directory.iterdir()) == [dotenv_path] +def windows_read_only_semantics(path): + # On Windows, a file without the owner-write bit can't be replaced or deleted. + return path.exists() and not path.stat().st_mode & stat.S_IWUSR + + +@pytest.mark.parametrize( + "rewrite", + [ + lambda path: dotenv.set_key(path, "a", "y"), + lambda path: dotenv.unset_key(path, "a"), + ], + ids=["set_key", "unset_key"], +) +def test_rewrite_read_only_file_leaves_no_temp_file(tmp_path, rewrite): + dotenv_path = tmp_path / ".env" + dotenv_path.write_text("a=x\n") + dotenv_path.chmod(stat.S_IREAD) + real_replace = os.replace + real_unlink = pathlib.Path.unlink + + def replace(src, dst): + if windows_read_only_semantics(pathlib.Path(dst)): + raise PermissionError( + 13, "Access is denied", os.fspath(src), None, os.fspath(dst) + ) + real_replace(src, dst) + + def unlink(self, missing_ok=False): + if windows_read_only_semantics(self): + raise PermissionError(13, "Access is denied", str(self)) + real_unlink(self, missing_ok=missing_ok) + + with mock.patch("dotenv.main.os.replace", side_effect=replace): + with mock.patch.object(pathlib.Path, "unlink", unlink): + with pytest.raises(PermissionError) as exc_info: + rewrite(dotenv_path) + + dotenv_path.chmod(stat.S_IREAD | stat.S_IWRITE) + assert exc_info.value.filename2 == str(dotenv_path) + assert dotenv_path.read_text() == "a=x\n" + assert list(tmp_path.iterdir()) == [dotenv_path] + + def test_get_key_no_file(tmp_path): nx_path = tmp_path / "nx" logger = logging.getLogger("dotenv.main")