FIX - limit number of embedded media items in a post (#10391)

* FIX - limit number of embedded media items in a post

* Add renamed settings to DeprecatedSettings

app/models/post.rb

@@ -269,7 +269,7 @@ class Post < ActiveRecord::Base
 
   %w{raw_mentions
     linked_hosts
-    image_count
+    embedded_media_count
     attachment_count
     link_count
     raw_links

app/models/post_analyzer.rb

@@ -47,10 +47,11 @@ class PostAnalyzer
   end
 
   # How many images are present in the post
-  def image_count
+  def embedded_media_count
     return 0 unless @raw.present?
 
-    cooked_stripped.css("img").reject do |t|
+    # TODO - do we need to look for tags other than img, video and audio?
+    cooked_stripped.css("img", "video", "audio").reject do |t|
       if dom_class = t["class"]
         (Post.allowed_image_classes & dom_class.split).count > 0
       end

config/locales/server.en.yml

@@ -306,11 +306,11 @@ en:
   too_many_mentions_newuser:
     one: "Sorry, new users can only mention one other user in a post."
     other: "Sorry, new users can only mention %{count} users in a post."
-  no_images_allowed_trust: "Sorry, you can't put images in a post"
-  no_images_allowed: "Sorry, new users can't put images in posts."
-  too_many_images:
-    one: "Sorry, new users can only put one image in a post."
-    other: "Sorry, new users can only put %{count} images in a post."
+  no_embedded_media_allowed_trust: "Sorry, you can't embed media items in a post."
+  no_embedded_media_allowed: "Sorry, new users can't embed media items in posts."
+  too_many_embedded_media:
+    one: "Sorry, new users can only put one embedded media item in a post."
+    other: "Sorry, new users can only put %{count} embedded media items in a post."
   no_attachments_allowed: "Sorry, new users can't put attachments in posts."
   too_many_attachments:
     one: "Sorry, new users can only put one attachment in a post."
@@ -1802,12 +1802,12 @@ en:
     min_trust_to_send_messages: "The minimum trust level required to create new personal messages."
     min_trust_to_flag_posts: "The minimum trust level required to flag posts"
     min_trust_to_post_links: "The minimum trust level required to include links in posts"
-    min_trust_to_post_images: "The minimum trust level required to include images in a post"
+    min_trust_to_post_embedded_media: "The minimum trust level required to embed media items in a post"
 
     allowed_link_domains: "Domains that users may link to even if they don't have the appropriate trust level to post links"
 
     newuser_max_links: "How many links a new user can add to a post."
-    newuser_max_images: "How many images a new user can add to a post."
+    newuser_max_embedded_media: "How many embedded media items a new user can add to a post."
     newuser_max_attachments: "How many attachments a new user can add to a post."
     newuser_max_mentions_per_post: "Maximum number of @name notifications a new user can use in a post."
     newuser_max_replies_per_topic: "Maximum number of replies a new user can make in a single topic until someone replies to them."

config/site_settings.yml

@@ -807,7 +807,7 @@ posting:
     default: ""
     type: list
   newuser_max_links: 2
-  newuser_max_images:
+  newuser_max_embedded_media:
     client: true
     default: 1
   newuser_max_attachments:
@@ -1359,7 +1359,7 @@ trust:
   min_trust_to_post_links:
     default: 0
     enum: "TrustLevelSetting"
-  min_trust_to_post_images:
+  min_trust_to_post_embedded_media:
     default: 0
     enum: "TrustLevelSetting"
   allow_flagging_staff: true

db/migrate/20200805163400_rename_post_image_site_settings.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class RenamePostImageSiteSettings < ActiveRecord::Migration[6.0]
+  def up
+    execute "UPDATE site_settings SET name = 'newuser_max_embedded_media' WHERE name = 'newuser_max_images'"
+    execute "UPDATE user_histories SET subject = 'newuser_max_embedded_media' WHERE subject = 'newuser_max_images'"
+
+    execute "UPDATE site_settings SET name = 'min_trust_to_post_embedded_media' WHERE name = 'min_trust_to_post_images'"
+    execute "UPDATE user_histories SET subject = 'min_trust_to_post_embedded_media' WHERE subject = 'min_trust_to_post_images'"
+  end
+
+  def down
+    execute "UPDATE site_settings SET name = 'newuser_max_images' WHERE name = 'newuser_max_embedded_media'"
+    execute "UPDATE user_histories SET subject = 'newuser_max_images' WHERE subject = 'newuser_max_embedded_media'"
+
+    execute "UPDATE site_settings SET name = 'min_trust_to_post_images' WHERE name = 'min_trust_to_post_embedded_media'"
+    execute "UPDATE user_histories SET subject = 'min_trust_to_post_images' WHERE subject = 'min_trust_to_post_embedded_media'"
+  end
+end

lib/site_settings/deprecated_settings.rb

@@ -6,7 +6,9 @@ module SiteSettings::DeprecatedSettings
   SETTINGS = [
     ['show_email_on_profile', 'moderators_view_emails', true, '2.4'],
     ['allow_moderators_to_create_categories', 'moderators_create_categories', true, '2.4'],
-    ['disable_edit_notifications', 'disable_system_edit_notifications', true, '2.4']
+    ['disable_edit_notifications', 'disable_system_edit_notifications', true, '2.4'],
+    ['newuser_max_images', 'newuser_max_embedded_media', true, '2.7'],
+    ['min_trust_to_post_images', 'min_trust_to_post_embedded_media', true, '2.7']
   ]
 
   def setup_deprecated_methods

lib/validators/post_validator.rb

@@ -11,7 +11,7 @@ class PostValidator < ActiveModel::Validator
     post_body_validator(record)
     max_posts_validator(record)
     max_mention_validator(record)
-    max_images_validator(record)
+    max_embedded_media_validator(record)
     max_attachments_validator(record)
     can_post_links_validator(record)
     unique_post_validator(record)
@@ -72,25 +72,25 @@ class PostValidator < ActiveModel::Validator
     end
   end
 
-  # Ensure new users can not put too many images in a post
-  def max_images_validator(post)
+  # Ensure new users can not put too many media embeds (images, video, audio) in a post
+  def max_embedded_media_validator(post)
     return if post.acting_user.blank? || post.acting_user&.staff?
 
-    if post.acting_user.trust_level < TrustLevel[SiteSetting.min_trust_to_post_images]
+    if post.acting_user.trust_level < TrustLevel[SiteSetting.min_trust_to_post_embedded_media]
       add_error_if_count_exceeded(
         post,
-        :no_images_allowed_trust,
-        :no_images_allowed_trust,
-        post.image_count,
+        :no_embedded_media_allowed_trust,
+        :no_embedded_media_allowed_trust,
+        post.embedded_media_count,
         0
       )
     elsif post.acting_user.trust_level == TrustLevel[0]
       add_error_if_count_exceeded(
         post,
-        :no_images_allowed,
-        :too_many_images,
-        post.image_count,
-        SiteSetting.newuser_max_images
+        :no_embedded_media_allowed,
+        :too_many_embedded_media,
+        post.embedded_media_count,
+        SiteSetting.newuser_max_embedded_media
       )
     end
   end

spec/components/post_revisor_spec.rb

@@ -425,7 +425,7 @@ describe PostRevisor do
       fab!(:changed_by) { Fabricate(:admin) }
 
       before do
-        SiteSetting.newuser_max_images = 0
+        SiteSetting.newuser_max_embedded_media = 0
         url = "http://i.imgur.com/wfn7rgU.jpg"
         Oneboxer.stubs(:onebox).with(url, anything).returns("<img src='#{url}'>")
         subject.revise!(changed_by, raw: "So, post them here!\n#{url}")
@@ -443,7 +443,7 @@ describe PostRevisor do
 
     describe "new user editing their own post" do
       before do
-        SiteSetting.newuser_max_images = 0
+        SiteSetting.newuser_max_embedded_media = 0
         url = "http://i.imgur.com/FGg7Vzu.gif"
         Oneboxer.stubs(:cached_onebox).with(url, anything).returns("<img src='#{url}'>")
         subject.revise!(post.user, raw: "So, post them here!\n#{url}")

spec/components/validators/post_validator_spec.rb

@@ -137,45 +137,45 @@ describe PostValidator do
     end
   end
 
-  context "too_many_images" do
+  context "too_many_embedded_media" do
     before do
-      SiteSetting.min_trust_to_post_images = 0
-      SiteSetting.newuser_max_images = 2
+      SiteSetting.min_trust_to_post_embedded_media = 0
+      SiteSetting.newuser_max_embedded_media = 2
     end
 
     it "should be invalid when new user exceeds max mentions limit" do
       post.acting_user = build(:newuser)
-      post.expects(:image_count).returns(3)
-      validator.max_images_validator(post)
+      post.expects(:embedded_media_count).returns(3)
+      validator.max_embedded_media_validator(post)
       expect(post.errors.count).to be > 0
     end
 
     it "should be valid when new user does not exceed max mentions limit" do
       post.acting_user = build(:newuser)
-      post.expects(:image_count).returns(2)
-      validator.max_images_validator(post)
+      post.expects(:embedded_media_count).returns(2)
+      validator.max_embedded_media_validator(post)
       expect(post.errors.count).to be(0)
     end
 
     it "should be invalid when user trust level is not sufficient" do
-      SiteSetting.min_trust_to_post_images = 4
+      SiteSetting.min_trust_to_post_embedded_media = 4
       post.acting_user = build(:leader)
-      post.expects(:image_count).returns(2)
-      validator.max_images_validator(post)
+      post.expects(:embedded_media_count).returns(2)
+      validator.max_embedded_media_validator(post)
       expect(post.errors.count).to be > 0
     end
 
     it "should be valid for moderator in all cases" do
       post.acting_user = build(:moderator)
-      post.expects(:image_count).never
-      validator.max_images_validator(post)
+      post.expects(:embedded_media_count).never
+      validator.max_embedded_media_validator(post)
       expect(post.errors.count).to be(0)
     end
 
     it "should be valid for admin in all cases" do
       post.acting_user = build(:admin)
-      post.expects(:image_count).never
-      validator.max_images_validator(post)
+      post.expects(:embedded_media_count).never
+      validator.max_embedded_media_validator(post)
       expect(post.errors.count).to be(0)
     end
   end
@@ -290,7 +290,7 @@ describe PostValidator do
       validator.expects(:raw_quality).never
       validator.expects(:max_posts_validator).never
       validator.expects(:max_mention_validator).never
-      validator.expects(:max_images_validator).never
+      validator.expects(:max_embedded_media_validator).never
       validator.expects(:max_attachments_validator).never
       validator.expects(:newuser_links_validator).never
       validator.expects(:unique_post_validator).never

spec/models/post_analyzer_spec.rb

@@ -115,52 +115,58 @@ describe PostAnalyzer do
     end
   end
 
-  describe "image_count" do
+  describe "embedded_media_count" do
     let(:raw_post_one_image_md) { "![sherlock](http://bbc.co.uk/sherlock.jpg)" }
     let(:raw_post_two_images_html) { "<img src='http://discourse.org/logo.png'> <img src='http://bbc.co.uk/sherlock.jpg'>" }
     let(:raw_post_with_avatars) { '<img alt="smiley" title=":smiley:" src="/assets/emoji/smiley.png" class="avatar"> <img alt="wink" title=":wink:" src="/assets/emoji/wink.png" class="avatar">' }
     let(:raw_post_with_favicon) { '<img src="/assets/favicons/wikipedia.png" class="favicon">' }
     let(:raw_post_with_thumbnail) { '<img src="/assets/emoji/smiley.png" class="thumbnail">' }
     let(:raw_post_with_two_classy_images) { "<img src='http://discourse.org/logo.png' class='classy'> <img src='http://bbc.co.uk/sherlock.jpg' class='classy'>" }
+    let(:raw_post_with_two_embedded_media) { '<video width="950" height="700" controls><source src="https://bbc.co.uk/news.mp4" type="video/mp4"></video><audio controls><source type="audio/mpeg" src="https://example.com/audio.mp3"></audio>' }
 
     it "returns 0 images for an empty post" do
       post_analyzer = PostAnalyzer.new("Hello world", nil)
-      expect(post_analyzer.image_count).to eq(0)
+      expect(post_analyzer.embedded_media_count).to eq(0)
     end
 
     it "finds images from markdown" do
       post_analyzer = PostAnalyzer.new(raw_post_one_image_md, default_topic_id)
-      expect(post_analyzer.image_count).to eq(1)
+      expect(post_analyzer.embedded_media_count).to eq(1)
     end
 
     it "finds images from HTML" do
       post_analyzer = PostAnalyzer.new(raw_post_two_images_html, default_topic_id)
-      expect(post_analyzer.image_count).to eq(2)
+      expect(post_analyzer.embedded_media_count).to eq(2)
+    end
+
+    it "finds video and audio from HTML" do
+      post_analyzer = PostAnalyzer.new(raw_post_with_two_embedded_media, default_topic_id)
+      expect(post_analyzer.embedded_media_count).to eq(2)
     end
 
     it "doesn't count avatars as images" do
       post_analyzer = PostAnalyzer.new(raw_post_with_avatars, default_topic_id)
       PrettyText.stubs(:cook).returns(raw_post_with_avatars)
-      expect(post_analyzer.image_count).to eq(0)
+      expect(post_analyzer.embedded_media_count).to eq(0)
     end
 
     it "doesn't count favicons as images" do
       post_analyzer = PostAnalyzer.new(raw_post_with_favicon, default_topic_id)
       PrettyText.stubs(:cook).returns(raw_post_with_favicon)
-      expect(post_analyzer.image_count).to eq(0)
+      expect(post_analyzer.embedded_media_count).to eq(0)
     end
 
     it "doesn't count thumbnails as images" do
       post_analyzer = PostAnalyzer.new(raw_post_with_thumbnail, default_topic_id)
       PrettyText.stubs(:cook).returns(raw_post_with_thumbnail)
-      expect(post_analyzer.image_count).to eq(0)
+      expect(post_analyzer.embedded_media_count).to eq(0)
     end
 
     it "doesn't count allowlisted images" do
       Post.stubs(:allowed_image_classes).returns(["classy"])
       PrettyText.stubs(:cook).returns(raw_post_with_two_classy_images)
       post_analyzer = PostAnalyzer.new(raw_post_with_two_classy_images, default_topic_id)
-      expect(post_analyzer.image_count).to eq(0)
+      expect(post_analyzer.embedded_media_count).to eq(0)
     end
   end
 

spec/models/post_spec.rb

@@ -237,7 +237,7 @@ describe Post do
     end
   end
 
-  describe "maximum images" do
+  describe "maximum media embeds" do
     fab!(:newuser) { Fabricate(:user, trust_level: TrustLevel[0]) }
     let(:post_no_images) { Fabricate.build(:post, post_args.merge(user: newuser)) }
     let(:post_one_image) { post_with_body("![sherlock](http://bbc.co.uk/sherlock.jpg)", newuser) }
@@ -249,78 +249,83 @@ describe Post do
     let(:post_image_within_pre) { post_with_body('<pre><img src="coolimage.png"></pre>', newuser) }
     let(:post_with_thumbnail) { post_with_body('<img src="/assets/emoji/smiley.png" class="thumbnail">', newuser) }
     let(:post_with_two_classy_images) { post_with_body("<img src='http://discourse.org/logo.png' class='classy'> <img src='http://bbc.co.uk/sherlock.jpg' class='classy'>", newuser) }
+    let(:post_with_two_embedded_media) { post_with_body('<video width="950" height="700" controls><source src="https://bbc.co.uk/news.mp4" type="video/mp4"></video><audio controls><source type="audio/mpeg" src="https://example.com/audio.mp3"></audio>', newuser) }
 
     it "returns 0 images for an empty post" do
-      expect(Fabricate.build(:post).image_count).to eq(0)
+      expect(Fabricate.build(:post).embedded_media_count).to eq(0)
     end
 
     it "finds images from markdown" do
-      expect(post_one_image.image_count).to eq(1)
+      expect(post_one_image.embedded_media_count).to eq(1)
     end
 
     it "finds images from HTML" do
-      expect(post_two_images.image_count).to eq(2)
+      expect(post_two_images.embedded_media_count).to eq(2)
     end
 
     it "doesn't count avatars as images" do
-      expect(post_with_avatars.image_count).to eq(0)
+      expect(post_with_avatars.embedded_media_count).to eq(0)
     end
 
     it "allows images by default" do
       expect(post_one_image).to be_valid
     end
 
-    it "doesn't allow more than `min_trust_to_post_images`" do
-      SiteSetting.min_trust_to_post_images = 4
+    it "doesn't allow more than `min_trust_to_post_embedded_media`" do
+      SiteSetting.min_trust_to_post_embedded_media = 4
       post_one_image.user.trust_level = 3
       expect(post_one_image).not_to be_valid
     end
 
-    it "doesn't allow more than `min_trust_to_post_images` in a quote" do
-      SiteSetting.min_trust_to_post_images = 4
+    it "doesn't allow more than `min_trust_to_post_embedded_media` in a quote" do
+      SiteSetting.min_trust_to_post_embedded_media = 4
       post_one_image.user.trust_level = 3
       expect(post_image_within_quote).not_to be_valid
     end
 
-    it "doesn't allow more than `min_trust_to_post_images` in code" do
-      SiteSetting.min_trust_to_post_images = 4
+    it "doesn't allow more than `min_trust_to_post_embedded_media` in code" do
+      SiteSetting.min_trust_to_post_embedded_media = 4
       post_one_image.user.trust_level = 3
       expect(post_image_within_code).not_to be_valid
     end
 
-    it "doesn't allow more than `min_trust_to_post_images` in pre" do
-      SiteSetting.min_trust_to_post_images = 4
+    it "doesn't allow more than `min_trust_to_post_embedded_media` in pre" do
+      SiteSetting.min_trust_to_post_embedded_media = 4
       post_one_image.user.trust_level = 3
       expect(post_image_within_pre).not_to be_valid
     end
 
-    it "doesn't allow more than `min_trust_to_post_images`" do
-      SiteSetting.min_trust_to_post_images = 4
+    it "doesn't allow more than `min_trust_to_post_embedded_media`" do
+      SiteSetting.min_trust_to_post_embedded_media = 4
       post_one_image.user.trust_level = 4
       expect(post_one_image).to be_valid
     end
 
     it "doesn't count favicons as images" do
       PrettyText.stubs(:cook).returns(post_with_favicon.raw)
-      expect(post_with_favicon.image_count).to eq(0)
+      expect(post_with_favicon.embedded_media_count).to eq(0)
     end
 
     it "doesn't count thumbnails as images" do
       PrettyText.stubs(:cook).returns(post_with_thumbnail.raw)
-      expect(post_with_thumbnail.image_count).to eq(0)
+      expect(post_with_thumbnail.embedded_media_count).to eq(0)
     end
 
     it "doesn't count allowlisted images" do
       Post.stubs(:allowed_image_classes).returns(["classy"])
       # I dislike this, but passing in a custom allowlist is hard
       PrettyText.stubs(:cook).returns(post_with_two_classy_images.raw)
-      expect(post_with_two_classy_images.image_count).to eq(0)
+      expect(post_with_two_classy_images.embedded_media_count).to eq(0)
+    end
+
+    it "counts video and audio as embedded media" do
+      expect(post_with_two_embedded_media.embedded_media_count).to eq(2)
     end
 
     context "validation" do
 
       before do
-        SiteSetting.newuser_max_images = 1
+        SiteSetting.newuser_max_embedded_media = 1
       end
 
       context 'newuser' do
@@ -328,10 +333,14 @@ describe Post do
           expect(post_one_image).to be_valid
         end
 
-        it "doesn't allow more than the maximum" do
+        it "doesn't allow more than the maximum number of images" do
           expect(post_two_images).not_to be_valid
         end
 
+        it "doesn't allow more than the maximum number of embedded media items" do
+          expect(post_with_two_embedded_media).not_to be_valid
+        end
+
         it "doesn't allow a new user to edit their post to insert an image" do
           post_no_images.user.trust_level = TrustLevel[0]
           post_no_images.save

test/javascripts/helpers/site-settings.js

@@ -50,7 +50,7 @@ const ORIGINAL_SETTINGS = {
   traditional_markdown_linebreaks: false,
   suppress_reply_directly_below: true,
   suppress_reply_directly_above: true,
-  newuser_max_images: 0,
+  newuser_max_embedded_media: 0,
   newuser_max_attachments: 0,
   display_name_on_posts: true,
   short_progress_text_threshold: 10000,

test/javascripts/lib/uploads-test.js

@@ -38,7 +38,7 @@ QUnit.test("uploading one file", function(assert) {
 });
 
 QUnit.test("new user cannot upload images", function(assert) {
-  this.siteSettings.newuser_max_images = 0;
+  this.siteSettings.newuser_max_embedded_media = 0;
   sandbox.stub(bootbox, "alert");
 
   assert.not(