diff --git a/app/controllers/accounts/logos_controller.rb b/app/controllers/accounts/logos_controller.rb index c4cd808..dbd06a8 100644 --- a/app/controllers/accounts/logos_controller.rb +++ b/app/controllers/accounts/logos_controller.rb @@ -8,9 +8,8 @@ class Accounts::LogosController < ApplicationController if stale?(etag: Current.account) expires_in 5.minutes, public: true, stale_while_revalidate: 1.week - if Current.account&.logo&.attached? - logo = Current.account.logo.variant(logo_variant).processed - send_png_file ActiveStorage::Blob.service.path_for(logo.key) + if (logo_variant = Current.account&.logo_variant(logo_size)) + send_png_file ActiveStorage::Blob.service.path_for(logo_variant.key) else send_stock_icon end @@ -23,9 +22,6 @@ class Accounts::LogosController < ApplicationController end private - LARGE_SQUARE_PNG_VARIANT = { resize_to_limit: [ 512, 512 ], format: :png } - SMALL_SQUARE_PNG_VARIANT = { resize_to_limit: [ 192, 192 ], format: :png } - def send_png_file(path) send_file path, content_type: "image/png", disposition: :inline end @@ -38,8 +34,8 @@ class Accounts::LogosController < ApplicationController end end - def logo_variant - small_logo? ? SMALL_SQUARE_PNG_VARIANT : LARGE_SQUARE_PNG_VARIANT + def logo_size + small_logo? ? :small : :large end def small_logo? diff --git a/app/controllers/users/avatars_controller.rb b/app/controllers/users/avatars_controller.rb index 7cedb40..79f7199 100644 --- a/app/controllers/users/avatars_controller.rb +++ b/app/controllers/users/avatars_controller.rb @@ -9,8 +9,7 @@ class Users::AvatarsController < ApplicationController if stale?(etag: @user) expires_in 30.minutes, public: true, stale_while_revalidate: 1.week - if @user.avatar.attached? - avatar_variant = @user.avatar.variant(SQUARE_WEBP_VARIANT).processed + if (avatar_variant = @user.avatar_variant) send_webp_blob_file avatar_variant.key elsif @user.bot? render_default_bot @@ -26,8 +25,6 @@ class Users::AvatarsController < ApplicationController end private - SQUARE_WEBP_VARIANT = { resize_to_limit: [ 512, 512 ], format: :webp } - def send_webp_blob_file(key) send_file ActiveStorage::Blob.service.path_for(key), content_type: "image/webp", disposition: :inline end diff --git a/app/models/account.rb b/app/models/account.rb index f0b7c98..dfa9bea 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,6 +1,14 @@ class Account < ApplicationRecord include Joinable - has_one_attached :logo + has_one_attached :logo do |attachable| + attachable.variant :large, resize_to_limit: [ 512, 512 ], format: :png + attachable.variant :small, resize_to_limit: [ 192, 192 ], format: :png + end + has_json :settings, restrict_room_creation_to_administrators: false + + def logo_variant(size) + logo.variant(size).processed if logo.variable? + end end diff --git a/app/models/user/avatar.rb b/app/models/user/avatar.rb index a9d2ad1..8a77f23 100644 --- a/app/models/user/avatar.rb +++ b/app/models/user/avatar.rb @@ -2,7 +2,9 @@ module User::Avatar extend ActiveSupport::Concern included do - has_one_attached :avatar + has_one_attached :avatar do |attachable| + attachable.variant :square, resize_to_limit: [ 512, 512 ], format: :webp + end end class_methods do @@ -14,4 +16,8 @@ module User::Avatar def avatar_token signed_id(purpose: :avatar) end + + def avatar_variant + avatar.variant(:square).processed if avatar.variable? + end end diff --git a/config/initializers/vips.rb b/config/initializers/vips.rb new file mode 100644 index 0000000..3c16801 --- /dev/null +++ b/config/initializers/vips.rb @@ -0,0 +1,9 @@ +# Disable unfuzzed libvips operations. +# +# To block loaders we need to call `Vips.block` after Rails and image_processing set their +# defaults. Force the order of operations by autoloading the file now. +ActiveStorage::Transformers::Vips +Vips.block_untrusted(true) +Vips.block("VipsForeignLoadOpenslide", true) # prevent sqlite segfault in forked parallel workers +Rails.application.config.active_storage.variable_content_types -= + %w[ image/bmp image/vnd.microsoft.icon image/vnd.adobe.photoshop ] diff --git a/test/controllers/accounts/logos_controller_test.rb b/test/controllers/accounts/logos_controller_test.rb index 857625e..46fce10 100644 --- a/test/controllers/accounts/logos_controller_test.rb +++ b/test/controllers/accounts/logos_controller_test.rb @@ -30,6 +30,13 @@ class Accounts::LogosControllerTest < ActionDispatch::IntegrationTest assert_valid_png_response size: 192 end + test "show stock when custom logo cannot be resized" do + accounts(:signal).update! logo: fixture_file_upload("pixel.bmp", "image/bmp") + + get account_logo_url + assert_valid_png_response size: 512 + end + test "destroy" do accounts(:signal).update! logo: fixture_file_upload("moon.jpg", "image/jpeg") diff --git a/test/controllers/users/avatars_controller_test.rb b/test/controllers/users/avatars_controller_test.rb index ee6e20b..fdc4501 100644 --- a/test/controllers/users/avatars_controller_test.rb +++ b/test/controllers/users/avatars_controller_test.rb @@ -18,6 +18,14 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest assert_equal "image/webp", @response.content_type end + test "show initials when image cannot be resized" do + users(:kevin).update! avatar: fixture_file_upload("pixel.bmp", "image/bmp") + get user_avatar_url(users(:kevin).avatar_token) + + assert_response :success + assert_select "text", text: "K" + end + test "show image with invalid token responds 404" do get user_avatar_url("not-a-valid-token") diff --git a/test/fixtures/files/pixel.bmp b/test/fixtures/files/pixel.bmp new file mode 100644 index 0000000..b5f29a2 Binary files /dev/null and b/test/fixtures/files/pixel.bmp differ diff --git a/test/lib/vips_loader_policy_test.rb b/test/lib/vips_loader_policy_test.rb new file mode 100644 index 0000000..e522f6a --- /dev/null +++ b/test/lib/vips_loader_policy_test.rb @@ -0,0 +1,118 @@ +require "test_helper" +require "vips" +require "tempfile" + +# libvips selects a loader from a file's actual bytes, not from its declared content type. These +# tests pin which loader is selected for each file type under the app's configured loader policy +# (config/initializers/vips.rb). +class VipsLoaderPolicyTest < ActiveSupport::TestCase + # Header bytes are enough for libvips to identify a format; native types are encoded live, exotic + # ones are represented by their magic bytes. + FTYP_AVIF = "\x00\x00\x00\x1cftypavif\x00\x00\x00\x00avifmif1miaf".b + FTYP_HEIC = "\x00\x00\x00\x1cftypheic\x00\x00\x00\x00heicmif1miaf".b + BMP = "BM" + [ 0, 0, 54 ].pack("V3") + "\x00" * 40 + PSD = "8BPS" + [ 1 ].pack("n") + "\x00" * 26 + ICO = "\x00\x00\x01\x00\x01\x00" + "\x00" * 16 + SVG = %q() + + test "loads PNG" do + assert_equal "VipsForeignLoadPngFile", loader_for(encode("png")) + end + + test "loads GIF" do + assert_equal "VipsForeignLoadNsgifFile", loader_for(encode("gif")) + end + + test "loads JPEG" do + assert_equal "VipsForeignLoadJpegFile", loader_for(encode("jpg")) + end + + test "loads TIFF" do + assert_equal "VipsForeignLoadTiffFile", loader_for(encode("tif")) + end + + test "loads WebP" do + assert_equal "VipsForeignLoadWebpFile", loader_for(encode("webp")) + end + + test "loads AVIF" do + assert_equal "VipsForeignLoadHeifFile", loader_for(FTYP_AVIF) + end + + test "loads HEIC" do + assert_equal "VipsForeignLoadHeifFile", loader_for(FTYP_HEIC) + end + + test "denies BMP through magickload" do + assert_nil loader_for(BMP) + end + + test "denies PSD through magickload" do + assert_nil loader_for(PSD) + end + + test "denies ICO through magickload" do + assert_nil loader_for(ICO) + end + + test "denies SVG through svgload" do + assert_nil loader_for(SVG) + end + + test "denies OpenSlide files through openslideload" do + # OpenSlide files can segfault the embedded sqlite in forked parallel workers + assert_loader_blocked :openslideload, ".svs" + end + + test "denies FITS files through fitsload" do + assert_loader_blocked :fitsload, ".fits" + end + + test "denies MATLAB files through matload" do + assert_loader_blocked :matload, ".mat" + end + + test "denies NIFTI files through niftiload" do + assert_loader_blocked :niftiload, ".nii" + end + + test "denies RAW files through dcrawload" do + assert_loader_blocked :dcrawload, ".raw" + end + + test "denies VIPS files through vipsload" do + assert_loader_blocked :vipsload, ".vips" + end + + private + # Invoke a specific libvips loader directly and assert it is refused because the + # operation is blocked (rather than because the bytes are not a valid image). + def assert_loader_blocked(operation, extension) + Tempfile.create([ "blocked_loader", extension ], binmode: true) do |file| + file.write "not an image" + file.flush + + error = assert_raises(Vips::Error) { Vips::Image.public_send(operation, file.path) } + actual = error.message.chomp + + # note that exception message may include multiple errors on separate lines, + # so `^` and `$` anchors are used instead of `\A` and `\z`. + if actual =~ /^VipsOperation: class \"#{operation}\" not found$/ + skip "libvips does not support #{operation} on this system" + end + assert_match(/^#{operation}: operation is blocked$/, actual) + end + end + + def encode(ext) + Vips::Image.black(8, 8).add(128).cast("uchar").write_to_buffer(".#{ext}") + end + + def loader_for(bytes) + Tempfile.create(%w[loader_probe .img], binmode: true) do |file| + file.write bytes + file.flush + Vips.vips_foreign_find_load(file.path) + end + end +end diff --git a/test/models/account_test.rb b/test/models/account_test.rb index 2585ba0..12c148f 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -15,4 +15,20 @@ class AccountTest < ActiveSupport::TestCase accounts(:signal).update!(settings: { "restrict_room_creation_to_administrators" => "false" }) assert_not accounts(:signal).reload.settings.restrict_room_creation_to_administrators? end + + test "logo_variant is a resized variant of a variable logo" do + accounts(:signal).logo.attach io: file_fixture("moon.jpg").open, filename: "moon.jpg", content_type: "image/jpeg" + + assert_kind_of ActiveStorage::VariantWithRecord, accounts(:signal).logo_variant(:large) + end + + test "logo_variant is nil when the logo cannot be resized" do + accounts(:signal).logo.attach io: file_fixture("pixel.bmp").open, filename: "pixel.bmp", content_type: "image/bmp" + + assert_nil accounts(:signal).logo_variant(:large) + end + + test "logo_variant is nil without a logo" do + assert_nil accounts(:signal).logo_variant(:large) + end end diff --git a/test/models/user/avatar_test.rb b/test/models/user/avatar_test.rb new file mode 100644 index 0000000..bef5beb --- /dev/null +++ b/test/models/user/avatar_test.rb @@ -0,0 +1,19 @@ +require "test_helper" + +class User::AvatarTest < ActiveSupport::TestCase + test "avatar_variant is a resized variant of a variable avatar" do + users(:kevin).avatar.attach io: file_fixture("moon.jpg").open, filename: "moon.jpg", content_type: "image/jpeg" + + assert_kind_of ActiveStorage::VariantWithRecord, users(:kevin).avatar_variant + end + + test "avatar_variant is nil when the avatar cannot be resized" do + users(:kevin).avatar.attach io: file_fixture("pixel.bmp").open, filename: "pixel.bmp", content_type: "image/bmp" + + assert_nil users(:kevin).avatar_variant + end + + test "avatar_variant is nil without an avatar" do + assert_nil users(:kevin).avatar_variant + end +end