summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorWinnie Hellmann <winnie@gitlab.com>2017-12-11 12:07:57 +0000
committerWinnie Hellmann <winnie@gitlab.com>2017-12-11 12:07:57 +0000
commit1eff1bd385a28ccde7d0dc3a991c499ada1a63bd (patch)
treeb57fbf22b38f0abe59219091842428b4a5358e9c
parent689bc9ea6db102006b548e6176125157955c7f2b (diff)
parentf71e48a0d09597e19aa629e4c7d42035ca08d852 (diff)
downloadgitlab-ce-1eff1bd385a28ccde7d0dc3a991c499ada1a63bd.tar.gz
Merge branch 'mk-pick-10-2-4-security-fixes' into 'master'
Pick 10.2.4 security fixes into master See merge request gitlab-org/gitlab-ce!15821
-rw-r--r--CHANGELOG.md11
-rw-r--r--app/assets/javascripts/notes/components/issue_note.vue3
-rw-r--r--app/controllers/projects_controller.rb2
-rw-r--r--app/helpers/preferences_helper.rb2
-rw-r--r--app/models/user.rb15
-rw-r--r--lib/api/entities.rb17
-rw-r--r--lib/api/issues.rb2
-rw-r--r--spec/factories/users.rb4
-rw-r--r--spec/features/groups/members/manage_members.rb21
-rw-r--r--spec/helpers/preferences_helper_spec.rb74
-rw-r--r--spec/javascripts/notes/components/issue_note_spec.js15
-rw-r--r--spec/models/user_spec.rb35
-rw-r--r--spec/requests/api/groups_spec.rb62
-rw-r--r--spec/requests/api/issues_spec.rb14
14 files changed, 231 insertions, 46 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6088a1b3515..78f8e457c70 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,17 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
+## 10.2.4 (2017-12-08)
+
+### Security (4 changes)
+
+- Fix e-mail address disclosure through member search fields
+- Prevent creating issues through API when user does not have permissions
+- Prevent an information disclosure in the Groups API
+- Fix user without access to private Wiki being able to see it on the project page
+- Fix Cross-Site Scripting (XSS) vulnerability while editing a comment
+
+
## 10.2.3 (2017-11-30)
### Fixed (7 changes)
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 @@
<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 noteHeader from './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 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/app/models/user.rb b/app/models/user.rb
index 093ff808626..92b461ce3ed 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -315,6 +315,8 @@ class User < ActiveRecord::Base
#
# Returns an ActiveRecord::Relation.
def search(query)
+ query = query.downcase
+
order = <<~SQL
CASE
WHEN users.name = %{query} THEN 0
@@ -324,8 +326,11 @@ class User < ActiveRecord::Base
END
SQL
- fuzzy_search(query, [:name, :email, :username])
- .reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name)
+ where(
+ fuzzy_arel_match(:name, query)
+ .or(fuzzy_arel_match(:username, query))
+ .or(arel_table[:email].eq(query))
+ ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name)
end
# searches user by given pattern
@@ -333,15 +338,17 @@ class User < ActiveRecord::Base
# This method uses ILIKE on PostgreSQL and LIKE on MySQL.
def search_with_secondary_emails(query)
+ query = query.downcase
+
email_table = Email.arel_table
matched_by_emails_user_ids = email_table
.project(email_table[:user_id])
- .where(Email.fuzzy_arel_match(:email, query))
+ .where(email_table[:email].eq(query))
where(
fuzzy_arel_match(:name, query)
- .or(fuzzy_arel_match(:email, query))
.or(fuzzy_arel_match(:username, query))
+ .or(arel_table[:email].eq(query))
.or(arel_table[:id].in(matched_by_emails_user_ids))
)
end
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/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/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 73fd188dbe5..bd888b2cbae 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 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
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
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,