Make .gitmodules parsing more resilient to syntax errors

3 files changed, 47 insertions, 16 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..c3bf32ca1cc 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1008,25 +1008,34 @@ module Gitlab
       def parse_gitmodules(commit, content)
         results = {}
 
-        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
-
-            if match_data[1] == "path"
+        name = nil
+        entry = nil
+        content.each_line do |line|
+          case line.strip
+          when /\A\[submodule "(?<name>[^"]+)"\]\z/ # Submodule header
+            name = $~[:name]
+            entry = results[name] = {}
+          when /\A(?<key>\w+)\s*=\s*(?<value>.*)\z/ # Key/value pair
+            key = $~[:key]
+            value = $~[:value].chomp
+
+            next unless name && entry
+
+            entry[key] = value
+
+            if key == 'path'
               begin
-                results[current]["id"] = blob_content(commit, target)
+                entry['id'] = blob_content(commit, value)
               rescue InvalidBlobName
-                results.delete(current)
+                # The current entry is invalid
+                results.delete(name)
+                name = entry = nil
               end
             end
+          when /\A#/ # Comment
+            next
+          else # Invalid line
+            name = entry = nil
           end
         end
 
@@ -1086,7 +1095,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