Merge pull request #1917 from basecamp/dont-resize-svg-avatars

Don't try to resize non-variable avatars

Author: kevinmcconnell

app/controllers/users/avatars_controller.rb

@@ -10,7 +10,7 @@ def show
     if @user.system?
       redirect_to view_context.image_path("system_user.png")
     elsif @user.avatar.attached?
-      redirect_to rails_blob_url(@user.avatar.variant(:thumb), disposition: "inline")
+      redirect_to rails_blob_url(@user.avatar_thumbnail, disposition: "inline")
     elsif stale? @user, cache_control: cache_control
       render_initials
     end

app/models/user.rb

@@ -1,12 +1,8 @@
 class User < ApplicationRecord
-  include Accessor, Assignee, Attachable, Configurable, EmailAddressChangeable,
+  include Accessor, Assignee, Attachable, Avatar, Configurable, EmailAddressChangeable,
     Mentionable, Named, Notifiable, Role, Searcher, Watcher
   include Timelined # Depends on Accessor
 
-  has_one_attached :avatar do |attachable|
-    attachable.variant :thumb, resize_to_fill: [ 256, 256 ]
-  end
-
   belongs_to :account
   belongs_to :identity, optional: true
 

app/models/user/avatar.rb

@@ -0,0 +1,24 @@
+module User::Avatar
+  extend ActiveSupport::Concern
+
+  ALLOWED_AVATAR_CONTENT_TYPES = %w[ image/jpeg image/png image/gif image/webp ].freeze
+
+  included do
+    has_one_attached :avatar do |attachable|
+      attachable.variant :thumb, resize_to_fill: [ 256, 256 ]
+    end
+
+    validate :avatar_content_type_allowed
+  end
+
+  def avatar_thumbnail
+    avatar.variable? ? avatar.variant(:thumb) : avatar
+  end
+
+  private
+    def avatar_content_type_allowed
+      if avatar.attached? && !ALLOWED_AVATAR_CONTENT_TYPES.include?(avatar.content_type)
+        errors.add(:avatar, "must be a JPEG, PNG, GIF, or WebP image")
+      end
+    end
+end

app/views/users/edit.html.erb

@@ -14,7 +14,7 @@
 
         <label class="avatar btn btn--circle input--file txt-xx-large center fill-white">
           <%= image_tag user_avatar_path(@user), aria: { hidden: "true" }, class: "avatar", size: 128, data: { upload_preview_target: "image" } %>
-          <%= form.file_field :avatar, id: "file", class: "input", accept: "image/*",
+          <%= form.file_field :avatar, id: "file", class: "input", accept: "image/jpeg, image/png, image/gif, image/webp",
                 data: { upload_preview_target: "input", action: "upload-preview#previewImage" } %>
           <span class="for-screen-reader">Profile avatar for <%= @user.name %></span>
         </label>

test/controllers/users/avatars_controller_test.rb

@@ -32,7 +32,7 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest
 
     get user_avatar_path(users(:david))
 
-    assert_redirected_to rails_blob_url(users(:david).avatar.variant(:thumb), disposition: "inline")
+    assert_redirected_to rails_blob_url(users(:david).avatar_thumbnail, disposition: "inline")
   end
 
   test "show other image redirects to the blob url" do
@@ -41,7 +41,7 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest
 
     get user_avatar_path(users(:kevin))
 
-    assert_redirected_to rails_blob_url(users(:kevin).avatar.variant(:thumb), disposition: "inline")
+    assert_redirected_to rails_blob_url(users(:kevin).avatar_thumbnail, disposition: "inline")
   end
 
   test "delete self" do

test/fixtures/files/avatar.svg

@@ -0,0 +1,30 @@
+require "test_helper"
+
+class User::AvatarTest < ActiveSupport::TestCase
+  test "avatar_thumbnail returns variant for variable images" do
+    users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg")
+
+    assert users(:david).avatar.variable?
+    assert_equal users(:david).avatar.variant(:thumb).blob, users(:david).avatar_thumbnail.blob
+  end
+
+  test "avatar_thumbnail returns original blob for non-variable images" do
+    users(:david).avatar.attach(io: File.open(file_fixture("avatar.svg")), filename: "avatar.svg", content_type: "image/svg+xml")
+
+    assert_not users(:david).avatar.variable?
+    assert_equal users(:david).avatar.blob, users(:david).avatar_thumbnail.blob
+  end
+
+  test "allows valid image content types" do
+    users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg")
+
+    assert users(:david).valid?
+  end
+
+  test "rejects SVG uploads" do
+    users(:david).avatar.attach(io: File.open(file_fixture("avatar.svg")), filename: "avatar.svg")
+
+    assert_not users(:david).valid?
+    assert_includes users(:david).errors[:avatar], "must be a JPEG, PNG, GIF, or WebP image"
+  end
+end