diff options
author | Winnie Hellmann <winnie@gitlab.com> | 2017-12-07 01:03:01 +0300 |
---|---|---|
committer | Winnie Hellmann <winnie@gitlab.com> | 2017-12-07 01:03:01 +0300 |
commit | 5773e4feedc062ed1f4b534f84b8c210a7a0e491 (patch) | |
tree | e0039e1665b37ea76308ad27080e061265e80695 | |
parent | 3141105ec49dd13434c2e8964b55427a764c7287 (diff) | |
parent | 0671211d44b36627787f44ceb4f95a6e40c33bf3 (diff) |
Merge branch '10-2-stable-patch-4' into '10-2-stable'
Prepare 10.2.4 security release
See merge request gitlab/gitlabhq!2241
-rw-r--r-- | app/assets/javascripts/notes/components/issue_note.vue | 3 | ||||
-rw-r--r-- | app/controllers/projects_controller.rb | 2 | ||||
-rw-r--r-- | app/helpers/preferences_helper.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 12 | ||||
-rw-r--r-- | changelogs/unreleased/bvl-email-disclosure.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/issue_30663.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/rs-security-group-api.yml | 5 | ||||
-rw-r--r-- | lib/api/entities.rb | 17 | ||||
-rw-r--r-- | lib/api/issues.rb | 2 | ||||
-rw-r--r-- | spec/factories/users.rb | 4 | ||||
-rw-r--r-- | spec/features/groups/members/manage_members.rb | 21 | ||||
-rw-r--r-- | spec/helpers/preferences_helper_spec.rb | 74 | ||||
-rw-r--r-- | spec/javascripts/notes/components/issue_note_spec.js | 15 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 35 | ||||
-rw-r--r-- | spec/requests/api/groups_spec.rb | 62 | ||||
-rw-r--r-- | spec/requests/api/issues_spec.rb | 14 |
16 files changed, 231 insertions, 47 deletions
diff --git a/app/assets/javascripts/notes/components/issue_note.vue b/app/assets/javascripts/notes/components/issue_note.vue index 40318f9a600..071e2d244fe 100644 --- a/app/assets/javascripts/notes/components/issue_note.vue +++ b/app/assets/javascripts/notes/components/issue_note.vue @@ -1,5 +1,6 @@ <script> import { mapGetters, mapActions } from 'vuex'; + import { escape } from 'underscore'; import Flash from '../../flash'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; import issueNoteHeader from './issue_note_header.vue'; @@ -85,7 +86,7 @@ }; this.isRequesting = true; this.oldContent = this.note.note_html; - this.note.note_html = noteText; + this.note.note_html = escape(noteText); this.updateNote(data) .then(() => { diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 2a473ec0cec..800e3cec762 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -271,7 +271,7 @@ class ProjectsController < Projects::ApplicationController return render 'projects/no_repo' unless @project.repository_exists? render 'projects/empty' if @project.empty_repo? else - if @project.wiki_enabled? + if can?(current_user, :read_wiki, @project) @project_wiki = @project.wiki @wiki_home = @project_wiki.find_page('home', params[:version_id]) elsif @project.feature_available?(:issues, current_user) diff --git a/app/helpers/preferences_helper.rb b/app/helpers/preferences_helper.rb index 8e822ed0ea2..aaee6eaeedd 100644 --- a/app/helpers/preferences_helper.rb +++ b/app/helpers/preferences_helper.rb @@ -58,7 +58,7 @@ module PreferencesHelper user_view elsif user_view == "activity" "activity" - elsif @project.wiki_enabled? + elsif can?(current_user, :read_wiki, @project) "wiki" elsif @project.feature_available?(:issues, current_user) "projects/issues/issues" diff --git a/app/models/user.rb b/app/models/user.rb index 3209138a772..316df26942d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -314,7 +314,8 @@ class User < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search(query) - table = arel_table + table = arel_table + query = query.downcase pattern = User.to_pattern(query) order = <<~SQL @@ -328,7 +329,7 @@ class User < ActiveRecord::Base where( table[:name].matches(pattern) - .or(table[:email].matches(pattern)) + .or(table[:email].eq(query)) .or(table[:username].matches(pattern)) ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) end @@ -340,12 +341,13 @@ class User < ActiveRecord::Base def search_with_secondary_emails(query) table = arel_table email_table = Email.arel_table - pattern = "%#{query}%" - matched_by_emails_user_ids = email_table.project(email_table[:user_id]).where(email_table[:email].matches(pattern)) + query = query.downcase + pattern = User.to_pattern(query) + matched_by_emails_user_ids = email_table.project(email_table[:user_id]).where(email_table[:email].eq(query)) where( table[:name].matches(pattern) - .or(table[:email].matches(pattern)) + .or(table[:email].eq(query)) .or(table[:username].matches(pattern)) .or(table[:id].in(matched_by_emails_user_ids)) ) diff --git a/changelogs/unreleased/bvl-email-disclosure.yml b/changelogs/unreleased/bvl-email-disclosure.yml new file mode 100644 index 00000000000..d6cd8709d9f --- /dev/null +++ b/changelogs/unreleased/bvl-email-disclosure.yml @@ -0,0 +1,5 @@ +--- +title: Don't match partial email adresses +merge_request: 2227 +author: +type: security diff --git a/changelogs/unreleased/issue_30663.yml b/changelogs/unreleased/issue_30663.yml new file mode 100644 index 00000000000..b20ed6a82e7 --- /dev/null +++ b/changelogs/unreleased/issue_30663.yml @@ -0,0 +1,5 @@ +--- +title: Prevent creating issues through API when user does not have permissions +merge_request: +author: +type: security diff --git a/changelogs/unreleased/rs-security-group-api.yml b/changelogs/unreleased/rs-security-group-api.yml new file mode 100644 index 00000000000..34a39ddd6dc --- /dev/null +++ b/changelogs/unreleased/rs-security-group-api.yml @@ -0,0 +1,5 @@ +--- +title: Prevent an information disclosure in the Groups API +merge_request: +author: +type: security diff --git a/lib/api/entities.rb b/lib/api/entities.rb index a382db92e8d..bcc0f6f86c9 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -212,8 +212,21 @@ module API end class GroupDetail < Group - expose :projects, using: Entities::Project - expose :shared_projects, using: Entities::Project + expose :projects, using: Entities::Project do |group, options| + GroupProjectsFinder.new( + group: group, + current_user: options[:current_user], + options: { only_owned: true } + ).execute + end + + expose :shared_projects, using: Entities::Project do |group, options| + GroupProjectsFinder.new( + group: group, + current_user: options[:current_user], + options: { only_shared: true } + ).execute + end end class Commit < Grape::Entity diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 74dfd9f96de..2ee7238135f 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -161,6 +161,8 @@ module API use :issue_params end post ':id/issues' do + authorize! :create_issue, user_project + # Setting created_at time only allowed for admins and project owners unless current_user.admin? || user_project.owner == current_user params.delete(:created_at) diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 4000cd085b7..8ace424f8af 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -58,6 +58,10 @@ FactoryGirl.define do end end + trait :readme do + project_view :readme + end + factory :omniauth_user do transient do extern_uid '123456' diff --git a/spec/features/groups/members/manage_members.rb b/spec/features/groups/members/manage_members.rb index da1e17225db..21f7b4999ad 100644 --- a/spec/features/groups/members/manage_members.rb +++ b/spec/features/groups/members/manage_members.rb @@ -38,6 +38,27 @@ feature 'Groups > Members > Manage members' do end end + scenario 'do not disclose email addresses', :js do + group.add_owner(user1) + create(:user, email: 'undisclosed_email@gitlab.com', name: "Jane 'invisible' Doe") + + visit group_group_members_path(group) + + find('.select2-container').click + select_input = find('.select2-input') + + select_input.send_keys('@gitlab.com') + wait_for_requests + + expect(page).to have_content('No matches found') + + select_input.native.clear + select_input.send_keys('undisclosed_email@gitlab.com') + wait_for_requests + + expect(page).to have_content("Jane 'invisible' Doe") + end + scenario 'remove user from group', :js do group.add_owner(user1) group.add_developer(user2) diff --git a/spec/helpers/preferences_helper_spec.rb b/spec/helpers/preferences_helper_spec.rb index 8b8080563d3..749aa25e632 100644 --- a/spec/helpers/preferences_helper_spec.rb +++ b/spec/helpers/preferences_helper_spec.rb @@ -77,15 +77,6 @@ describe PreferencesHelper do end end - def stub_user(messages = {}) - if messages.empty? - allow(helper).to receive(:current_user).and_return(nil) - else - allow(helper).to receive(:current_user) - .and_return(double('user', messages)) - end - end - describe '#default_project_view' do context 'user not signed in' do before do @@ -125,5 +116,70 @@ describe PreferencesHelper do end end end + + context 'user signed in' do + let(:user) { create(:user, :readme) } + let(:project) { create(:project, :public, :repository) } + + before do + helper.instance_variable_set(:@project, project) + allow(helper).to receive(:current_user).and_return(user) + end + + context 'when the user is allowed to see the code' do + it 'returns the project view' do + allow(helper).to receive(:can?).with(user, :download_code, project).and_return(true) + + expect(helper.default_project_view).to eq('readme') + end + end + + context 'with wikis enabled and the right policy for the user' do + before do + project.project_feature.update_attribute(:issues_access_level, 0) + allow(helper).to receive(:can?).with(user, :download_code, project).and_return(false) + end + + it 'returns wiki if the user has the right policy' do + allow(helper).to receive(:can?).with(user, :read_wiki, project).and_return(true) + + expect(helper.default_project_view).to eq('wiki') + end + + it 'returns customize_workflow if the user does not have the right policy' do + allow(helper).to receive(:can?).with(user, :read_wiki, project).and_return(false) + + expect(helper.default_project_view).to eq('customize_workflow') + end + end + + context 'with issues as a feature available' do + it 'return issues' do + allow(helper).to receive(:can?).with(user, :download_code, project).and_return(false) + allow(helper).to receive(:can?).with(user, :read_wiki, project).and_return(false) + + expect(helper.default_project_view).to eq('projects/issues/issues') + end + end + + context 'with no activity, no wikies and no issues' do + it 'returns customize_workflow as default' do + project.project_feature.update_attribute(:issues_access_level, 0) + allow(helper).to receive(:can?).with(user, :download_code, project).and_return(false) + allow(helper).to receive(:can?).with(user, :read_wiki, project).and_return(false) + + expect(helper.default_project_view).to eq('customize_workflow') + end + end + end + end + + def stub_user(messages = {}) + if messages.empty? + allow(helper).to receive(:current_user).and_return(nil) + else + allow(helper).to receive(:current_user) + .and_return(double('user', messages)) + end end end diff --git a/spec/javascripts/notes/components/issue_note_spec.js b/spec/javascripts/notes/components/issue_note_spec.js index 7ef85d5b4f0..514b135679a 100644 --- a/spec/javascripts/notes/components/issue_note_spec.js +++ b/spec/javascripts/notes/components/issue_note_spec.js @@ -41,4 +41,19 @@ describe('issue_note', () => { it('should render issue body', () => { expect(vm.$el.querySelector('.note-text').innerHTML).toEqual(note.note_html); }); + + it('prevents note preview xss', (done) => { + const imgSrc = 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7'; + const noteBody = `<img src="${imgSrc}" onload="alert(1)" />`; + const alertSpy = spyOn(window, 'alert'); + vm.updateNote = () => new Promise($.noop); + + vm.formUpdateHandler(noteBody, null, $.noop); + + setTimeout(() => { + expect(alertSpy).not.toHaveBeenCalled(); + expect(vm.note.note_html).toEqual(_.escape(noteBody)); + done(); + }, 0); + }); }); diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3b5ee70ae7c..32b1ad19903 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -889,11 +889,11 @@ describe User do describe 'email matching' do it 'returns users with a matching Email' do - expect(described_class.search(user.email)).to eq([user, user2]) + expect(described_class.search(user.email)).to eq([user]) end - it 'returns users with a partially matching Email' do - expect(described_class.search(user.email[0..2])).to eq([user, user2]) + it 'does not return users with a partially matching Email' do + expect(described_class.search(user.email[0..2])).not_to include(user, user2) end it 'returns users with a matching Email regardless of the casing' do @@ -949,8 +949,8 @@ describe User do expect(search_with_secondary_emails(user.email)).to eq([user]) end - it 'returns users with a partially matching email' do - expect(search_with_secondary_emails(user.email[0..2])).to eq([user]) + it 'does not return users with a partially matching email' do + expect(search_with_secondary_emails(user.email[0..2])).not_to include([user]) end it 'returns users with a matching email regardless of the casing' do @@ -973,29 +973,8 @@ describe User do expect(search_with_secondary_emails(email.email)).to eq([email.user]) end - it 'returns users with a matching part of secondary email' do - expect(search_with_secondary_emails(email.email[1..4])).to eq([email.user]) - end - - it 'return users with a matching part of secondary email regardless of case' do - expect(search_with_secondary_emails(email.email[1..4].upcase)).to eq([email.user]) - expect(search_with_secondary_emails(email.email[1..4].downcase)).to eq([email.user]) - expect(search_with_secondary_emails(email.email[1..4].capitalize)).to eq([email.user]) - end - - it 'returns multiple users with matching secondary emails' do - email1 = create(:email, email: '1_testemail@example.com') - email2 = create(:email, email: '2_testemail@example.com') - email3 = create(:email, email: 'other@email.com') - email3.user.update_attributes!(email: 'another@mail.com') - - expect( - search_with_secondary_emails('testemail@example.com').map(&:id) - ).to include(email1.user.id, email2.user.id) - - expect( - search_with_secondary_emails('testemail@example.com').map(&:id) - ).not_to include(email3.user.id) + it 'does not return users with a matching part of secondary email' do + expect(search_with_secondary_emails(email.email[1..4])).not_to include([email.user]) end end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 780dbce6488..0f6886b9132 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -173,6 +173,28 @@ describe API::Groups do end describe "GET /groups/:id" do + # Given a group, create one project for each visibility level + # + # group - Group to add projects to + # share_with - If provided, each project will be shared with this Group + # + # Returns a Hash of visibility_level => Project pairs + def add_projects_to_group(group, share_with: nil) + projects = { + public: create(:project, :public, namespace: group), + internal: create(:project, :internal, namespace: group), + private: create(:project, :private, namespace: group) + } + + if share_with + create(:project_group_link, project: projects[:public], group: share_with) + create(:project_group_link, project: projects[:internal], group: share_with) + create(:project_group_link, project: projects[:private], group: share_with) + end + + projects + end + context 'when unauthenticated' do it 'returns 404 for a private group' do get api("/groups/#{group2.id}") @@ -183,6 +205,26 @@ describe API::Groups do get api("/groups/#{group1.id}") expect(response).to have_gitlab_http_status(200) end + + it 'returns only public projects in the group' do + public_group = create(:group, :public) + projects = add_projects_to_group(public_group) + + get api("/groups/#{public_group.id}") + + expect(json_response['projects'].map { |p| p['id'].to_i }) + .to contain_exactly(projects[:public].id) + end + + it 'returns only public projects shared with the group' do + public_group = create(:group, :public) + projects = add_projects_to_group(public_group, share_with: group1) + + get api("/groups/#{group1.id}") + + expect(json_response['shared_projects'].map { |p| p['id'].to_i }) + .to contain_exactly(projects[:public].id) + end end context "when authenticated as user" do @@ -222,6 +264,26 @@ describe API::Groups do expect(response).to have_gitlab_http_status(404) end + + it 'returns only public and internal projects in the group' do + public_group = create(:group, :public) + projects = add_projects_to_group(public_group) + + get api("/groups/#{public_group.id}", user2) + + expect(json_response['projects'].map { |p| p['id'].to_i }) + .to contain_exactly(projects[:public].id, projects[:internal].id) + end + + it 'returns only public and internal projects shared with the group' do + public_group = create(:group, :public) + projects = add_projects_to_group(public_group, share_with: group1) + + get api("/groups/#{group1.id}", user2) + + expect(json_response['shared_projects'].map { |p| p['id'].to_i }) + .to contain_exactly(projects[:public].id, projects[:internal].id) + end end context "when authenticated as admin" do diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 99525cd0a6a..3f5070a1fd2 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -860,6 +860,20 @@ describe API::Issues, :mailer do end end + context 'user does not have permissions to create issue' do + let(:not_member) { create(:user) } + + before do + project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE) + end + + it 'renders 403' do + post api("/projects/#{project.id}/issues", not_member), title: 'new issue' + + expect(response).to have_gitlab_http_status(403) + end + end + it 'creates a new project issue' do post api("/projects/#{project.id}/issues", user), title: 'new issue', labels: 'label, label2', weight: 3, |