Escape the underscore char inside the LIKE operator33323-incorrect-project-authorizations

An underscore can be used as a wildcard inside the LIKE operator, if we
really want to match it then we need to escape it otherwise the query
will return invalid results.

3 files changed, 56 insertions, 3 deletions

diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index c4463abdfe6..2f6520521c8 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -94,7 +94,7 @@ module Routable
     # Returns an ActiveRecord::Relation.
     def member_descendants(user_id)
       joins(:route).
-        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%')
+        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(REPLACE(r2.path, '_', '\\_'), '/%')
                INNER JOIN members ON members.source_id = r2.source_id
                AND members.source_type = r2.source_type").
         where('members.user_id = ?', user_id)
@@ -111,7 +111,7 @@ module Routable
     # Returns an ActiveRecord::Relation.
     def member_self_and_descendants(user_id)
       joins(:route).
-        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%')
+        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(REPLACE(r2.path, '_', '\\_'), '/%')
                OR routes.path = r2.path
                INNER JOIN members ON members.source_id = r2.source_id
                AND members.source_type = r2.source_type").
@@ -162,7 +162,7 @@ module Routable
       wheres = paths.map do |path|
         "#{connection.quote(path)} = routes.path
          OR
-         #{connection.quote(path)} LIKE CONCAT(routes.path, '/%')"
+         #{connection.quote(path)} LIKE CONCAT(REPLACE(routes.path, '_', '\\_'), '/%')"
       end
 
       joins(:route).where(wheres.join(' OR '))
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index 49a4132f763..2f337c14d96 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -133,6 +133,29 @@ describe Group, 'Routable' do
     subject { described_class.member_self_and_descendants(user.id) }
 
     it { is_expected.to match_array [group, nested_group] }
+
+    context 'when the group has special chars in its path' do
+      let(:user1) { create(:user) }
+      let(:group1) { create(:group, name: 'demo', path: 'demo') }
+      let(:nested_group1) { create(:group, name: 'nest', path: 'nest', parent: group1) }
+      let!(:project1) { create(:empty_project, group: nested_group1) }
+
+      let(:user2) { create(:user) }
+      let(:group2) { create(:group, name: '____', path: '____') }
+      let(:nested_group2) { create(:group, name: 'test', path: 'test', parent: group2) }
+      let!(:project2) { create(:empty_project, group: nested_group2) }
+
+      before do
+        group1.add_master(user1)
+        group2.add_master(user2)
+      end
+
+      it 'only returns the right groups' do
+        groups = described_class.member_self_and_descendants(user2.id)
+
+        expect(groups).to match_array([group2, nested_group2])
+      end
+    end
   end
 
   describe '.member_hierarchy' do
diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb
index b19374ef1a2..39fae22c623 100644
--- a/spec/services/users/refresh_authorized_projects_service_spec.rb
+++ b/spec/services/users/refresh_authorized_projects_service_spec.rb
@@ -115,6 +115,36 @@ describe Users::RefreshAuthorizedProjectsService do
 
       expect(user.authorized_projects_populated).to eq(true)
     end
+
+    context 'when the group has special chars in its path' do
+      let(:user1) { create(:user) }
+      let(:group1) { create(:group, name: 'demo', path: 'demo') }
+      let(:nested_group1) { create(:group, name: 'nest', path: 'nest', parent: group1) }
+      let!(:project1) { create(:empty_project, group: nested_group1) }
+
+      let(:user2) { create(:user) }
+      let(:group2) { create(:group, name: '____', path: '____') }
+      let(:nested_group2) { create(:group, name: 'test', path: 'test', parent: group2) }
+      let!(:project2) { create(:empty_project, group: nested_group2) }
+
+      before do
+        group1.add_master(user1)
+        group2.add_master(user2)
+
+        Users::RefreshAuthorizedProjectsService.new(user1).execute
+        Users::RefreshAuthorizedProjectsService.new(user2).execute
+      end
+
+      it "it doesn't give authorization to foreign projects" do
+        expect(user1.authorized_projects).not_to include(project2)
+        expect(user2.authorized_projects).not_to include(project1)
+      end
+
+      it 'only gives authorization to the right projects' do
+        expect(user1.authorized_projects).to match_array([project1])
+        expect(user2.authorized_projects).to match_array([project2])
+      end
+    end
   end
 
   describe '#fresh_access_levels_per_project' do