Improve Go module version processing

- Clean up pseudo-version logic
- Batch and memoize fetching blobs for a version
  + Fixes Gitaly N+1
- Remove archive generation from GoProxy (into GoModuleVersion)
- Test module finder for path traversal
- Also, add GitLab version to GoProxy API details

ee/app/finders/packages/go/module_finder.rb

@@ -10,6 +10,7 @@ module Packages
       attr_reader :project, :module_name
 
       def initialize(project, module_name)
+        module_name = CGI.unescape(module_name)
         module_name = Pathname.new(module_name).cleanpath.to_s
 
         @project = project

ee/app/finders/packages/go/version_finder.rb

@@ -51,7 +51,7 @@ module Packages
 
       def find_pseudo_version(str)
         semver = parse_semver(str)
-        raise ArgumentError.new 'target is not a pseudo-version' unless semver && PSEUDO_VERSION_REGEX.match?(str)
+        raise ArgumentError.new 'target is not a pseudo-version' unless pseudo_version?(semver)
 
         # valid pseudo-versions are
         #   vX.0.0-yyyymmddhhmmss-sha1337beef0, when no earlier tagged commit exists for X

ee/app/models/packages/go_module_version.rb

@@ -37,10 +37,25 @@ class Packages::GoModuleVersion
     @name || @ref&.name
   end
 
+  def full_name
+    "#{mod.name}@#{name || commit.sha}"
+  end
+
   def gomod
     @gomod ||= blob_at(@mod.path + '/go.mod')
   end
 
+  def archive
+    suffix_len = @mod.path == '' ? 0 : @mod.path.length + 1
+
+    Zip::OutputStream.write_buffer do |zip|
+      files.each do |file|
+        zip.put_next_entry "#{full_name}/#{file.path[suffix_len...]}"
+        zip.write blob_at(file.path)
+      end
+    end
+  end
+
   def files
     return @files if defined?(@files)
 
@@ -51,10 +66,22 @@ class Packages::GoModuleVersion
   end
 
   def blob_at(path)
-    @mod.project.repository.blob_at(@commit.sha, path)&.data
+    return if path.nil? || path.empty?
+
+    path = path[1..] if path.start_with? '/'
+
+    blobs.find { |x| x.path == path }&.data
   end
 
   def valid?
     @mod.path_valid?(major) && @mod.gomod_valid?(gomod)
   end
+
+  private
+
+  def blobs
+    return @blobs if defined?(@blobs)
+
+    @blobs = @mod.project.repository.batch_blobs(files.map { |x| [@commit.sha, x.path] })
+  end
 end

ee/lib/api/go_proxy.rb

@@ -66,7 +66,7 @@ module API
 
       namespace ':id/packages/go/*module_name/@v' do
         desc 'Get all tagged versions for a given Go module' do
-          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/list'
+          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/list. This feature was introduced in GitLab 13.0.'
         end
         get 'list' do
           mod = find_module
@@ -76,7 +76,7 @@ module API
         end
 
         desc 'Get information about the given module version' do
-          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/<version>.info'
+          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/<version>.info. This feature was introduced in GitLab 13.0.'
           success EE::API::Entities::GoModuleVersion
         end
         params do
@@ -89,7 +89,7 @@ module API
         end
 
         desc 'Get the module file of the given module version' do
-          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/<version>.mod'
+          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/<version>.mod. This feature was introduced in GitLab 13.0.'
         end
         params do
           requires :module_version, type: String, desc: 'Module version'
@@ -102,7 +102,7 @@ module API
         end
 
         desc 'Get a zip of the source of the given module version' do
-          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/<version>.zip'
+          detail 'See `go help goproxy`, GET $GOPROXY/<module>/@v/<version>.zip. This feature was introduced in GitLab 13.0.'
         end
         params do
           requires :module_version, type: String, desc: 'Module version'
@@ -110,21 +110,13 @@ module API
         get ':module_version.zip', requirements: MODULE_VERSION_REQUIREMENTS do
           ver = find_version
 
-          suffix_len = ver.mod.path == '' ? 0 : ver.mod.path.length + 1
-
-          s = Zip::OutputStream.write_buffer do |zip|
-            ver.files.each do |file|
-              zip.put_next_entry "#{ver.mod.name}@#{ver.name}/#{file.path[suffix_len...]}"
-              zip.write ver.blob_at(file.path)
-            end
-          end
-
+          # TODO: Content-Type should be application/zip, see #214876
           header['Content-Disposition'] = ActionDispatch::Http::ContentDisposition.format(disposition: 'attachment', filename: ver.name + '.zip')
           header['Content-Transfer-Encoding'] = 'binary'
           content_type 'text/plain'
           # content_type 'application/zip'
           status :ok
-          body s.string
+          body ver.archive.string
         end
       end
     end

ee/lib/api/helpers/packages/go/module_helpers.rb

@@ -5,12 +5,6 @@ module API
     module Packages
       module Go
         module ModuleHelpers
-          # basic semver regex
-          SEMVER_REGEX = /v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([-.a-z0-9]+))?(?:\+([-.a-z0-9]+))?/i.freeze
-
-          # semver, but the prerelease component follows a specific format
-          PSEUDO_VERSION_REGEX = /^v\d+\.(0\.0-|\d+\.\d+-([^+]*\.)?0\.)\d{14}-[A-Za-z0-9]+(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$/i.freeze
-
           def case_encode(str)
             str.gsub(/A-Z/) { |s| "!#{s.downcase}"}
           end
@@ -25,8 +19,29 @@ module API
             ::Packages::SemVer.match?(tag.name, prefixed: true)
           end
 
-          def pseudo_version?(str)
-            ::Packages::SemVer.match?(str, prefixed: true) && PSEUDO_VERSION_REGEX.match?(str)
+          def pseudo_version?(version)
+            return false unless version
+
+            if version.is_a? String
+              version = parse_semver version
+              return false unless version
+            end
+
+            pre = version.prerelease
+
+            # valid pseudo-versions are
+            #   vX.0.0-yyyymmddhhmmss-sha1337beef0, when no earlier tagged commit exists for X
+            #   vX.Y.Z-pre.0.yyyymmddhhmmss-sha1337beef0, when most recent prior tag is vX.Y.Z-pre
+            #   vX.Y.(Z+1)-0.yyyymmddhhmmss-sha1337beef0, when most recent prior tag is vX.Y.Z
+
+            if version.minor != 0 || version.patch != 0
+              m = /\A(.*\.)?0\./.freeze.match pre
+              return false unless m
+
+              pre = pre[m[0].length..]
+            end
+
+            /\A\d{14}-[A-Za-z0-9]+\z/.freeze.match? pre
           end
 
           def parse_semver(str)

ee/spec/factories/go_module_versions.rb

@@ -19,7 +19,7 @@ FactoryBot.define do
       new(p.mod, p.type, p.commit, name: p.name, semver: s, ref: p.ref)
     end
 
-    mod { go_module }
+    mod { create :go_module }
     type { :commit }
     commit { raise ArgumentError.new("commit is required") }
     name { nil }

ee/spec/factories/go_modules.rb

@@ -7,6 +7,6 @@ FactoryBot.define do
 
     project
     path { '' }
-    name { "#{Settings.build_gitlab_go_url}/#{project.full_path}#{path.empty? ? '' : path}" }
+    name { "#{Settings.build_gitlab_go_url}/#{project.full_path}#{path.empty? ? '' : '/'}#{path}" }
   end
 end

ee/spec/finders/packages/go/module_finder_spec.rb

@@ -5,10 +5,25 @@ require 'spec_helper'
 describe Packages::Go::ModuleFinder do
   let_it_be(:project) { create :project }
   let_it_be(:other_project) { create :project }
+  let(:finder) { described_class.new project, module_name }
+
+  shared_examples 'an invalid path' do
+    describe '#module_name' do
+      it 'returns the expected name' do
+        expect(finder.module_name).to eq(expected_name)
+      end
+    end
+
+    describe '#execute' do
+      it 'returns nil' do
+        expect(finder.execute).to be_nil
+      end
+    end
+  end
 
   describe '#execute' do
     context 'with module name equal to project name' do
-      let(:finder) { described_class.new(project, base_url(project)) }
+      let(:module_name) { base_url(project) }
 
       it 'returns a module with empty path' do
         mod = finder.execute
@@ -18,7 +33,7 @@ describe Packages::Go::ModuleFinder do
     end
 
     context 'with module name starting with project name and slash' do
-      let(:finder) { described_class.new(project, base_url(project) + '/mod') }
+      let(:module_name) { base_url(project) + '/mod' }
 
       it 'returns a module with non-empty path' do
         mod = finder.execute
@@ -28,7 +43,7 @@ describe Packages::Go::ModuleFinder do
     end
 
     context 'with a module name not equal to and not starting with project name' do
-      let(:finder) { described_class.new(project, base_url(other_project)) }
+      let(:module_name) { base_url(other_project) }
 
       it 'returns nil' do
         expect(finder.execute).to be_nil
@@ -36,6 +51,27 @@ describe Packages::Go::ModuleFinder do
     end
   end
 
+  context 'with relative path component' do
+    it_behaves_like 'an invalid path' do
+      let(:module_name) { base_url(project) + '/../xyz' }
+      let(:expected_name) { base_url(project.namespace) + '/xyz' }
+    end
+  end
+
+  context 'with a URL encoded relative path component' do
+    it_behaves_like 'an invalid path' do
+      let(:module_name) { base_url(project) + '/%2E%2E%2Fxyz' }
+      let(:expected_name) { base_url(project.namespace) + '/xyz' }
+    end
+  end
+
+  context 'with many relative path components' do
+    it_behaves_like 'an invalid path' do
+      let(:module_name) { base_url(project) + ('/..' * 10) + '/xyz' }
+      let(:expected_name) { ('../' * 7) + 'xyz' }
+    end
+  end
+
   def base_url(project)
     "#{Settings.build_gitlab_go_url}/#{project.full_path}"
   end

ee/spec/finders/packages/go/version_finder_spec.rb

@@ -39,19 +39,19 @@ describe Packages::Go::VersionFinder do
     end
 
     context 'for the package' do
-      let(:mod) { create :go_module, project: project, path: '/pkg' }
+      let(:mod) { create :go_module, project: project, path: 'pkg' }
 
       it_behaves_like '#execute'
     end
 
     context 'for the submodule' do
-      let(:mod) { create :go_module, project: project, path: '/mod' }
+      let(:mod) { create :go_module, project: project, path: 'mod' }
 
       it_behaves_like '#execute', 'v1.0.3'
     end
 
     context 'for the root module v2' do
-      let(:mod) { create :go_module, project: project, path: '/v2' }
+      let(:mod) { create :go_module, project: project, path: 'v2' }
 
       it_behaves_like '#execute', 'v2.0.0'
     end

ee/spec/models/packages/go_module_version_spec.rb

@@ -17,6 +17,28 @@ describe Packages::GoModuleVersion, type: :model do
     create :go_module_commit, :files,   project: project, tag: 'v2.0.0', files: { 'v2/x.go' => "package a\n" }
   end
 
+  shared_examples '#files' do |desc, *entries|
+    it "returns #{desc}" do
+      actual = version.files.map { |x| x.path }.to_set
+      expect(actual).to eq(entries.to_set)
+    end
+  end
+
+  shared_examples '#archive' do |desc, *entries|
+    it "returns an archive of #{desc}" do
+      expected = entries.map { |e| "#{version.full_name}/#{e}" }.to_set
+
+      actual = Set[]
+      Zip::InputStream.open(StringIO.new(version.archive.string)) do |zip|
+        while (entry = zip.get_next_entry)
+          actual.add(entry.name)
+        end
+      end
+
+      expect(actual).to eq(expected)
+    end
+  end
+
   describe '#name' do
     context 'with ref and name specified' do
       let_it_be(:version) { create :go_module_version, mod: mod, name: 'foobar', commit: project.repository.head_commit, ref: project.repository.find_tag('v1.0.0') }
@@ -50,20 +72,42 @@ describe Packages::GoModuleVersion, type: :model do
     context 'with a root module' do
       context 'with an empty module path' do
         let_it_be(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.2' }
-        it('returns all the files') { expect(version.files.map { |x| x.path }.to_set).to eq(Set['README.md', 'go.mod', 'a.go', 'pkg/b.go']) }
+        it_behaves_like '#files', 'all the files', 'README.md', 'go.mod', 'a.go', 'pkg/b.go'
+      end
+    end
+
+    context 'with a root module and a submodule' do
+      context 'with an empty module path' do
+        let_it_be(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.3' }
+        it_behaves_like '#files', 'files excluding the submodule', 'README.md', 'go.mod', 'a.go', 'pkg/b.go'
+      end
+
+      context 'with the submodule\'s path' do
+        let_it_be(:mod) { create :go_module, project: project, path: 'mod' }
+        let_it_be(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.3' }
+        it_behaves_like '#files', 'the submodule\'s files', 'mod/go.mod', 'mod/a.go'
+      end
+    end
+  end
+
+  describe '#archive' do
+    context 'with a root module' do
+      context 'with an empty module path' do
+        let_it_be(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.2' }
+        it_behaves_like '#archive', 'all the files', 'README.md', 'go.mod', 'a.go', 'pkg/b.go'
       end
     end
 
     context 'with a root module and a submodule' do
       context 'with an empty module path' do
         let_it_be(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.3' }
-        it('returns files excluding the submodule') { expect(version.files.map { |x| x.path }.to_set).to eq(Set['README.md', 'go.mod', 'a.go', 'pkg/b.go']) }
+        it_behaves_like '#archive', 'files excluding the submodule', 'README.md', 'go.mod', 'a.go', 'pkg/b.go'
       end
 
       context 'with the submodule\'s path' do
         let_it_be(:mod) { create :go_module, project: project, path: 'mod' }
         let_it_be(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.3' }
-        it('returns the submodule\'s files') { expect(version.files.map { |x| x.path }.to_set).to eq(Set['mod/go.mod', 'mod/a.go']) }
+        it_behaves_like '#archive', 'the submodule\'s files', 'go.mod', 'a.go'
       end
     end
   end