diff options
22 files changed, 323 insertions, 20 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index ef2ce0ab529..7e54cb1ccee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.8.3 (2019-03-19) + +### Security (1 change) + +- Remove project serialization in quick actions response. + + ## 11.8.2 (2019-03-13) ### Security (1 change) @@ -1 +1 @@ -11.8.2 +11.8.3 diff --git a/app/assets/javascripts/issue.js b/app/assets/javascripts/issue.js index 94b78907d9a..b3508f36cf9 100644 --- a/app/assets/javascripts/issue.js +++ b/app/assets/javascripts/issue.js @@ -16,7 +16,9 @@ export default class Issue { Issue.createMrDropdownWrap = document.querySelector('.create-mr-dropdown-wrap'); Issue.initMergeRequests(); - Issue.initRelatedBranches(); + if (document.querySelector('#related-branches')) { + Issue.initRelatedBranches(); + } this.closeButtons = $('a.btn-close'); this.reopenButtons = $('a.btn-reopen'); diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 0319948a12f..80b9bdc8f24 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -54,7 +54,7 @@ module NotesActions respond_to do |format| format.json do json = { - commands_changes: @note.commands_changes + commands_changes: @note.commands_changes&.slice(:emoji_award, :time_estimate, :spend_time) } if @note.persisted? && return_discussion? diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index b9d02a62fc3..2cb40697b5c 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -39,6 +39,7 @@ class Projects::IssuesController < Projects::ApplicationController before_action :authorize_create_merge_request_from!, only: [:create_merge_request] before_action :authorize_import_issues!, only: [:import_csv] + before_action :authorize_download_code!, only: [:related_branches] before_action :set_suggested_issues_feature_flags, only: [:new] diff --git a/app/models/label.rb b/app/models/label.rb index 1c3db3eb35d..08ab07bba7a 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -126,6 +126,10 @@ class Label < ActiveRecord::Base fuzzy_search(query, [:title, :description]) end + def self.by_ids(ids) + where(id: ids) + end + def open_issues_count(user = nil) issues_count(user, state: 'opened') end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 85143c5d339..504c1ec5c3a 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -178,7 +178,6 @@ class ProjectPolicy < BasePolicy enable :read_cycle_analytics enable :award_emoji enable :read_pages_content - enable :read_release end # These abilities are not allowed to admins that are not members of the project, @@ -204,6 +203,7 @@ class ProjectPolicy < BasePolicy enable :read_deployment enable :read_merge_request enable :read_sentry_issue + enable :read_release end # We define `:public_user_access` separately because there are cases in gitlab-ee diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 1e1f2fbd08e..e1390ecb59c 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -70,10 +70,14 @@ class IssuableBaseService < BaseService end def filter_labels - filter_labels_in_param(:add_label_ids) - filter_labels_in_param(:remove_label_ids) - filter_labels_in_param(:label_ids) - find_or_create_label_ids + params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids) if params[:add_label_ids] + params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids) if params[:remove_label_ids] + + if params[:label_ids] + params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids) + elsif params[:labels] + params[:label_ids] = labels_service.find_or_create_by_titles.map(&:id) + end end # rubocop: disable CodeReuse/ActiveRecord @@ -101,6 +105,10 @@ class IssuableBaseService < BaseService end.compact end + def labels_service + @labels_service ||= ::Labels::AvailableLabelsService.new(current_user, parent, params) + end + def process_label_ids(attributes, existing_label_ids: nil) label_ids = attributes.delete(:label_ids) add_label_ids = attributes.delete(:add_label_ids) @@ -118,10 +126,6 @@ class IssuableBaseService < BaseService new_label_ids end - def available_labels - @available_labels ||= LabelsFinder.new(current_user, project_id: @project.id, include_ancestor_groups: true).execute - end - def handle_quick_actions_on_create(issuable) merge_quick_actions_into_params!(issuable) end diff --git a/app/services/labels/available_labels_service.rb b/app/services/labels/available_labels_service.rb new file mode 100644 index 00000000000..fe477d96970 --- /dev/null +++ b/app/services/labels/available_labels_service.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true +module Labels + class AvailableLabelsService + attr_reader :current_user, :parent, :params + + def initialize(current_user, parent, params) + @current_user = current_user + @parent = parent + @params = params + end + + def find_or_create_by_titles + labels = params.delete(:labels) + + return [] unless labels + + labels = labels.split(',') if labels.is_a?(String) + + labels.map do |label_name| + label = Labels::FindOrCreateService.new( + current_user, + parent, + include_ancestor_groups: true, + title: label_name.strip, + available_labels: available_labels + ).execute + + label + end.compact + end + + def filter_labels_ids_in_param(key) + return [] if params[key].to_a.empty? + + # rubocop:disable CodeReuse/ActiveRecord + available_labels.by_ids(params[key]).pluck(:id) + # rubocop:enable CodeReuse/ActiveRecord + end + + private + + def available_labels + @available_labels ||= LabelsFinder.new(current_user, finder_params).execute + end + + def finder_params + params = { include_ancestor_groups: true } + + case parent + when Group + params[:group_id] = parent.id + params[:only_group_labels] = true + when Project + params[:project_id] = parent.id + end + + params + end + end +end diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 653b7d4c6f3..0f65560cd7e 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -77,8 +77,9 @@ #merge-requests{ data: { url: referenced_merge_requests_project_issue_path(@project, @issue) } } // This element is filled in using JavaScript. - #related-branches{ data: { url: related_branches_project_issue_path(@project, @issue) } } - // This element is filled in using JavaScript. + - if can?(current_user, :download_code, @project) + #related-branches{ data: { url: related_branches_project_issue_path(@project, @issue) } } + // This element is filled in using JavaScript. .content-block.emoji-block.emoji-block-sticky .row diff --git a/app/views/projects/merge_requests/conflicts/_submit_form.html.haml b/app/views/projects/merge_requests/conflicts/_submit_form.html.haml index 8181267184a..55c89f137c5 100644 --- a/app/views/projects/merge_requests/conflicts/_submit_form.html.haml +++ b/app/views/projects/merge_requests/conflicts/_submit_form.html.haml @@ -6,7 +6,7 @@ .form-group.row .col-md-4 %h4= _('Resolve conflicts on source branch') - .resolve-info + .resolve-info{ "v-pre": true } = translation.html_safe .col-md-8 %label.label-bold{ "for" => "commit-message" } diff --git a/changelogs/unreleased/disallow-guests-to-access-releases.yml b/changelogs/unreleased/disallow-guests-to-access-releases.yml new file mode 100644 index 00000000000..f2d518108d2 --- /dev/null +++ b/changelogs/unreleased/disallow-guests-to-access-releases.yml @@ -0,0 +1,5 @@ +--- +title: Disallow guest users from accessing Releases +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-56224.yml b/changelogs/unreleased/security-56224.yml new file mode 100644 index 00000000000..a4e274e6ca5 --- /dev/null +++ b/changelogs/unreleased/security-56224.yml @@ -0,0 +1,5 @@ +--- +title: Hide "related branches" when user does not have permission +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-56927-xss-resolve-conflicts-branch-name.yml b/changelogs/unreleased/security-56927-xss-resolve-conflicts-branch-name.yml new file mode 100644 index 00000000000..f92d2c0dcb1 --- /dev/null +++ b/changelogs/unreleased/security-56927-xss-resolve-conflicts-branch-name.yml @@ -0,0 +1,5 @@ +--- +title: Fix XSS in resolve conflicts form +merge_request: +author: +type: security diff --git a/doc/update/11.7-to-11.8.md b/doc/update/11.7-to-11.8.md index d5cd557d7b5..359b767d456 100644 --- a/doc/update/11.7-to-11.8.md +++ b/doc/update/11.7-to-11.8.md @@ -30,8 +30,8 @@ sudo -u git -H bundle exec rake gitlab:backup:create RAILS_ENV=production ### 3. Update Ruby -NOTE: Beginning in GitLab 11.0, we only support Ruby 2.4 or higher, and dropped -support for Ruby 2.3. Be sure to upgrade if necessary. +NOTE: Beginning in GitLab 11.6, we only support Ruby 2.5 or higher, and dropped +support for Ruby 2.4. Be sure to upgrade if necessary. You can check which version you are running with `ruby -v`. diff --git a/ee/changelogs/unreleased/security-milestone-labels.yml b/ee/changelogs/unreleased/security-milestone-labels.yml new file mode 100644 index 00000000000..4f8abcbc8be --- /dev/null +++ b/ee/changelogs/unreleased/security-milestone-labels.yml @@ -0,0 +1,5 @@ +--- +title: Check label_ids parent when updating issue board +merge_request: +author: +type: security diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 81892575889..ec91a760388 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -397,6 +397,37 @@ describe Projects::NotesController do end end end + + context 'when creating a note with quick actions' do + context 'with commands that return changes' do + let(:note_text) { "/award :thumbsup:\n/estimate 1d\n/spend 3h" } + + it 'includes changes in commands_changes ' do + post :create, params: request_params.merge(note: { note: note_text }, format: :json) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['commands_changes']).to include('emoji_award', 'time_estimate', 'spend_time') + expect(json_response['commands_changes']).not_to include('target_project', 'title') + end + end + + context 'with commands that do not return changes' do + let(:issue) { create(:issue, project: project) } + let(:other_project) { create(:project) } + let(:note_text) { "/move #{other_project.full_path}\n/title AAA" } + + before do + other_project.add_developer(user) + end + + it 'does not include changes in commands_changes' do + post :create, params: request_params.merge(note: { note: note_text }, target_type: 'issue', target_id: issue.id, format: :json) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['commands_changes']).not_to include('target_project', 'title') + end + end + end end describe 'PUT update' do diff --git a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb index 693ad89069c..0a006011c89 100644 --- a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb +++ b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' describe 'User creates branch and merge request on issue page', :js do + let(:membership_level) { :developer } let(:user) { create(:user) } let!(:project) { create(:project, :repository) } let(:issue) { create(:issue, project: project, title: 'Cherry-Coloured Funk') } @@ -17,7 +18,7 @@ describe 'User creates branch and merge request on issue page', :js do context 'when signed in' do before do - project.add_developer(user) + project.add_user(user, membership_level) sign_in(user) end @@ -167,6 +168,39 @@ describe 'User creates branch and merge request on issue page', :js do expect(page).not_to have_css('.create-mr-dropdown-wrap') end end + + context 'when related branch exists' do + let!(:project) { create(:project, :repository, :private) } + let(:branch_name) { "#{issue.iid}-foo" } + + before do + project.repository.create_branch(branch_name, 'master') + + visit project_issue_path(project, issue) + end + + context 'when user is developer' do + it 'shows related branches' do + expect(page).to have_css('#related-branches') + + wait_for_requests + + expect(page).to have_content(branch_name) + end + end + + context 'when user is guest' do + let(:membership_level) { :guest } + + it 'does not show related branches' do + expect(page).not_to have_css('#related-branches') + + wait_for_requests + + expect(page).not_to have_content(branch_name) + end + end + end end private diff --git a/spec/features/merge_request/user_resolves_conflicts_spec.rb b/spec/features/merge_request/user_resolves_conflicts_spec.rb index 16c058ab6bd..8fd44b87e5a 100644 --- a/spec/features/merge_request/user_resolves_conflicts_spec.rb +++ b/spec/features/merge_request/user_resolves_conflicts_spec.rb @@ -164,6 +164,21 @@ describe 'Merge request > User resolves conflicts', :js do expect(page).to have_content('Gregor Samsa woke from troubled dreams') end end + + context "with malicious branch name" do + let(:bad_branch_name) { "malicious-branch-{{toString.constructor('alert(/xss/)')()}}" } + let(:branch) { project.repository.create_branch(bad_branch_name, 'conflict-resolvable') } + let(:merge_request) { create_merge_request(branch.name) } + + before do + visit project_merge_request_path(project, merge_request) + click_link('conflicts', href: %r{/conflicts\Z}) + end + + it "renders bad name without xss issues" do + expect(find('.resolve-conflicts-form .resolve-info')).to have_content(bad_branch_name) + end + end end UNRESOLVABLE_CONFLICTS = { diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index f8d581ef38f..467aa9df95c 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -15,7 +15,7 @@ describe ProjectPolicy do read_project_for_iids read_issue_iid read_label read_milestone read_project_snippet read_project_member read_note create_project create_issue create_note upload_file create_merge_request_in - award_emoji read_release + award_emoji ] end @@ -24,7 +24,7 @@ describe ProjectPolicy do download_code fork_project create_project_snippet update_issue admin_issue admin_label admin_list read_commit_status read_build read_container_image read_pipeline read_environment read_deployment - read_merge_request download_wiki_code read_sentry_issue + read_merge_request download_wiki_code read_sentry_issue read_release ] end diff --git a/spec/requests/api/releases_spec.rb b/spec/requests/api/releases_spec.rb index 1f317971a66..71ec091c42c 100644 --- a/spec/requests/api/releases_spec.rb +++ b/spec/requests/api/releases_spec.rb @@ -4,12 +4,14 @@ describe API::Releases do let(:project) { create(:project, :repository, :private) } let(:maintainer) { create(:user) } let(:reporter) { create(:user) } + let(:guest) { create(:user) } let(:non_project_member) { create(:user) } let(:commit) { create(:commit, project: project) } before do project.add_maintainer(maintainer) project.add_reporter(reporter) + project.add_guest(guest) project.repository.add_tag(maintainer, 'v0.1', commit.id) project.repository.add_tag(maintainer, 'v0.2', commit.id) @@ -66,6 +68,24 @@ describe API::Releases do end end + context 'when user is a guest' do + it 'responds 403 Forbidden' do + get api("/projects/#{project.id}/releases", guest) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + context 'when project is public' do + let(:project) { create(:project, :repository, :public) } + + it 'responds 200 OK' do + get api("/projects/#{project.id}/releases", guest) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + context 'when user is not a project member' do it 'cannot find the project' do get api("/projects/#{project.id}/releases", non_project_member) @@ -189,6 +209,24 @@ describe API::Releases do end end end + + context 'when user is a guest' do + it 'responds 403 Forbidden' do + get api("/projects/#{project.id}/releases/v0.1", guest) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + context 'when project is public' do + let(:project) { create(:project, :repository, :public) } + + it 'responds 200 OK' do + get api("/projects/#{project.id}/releases/v0.1", guest) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end end context 'when specified tag is not found in the project' do diff --git a/spec/services/labels/available_labels_service_spec.rb b/spec/services/labels/available_labels_service_spec.rb new file mode 100644 index 00000000000..4d5c87ecc53 --- /dev/null +++ b/spec/services/labels/available_labels_service_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Labels::AvailableLabelsService do + let(:user) { create(:user) } + let(:project) { create(:project, :public, group: group) } + let(:group) { create(:group) } + + let(:project_label) { create(:label, project: project) } + let(:other_project_label) { create(:label) } + let(:group_label) { create(:group_label, group: group) } + let(:other_group_label) { create(:group_label) } + let(:labels) { [project_label, other_project_label, group_label, other_group_label] } + + context '#find_or_create_by_titles' do + let(:label_titles) { labels.map(&:title).push('non existing title') } + + context 'when parent is a project' do + context 'when a user is not a project member' do + it 'returns only relevant label ids' do + result = described_class.new(user, project, labels: label_titles).find_or_create_by_titles + + expect(result).to match_array([project_label, group_label]) + end + end + + context 'when a user is a project member' do + before do + project.add_developer(user) + end + + it 'creates new labels for not found titles' do + result = described_class.new(user, project, labels: label_titles).find_or_create_by_titles + + expect(result.count).to eq(5) + expect(result).to include(project_label, group_label) + expect(result).not_to include(other_project_label, other_group_label) + end + end + end + + context 'when parent is a group' do + context 'when a user is not a group member' do + it 'returns only relevant label ids' do + result = described_class.new(user, group, labels: label_titles).find_or_create_by_titles + + expect(result).to match_array([group_label]) + end + end + + context 'when a user is a group member' do + before do + group.add_developer(user) + end + + it 'creates new labels for not found titles' do + result = described_class.new(user, group, labels: label_titles).find_or_create_by_titles + + expect(result.count).to eq(5) + expect(result).to include(group_label) + expect(result).not_to include(project_label, other_project_label, other_group_label) + end + end + end + end + + context '#filter_labels_ids_in_param' do + let(:label_ids) { labels.map(&:id).push(99999) } + + context 'when parent is a project' do + it 'returns only relevant label ids' do + result = described_class.new(user, project, ids: label_ids).filter_labels_ids_in_param(:ids) + + expect(result).to match_array([project_label.id, group_label.id]) + end + end + + context 'when parent is a group' do + it 'returns only relevant label ids' do + result = described_class.new(user, group, ids: label_ids).filter_labels_ids_in_param(:ids) + + expect(result).to match_array([group_label.id]) + end + end + end +end |