Merge branch 'fix/gb/fix-container-registry-tag-routing' into 'master'

Fix docker tag reference routing constraints

Closes #35220 and gitlab-com/support-forum#2246

See merge request !12961

changelogs/unreleased/fix-gb-fix-container-registry-tag-routing.yml

@@ -0,0 +1,4 @@
+---
+title: Fix docker tag reference routing constraints
+merge_request: 12961
+author:

config/routes/project.rb

@@ -272,7 +272,7 @@ constraints(ProjectUrlConstrainer.new) do
       namespace :registry do
         resources :repository, only: [] do
           resources :tags, only: [:destroy],
-                           constraints: { id: Gitlab::Regex.container_registry_reference_regex }
+                           constraints: { id: Gitlab::Regex.container_registry_tag_regex }
         end
       end
 

lib/gitlab/regex.rb

@@ -19,17 +19,23 @@ module Gitlab
       "It must start with letter, digit, emoji or '_'."
     end
 
-    def container_registry_reference_regex
-      Gitlab::PathRegex.git_reference_regex
-    end
-
     ##
-    # Docker Distribution Registry 2.4.1 repository name rules
+    # Docker Distribution Registry repository / tag name rules
+    #
+    # See https://github.com/docker/distribution/blob/master/reference/regexp.go.
     #
     def container_repository_name_regex
       @container_repository_regex ||= %r{\A[a-z0-9]+(?:[-._/][a-z0-9]+)*\Z}
     end
 
+    ##
+    # We do not use regexp anchors here because these are not allowed when
+    # used as a routing constraint.
+    #
+    def container_registry_tag_regex
+      @container_registry_tag_regex ||= /[\w][\w.-]{0,127}/
+    end
+
     def environment_name_regex_chars
       'a-zA-Z0-9_/\\$\\{\\}\\. -'
     end

spec/controllers/projects/registry/tags_controller_spec.rb

@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+describe Projects::Registry::TagsController do
+  let(:user)    { create(:user) }
+  let(:project) { create(:empty_project, :private) }
+
+  before do
+    sign_in(user)
+    stub_container_registry_config(enabled: true)
+  end
+
+  context 'when user has access to registry' do
+    before do
+      project.add_developer(user)
+    end
+
+    describe 'POST destroy' do
+      context 'when there is matching tag present' do
+        before do
+          stub_container_registry_tags(repository: /image/, tags: %w[rc1 test.])
+        end
+
+        let(:repository) do
+          create(:container_repository, name: 'image', project: project)
+        end
+
+        it 'makes it possible to delete regular tag' do
+          expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete)
+
+          destroy_tag('rc1')
+        end
+
+        it 'makes it possible to delete a tag that ends with a dot' do
+          expect_any_instance_of(ContainerRegistry::Tag).to receive(:delete)
+
+          destroy_tag('test.')
+        end
+      end
+    end
+  end
+
+  def destroy_tag(name)
+    post :destroy, namespace_id: project.namespace,
+                   project_id: project,
+                   repository_id: repository,
+                   id: name
+  end
+end

spec/lib/gitlab/regex_spec.rb

@@ -38,4 +38,15 @@ describe Gitlab::Regex, lib: true do
     it { is_expected.not_to match('9foo') }
     it { is_expected.not_to match('foo-') }
   end
+
+  describe '.container_repository_name_regex' do
+    subject { described_class.container_repository_name_regex }
+
+    it { is_expected.to match('image') }
+    it { is_expected.to match('my/image') }
+    it { is_expected.to match('my/awesome/image-1') }
+    it { is_expected.to match('my/awesome/image.test') }
+    it { is_expected.not_to match('.my/image') }
+    it { is_expected.not_to match('my/image.') }
+  end
 end

spec/routing/project_routing_spec.rb

@@ -609,4 +609,26 @@ describe 'project routing' do
       expect(get('/gitlab/gitlabhq/pages/domains/my.domain.com')).to route_to('projects/pages_domains#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'my.domain.com')
     end
   end
+
+  describe Projects::Registry::TagsController, :routing do
+    describe '#destroy' do
+      it 'correctly routes to a destroy action' do
+        expect(delete('/gitlab/gitlabhq/registry/repository/1/tags/rc1'))
+          .to route_to('projects/registry/tags#destroy',
+                       namespace_id: 'gitlab',
+                       project_id: 'gitlabhq',
+                       repository_id: '1',
+                       id: 'rc1')
+      end
+
+      it 'takes registry tag name constrains into account' do
+        expect(delete('/gitlab/gitlabhq/registry/repository/1/tags/-rc1'))
+          .not_to route_to('projects/registry/tags#destroy',
+                           namespace_id: 'gitlab',
+                           project_id: 'gitlabhq',
+                           repository_id: '1',
+                           id: '-rc1')
+      end
+    end
+  end
 end