Fix project search results for digits surrounded by colons

A file containing /:\d+:/ in its contents would break the search results if
those contents were part of the results, because we were splitting on colons,
which can't work with untrusted input.

Changing to use the null byte as a separator is much safer.

Author: smcgivern

app/models/repository.rb

@@ -932,7 +932,7 @@ def search_files_by_content(query, ref)
     return [] if empty? || query.blank?
 
     offset = 2
-    args = %W(grep -i -I -n --before-context #{offset} --after-context #{offset} -E -e #{Regexp.escape(query)} #{ref || root_ref})
+    args = %W(grep -i -I -n -z --before-context #{offset} --after-context #{offset} -E -e #{Regexp.escape(query)} #{ref || root_ref})
 
     run_git(args).first.scrub.split(/^--$/)
   end

changelogs/unreleased/41666-cannot-search-with-keyword-merge.yml

@@ -0,0 +1,6 @@
+---
+title: Fix file search results when they match file contents with a number between
+  two colons
+merge_request: 16462
+author:
+type: fixed

lib/gitlab/project_search_results.rb

@@ -44,25 +44,20 @@ def self.parse_search_result(result)
       ref = nil
       filename = nil
       basename = nil
+      data = ""
       startline = 0
 
-      result.each_line.each_with_index do |line, index|
-        matches = line.match(/^(?<ref>[^:]*):(?<filename>.*):(?<startline>\d+):/)
-        if matches
+      result.strip.each_line.each_with_index do |line, index|
+        prefix ||= line.match(/^(?<ref>[^:]*):(?<filename>.*)\x00(?<startline>\d+)\x00/)&.tap do |matches|
           ref = matches[:ref]
           filename = matches[:filename]
           startline = matches[:startline]
           startline = startline.to_i - index
           extname = Regexp.escape(File.extname(filename))
           basename = filename.sub(/#{extname}$/, '')
-          break
         end
-      end
-
-      data = ""
 
-      result.each_line do |line|
-        data << line.sub(ref, '').sub(filename, '').sub(/^:-\d+-/, '').sub(/^::\d+:/, '')
+        data << line.sub(prefix.to_s, '')
       end
 
       FoundBlob.new(

spec/lib/gitlab/project_search_results_spec.rb

@@ -70,15 +70,6 @@
 
       subject { described_class.parse_search_result(search_result) }
 
-      it 'can correctly parse filenames including ":"' do
-        special_char_result = "\nmaster:testdata/project::function1.yaml-1----\nmaster:testdata/project::function1.yaml:2:test: data1\n"
-
-        blob = described_class.parse_search_result(special_char_result)
-
-        expect(blob.ref).to eq('master')
-        expect(blob.filename).to eq('testdata/project::function1.yaml')
-      end
-
       it "returns a valid FoundBlob" do
         is_expected.to be_an Gitlab::SearchResults::FoundBlob
         expect(subject.id).to be_nil
@@ -90,16 +81,40 @@
         expect(subject.data.lines[2]).to eq("  - Feature: Replace teams with group membership\n")
       end
 
+      context 'when the matching filename contains a colon' do
+        let(:search_result) { "\nmaster:testdata/project::function1.yaml\x001\x00---\n" }
+
+        it 'returns a valid FoundBlob' do
+          expect(subject.filename).to eq('testdata/project::function1.yaml')
+          expect(subject.basename).to eq('testdata/project::function1')
+          expect(subject.ref).to eq('master')
+          expect(subject.startline).to eq(1)
+          expect(subject.data).to eq('---')
+        end
+      end
+
+      context 'when the matching content contains a number surrounded by colons' do
+        let(:search_result) { "\nmaster:testdata/foo.txt\x001\x00blah:9:blah" }
+
+        it 'returns a valid FoundBlob' do
+          expect(subject.filename).to eq('testdata/foo.txt')
+          expect(subject.basename).to eq('testdata/foo')
+          expect(subject.ref).to eq('master')
+          expect(subject.startline).to eq(1)
+          expect(subject.data).to eq('blah:9:blah')
+        end
+      end
+
       context "when filename has extension" do
-        let(:search_result) { "master:CONTRIBUTE.md:5:- [Contribute to GitLab](#contribute-to-gitlab)\n" }
+        let(:search_result) { "master:CONTRIBUTE.md\x005\x00- [Contribute to GitLab](#contribute-to-gitlab)\n" }
 
         it { expect(subject.path).to eq('CONTRIBUTE.md') }
         it { expect(subject.filename).to eq('CONTRIBUTE.md') }
         it { expect(subject.basename).to eq('CONTRIBUTE') }
       end
 
       context "when file under directory" do
-        let(:search_result) { "master:a/b/c.md:5:a b c\n" }
+        let(:search_result) { "master:a/b/c.md\x005\x00a b c\n" }
 
         it { expect(subject.path).to eq('a/b/c.md') }
         it { expect(subject.filename).to eq('a/b/c.md') }
@@ -144,7 +159,7 @@
     end
 
     it 'finds by content' do
-      expect(results).to include("master:Title.md:1:Content\n")
+      expect(results).to include("master:Title.md\x001\x00Content\n")
     end
   end
 

spec/models/repository_spec.rb

@@ -657,7 +657,7 @@ def expect_to_raise_storage_error
       subject { results.first }
 
       it { is_expected.to be_an String }
-      it { expect(subject.lines[2]).to eq("master:CHANGELOG:190:  - Feature: Replace teams with group membership\n") }
+      it { expect(subject.lines[2]).to eq("master:CHANGELOG\x00190\x00  - Feature: Replace teams with group membership\n") }
     end
   end