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/src/dotenv/main.py b/src/dotenv/main.py index 95ee9db2..13f6776c 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, @@ -191,10 +210,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 ded4f1f6..27c5b5fd 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,48 @@ 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, caplog): + 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 + [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): dotenv_path = tmp_path / "nx_dir" / ".env" @@ -227,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")