Merge branch 'security-unique-domain-takeover-fix-17-5' into '17-5-stable-ee'

Restrict user and group creation when same pages unique domain exist

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/4576



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Eugie Limpin <elimpin@gitlab.com>
Co-authored-by: ngala <ngala@gitlab.com>

app/models/group.rb

@@ -172,6 +172,8 @@ def of_ancestors_and_self
 
   validates :group_feature, presence: true
 
+  validate :top_level_group_name_not_assigned_to_pages_unique_domain, if: :path_changed?
+
   add_authentication_token_field :runners_token,
     encrypted: :required,
     format_with_prefix: :runners_token_prefix,
@@ -1108,6 +1110,15 @@ def two_factor_authentication_allowed
   def runners_token_prefix
     RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX
   end
+
+  def top_level_group_name_not_assigned_to_pages_unique_domain
+    return unless parent_id.nil?
+
+    return unless ProjectSetting.unique_domain_exists?(path)
+
+    # We cannot disclose the Pages unique domain, hence returning generic error message
+    errors.add(:path, _('has already been taken'))
+  end
 end
 
 Group.prepend_mod_with('Group')

app/models/namespace.rb

@@ -333,7 +333,7 @@ def gfm_autocomplete_search(query)
     def clean_path(path, limited_to: Namespace.all)
       slug = Gitlab::Slug::Path.new(path).generate
       path = Namespaces::RandomizedSuffixPath.new(slug)
-      Gitlab::Utils::Uniquify.new.string(path) { |s| limited_to.find_by_path_or_name(s) }
+      Gitlab::Utils::Uniquify.new.string(path) { |s| limited_to.find_by_path_or_name(s) || ProjectSetting.unique_domain_exists?(s) }
     end
 
     def clean_name(value)

app/models/user.rb

@@ -325,6 +325,7 @@ def update_tracked_fields!(request)
     presence: true,
     exclusion: { in: Gitlab::PathRegex::TOP_LEVEL_ROUTES, message: N_('%{value} is a reserved name') }
   validates :username, uniqueness: true, unless: :namespace
+  validate :username_not_assigned_to_pages_unique_domain, if: :username_changed?
   validates :name, presence: true, length: { maximum: 255 }
   validates :first_name, length: { maximum: 127 }
   validates :last_name, length: { maximum: 127 }
@@ -2785,6 +2786,13 @@ def audit_lock_access(reason: nil); end
 
   # method overridden in EE
   def audit_unlock_access(author: self); end
+
+  def username_not_assigned_to_pages_unique_domain
+    if ProjectSetting.unique_domain_exists?(username)
+      # We cannot disclose the Pages unique domain, hence returning generic error message
+      errors.add(:username, _('has already been taken'))
+    end
+  end
 end
 
 User.prepend_mod_with('User')

ee/spec/support/shared_examples/services/service_accounts/create_service_shared_examples.rb

@@ -86,5 +86,15 @@
       expect(result.status).to eq(:error)
       expect(result.message).to eq('Username has already been taken')
     end
+
+    it 'throws error when the username is assigned to another project pages unique domain' do
+      # Simulate the existing domain being in use
+      create(:project_setting, pages_unique_domain: 'test')
+
+      result = service.execute
+
+      expect(result.status).to eq(:error)
+      expect(result.message).to eq('Username has already been taken')
+    end
   end
 end

lib/api/namespaces.rb

@@ -93,7 +93,7 @@ def custom_namespace_present_options
         namespace_path = params[:id]
         existing_namespaces_within_the_parent = Namespace.without_project_namespaces.by_parent(params[:parent_id])
 
-        exists = existing_namespaces_within_the_parent.filter_by_path(namespace_path).exists?
+        exists = existing_namespaces_within_the_parent.filter_by_path(namespace_path).exists? || ProjectSetting.unique_domain_exists?(namespace_path)
         suggestions = exists ? [Namespace.clean_path(namespace_path, limited_to: existing_namespaces_within_the_parent)] : []
 
         present :exists, exists

lib/gitlab/pages.rb

@@ -57,7 +57,9 @@ def multiple_versions_enabled_for?(project)
       def generate_unique_domain(project)
         10.times do
           pages_unique_domain = Gitlab::Pages::RandomDomain.generate(project_path: project.path)
-          return pages_unique_domain unless ProjectSetting.unique_domain_exists?(pages_unique_domain)
+          return pages_unique_domain unless
+            ProjectSetting.unique_domain_exists?(pages_unique_domain) ||
+              Namespace.top_most.by_path(pages_unique_domain).present?
         end
 
         raise UniqueDomainGenerationFailure

spec/lib/gitlab/pages/virtual_host_finder_spec.rb

@@ -123,9 +123,10 @@
 
         context 'when a project path conflicts with a unique domain' do
           it 'prioritizes the unique domain project' do
-            group = create(:group, path: 'unique-domain')
+            group = build(:group, path: 'unique-domain')
+                      .tap { |g| g.save!(validate: false) }
             other_project = build(:project, path: 'unique-domain.example.com', group: group)
-              .tap { |project| project.save!(validate: false) }
+                              .tap { |project| project.save!(validate: false) }
 
             create(:pages_deployment, project: project)
             create(:pages_deployment, project: other_project)

spec/lib/gitlab/pages_spec.rb

@@ -146,6 +146,24 @@
         end
       end
 
+      RSpec.shared_examples 'generates a different unique domain' do |entity|
+        let!(:existing_entity) { create(entity, path: 'existing-path') }
+
+        context "when #{entity} path is already in use" do
+          it 'assigns a different unique domain to pages' do
+            allow(Gitlab::Pages::RandomDomain).to receive(:generate).and_return('existing-path', 'new-unique-domain')
+
+            described_class.add_unique_domain_to(project)
+
+            expect(project.project_setting.pages_unique_domain_enabled).to eq(true)
+            expect(project.project_setting.pages_unique_domain).to eq('new-unique-domain')
+          end
+        end
+      end
+
+      it_behaves_like 'generates a different unique domain', :group
+      it_behaves_like 'generates a different unique domain', :namespace
+
       context 'when generated 10 unique domains are already in use' do
         it 'raises an error' do
           allow(Gitlab::Pages::RandomDomain).to receive(:generate).and_return('existing-domain')

spec/models/group_spec.rb

@@ -316,6 +316,16 @@
 
         expect(group).not_to be_valid
       end
+
+      it 'rejects paths already assigned to any pages unique domain' do
+        # Simulate the existing domain being in use
+        create(:project_setting, pages_unique_domain: 'existing-domain')
+
+        group = build(:group, path: 'existing-domain')
+
+        expect(group).not_to be_valid
+        expect(group.errors.full_messages.to_sentence).to eq('Group URL has already been taken')
+      end
     end
 
     describe '#notification_settings' do
@@ -2746,6 +2756,18 @@ def setup_group_members(group)
           group.update!(name: 'new name')
         end
       end
+
+      context 'when the path is changed to existing pages unique domain' do
+        let(:new_path) { 'existing-domain' }
+
+        it 'rejects path' do
+          # Simulate the existing domain being in use
+          create(:project_setting, pages_unique_domain: 'existing-domain')
+
+          expect(group.update(path: new_path)).to be_falsey
+          expect(group.errors.full_messages.to_sentence).to eq('Group URL has already been taken')
+        end
+      end
     end
   end
 

spec/models/namespace_spec.rb

@@ -1416,12 +1416,14 @@
     it "cleans the path and makes sure it's available", time_travel_to: '2023-04-20 00:07 -0700' do
       create :user, username: "johngitlab-etc"
       create :namespace, path: "JohnGitLab-etc1"
+      create :project_setting, pages_unique_domain: 'existing-domain'
       [nil, 1, 2, 3].each do |count|
         create :namespace, path: "pickle#{count}"
       end
 
       expect(described_class.clean_path("-john+gitlab-ETC%.git@gmail.com")).to eq("johngitlab-ETC2")
       expect(described_class.clean_path("--%+--valid_*&%name=.git.%.atom.atom.@email.com")).to eq("valid_name")
+      expect(described_class.clean_path("existing-domain")).to eq("existing-domain1")
 
       # when we have more than MAX_TRIES count of a path use a more randomized suffix
       expect(described_class.clean_path("pickle@gmail.com")).to eq("pickle4")

spec/models/user_spec.rb

@@ -699,6 +699,18 @@
         end
       end
 
+      context 'when the username is assigned to another project pages unique domain' do
+        let(:username) { 'existing-domain' }
+
+        it 'is invalid' do
+          # Simulate the existing domain being in use
+          create(:project_setting, pages_unique_domain: 'existing-domain')
+
+          expect(user).not_to be_valid
+          expect(user.errors.full_messages).to eq(['Username has already been taken'])
+        end
+      end
+
       Mime::EXTENSION_LOOKUP.keys.each do |type|
         context 'with extension format' do
           let(:username) { "test.#{type}" }
@@ -6343,6 +6355,14 @@ def add_user(access)
               expect(user.errors[:base]).to include('A user, alias, or group already exists with that username.')
             end
           end
+
+          it 'when the username is assigned to another project pages unique domain' do
+            # Simulate the existing domain being in use
+            create(:project_setting, pages_unique_domain: 'existing-domain')
+
+            expect(user.update(username: 'existing-domain')).to be_falsey
+            expect(user.errors.full_messages).to eq(['Username has already been taken'])
+          end
         end
 
         context 'when the username is not changed' do

spec/requests/api/groups_spec.rb

@@ -1261,6 +1261,25 @@ def make_upload_request
           expect(json_response['description']).to eq('it works')
         end
       end
+
+      context 'update path with existing pages unique domain' do
+        before do
+          stub_pages_setting(enabled: true)
+
+          create(
+            :project_setting,
+            project: project1,
+            pages_unique_domain_enabled: true,
+            pages_unique_domain: 'existing-domain')
+        end
+
+        it "returns 400 bad request error" do
+          put api("/groups/#{group1.id}", user1), params: { path: 'existing-domain' }
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to eq({ "path" => ["has already been taken"] })
+        end
+      end
     end
 
     context 'when authenticated as the admin' do
@@ -2931,6 +2950,27 @@ def make_upload_request
 
         expect(response).to have_gitlab_http_status(:bad_request)
       end
+
+      context 'with existing pages unique domain' do
+        before do
+          stub_pages_setting(enabled: true)
+
+          create(
+            :project_setting,
+            project: project1,
+            pages_unique_domain_enabled: true,
+            pages_unique_domain: 'existing-domain')
+        end
+
+        it "returns 400 bad request error if path is already used by pages unique domain" do
+          expect do
+            post api("/groups", user3), params: { name: 'test', path: 'existing-domain' }
+          end.not_to change { Group.count }
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to eq("Failed to save group {:path=>[\"has already been taken\"]}")
+        end
+      end
     end
   end
 

spec/requests/api/namespaces_spec.rb

@@ -367,6 +367,17 @@ def request
         expect(response.body).to eq(expected_json)
       end
 
+      it 'returns JSON indicating the namespace exists and a suggestion when same pages unique domain exists' do
+        # Simulate the existing domain being in use
+        create(:project_setting, pages_unique_domain: 'existing-domain')
+
+        get api("#{path}/existing-domain/exists", user)
+
+        expected_json = { exists: true, suggests: ["existing-domain1"] }.to_json
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response.body).to eq(expected_json)
+      end
+
       it 'ignores paths of groups present in other hierarchies when making suggestions' do
         (1..2).to_a.each do |suffix|
           create(:group, name: "mygroup#{suffix}", path: "mygroup#{suffix}", parent: namespace2)

spec/requests/api/users_spec.rb

@@ -1559,6 +1559,34 @@
       end
     end
 
+    context 'with existing pages unique domain' do
+      let_it_be(:project) { create(:project) }
+
+      before do
+        stub_pages_setting(enabled: true)
+
+        create(
+          :project_setting,
+          project: project,
+          pages_unique_domain_enabled: true,
+          pages_unique_domain: 'unique-domain')
+      end
+
+      it 'returns 400 bad request error if same username is already used by pages unique domain' do
+        expect do
+          post api(path, admin, admin_mode: true),
+            params: {
+              name: 'foo',
+              email: 'foo@example.com',
+              password: User.random_password,
+              username: 'unique-domain'
+            }
+        end.to change { User.count }.by(0)
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']).to eq({ "username" => ["has already been taken"] })
+      end
+    end
+
     context 'when user with a primary email exists' do
       context 'when the primary email is confirmed' do
         let!(:confirmed_user) { create(:user, email: 'foo@example.com') }
@@ -1847,6 +1875,26 @@ def update_password(user, admin, password = User.random_password)
       expect(user.reload.username).to eq(user.username)
     end
 
+    context 'with existing pages unique domain' do
+      let_it_be(:project) { create(:project) }
+
+      before do
+        stub_pages_setting(enabled: true)
+
+        create(
+          :project_setting,
+          project: project,
+          pages_unique_domain_enabled: true,
+          pages_unique_domain: 'unique-domain')
+      end
+
+      it 'returns 400 bad request error if same username is already used by pages unique domain' do
+        put api(path, admin, admin_mode: true), params: { username: 'unique-domain' }
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']).to eq({ "username" => ["has already been taken"] })
+      end
+    end
+
     it "updates user's existing identity" do
       put api("/users/#{ldap_user.id}", admin, admin_mode: true), params: { provider: 'ldapmain', extern_uid: '654321' }