SRCH-1771 block html injection (#656)

SRCH-1771 - block html injection

app/models/routed_query_keyword.rb

@@ -1,7 +1,10 @@
+# frozen_string_literal: true
+
 class RoutedQueryKeyword < ApplicationRecord
   include Dupable
 
   before_validation do |record|
+    AttributeProcessor.sanitize_attributes record, :keyword
     AttributeProcessor.squish_attributes record,
                                          :keyword,
                                          assign_nil_on_blank: true
@@ -11,30 +14,39 @@ class RoutedQueryKeyword < ApplicationRecord
   belongs_to :routed_query, inverse_of: :routed_query_keywords
   validates :routed_query, presence: true
 
-  validates_presence_of :keyword
-  validates_uniqueness_of :keyword, scope: :routed_query_id, case_sensitive: false
+  validates :keyword, presence: true
+  validates :keyword, uniqueness: {
+    scope: :routed_query_id,
+    case_sensitive: false
+  }
 
   validate :keyword_unique_to_affiliate
 
   def self.do_not_dup_attributes
-    @@do_not_dup_attributes ||= %w(routed_query_id).freeze
+    @do_not_dup_attributes ||= %w[routed_query_id].freeze
   end
 
   def label
     keyword
   end
 
   def keyword_unique_to_affiliate
-    return unless routed_query && routed_query.affiliate
+    return unless routed_query&.affiliate
+    return unless relation.any?
+
+    errors[:keyword] <<
+      "The keyword '#{keyword}' is already in use for a different routed query"
+  end
 
-    relation = routed_query.affiliate.routed_queries
-               .joins(:routed_query_keywords)
-               .where('routed_query_keywords.keyword = ?', keyword)
+  def relation
+    relation = routed_query.
+      affiliate.
+      routed_queries.
+      joins(:routed_query_keywords).
+      where('routed_query_keywords.keyword = ?', keyword)
 
     relation = relation.where('routed_query_id != ?', routed_query_id) if routed_query_id
 
-    if relation.any?
-      errors[:keyword] << "The keyword '#{keyword}' is already in use for a different routed query"
-    end
+    relation
   end
 end

spec/models/routed_query_keyword_spec.rb

@@ -1,4 +1,4 @@
-require 'spec_helper'
+# frozen_string_literal: true
 
 describe RoutedQueryKeyword do
   fixtures :affiliates, :routed_queries, :routed_query_keywords
@@ -10,52 +10,74 @@
     it { is_expected.to validate_presence_of :keyword }
     it { is_expected.to validate_uniqueness_of(:keyword).scoped_to(:routed_query_id).case_insensitive }
 
-    it 'should create a new instance given valid attributes' do
+    it 'creates a new instance given valid attributes' do
       routed_query.routed_query_keywords.create!(keyword: 'route me')
     end
 
-    it 'should downcase, squish and strip whitespace from keyword before inserting in DB' do
+    it 'downcases, squish and strip whitespace from keyword before inserting in DB' do
       keyword = ' leading          and trailing whitespaces AND CAPITALS'
       rqk = routed_query.routed_query_keywords.create!(keyword: keyword)
       expect(rqk.keyword).to eq('leading and trailing whitespaces and capitals')
     end
 
-    it 'should not allow the same keyword to be reused within a single affiliate' do
+    it 'does not allow the same keyword to be reused within a single affiliate' do
       routed_queries(:unclaimed_money).routed_query_keywords.create!(keyword: 'route me')
-      rqk = routed_queries(:moar_unclaimed_money).routed_query_keywords.build(keyword: 'route me')
+      rqk = routed_queries(:moar_unclaimed_money).routed_query_keywords.build(
+        keyword: 'route me'
+      )
+      expected_error =
+        'The keyword \'route me\' is already in use for a different routed query'
+
       expect(rqk).not_to be_valid
-      expect(rqk.errors[:keyword]).to eq(["The keyword 'route me' is already in use for a different routed query"])
+      expect(rqk.errors[:keyword]).to eq([expected_error])
     end
 
     it 'creates a matching SaytSuggestion' do
       routed_query.routed_query_keywords.create!(keyword: 'route me')
-      sayt_suggestion = SaytSuggestion.find_by_affiliate_id_and_phrase_and_is_protected(routed_query.affiliate.id, 'route me', true)
+      sayt_suggestion = SaytSuggestion.find_by(affiliate_id: routed_query.affiliate.id,
+                                               phrase: 'route me',
+                                               is_protected: true)
       expect(sayt_suggestion).to be_present
     end
+
+    context 'when a keyword contains html' do
+      let(:keyword) do
+        routed_query.routed_query_keywords.create!(
+          keyword: 'keyword <div>with html'
+        )
+      end
+
+      it 'strips the html from the keyword' do
+        expect(keyword.keyword).to eq('keyword with html')
+      end
+    end
   end
 
   describe 'Updating a keyword' do
     before do
       keyword = routed_query.routed_query_keywords.create!(keyword: 'initial keyword')
-      keyword.update_attribute(:keyword, 'updated keyword')
+      keyword.update(keyword: 'updated keyword')
     end
 
     it 'updates the matching SaytSuggestion' do
-      sayt_suggestion = SaytSuggestion.find_by_affiliate_id_and_phrase_and_is_protected(routed_query.affiliate.id, 'updated keyword', true)
+      sayt_suggestion = SaytSuggestion.find_by(affiliate_id: routed_query.affiliate.id,
+                                               phrase: 'updated keyword',
+                                               is_protected: true)
       expect(sayt_suggestion).to be_present
     end
   end
 
   describe 'deleting a keyword' do
     before do
       routed_query.routed_query_keywords.create!(keyword: 'some keyword')
-      sayt_suggestion = SaytSuggestion.find_by_affiliate_id_and_phrase_and_is_protected(routed_query.affiliate.id, 'some keyword', true)
-      expect(sayt_suggestion).to be_present
     end
 
     it 'deletes the matching SaytSuggestion' do
-      RoutedQueryKeyword.last.destroy
-      sayt_suggestion = SaytSuggestion.find_by_affiliate_id_and_phrase_and_is_protected(routed_query.affiliate.id, 'some keyword', true)
+      described_class.last.destroy
+      sayt_suggestion = SaytSuggestion.find_by(affiliate_id: routed_query.affiliate.id,
+                                               phrase: 'some keyword',
+                                               is_protected: true)
+
       expect(sayt_suggestion).to be_nil
     end
   end
@@ -68,6 +90,7 @@
 
   describe '#dup' do
     subject(:original_instance) { routed_query_keywords(:one) }
-    include_examples 'dupable', %w(routed_query_id)
+
+    include_examples 'dupable', %w[routed_query_id]
   end
 end

spec/models/routed_query_spec.rb

@@ -1,75 +1,106 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 describe RoutedQuery do
   fixtures :affiliates, :routed_queries
 
+  subject(:routed_query) { described_class.new }
+
   describe 'Creating new instance' do
     let(:affiliate) { affiliates(:basic_affiliate) }
     let(:valid_attributes) do
       {
         description: 'Some desc',
         url: 'http://www.gov.gov/url.html',
-        routed_query_keywords_attributes: { '0' => { 'keyword' => 'some keyword phrase' } }
+        routed_query_keywords_attributes: {
+          '0' => { 'keyword' => 'some keyword phrase' }
+        }
       }
     end
 
     it { is_expected.to belong_to :affiliate }
-    it { is_expected.to have_many(:routed_query_keywords).dependent(:destroy).inverse_of(:routed_query)}
+
+    it 'has keywords' do
+      expect(routed_query).to have_many(:routed_query_keywords).
+        dependent(:destroy).
+        inverse_of(:routed_query)
+    end
+
     it { is_expected.to validate_presence_of :description }
     it { is_expected.to validate_uniqueness_of(:description).scoped_to(:affiliate_id) }
     it { is_expected.to validate_presence_of :affiliate }
     it { is_expected.to allow_value('http://www.foo.com').for(:url) }
     it { is_expected.not_to allow_value('www.foo.com').for(:url) }
 
-    it 'should create a new instance given valid attributes' do
+    it 'creates a new instance given valid attributes' do
       affiliate.routed_queries.create!(valid_attributes)
     end
 
     context 'when a keyword is duplicated' do
       let(:dup_attributes) do
-        valid_attributes.merge({
-          routed_query_keywords_attributes: {
-            '0' => { 'keyword' => 'some keyword phrase' },
-            '1' => { 'keyword' => 'Some Keyword Phrase' },
+        valid_attributes.merge(
+          {
+            routed_query_keywords_attributes:
+            {
+              '0' => { 'keyword' => 'some keyword phrase' },
+              '1' => { 'keyword' => 'Some Keyword Phrase' }
+            }
           }
-        })
+        )
       end
 
-      it 'should reject the save of the keywords' do
+      let(:expected_errors) do
+        'The following keyword has been duplicated: \'some keyword phrase\'. ' \
+        'Each keyword is case-insensitive and should be added only once.'
+      end
+
+      it 'rejects the save of the keywords' do
         rq = affiliate.routed_queries.build(dup_attributes)
         expect(rq.valid?).to be false
-        expect(rq.errors[:routed_query_keywords]).to include("The following keyword has been duplicated: 'some keyword phrase'. Each keyword is case-insensitive and should be added only once.")
+        expect(rq.errors[:routed_query_keywords]).to include(expected_errors)
       end
     end
 
     context 'when multiple keywords are duplicated' do
       let(:dup_attributes) do
-        valid_attributes.merge({
-          routed_query_keywords_attributes: {
-            '0' => { 'keyword' => 'some keyword phrase' },
-            '1' => { 'keyword' => 'Some Keyword Phrase' },
-            '2' => { 'keyword' => 'some other keyword phrase' },
-            '3' => { 'keyword' => 'Some Other Keyword Phrase' },
+        valid_attributes.merge(
+          {
+            routed_query_keywords_attributes:
+            {
+              '0' => { 'keyword' => 'some keyword phrase' },
+              '1' => { 'keyword' => 'Some Keyword Phrase' },
+              '2' => { 'keyword' => 'some other keyword phrase' },
+              '3' => { 'keyword' => 'Some Other Keyword Phrase' }
+            }
           }
-        })
+        )
       end
 
-      it 'should reject the save of the keywords' do
+      let(:expected_errors) do
+        'The following keywords have been duplicated: \'some keyword phrase\', ' \
+        '\'some other keyword phrase\'. ' \
+        'Each keyword is case-insensitive and should be added only once.'
+      end
+
+      it 'rejects the save of the keywords' do
         rq = affiliate.routed_queries.build(dup_attributes)
         expect(rq.valid?).to be false
-        expect(rq.errors[:routed_query_keywords]).to include("The following keywords have been duplicated: 'some keyword phrase', 'some other keyword phrase'. Each keyword is case-insensitive and should be added only once.")
+        expect(rq.errors[:routed_query_keywords]).to include(expected_errors)
       end
     end
   end
 
   describe '#label' do
     it 'returns a label containing the url and description' do
-      expect(routed_queries(:unclaimed_money).label).to eq('https://www.usa.gov/unclaimed_money: Everybody wants it')
+      expect(routed_queries(:unclaimed_money).label).
+        to eq('https://www.usa.gov/unclaimed_money: Everybody wants it')
     end
   end
 
   describe '#dup' do
     subject(:original_instance) { routed_queries(:unclaimed_money) }
+
     include_examples 'site dupable'
   end
 end