Merge pull request #83 from AyuntamientoMadrid/tag-security-15

Tag security

app/models/debate.rb

@@ -13,6 +13,7 @@ class Debate < ActiveRecord::Base
   validates :terms_of_service, acceptance: { allow_nil: false }, on: :create
 
   before_validation :sanitize_description
+  before_validation :sanitize_tag_list
 
   def likes
     get_likes.size
@@ -40,4 +41,8 @@ class Debate < ActiveRecord::Base
     self.description = WYSIWYGSanitizer.new.sanitize(description)
   end
 
+  def sanitize_tag_list
+    self.tag_list  = TagSanitizer.new.sanitize_tag_list(self.tag_list)
+  end
+
 end

lib/tag_sanitizer.rb

@@ -0,0 +1,17 @@
+class TagSanitizer
+
+  DISALLOWED_STRINGS = %w(? < > = /)
+
+  def sanitize_tag(tag)
+    tag = tag.dup
+    DISALLOWED_STRINGS.each do |s|
+      tag.gsub!(s, '')
+    end
+    tag
+  end
+
+  def sanitize_tag_list(tag_list)
+    tag_list.map { |tag| sanitize_tag(tag) }
+  end
+
+end

spec/features/debates_spec.rb

@@ -62,6 +62,27 @@ feature 'Debates' do
     expect(page.html).to_not include '<script>alert("an attack");</script>'
   end
 
+  scenario 'tagging using dangerous strings' do
+
+    author = create(:user)
+    login_as(author)
+
+    visit new_debate_path
+
+    fill_in 'debate_title', with: 'A test'
+    fill_in 'debate_description', with: 'A test'
+    fill_in 'debate_tag_list', with: 'user_id=1, &a=3, <script>alert("hey");</script>'
+    check 'debate_terms_of_service'
+
+    click_button 'Create Debate'
+
+    expect(page).to have_content 'Debate was successfully created.'
+    expect(page).to have_content 'user_id1'
+    expect(page).to have_content 'a3'
+    expect(page).to have_content 'scriptalert("hey");script'
+    expect(page.html).to_not include 'user_id=1, &a=3, <script>alert("hey");</script>'
+  end
+
   scenario 'Update should not be posible if logged user is not the author' do
     debate = create(:debate)
     expect(debate).to be_editable

spec/lib/tag_sanitizer_spec.rb

@@ -0,0 +1,23 @@
+require 'rails_helper'
+
+describe TagSanitizer do
+
+  subject { described_class.new }
+
+  describe '#sanitize_tag' do
+    it 'allows regular text, even spaces' do
+      expect(subject.sanitize_tag('hello there')).to eq('hello there')
+    end
+
+    it 'filters out dangerous strings' do
+      expect(subject.sanitize_tag('user_id=1')).to eq('user_id1')
+    end
+  end
+
+  describe '#sanitize_tag_list' do
+    it 'returns a new tag list with sanitized tags' do
+      expect(subject.sanitize_tag_list(%w{x=1 y?z})).to eq(%w(x1 yz))
+    end
+  end
+
+end

spec/models/debate_spec.rb

@@ -31,6 +31,12 @@ describe Debate do
     expect(@debate.description).to eq("alert('danger');")
   end
 
+  it "should sanitize the tag list" do
+    @debate.tag_list = "user_id=1"
+    @debate.valid?
+    expect(@debate.tag_list).to eq(['user_id1'])
+  end
+
   it "should not be valid without accepting terms of service" do
     @debate.terms_of_service = nil
     expect(@debate).to_not be_valid