From b065b40a34b788eafcfe5145fc3e9b6d4fd2e996 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 28 Jul 2026 11:57:57 -0400 Subject: [PATCH] Disable libvips unfuzzed operations (#226) and add test coverage for (un)supported file types. The avatar and logo variants move into the models and return nil for content types that are no longer variable, so the controllers fall back to the initials avatar and stock logo icon instead of raising `ActiveStorage::InvariableError`. --- app/controllers/accounts/logos_controller.rb | 12 +- app/controllers/users/avatars_controller.rb | 5 +- app/models/account.rb | 10 +- app/models/user/avatar.rb | 8 +- config/initializers/vips.rb | 9 ++ .../accounts/logos_controller_test.rb | 7 ++ .../users/avatars_controller_test.rb | 8 ++ test/fixtures/files/pixel.bmp | Bin 0 -> 58 bytes test/lib/vips_loader_policy_test.rb | 118 ++++++++++++++++++ test/models/account_test.rb | 16 +++ test/models/user/avatar_test.rb | 19 +++ 11 files changed, 198 insertions(+), 14 deletions(-) create mode 100644 config/initializers/vips.rb create mode 100644 test/fixtures/files/pixel.bmp create mode 100644 test/lib/vips_loader_policy_test.rb create mode 100644 test/models/user/avatar_test.rb 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 0000000000000000000000000000000000000000..b5f29a285f393f53c98c3aaff84aff01b8fc06b1 GIT binary patch literal 58 ecmZ?rwPJt(Ga#h_#Eft(0hV9^lc>ahAQu2dn*v4v literal 0 HcmV?d00001 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