From 7f178484c734c1d962026bb3cfd81223f1551250 Mon Sep 17 00:00:00 2001
From: Dominik Klein <dk@zammad.com>
Date: Wed, 22 Sep 2021 13:19:28 +0200
Subject: [PATCH] Maintenance: Refactoring of Avatar storage logic.

---
 app/controllers/users_controller.rb | 33 ++++++++++++------
 app/models/avatar.rb                |  1 -
 spec/requests/user_spec.rb          | 54 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index d69bea299..6dde9b568 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -722,31 +722,28 @@ curl http://localhost/api/v1/users/image/8d6cca1c6bdc226cf2ba131e264ca2c7 -v -u
 =end
 
   def image
-
     # cache image
     response.headers['Expires']       = 1.year.from_now.httpdate
     response.headers['Cache-Control'] = 'cache, store, max-age=31536000, must-revalidate'
     response.headers['Pragma']        = 'cache'
 
     file = Avatar.get_by_hash(params[:hash])
+
     if file
+      file_content_type = file.preferences['Content-Type'] || file.preferences['Mime-Type']
+
+      return serve_default_image if ActiveStorage.content_types_allowed_inline.exclude?(file_content_type)
+
       send_data(
         file.content,
         filename:    file.filename,
-        type:        file.preferences['Content-Type'] || file.preferences['Mime-Type'],
+        type:        file_content_type,
         disposition: 'inline'
       )
       return
     end
 
-    # serve default image
-    image = 'R0lGODdhMAAwAOMAAMzMzJaWlr6+vqqqqqOjo8XFxbe3t7GxsZycnAAAAAAAAAAAAAAAAAAAAAAAAAAAACwAAAAAMAAwAAAEcxDISau9OOvNu/9gKI5kaZ5oqq5s675wLM90bd94ru98TwuAA+KQAQqJK8EAgBAgMEqmkzUgBIeSwWGZtR5XhSqAULACCoGCJGwlm1MGQrq9RqgB8fm4ZTUgDBIEcRR9fz6HiImKi4yNjo+QkZKTlJWWkBEAOw=='
-    send_data(
-      Base64.decode64(image),
-      filename:    'image.gif',
-      type:        'image/gif',
-      disposition: 'inline'
-    )
+    serve_default_image
   end
 
 =begin
@@ -778,6 +775,11 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content
       return
     end
 
+    if ActiveStorage::Variant::WEB_IMAGE_CONTENT_TYPES.exclude?(file_full[:mime_type])
+      render json: { error: 'Mime type is invalid' }, status: :unprocessable_entity
+      return
+    end
+
     begin
       file_resize = StaticAssets.data_url_attributes(params[:avatar_resize])
     rescue
@@ -1061,4 +1063,15 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content
 
     render json: { message: 'ok' }, status: :created
   end
+
+  def serve_default_image
+    image = 'R0lGODdhMAAwAOMAAMzMzJaWlr6+vqqqqqOjo8XFxbe3t7GxsZycnAAAAAAAAAAAAAAAAAAAAAAAAAAAACwAAAAAMAAwAAAEcxDISau9OOvNu/9gKI5kaZ5oqq5s675wLM90bd94ru98TwuAA+KQAQqJK8EAgBAgMEqmkzUgBIeSwWGZtR5XhSqAULACCoGCJGwlm1MGQrq9RqgB8fm4ZTUgDBIEcRR9fz6HiImKi4yNjo+QkZKTlJWWkBEAOw=='
+
+    send_data(
+      Base64.decode64(image),
+      filename:    'image.gif',
+      type:        'image/gif',
+      disposition: 'inline'
+    )
+  end
 end
diff --git a/app/models/avatar.rb b/app/models/avatar.rb
index 0c7858ca5..5aa8be1f9 100644
--- a/app/models/avatar.rb
+++ b/app/models/avatar.rb
@@ -72,7 +72,6 @@ add avatar by url
 =end
 
   def self.add(data)
-
     # lookups
     if data[:object]
       object_id = ObjectLookup.by_name(data[:object])
diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb
index 3d807086c..964d3b4f7 100644
--- a/spec/requests/user_spec.rb
+++ b/spec/requests/user_spec.rb
@@ -1517,6 +1517,60 @@ RSpec.describe 'User', type: :request do
       expect { make_request(avatar_full: base64, avatar_resize: base64) }
         .to change { Avatar.list('User', user.id) }
     end
+
+    context 'with a not allowed mime-type' do
+      let(:base64) { 'data:image/svg+xml;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==' }
+
+      it 'returns verbose error for a not allowed mime-type' do
+        make_request(avatar_full: base64)
+        expect(json_response).to include('error' => 'Mime type is invalid')
+      end
+    end
+  end
+
+  describe 'GET /api/v1/users/image/:hash', authenticated_as: :user do
+    let(:user) { create(:user) }
+    let(:avatar_mime_type) { 'image/png' }
+    let(:avatar) do
+      file = File.open('test/data/image/1000x1000.png', 'rb')
+      contents = file.read
+      Avatar.add(
+        object:        'User',
+        o_id:          user.id,
+        default:       true,
+        resize:        {
+          content:   contents,
+          mime_type: avatar_mime_type,
+        },
+        source:        'web',
+        deletable:     true,
+        updated_by_id: 1,
+        created_by_id: 1,
+      )
+    end
+    let(:avatar_content) { Avatar.get_by_hash(avatar.store_hash).content }
+
+    before do
+      user.update!(image: avatar.store_hash)
+    end
+
+    def make_request(image_hash, params: {})
+      get "/api/v1/users/image/#{image_hash}", params: params, as: :json
+    end
+
+    it 'returns verbose error when full image is missing' do
+      make_request(avatar.store_hash)
+      expect(response.body).to eq(avatar_content)
+    end
+
+    context 'with a not allowed inline mime-type' do
+      let(:avatar_mime_type) { 'image/svg+xml' }
+
+      it 'returns the default image' do
+        make_request(avatar.store_hash)
+        expect(response.headers['Content-Type']).to include('image/gif')
+      end
+    end
   end
 
   describe 'GET /api/v1/users/search, checks usage of the ids parameter', authenticated_as: :agent do