fix: do not read through a symlink when rewriting a .env file - #690
fix: do not read through a symlink when rewriting a .env file#690Gares95 wants to merge 1 commit into
Conversation
`rewrite()` no longer follows symlinks when replacing a `.env` file, but it still opened the path for reading with `open()`, which does follow them. When the path was a symlink, the target's contents were read and carried into the regular file that replaced the link, so keys from the target appeared in the new `.env`. The target itself was never modified, so this is not the file-overwrite issue addressed previously. It is a narrower mismatch between what the rewrite says it does and what it does: `os.replace()` replaces the link rather than its target, so the target's contents are not the contents of the file being written. Open the source with `O_NOFOLLOW` unless `follow_symlinks=True`, and treat the resulting error the same way a missing file is treated, since a symlink we are not following has no existing content to carry over. `O_NOFOLLOW` is POSIX only; where it is unavailable the flag is a no-op and behaviour is unchanged. The existing symlink tests could not catch this. Both used a target whose only key was the key being set or unset, so anything read through the link was overwritten by the operation, and the `set_key` test asserted with `in` rather than `==`. Both now use a different key in the target and assert exact contents.
|
Withdrawing this. On further testing the change loses data, so it should not be
The behaviour I was trying to tighten is minor next to that, so the trade is not Worth flagging for anyone who picks this up later: the existing symlink test did Sorry for the noise, and thanks for the CI time. |
Description
rewrite()no longer follows symlinks when it replaces a.envfile, but itstill opens the path for reading with
open(), which does follow them. When thepath is a symlink, the target's contents are read and carried into the regular
file that replaces the link:
unset_keybehaves the same way.To be clear about what this is not: the target file is never modified, so this
is not the file-overwrite problem fixed in 1.2.2, and the replacement file is
created with mode
0600, so it is not readable by another user. It is anarrower mismatch between what the rewrite says it does and what it does. The
1.2.2 change described itself as "
set_keyandunset_keyused to followsymlinks in some situations. This is no longer the case", and
os.replace()replaces the link rather than its target, so the target's contents are not the
contents of the file being written.
The fix
Open the source with
O_NOFOLLOWunlessfollow_symlinks=True, and treat theresulting error the way a missing file is already treated: a symlink that is not
being followed has no existing content to carry over into its replacement.
O_NOFOLLOWis POSIX only. Where it is unavailable the flag is a no-op andbehaviour is unchanged, which is why the platform errno set is built defensively
rather than assumed.
This is a behaviour change in the same sense the 1.2.2 notes used: a
.envthatis a symlink to a file with other keys will now be replaced by a file containing
only what was written, rather than the target's keys plus what was written.
Why the existing tests did not catch it
test_set_key_symlink_to_existing_fileandtest_unset_key_symlink_to_existing_fileboth used a target whose only key wasthe key being set or unset, so anything read through the link was overwritten by
the operation and never visible. The
set_keytest also asserted withinrather than
==, which tolerated extra content.Both now put a different key in the target and assert exact contents, so they
fail against the previous behaviour and pass with this change.
Validation
pytest: 252 passed, 1 skipped.ruff check src testsandruff format --check src testsclean.mypy src testsreports no issues oneach of the versions the lint environment checks, 3.10 through 3.14.