Merge branch 'dm-gitmodules-parsing' into 'master'

Make .gitmodules parsing more resilient to syntax errors

Closes #26009

See merge request !11803

3 files changed, 47 insertions, 17 deletions

diff --git a/changelogs/unreleased/dm-gitmodules-parsing.yml b/changelogs/unreleased/dm-gitmodules-parsing.yml
new file mode 100644
index 00000000000..a7d755d6c4d
--- /dev/null
+++ b/changelogs/unreleased/dm-gitmodules-parsing.yml
@@ -0,0 +1,4 @@
+---
+title: Make .gitmodules parsing more resilient to syntax errors
+merge_request:
+author:
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index b9f1ac144b6..9d6adbdb4ac 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1006,31 +1006,39 @@ module Gitlab
       # Parses the contents of a .gitmodules file and returns a hash of
       # submodule information.
       def parse_gitmodules(commit, content)
-        results = {}
+        modules = {}
 
-        current = ""
-        content.split("\n").each do |txt|
-          if txt =~ /^\s*\[/
-            current = txt.match(/(?<=").*(?=")/)[0]
-            results[current] = {}
-          else
-            next unless results[current]
-            match_data = txt.match(/(\w+)\s*=\s*(.*)/)
-            next unless match_data
-            target = match_data[2].chomp
-            results[current][match_data[1]] = target
+        name = nil
+        content.each_line do |line|
+          case line.strip
+          when /\A\[submodule "(?<name>[^"]+)"\]\z/ # Submodule header
+            name = $~[:name]
+            modules[name] = {}
+          when /\A(?<key>\w+)\s*=\s*(?<value>.*)\z/ # Key/value pair
+            key = $~[:key]
+            value = $~[:value].chomp
+
+            next unless name && modules[name]
+
+            modules[name][key] = value
 
-            if match_data[1] == "path"
+            if key == 'path'
               begin
-                results[current]["id"] = blob_content(commit, target)
+                modules[name]['id'] = blob_content(commit, value)
               rescue InvalidBlobName
-                results.delete(current)
+                # The current entry is invalid
+                modules.delete(name)
+                name = nil
               end
             end
+          when /\A#/ # Comment
+            next
+          else # Invalid line
+            name = nil
           end
         end
 
-        results
+        modules
       end
 
       # Returns true if +commit+ introduced changes to +path+, using commit
@@ -1086,7 +1094,12 @@ module Gitlab
           elsif tmp_entry.nil?
             return nil
           else
-            tmp_entry = rugged.lookup(tmp_entry[:oid])
+            begin
+              tmp_entry = rugged.lookup(tmp_entry[:oid])
+            rescue Rugged::OdbError, Rugged::InvalidError, Rugged::ReferenceError
+              return nil
+            end
+
             return nil unless tmp_entry.type == :tree
             tmp_entry = tmp_entry[dir]
           end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index cb107c6d1f9..9d0e95d5b19 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -381,6 +381,19 @@ describe Gitlab::Git::Repository, seed_helper: true do
           }
         ])
       end
+
+      it 'should not break on invalid syntax' do
+        allow(repository).to receive(:blob_content).and_return(<<-GITMODULES.strip_heredoc)
+          [submodule "six"]
+          path = six
+          url = git://github.com/randx/six.git
+
+          [submodule]
+          foo = bar
+        GITMODULES
+
+        expect(submodules).to have_key('six')
+      end
     end
 
     context 'where repo doesn\'t have submodules' do