From 614a1d51b0740955a4ef07c385e88962b4870642 Mon Sep 17 00:00:00 2001
From: Mike Dalessio <mike@37signals.com>
Date: Tue, 28 Jul 2026 10:46:54 -0400
Subject: [PATCH] Disable libvips unfuzzed operations

and add test coverage for (un)supported file types.

BMP, ICO, and PSD attachments are no longer variable, and the picture views call
`variant(:large)` unconditionally, which raises `ActiveStorage::InvariableError`.
`Picture#large_image` will fall back to the original attachment.
---
 app/models/picture.rb                         |   4 +
 app/views/leafables/show.html.erb             |   2 +-
 app/views/leaves/_edit.html.erb               |   2 +-
 app/views/leaves/_leaf.html.erb               |   2 +-
 app/views/pictures/_form.html.erb             |   2 +-
 config/initializers/vips.rb                   |   8 ++
 test/controllers/leafables_controller_test.rb |  10 ++
 test/fixtures/files/pixel.bmp                 | Bin 0 -> 58 bytes
 test/lib/vips_loader_policy_test.rb           | 118 ++++++++++++++++++
 test/models/picture_test.rb                   |  11 ++
 10 files changed, 155 insertions(+), 4 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

diff --git a/app/models/picture.rb b/app/models/picture.rb
index bb98a115..6573a97b 100644
--- a/app/models/picture.rb
+++ b/app/models/picture.rb
@@ -5,6 +5,10 @@ class Picture < ApplicationRecord
     attachable.variant :large, resize_to_limit: [ 1500, 1500 ]
   end
 
+  def large_image
+    image.variable? ? image.variant(:large) : image
+  end
+
   def markable
     caption
   end
diff --git a/app/views/leafables/show.html.erb b/app/views/leafables/show.html.erb
index 99d0af3c..bc64e1bc 100644
--- a/app/views/leafables/show.html.erb
+++ b/app/views/leafables/show.html.erb
@@ -27,7 +27,7 @@
             action: "lightbox#open:prevent",
             lightbox_target: "image",
             lightbox_url_value: rails_blob_path(@leaf.picture.image, disposition: "attachment", only_path: true) } do %>
-        <%= image_tag @leaf.picture.image.variant(:large), loading: "lazy" %>
+        <%= image_tag @leaf.picture.large_image, loading: "lazy" %>
       <% end %>
     <% else %>
       <%= image_tag "default-picture.webp", alt: "No image uploaded", loading: "lazy" %>
diff --git a/app/views/leaves/_edit.html.erb b/app/views/leaves/_edit.html.erb
index 43633f58..559939d5 100644
--- a/app/views/leaves/_edit.html.erb
+++ b/app/views/leaves/_edit.html.erb
@@ -7,7 +7,7 @@
   <%= link_to leafable_path(leaf), class: "toc__thumbnail", data: { turbo_frame: "_top" } do %>
     <%= leaf.section.body if leaf.section? %>
     <%= sanitize_content(leaf.leafable.body.to_html) if leaf.page? %>
-    <%= image_tag leaf.leafable.image.variant(:large) if leaf.picture&.image&.attached? %>
+    <%= image_tag leaf.leafable.large_image if leaf.picture&.image&.attached? %>
   <% end %>
 
   <div class="toc__title flex align-center min-width">
diff --git a/app/views/leaves/_leaf.html.erb b/app/views/leaves/_leaf.html.erb
index dccdd672..e6282542 100644
--- a/app/views/leaves/_leaf.html.erb
+++ b/app/views/leaves/_leaf.html.erb
@@ -27,7 +27,7 @@
       <%= sanitize_content leaf.page.html_preview if leaf.page? %>
 
       <% if leaf.picture? %>
-        <%= image_tag leaf.leafable.image.attached? ? leaf.leafable.image.variant(:large) : "default-picture.webp" %>
+        <%= image_tag leaf.leafable.image.attached? ? leaf.leafable.large_image : "default-picture.webp" %>
       <% end %>
     </div>
 
diff --git a/app/views/pictures/_form.html.erb b/app/views/pictures/_form.html.erb
index da1b4948..1805eb72 100644
--- a/app/views/pictures/_form.html.erb
+++ b/app/views/pictures/_form.html.erb
@@ -1,6 +1,6 @@
 <%= leafable_edit_form(picture, id: "leafable-editor") do |form| %>
   <label class="input input--file input--picture unpad" data-controller="upload-preview">
-    <%= image_tag picture.image.attached? ? picture.image.variant(:large) : "default-picture.webp", alt: "Picture",
+    <%= image_tag picture.image.attached? ? picture.large_image : "default-picture.webp", alt: "Picture",
         data: { upload_preview_target: "image" } %>
     <%= form.file_field :image, class: "input", accept: "image/png, image/jpeg, image/jpg, image/webp", autofocus: true,
         data: { upload_preview_target: "input", action: "upload-preview#previewImage" } %>
diff --git a/config/initializers/vips.rb b/config/initializers/vips.rb
new file mode 100644
index 00000000..3a894fa6
--- /dev/null
+++ b/config/initializers/vips.rb
@@ -0,0 +1,8 @@
+# Disable unfuzzed libvips operations.
+#
+# Reference the Active Storage transformer to ensure Vips is loaded before we block operations.
+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/leafables_controller_test.rb b/test/controllers/leafables_controller_test.rb
index 167c600b..d9e383c7 100644
--- a/test/controllers/leafables_controller_test.rb
+++ b/test/controllers/leafables_controller_test.rb
@@ -76,6 +76,16 @@ class LeafablesControllerTest < ActionDispatch::IntegrationTest
     assert_in_body "title: \"Reading\""
   end
 
+  test "show a picture whose image cannot be resized" do
+    leaves(:reading_picture).leafable.image.attach io: file_fixture("pixel.bmp").open,
+      filename: "pixel.bmp", content_type: "image/bmp"
+
+    get leafable_slug_path(leaves(:reading_picture))
+
+    assert_response :success
+    assert_select "figure img[src*=\"pixel.bmp\"]"
+  end
+
   test "show with markdown format does not escape HTML entities" do
     leaves(:welcome_page).leafable.update!(body: "This has <a href='http://example.com'>a link</a>")
 
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 00000000..e522f6a6
--- /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(<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8"/>)
+
+  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/picture_test.rb b/test/models/picture_test.rb
index bc609402..18a2a560 100644
--- a/test/models/picture_test.rb
+++ b/test/models/picture_test.rb
@@ -12,4 +12,15 @@ class PictureTest < ActiveSupport::TestCase
 
     assert_nil picture.markable
   end
+
+  test "large_image is the resized variant of a variable image" do
+    assert_kind_of ActiveStorage::VariantWithRecord, pictures(:reading).large_image
+  end
+
+  test "large_image is the original image when the image cannot be resized" do
+    picture = pictures(:reading)
+    picture.image.attach io: file_fixture("pixel.bmp").open, filename: "pixel.bmp", content_type: "image/bmp"
+
+    assert_equal picture.image, picture.large_image
+  end
 end