From f484336eddcfc87bde0e60eebee0da61b91d6f1d Mon Sep 17 00:00:00 2001 From: Jeremy Walker Date: Tue, 27 Oct 2026 13:45:36 +0900 Subject: [PATCH 1/2] Restrict avatar uploads to standard image formats --- app/commands/user/retrieve_avatar.rb | 47 +++++++++++ app/controllers/api/users_controller.rb | 10 ++- app/controllers/avatars_controller.rb | 21 ++--- .../profile/avatar-selector/Photo.tsx | 7 +- app/models/user.rb | 15 ++++ config/locales/api/en.yml | 1 + test/commands/user/retrieve_avatar_test.rb | 82 +++++++++++++++++++ test/commands/user/update_avatar_test.rb | 49 +++++++++-- test/controllers/api/users_controller_test.rb | 35 ++++++++ test/controllers/avatars_controller_test.rb | 12 +++ test/models/user_test.rb | 27 ++++++ 11 files changed, 286 insertions(+), 20 deletions(-) create mode 100644 app/commands/user/retrieve_avatar.rb create mode 100644 test/commands/user/retrieve_avatar_test.rb create mode 100644 test/controllers/avatars_controller_test.rb diff --git a/app/commands/user/retrieve_avatar.rb b/app/commands/user/retrieve_avatar.rb new file mode 100644 index 0000000000..709f70db50 --- /dev/null +++ b/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 diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb index 7aceed1003..3fa6db9896 100644 --- a/app/controllers/api/users_controller.rb +++ b/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? diff --git a/app/controllers/avatars_controller.rb b/app/controllers/avatars_controller.rb index 29eba72ba9..f5403f2f7e 100644 --- a/app/controllers/avatars_controller.rb +++ b/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,16 +15,7 @@ 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) @@ -30,12 +23,16 @@ def show 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 diff --git a/app/javascript/components/profile/avatar-selector/Photo.tsx b/app/javascript/components/profile/avatar-selector/Photo.tsx index 3655009134..afc501c9f5 100644 --- a/app/javascript/components/profile/avatar-selector/Photo.tsx +++ b/app/javascript/components/profile/avatar-selector/Photo.tsx @@ -33,7 +33,12 @@ export const Photo = ({
{t('photo.uploadNewImage')}
- +
{t('photo.cropping')}
diff --git a/app/models/user.rb b/app/models/user.rb index 3cbf5c9275..4a1a789d84 100644 --- a/app/models/user.rb +++ b/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 diff --git a/config/locales/api/en.yml b/config/locales/api/en.yml index 58d069b54f..3c720268f9 100644 --- a/config/locales/api/en.yml +++ b/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" diff --git a/test/commands/user/retrieve_avatar_test.rb b/test/commands/user/retrieve_avatar_test.rb new file mode 100644 index 0000000000..f59ac3d275 --- /dev/null +++ b/test/commands/user/retrieve_avatar_test.rb @@ -0,0 +1,82 @@ +require "test_helper" + +class User::RetrieveAvatarTest < ActiveSupport::TestCase + 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 diff --git a/test/commands/user/update_avatar_test.rb b/test/commands/user/update_avatar_test.rb index f6db7894fe..90d76b0c5f 100644 --- a/test/commands/user/update_avatar_test.rb +++ b/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", '', '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", "Hello", '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 diff --git a/test/controllers/api/users_controller_test.rb b/test/controllers/api/users_controller_test.rb index 12d3a1b7bb..ad2ae08c40 100644 --- a/test/controllers/api/users_controller_test.rb +++ b/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('') + 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) diff --git a/test/controllers/avatars_controller_test.rb b/test/controllers/avatars_controller_test.rb new file mode 100644 index 0000000000..1f354e15b9 --- /dev/null +++ b/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 diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a6f5d28a15..9319920c32 100644 --- a/test/models/user_test.rb +++ b/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(''), + 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(''), + 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 From fe072b1656acaac74358a734b3dfc61a71f8b175 Mon Sep 17 00:00:00 2001 From: Jeremy Walker Date: Tue, 27 Oct 2026 15:36:08 +0900 Subject: [PATCH 2/2] Keep the avatar file picker open to all image types --- .../components/profile/avatar-selector/Photo.tsx | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/javascript/components/profile/avatar-selector/Photo.tsx b/app/javascript/components/profile/avatar-selector/Photo.tsx index afc501c9f5..3655009134 100644 --- a/app/javascript/components/profile/avatar-selector/Photo.tsx +++ b/app/javascript/components/profile/avatar-selector/Photo.tsx @@ -33,12 +33,7 @@ export const Photo = ({
{t('photo.uploadNewImage')}
- +
{t('photo.cropping')}