FIX: prevent creation of tags with invalid characters

app/controllers/tags_controller.rb

@@ -43,8 +43,8 @@ class TagsController < ::ApplicationController
   # TODO: move all this to ListController
   Discourse.filters.each do |filter|
     define_method("show_#{filter}") do
-      @tag_id = DiscourseTagging.clean_tag(params[:tag_id])
-      @additional_tags = params[:additional_tag_ids].to_s.split('/').map { |tag| DiscourseTagging.clean_tag(tag) }
+      @tag_id = params[:tag_id]
+      @additional_tags = params[:additional_tag_ids].to_s.split('/')
 
       page = params[:page].to_i
       list_opts = build_topic_list_options
@@ -118,7 +118,7 @@ class TagsController < ::ApplicationController
   def tag_feed
     discourse_expires_in 1.minute
 
-    tag_id = DiscourseTagging.clean_tag(params[:tag_id])
+    tag_id = params[:tag_id]
     @link = "#{Discourse.base_url}/tags/#{tag_id}"
     @description = I18n.t("rss_by_tag", tag: tag_id)
     @title = "#{SiteSetting.title} - #{@description}"
@@ -159,8 +159,7 @@ class TagsController < ::ApplicationController
 
     json_response = { results: tags }
 
-    t = DiscourseTagging.clean_tag(params[:q])
-    if Tag.where(name: t).exists? && !tags.find { |h| h[:id] == t }
+    if Tag.where(name: params[:q]).exists? && !tags.find { |h| h[:id] == t }
       # filter_allowed_tags determined that the tag entered is not allowed
       json_response[:forbidden] = t
     end

lib/discourse_tagging.rb

@@ -1,7 +1,9 @@
+require_dependency 'slug'
+
 module DiscourseTagging
 
   TAGS_FIELD_NAME = "tags"
-  TAGS_FILTER_REGEXP = /[<\\\/\>\#\?\&\s]/
+  TAGS_FILTER_REGEXP = Slug::CHAR_FILTER_REGEXP
 
 
   def self.tag_topic_by_names(topic, guardian, tag_names_arg)
@@ -141,7 +143,7 @@ module DiscourseTagging
   end
 
   def self.clean_tag(tag)
-    tag.downcase.strip[0...SiteSetting.max_tag_length].gsub(TAGS_FILTER_REGEXP, '')
+    Slug.sanitize(tag.downcase.strip)[0...SiteSetting.max_tag_length]
   end
 
   def self.staff_only_tags(tags)
@@ -155,19 +157,16 @@ module DiscourseTagging
     tag_diff.present? ? tag_diff : nil
   end
 
-  def self.tags_for_saving(tags, guardian, opts={})
+  def self.tags_for_saving(tags_arg, guardian, opts={})
 
-    return [] unless guardian.can_tag_topics?
+    return [] unless guardian.can_tag_topics? && tags_arg.present?
 
-    return unless tags.present?
+    tag_names = Tag.where(name: tags_arg).pluck(:name)
 
-    tag_names = tags.map {|t| clean_tag(t) }
+    if guardian.can_create_tag?
+      tag_names += (tags_arg - tag_names).map { |t| clean_tag(t) }
       tag_names.delete_if { |t| t.blank? }
       tag_names.uniq!
-
-    # If the user can't create tags, remove any tags that don't already exist
-    unless guardian.can_create_tag?
-      tag_names = Tag.where(name: tag_names).pluck(:name)
     end
 
     return opts[:unlimited] ? tag_names : tag_names[0...SiteSetting.max_tags_per_topic]

lib/slug.rb

@@ -2,6 +2,8 @@
 
 module Slug
 
+  CHAR_FILTER_REGEXP = /[:\/\?#\[\]@!\$&'\(\)\*\+,;=_\.~%\\`^\s|\{\}"<>]+/ # :/?#[]@!$&'()*+,;=_.~%\`^|{}"<>
+
   def self.for(string, default = 'topic')
     slug = case (SiteSetting.slug_generation_method || :ascii).to_sym
            when :ascii then self.ascii_generator(string)
@@ -31,7 +33,7 @@ module Slug
     # See also URI::REGEXP::PATTERN.
     string.strip
           .gsub(/\s+/, '-')
-          .gsub(/[:\/\?#\[\]@!\$&'\(\)\*\+,;=_\.~%\\`^\s|\{\}"<>]+/, '') # :/?#[]@!$&'()*+,;=_.~%\`^|{}"<>
+          .gsub(CHAR_FILTER_REGEXP, '')
           .gsub(/\A-+|-+\z/, '') # remove possible trailing and preceding dashes
           .squeeze('-') # squeeze continuous dashes to prettify slug
   end

spec/discourse_tagging_spec.rb

@@ -0,0 +1,54 @@
+require 'rails_helper'
+require_dependency 'discourse_tagging'
+
+describe DiscourseTagging do
+  let(:user)     { Fabricate(:user) }
+  let(:guardian) { Guardian.new(user) }
+
+  describe '#tags_for_saving' do
+    it "returns empty array if input is nil" do
+      expect(described_class.tags_for_saving(nil, guardian)).to eq([])
+    end
+
+    it "returns empty array if input is empty" do
+      expect(described_class.tags_for_saving([], guardian)).to eq([])
+    end
+
+    it "returns empty array if can't tag topics" do
+      guardian.stubs(:can_tag_topics?).returns(false)
+      expect(described_class.tags_for_saving(['newtag'], guardian)).to eq([])
+    end
+
+    context "can tag topics but not create tags" do
+      before do
+        guardian.stubs(:can_create_tag?).returns(false)
+        guardian.stubs(:can_tag_topics?).returns(true)
+      end
+
+      it "returns empty array if all tags are new" do
+        expect(described_class.tags_for_saving(['newtag', 'newtagplz'], guardian)).to eq([])
+      end
+
+      it "returns only existing tag names" do
+        Fabricate(:tag, name: 'oldtag')
+        expect(described_class.tags_for_saving(['newtag', 'oldtag'], guardian).try(:sort)).to eq(['oldtag'])
+      end
+    end
+
+    context "can tag topics and create tags" do
+      before do
+        guardian.stubs(:can_create_tag?).returns(true)
+        guardian.stubs(:can_tag_topics?).returns(true)
+      end
+
+      it "returns given tag names if can create new tags and tag topics" do
+        expect(described_class.tags_for_saving(['newtag1', 'newtag2'], guardian).try(:sort)).to eq(['newtag1', 'newtag2'])
+      end
+
+      it "only sanitizes new tags" do # for backwards compat
+        Tag.new(name: 'math=fun').save(validate: false)
+        expect(described_class.tags_for_saving(['math=fun', 'fun*2@gmail.com'], guardian).try(:sort)).to eq(['math=fun', 'fun2gmailcom'].sort)
+      end
+    end
+  end
+end