From 3593e582a3395d18c9d76fcc75d91e7a47447b0a Mon Sep 17 00:00:00 2001 From: jbrw Date: Fri, 7 Aug 2020 12:08:59 -0400 Subject: [PATCH] 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 | 2 +- app/models/post_analyzer.rb | 5 +- config/locales/server.en.yml | 14 +++--- config/site_settings.yml | 4 +- ...5163400_rename_post_image_site_settings.rb | 19 +++++++ lib/site_settings/deprecated_settings.rb | 4 +- lib/validators/post_validator.rb | 22 ++++----- spec/components/post_revisor_spec.rb | 4 +- .../validators/post_validator_spec.rb | 30 ++++++------ spec/models/post_analyzer_spec.rb | 22 ++++++--- spec/models/post_spec.rb | 49 +++++++++++-------- test/javascripts/helpers/site-settings.js | 2 +- test/javascripts/lib/uploads-test.js | 2 +- 13 files changed, 108 insertions(+), 71 deletions(-) create mode 100644 db/migrate/20200805163400_rename_post_image_site_settings.rb diff --git a/app/models/post.rb b/app/models/post.rb index 9a003d3188..8bfe3cb8f1 100644 --- a/app/models/post.rb +++ b/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 diff --git a/app/models/post_analyzer.rb b/app/models/post_analyzer.rb index 941c7f5449..e56d1f7c49 100644 --- a/app/models/post_analyzer.rb +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 65afb842e2..bef10efd4c 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/config/site_settings.yml b/config/site_settings.yml index 375834f7d1..0ccf92515c 100644 --- a/config/site_settings.yml +++ b/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 diff --git a/db/migrate/20200805163400_rename_post_image_site_settings.rb b/db/migrate/20200805163400_rename_post_image_site_settings.rb new file mode 100644 index 0000000000..d97752a00c --- /dev/null +++ b/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 diff --git a/lib/site_settings/deprecated_settings.rb b/lib/site_settings/deprecated_settings.rb index a414c7ef91..a5bb6ed026 100644 --- a/lib/site_settings/deprecated_settings.rb +++ b/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 diff --git a/lib/validators/post_validator.rb b/lib/validators/post_validator.rb index 5b16002660..8428bed841 100644 --- a/lib/validators/post_validator.rb +++ b/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 diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb index bb8c075175..78ee3c2944 100644 --- a/spec/components/post_revisor_spec.rb +++ b/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("") 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("") subject.revise!(post.user, raw: "So, post them here!\n#{url}") diff --git a/spec/components/validators/post_validator_spec.rb b/spec/components/validators/post_validator_spec.rb index 36db615006..30c7e3a0d3 100644 --- a/spec/components/validators/post_validator_spec.rb +++ b/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 diff --git a/spec/models/post_analyzer_spec.rb b/spec/models/post_analyzer_spec.rb index f0dd58d814..dccd31597c 100644 --- a/spec/models/post_analyzer_spec.rb +++ b/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) { " " } let(:raw_post_with_avatars) { 'smiley wink' } let(:raw_post_with_favicon) { '' } let(:raw_post_with_thumbnail) { '' } let(:raw_post_with_two_classy_images) { " " } + let(:raw_post_with_two_embedded_media) { '' } 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 diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 559b1ff06e..2e3f44d183 100644 --- a/spec/models/post_spec.rb +++ b/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('
', newuser) } let(:post_with_thumbnail) { post_with_body('', newuser) } let(:post_with_two_classy_images) { post_with_body(" ", newuser) } + let(:post_with_two_embedded_media) { post_with_body('', 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 diff --git a/test/javascripts/helpers/site-settings.js b/test/javascripts/helpers/site-settings.js index 4622399394..dd040c9066 100644 --- a/test/javascripts/helpers/site-settings.js +++ b/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, diff --git a/test/javascripts/lib/uploads-test.js b/test/javascripts/lib/uploads-test.js index 16063e840c..fae70421fb 100644 --- a/test/javascripts/lib/uploads-test.js +++ b/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(