diff options
| -rw-r--r-- | app/controllers/groups/milestones_controller.rb | 6 | ||||
| -rw-r--r-- | app/controllers/projects/milestones_controller.rb | 14 | ||||
| -rw-r--r-- | app/finders/milestones_finder.rb | 8 | ||||
| -rw-r--r-- | app/models/global_milestone.rb | 8 | ||||
| -rw-r--r-- | changelogs/unreleased/milestones-finder-order-fix.yml | 5 | ||||
| -rw-r--r-- | spec/finders/milestones_finder_spec.rb | 41 | ||||
| -rw-r--r-- | spec/services/projects/autocomplete_service_spec.rb | 15 |
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 |