Properly build uploaded tempfile with basename

lib/open_project/files.rb

@@ -30,43 +30,29 @@ module OpenProject
   module Files
     module_function
 
-    ##
-    # Creates a temp file with the given file name.
-    # It will reside in some temporary directory.
-    def create_temp_file(name: "test.txt", content: "test content", binary: false)
-      tmp = Tempfile.new name
-      path = Pathname(tmp)
-
-      tmp.delete # delete temp file
-      path.mkdir # create temp directory
-
-      file_path = path.join name
-      File.open(file_path, "w" + (binary ? "b" : "")) do |f|
-        f.write content
-      end
-
-      File.new file_path
-    end
-
     def build_uploaded_file(tempfile, type, binary: true, file_name: nil)
-      uploaded_file = Rack::Multipart::UploadedFile.new tempfile.path,
-                                                        type,
-                                                        binary
-      if file_name
-        # I wish I could set the file name in a better way *sigh*
-        uploaded_file.instance_variable_set(:@original_filename, file_name)
-      end
-
-      uploaded_file
+      Rack::Multipart::UploadedFile.new tempfile.path,
+                                        type,
+                                        binary,
+                                        filename: file_name
     end
 
-    def create_uploaded_file(name: "test.txt",
-                             content_type: "text/plain",
-                             content: "test content",
-                             binary: false)
+    def create_uploaded_file(name:, content_type:, content:, binary: false)
+      create_temp_file(name:, content:, binary:) do |f|
+        build_uploaded_file f, content_type, binary:, file_name: File.basename(name)
+      end
+    end
 
-      tmp = create_temp_file(name:, content:, binary:)
-      build_uploaded_file tmp, content_type, binary:
+    def create_temp_file(name:, content:, binary: false, &)
+      basename = name
+      Tempfile.create(basename) do |f|
+        f.binmode if binary
+
+        f.write content
+        f.rewind
+
+        yield f
+      end
     end
   end
 end

spec/lib/open_project/files_spec.rb

@@ -59,14 +59,14 @@
 require "spec_helper"
 
 RSpec.describe OpenProject::Files do
-  describe "build_uploaded_file" do
+  describe ".build_uploaded_file" do
     let(:original_filename) { "test.png" }
     let(:content_type) { "image/png" }
     let(:file) do
-      OpenProject::Files.create_temp_file(name: original_filename)
+      Tempfile.new(File.basename(original_filename))
     end
 
-    subject { OpenProject::Files.build_uploaded_file(file, content_type) }
+    subject { described_class.build_uploaded_file(file, content_type, file_name: original_filename) }
 
     it "has the original file name" do
       expect(subject.original_filename).to eql(original_filename)
@@ -79,7 +79,7 @@ RSpec.describe OpenProject::Files do
     context "with custom file name" do
       let(:file_name) { "my-custom-filename.png" }
 
-      subject { OpenProject::Files.build_uploaded_file(file, content_type, file_name:) }
+      subject { described_class.build_uploaded_file(file, content_type, file_name:) }
 
       it "has the custom file name" do
         expect(subject.original_filename).to eql(file_name)
@@ -87,39 +87,16 @@ RSpec.describe OpenProject::Files do
     end
   end
 
-  describe "create_uploaded_file" do
-    context "without parameters" do
-      let(:file) { OpenProject::Files.create_uploaded_file }
-
-      it 'creates a file with the default name "test.txt"' do
-        expect(file.original_filename).to eq "test.txt"
-      end
-
-      it "creates distinct files even with identical names" do
-        file_2 = OpenProject::Files.create_uploaded_file
-
-        expect(file.original_filename).to eq file_2.original_filename
-        expect(file.path).not_to eq file_2.path
-      end
-
-      it 'writes some default content "test content"' do
-        expect(file.read).to eq "test content"
-      end
-
-      it 'set default content type "text/plain"' do
-        expect(file.content_type).to eq "text/plain"
-      end
-    end
-
+  describe ".create_uploaded_file" do
     context "with a custom name, content and content type" do
       let(:name)         { "foo.jpg" }
       let(:content)      { "not-really-a-jpg" }
       let(:content_type) { "image/jpeg" }
 
       let(:file) do
-        OpenProject::Files.create_uploaded_file name:,
-                                                content:,
-                                                content_type:
+        described_class.create_uploaded_file name:,
+                                             content:,
+                                             content_type:
       end
 
       it 'creates a file called "foo.jpg"' do
@@ -138,7 +115,14 @@ RSpec.describe OpenProject::Files do
     context "with binary content" do
       let(:content) { "\xD1\x9B\x86".b }
       let(:binary)  { false }
-      let(:file)    { OpenProject::Files.create_uploaded_file content:, binary: }
+      let(:file)    do
+        described_class.create_uploaded_file(
+          name: "binary_file",
+          content_type: "application/octet-stream",
+          content: content,
+          binary: binary
+        )
+      end
 
       it "fails when the content is not marked as binary" do
         expect { file }.to raise_error(Encoding::UndefinedConversionError)
@@ -152,5 +136,51 @@ RSpec.describe OpenProject::Files do
         end
       end
     end
+
+    context "when a relative filename is provided" do
+      let(:name) { "../../hello/../../../foo.txt" }
+      let(:file) do
+        described_class.create_uploaded_file name:,
+                                             content: "content",
+                                             content_type: "text/plain"
+      end
+
+      it "sanitizes the file name" do
+        expect(file.original_filename).to eq "foo.txt"
+      end
+    end
+  end
+
+  describe ".create_temp_file" do
+    let(:name) { "tempfile.txt" }
+    let(:content) { "temporary content" }
+    let(:binary) { false }
+
+    subject do
+      described_class.create_temp_file(name:, content:, binary:, &:read)
+    end
+
+    it "yields a file with the given content" do
+      expect(subject).to eq content
+    end
+
+    context "with binary content" do
+      let(:content) { "\xD1\x9B\x86".b }
+      let(:binary)  { true }
+
+      it "yields the binary content" do
+        expect(subject).to eq content
+      end
+    end
+
+    context "when a relative filename is provided" do
+      let(:name) { "../../tempfile.txt" }
+
+      it "sanitizes the file name" do
+        described_class.create_temp_file(name:, content:, binary:) do |f|
+          expect(f.path).to eq(File.expand_path(f.path))
+        end
+      end
+    end
   end
 end

spec/support/file_helpers.rb

@@ -35,8 +35,10 @@ module FileHelpers
                          content_type: "text/plain",
                          content: "test content",
                          binary: false)
+    ::OpenProject::Files.create_uploaded_file(name:, content_type:, content:, binary:)
 
-    tmp = ::OpenProject::Files.create_temp_file(name:, content:, binary:)
-    Rack::Test::UploadedFile.new tmp.path, content_type, binary
+    ::OpenProject::Files.create_temp_file(name:, content:, binary:) do |tmp|
+      Rack::Test::UploadedFile.new tmp.path, content_type, binary, original_filename: File.basename(name)
+    end
   end
 end