FIX: Never allow custom emoji to be marked secure (#8965)

FIX: Never allow custom emoji to be marked secure (#8965)

  • Because custom emoji count as post “uploads” we were marking them as secure when updating the secure status for post uploads.
  • We were also giving them an access control post id, which meant broken image previews from 403 errors in the admin custom emoji list.
  • We now check if an upload is used as a custom emoji and do not assign the access control post + never mark as secure.
diff --git a/app/models/post.rb b/app/models/post.rb
index 0a1a70e6cd..c0d69eac99 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -916,7 +916,11 @@ class Post < ActiveRecord::Base
       end
 
       if SiteSetting.secure_media?
-        Upload.where(id: upload_ids, access_control_post_id: nil).update_all(
+        Upload.where(
+          id: upload_ids, access_control_post_id: nil
+        ).where.not(
+          id: CustomEmoji.pluck(:upload_id)
+        ).update_all(
           access_control_post_id: self.id
         )
       end
diff --git a/app/models/upload.rb b/app/models/upload.rb
index d71365cc99..d306ef9538 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -254,7 +254,6 @@ class Upload < ActiveRecord::Base
   end
 
   def update_secure_status(secure_override_value: nil)
-    return false if self.for_theme || self.for_site_setting
     mark_secure = secure_override_value.nil? ? UploadSecurity.new(self).should_be_secure? : secure_override_value
 
     secure_status_did_change = self.secure? != mark_secure
diff --git a/lib/upload_security.rb b/lib/upload_security.rb
index b20b7af257..816377d526 100644
--- a/lib/upload_security.rb
+++ b/lib/upload_security.rb
@@ -28,7 +28,7 @@ class UploadSecurity
   private
 
   def uploading_in_public_context?
-    @upload.for_theme || @upload.for_site_setting || @upload.for_gravatar || public_type?
+    @upload.for_theme || @upload.for_site_setting || @upload.for_gravatar || public_type? || used_for_custom_emoji?
   end
 
   def supported_media?
@@ -70,4 +70,9 @@ class UploadSecurity
   def uploading_in_composer?
     @upload_type == "composer"
   end
+
+  def used_for_custom_emoji?
+    return false if @upload.id.blank?
+    CustomEmoji.exists?(upload_id: @upload.id)
+  end
 end
diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb
index 33204e30f2..80efa2ed1f 100644
--- a/spec/models/post_spec.rb
+++ b/spec/models/post_spec.rb
@@ -1376,6 +1376,16 @@ describe Post do
           expect(image_upload.access_control_post_id).to eq(post.id)
           expect(video_upload.access_control_post_id).not_to eq(post.id)
         end
+
+        context "for custom emoji" do
+          before do
+            CustomEmoji.create(name: "meme", upload: image_upload)
+          end
+          it "never sets an access control post because they should not be secure" do
+            post.link_post_uploads
+            expect(image_upload.reload.access_control_post_id).to eq(nil)
+          end
+        end
       end
     end
 
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index fed2faf1fa..2b7479c06e 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -419,7 +419,17 @@ describe Upload do
         expect { upload.update_secure_status }
           .to change { upload.secure }
 
-        expect(upload.secure).to eq(true)
+        expect(upload.reload.secure).to eq(true)
+      end
+
+      it 'does not mark an upload used for a custom emoji as secure' do
+        SiteSetting.login_required = true
+        upload.update!(secure: false)
+        CustomEmoji.create(name: 'meme', upload: upload)
+        expect { upload.update_secure_status }
+          .not_to change { upload.secure }
+
+        expect(upload.reload.secure).to eq(false)
       end
     end
   end

GitHub sha: 56b16bc6

2 Likes

This commit appears in #8965 which was merged by @martin.

CustomEmoji.select would be better as it’s not doing an additional SQL query and loading all the custom emoji upload ids in memory :wink:

Could have been written as a one-liner :wink:

def used_for_custom_emoji?
  @upload.id.present? && CustomEmoji.exists?(upload_id: @upload.id)
end