Merge branch '254945-bold-search-terms-in-results' into 'master'

[RUN AS-IF-FOSS] Global Search - Bold Issue Search Term (sanitized)

See merge request gitlab-org/gitlab!43124

app/controllers/projects/settings/ci_cd_controller.rb

@@ -12,6 +12,8 @@ module Projects
         push_frontend_feature_flag(:ajax_new_deploy_token, @project)
       end
 
+      helper_method :highlight_badge
+
       def show
         if Feature.enabled?(:ci_pipeline_triggers_settings_vue_ui, @project)
           @triggers_json = ::Ci::TriggerSerializer.new.represent(
@@ -60,6 +62,10 @@ module Projects
 
       private
 
+      def highlight_badge(name, content, language = nil)
+        Gitlab::Highlight.highlight(name, content, language: language)
+      end
+
       def update_params
         params.require(:project).permit(*permitted_project_params)
       end

app/controllers/search_controller.rb

@@ -41,6 +41,7 @@ class SearchController < ApplicationController
     @show_snippets = search_service.show_snippets?
     @search_results = search_service.search_results
     @search_objects = search_service.search_objects(preload_method)
+    @search_highlight = search_service.search_highlight
 
     render_commits if @scope == 'commits'
     eager_load_user_status if @scope == 'users'

app/helpers/blob_helper.rb

 # frozen_string_literal: true
 
 module BlobHelper
-  def highlight(file_name, file_content, language: nil, plain: false)
-    highlighted = Gitlab::Highlight.highlight(file_name, file_content, plain: plain, language: language)
-
-    raw %(<pre class="code highlight"><code>#{highlighted}</code></pre>)
-  end
-
   def no_highlight_files
     %w(credits changelog news copying copyright license authors)
   end

app/helpers/search_helper.rb

@@ -301,8 +301,21 @@ module SearchHelper
     sanitize(html, tags: %w(a p ol ul li pre code))
   end
 
+  def simple_search_highlight_and_truncate(text, phrase, options = {})
+    text = Truncato.truncate(
+      text,
+      count_tags: false,
+      count_tail: false,
+      max_length: options.delete(:length) { 200 }
+    )
+
+    highlight(text, phrase.split, options)
+  end
+
   # _search_highlight is used in EE override
   def highlight_and_truncate_issue(issue, search_term, _search_highlight)
+    return unless issue.description.present?
+
     simple_search_highlight_and_truncate(issue.description, search_term, highlighter: '<span class="gl-text-black-normal gl-font-weight-bold">\1</span>')
   end
 

app/services/search_service.rb

@@ -65,6 +65,10 @@ class SearchService
     @search_objects ||= redact_unauthorized_results(search_results.objects(scope, page: params[:page], per_page: per_page, preload_method: preload_method))
   end
 
+  def search_highlight
+    search_results.highlight_map(scope)
+  end
+
   private
 
   def per_page

app/views/projects/settings/ci_cd/_badge.html.haml

@@ -15,18 +15,18 @@
           .col-md-2.text-center
             Markdown
           .col-md-10.code.js-syntax-highlight
-            = highlight('.md', badge.to_markdown, language: 'markdown')
+            = highlight_badge('.md', badge.to_markdown, language: 'markdown')
         .row
           %hr
         .row
           .col-md-2.text-center
             HTML
           .col-md-10.code.js-syntax-highlight
-            = highlight('.html', badge.to_html, language: 'html')
+            = highlight_badge('.html', badge.to_html, language: 'html')
         .row
           %hr
         .row
           .col-md-2.text-center
             AsciiDoc
           .col-md-10.code.js-syntax-highlight
-            = highlight('.adoc', badge.to_asciidoc)
+            = highlight_badge('.adoc', badge.to_asciidoc)

app/views/search/results/_issue.html.haml

@@ -9,6 +9,5 @@
       %span.term.str-truncated.gl-font-weight-bold.gl-ml-2= issue.title
   .gl-text-gray-500.gl-my-3
     = sprintf(s_(' %{project_name}#%{issue_iid} · opened %{issue_created} by %{author}'), { project_name: issue.project.full_name, issue_iid: issue.iid, issue_created: time_ago_with_tooltip(issue.created_at, placement: 'bottom'), author: link_to_member(@project, issue.author, avatar: false) }).html_safe
-  - if issue.description.present?
-    .description.term.col-sm-10.gl-px-0
-      = truncate(issue.description, length: 200)
+  .description.term.col-sm-10.gl-px-0
+    = highlight_and_truncate_issue(issue, @search_term, @search_highlight)

changelogs/unreleased/244427-bold-search-term-issues.yml

+---
+title: Global Search - Bold Issue's Search Term
+merge_request: 43124
+author:
+type: changed

ee/app/helpers/ee/search_helper.rb

@@ -72,8 +72,13 @@ module EE
     def highlight_and_truncate_issue(issue, search_term, search_highlight)
       return super unless search_service.use_elasticsearch? && search_highlight[issue.id]&.description.present?
 
-      # We use Elasticsearch highlighting for results from Elasticsearch
-      Truncato.truncate(search_highlight[issue.id].description.first, count_tags: false, count_tail: false, max_length: 200).html_safe
+      # We use Elasticsearch highlighting for results from Elasticsearch. Sanitize the description, replace the
+      # pre/post tags from Elasticsearch with highlighting, truncate, and mark as html_safe. HTML tags are not
+      # counted towards the character limit.
+      text = sanitize(search_highlight[issue.id].description.first)
+      text.gsub!(::Elastic::Latest::GitClassProxy::HIGHLIGHT_START_TAG, '<span class="gl-text-black-normal gl-font-weight-bold">')
+      text.gsub!(::Elastic::Latest::GitClassProxy::HIGHLIGHT_END_TAG, '</span>')
+      Truncato.truncate(text, count_tags: false, count_tail: false, max_length: 200).html_safe
     end
 
     def advanced_search_status_marker(project)

ee/lib/elastic/latest/application_class_proxy.rb

@@ -45,7 +45,13 @@ module Elastic
           memo[field.to_sym] = {}
         end
 
-        { fields: es_fields }
+        # Adding number_of_fragments: 0 to not split results into snippets.  This way controllers can decide how to handle the highlighted data.
+        {
+            fields: es_fields,
+            number_of_fragments: 0,
+            pre_tags: [::Elastic::Latest::GitClassProxy::HIGHLIGHT_START_TAG],
+            post_tags: [::Elastic::Latest::GitClassProxy::HIGHLIGHT_END_TAG]
+        }
       end
 
       def basic_query_hash(fields, query)

ee/lib/gitlab/elastic/search_results.rb

@@ -47,6 +47,25 @@ module Gitlab
         end
       end
 
+      # Pull the highlight attribute out of Elasticsearch results
+      # and map it to the result id
+      def highlight_map(scope)
+        results = case scope
+                  when 'projects'
+                    projects
+                  when 'issues'
+                    issues
+                  when 'merge_requests'
+                    merge_requests
+                  when 'milestones'
+                    milestones
+                  when 'notes'
+                    notes
+                  end
+
+        results.to_h { |x| [x[:_source][:id], x[:highlight]] } if results.present?
+      end
+
       def formatted_count(scope)
         case scope
         when 'projects'

ee/lib/gitlab/elastic/snippet_search_results.rb

@@ -16,6 +16,10 @@ module Gitlab
         limited_snippet_titles_count
       end
 
+      def highlight_map(scope)
+        snippet_titles.to_h { |x| [x[:_source][:id], x[:highlight]] }
+      end
+
       private
 
       def snippet_titles

ee/spec/helpers/search_helper_spec.rb

@@ -162,4 +162,53 @@ RSpec.describe SearchHelper do
       it_behaves_like 'returns old message'
     end
   end
+
+  describe '#highlight_and_truncate_issue' do
+    let(:description) { 'hello world' }
+    let(:issue) { create(:issue, description: description) }
+    let(:user) { create(:user) }
+    let(:search_highlight) { {} }
+
+    before do
+      allow(self).to receive(:current_user).and_return(user)
+      stub_ee_application_setting(search_using_elasticsearch: true)
+    end
+
+    # Elasticsearch returns Elasticsearch::Model::HashWrapper class for the highlighting
+    subject { highlight_and_truncate_issue(issue, 'test', Elasticsearch::Model::HashWrapper.new(search_highlight)) }
+
+    context 'when description is not present' do
+      let(:description) { nil }
+
+      it 'does nothing' do
+        expect(self).not_to receive(:sanitize)
+
+        subject
+      end
+    end
+
+    context 'when description present' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:description, :search_highlight, :expected) do
+        'test'                                                                 | { 1 => { description: ['gitlabelasticsearch→test←gitlabelasticsearch'] } } | "<span class='gl-text-black-normal gl-font-weight-bold'>test</span>"
+        '<span style="color: blue;">this test should not be blue</span>'       | { 1 => { description: ['<span style="color: blue;">this gitlabelasticsearch→test←gitlabelasticsearch should not be blue</span>'] } } | "<span>this <span class='gl-text-black-normal gl-font-weight-bold'>test</span> should not be blue</span>"
+        '<a href="#" onclick="alert(\'XSS\')">Click Me test</a>'               | { 1 => { description: ['<a href="#" onclick="alert(\'XSS\')">Click Me gitlabelasticsearch→test←gitlabelasticsearch</a>'] } } | "<a href='#'>Click Me <span class='gl-text-black-normal gl-font-weight-bold'>test</span></a>"
+        '<script type="text/javascript">alert(\'Another XSS\');</script> test' | { 1 => { description: ['<script type="text/javascript">alert(\'Another XSS\');</script> gitlabelasticsearch→test←gitlabelasticsearch'] } } | "alert(&apos;Another XSS&apos;); <span class='gl-text-black-normal gl-font-weight-bold'>test</span>"
+        'Lorem test ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec.' | { 1 => { description: ['Lorem gitlabelasticsearch→test←gitlabelasticsearch ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec.'] } } | "Lorem <span class='gl-text-black-normal gl-font-weight-bold'>test</span> ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Don..."
+      end
+
+      with_them do
+        before do
+          # table syntax doesn't allow use of calculated fields so we must fake issue.id
+          # to ensure the test goes down the correct path
+          allow(issue).to receive(:id).and_return(1)
+        end
+
+        it 'sanitizes, truncates, and highlights the search term' do
+          expect(subject).to eq(expected)
+        end
+      end
+    end
+  end
 end

ee/spec/lib/gitlab/elastic/search_results_spec.rb

@@ -12,6 +12,32 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
   let(:project_2) { create(:project, :public, :repository, :wiki_repo) }
   let(:limit_project_ids) { [project_1.id] }
 
+  describe '#highlight_map' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:results) { described_class.new(user, 'hello world', limit_project_ids) }
+
+    where(:scope, :results_method, :expected) do
+      'projects'       | :projects       | { 1 => 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' }
+      'milestones'     | :milestones     | { 1 => 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' }
+      'notes'          | :notes          | { 1 => 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' }
+      'issues'         | :issues         | { 1 => 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' }
+      'merge_requests' | :merge_requests | { 1 => 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' }
+      'blobs'          | nil             | nil
+      'wiki_blobs'     | nil             | nil
+      'commits'        | nil             | nil
+      'users'          | nil             | nil
+      'unknown'        | nil             | nil
+    end
+
+    with_them do
+      it 'returns the expected highlight map' do
+        expect(results).to receive(results_method).and_return([{ _source: { id: 1 }, highlight: 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' }]) if results_method
+        expect(results.highlight_map(scope)).to eq(expected)
+      end
+    end
+  end
+
   describe '#formatted_count' do
     using RSpec::Parameterized::TableSyntax
 
@@ -635,7 +661,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
     end
   end
 
-  describe 'Blobs' do
+  describe 'blobs' do
     before do
       project_1.repository.index_commits_and_blobs
 
@@ -845,7 +871,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
     end
   end
 
-  describe 'Wikis' do
+  describe 'wikis' do
     let(:results) { described_class.new(user, 'term', limit_project_ids) }
 
     subject(:wiki_blobs) { results.objects('wiki_blobs') }
@@ -932,7 +958,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
     end
   end
 
-  describe 'Commits' do
+  describe 'commits' do
     before do
       project_1.repository.index_commits_and_blobs
       ensure_elasticsearch_index!
@@ -968,7 +994,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
     end
   end
 
-  describe 'Visibility levels' do
+  describe 'visibility levels' do
     let(:internal_project) { create(:project, :internal, :repository, :wiki_repo, description: "Internal project") }
     let(:private_project1) { create(:project, :private, :repository, :wiki_repo, description: "Private project") }
     let(:private_project2) { create(:project, :private, :repository, :wiki_repo, description: "Private project where I'm a member") }
@@ -979,7 +1005,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
       private_project2.project_members.create(user: user, access_level: ProjectMember::DEVELOPER)
     end
 
-    context 'Issues' do
+    context 'issues' do
       it 'finds right set of issues' do
         issue_1 = create :issue, project: internal_project, title: "Internal project"
         create :issue, project: private_project1, title: "Private project"
@@ -1006,7 +1032,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
       end
     end
 
-    context 'Milestones' do
+    context 'milestones' do
       let!(:milestone_1) { create(:milestone, project: internal_project, title: "Internal project") }
       let!(:milestone_2) { create(:milestone, project: private_project1, title: "Private project") }
       let!(:milestone_3) { create(:milestone, project: private_project2, title: "Private project which user is member") }
@@ -1126,7 +1152,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
       end
     end
 
-    context 'Projects' do
+    context 'projects' do
       it_behaves_like 'a paginated object', 'projects'
 
       it 'finds right set of projects' do
@@ -1155,7 +1181,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
       end
     end
 
-    context 'Merge Requests' do
+    context 'merge requests' do
       it 'finds right set of merge requests' do
         merge_request_1 = create :merge_request, target_project: internal_project, source_project: internal_project, title: "Internal project"
         create :merge_request, target_project: private_project1, source_project: private_project1, title: "Private project"
@@ -1182,7 +1208,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
       end
     end
 
-    context 'Wikis' do
+    context 'wikis' do
       before do
         [public_project, internal_project, private_project1, private_project2].each do |project|
           project.wiki.create_page('index_page', 'term')
@@ -1209,7 +1235,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
       end
     end
 
-    context 'Commits' do
+    context 'commits' do
       it 'finds right set of commits' do
         [internal_project, private_project1, private_project2, public_project].each do |project|
           project.repository.create_file(
@@ -1241,7 +1267,7 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need
       end
     end
 
-    context 'Blobs' do
+    context 'blobs' do
       it 'finds right set of blobs' do
         [internal_project, private_project1, private_project2, public_project].each do |project|
           project.repository.create_file(

ee/spec/lib/gitlab/elastic/snippet_search_results_spec.rb

@@ -40,6 +40,13 @@ RSpec.describe Gitlab::Elastic::SnippetSearchResults, :elastic, :sidekiq_might_n
     end
   end
 
+  describe '#highlight_map' do
+    it 'returns the expected highlight map' do
+      expect(results).to receive(:snippet_titles).and_return([{ _source: { id: 1 }, highlight: 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' }])
+      expect(results.highlight_map('snippet_titles')).to eq({ 1 => 'test <span class="gl-text-black-normal gl-font-weight-bold">highlight</span>' })
+    end
+  end
+
   context 'when user is not author' do
     let(:results) { described_class.new(create(:user), 'foo', []) }
 

lib/gitlab/search_results.rb

@@ -108,6 +108,11 @@ module Gitlab
       UsersFinder.new(current_user, search: query).execute
     end
 
+    # highlighting is only performed by Elasticsearch backed results
+    def highlight_map(scope)
+      {}
+    end
+
     private
 
     def collection_for(scope)

spec/features/projects/badges/list_spec.rb

@@ -17,10 +17,10 @@ RSpec.describe 'list of badges' do
       expect(page).to have_content 'Markdown'
       expect(page).to have_content 'HTML'
       expect(page).to have_content 'AsciiDoc'
-      expect(page).to have_css('.highlight', count: 3)
+      expect(page).to have_css('.js-syntax-highlight', count: 3)
       expect(page).to have_xpath("//img[@alt='pipeline status']")
 
-      page.within('.highlight', match: :first) do
+      page.within('.js-syntax-highlight', match: :first) do
         expect(page).to have_content 'badges/master/pipeline.svg'
       end
     end
@@ -32,10 +32,10 @@ RSpec.describe 'list of badges' do
       expect(page).to have_content 'Markdown'
       expect(page).to have_content 'HTML'
       expect(page).to have_content 'AsciiDoc'
-      expect(page).to have_css('.highlight', count: 3)
+      expect(page).to have_css('.js-syntax-highlight', count: 3)
       expect(page).to have_xpath("//img[@alt='coverage report']")
 
-      page.within('.highlight', match: :first) do
+      page.within('.js-syntax-highlight', match: :first) do
         expect(page).to have_content 'badges/master/coverage.svg'
       end
     end

spec/helpers/blob_helper_spec.rb

@@ -5,16 +5,6 @@ require 'spec_helper'
 RSpec.describe BlobHelper do
   include TreeHelper
 
-  describe '#highlight' do
-    it 'wraps highlighted content' do
-      expect(helper.highlight('test.rb', '52')).to eq(%q[<pre class="code highlight"><code><span id="LC1" class="line" lang="ruby"><span class="mi">52</span></span></code></pre>])
-    end
-
-    it 'handles plain version' do
-      expect(helper.highlight('test.rb', '52', plain: true)).to eq(%q[<pre class="code highlight"><code><span id="LC1" class="line" lang="">52</span></code></pre>])
-    end
-  end
-
   describe "#sanitize_svg_data" do
     let(:input_svg_path) { File.join(Rails.root, 'spec', 'fixtures', 'unsanitized.svg') }
     let(:data) { File.read(input_svg_path) }

spec/helpers/search_helper_spec.rb

@@ -408,4 +408,44 @@ RSpec.describe SearchHelper do
       it { is_expected.to eq('111111') }
     end
   end
+
+  describe '#highlight_and_truncate_issue' do
+    let(:description) { 'hello world' }
+    let(:issue) { create(:issue, description: description) }
+    let(:user) { create(:user) }
+
+    before do
+      allow(self).to receive(:current_user).and_return(user)
+    end
+
+    subject { highlight_and_truncate_issue(issue, 'test', {}) }
+
+    context 'when description is not present' do
+      let(:description) { nil }
+
+      it 'does nothing' do
+        expect(self).not_to receive(:simple_search_highlight_and_truncate)
+
+        subject
+      end
+    end
+
+    context 'when description present' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:description, :expected) do
+        'test'                                                                 | '<span class="gl-text-black-normal gl-font-weight-bold">test</span>'
+        '<span style="color: blue;">this test should not be blue</span>'       | '<span>this <span class="gl-text-black-normal gl-font-weight-bold">test</span> should not be blue</span>'
+        '<a href="#" onclick="alert(\'XSS\')">Click Me test</a>'               | '<a href="#">Click Me <span class="gl-text-black-normal gl-font-weight-bold">test</span></a>'
+        '<script type="text/javascript">alert(\'Another XSS\');</script> test' | ' <span class="gl-text-black-normal gl-font-weight-bold">test</span>'
+        'Lorem test ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec.' | 'Lorem <span class="gl-text-black-normal gl-font-weight-bold">test</span> ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Don...'
+      end
+
+      with_them do
+        it 'sanitizes, truncates, and highlights the search term' do
+          expect(subject).to eq(expected)
+        end
+      end
+    end
+  end
 end

spec/lib/gitlab/search_results_spec.rb

@@ -60,6 +60,25 @@ RSpec.describe Gitlab::SearchResults do
       end
     end
 
+    describe '#highlight_map' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:scope, :expected) do
+        'projects'       | {}
+        'issues'         | {}
+        'merge_requests' | {}
+        'milestones'     | {}
+        'users'          | {}
+        'unknown'        | {}
+      end
+
+      with_them do
+        it 'returns the expected highlight_map' do
+          expect(results.highlight_map(scope)).to eq(expected)
+        end
+      end
+    end
+
     describe '#formatted_limited_count' do
       using RSpec::Parameterized::TableSyntax
 

spec/lib/gitlab/snippet_search_results_spec.rb

@@ -21,6 +21,12 @@ RSpec.describe Gitlab::SnippetSearchResults do
     end
   end
 
+  describe '#highlight_map' do
+    it 'returns the expected highlight map' do
+      expect(results.highlight_map('snippet_titles')).to eq({})
+    end
+  end
+
   describe '#objects' do
     it 'uses page and per_page to paginate results' do
       snippet2 = create(:snippet, :public, content: 'foo', file_name: 'foo')