Merge branch '349047-do-not-allow-special-characters-in-new-project-names' into 'master'

Do not allow special characters in new project names

See merge request gitlab-org/gitlab!80055

app/models/project.rb

@@ -502,6 +502,7 @@ class Project < ApplicationRecord
     presence: true,
     project_path: true,
     length: { maximum: 255 }
+  validate :container_registry_project_path_validation
 
   validates :project_feature, presence: true
 
@@ -892,6 +893,14 @@ class Project < ApplicationRecord
     super
   end
 
+  def container_registry_project_path_validation
+    if Feature.enabled?(:restrict_special_characters_in_project_path, self, default_enabled: :yaml) &&
+        path_changed? &&
+        !path.match?(Gitlab::Regex.oci_repository_path_regex)
+      errors.add(:path, Gitlab::Regex.oci_repository_path_regex_message)
+    end
+  end
+
   def parent_loaded?
     association(:namespace).loaded?
   end

config/feature_flags/development/restrict_special_characters_in_project_path.yml

+---
+name: restrict_special_characters_in_project_path
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80055
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353054
+milestone: '14.8'
+type: development
+group: group::workspace
+default_enabled: false

doc/api/projects.md

@@ -1214,7 +1214,7 @@ POST /projects
 | Attribute                                                   | Type    | Required               | Description |
 |-------------------------------------------------------------|---------|------------------------|-------------|
 | `name`                                                      | string  | **{check-circle}** Yes (if path isn't provided) | The name of the new project. Equals path if not provided. |
-| `path`                                                      | string  | **{check-circle}** Yes (if name isn't provided) | Repository name for new project. Generated based on name if not provided (generated as lowercase with dashes). |
+| `path`                                                      | string  | **{check-circle}** Yes (if name isn't provided) | Repository name for new project. Generated based on name if not provided (generated as lowercase with dashes). Starting with GitLab 14.9, path must not start or end with a special character and must not contain consecutive special characters. |
 | `allow_merge_on_skipped_pipeline`                           | boolean | **{dotted-circle}** No | Set whether or not merge requests can be merged with skipped jobs. |
 | `analytics_access_level`                                    | string  | **{dotted-circle}** No | One of `disabled`, `private` or `enabled` |
 | `approvals_before_merge` **(PREMIUM)**                      | integer | **{dotted-circle}** No | How many approvers should approve merge requests by default. To configure approval rules, see [Merge request approvals API](merge_request_approvals.md). |

lib/gitlab/regex.rb

@@ -259,6 +259,15 @@ module Gitlab
       "It must start with a letter, digit, emoji, or '_'."
     end
 
+    # Project path must conform to this regex. See https://gitlab.com/gitlab-org/gitlab/-/issues/27483
+    def oci_repository_path_regex
+      @oci_repository_path_regex ||= %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z}.freeze
+    end
+
+    def oci_repository_path_regex_message
+      'must not start or end with a special character and must not contain consecutive special characters.'
+    end
+
     def group_name_regex
       @group_name_regex ||= /\A#{group_name_regex_chars}\z/.freeze
     end

spec/lib/learn_gitlab/project_spec.rb

@@ -5,7 +5,6 @@ require 'spec_helper'
 RSpec.describe LearnGitlab::Project do
   let_it_be(:current_user) { create(:user) }
   let_it_be(:learn_gitlab_project) { create(:project, name: LearnGitlab::Project::PROJECT_NAME) }
-  let_it_be(:learn_gitlab_ultimate_trial_project) { create(:project, name: LearnGitlab::Project::PROJECT_NAME_ULTIMATE_TRIAL) }
   let_it_be(:learn_gitlab_board) { create(:board, project: learn_gitlab_project, name: LearnGitlab::Project::BOARD_NAME) }
   let_it_be(:learn_gitlab_label) { create(:label, project: learn_gitlab_project, name: LearnGitlab::Project::LABEL_NAME) }
 
@@ -48,7 +47,7 @@ RSpec.describe LearnGitlab::Project do
     it { is_expected.to eq learn_gitlab_project }
 
     context 'when it is created during trial signup' do
-      let_it_be(:learn_gitlab_project) { create(:project, name: LearnGitlab::Project::PROJECT_NAME_ULTIMATE_TRIAL) }
+      let_it_be(:learn_gitlab_project) { create(:project, name: LearnGitlab::Project::PROJECT_NAME_ULTIMATE_TRIAL, path: 'learn-gitlab-ultimate-trial') }
 
       it { is_expected.to eq learn_gitlab_project }
     end

spec/models/project_spec.rb

@@ -574,10 +574,62 @@ RSpec.describe Project, factory_default: :keep do
         expect(project).to be_valid
       end
 
-      it 'allows a path ending in a period' do
-        project = build(:project, path: 'foo.')
+      context 'path is unchanged' do
+        let_it_be(:invalid_path_project) do
+          project = create(:project, :repository, :public)
+          project.update_attribute(:path, 'foo.')
+          project
+        end
 
-        expect(project).to be_valid
+        it 'does not raise validation error for path for existing project' do
+          expect { invalid_path_project.update!(name: 'Foo') }.not_to raise_error
+        end
+      end
+
+      %w[. - _].each do |special_character|
+        it "rejects a path ending in '#{special_character}'" do
+          project = build(:project, path: "foo#{special_character}")
+
+          expect(project).not_to be_valid
+        end
+
+        it "rejects a path starting with '#{special_character}'" do
+          project = build(:project, path: "#{special_character}foo")
+
+          expect(project).not_to be_valid
+        end
+
+        context 'restrict_special_characters_in_project_path feature flag is disabled' do
+          before do
+            stub_feature_flags(restrict_special_characters_in_project_path: false)
+          end
+
+          it "allows a path ending in '#{special_character}'" do
+            project = build(:project, path: "foo#{special_character}")
+
+            expect(project).to be_valid
+          end
+        end
+      end
+
+      context 'restrict_special_characters_in_project_path feature flag is disabled' do
+        before do
+          stub_feature_flags(restrict_special_characters_in_project_path: false)
+        end
+
+        %w[. _].each do |special_character|
+          it "allows a path starting with '#{special_character}'" do
+            project = build(:project, path: "#{special_character}foo")
+
+            expect(project).to be_valid
+          end
+        end
+
+        it "rejects a path starting with '-'" do
+          project = build(:project, path: "-foo")
+
+          expect(project).not_to be_valid
+        end
       end
     end
   end

spec/requests/git_http_spec.rb

@@ -1019,7 +1019,11 @@ RSpec.describe 'Git HTTP requests' do
       let(:path) { "#{project.full_path}.git" }
 
       context "when the project is public" do
-        let(:project) { create(:project, :repository, :public, path: 'foo.') }
+        let(:project) do
+          project = create(:project, :repository, :public)
+          project.update_attribute(:path, 'foo.')
+          project
+        end
 
         it_behaves_like 'pushes require Basic HTTP Authentication'
 
@@ -1158,7 +1162,11 @@ RSpec.describe 'Git HTTP requests' do
       end
 
       context "when the project is private" do
-        let(:project) { create(:project, :repository, :private, path: 'foo.') }
+        let(:project) do
+          project = create(:project, :repository, :private)
+          project.update_attribute(:path, 'foo.')
+          project
+        end
 
         it_behaves_like 'pulls require Basic HTTP Authentication'
         it_behaves_like 'pushes require Basic HTTP Authentication'
@@ -1586,11 +1594,19 @@ RSpec.describe 'Git HTTP requests' do
       end
 
       it_behaves_like 'project path without .git suffix' do
-        let(:repository_path) { create(:project, :repository, :public, path: 'project.').full_path }
+        let(:repository_path) do
+          project = create(:project, :repository, :public)
+          project.update_attribute(:path, 'project.')
+          project.full_path
+        end
       end
 
       context "retrieving an info/refs file" do
-        let(:project) { create(:project, :repository, :public, path: 'project.') }
+        let(:project) do
+          project = create(:project, :repository, :public)
+          project.update_attribute(:path, 'project.')
+          project
+        end
 
         context "when the file exists" do
           before do
@@ -1625,7 +1641,11 @@ RSpec.describe 'Git HTTP requests' do
       let(:path) { "/#{wiki.repository.full_path}.git" }
 
       context "when the project is public" do
-        let(:project) { create(:project, :wiki_repo, :public, :wiki_enabled, path: 'foo.') }
+        let(:project) do
+          project = create(:project, :wiki_repo, :public, :wiki_enabled)
+          project.update_attribute(:path, 'foo.')
+          project
+        end
 
         it_behaves_like 'pushes require Basic HTTP Authentication'
 
@@ -1652,7 +1672,11 @@ RSpec.describe 'Git HTTP requests' do
             end
 
             context 'but the repo is disabled' do
-              let(:project) { create(:project, :wiki_repo, :public, :repository_disabled, :wiki_enabled, path: 'foo.') }
+              let(:project) do
+                project = create(:project, :wiki_repo, :public, :repository_disabled, :wiki_enabled)
+                project.update_attribute(:path, 'foo.')
+                project
+              end
 
               it_behaves_like 'pulls are allowed'
               it_behaves_like 'pushes are allowed'
@@ -1673,7 +1697,11 @@ RSpec.describe 'Git HTTP requests' do
       end
 
       context "when the project is private" do
-        let(:project) { create(:project, :wiki_repo, :private, :wiki_enabled, path: 'foo.') }
+        let(:project) do
+          project = create(:project, :wiki_repo, :private, :wiki_enabled)
+          project.update_attribute(:path, 'foo.')
+          project
+        end
 
         it_behaves_like 'pulls require Basic HTTP Authentication'
         it_behaves_like 'pushes require Basic HTTP Authentication'
@@ -1700,7 +1728,11 @@ RSpec.describe 'Git HTTP requests' do
             end
 
             context 'but the repo is disabled' do
-              let(:project) { create(:project, :wiki_repo, :private, :repository_disabled, :wiki_enabled, path: 'foo.') }
+              let(:project) do
+                project = create(:project, :wiki_repo, :private, :repository_disabled, :wiki_enabled)
+                project.update_attribute(:path, 'foo.')
+                project
+              end
 
               it 'allows clones' do
                 download(path, user: user.username, password: user.password) do |response|