Merge pull request #229 from basecamp/fix-version-number

Fix version number for CI built images

.github/workflows/publish-image.yml

@@ -67,6 +67,17 @@ jobs:
           CANONICAL_IMAGE="${REGISTRY}/${IMAGE_REF,,}"
           echo "canonical=${CANONICAL_IMAGE}" >> "$GITHUB_OUTPUT"
 
+      - name: Compute app version
+        id: version
+        shell: bash
+        run: |
+          set -eu
+          if [[ "${GITHUB_REF}" == refs/tags/v* ]]; then
+            echo "version=${GITHUB_REF#refs/tags/v}" >> "$GITHUB_OUTPUT"
+          else
+            echo "version=${GITHUB_SHA::7}" >> "$GITHUB_OUTPUT"
+          fi
+
       - name: Extract Docker metadata (tags, labels) with arch suffix
         id: meta
         uses: docker/metadata-action@c1e51972afc2121e065aed6d45c65596fe445f3f # v5.8.0
@@ -94,6 +105,8 @@ jobs:
           build-args: |
             OCI_SOURCE=${{ env.SOURCE_URL }}
             OCI_DESCRIPTION=${{ env.IMAGE_DESCRIPTION }}
+            APP_VERSION=${{ steps.version.outputs.version }}
+            GIT_REVISION=${{ github.sha }}
           platforms: ${{ matrix.platform }}
           push: ${{ github.event_name != 'pull_request' }}
           tags: ${{ steps.meta.outputs.tags }}

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?
+      if (logo_variant = Current.account&.logo_variant(logo_size))
-        logo = Current.account.logo.variant(logo_variant).processed
+        send_png_file ActiveStorage::Blob.service.path_for(logo_variant.key)
-        send_png_file ActiveStorage::Blob.service.path_for(logo.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
+    def logo_size
-      small_logo? ? SMALL_SQUARE_PNG_VARIANT : LARGE_SQUARE_PNG_VARIANT
+      small_logo? ? :small : :large
     end
 
     def small_logo?

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?
+      if (avatar_variant = @user.avatar_variant)
-        avatar_variant = @user.avatar.variant(SQUARE_WEBP_VARIANT).processed
         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

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

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

config/initializers/version.rb

@@ -1,2 +1,2 @@
-Rails.application.config.app_version = ENV.fetch("APP_VERSION", "0")
+Rails.application.config.app_version = ENV["APP_VERSION"].presence || ENV["GIT_REVISION"].presence || "0"
 Rails.application.config.git_revision = ENV["GIT_REVISION"]

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 ]

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")
 

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")
 

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

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

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