From 0328141e058206eb096f8f6c90d038f2a06f2d06 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Wed, 12 Oct 2016 15:44:26 -0400 Subject: [PATCH] FIX: prevent creation of tags with invalid characters --- app/controllers/tags_controller.rb | 9 +++-- lib/discourse_tagging.rb | 23 ++++++------- lib/slug.rb | 4 ++- spec/discourse_tagging_spec.rb | 54 ++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 18 deletions(-) create mode 100644 spec/discourse_tagging_spec.rb diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 71d434d8b8e..bbf7a69c3ee 100644 --- a/app/controllers/tags_controller.rb +++ b/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 diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 9cf89af15c7..42753628a9b 100644 --- a/lib/discourse_tagging.rb +++ b/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) } - 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) + 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! end return opts[:unlimited] ? tag_names : tag_names[0...SiteSetting.max_tags_per_topic] diff --git a/lib/slug.rb b/lib/slug.rb index 133a4e1bd1b..0f3b62b3f07 100644 --- a/lib/slug.rb +++ b/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 diff --git a/spec/discourse_tagging_spec.rb b/spec/discourse_tagging_spec.rb new file mode 100644 index 00000000000..80ac2caa13d --- /dev/null +++ b/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