Hard coded chmod when temp_file=True

[Erotemic]

First off, this package is wonderful. Thank you for writing/maintaining it.

Now, the issue. When calling safer.open with temp_file=True, there the final on-success step will attempt to run chmod on the temporary file if the ultimate destination does not exist, otherwise it will attempt to copy the permissions from that file to the new file.

For a new file this results in a different set of permissions than if you were to run without tempfile. Specifically, by default my ubuntu system will write a new file with 664 permissions, whereas this forces the use of 644 (with the S_ISVTX sticky bit for some reason?). I have a MWE which demonstrates this:

"""
References:
    https://stackoverflow.com/questions/36745577/how-do-you-create-in-python-a-file-with-permissions-other-users-can-write
"""

import ubelt as ub
import safer
import os
dpath = ub.Path.appdir('misc/tests/file_perm_with_open').delete().ensuredir()

new_fpath1 = dpath / 'fpath_builtin_open.txt'
with open(new_fpath1, 'w') as f:
    f.write('foo')

new_fpath2 = dpath / 'fpath_os_open.txt'
descriptor = os.open(
    path=new_fpath2,
    flags=(
        os.O_WRONLY  # access mode: write only
        | os.O_CREAT  # create if not exists
        | os.O_TRUNC  # truncate the file to zero
    ),
    mode=0o640
)
with open(descriptor, 'w') as f:
    f.write('some text')

new_fpath3 = dpath / 'fpath_safer_open_defualt.txt'
with safer.open(new_fpath3, 'w') as f:
    f.write('safer without tempfile')

new_fpath4 = dpath / 'fpath_safer_open_temp.txt'
with safer.open(new_fpath4, 'w', temp_file=True) as f:
    f.write('safer with tempfile')

_ = ub.cmd('ls -al', cwd=dpath, verbose=3)

with the final output being:

┌─── START CMD ───
[ubelt.cmd] joncrall@toothbrush:~/.cache/misc/tests/file_perm_with_open$ ls -al
total 24
drwxrwxr-x 2 joncrall joncrall 4096 Sep  5 12:32 .
drwxrwxr-x 3 joncrall joncrall 4096 Sep  5 12:32 ..
-rw-rw-r-- 1 joncrall joncrall    3 Sep  5 12:32 fpath_builtin_open.txt
-rw-r----- 1 joncrall joncrall    9 Sep  5 12:32 fpath_os_open.txt
-rw-rw-r-- 1 joncrall joncrall   22 Sep  5 12:32 fpath_safer_open_defualt.txt
-rw-r--r-- 1 joncrall joncrall   19 Sep  5 12:32 fpath_safer_open_temp.txt
└─── END CMD ───

In this example I test 4 methods of writing a file:

• using the builtin open function
• using os.open to create a file descriptor with custom permissions before any writing is done
• safer.open with default arguments
• safer.open with temp_file=True

We can see that the temp file causes different perms to be written.

It seems like "the right" thing to do, would be one of the following:

• don't change the permission of the temp file by default
• allow the user to specify what the final permissions should be (or perhaps what the permissions for a new file should be?)

It might also make sense for safer to roll its own temporary file mkstemp function so it can control the permissions the temporary file gets created with rather than try to change them after the fact.

[rec]

Wow, this is one of the best written bug reports for my stuff I've gotten, kudos!

Maybe I like the second idea better because it's a bit more general, though a bit more work.

Now I write that, maybe I like the first idea better, because less is more and adding more to the API is... more.

😀

I won't have time to really look at this until Saturday: I'm creating a calendar entry so I don't forget, but don't hesitate to remind me if you don't hear from me by Sunday!

Or if you were bored and had spare time, I also accept pull requests.

Thanks for finding and documenting this!!

[rec]

Interesting update: I couldn't reproduce this on my Mac, so I fired up someone's Linux qgpu3 5.15.0-97-generic #107-Ubuntu SMP Wed Feb 7 13:26:48 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux and I got the same results as you did!

So there will be a delay while I set up this Ubuntu instance for development.

On the Mac, I got

┌─── START CMD ───
[ubelt.cmd] tom@bolt.local:~/.cache/misc/tests/file_perm_with_open$ ls -al
total 32
drwxr-xr-x  6 tom  staff  192 Sep  7 19:13 .
drwxr-xr-x  3 tom  staff   96 Sep  7 19:13 ..
-rw-r--r--  1 tom  staff    3 Sep  7 19:13 fpath_builtin_open.txt
-rw-r-----  1 tom  staff    9 Sep  7 19:13 fpath_os_open.txt
-rw-r--r--  1 tom  staff   22 Sep  7 19:13 fpath_safer_open_defualt.txt
-rw-r--r--  1 tom  staff   19 Sep  7 19:13 fpath_safer_open_temp.txt
└─── END CMD ───

[Erotemic]

The rules for default file permissions will vary based on the operating system and even on linux, access control lists (ACL) will influence the permission a file gets created with. I think the safest thing to do is just use whatever the operating system default is when the file is created and avoid ever calling chmod inside safer itself.

I also think that if there is going to be any modification of permission, it needs to happen when the file is created via the os.open low level file descriptor interface. Otherwise you could run into a security issue (e.g. the default might allow group reads, but perhaps the user does not intend for this to happen).

I'll also add, the reason I found this in the firstplace is because I work on a server where there is a constant chronjob that every 15 minutes loops over the entire filesystem and changes the owner to root, and the group to something based on a directory. (It's kinda gross). This script executed in the middle of me writing a large file with safer, so when the temp file was created I was the owner, but the chron job hit it, which set the owner to root. Something I learned is that even if you have group write permissions for a file only the owner of a file can chmod it, so when safer called chmod it caused an error.

[rec]

So I emitted a pull request which has a test for your case, #25

Feel free to give it a review! I'm going to write up a bit what's going on in the review.

[rec]

Sorry, I didn't see the response and let it slide.

This change is not 100% backward compatible, so I decided to make a major release - released as safer 5.0.0.

Hope this fixes your issue! Tweaks may be possible. :-D