From 0f711b0818888523b400e898b19c5a2954a2613d Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 21 Mar 2017 17:22:27 +0000 Subject: Merge branch '29583-routes-like-fix' into 'master' Escape route path for LIKE queries Closes #29583 See merge request !10117 --- app/models/namespace.rb | 2 +- app/models/project.rb | 2 +- app/models/route.rb | 4 +++- spec/models/project_spec.rb | 11 +++++++++-- spec/models/route_spec.rb | 24 ++++++++++++++++++++++-- 5 files changed, 36 insertions(+), 7 deletions(-) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index d350f1d6770..826ded22ae5 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -195,7 +195,7 @@ class Namespace < ActiveRecord::Base # Scopes the model on direct and indirect children of the record def descendants - self.class.joins(:route).where('routes.path LIKE ?', "#{route.path}/%").reorder('routes.path ASC') + self.class.joins(:route).merge(Route.inside_path(route.path)).reorder('routes.path ASC') end def user_ids_for_project_authorizations diff --git a/app/models/project.rb b/app/models/project.rb index da4704554b3..04641dd58a0 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -238,7 +238,7 @@ class Project < ActiveRecord::Base # We need routes alias rs for JOIN so it does not conflict with # includes(:route) which we use in ProjectsFinder. joins("INNER JOIN routes rs ON rs.source_id = projects.id AND rs.source_type = 'Project'"). - where('rs.path LIKE ?', "#{path}/%") + where('rs.path LIKE ?', "#{sanitize_sql_like(path)}/%") end # "enabled" here means "not disabled". It includes private features! diff --git a/app/models/route.rb b/app/models/route.rb index 41e6eb7cb73..4b3efab5c3c 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -10,9 +10,11 @@ class Route < ActiveRecord::Base after_update :rename_descendants + scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") } + def rename_descendants if path_changed? || name_changed? - descendants = Route.where('path LIKE ?', "#{path_was}/%") + descendants = self.class.inside_path(path_was) descendants.each do |route| attributes = {} diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 5e5f690acd4..2b5d6f84776 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1762,11 +1762,18 @@ describe Project, models: true do end describe 'inside_path' do - let!(:project1) { create(:empty_project) } + let!(:project1) { create(:empty_project, namespace: create(:namespace, path: 'name_pace')) } let!(:project2) { create(:empty_project) } + let!(:project3) { create(:empty_project, namespace: create(:namespace, path: 'namespace')) } let!(:path) { project1.namespace.full_path } - it { expect(Project.inside_path(path)).to eq([project1]) } + it 'returns 1 project' do + expect(Project.inside_path(path).count).to eq(1) + end + + it 'returns correct project' do + expect(Project.inside_path(path)).to eq([project1]) + end end describe '#route_map_for' do diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index bc8ae4ae5a8..05afd9f5b5a 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Route, models: true do - let!(:group) { create(:group, path: 'gitlab', name: 'gitlab') } + let!(:group) { create(:group, path: 'git_lab', name: 'git_lab') } let!(:route) { group.route } describe 'relationships' do @@ -14,10 +14,28 @@ describe Route, models: true do it { is_expected.to validate_uniqueness_of(:path) } end + describe '.inside_path' do + let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) } + let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) } + let!(:another_group) { create(:group, path: 'other') } + let!(:similar_group) { create(:group, path: 'gitllab') } + let!(:another_group_nested) { create(:group, path: 'another', name: 'another', parent: similar_group) } + + it 'returns 2 routes' do + expect(Route.inside_path('git_lab').count).to eq(2) + end + + it 'returns correct routes' do + expect(Route.inside_path('git_lab')).to match_array([nested_group.route, deep_nested_group.route]) + end + end + describe '#rename_descendants' do let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) } let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) } let!(:similar_group) { create(:group, path: 'gitlab-org', name: 'gitlab-org') } + let!(:another_group) { create(:group, path: 'gittlab', name: 'gitllab') } + let!(:another_group_nested) { create(:group, path: 'git_lab', name: 'git_lab', parent: another_group) } context 'path update' do context 'when route name is set' do @@ -28,6 +46,8 @@ describe Route, models: true do expect(described_class.exists?(path: 'bar/test')).to be_truthy expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy expect(described_class.exists?(path: 'gitlab-org')).to be_truthy + expect(described_class.exists?(path: 'gittlab')).to be_truthy + expect(described_class.exists?(path: 'gittlab/git_lab')).to be_truthy end end @@ -44,7 +64,7 @@ describe Route, models: true do context 'name update' do it "updates children routes with new path" do - route.update_attributes(name: 'bar') + route.update_attributes(name: 'bar') expect(described_class.exists?(name: 'bar')).to be_truthy expect(described_class.exists?(name: 'bar / test')).to be_truthy -- cgit v1.2.3