requests.utils. atomic_open does not respect umask

[austinzh]

Create a new file using requests.utils. atomic_open with umask set to 0o000

Expected Result

0o777

Actual Result

0o600

Reproduction Steps

import os
import requests.utils

os.umask(0o000)

with requests.utils.atomic_open('example.txt') as f:
    f.write(b'Hello, world!')

file_stat = os.stat('example.txt')
print(oct(file_stat.st_mode & 0o777))

System Information

$ python -m requests.help

{
  "chardet": {
    "version": null
  },
  "charset_normalizer": {
    "version": "3.3.2"
  },
  "cryptography": {
    "version": ""
  },
  "idna": {
    "version": "3.7"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.10.6"
  },
  "platform": {
    "release": "6.2.0-1016-aws",
    "system": "Linux"
  },
  "pyOpenSSL": {
    "openssl_version": "",
    "version": null
  },
  "requests": {
    "version": "2.32.3"
  },
  "system_ssl": {
    "version": "30000020"
  },
  "urllib3": {
    "version": "2.2.1"
  },
  "using_charset_normalizer": true,
  "using_pyopenssl": false
}

[sigmavirus24]

atomic_open is not intended for public consumption. Even still, the sole purpose of it in requests is to extract a CA truststore file from a zip (where necessary) and place it in a given location. In that case, regardless of what one might expect from umask, it's for the user's protection (and this desirable) that a file only be readable and writable by them to avoid someone corrupting the store and introducing an opportunity for a MitM attack by injecting an untrusted root.

Further still, requests itself does not change the permissions on the file (although now that you raise this I believe it should for the reasons I mentioned above) and I suspect the bug is actually in one of the standard library functions we use to implement this atomic open function, not requests.

[WajahatKanju]

I've been working on the issue with requests.utils.atomic_open not respecting the umask settings, and I've tried a few different approaches to fix it. Here’s what I’ve done so far and what I’ve found:

What I Tried

1. Original Approach:
   Initially, I used tempfile.mkstemp to create a temporary file, and then replaced it with the target file. However, the file permissions still ended up being the default settings due to umask.

2. Adjusting Permissions:
   Added os.chmod(tmp_name, desired_permissions) before and after replacing the file to explicitly set permissions on the temporary file. This approach still resulted in the final file having default umask permissions (e.g., 0o666), rather than the desired permissions (e.g., 0o777).

3. Double os.chmod Call:
   Modified the function to include a second os.chmod call after replacing the temporary file with the target file. This approach also failed to achieve the desired permissions, consistently resulting in default permissions.

Observations

• The permissions of the final file consistently reflect the default umask settings (e.g., 0o666) rather than the intended settings (e.g., 0o777).
• The issue seems to be related to how umask is applied or handled during file creation and replacement.

@contextlib.contextmanager
def atomic_open(filename):
    # Create a temporary file with default permissions
    tmp_descriptor, tmp_name = tempfile.mkstemp(dir=os.path.dirname(filename))
    try:
        with os.fdopen(tmp_descriptor, "wb") as tmp_handler:
            yield tmp_handler

        # Get the current umask and restore it after setting desired permissions
        current_umask = os.umask(0)
        os.umask(current_umask)
        desired_permissions = 0o777 & ~current_umask

        # Set permissions of the temporary file
        os.chmod(tmp_name, desired_permissions)

        # Replace the target file with the temporary file
        os.replace(tmp_name, filename)

        # Ensure the final file has the correct permissions
        os.chmod(filename, desired_permissions)
        
        # Verify and print permissions of the replaced file
        file_stat = os.stat(filename)
        print(f"Desired permissions: {oct(desired_permissions)}")
        print(f"Actual permissions after replacement: {oct(file_stat.st_mode & 0o777)}")

    except Exception as e:
        # Clean up the temporary file if an exception occurs
        os.remove(tmp_name)
        raise e

Thank you for your attention to this matter. Any insights or suggestions would be greatly appreciated.