Ensure uploaded images are image files

app/controllers/catalog_controller.rb

@@ -3,6 +3,7 @@ class CatalogController < ApplicationController
   include Mixins::ServiceDialogCreationMixin
   include Mixins::BreadcrumbsMixin
   include Mixins::AutomationMixin
+  include Mixins::ImageValidationMixin
 
   before_action :check_privileges
   before_action :get_session_data
@@ -533,31 +534,28 @@ def st_upload_image
 
     err = false
     identify_catalog(params[:id])
+    image_file = params.dig(:upload, :image)
     if params[:pressed]
       @record.picture = nil
       @record.save
       msg = _("Custom Image successfully removed")
-    elsif params[:upload] && params[:upload][:image] &&
-          params[:upload][:image].respond_to?(:read)
-      ext = params[:upload][:image].original_filename.split(".").last.downcase
-      if !%w[png jpg].include?(ext)
-        msg = _("Custom Image must be a .png or .jpg file")
-        err = true
-      else
-        picture = {:content   => params[:upload][:image].read,
-                   :extension => ext}
-        if @record.picture.nil?
-          @record.picture = Picture.new(picture)
-        else
-          @record.picture.update(picture)
-        end
-        @record.save
-        msg = _("Custom Image file \"%{name}\" successfully uploaded") %
-              {:name => params[:upload][:image].original_filename}
-      end
-    else
+    elsif !image_file&.respond_to?(:read)
       msg = _("Use the Choose file button to locate a .png or .jpg image file")
       err = true
+    elsif !valid_image_file?(image_file)
+      msg = _("Custom Image must be a .png or .jpg file")
+      err = true
+    else
+      ext = image_file.original_filename.split(".").last.downcase
+      picture = {:content => image_file.read, :extension => ext}
+      if @record.picture.nil?
+        @record.picture = Picture.new(picture)
+      else
+        @record.picture.update(picture)
+      end
+      @record.save
+      msg = _("Custom Image file \"%{name}\" successfully uploaded") %
+            {:name => image_file.original_filename}
     end
     params[:id] = x_build_node_id(@record) # Get the tree node id
     add_flash(msg, err == true ? :error : nil)

app/controllers/mixins/image_validation_mixin.rb

@@ -0,0 +1,32 @@
+module Mixins
+  module ImageValidationMixin
+    private
+
+    # @param file request parameter for a file
+    # @param ext if present, the only extension supported (default: nil / accept all extensions)
+    def valid_image_file?(file, type = nil)
+      ext = file.original_filename.split(".").last.downcase
+      return false if type && !Array.wrap(type).include?(ext)
+
+      valid_magic_number =
+        case ext
+        when "ico"
+          "\x00\x00\x01\x00".force_encoding("ASCII-8BIT")
+        when "png"
+          "\x89PNG\r\n\x1A\n".force_encoding("ASCII-8BIT")
+        when "jpg"
+          "\xff\xd8\xff".force_encoding("ASCII-8BIT")
+        else
+          return false
+        end
+
+      magic_number = File.open(file.tempfile.path, 'rb') do |f|
+        f.readpartial(valid_magic_number.size)
+      end
+
+      magic_number == valid_magic_number
+    rescue EOFError
+      return false
+    end      
+  end
+end

app/controllers/ops_controller/settings/upload.rb

@@ -1,5 +1,6 @@
 module OpsController::Settings::Upload
   extend ActiveSupport::Concern
+  include Mixins::ImageValidationMixin
 
   def upload_logo
     assert_privileges("ops_settings")
@@ -31,7 +32,7 @@ def upload_favicon
 
   def upload_logos(file, field, text, type)
     if field && field[:logo] && field[:logo].respond_to?(:read)
-      unless valid_file?(field[:logo], type)
+      unless valid_image_file?(field[:logo], type)
         add_flash(_("%{image} must be a .%{type} file") % {:image => text, :type => type}, :error)
       else
         File.open(file, "wb") { |f| f.write(field[:logo].read) }
@@ -115,28 +116,4 @@ def logo_dir
     Dir.mkdir(dir) unless dir.exist?
     dir.to_s
   end
-
-  def valid_file?(file, type)
-    ext = file.original_filename.split(".").last.downcase
-    return false unless ext == type
-
-    case type
-    when "ico"
-      valid_magic_number = "\x00\x00\x01\x00".force_encoding("ASCII-8BIT")
-    when "png"
-      valid_magic_number = "\x89PNG\r\n\x1A\n".force_encoding("ASCII-8BIT")
-    else
-      return false
-    end
-
-    magic_number = File.open(file.tempfile.path, 'rb') do |f|
-      begin
-        f.readpartial(valid_magic_number.size)
-      rescue EOFError
-        return false
-      end
-    end
-
-    magic_number == valid_magic_number
-  end
 end

spec/controllers/catalog_controller_spec.rb

@@ -353,13 +353,19 @@
       end
 
       it "uploads a selected png file " do
-        file = fixture_file_upload('files/upload_image.png', 'image/png')
+        file = fixture_file_upload('spec/fixtures/image.png', 'image/png')
         post :st_upload_image, :params => { :format => :js, :id => @st.id, :upload => {:image => file}, :active_tree => :sandt_tree, :commit => 'Upload' }
-        expect(assigns(:flash_array).first[:message]).to include('Custom Image file "upload_image.png" successfully uploaded')
+        expect(assigns(:flash_array).first[:message]).to include('Custom Image file "image.png" successfully uploaded')
+      end
+
+      it "uploads a selected jpg file " do
+        file = fixture_file_upload('spec/fixtures/image.jpg', 'image/jpeg')
+        post :st_upload_image, :params => { :format => :js, :id => @st.id, :upload => {:image => file}, :active_tree => :sandt_tree, :commit => 'Upload' }
+        expect(assigns(:flash_array).first[:message]).to include('Custom Image file "image.jpg" successfully uploaded')
       end
 
       it "displays an error when the selected file is not a png file or .jpg " do
-        file = fixture_file_upload('files/upload_image.txt', 'image/png')
+        file = fixture_file_upload('spec/fixtures/test.txt.png', 'image/png')
         post :st_upload_image, :params => { :format => :js, :id => @st.id, :upload => {:image => file}, :commit => 'Upload' }
         expect(assigns(:flash_array).first[:message]).to include("Custom Image must be a .png or .jpg file")
       end

spec/controllers/ops_controller/settings/upload_spec.rb

@@ -11,58 +11,49 @@
     end
 
     it 'adds success flash message to @flash_array regarding updating custom logo image with a .png file' do
-      file = 'app/assets/images/layout/login-screen-logo.png'
-      controller.params = {:upload => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'login-screen-logo.png', :type => 'image/png')}}
+      controller.params = {:upload => {:logo => fixture_file_upload('spec/fixtures/image.png', 'image/png')}}
       controller.send(:upload_logo)
-      expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom logo image "login-screen-logo.png" uploaded', :level => :success)
+      expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom logo image "image.png" uploaded', :level => :success)
     end
 
     it 'adds error flash message to @flash_array regarding updating custom logo image with a non-png file' do
-      file = File.join(__dir__, '/data/test.txt.png')
-      controller.params = {:upload => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'test.txt.png', :type => 'image/png')}}
+      controller.params = {:upload => {:logo => fixture_file_upload('spec/fixtures/test.txt.png', 'image/png')}}
       controller.send(:upload_logo)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom logo image must be a .png file', :level => :error)
 
-      file = 'app/assets/images/favicon.ico'
-      controller.params = {:upload => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'favicon.ico', :type => 'image/png')}}
+      controller.params = {:upload => {:logo => fixture_file_upload('spec/fixtures/favicon.ico', 'image/png')}}
       controller.send(:upload_logo)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom logo image must be a .png file', :level => :error)
     end
 
     it 'adds success flash message to @flash_array regarding updating custom login and about screen background image with a .png file' do
-      file = 'app/assets/images/layout/login-screen-logo.png'
-      controller.params = {:login => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'login-screen-logo.png', :type => 'image/png')}}
+      controller.params = {:login => {:logo => fixture_file_upload('spec/fixtures/image.png', 'image/png')}}
       controller.send(:upload_login_logo)
-      expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom login image "login-screen-logo.png" uploaded', :level => :success)
+      expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom login image "image.png" uploaded', :level => :success)
     end
 
     it 'adds error flash message to @flash_array regarding updating custom login and about screen background image with a non .png file' do
-      file = File.join(__dir__, '/data/test.txt.png')
-      controller.params = {:login => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'test.txt.png', :type => 'image/png')}}
+      controller.params = {:login => {:logo => fixture_file_upload('spec/fixtures/test.txt.png', 'image/png')}}
       controller.send(:upload_login_logo)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom login image must be a .png file', :level => :error)
 
-      file = 'app/assets/images/favicon.ico'
-      controller.params = {:login => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'favicon.ico', :type => 'image/png')}}
+      controller.params = {:login => {:logo => fixture_file_upload('spec/fixtures/favicon.ico', 'image/png')}}
       controller.send(:upload_login_logo)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom login image must be a .png file', :level => :error)
     end
 
     it 'adds success flash message to @flash_array regarding updating custom brand image with a .png file' do
-      file = 'app/assets/images/layout/login-screen-logo.png'
-      controller.params = {:brand => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'login-screen-logo.png', :type => 'image/png')}}
+      controller.params = {:brand => {:logo => fixture_file_upload('spec/fixtures/image.png', 'image/png')}}
       controller.send(:upload_login_brand)
-      expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom brand "login-screen-logo.png" uploaded', :level => :success)
+      expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom brand "image.png" uploaded', :level => :success)
     end
 
     it 'adds error flash message to @flash_array regarding updating custom brand image with a non .png file' do
-      file = File.join(__dir__, '/data/test.txt.png')
-      controller.params = {:brand => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'test.txt.png', :type => 'image/png')}}
+      controller.params = {:brand => {:logo => fixture_file_upload('spec/fixtures/test.txt.png', 'image/png')}}
       controller.send(:upload_login_brand)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom brand must be a .png file', :level => :error)
 
-      file = 'app/assets/images/favicon.ico'
-      controller.params = {:brand => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'favicon.ico', :type => 'image/png')}}
+      controller.params = {:brand => {:logo => fixture_file_upload('spec/fixtures/favicon.ico', 'image/png')}}
       controller.send(:upload_login_brand)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom brand must be a .png file', :level => :error)
     end
@@ -76,20 +67,17 @@
     end
 
     it 'adds success flash message to @flash_array regarding updating custom favicon with an .ico file' do
-      file = 'app/assets/images/favicon.ico'
-      controller.params = {:favicon => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'favicon.ico', :type => 'image/vnd.microsoft.icon')}}
+      controller.params = {:favicon => {:logo => fixture_file_upload('spec/fixtures/favicon.ico', 'image/vnd.microsoft.icon')}}
       controller.send(:upload_favicon)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom favicon "favicon.ico" uploaded', :level => :success)
     end
 
     it 'adds error flash message to @flash_array regarding updating custom favicon with a non .ico file' do
-      file = File.join(__dir__, '/data/test.txt.ico')
-      controller.params = {:favicon => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'test.txt.ico', :type => 'image/png')}}
+      controller.params = {:favicon => {:logo => fixture_file_upload('spec/fixtures/test.txt.ico', 'image/vnd.microsoft.icon')}}
       controller.send(:upload_favicon)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom favicon must be a .ico file', :level => :error)
 
-      file = 'app/assets/images/layout/login-screen-logo.png'
-      controller.params = {:favicon => {:logo => ActionDispatch::Http::UploadedFile.new(:tempfile => File.open(file), :filename => 'test.ico', :type => 'image/vnd.microsoft.icon')}}
+      controller.params = {:favicon => {:logo => fixture_file_upload('spec/fixtures/favicon.png.ico', 'image/vnd.microsoft.icon')}}
       controller.send(:upload_favicon)
       expect(controller.instance_variable_get(:@flash_array)).to include(:message => 'Custom favicon must be a .ico file', :level => :error)
     end