fetchart: Creates files with incorrect permissions

[rsammelson]

On linux, running beet fetchart creates files with 600 permissions, rather than 644 (or using the default provided by umask).

Setup

• OS: Debian 11
• Python version: 3.9.2
• beets version: 1.6.0
• Turning off plugins made problem go away (yes/no): issue with plugin

My configuration (output of beet config) is:

fetchart:                                           
    sources: filesystem coverart
    auto: yes                                       
    minwidth: 0                                     
    maxwidth: 0                                     
    quality: 0                                      
    max_filesize: 0                                 
    enforce_ratio: no                               
    cautious: no                                    
    cover_names:                                    
    - cover                                         
    - front                                         
    - art                                           
    - album                                         
    - folder                                        
    google_key: REDACTED                            
    google_engine: 001442825323518660753:hrh5ch1gjzm 
    fanarttv_key: REDACTED
    lastfm_key: REDACTED                            
    store_source: no                                
    high_resolution: no                             
    deinterlace: no                                 
    cover_format:

[sampsyo]

Hi! Can you please include your full configuration, and perhaps a verbose log from a case where fetchart did this?

[rsammelson]

The issue is cause by the fact that temporary files are created with 600 permissions. In set_art: https://github.com/beetbox/beets/blob/6989bce6cab134e937acd21720b4a165f916a60b/beets/library.py#L1340-L1343 So if copy is false, the temporary files are copied over, including their incorrect permissions. The value of copy is from not self.src_removed, here: https://github.com/beetbox/beets/blob/6989bce6cab134e937acd21720b4a165f916a60b/beetsplug/fetchart.py#L1064 where self.src_removed is: https://github.com/beetbox/beets/blob/6989bce6cab134e937acd21720b4a165f916a60b/beetsplug/fetchart.py#L981-L982 so the issue occurs whenever delete or move are set.

I believe the solution is to not use move for temporary files, I'm not sure how to best implement that.

[sampsyo]

Hmm; I'm not entirely sure I understand the root cause: namely, it seems important to know why the original files (the ones being moved) are created with 0600 permissions. It seems like those should be created with umask defaults?

[rsammelson]

From the python documentation (https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp)

Creates a temporary file in the most secure manner possible. There are no race conditions in the file’s creation, assuming that the platform properly implements the os.O_EXCL flag for os.open(). The file is readable and writable only by the creating user ID. If the platform uses permission bits to indicate whether a file is executable, the file is executable by no one. The file descriptor is not inherited by child processes.

So this is expected behavior from the library. I see in the util folder: https://github.com/beetbox/beets/blob/6989bce6cab134e937acd21720b4a165f916a60b/beets/util/init.py#L468-L475 So here it is implied that removing metadata is bad—which is probably true when moving existing images—but when using new images downloaded by the program, the metadata (from tempfile) should be discarded in favor of the default metadata.

[sampsyo]

Got it; wow; thanks for clarifying!! I did not know this about mkstemp and friends.

Seems like we clearly need to either:

• Do something different from move's current behavior when moving the images into place, or:
• Create the temporary files with umask default permissions, instead of mkstemp's default permissions.

[rsammelson]

mkstemp does not have a way to change the permissions as far as I can tell, so I'm guessing that the easiest way to do it is to use copy instead of move for temporary files. I'm not sure what the best way to implement that would be though.

[sampsyo]

That makes sense, although it's a tad unfortunate. There are two things that make that route (leaving the temporary files as they are, and copying them instead of moving):

• It's a little wasteful, obviously, when the destination is on the same filesystem as the temporary directory.
• It complicates the case when the source is not /tmp but some existing file on the filesystem. Namely, the fetchart plugin can also pick up files from the filesystem (aside from just downloading images from online sources). So this could complicate the code a bit.

[rsammelson]

Yeah, the second one is why I couldn't figure out a good way to implement it. It would probably be the best to just reset the permissions, but I haven't found a great way of doing that in python that is cross-platform.

[aereaux]

Hi, I'm getting this problem when importing music from a zip file. It looks like the temporary files created here (https://github.com/beetbox/beets/blob/master/beets/util/init.py#L527) are created with 600 permissions. I was wondering why util.move uses all the code it does instead of using something like shutil.move (https://docs.python.org/3/library/shutil.html#shutil.move)?