From d332c8c78a77ee400e01f91fd2c573f12caef21d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 21 Nov 2017 17:58:42 +0000 Subject: [PATCH 1/7] Merge branch '36679-non-authorized-user-may-see-wikis-or-pipeline-page' into 'security-10-2' Fixes project visibility guidelines See merge request gitlab/gitlabhq!2226 (cherry picked from commit 877c42c0aaf3298d6001614c9706bc366ae4014c) e4fd1c26 Ensure project wiki visibility guidelines are met --- app/controllers/projects_controller.rb | 2 +- app/helpers/preferences_helper.rb | 2 +- spec/factories/users.rb | 4 ++ spec/helpers/preferences_helper_spec.rb | 74 ++++++++++++++++++++++--- 4 files changed, 71 insertions(+), 11 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 3882fa4791d..8e9d6766d80 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -272,7 +272,7 @@ class ProjectsController < Projects::ApplicationController 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/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/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 From 8c0aa7d4a791cd05eddd9163fdc8270b933ffc6b Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 24 Nov 2017 09:42:12 +0000 Subject: [PATCH 2/7] Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2' (10.2) Avoid partial partial email adresses for matching See merge request gitlab/gitlabhq!2232 (cherry picked from commit 081aa1e91a777c9acb31be4a1e76b3dd7032fa9a) There are unresolved conflicts in app/models/user.rb. fa85a3fd Don't allow searching for partial user emails --- app/models/user.rb | 27 ++++++++++++++ .../unreleased/bvl-email-disclosure.yml | 5 +++ .../features/groups/members/manage_members.rb | 21 +++++++++++ spec/models/user_spec.rb | 35 ++++--------------- 4 files changed, 60 insertions(+), 28 deletions(-) create mode 100644 changelogs/unreleased/bvl-email-disclosure.yml diff --git a/app/models/user.rb b/app/models/user.rb index af1c36d9c93..7dc18c351e6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -315,6 +315,13 @@ class User < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search(query) +<<<<<<< HEAD +======= + table = arel_table + query = query.downcase + pattern = User.to_pattern(query) + +>>>>>>> f45fc58d84... Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2' order = <<~SQL CASE WHEN users.name = %{query} THEN 0 @@ -324,8 +331,16 @@ class User < ActiveRecord::Base END SQL +<<<<<<< HEAD fuzzy_search(query, [:name, :email, :username]) .reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) +======= + where( + table[:name].matches(pattern) + .or(table[:email].eq(query)) + .or(table[:username].matches(pattern)) + ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) +>>>>>>> f45fc58d84... Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2' end # searches user by given pattern @@ -334,6 +349,7 @@ class User < ActiveRecord::Base def search_with_secondary_emails(query) email_table = Email.arel_table +<<<<<<< HEAD matched_by_emails_user_ids = email_table .project(email_table[:user_id]) .where(Email.fuzzy_arel_match(:email, query)) @@ -343,6 +359,17 @@ class User < ActiveRecord::Base .or(fuzzy_arel_match(:email, query)) .or(fuzzy_arel_match(:username, query)) .or(arel_table[:id].in(matched_by_emails_user_ids)) +======= + 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].eq(query)) + .or(table[:username].matches(pattern)) + .or(table[:id].in(matched_by_emails_user_ids)) +>>>>>>> f45fc58d84... Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2' ) end 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/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/models/user_spec.rb b/spec/models/user_spec.rb index cdabd35b6ba..4687d9dfa00 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -913,11 +913,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 @@ -973,8 +973,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 @@ -997,29 +997,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 From 8f29d2640ffb29c7ca8c0ab1136aa1959582db3a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 28 Nov 2017 08:28:35 +0000 Subject: [PATCH 3/7] Merge branch 'rs-security-group-api' into 'security-10-2' [10.2] Ensure we expose group projects using GroupProjectsFinder See merge request gitlab/gitlabhq!2234 (cherry picked from commit 072f8f2fd6ec794645375a16ca4ddc1cbeb76d7a) a2240338 Ensure we expose group projects using GroupProjectsFinder --- .../unreleased/rs-security-group-api.yml | 5 ++ lib/api/entities.rb | 17 ++++- spec/requests/api/groups_spec.rb | 62 +++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/rs-security-group-api.yml 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 d96e7f2770f..928706dfda7 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -248,8 +248,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/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 554723d6b1e..6330c140246 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 From c59ae5470546d1169ee3ab89486140e815400f31 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 22 Nov 2017 17:24:11 +0000 Subject: [PATCH 4/7] Merge branch 'issue_30663' into 'security-10-2' Prevent creating issues through API without having permissions See merge request gitlab/gitlabhq!2225 (cherry picked from commit c298bbaa88883343dc9cbbb6abec0808fb3b546c) 915b97c5 Prevent creating issues through API without having permissions --- changelogs/unreleased/issue_30663.yml | 5 +++++ lib/api/issues.rb | 2 ++ spec/requests/api/issues_spec.rb | 14 ++++++++++++++ 3 files changed, 21 insertions(+) create mode 100644 changelogs/unreleased/issue_30663.yml 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/lib/api/issues.rb b/lib/api/issues.rb index e60e00d7956..5f943ba27d1 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/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, From f4fbe61a9e073d8e49b0e8104961b2556ce3ac05 Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Wed, 6 Dec 2017 20:10:32 +0000 Subject: [PATCH 5/7] Merge branch 'note-preview' into 'security-10-2' prevent potential XSS when editing comment See merge request gitlab/gitlabhq!2238 (cherry picked from commit 80ed6d25a46c0f70ec8baea78b5777118d63876c) 7480e462 prevent potential XSS when editing comment --- .../javascripts/notes/components/issue_note.vue | 3 ++- .../notes/components/issue_note_spec.js | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/notes/components/issue_note.vue b/app/assets/javascripts/notes/components/issue_note.vue index 8c81c5d6df3..3ceb961f58e 100644 --- a/app/assets/javascripts/notes/components/issue_note.vue +++ b/app/assets/javascripts/notes/components/issue_note.vue @@ -1,5 +1,6 @@