WIP: Set permissions for atomic open_file() #1382
Conversation
Previously, when `open_file()` was called with `atomic=True`, the target file's permissions were always set to 0600, i.e. readable and writable by the current user. These permissions come from the temporary file created by `tempfile.mkstemp()` [1]. This commit changes an atomic `open_file()` call to set the permissions of the target file. If the target file already exists, then its current permissions are retained. Otherwise, the permissions respect the current umask. [1] https://docs.python.org/3.7/library/tempfile.html#tempfile.mkstemp
msmolens
changed the title
click/_compat.py
Outdated
| @@ -33,6 +34,12 @@ def _make_text_stream(stream, encoding, errors, | |||
| force_readable=force_readable, | |||
| force_writable=force_writable) | |||
|
|
|||
| def _get_current_umask(): | |||
| """Get current umask.""" | |||
| umask = os.umask(0) | |||
There was a problem hiding this comment.
This is, unfortunately, a potential security problem in the presence of multiple threads. (See https://bugs.python.org/issue21082.)
I’m not sure what to suggest instead, other than avoiding tempfile and writing the loop to safely create a fresh filename manually.
click/_compat.py
Outdated
| try: | ||
| permissions = stat.S_IMODE(os.stat(real_filename).st_mode) | ||
| except OSError: | ||
| permissions = 0o666 & ~_get_current_umask() |
There was a problem hiding this comment.
IMO for the fallback it's better to set here a default permission, like 0o666 or even more paranoidal 0o600. As @andersk right said it's not thread-safe.
There was a problem hiding this comment.
Or even better, just pass and let the permissions be None.
There was a problem hiding this comment.
The default permissions if you didn’t specify atomic=True are implicitly 0o666 & ~umask, because that’s the default behavior of open() (no explicit umask computation is required). So it’s unexpected for atomic=True to result in different permissions. permissions = 0o666 is definitely too permissive, as chmod is not implicitly masked by umask, while permissions = 0o600 (or pass, which has the same effect) is too restrictive.
The underlying problem is that tempfile simply has the wrong permissions behavior for this use case. It’s designed for temporary files that will later be deleted, not for creating files with which to atomically replace other files.
There was a problem hiding this comment.
You are right, anyways at least on save an existing file there will be correct permissions.
Add vendored tempfile modules that are modified to create files with standard permissions that respect the current umask. The built-in tempfile modules create files that are readable and writable only by the creating user. Use the modules when creating a temporary file for atomic open_file().
msmolens
changed the title
|
I really appreciate the work you're putting into this, but this is way more complexity than I'm willing to add and support. Vendoring two copies of Python's tempfile module is not worth it. Does python-atomicwrites suggested in #320 have this issue? cc @untitaker |
|
@davidism Thanks for the review. I agree that vendoring the modules is heavy-handed and not a great way forward. I'll close this pull request for now. Note that python-atomicwrites also changes the permissions: untitaker/python-atomicwrites#42 |
|
Atomicwrites does NOT intentionally change permissions. Different permissions come from a result of how the file write is done. |
|
I took a more minimalist stab at this myself in #1400. @msmolens I copied your tests from here; hope that’s fine. @untitaker You can certainly argue that it “does not change permissions” because it internally works by creating a new file with different permissions and renaming it over the old file. But that argument is of little interest to a developer who wanted the permissions to stay the same. |
|
I believe #1400 is a correct fix for this, and I am surprised that one exists. It's been a while since I looked into this, but somehow I came to the wrong conclusion that the umask was broken on file rename, not on We could keep the impl in click simple and have atomicwrites as optional dep for "better" behavior. |
Previously, when
open_file()was called withatomic=True, the targetfile's permissions were always set to 0600, i.e. readable and writable
by the current user. These permissions come from the temporary file
created by
tempfile.mkstemp().This commit changes an atomic
open_file()call to set the permissionsof the target file. If the target file already exists, then its current
permissions are retained. Otherwise, the permissions respect the current
umask.
Fixes #1376