Prevent file_move_safe() from overwriting a concurrently created destination.

[Sarah Boyce]

When allow_overwrite=False, django.core.files.move.file_move_safe() first checks whether the destination exists before calling os.rename():

    if not allow_overwrite and os.access(new_file_name, os.F_OK):
        raise FileExistsError(
            f"Destination file {new_file_name} exists and allow_overwrite is False."
        )

    try:
        os.rename(old_file_name, new_file_name)
        return
    except OSError:
        # OSError happens with os.rename() if moving to another filesystem or
        # when moving opened files on certain operating systems.
        pass

This introduces a race window where the destination may be created after the existence check but before os.rename() is called. On POSIX systems, os.rename() replaces an existing destination, so the file may be overwritten despite allow_overwrite=False.

The following test demonstrates the issue:

tests/files/tests.py

@@ -520 +520 @@
 
         self.assertEqual(content, b"content")
 
+    @mock.patch("django.core.files.move.os.access")
+    def test_file_move_created_after_exists_check(self, mocked_exists_check):
+        with tempfile.NamedTemporaryFile(delete=False) as src:
+            src.write(b"file to move")
+            src_name = src.name
+
+        dest_dir = tempfile.mkdtemp()
+        dest_name = os.path.join(dest_dir, "destination.txt")
+
+        def create_destination(*args, **kwargs):
+            with open(dest_name, "wb") as dest:
+                dest.write(b"existing destination")
+            return False
+
+        # Simulate the destination being created after the existence check.
+        mocked_exists_check.side_effect = create_destination
+
+        # TODO: This should probably raise a FileExistsError.
+        file_move_safe(src_name, dest_name, allow_overwrite=False)
+
+        with open(dest_name, "rb") as dest:
+            self.assertEqual(dest.read(), b"existing destination")
+        self.assertTrue(os.path.exists(src_name))

The expected behavior is that the existing destination is preserved and the source file is not moved, likely by raising FileExistsError.

Thanks to Peng Zhou for originally reporting this issue to the Django security team. After review, it was determined not to be a security issue, but worth addressing. Similar file_move_safe() races have previously been addressed through the public issue tracker (#20486, #35323).

[Natalia Bidart]

Thanks!

[vimalsahani2005]

Hi everyone,

I noticed that this ticket was accepted but has had no active PR or updates for the past two months. I would like to work on this issue and prevent the race condition in file_move_safe() when allow_overwrite=False.

I can implement the fix to ensure a concurrently created destination is preserved (raising FileExistsError) along with the corresponding unit tests in tests/files/tests.py.

Could you please assign this ticket to me? Thank you!

[SnippyCodes]

Created ​https://github.com/django/django/pull/22014 to address this issue.