Fix Go module proxy issues

- Fix zip entry paths. The Go proxy spec requires zip entries to conform
  to `module@version/file`, where `file` is the path within the module.
- Fix /v2+ handling. For major versions 2+, the module name must include
  the major version as a suffix, e.g. /v2.
- Handle case encoding. Requests to the Go proxy encode uppercase
  characters in URLs as '!' followed by the character in lowercase.
- Per Zoom discussion with @trizzi, @sabrams, and team, modules with an
  invalid module name in go.mod will be ignored, initially.

ee/app/models/packages/go_module.rb

 # frozen_string_literal: true
 
 class Packages::GoModule
-  SEMVER_TAG_REGEX = Regexp.new("^#{::Packages::GoModuleVersion::SEMVER_REGEX.source}$").freeze
+  SEMVER_TAG_REGEX = /^v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([-.a-z0-9]+))?(?:\+([-.a-z0-9]+))?$/i.freeze
 
   # belongs_to :project
 
@@ -22,7 +22,7 @@ class Packages::GoModule
   end
 
   def versions
-    @versions ||= project.repository.tags
+    @versions ||= @project.repository.tags
       .filter { |tag| SEMVER_TAG_REGEX.match?(tag.name) && !tag.dereferenced_target.nil? }
       .map    { |tag| ::Packages::GoModuleVersion.new self, tag }
       .filter { |ver| ver.valid? }

ee/app/models/packages/go_module_version.rb

 # frozen_string_literal: true
 
 class Packages::GoModuleVersion
-  SEMVER_REGEX = /v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([-.A-Z0-9]+))?(?:\+([-.A-Z0-9]+))?/i.freeze
+  SEMVER_REGEX = /v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([-.a-z0-9]+))?(?:\+([-.a-z0-9]+))?/i.freeze
   VERSION_SUFFIX_REGEX = /\/v([1-9]\d*)$/i.freeze
 
   # belongs_to :mod
@@ -16,22 +16,30 @@ class Packages::GoModuleVersion
   end
 
   def gomod
-    return @gomod unless @gomod.nil?
-
-    blob = @mod.project.repository.blob_at(@tag.dereferenced_target.sha, @mod.path + '/go.mod')
-    @gomod = blob ? blob.data : ''
+    @gomod ||= @mod.project.repository.blob_at(@tag.dereferenced_target.sha, @mod.path + '/go.mod')&.data
   end
 
   def valid?
-    m = gomod.split("\n", 2).first
+    valid_path? && valid_module?
+  end
+
+  def valid_path?
+    m = VERSION_SUFFIX_REGEX.match(@mod.name)
+
     case major
     when 0, 1
-      m == "module #{@mod.name}"
+      m.nil?
     else
-      m == "module #{@mod.name}/v#{major}"
+      !m.nil? && m[1].to_i == major
     end
   end
 
+  def valid_module?
+    return false unless gomod
+
+    gomod.split("\n", 2).first == "module #{@mod.name}"
+  end
+
   def major
     SEMVER_REGEX.match(@tag.name)[1].to_i
   end
@@ -56,8 +64,8 @@ class Packages::GoModuleVersion
     return @files unless @files.nil?
 
     sha = @tag.dereferenced_target.sha
-    tree = @mod.project.repository.tree(sha, mod.path, recursive: true).entries.filter { |e| e.file? }
-    nested = tree.filter { |e| e.name == 'go.mod' && !(mod.path == '' && e.path == 'go.mod' || e.path == mod.path + '/go.mod') }.map { |e| e.path[0..-7] }
+    tree = @mod.project.repository.tree(sha, @mod.path, recursive: true).entries.filter { |e| e.file? }
+    nested = tree.filter { |e| e.name == 'go.mod' && !(@mod.path == '' && e.path == 'go.mod' || e.path == @mod.path + '/go.mod') }.map { |e| e.path[0..-7] }
     @files = tree.filter { |e| !nested.any? { |n| e.path.start_with? n } }
   end
 

ee/lib/api/go_proxy.rb

@@ -3,13 +3,18 @@ module API
   class GoProxy < Grape::API
     helpers ::API::Helpers::PackagesHelpers
 
-    MODULE_VERSION_REQUIREMENTS = { module_version: ::Packages::GoModuleVersion::SEMVER_REGEX }.freeze
+    MODULE_VERSION_REGEX = /v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([-.!a-z0-9]+))?(?:\+([-.!a-z0-9]+))?/.freeze
+    MODULE_VERSION_REQUIREMENTS = { module_version: MODULE_VERSION_REGEX }.freeze
 
     before { require_packages_enabled! }
 
     helpers do
+      def case_decode(str)
+        str.gsub(/![[:alpha:]]/) { |s| s[1..].upcase }
+      end
+
       def find_module
-        module_name = params[:module_name].gsub(/![[:alpha:]]/) { |s| s[1..].upcase }
+        module_name = case_decode params[:module_name]
 
         bad_request!('Module Name') if module_name.blank?
 
@@ -18,6 +23,15 @@ module API
 
         mod
       end
+
+      def find_version
+        mod = find_module
+
+        ver = mod.find_version case_decode params[:module_version]
+        not_found! unless ver
+
+        ver
+      end
     end
 
     params do
@@ -49,10 +63,7 @@ module API
           requires :module_version, type: String, desc: 'Module version'
         end
         get ':module_version.info', requirements: MODULE_VERSION_REQUIREMENTS do
-          mod = find_module
-
-          ver = mod.find_version params[:module_version]
-          not_found! unless ver
+          ver = find_version
 
           present ::Packages::Go::ModuleVersionPresenter.new(ver), with: EE::API::Entities::GoModuleVersion
         end
@@ -64,10 +75,7 @@ module API
           requires :module_version, type: String, desc: 'Module version'
         end
         get ':module_version.mod', requirements: MODULE_VERSION_REQUIREMENTS do
-          mod = find_module
-
-          ver = mod.find_version params[:module_version]
-          not_found! unless ver
+          ver = find_version
 
           content_type 'text/plain'
           ver.gomod
@@ -80,14 +88,13 @@ module API
           requires :module_version, type: String, desc: 'Module version'
         end
         get ':module_version.zip', requirements: MODULE_VERSION_REQUIREMENTS do
-          mod = find_module
+          ver = find_version
 
-          ver = mod.find_version params[:module_version]
-          not_found! unless ver
+          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 file.path
+              zip.put_next_entry "#{ver.mod.name}@#{ver.name}/#{file.path[suffix_len...]}"
               zip.write ver.blob_at(file.path)
             end
           end

ee/spec/requests/api/go_proxy_spec.rb

@@ -3,23 +3,158 @@
 require 'spec_helper'
 
 describe API::GoProxy do
+  let_it_be(:domain) do
+    port = ::Gitlab.config.gitlab.port
+    host = ::Gitlab.config.gitlab.host
+    case port when 80, 443 then host else "#{host}:#{port}" end
+  end
+
   let_it_be(:user) { create :user }
   let_it_be(:project) { create :project_empty_repo, creator: user, path: 'my-go-lib' }
-  let_it_be(:base) { module_base project }
+  let_it_be(:base) { "#{domain}/#{project.path_with_namespace}" }
 
   let_it_be(:oauth) { create :oauth_access_token, scopes: 'api', resource_owner: user }
   let_it_be(:job) { create :ci_build, user: user }
   let_it_be(:pa_token) { create :personal_access_token, user: user }
 
+  # rubocop: disable Layout/IndentationConsistency
   let_it_be(:modules) do
-    create_version(1, 0, 0, create_readme)
+    create_version(1, 0, 0, create_file('README.md', 'Hi', commit_message: 'Add README.md'))
     create_version(1, 0, 1, create_module)
     create_version(1, 0, 2, create_package('pkg'))
     create_version(1, 0, 3, create_module('mod'))
+                            create_module('v2')
+    create_version(2, 0, 0, create_file('v2/x.go', "package a\n"))
+                            create_file('v2/y.go', "package a\n") # todo tag this v1.0.4
 
     project.repository.head_commit
   end
 
+  context 'with an invalid module directive' do
+    let_it_be(:project) { create :project_empty_repo, :public, creator: user }
+    let_it_be(:base) { "#{domain}/#{project.path_with_namespace}" }
+
+    # rubocop: disable Layout/IndentationWidth
+    let_it_be(:modules) do
+                              create_file('a.go', "package a\nfunc Hi() { println(\"Hello world!\") }\n")
+      create_version(1, 0, 0, create_file('go.mod', "module not/a/real/module\n"))
+                              create_file('v2/a.go', "package a\nfunc Hi() { println(\"Hello world!\") }\n")
+      create_version(2, 0, 0, create_file('v2/go.mod', "module #{base}\n"))
+
+      project.repository.head_commit
+    end
+
+    describe 'GET /projects/:id/packages/go/*module_name/@v/list' do
+      let(:resource) { "list" }
+
+      context 'with a completely wrong directive for v1' do
+        let(:module_name) { base }
+
+        it 'returns nothing' do
+          get_resource(user)
+
+          expect_module_version_list
+        end
+      end
+
+      context 'with a directive omitting the suffix for v2' do
+        let(:module_name) { "#{base}/v2" }
+
+        it 'returns nothing' do
+          get_resource(user)
+
+          expect_module_version_list
+        end
+      end
+    end
+
+    describe 'GET /projects/:id/packages/go/*module_name/@v/:module_version.info' do
+      context 'with a completely wrong directive for v1' do
+        let(:module_name) { base }
+        let(:resource) { "v1.0.0.info" }
+
+        it 'returns 404' do
+          get_resource(user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+
+      context 'with a directive omitting the suffix for v2' do
+        let(:module_name) { "#{base}/v2" }
+        let(:resource) { "v2.0.0.info" }
+
+        it 'returns 404' do
+          get_resource(user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+  end
+
+  context 'with a case sensitive project and versions' do
+    let_it_be(:project) { create :project_empty_repo, :public, creator: user, path: 'MyGoLib' }
+    let_it_be(:base) { "#{domain}/#{project.path_with_namespace}" }
+    let_it_be(:base_encoded) { base.gsub(/[A-Z]/) { |s| "!#{s.downcase}"} }
+
+    let_it_be(:modules) do
+                              create_file('README.md', 'Hi', commit_message: 'Add README.md')
+      create_version(1, 0, 1, create_module, prerelease: 'prerelease')
+      create_version(1, 0, 1, create_package('pkg'), prerelease: 'Prerelease')
+
+      project.repository.head_commit
+    end
+
+    describe 'GET /projects/:id/packages/go/*module_name/@v/list' do
+      let(:resource) { "list" }
+
+      context 'with a case encoded path' do
+        let(:module_name) { base_encoded }
+
+        it 'returns the tags' do
+          get_resource(user)
+
+          expect_module_version_list('v1.0.1-prerelease', 'v1.0.1-Prerelease')
+        end
+      end
+
+      context 'without a case encoded path' do
+        let(:module_name) { base.downcase }
+
+        it 'returns 404' do
+          get_resource(user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    describe 'GET /projects/:id/packages/go/*module_name/@v/:module_version.info' do
+      context 'with a case encoded path' do
+        let(:module_name) { base_encoded }
+        let(:resource) { "v1.0.1-!prerelease.info" }
+
+        it 'returns the uppercase tag' do
+          get_resource(user)
+
+          expect_module_version_info('v1.0.1-Prerelease')
+        end
+      end
+
+      context 'without a case encoded path' do
+        let(:module_name) { base_encoded }
+        let(:resource) { "v1.0.1-prerelease.info" }
+
+        it 'returns the lowercase tag' do
+          get_resource(user)
+
+          expect_module_version_info('v1.0.1-prerelease')
+        end
+      end
+    end
+  end
+
   describe 'GET /projects/:id/packages/go/*module_name/@v/list' do
     let(:resource) { "list" }
 
@@ -64,6 +199,16 @@ describe API::GoProxy do
         expect_module_version_list('v1.0.3')
       end
     end
+
+    context 'for the root module v2' do
+      let(:module_name) { "#{base}/v2" }
+
+      it 'returns v2.0.0' do
+        get_resource(user)
+
+        expect_module_version_list('v2.0.0')
+      end
+    end
   end
 
   describe 'GET /projects/:id/packages/go/*module_name/@v/:module_version.info' do
@@ -89,6 +234,17 @@ describe API::GoProxy do
       end
     end
 
+    context 'with the root module v2.0.0' do
+      let(:module_name) { "#{base}/v2" }
+      let(:resource) { "v2.0.0.info" }
+
+      it 'returns correct information' do
+        get_resource(user)
+
+        expect_module_version_info('v2.0.0')
+      end
+    end
+
     context 'with an invalid path' do
       let(:module_name) { "#{base}/pkg" }
       let(:resource) { "v1.0.3.info" }
@@ -135,6 +291,17 @@ describe API::GoProxy do
       end
     end
 
+    context 'with the root module v2.0.0' do
+      let(:module_name) { "#{base}/v2" }
+      let(:resource) { "v2.0.0.mod" }
+
+      it 'returns correct content' do
+        get_resource(user)
+
+        expect_module_version_mod(module_name)
+      end
+    end
+
     context 'with an invalid path' do
       let(:module_name) { "#{base}/pkg" }
       let(:resource) { "v1.0.3.mod" }
@@ -166,7 +333,7 @@ describe API::GoProxy do
       it 'returns a zip of everything' do
         get_resource(user)
 
-        expect_module_version_zip(Set['README.md', 'go.mod', 'a.go'])
+        expect_module_version_zip(module_name, 'v1.0.1', ['README.md', 'go.mod', 'a.go'])
       end
     end
 
@@ -177,7 +344,7 @@ describe API::GoProxy do
       it 'returns a zip of everything' do
         get_resource(user)
 
-        expect_module_version_zip(Set['README.md', 'go.mod', 'a.go', 'pkg/b.go'])
+        expect_module_version_zip(module_name, 'v1.0.2', ['README.md', 'go.mod', 'a.go', 'pkg/b.go'])
       end
     end
 
@@ -188,7 +355,7 @@ describe API::GoProxy do
       it 'returns a zip of everything, excluding the submodule' do
         get_resource(user)
 
-        expect_module_version_zip(Set['README.md', 'go.mod', 'a.go', 'pkg/b.go'])
+        expect_module_version_zip(module_name, 'v1.0.3', ['README.md', 'go.mod', 'a.go', 'pkg/b.go'])
       end
     end
 
@@ -199,7 +366,18 @@ describe API::GoProxy do
       it 'returns a zip of the submodule' do
         get_resource(user)
 
-        expect_module_version_zip(Set['go.mod', 'a.go'])
+        expect_module_version_zip(module_name, 'v1.0.3', ['go.mod', 'a.go'])
+      end
+    end
+
+    context 'with the root module v2.0.0' do
+      let(:module_name) { "#{base}/v2" }
+      let(:resource) { "v2.0.0.zip" }
+
+      it 'returns a zip of v2 of the root module' do
+        get_resource(user)
+
+        expect_module_version_zip(module_name, 'v2.0.0', ['go.mod', 'a.go', 'x.go'])
       end
     end
   end
@@ -256,51 +434,47 @@ describe API::GoProxy do
     get api("/projects/#{project.id}/packages/go/#{module_name}/@v/#{resource}", user), params: params
   end
 
-  def module_base(project)
-    Gitlab::Routing.url_helpers.project_url(project).split('://', 2)[1]
-  end
-
-  def create_readme(commit_message: 'Add README.md')
-    get_result("create readme", Files::CreateService.new(
+  def create_file(path, content, commit_message: 'Add file')
+    get_result("create file", Files::CreateService.new(
       project,
       project.owner,
-      commit_message: 'Add README.md',
+      commit_message: commit_message,
       start_branch: 'master',
       branch_name: 'master',
-      file_path: 'README.md',
-      file_content: 'Hi'
+      file_path: path,
+      file_content: content
     ).execute)
   end
 
-  def create_module(path = '', commit_message: 'Add module')
-    name = module_base(project)
-    if path != ''
-      name += '/' + path
-      path += '/'
-    end
-
-    get_result("create module '#{name}'", ::Files::MultiService.new(
+  def create_package(path, commit_message: 'Add package')
+    get_result("create package '#{path}'", Files::MultiService.new(
       project,
       project.owner,
       commit_message: commit_message,
       start_branch: project.repository.root_ref,
       branch_name: project.repository.root_ref,
       actions: [
-        { action: :create, file_path: path + 'go.mod', content: "module #{name}\n" },
-        { action: :create, file_path: path + 'a.go', content: "package a\nfunc Hi() { println(\"Hello world!\") }\n" }
+        { action: :create, file_path: path + '/b.go', content: "package b\nfunc Bye() { println(\"Goodbye world!\") }\n" }
       ]
     ).execute)
   end
 
-  def create_package(path, commit_message: 'Add package')
-    get_result("create package '#{path}'", ::Files::MultiService.new(
+  def create_module(path = '', commit_message: 'Add module')
+    name = "#{domain}/#{project.path_with_namespace}"
+    if path != ''
+      name += '/' + path
+      path += '/'
+    end
+
+    get_result("create module '#{name}'", Files::MultiService.new(
       project,
       project.owner,
       commit_message: commit_message,
       start_branch: project.repository.root_ref,
       branch_name: project.repository.root_ref,
       actions: [
-        { action: :create, file_path: path + '/b.go', content: "package b\nfunc Bye() { println(\"Goodbye world!\") }\n" }
+        { action: :create, file_path: path + 'go.mod', content: "module #{name}\n" },
+        { action: :create, file_path: path + 'a.go', content: "package a\nfunc Hi() { println(\"Hello world!\") }\n" }
       ]
     ).execute)
   end
@@ -310,7 +484,7 @@ describe API::GoProxy do
     name += "-#{prerelease}" if prerelease
     name += "+#{build}" if build
 
-    get_result("create version #{name[1..]}", ::Tags::CreateService.new(project, project.owner).execute(name, sha, tag_message))
+    get_result("create version #{name[1..]}", Tags::CreateService.new(project, project.owner).execute(name, sha, tag_message))
   end
 
   def get_result(op, ret)
@@ -321,7 +495,7 @@ describe API::GoProxy do
 
   def expect_module_version_list(*versions)
     expect(response).to have_gitlab_http_status(:ok)
-    expect(response.body.split("\n")).to eq(versions)
+    expect(response.body.split("\n").to_set).to eq(versions.to_set)
   end
 
   def expect_module_version_info(version)
@@ -338,9 +512,10 @@ describe API::GoProxy do
     expect(response.body.split("\n", 2).first).to eq("module #{name}")
   end
 
-  def expect_module_version_zip(entries)
+  def expect_module_version_zip(path, version, entries)
     expect(response).to have_gitlab_http_status(:ok)
 
+    entries = entries.map { |e| "#{path}@#{version}/#{e}" }.to_set
     actual = Set[]
     Zip::InputStream.open(StringIO.new(response.body)) do |zip|
       while (entry = zip.get_next_entry)