From b0e6181a550b003825631c65f9227d6e1e2f2958 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Mon, 7 Feb 2022 19:17:23 +0530 Subject: [PATCH 01/12] Do not allow special characters at the start or end of project path Special characters in project path causes ContainerRegistry::Path::InvalidRegistryPathError. The regex defined in the OCI Distribution has been modified as we have case insensitive route searches and the project path cannot include '/'. See https://gitlab.com/gitlab-org/gitlab/-/issues/27483 --- app/models/project.rb | 4 ++++ lib/gitlab/regex.rb | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/app/models/project.rb b/app/models/project.rb index 7590a27a24a6dd..f21c43758b0651 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -502,6 +502,10 @@ def self.integration_association_name(name) presence: true, project_path: true, length: { maximum: 255 } + validates :path, + format: { with: Gitlab::Regex.oci_repository_path_regex, + message: Gitlab::Regex.oci_repository_path_regex_message }, + if: :path_changed? validates :project_feature, presence: true diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index ddb6bc37bbae77..da56e9923817fe 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -259,6 +259,14 @@ def project_name_regex_message "It must start with a letter, digit, emoji, or '_'." end + def oci_repository_path_regex + %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z} + end + + def oci_repository_path_regex_message + 'must not start or end with special characters and must not contain consecutive special characters.' + end + def group_name_regex @group_name_regex ||= /\A#{group_name_regex_chars}\z/.freeze end -- GitLab From 4189d056431bae48a2166185ad8e6168e9877cd1 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Fri, 4 Mar 2022 16:38:28 +0530 Subject: [PATCH 02/12] Update specs to disallow leading/trailing special characters in project Some specs use #update_attribute to skip validations so that existing records can be tested. --- spec/models/project_spec.rb | 4 +-- spec/requests/git_http_spec.rb | 48 ++++++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 23cfa1b903b674..79696679fa7b94 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -574,10 +574,10 @@ expect(project).to be_valid end - it 'allows a path ending in a period' do + it 'rejects a path ending in a period' do project = build(:project, path: 'foo.') - expect(project).to be_valid + expect(project).not_to be_valid end end end diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 340ed7bde534f3..9f9e1cfd90ed4d 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| -- GitLab From ccaea570dabf364e4ac00ecc403b73f98daa8415 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Wed, 9 Feb 2022 07:25:52 +0530 Subject: [PATCH 03/12] Add specs for leading/trailing special characters in project path --- spec/models/project_spec.rb | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 79696679fa7b94..9c52831bd52e04 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -574,10 +574,18 @@ expect(project).to be_valid end - it 'rejects a path ending in a period' do - project = build(:project, path: 'foo.') + %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 + 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 end end end -- GitLab From 2aa9e1d837d8c721a4ed54deee04b1a244059e6b Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Wed, 16 Feb 2022 23:44:12 +0530 Subject: [PATCH 04/12] Pass Learn Gitlab project path while creating factory object The new validation disallows consecutive special characters in project path. When path is not passed to the project factory spaces are replaced with underscores in factories/projects.rb. --- spec/lib/learn_gitlab/project_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/learn_gitlab/project_spec.rb b/spec/lib/learn_gitlab/project_spec.rb index 5d649740c65c27..30820b5189e4af 100644 --- a/spec/lib/learn_gitlab/project_spec.rb +++ b/spec/lib/learn_gitlab/project_spec.rb @@ -5,7 +5,7 @@ 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_ultimate_trial_project) { create(:project, name: LearnGitlab::Project::PROJECT_NAME_ULTIMATE_TRIAL, path: 'learn-gitlab-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 +48,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 -- GitLab From 97f0f2e0524843899ed41ed6c46a35cdf822506e Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Thu, 17 Feb 2022 12:58:02 +0530 Subject: [PATCH 05/12] Add dev feature flag for project path validation --- .../restrict_special_characters_in_project_path.yml | 8 ++++++++ lib/gitlab/regex.rb | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 config/feature_flags/development/restrict_special_characters_in_project_path.yml 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 00000000000000..d2eff14660555c --- /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/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index da56e9923817fe..d7b460b52229a0 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -260,7 +260,11 @@ def project_name_regex_message end def oci_repository_path_regex - %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z} + if Feature.enabled?(:restrict_special_characters_in_project_path) + %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z} + else + %r{.*} + end end def oci_repository_path_regex_message -- GitLab From 0626d3e8b711dd802af3dc77c3f1d3678d7418b3 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Thu, 17 Feb 2022 15:57:41 +0530 Subject: [PATCH 06/12] Use custom validation for project path to use feature flag Otherwise, the changes to the feature flag require rails restart. --- app/models/project.rb | 13 +++++++++---- lib/gitlab/regex.rb | 6 +----- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index f21c43758b0651..e21f5575b14339 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -502,10 +502,7 @@ def self.integration_association_name(name) presence: true, project_path: true, length: { maximum: 255 } - validates :path, - format: { with: Gitlab::Regex.oci_repository_path_regex, - message: Gitlab::Regex.oci_repository_path_regex_message }, - if: :path_changed? + validate :project_path_oci_validation validates :project_feature, presence: true @@ -896,6 +893,14 @@ def initialize(attributes = nil) super end + def project_path_oci_validation + if Feature.enabled?(:restrict_special_characters_in_project_path, self) && + 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/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index d7b460b52229a0..da56e9923817fe 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -260,11 +260,7 @@ def project_name_regex_message end def oci_repository_path_regex - if Feature.enabled?(:restrict_special_characters_in_project_path) - %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z} - else - %r{.*} - end + %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z} end def oci_repository_path_regex_message -- GitLab From cdd51a06d0dc5332acb083b96c78fec5222d96dd Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Mon, 21 Feb 2022 15:01:40 +0530 Subject: [PATCH 07/12] Cache regex in instance variable --- lib/gitlab/regex.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index da56e9923817fe..a8c1667265aea6 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -259,8 +259,9 @@ 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 - %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z} + @oci_repository_path_regex ||= %r{\A[a-zA-Z0-9]+([._-][a-zA-Z0-9]+)*\z}.freeze end def oci_repository_path_regex_message -- GitLab From ecb77f86beb8047386939df9476f737d5b97998d Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Mon, 21 Feb 2022 15:02:30 +0530 Subject: [PATCH 08/12] Add specs to spec/models/project_spec.rb --- spec/models/project_spec.rb | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9c52831bd52e04..8b2b57949fc666 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -574,6 +574,18 @@ expect(project).to be_valid end + context 'path is unchanged' do + let_it_be(:invalid_path_project) do + project = create(:project, :repository, :public) + project.update_attribute(:path, 'foo.') + project + end + + 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}") @@ -586,6 +598,38 @@ 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 -- GitLab From 11c98170da322adb066c51d1001baed1417b4d0e Mon Sep 17 00:00:00 2001 From: Fiona Neill Date: Mon, 21 Feb 2022 16:41:58 +0000 Subject: [PATCH 09/12] Fix the project path validation message --- lib/gitlab/regex.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index a8c1667265aea6..c9202c6c54c632 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -265,7 +265,7 @@ def oci_repository_path_regex end def oci_repository_path_regex_message - 'must not start or end with special characters and must not contain consecutive special characters.' + 'must not start or end with a special character and must not contain consecutive special characters.' end def group_name_regex -- GitLab From ccd1a1cb9d0e7799f360df6b3de1d11290cadd25 Mon Sep 17 00:00:00 2001 From: Reuben Pereira <2967854-rpereira2@users.noreply.gitlab.com> Date: Tue, 22 Feb 2022 08:23:07 +0000 Subject: [PATCH 10/12] Use default_enabled param in feature flag --- app/models/project.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index e21f5575b14339..d5d71f1acb04af 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -894,9 +894,9 @@ def initialize(attributes = nil) end def project_path_oci_validation - if Feature.enabled?(:restrict_special_characters_in_project_path, self) && - path_changed? && - !path.match?(Gitlab::Regex.oci_repository_path_regex) + 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 -- GitLab From 5b1876cb05cc6147aa0f01cbbaa2bcb9fdf8b3e4 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Wed, 23 Feb 2022 13:58:05 +0530 Subject: [PATCH 11/12] Rename method and remove unused variable --- app/models/project.rb | 4 ++-- spec/lib/learn_gitlab/project_spec.rb | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index d5d71f1acb04af..5009824bba7de5 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -502,7 +502,7 @@ def self.integration_association_name(name) presence: true, project_path: true, length: { maximum: 255 } - validate :project_path_oci_validation + validate :container_registry_project_path_validation validates :project_feature, presence: true @@ -893,7 +893,7 @@ def initialize(attributes = nil) super end - def project_path_oci_validation + 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) diff --git a/spec/lib/learn_gitlab/project_spec.rb b/spec/lib/learn_gitlab/project_spec.rb index 30820b5189e4af..23784709817d32 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, path: 'learn-gitlab-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) } -- GitLab From 4e6b9a71f04c7e486d7f873b25c3982620a1c341 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Wed, 2 Mar 2022 15:26:28 +0530 Subject: [PATCH 12/12] Update doc for project path validation --- doc/api/projects.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/projects.md b/doc/api/projects.md index 90561846fc31e0..ee649377745d46 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). | -- GitLab