diff --git a/app/models/project.rb b/app/models/project.rb index 7590a27a24a6ddd9b45444d7ada95214f82392c5..5009824bba7de570942b793a67237ad38d6f7d4c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -502,6 +502,7 @@ def self.integration_association_name(name) presence: true, project_path: true, length: { maximum: 255 } + validate :container_registry_project_path_validation validates :project_feature, presence: true @@ -892,6 +893,14 @@ def initialize(attributes = nil) 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 diff --git a/config/feature_flags/development/restrict_special_characters_in_project_path.yml b/config/feature_flags/development/restrict_special_characters_in_project_path.yml new file mode 100644 index 0000000000000000000000000000000000000000..d2eff14660555c6e486062a543455ad1f43e2a50 --- /dev/null +++ b/config/feature_flags/development/restrict_special_characters_in_project_path.yml @@ -0,0 +1,8 @@ +--- +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 diff --git a/doc/api/projects.md b/doc/api/projects.md index 90561846fc31e06766b919a73f886606fdcfcddc..ee649377745d46c3d92407729230c1b1e5f0f8f5 100644 --- a/doc/api/projects.md +++ b/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). | diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index ddb6bc37bbae779a469eca8e13857634db65c421..c9202c6c54c632fbc9d10d29c522fcb2fa6c302c 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -259,6 +259,15 @@ def project_name_regex_message "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 diff --git a/spec/lib/learn_gitlab/project_spec.rb b/spec/lib/learn_gitlab/project_spec.rb index 5d649740c65c27cf75ac070f17e93574fca918a8..23784709817d32d6afe4c70a2dda388540a6d35d 100644 --- a/spec/lib/learn_gitlab/project_spec.rb +++ b/spec/lib/learn_gitlab/project_spec.rb @@ -5,7 +5,6 @@ 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 @@ 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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 23cfa1b903b674c1f0881a02a3d774432a197ebf..8b2b57949fc66655942abffe8fdd1eacffe7b5e0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -574,10 +574,62 @@ 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 diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 340ed7bde534f3f71dd3f5883153ba24de6adcb0..9f9e1cfd90ed4d7367140fd3893bdb6ef83a8bb5 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -1019,7 +1019,11 @@ def attempt_login(include_password) 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 @@ def attempt_login(include_password) 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 @@ def attempt_login(include_password) 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 @@ def attempt_login(include_password) 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 @@ def attempt_login(include_password) 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 @@ def attempt_login(include_password) 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 @@ def attempt_login(include_password) 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|