Merge branch 'milestones-finder-order-fix' into 'security-10-3'

Remove order param from the MilestoneFinder

See merge request gitlab/gitlabhq!2259

(cherry picked from commit 14408042e78f2ebc2644f956621b461dbfa3d36d)

155881e7 Remove order param from the MilestoneFinder

7 files changed, 43 insertions, 54 deletions

diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index f013d21275e..acf6aaf57f4 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -75,8 +75,6 @@ class Groups::MilestonesController < Groups::ApplicationController
   end
 
   def milestones
-    search_params = params.merge(group_ids: group.id)
-
     milestones = MilestonesFinder.new(search_params).execute
     legacy_milestones = GroupMilestone.build_collection(group, group_projects, params)
 
@@ -94,4 +92,8 @@ class Groups::MilestonesController < Groups::ApplicationController
 
     render_404 unless @milestone
   end
+
+  def search_params
+    params.permit(:state).merge(group_ids: group.id)
+  end
 end
diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index 980bbf699b6..0f70efbce40 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/app/controllers/projects/milestones_controller.rb
@@ -92,12 +92,6 @@ class Projects::MilestonesController < Projects::ApplicationController
 
   def milestones
     @milestones ||= begin
-      if @project.group && can?(current_user, :read_group, @project.group)
-        group = @project.group
-      end
-
-      search_params = params.merge(project_ids: @project.id, group_ids: group&.id)
-
       MilestonesFinder.new(search_params).execute
     end
   end
@@ -113,4 +107,12 @@ class Projects::MilestonesController < Projects::ApplicationController
   def milestone_params
     params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event)
   end
+
+  def search_params
+    if @project.group && can?(current_user, :read_group, @project.group)
+      group = @project.group
+    end
+
+    params.permit(:state).merge(project_ids: @project.id, group_ids: group&.id)
+  end
 end
diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb
index 0a5a0ea2f35..b4605fca193 100644
--- a/app/finders/milestones_finder.rb
+++ b/app/finders/milestones_finder.rb
@@ -46,11 +46,7 @@ class MilestonesFinder
   end
 
   def order(items)
-    if params.has_key?(:order)
-      items.reorder(params[:order])
-    else
-      order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
-      items.reorder(order_statement)
-    end
+    order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC')
+    items.reorder(order_statement).order('title ASC')
   end
 end
diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb
index c0864769314..dc2f6817190 100644
--- a/app/models/global_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -44,10 +44,10 @@ class GlobalMilestone
   def self.group_milestones_states_count(group)
     return STATE_COUNT_HASH unless group
 
-    params = { group_ids: [group.id], state: 'all', order: nil }
+    params = { group_ids: [group.id], state: 'all' }
 
     relation = MilestonesFinder.new(params).execute
-    grouped_by_state = relation.group(:state).count
+    grouped_by_state = relation.reorder(nil).group(:state).count
 
     {
       opened: grouped_by_state['active'] || 0,
@@ -60,10 +60,10 @@ class GlobalMilestone
   def self.legacy_group_milestone_states_count(projects)
     return STATE_COUNT_HASH unless projects
 
-    params = { project_ids: projects.map(&:id), state: 'all', order: nil }
+    params = { project_ids: projects.map(&:id), state: 'all' }
 
     relation = MilestonesFinder.new(params).execute
-    project_milestones_by_state_and_title = relation.group(:state, :title).count
+    project_milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count
 
     opened = count_by_state(project_milestones_by_state_and_title, 'active')
     closed = count_by_state(project_milestones_by_state_and_title, 'closed')
diff --git a/changelogs/unreleased/milestones-finder-order-fix.yml b/changelogs/unreleased/milestones-finder-order-fix.yml
new file mode 100644
index 00000000000..983c301a82d
--- /dev/null
+++ b/changelogs/unreleased/milestones-finder-order-fix.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent a SQL injection in the MilestonesFinder
+merge_request:
+author:
+type: security
diff --git a/spec/finders/milestones_finder_spec.rb b/spec/finders/milestones_finder_spec.rb
index 8ae08656e01..0b3cf7ece5f 100644
--- a/spec/finders/milestones_finder_spec.rb
+++ b/spec/finders/milestones_finder_spec.rb
@@ -21,10 +21,19 @@ describe MilestonesFinder do
     expect(result).to contain_exactly(milestone_1, milestone_2)
   end
 
-  it 'returns milestones for groups and projects' do
-    result = described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute
+  context 'milestones for groups and project' do
+    let(:result) do
+      described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute
+    end
+
+    it 'returns milestones for groups and projects' do
+      expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4)
+    end
 
-    expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4)
+    it 'orders milestones by due date' do
+      expect(result.first).to eq(milestone_1)
+      expect(result.second).to eq(milestone_3)
+    end
   end
 
   context 'with filters' do
@@ -61,30 +70,4 @@ describe MilestonesFinder do
       expect(result.to_a).to contain_exactly(milestone_1)
     end
   end
-
-  context 'with order' do
-    let(:params) do
-      {
-        project_ids: [project_1.id, project_2.id],
-        group_ids: group.id,
-        state: 'all'
-      }
-    end
-
-    it "default orders by due date" do
-      result = described_class.new(params).execute
-
-      expect(result.first).to eq(milestone_1)
-      expect(result.second).to eq(milestone_3)
-    end
-
-    it "orders by parameter" do
-      result = described_class.new(params.merge(order: 'id DESC')).execute
-
-      expect(result.first).to eq(milestone_4)
-      expect(result.second).to eq(milestone_3)
-      expect(result.third).to eq(milestone_2)
-      expect(result.fourth).to eq(milestone_1)
-    end
-  end
 end
diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb
index 7a8c54673f7..f7ff8b80bd7 100644
--- a/spec/services/projects/autocomplete_service_spec.rb
+++ b/spec/services/projects/autocomplete_service_spec.rb
@@ -93,26 +93,27 @@ describe Projects::AutocompleteService do
     let(:user) { create(:user) }
     let(:group) { create(:group) }
     let(:project) { create(:project, group: group) }
-    let!(:group_milestone) { create(:milestone, group: group) }
-    let!(:project_milestone) { create(:milestone, project: project) }
+    let!(:group_milestone1) { create(:milestone, group: group, due_date: '2017-01-01', title: 'Second Title') }
+    let!(:group_milestone2) { create(:milestone, group: group, due_date: '2017-01-01', title: 'First Title') }
+    let!(:project_milestone) { create(:milestone, project: project, due_date: '2016-01-01') }
 
     let(:milestone_titles) { described_class.new(project, user).milestones.map(&:title) }
 
-    it 'includes project and group milestones' do
-      expect(milestone_titles).to eq([group_milestone.title, project_milestone.title])
+    it 'includes project and group milestones and sorts them correctly' do
+      expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title, group_milestone1.title])
     end
 
     it 'does not include closed milestones' do
-      group_milestone.close
+      group_milestone1.close
 
-      expect(milestone_titles).to eq([project_milestone.title])
+      expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title])
     end
 
     it 'does not include milestones from other projects in the group' do
       other_project = create(:project, group: group)
       project_milestone.update!(project: other_project)
 
-      expect(milestone_titles).to eq([group_milestone.title])
+      expect(milestone_titles).to eq([group_milestone2.title, group_milestone1.title])
     end
   end
 end