Restrict avatar uploads to standard image formats

app/commands/user/retrieve_avatar.rb

@@ -0,0 +1,47 @@
+require 'open-uri'
+
+# Returns the bytes and content type of a user's avatar, for AvatarsController.
+# Only the image formats in User::AVATAR_CONTENT_TYPES are returned. Anything
+# else falls back to the placeholder.
+class User::RetrieveAvatar
+  include Mandate
+
+  initialize_with :user
+
+  def self.placeholder_url = "#{Exercism.config.website_icons_host}/placeholders/user-avatar.svg"
+
+  def call
+    return attached_avatar if attached_avatar_allowed?
+    return external_avatar if external_avatar_url.present?
+
+    placeholder
+  end
+
+  private
+  def attached_avatar_allowed?
+    user.avatar.attached? && allowed_content_type?(user.avatar.content_type)
+  end
+
+  def attached_avatar = [user.avatar.download, user.avatar.content_type]
+
+  def external_avatar
+    data, content_type = fetch(external_avatar_url)
+    return placeholder unless allowed_content_type?(content_type)
+
+    [data, content_type]
+  end
+
+  # The placeholder is our own SVG, so we allow it by URL.
+  def placeholder = fetch(self.class.placeholder_url)
+
+  def fetch(url)
+    file = URI.parse(url).open
+    [file.read, file.content_type]
+  end
+
+  def allowed_content_type?(content_type) = User::AVATAR_CONTENT_TYPES.include?(content_type)
+
+  # User#avatar_url is overridden to point at our own avatars host, so the
+  # external URL has to come off the attributes directly.
+  def external_avatar_url = user.attributes['avatar_url']
+end

app/controllers/api/users_controller.rb

@@ -9,8 +9,14 @@ def show
   end
 
   def update
-    if params.dig(:user, :avatar).present? # rubocop:disable Style/IfUnlessModifier:
-      User::UpdateAvatar.(current_user, params.require(:user)[:avatar])
+    if params.dig(:user, :avatar).present?
+      begin
+        User::UpdateAvatar.(current_user, params.require(:user)[:avatar])
+      rescue ActiveRecord::RecordInvalid => e
+        raise unless e.record.errors.include?(:avatar)
+
+        return render_400(:invalid_avatar_format, errors: e.record.errors)
+      end
     end
 
     if params.dig(:user, :seniority).present?

app/controllers/avatars_controller.rb

@@ -2,6 +2,8 @@
 
 # This should inherit from ActionController::Base, not ApplicationController
 class AvatarsController < ActionController::Base # rubocop:disable Rails/ApplicationController
+  before_action :set_nosniff_header
+
   def show
     # Save a load of testing headaches
     if Rails.env.test?
@@ -13,29 +15,24 @@ def show
     # We don't want future requests to be cached before they should be!
     raise ActiveRecord::RecordNotFound if params[:version].to_i > user.version
 
-    if user.avatar.attached?
-      data = user.avatar.download
-      content_type = user.avatar.content_type
-    else
-      url = user.attributes['avatar_url'].presence || "#{Exercism.config.website_icons_host}/placeholders/user-avatar.svg"
-
-      file = URI.parse(url).open
-      data = file.read
-      content_type = file.content_type
-    end
+    data, content_type = User::RetrieveAvatar.(user)
 
     expires_in 5.years, public: true
     response.set_header("Content-Type", content_type)
     send_data data, type: content_type, disposition: 'inline'
   rescue ActiveRecord::RecordNotFound
     head :not_found
   rescue OpenURI::HTTPError
-    url = "#{Exercism.config.website_icons_host}/placeholders/user-avatar.svg"
-    file = URI.parse(url).open
+    file = URI.parse(User::RetrieveAvatar.placeholder_url).open
     expires_in 5.minutes, public: true
     send_data file.read, type: file.content_type, disposition: 'inline'
   rescue StandardError => e
     Sentry.capture_exception(e)
     head :internal_server_error
   end
+
+  private
+  def set_nosniff_header
+    response.set_header("X-Content-Type-Options", "nosniff")
+  end
 end

app/models/user.rb

@@ -6,6 +6,9 @@ class User < ApplicationRecord
   IHID_USER_ID = 1530
   MIN_REP_TO_MENTOR = 20
 
+  # The image formats we accept for uploaded avatars.
+  AVATAR_CONTENT_TYPES = %w[image/png image/jpeg image/gif image/webp].freeze
+
   enum flair: {
     founder: 0,
     staff: 1,
@@ -155,6 +158,7 @@ class User < ApplicationRecord
   validates :email, length: { maximum: 190 }
   validates :pronouns, length: { maximum: 255 }
   validates :location, length: { maximum: 255 }
+  validate :avatar_content_type_must_be_allowed
 
   has_one_attached :avatar do |attachable|
     # convert: :png because resize_to_fill on its own preserves the source
@@ -389,4 +393,15 @@ def watched_video?(video_provider, video_id)
   end
 
   def on_jiki_waiting_list? = jiki_signup.exists?
+
+  private
+  # Only check a newly attached avatar, so that existing users with an avatar
+  # in another format can still save the rest of their record.
+  def avatar_content_type_must_be_allowed
+    return unless attachment_changes.key?('avatar')
+    return unless avatar.attached?
+    return if AVATAR_CONTENT_TYPES.include?(avatar.content_type)
+
+    errors.add(:avatar, "must be a PNG, JPEG, GIF or WebP image")
+  end
 end

config/locales/api/en.yml

@@ -57,6 +57,7 @@ en:
       incorrect_password: "Incorrect password"
       passwords_dont_match: "Passwords don't match"
       invalid_locale: "That is not a language we serve"
+      invalid_avatar_format: "Your profile picture must be a PNG, JPEG, GIF or WebP image"
       no_iterations_submitted_yet: "You haven't submitted an iteration yet."
       no_mentoring_slots_available: "You have no mentoring slots available"
       profile_criteria_not_fulfilled: "You need at least 5 reputation to create a profile"

test/commands/user/retrieve_avatar_test.rb

@@ -0,0 +1,82 @@
+require "test_helper"
+
+class User::RetrieveAvatarTest < ActiveSupport::TestCase
+  SVG = '<svg xmlns="http://www.w3.org/2000/svg"></svg>'.freeze
+
+  def png_bytes = File.binread(Rails.root.join("app", "images", "favicon.png"))
+
+  test "returns an attached avatar" do
+    user = create :user
+
+    data, content_type = User::RetrieveAvatar.(user)
+
+    assert_equal "image/png", content_type
+    assert_equal png_bytes, data
+  end
+
+  test "returns the placeholder for an attached avatar in another format" do
+    user = create :user
+    attach_svg_avatar!(user)
+    stub_placeholder
+
+    data, content_type = User::RetrieveAvatar.(User.find(user.id))
+
+    assert_equal "image/svg+xml", content_type
+    assert_equal "placeholder", data
+  end
+
+  test "returns an external avatar" do
+    user = create :user, :external_avatar_url
+    stub_request(:get, user.attributes['avatar_url']).to_return(
+      body: png_bytes, headers: { 'Content-Type' => 'image/png' }
+    )
+
+    data, content_type = User::RetrieveAvatar.(user)
+
+    assert_equal "image/png", content_type
+    assert_equal png_bytes, data
+  end
+
+  test "returns the placeholder for an external avatar in another format" do
+    user = create :user, :external_avatar_url
+    stub_placeholder
+
+    %w[image/svg+xml text/html].each do |external_content_type|
+      stub_request(:get, user.attributes['avatar_url']).to_return(
+        body: SVG, headers: { 'Content-Type' => external_content_type }
+      )
+
+      data, content_type = User::RetrieveAvatar.(user)
+
+      assert_equal "image/svg+xml", content_type
+      assert_equal "placeholder", data
+    end
+  end
+
+  test "returns the placeholder when there is no avatar" do
+    user = create :user, :external_avatar_url
+    user.update_column(:avatar_url, nil)
+    stub_placeholder
+
+    data, content_type = User::RetrieveAvatar.(user.reload)
+
+    assert_equal "image/svg+xml", content_type
+    assert_equal "placeholder", data
+  end
+
+  private
+  def stub_placeholder
+    stub_request(:get, User::RetrieveAvatar.placeholder_url).to_return(
+      body: "placeholder", headers: { 'Content-Type' => 'image/svg+xml' }
+    )
+  end
+
+  # Older avatars may predate the format check, so attach one directly.
+  def attach_svg_avatar!(user)
+    user.avatar.purge
+    blob = ActiveStorage::Blob.create_and_upload!(
+      io: StringIO.new(SVG), filename: "avatar.svg", content_type: "image/svg+xml"
+    )
+    ActiveStorage::Attachment.create!(record: user, name: "avatar", blob:)
+  end
+end

test/commands/user/update_avatar_test.rb

@@ -2,11 +2,7 @@
 
 class User::UpdateAvatarTest < ActiveSupport::TestCase
   setup do
-    filename = "world-map.png"
-    tempfile = Tempfile.new([SecureRandom.uuid, '.png'])
-    tempfile.write(File.read(Rails.root.join("app", "images", filename)))
-    tempfile.rewind
-    @avatar = Rack::Test::UploadedFile.new(tempfile.path, 'image/png')
+    @avatar = upload_file("image.png", File.read(Rails.root.join("app", "images", "world-map.png")), 'image/png')
   end
 
   test "updates avatar" do
@@ -25,4 +21,47 @@ class User::UpdateAvatarTest < ActiveSupport::TestCase
 
     assert_equal 16, user.version
   end
+
+  test "accepts jpeg, gif and webp images" do
+    user = create :user
+    source = Rails.root.join("app", "images", "favicon.png")
+
+    %w[jpg gif webp].each do |format|
+      converted = ImageProcessing::Vips.source(source).convert(format).()
+      User::UpdateAvatar.(user, upload_file("image.#{format}", File.binread(converted.path), "image/#{format}"))
+
+      assert user.reload.avatar.attached?
+    end
+  end
+
+  test "rejects svg images" do
+    user = create :user, version: 15
+    svg = upload_file("image.svg", '<svg xmlns="http://www.w3.org/2000/svg" width="10" height="10"></svg>', 'image/svg+xml')
+
+    error = assert_raises ActiveRecord::RecordInvalid do
+      User::UpdateAvatar.(user, svg)
+    end
+
+    assert_includes error.record.errors[:avatar], "must be a PNG, JPEG, GIF or WebP image"
+    assert_equal "image/png", user.reload.avatar.content_type
+    assert_equal 15, user.version
+  end
+
+  test "rejects files that are not images" do
+    user = create :user
+    html = upload_file("image.html", "<html><body>Hello</body></html>", 'text/html')
+
+    assert_raises ActiveRecord::RecordInvalid do
+      User::UpdateAvatar.(user, html)
+    end
+  end
+
+  private
+  def upload_file(filename, content, content_type)
+    tempfile = Tempfile.new([SecureRandom.uuid, File.extname(filename)])
+    tempfile.binmode
+    tempfile.write(content)
+    tempfile.rewind
+    Rack::Test::UploadedFile.new(tempfile.path, content_type)
+  end
 end

test/controllers/api/users_controller_test.rb

@@ -35,6 +35,41 @@ class API::UsersControllerTest < API::BaseTestCase
     assert_response :ok
   end
 
+  test "update avatar" do
+    setup_user
+    @current_user.update!(version: 3)
+    avatar = Rack::Test::UploadedFile.new(Rails.root.join("app", "images", "world-map.png"), 'image/png')
+
+    patch api_user_path, params: { user: { avatar: } }, headers: @headers
+
+    assert_response :ok
+    assert_equal 4, @current_user.reload.version
+    assert_equal "world-map.png", @current_user.avatar.filename.to_s
+  end
+
+  test "update avatar rejects unsupported formats" do
+    setup_user
+    @current_user.update!(version: 3)
+    tempfile = Tempfile.new([SecureRandom.uuid, '.svg'])
+    tempfile.write('<svg xmlns="http://www.w3.org/2000/svg"></svg>')
+    tempfile.rewind
+    avatar = Rack::Test::UploadedFile.new(tempfile.path, 'image/svg+xml')
+
+    patch api_user_path, params: { user: { avatar: } }, headers: @headers
+
+    assert_response :bad_request
+    expected = {
+      error: {
+        type: "invalid_avatar_format",
+        message: I18n.t("api.errors.invalid_avatar_format"),
+        errors: { avatar: ["must be a PNG, JPEG, GIF or WebP image"] }
+      }
+    }
+    assert_equal expected.to_json, response.body
+    assert_equal 3, @current_user.reload.version
+    assert_equal "image/png", @current_user.avatar.content_type
+  end
+
   test "update seniority" do
     setup_user
     @current_user.update(seniority: nil)

test/controllers/avatars_controller_test.rb

@@ -0,0 +1,12 @@
+require "test_helper"
+
+class AvatarsControllerTest < ActionDispatch::IntegrationTest
+  test "serves avatars with nosniff" do
+    user = create :user
+
+    get "/avatars/#{user.id}/#{user.version}"
+
+    assert_response :ok
+    assert_equal "nosniff", response.headers["X-Content-Type-Options"]
+  end
+end

test/models/user_test.rb

@@ -621,4 +621,31 @@ class UserTest < ActiveSupport::TestCase
     create :user_watched_video, user:, video_provider: provider, video_id: id
     assert user.watched_video?(provider, id)
   end
+
+  test "rejects a newly attached avatar in an unsupported format" do
+    user = create :user
+
+    user.avatar = {
+      io: StringIO.new('<svg xmlns="http://www.w3.org/2000/svg"></svg>'),
+      filename: "avatar.svg",
+      content_type: "image/svg+xml"
+    }
+
+    refute user.valid?
+    assert_includes user.errors[:avatar], "must be a PNG, JPEG, GIF or WebP image"
+  end
+
+  test "users with an existing avatar in another format can still be saved" do
+    user = create :user
+    user.avatar.purge
+    blob = ActiveStorage::Blob.create_and_upload!(
+      io: StringIO.new('<svg xmlns="http://www.w3.org/2000/svg"></svg>'),
+      filename: "avatar.svg",
+      content_type: "image/svg+xml"
+    )
+    ActiveStorage::Attachment.create!(record: user, name: "avatar", blob:)
+    user = User.find(user.id)
+
+    assert user.update(name: "New name")
+  end
 end