From 0c0a11b66a0ee3d9011c6793558d927297f6edc5 Mon Sep 17 00:00:00 2001
From: Andrei Prigorshnev <a.prigorshnev@gmail.com>
Date: Thu, 5 Aug 2021 13:38:39 +0400
Subject: [PATCH] FEATURE: Disallow putting urls in the title for TL-0 users
 (#13947)

This disallows putting URLs in topic titles for TL0 users, which means that:

If a TL-0 user puts a link into the title, a topic featured link won't be generated (as if it was disabled in the site settings)
Server methods for creating and updating topics will be refusing featured links when they are called by TL-0 users
TL-0 users won't be able to put any link into the topic title. For example, the title "Hey, take a look at https://my-site.com" will be rejected.

Also, it improves a bit server behavior when creating or updating feature links on topics in the categories with disabled featured links. Before the server just silently ignored a featured link field that was passed to him, now it will be returning 422 response.
---
 .../discourse/app/controllers/topic.js        |  4 +
 .../discourse/app/models/composer.js          | 18 ++++-
 app/models/topic.rb                           |  6 +-
 config/locales/server.en.yml                  |  2 +-
 lib/guardian.rb                               |  4 +
 lib/guardian/topic_guardian.rb                |  2 +-
 lib/post_revisor.rb                           | 31 +++++--
 lib/topic_creator.rb                          |  7 +-
 lib/url_helper.rb                             |  5 ++
 .../urls_in_topic_title_validator.rb          | 20 +++++
 spec/components/guardian_spec.rb              |  2 +-
 spec/components/post_revisor_spec.rb          | 28 +++++++
 spec/fabricators/user_fabricator.rb           |  8 ++
 spec/requests/posts_controller_spec.rb        | 81 +++++++++++++++++++
 spec/requests/topics_controller_spec.rb       | 79 ++++++++++++++++++
 15 files changed, 276 insertions(+), 21 deletions(-)
 create mode 100644 lib/validators/urls_in_topic_title_validator.rb

diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js
index f48dda98231..d41257b8afd 100644
--- a/app/assets/javascripts/discourse/app/controllers/topic.js
+++ b/app/assets/javascripts/discourse/app/controllers/topic.js
@@ -172,6 +172,10 @@ export default Controller.extend(bufferedProperty("model"), {
 
   @discourseComputed("model.isPrivateMessage", "model.category.id")
   canEditTopicFeaturedLink(isPrivateMessage, categoryId) {
+    if (this.currentUser && this.currentUser.trust_level === 0) {
+      return false;
+    }
+
     if (!this.siteSettings.topic_featured_link_enabled || isPrivateMessage) {
       return false;
     }
diff --git a/app/assets/javascripts/discourse/app/models/composer.js b/app/assets/javascripts/discourse/app/models/composer.js
index b93ab8c6e8a..85c69ad880c 100644
--- a/app/assets/javascripts/discourse/app/models/composer.js
+++ b/app/assets/javascripts/discourse/app/models/composer.js
@@ -280,8 +280,22 @@ const Composer = RestModel.extend({
     "notPrivateMessage"
   ),
 
-  @discourseComputed("canEditTitle", "creatingPrivateMessage", "categoryId")
-  canEditTopicFeaturedLink(canEditTitle, creatingPrivateMessage, categoryId) {
+  @discourseComputed(
+    "canEditTitle",
+    "creatingPrivateMessage",
+    "categoryId",
+    "user.trust_level"
+  )
+  canEditTopicFeaturedLink(
+    canEditTitle,
+    creatingPrivateMessage,
+    categoryId,
+    userTrustLevel
+  ) {
+    if (userTrustLevel === 0) {
+      return false;
+    }
+
     if (
       !this.siteSettings.topic_featured_link_enabled ||
       !canEditTitle ||
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 601c9759775..ce78ed1433a 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -164,6 +164,7 @@ class Topic < ActiveRecord::Base
                     topic_title_length: true,
                     censored_words: true,
                     watched_words: true,
+                    urls_in_topic_title: true,
                     quality_title: { unless: :private_message? },
                     max_emojis: true,
                     unique_among: { unless: Proc.new { |t| (SiteSetting.allow_duplicate_topic_titles? || t.private_message?) },
@@ -190,8 +191,9 @@ class Topic < ActiveRecord::Base
 
   validates :featured_link, allow_nil: true, url: true
   validate if: :featured_link do
-    errors.add(:featured_link, :invalid_category) unless !featured_link_changed? ||
-      Guardian.new.can_edit_featured_link?(category_id)
+    if featured_link_changed? && !Guardian.new(user).can_edit_featured_link?(category_id)
+      errors.add(:featured_link)
+    end
   end
 
   before_validation do
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 2d4f846f5da..c5c22c7d75e 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -358,6 +358,7 @@ en:
     other: "Sorry, new users can only put %{count} attachments in a post."
   no_links_allowed: "Sorry, new users can't put links in posts."
   links_require_trust: "Sorry, you can't include links in your posts."
+  urls_in_title_require_trust_level: "Sorry, new users can't include links in topic titles."
   too_many_links:
     one: "Sorry, new users can only put one link in a post."
     other: "Sorry, new users can only put %{count} links in a post."
@@ -579,7 +580,6 @@ en:
               unable_to_tag: "There was an error tagging the topic."
             featured_link:
               invalid: "is invalid. URL should include http:// or https://."
-              invalid_category: "can't be edited in this category."
         user:
           attributes:
             password:
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 54822b95b92..77e32aa3d02 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -540,6 +540,10 @@ class Guardian
     !SiteSetting.login_required? || authenticated?
   end
 
+  def can_put_urls_in_topic_title?
+    @user.trust_level >= TrustLevel.levels[:basic]
+  end
+
   def auth_token
     if cookie = request&.cookies[Auth::DefaultCurrentUserProvider::TOKEN_COOKIE]
       UserAuthToken.hash_token(cookie)
diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb
index 98e51453e7c..a8504cb8b34 100644
--- a/lib/guardian/topic_guardian.rb
+++ b/lib/guardian/topic_guardian.rb
@@ -206,6 +206,7 @@ module TopicGuardian
 
   def can_edit_featured_link?(category_id)
     return false unless SiteSetting.topic_featured_link_enabled
+    return false unless @user.trust_level >= TrustLevel.levels[:basic]
     Category.where(id: category_id || SiteSetting.uncategorized_category_id, topic_featured_link_allowed: true).exists?
   end
 
@@ -251,5 +252,4 @@ module TopicGuardian
   def affected_by_slow_mode?(topic)
     topic&.slow_mode_seconds.to_i > 0 && @user.human? && !is_staff?
   end
-
 end
diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb
index eee848607e7..1cfc60a8dc0 100644
--- a/lib/post_revisor.rb
+++ b/lib/post_revisor.rb
@@ -67,14 +67,28 @@ class PostRevisor
     end
   end
 
-  # Fields we want to record revisions for by default
-  %i{title archetype}.each do |field|
-    track_topic_field(field) do |tc, attribute|
-      tc.record_change(field, tc.topic.public_send(field), attribute)
-      tc.topic.public_send("#{field}=", attribute)
+  def self.track_and_revise(topic_changes, field, attribute)
+    topic_changes.record_change(
+      field,
+      topic_changes.topic.public_send(field),
+      attribute
+    )
+    topic_changes.topic.public_send("#{field}=", attribute)
+  end
+
+  track_topic_field(:title) do |topic_changes, attribute|
+    if UrlHelper.contains_url?(attribute) && !topic_changes.guardian.can_put_urls_in_topic_title?
+      topic_changes.topic.errors.add(:base, I18n.t("urls_in_title_require_trust_level"))
+      topic_changes.check_result(false)
+    else
+      track_and_revise topic_changes, :title, attribute
     end
   end
 
+  track_topic_field(:archetype) do |topic_changes, attribute|
+    track_and_revise topic_changes, :archetype, attribute
+  end
+
   track_topic_field(:category_id) do |tc, category_id, fields|
     if category_id == 0 && tc.topic.private_message?
       tc.record_change('category_id', tc.topic.category_id, nil)
@@ -111,9 +125,10 @@ class PostRevisor
   end
 
   track_topic_field(:featured_link) do |topic_changes, featured_link|
-    if SiteSetting.topic_featured_link_enabled &&
-       topic_changes.guardian.can_edit_featured_link?(topic_changes.topic.category_id)
-
+    if !SiteSetting.topic_featured_link_enabled ||
+      !topic_changes.guardian.can_edit_featured_link?(topic_changes.topic.category_id)
+      topic_changes.check_result(false)
+    else
       topic_changes.record_change('featured_link', topic_changes.topic.featured_link, featured_link)
       topic_changes.topic.featured_link = featured_link
     end
diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb
index 7ba59e945aa..a250ef69fbf 100644
--- a/lib/topic_creator.rb
+++ b/lib/topic_creator.rb
@@ -132,15 +132,10 @@ class TopicCreator
     end
 
     topic_params[:category_id] = category.id if category.present?
-
     topic_params[:created_at] = convert_time(@opts[:created_at]) if @opts[:created_at].present?
-
     topic_params[:pinned_at] = convert_time(@opts[:pinned_at]) if @opts[:pinned_at].present?
     topic_params[:pinned_globally] = @opts[:pinned_globally] if @opts[:pinned_globally].present?
-
-    if SiteSetting.topic_featured_link_enabled && @opts[:featured_link].present? && @guardian.can_edit_featured_link?(topic_params[:category_id])
-      topic_params[:featured_link] = @opts[:featured_link]
-    end
+    topic_params[:featured_link] = @opts[:featured_link]
 
     topic_params
   end
diff --git a/lib/url_helper.rb b/lib/url_helper.rb
index b1f10df26cd..f55b40e02ab 100644
--- a/lib/url_helper.rb
+++ b/lib/url_helper.rb
@@ -65,6 +65,11 @@ class UrlHelper
     Addressable::URI.normalized_encode(uri)
   end
 
+  def self.contains_url?(string)
+    uri_regexp = Discourse::Utils::URI_REGEXP
+    uri_regexp.match?(string)
+  end
+
   def self.rails_route_from_url(url)
     path = URI.parse(encode(url)).path
     Rails.application.routes.recognize_path(path)
diff --git a/lib/validators/urls_in_topic_title_validator.rb b/lib/validators/urls_in_topic_title_validator.rb
new file mode 100644
index 00000000000..1bb03bc95b1
--- /dev/null
+++ b/lib/validators/urls_in_topic_title_validator.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class UrlsInTopicTitleValidator < ActiveModel::Validator
+  def validate(record)
+    if UrlHelper.contains_url?(record.title) && !can_put_urls?(record)
+      record.errors.add(:base, error_message)
+    end
+  end
+
+  private
+
+  def can_put_urls?(topic)
+    guardian = Guardian.new(topic.acting_user)
+    guardian.can_put_urls_in_topic_title?
+  end
+
+  def error_message
+    I18n.t("urls_in_title_require_trust_level")
+  end
+end
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index e07f63f5e81..9695a98da90 100644
--- a/spec/components/guardian_spec.rb
+++ b/spec/components/guardian_spec.rb
@@ -3688,7 +3688,7 @@ describe Guardian do
 
   context 'topic featured link category restriction' do
     before { SiteSetting.topic_featured_link_enabled = true }
-    let(:guardian) { Guardian.new }
+    let(:guardian) { Guardian.new(user) }
     let(:uncategorized) { Category.find(SiteSetting.uncategorized_category_id) }
 
     context "uncategorized" do
diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb
index fb748471aba..59bd2ad2a6b 100644
--- a/spec/components/post_revisor_spec.rb
+++ b/spec/components/post_revisor_spec.rb
@@ -661,6 +661,34 @@ describe PostRevisor do
       expect(post.raw).to eq("    <-- whitespaces -->")
     end
 
+    it "revises and tracks changes of topic titles" do
+      new_title = "New topic title"
+      result = subject.revise!(
+        post.user,
+        { title: new_title },
+        revised_at: post.updated_at + 10.minutes
+      )
+
+      expect(result).to eq(true)
+      post.reload
+      expect(post.topic.title).to eq(new_title)
+      expect(post.revisions.first.modifications["title"][1]).to eq(new_title)
+    end
+
+    it "revises and tracks changes of topic archetypes" do
+      new_archetype = Archetype.banner
+      result = subject.revise!(
+        post.user,
+        { archetype: new_archetype },
+        revised_at: post.updated_at + 10.minutes
+      )
+
+      expect(result).to eq(true)
+      post.reload
+      expect(post.topic.archetype).to eq(new_archetype)
+      expect(post.revisions.first.modifications["archetype"][1]).to eq(new_archetype)
+    end
+
     context "#publish_changes" do
       let!(:post) { Fabricate(:post, topic: topic) }
 
diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb
index 759a5a43d3e..9c60bc95f7d 100644
--- a/spec/fabricators/user_fabricator.rb
+++ b/spec/fabricators/user_fabricator.rb
@@ -87,6 +87,14 @@ Fabricator(:leader, from: :user) do
   trust_level TrustLevel[3]
 end
 
+Fabricator(:trust_level_0, from: :user) do
+  trust_level TrustLevel[0]
+end
+
+Fabricator(:trust_level_1, from: :user) do
+  trust_level TrustLevel[1]
+end
+
 Fabricator(:trust_level_4, from: :user) do
   name 'Leader McElderson'
   username { sequence(:username) { |i| "tl4#{i}" } }
diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb
index 5c0c00d7044..424fa1279f2 100644
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@@ -81,6 +81,8 @@ describe PostsController do
   fab!(:admin) { Fabricate(:admin) }
   fab!(:moderator) { Fabricate(:moderator) }
   fab!(:user) { Fabricate(:user) }
+  fab!(:user_trust_level_0) { Fabricate(:trust_level_0) }
+  fab!(:user_trust_level_1) { Fabricate(:trust_level_1) }
   fab!(:category) { Fabricate(:category) }
   fab!(:topic) { Fabricate(:topic) }
   fab!(:post_by_user) { Fabricate(:post, user: user) }
@@ -1457,6 +1459,85 @@ describe PostsController do
       end
     end
 
+    describe "urls in the title" do
+      let!(:title_with_url) { "A title with the URL https://google.com" }
+
+      it "doesn't allow TL0 users to put urls into the title" do
+        sign_in(user_trust_level_0)
+
+        post "/posts.json", params: {
+          raw: 'this is the test content',
+          title: title_with_url
+        }
+
+        expect(response.status).to eq(422)
+        expect(response.body).to include(I18n.t('urls_in_title_require_trust_level'))
+      end
+
+      it "allows TL1 users to put urls into the title" do
+        sign_in(user_trust_level_1)
+
+        post "/posts.json", params: {
+          raw: 'this is the test content',
+          title: title_with_url
+        }
+
+        expect(response.status).to eq(200)
+      end
+    end
+
+    describe "featured links" do
+      it "allows to create topics with featured links" do
+        sign_in(user_trust_level_1)
+
+        post "/posts.json", params: {
+          title: "this is the test title for the topic",
+          raw: "this is the test content",
+          featured_link: "https://discourse.org"
+        }
+
+        expect(response.status).to eq(200)
+      end
+
+      it "doesn't allow TL0 users to create topics with featured links" do
+        sign_in(user_trust_level_0)
+
+        post "/posts.json", params: {
+          title: "this is the test title for the topic",
+          raw: "this is the test content",
+          featured_link: "https://discourse.org"
+        }
+
+        expect(response.status).to eq(422)
+      end
+
+      it "doesn't allow to create topics with featured links if featured links are disabled in settings" do
+        SiteSetting.topic_featured_link_enabled = false
+        sign_in(user_trust_level_1)
+
+        post "/posts.json", params: {
+          title: "this is the test title for the topic",
+          raw: "this is the test content",
+          featured_link: "https://discourse.org"
+        }
+
+        expect(response.status).to eq(422)
+      end
+
+      it "doesn't allow to create topics with featured links in the category with forbidden feature links" do
+        category = Fabricate(:category, topic_featured_link_allowed: false)
+        sign_in(user_trust_level_1)
+
+        post "/posts.json", params: {
+          title: "this is the test title for the topic",
+          raw: "this is the test content",
+          featured_link: "https://discourse.org",
+          category: category.id
+        }
+
+        expect(response.status).to eq(422)
+      end
+    end
   end
 
   describe '#revisions' do
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index 46bcfab2e00..9cd8c369c7b 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -9,6 +9,8 @@ RSpec.describe TopicsController do
   fab!(:user) { Fabricate(:user) }
   fab!(:moderator) { Fabricate(:moderator) }
   fab!(:admin) { Fabricate(:admin) }
+  fab!(:trust_level_0) { Fabricate(:trust_level_0) }
+  fab!(:trust_level_1) { Fabricate(:trust_level_1) }
   fab!(:trust_level_4) { Fabricate(:trust_level_4) }
 
   fab!(:category) { Fabricate(:category) }
@@ -1650,6 +1652,83 @@ RSpec.describe TopicsController do
         end
       end
     end
+
+    describe "urls in the title" do
+      let!(:title_with_url) { "A title with the URL https://google.com" }
+
+      it "doesn't allow TL0 users to put urls into the title" do
+        sign_in(trust_level_0)
+        topic = Fabricate(:topic, user: trust_level_0)
+        Fabricate(:post, topic: topic)
+        put "/t/#{topic.slug}/#{topic.id}.json", params: { title: title_with_url }
+
+        expect(response.status).to eq(422)
+        expect(response.body).to include(I18n.t('urls_in_title_require_trust_level'))
+      end
+
+      it "allows TL1 users to put urls into the title" do
+        sign_in(trust_level_1)
+        topic = Fabricate(:topic, user: trust_level_1)
+        Fabricate(:post, topic: topic)
+        put "/t/#{topic.slug}/#{topic.id}.json", params: { title: title_with_url }
+
+        expect(response.status).to eq(200)
+      end
+    end
+
+    describe "featured links" do
+      def fabricate_topic(user, category = nil)
+        topic = Fabricate(:topic, user: user, category: category)
+        Fabricate(:post, topic: topic)
+        topic
+      end
+
+      it "allows to update topic featured link" do
+        sign_in(trust_level_1)
+
+        topic = fabricate_topic(trust_level_1)
+        put "/t/#{topic.slug}/#{topic.id}.json", params: {
+          featured_link: "https://discourse.org"
+        }
+
+        expect(response.status).to eq(200)
+      end
+
+      it "doesn't allow TL0 users to update topic featured link" do
+        sign_in(trust_level_0)
+
+        topic = fabricate_topic(trust_level_0)
+        put "/t/#{topic.slug}/#{topic.id}.json", params: {
+          featured_link: "https://discourse.org"
+        }
+
+        expect(response.status).to eq(422)
+      end
+
+      it "doesn't allow to update topic featured link if featured links are disabled in settings" do
+        sign_in(trust_level_1)
+
+        SiteSetting.topic_featured_link_enabled = false
+        topic = fabricate_topic(trust_level_1)
+        put "/t/#{topic.slug}/#{topic.id}.json", params: {
+          featured_link: "https://discourse.org"
+        }
+
+        expect(response.status).to eq(422)
+      end
+
+      it "doesn't allow to update topic featured link in the category with forbidden feature links" do
+        sign_in(trust_level_1)
+
+        category = Fabricate(:category, topic_featured_link_allowed: false)
+        topic = fabricate_topic(trust_level_1, category)
+        put "/t/#{topic.slug}/#{topic.id}.json", params: {
+          featured_link: "https://discourse.org"
+        }
+
+        expect(response.status).to eq(422)
+      end
+    end
   end
 
   describe '#show' do