From 341f87efb775c261e8366806ed16a4b39e5a1131 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Thu, 18 May 2023 10:36:34 +0200
Subject: [PATCH] FIX: Show gif upload size limit error straight away (#21633)

When uploading images via direct to S3 upload, we were
assuming that we could not pre-emptively check the file
size because the client may do preprocessing to reduce
the size, and UploadCreator could also further reduce the
size.

This, however, is not true of gifs, so we would have an
issue where you upload a gif > the max_image_size_kb
setting and had to wait until the upload completed for
this error to show.

Now, instead, when we direct upload gifs to S3, we check
the size straight away and present a file size error to
the user rather than making them wait. This will increase
meme efficiency by approximately 1000%.
---
 app/controllers/uploads_controller.rb    | 24 +++++++++++++++++++++---
 spec/requests/uploads_controller_spec.rb | 10 ++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index b71240638ee..3eeb85e7149 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -221,7 +221,7 @@ class UploadsController < ApplicationController
   def validate_file_size(file_name:, file_size:)
     raise ExternalUploadValidationError.new(I18n.t("upload.size_zero_failure")) if file_size.zero?
 
-    if file_size_too_big?(file_name, file_size)
+    if attachment_too_big?(file_name, file_size)
       raise ExternalUploadValidationError.new(
               I18n.t(
                 "upload.attachments.too_large_humanized",
@@ -232,6 +232,18 @@ class UploadsController < ApplicationController
               ),
             )
     end
+
+    if image_too_big?(file_name, file_size)
+      raise ExternalUploadValidationError.new(
+              I18n.t(
+                "upload.images.too_large_humanized",
+                max_size:
+                  ActiveSupport::NumberHelper.number_to_human_size(
+                    SiteSetting.max_image_size_kb.kilobytes,
+                  ),
+              ),
+            )
+    end
   end
 
   def force_download?
@@ -306,14 +318,20 @@ class UploadsController < ApplicationController
 
   private
 
-  # We can preemptively check size for attachments, but not for images
+  # We can preemptively check size for attachments, but not for (most) images
   # as they may be further reduced in size by UploadCreator (at this point
   # they may have already been reduced in size by preprocessors)
-  def file_size_too_big?(file_name, file_size)
+  def attachment_too_big?(file_name, file_size)
     !FileHelper.is_supported_image?(file_name) &&
       file_size >= SiteSetting.max_attachment_size_kb.kilobytes
   end
 
+  # Gifs are not resized on the client and not reduced in size by UploadCreator
+  def image_too_big?(file_name, file_size)
+    FileHelper.is_supported_image?(file_name) && File.extname(file_name) == ".gif" &&
+      file_size >= SiteSetting.max_image_size_kb.kilobytes
+  end
+
   def send_file_local_upload(upload)
     opts = {
       filename: upload.original_filename,
diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb
index 44eee47daec..e0101f269fd 100644
--- a/spec/requests/uploads_controller_spec.rb
+++ b/spec/requests/uploads_controller_spec.rb
@@ -853,6 +853,16 @@ RSpec.describe UploadsController do
         )
       end
 
+      it "returns 422 when the file is an gif and it's too big, since gifs cannot be resized on client" do
+        SiteSetting.max_image_size_kb = 1024
+        post "/uploads/create-multipart.json",
+             **{ params: { file_name: "test.gif", file_size: 9_999_999, upload_type: "composer" } }
+        expect(response.status).to eq(422)
+        expect(response.body).to include(
+          I18n.t("upload.images.too_large_humanized", max_size: "1 MB"),
+        )
+      end
+
       it "returns a sensible error if the file size is 0 bytes" do
         SiteSetting.authorized_extensions = "*"
         stub_create_multipart_request