From 1b994fe27235a0a9efd8ee76acdc531c4db7566c Mon Sep 17 00:00:00 2001
From: Jens Kraemer <jk@jkraemer.net>
Date: Mon, 5 Jul 2021 11:16:38 +0800
Subject: [PATCH] ensure unique attachment filenames

Checking file name existence without actually creating a file with that
name creates a race condition where parallel uploads of same-named files
can lead to collisions.

This patch changes Attachment.disk_filename so it actually tries to
create a file with the File::EXCL flag set. It will then yield the open file
handle once the operation succeeded, so it can be used in File.open's
place in the calling code.

I renamed the method to create_diskfile to reflect the changed semantics.
---
 app/models/attachment.rb     | 31 ++++++++++++++++++-------------
 test/unit/attachment_test.rb | 28 ++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index c3c3fc8b3..c65c56722 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -138,14 +138,10 @@ class Attachment < ActiveRecord::Base
   def files_to_final_location
     if @temp_file
       self.disk_directory = target_directory
-      self.disk_filename = Attachment.disk_filename(filename, disk_directory)
-      logger.info("Saving attachment '#{self.diskfile}' (#{@temp_file.size} bytes)") if logger
-      path = File.dirname(diskfile)
-      unless File.directory?(path)
-        FileUtils.mkdir_p(path)
-      end
       sha = Digest::SHA256.new
-      File.open(diskfile, "wb") do |f|
+      Attachment.create_diskfile(filename, disk_directory) do |f|
+        self.disk_filename = File.basename f.path
+        logger.info("Saving attachment '#{self.diskfile}' (#{@temp_file.size} bytes)") if logger
         if @temp_file.respond_to?(:read)
           buffer = ""
           while (buffer = @temp_file.read(8192))
@@ -557,9 +553,8 @@ class Attachment < ActiveRecord::Base
 
   # Singleton class method is public
   class << self
-    # Returns an ASCII or hashed filename that do not
-    # exists yet in the given subdirectory
-    def disk_filename(filename, directory=nil)
+    # Claims a unique ASCII or hashed filename, yields the open file handle
+    def create_diskfile(filename, directory=nil, &block)
       timestamp = DateTime.now.strftime("%y%m%d%H%M%S")
       ascii = ''
       if %r{^[a-zA-Z0-9_\.\-]*$}.match?(filename) && filename.length <= 50
@@ -569,11 +564,21 @@ class Attachment < ActiveRecord::Base
         # keep the extension if any
         ascii << $1 if filename =~ %r{(\.[a-zA-Z0-9]+)$}
       end
-      while File.exist?(File.join(storage_path, directory.to_s,
-                                  "#{timestamp}_#{ascii}"))
+
+      path = File.join storage_path, directory.to_s
+      FileUtils.mkdir_p(path) unless File.directory?(path)
+      begin
+        name = "#{timestamp}_#{ascii}"
+        File.open(
+          File.join(path, name),
+          flags: File::CREAT | File::EXCL | File::BINARY | File::WRONLY,
+          binmode: true,
+          &block
+        )
+      rescue Errno::EEXIST
         timestamp.succ!
+        retry
       end
-      "#{timestamp}_#{ascii}"
     end
   end
 end
diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb
index 9484c9360..2aee87964 100644
--- a/test/unit/attachment_test.rb
+++ b/test/unit/attachment_test.rb
@@ -275,12 +275,28 @@ class AttachmentTest < ActiveSupport::TestCase
     assert_equal 'valid_[] invalid_chars', a.filename
   end
 
-  def test_diskfilename
-    assert Attachment.disk_filename("test_file.txt") =~ /^\d{12}_test_file.txt$/
-    assert_equal 'test_file.txt', Attachment.disk_filename("test_file.txt")[13..-1]
-    assert_equal '770c509475505f37c2b8fb6030434d6b.txt', Attachment.disk_filename("test_accentué.txt")[13..-1]
-    assert_equal 'f8139524ebb8f32e51976982cd20a85d', Attachment.disk_filename("test_accentué")[13..-1]
-    assert_equal 'cbb5b0f30978ba03731d61f9f6d10011', Attachment.disk_filename("test_accentué.ça")[13..-1]
+  def test_create_diskfile
+    Attachment.create_diskfile("test_file.txt") do |f|
+      path = f.path
+      assert_match(/^\d{12}_test_file.txt$/, File.basename(path))
+      assert_equal 'test_file.txt', File.basename(path)[13..-1]
+      File.unlink f.path
+    end
+
+    Attachment.create_diskfile("test_accentué.txt") do |f|
+      assert_equal '770c509475505f37c2b8fb6030434d6b.txt', File.basename(f.path)[13..-1]
+      File.unlink f.path
+    end
+
+    Attachment.create_diskfile("test_accentué") do |f|
+      assert_equal 'f8139524ebb8f32e51976982cd20a85d', File.basename(f.path)[13..-1]
+      File.unlink f.path
+    end
+
+    Attachment.create_diskfile("test_accentué.ça") do |f|
+      assert_equal 'cbb5b0f30978ba03731d61f9f6d10011', File.basename(f.path)[13..-1]
+      File.unlink f.path
+    end
   end
 
   def test_title
-- 
2.20.1