Commit 222b0b6e authored by Robert Speicher's avatar Robert Speicher

Merge branch 'jarv/dev-to-gitlab-2019-04-02' into 'master'

Jarv/dev to gitlab 2019 04 02

See merge request gitlab-org/gitlab-ee!10532
parents 4e6a4d3d 8d4eae51
Please view this file on the master branch, on stable branches it's out of date.
## 11.9.3 (2019-03-27)
### Security (1 change)
- Check label_ids parent when updating issue board.
## 11.9.2 (2019-03-26)
### Security (2 changes)
- Geo - Improve security while redirecting user back to the secondary after a logout & re-login via the primary.
- Check label_ids parent when updating issue board.
## 11.9.1 (2019-03-25)
### Fixed (1 change)
......@@ -222,6 +237,21 @@ Please view this file on the master branch, on stable branches it's out of date.
- Creates an EE component for the pipeline graph.
## 11.7.10 (2019-03-28)
### Security (1 change)
- Check label_ids parent when updating issue board.
## 11.7.8 (2019-03-26)
### Security (2 changes)
- Geo - Improve security while redirecting user back to the secondary after a logout & re-login via the primary.
- Check label_ids parent when updating issue board.
## 11.7.7 (2019-03-19)
- No changes.
......
......@@ -2,6 +2,14 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
## 11.9.3 (2019-03-27)
- No changes.
## 11.9.2 (2019-03-26)
- No changes.
## 11.9.1 (2019-03-25)
### Fixed (7 changes)
......@@ -548,6 +556,23 @@ entry.
- Creates mixin to reduce code duplication between CE and EE in graph component.
## 11.7.10 (2019-03-28)
### Security (7 changes)
- Disallow guest users from accessing Releases.
- Fix PDF.js vulnerability.
- Hide "related branches" when user does not have permission.
- Fix XSS in resolve conflicts form.
- Added rake task for removing EXIF data from existing uploads.
- Disallow updating namespace when updating a project.
- Use UntrustedRegexp for matching refs policy.
## 11.7.8 (2019-03-26)
- No changes.
## 11.7.7 (2019-03-19)
### Security (2 changes)
......
......@@ -47,7 +47,7 @@ class ProjectsController < Projects::ApplicationController
end
def create
@project = ::Projects::CreateService.new(current_user, project_params).execute
@project = ::Projects::CreateService.new(current_user, project_params(attributes: project_params_create_attributes)).execute
if @project.saved?
cookies[:issue_board_welcome_hidden] = { path: project_path(@project), value: nil, expires: Time.at(0) }
......@@ -328,9 +328,9 @@ class ProjectsController < Projects::ApplicationController
end
# rubocop: enable CodeReuse/ActiveRecord
def project_params
def project_params(attributes: [])
params.require(:project)
.permit(project_params_attributes)
.permit(project_params_attributes + attributes)
end
def project_params_attributes
......@@ -349,11 +349,10 @@ class ProjectsController < Projects::ApplicationController
:last_activity_at,
:lfs_enabled,
:name,
:namespace_id,
:only_allow_merge_if_all_discussions_are_resolved,
:only_allow_merge_if_pipeline_succeeds,
:printing_merge_request_link_enabled,
:path,
:printing_merge_request_link_enabled,
:public_builds,
:request_access_enabled,
:runners_token,
......@@ -375,6 +374,10 @@ class ProjectsController < Projects::ApplicationController
]
end
def project_params_create_attributes
[:namespace_id]
end
def custom_import_params
{}
end
......
......@@ -133,6 +133,10 @@ class Label < ApplicationRecord
1
end
def self.by_ids(ids)
where(id: ids)
end
def open_issues_count(user = nil)
issues_count(user, state: 'opened')
end
......
......@@ -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
def filter_labels_in_param(key)
......@@ -99,6 +103,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)
......@@ -116,10 +124,6 @@ class IssuableBaseService < BaseService
new_label_ids.uniq
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
......
# 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
---
title: Disallow updating namespace when updating a project
merge_request:
author:
type: security
......@@ -14,7 +14,7 @@ module EE
return super if signed_in?
if ::Gitlab::Geo.secondary_with_primary?
redirect_to oauth_geo_auth_url(state: geo_login_state.encode)
redirect_to oauth_geo_auth_url(host: ::Gitlab::Geo.current_node.url, state: geo_login_state.encode)
else
super
end
......@@ -33,7 +33,7 @@ module EE
end
def geo_login_state
::Gitlab::Geo::Oauth::LoginState.new(return_to: geo_return_to_after_login)
::Gitlab::Geo::Oauth::LoginState.new(return_to: sanitize_redirect(geo_return_to_after_login))
end
def geo_logout_state
......
......@@ -35,20 +35,15 @@ module EE
# rubocop: enable CodeReuse/ActiveRecord
def set_labels
labels = params.delete(:labels)
return unless labels
params[:label_ids] = labels.split(",").map do |label_name|
label = Labels::FindOrCreateService.new(
current_user,
parent,
title: label_name.strip,
include_ancestor_groups: true
).execute
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
label.try(:id)
end.compact
def labels_service
@labels_service ||= ::Labels::AvailableLabelsService.new(current_user, parent, params)
end
end
end
......
---
title: Geo - Improve security while redirecting user back to the secondary after a
logout & re-login via the primary
merge_request:
author:
type: security
---
title: Check label_ids parent when updating issue board
merge_request:
author:
type: security
......@@ -7,40 +7,57 @@ module Gitlab
attr_reader :return_to
def self.from_state(state)
salt, hmac, return_to = state.to_s.split(':', 3)
self.new(salt: salt, hmac: hmac, return_to: return_to)
salt, token, return_to = state.to_s.split(':', 3)
self.new(salt: salt, token: token, return_to: return_to)
end
def initialize(return_to:, salt: nil, hmac: nil)
@return_to = return_to
def initialize(return_to:, salt: nil, token: nil)
@return_to = Gitlab::ReturnToLocation.new(return_to).full_path
@salt = salt
@hmac = hmac
@token = token
end
def valid?
return false unless salt.present? && hmac.present?
hmac == generate_hmac
def encode
"#{salt}:#{hmac_token}:#{return_to}"
end
def encode
"#{salt}:#{generate_hmac}:#{return_to}"
def valid?
return false unless salt.present? && token.present?
decoded_token = JSONWebToken::HMACToken.decode(token, key).first
secure_compare(decoded_token.dig('data', 'return_to'))
rescue JWT::DecodeError
false
end
private
attr_reader :hmac
attr_reader :token
def generate_hmac
digest = OpenSSL::Digest::SHA256.new
key = Gitlab::Application.secrets.secret_key_base + salt
def expiration_time
1.minute
end
def hmac_token
hmac_token = JSONWebToken::HMACToken.new(key)
hmac_token.expire_time = Time.now + expiration_time
hmac_token[:data] = { return_to: return_to.to_s }
hmac_token.encoded
end
OpenSSL::HMAC.hexdigest(digest, key, return_to.to_s)
def key
ActiveSupport::KeyGenerator
.new(Gitlab::Application.secrets.secret_key_base)
.generate_key(salt)
end
def salt
@salt ||= SecureRandom.hex(8)
end
def secure_compare(value)
ActiveSupport::SecurityUtils.secure_compare(return_to.to_s, value)
end
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe SessionsController do
include DeviseHelpers
include EE::GeoHelpers
describe '#new' do
before do
set_devise_mapping(context: @request)
end
context 'on a Geo secondary node' do
set(:primary_node) { create(:geo_node, :primary) }
set(:secondary_node) { create(:geo_node) }
before do
stub_current_geo_node(secondary_node)
end
shared_examples 'a valid oauth authentication redirect' do
it 'redirects to the correct oauth_geo_auth_url' do
get(:new)
redirect_uri = URI.parse(response.location)
redirect_params = CGI.parse(redirect_uri.query)
expect(response).to have_gitlab_http_status(302)
expect(response).to redirect_to %r(\A#{primary_node.url}oauth/geo/auth)
expect(redirect_params['state'].first).to end_with(':')
end
end
context 'with a tampered HOST header' do
before do
request.headers['HOST'] = 'http://this.is.not.my.host'
end
it_behaves_like 'a valid oauth authentication redirect'
end
context 'with a tampered X-Forwarded-Host header' do
before do
request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host'
end
it_behaves_like 'a valid oauth authentication redirect'
end
context 'without a tampered header' do
it_behaves_like 'a valid oauth authentication redirect'
end
end
end
end
......@@ -27,12 +27,34 @@ describe Oauth::GeoAuthController do
expect(response).to redirect_to(root_url)
end
it "redirects to primary node's oauth endpoint" do
oauth_endpoint = Gitlab::Geo::Oauth::Session.new.authorize_url(redirect_uri: oauth_geo_callback_url, state: login_state)
shared_examples "a valid redirect to to primary node's oauth endpoint" do
it "redirects to primary node's oauth endpoint" do
oauth_endpoint = Gitlab::Geo::Oauth::Session.new.authorize_url(redirect_uri: oauth_geo_callback_url, state: login_state)
get :auth, params: { state: login_state }
get :auth, params: { state: login_state }
expect(response).to redirect_to(oauth_endpoint)
end
end
context 'without a tampered header' do
it_behaves_like "a valid redirect to to primary node's oauth endpoint"
end
context 'with a tampered HOST header' do
before do
request.headers['HOST'] = 'http://this.is.not.my.host'
end
it_behaves_like "a valid redirect to to primary node's oauth endpoint"
end
context 'with a tampered X-Forwarded-Host header' do
before do
request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host'
end
expect(response).to redirect_to(oauth_endpoint)
it_behaves_like "a valid redirect to to primary node's oauth endpoint"
end
end
......@@ -55,16 +77,40 @@ describe Oauth::GeoAuthController do
expect(response).to redirect_to(new_user_session_path)
end
it 'redirects to redirect_url if state is valid' do
get :callback, params: { state: login_state }
context 'with a valid state' do
shared_examples 'a valid redirect to redirect_url' do
it "redirects to primary node's oauth endpoint" do
get :callback, params: { state: login_state }
expect(response).to redirect_to(secondary_node.url)
end
expect(response).to redirect_to('/')
end
end
it 'does not display a flash message if state is valid' do
get :callback, params: { state: login_state }
context 'without a tampered header' do
it_behaves_like 'a valid redirect to redirect_url'
end
context 'with a tampered HOST header' do
before do
request.headers['HOST'] = 'http://this.is.not.my.host'
end
it_behaves_like 'a valid redirect to redirect_url'
end
context 'with a tampered X-Forwarded-Host header' do
before do
request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host'
end
it_behaves_like 'a valid redirect to redirect_url'
end
it 'does not display a flash message' do
get :callback, params: { state: login_state }
expect(controller).to set_flash[:alert].to(nil)
expect(controller).to set_flash[:alert].to(nil)
end
end
end
......
......@@ -141,7 +141,7 @@ describe Projects::BoardsController do
let(:board) { create(:board, project: project, name: 'Backend') }
let(:user) { create(:user) }
let(:milestone) { create(:milestone, project: project) }
let(:label) { create(:label) }
let(:label) { create(:label, project: project) }
let(:update_params) do
{ name: 'Frontend',
......
......@@ -3,9 +3,14 @@
require 'spec_helper'
describe Gitlab::Geo::Oauth::LoginState do
let(:salt) { '100d8cbd1750a2bb' }
let(:hmac) { '62fdcface89baab582f33de6672f10499974c28b5cc269795c4830b8b3ab06be' }
let(:oauth_return_to) { 'http://fake-secondary.com:3000/project/test' }
let(:salt) { 'b9653b6aa2ff6b54' }
let(:token) { 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkYXRhIjp7InJldHVybl90byI6Ii9wcm9qZWN0L3Rlc3Q_Zm9vPWJhciN6b28ifSwianRpIjoiODdjZDQ2M2MtOTgyNC00ZjliLWI5NDMtOGFkMjJmY2E2MmZhIiwiaWF0IjoxNTQ5ODI1MjAwLCJuYmYiOjE1NDk4MjUxOTUsImV4cCI6MTU0OTgyNTI2MH0.qZE6kuoeW6BK1URuIl8l8MiCfGjtTTXixVdMCE80gVA' }
let(:return_to) { 'http://fake-secondary.com:3000/project/test?foo=bar#zoo' }
let(:timestamp) { Time.utc(2019, 2, 10, 19, 0, 0) }
around do |example|
Timecop.freeze(timestamp) { example.run }
end
before do
allow(Gitlab::Application.secrets).to receive(:secret_key_base)
......@@ -22,7 +27,7 @@ describe Gitlab::Geo::Oauth::LoginState do
end
it 'returns a valid instance when state is valid' do
expect(described_class.from_state("#{salt}:#{hmac}:#{oauth_return_to}")).to be_valid
expect(described_class.from_state("#{salt}:#{token}:#{return_to}")).to be_valid
end
end
......@@ -39,32 +44,42 @@ describe Gitlab::Geo::Oauth::LoginState do
expect(subject.valid?).to eq false
end
it 'returns false when hmac is nil' do
subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: nil)
it 'returns false when token is nil' do
subject = described_class.new(return_to: return_to, salt: salt, token: nil)
expect(subject.valid?).to eq false
end
it 'returns false when hmac is empty' do
subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: '')
it 'returns false when token is empty' do
subject = described_class.new(return_to: return_to, salt: salt, token: '')
expect(subject.valid?).to eq false
end
it 'returns false when salt not match' do
subject = described_class.new(return_to: oauth_return_to, salt: 'salt', hmac: hmac)
subject = described_class.new(return_to: return_to, salt: 'invalid-salt', token: token)
expect(subject.valid?).to eq(false)
end
it 'returns false when hmac does not match' do
subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: 'hmac')
it 'returns false when token does not match' do
subject = described_class.new(return_to: return_to, salt: salt, token: 'invalid-token')
expect(subject.valid?).to eq(false)
end
it 'returns true when hmac matches' do
subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: hmac)
it "returns false when token's expired" do
subject = described_class.new(return_to: return_to, salt: salt, token: token)
# Needs to be at least 120 seconds, because the default expiry is
# 60 seconds with an additional 60 second leeway.
Timecop.freeze(timestamp + 125) do
expect(subject.valid?).to eq(false)
end
end
it 'returns true when token matches' do
subject = described_class.new(return_to: return_to, salt: salt, token: token)
expect(subject.valid?).to eq(true)
end
......@@ -77,14 +92,34 @@ describe Gitlab::Geo::Oauth::LoginState do
expect { subject.encode }.not_to raise_error
end
it 'returns a string with salt, hmac, and return_to colon separated' do
subject = described_class.new(return_to: oauth_return_to)
it 'returns a string with salt, token, and return_to colon separated' do
subject = described_class.new(return_to: return_to)
salt, hmac, return_to = subject.encode.split(':', 3)
salt, token, return_to = subject.encode.split(':', 3)
expect(salt).not_to be_blank
expect(hmac).not_to be_blank
expect(return_to).to eq oauth_return_to
expect(token).not_to be_blank
expect(return_to).to eq return_to
end
end
describe '#return_to' do
it 'returns nil when return_to is nil' do
subject = described_class.new(return_to: nil)
expect(subject.return_to).to be_nil
end
it 'returns an empty string when return_to is empty' do
subject = described_class.new(return_to: '')
expect(subject.return_to).to eq('')
end
it 'returns the full path of the return_to URL' do
subject = described_class.new(return_to: return_to)
expect(subject.return_to).to eq('/project/test?foo=bar#zoo')
end
end
end
......@@ -17,7 +17,7 @@ describe Boards::UpdateService, services: true do
end
describe '#execute' do
let(:project) { create(:project) }
let(:project) { create(:project, group: group) }
let(:group) { create(:group) }
let!(:board) { create(:board, group: group, name: 'Backend') }
......@@ -44,10 +44,11 @@ describe Boards::UpdateService, services: true do
it 'updates the configuration params when scoped issue board is enabled' do
stub_licensed_features(scoped_issue_board: true)
assignee = create(:user)
milestone = create(:milestone, project: project)
label = create(:label, project: project)
milestone = create(:milestone, group: group)
label = create(:group_label, group: board.group)
user = create(:user)
service = described_class.new(project, double,
service = described_class.new(group, user,
milestone_id: milestone.id,
assignee_id: assignee.id,
label_ids: [label.id])
......@@ -213,6 +214,18 @@ describe Boards::UpdateService, services: true do
expect_label_assigned(user, board, %w{group_label project_label new_label}, %w{group_label project_label})
end
end
context 'when label_ids param is provided' do
it 'updates using only labels accessible by the project board' do
other_project_label = create(:label, title: 'other_project_label')
other_group_label = create(:group_label, title: 'other_group_label')
label_ids = [group_label.id, label.id, other_project_label.id, other_group_label.id]
described_class.new(board.parent, user, label_ids: label_ids).execute(board)
expect(board.reload.labels).to contain_exactly(group_label, label)
end
end
end
end
end
......
......@@ -392,6 +392,23 @@ describe ProjectsController do
end
end
it 'does not update namespace' do
controller.instance_variable_set(:@project, project)
params = {
namespace_id: 'test'
}
expect do
put :update,
params: {
namespace_id: project.namespace,
id: project.id,
project: params
}
end.not_to change { project.namespace.reload }
end
def update_project(**parameters)
put :update,
params: {
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment