Commit aac78104 authored by Georges Racinet's avatar Georges Racinet 🏕

Merged GitLab v13.9.2 into release branch

and updated HEPTAPOD_CHANGELOG directly

--HG--
branch : heptapod-0-20
......@@ -2,6 +2,18 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
## 13.9.2 (2021-03-04)
### Security (6 changes)
- Bump thrift gem to 0.14.0.
- Allow only owners to manage group variables.
- Do not store marshalled sessions ids in Redis.
- Fix XSS in wiki author email and name.
- Workhorse: prevent escaped router path traversal.
- Fix XSS vulnerability for swagger file viewer.
## 13.9.1 (2021-02-23)
### Fixed (6 changes, 1 of them is from the community)
......
13.9.1
\ No newline at end of file
13.9.2
\ No newline at end of file
......@@ -310,6 +310,9 @@ gem 'premailer-rails', '~> 1.10.3'
# LabKit: Tracing and Correlation
gem 'gitlab-labkit', '0.14.0'
# Thrift is a dependency of gitlab-labkit, we want a version higher than 0.14.0
# because of https://gitlab.com/gitlab-org/gitlab/-/issues/321900
gem 'thrift', '>= 0.14.0'
# I18n
gem 'ruby_parser', '~> 3.15', require: false
......
......@@ -1186,7 +1186,7 @@ GEM
rack (>= 1, < 3)
thor (1.1.0)
thread_safe (0.3.6)
thrift (0.13.0)
thrift (0.14.0)
tilt (2.0.10)
timecop (0.9.1)
timeliness (0.3.10)
......@@ -1535,6 +1535,7 @@ DEPENDENCIES
terser (= 1.0.2)
test-prof (~> 0.12.0)
thin (~> 1.8.0)
thrift (>= 0.14.0)
timecop (~> 0.9.1)
toml-rb (~> 1.0.0)
truncato (~> 0.7.11)
......
......@@ -15,6 +15,11 @@ the exact revision of the main repository in the user help page.
Please read migration instructions in the
[installation notice](INSTALL_HEPTAPOD.md)
### Heptapod 0.20.1 (2021-03-08)
- heptapod#433: upstream GitLab update to 13.9.2 (from 13.9.1)
### Heptapod 0.20.0 (2021-03-01)
#### Summary of changes since Heptapod 0.19.3
......
13.9.1
\ No newline at end of file
13.9.2
\ No newline at end of file
......@@ -2,7 +2,7 @@
module Groups
class VariablesController < Groups::ApplicationController
before_action :authorize_admin_build!
before_action :authorize_admin_group!
skip_cross_project_access_check :show, :update
......
......@@ -8,9 +8,8 @@ def index
end
def destroy
# params[:id] can be either an Rack::Session::SessionId#private_id
# or an encrypted Rack::Session::SessionId#public_id
ActiveSession.destroy_with_deprecated_encryption(current_user, params[:id])
# params[:id] can be an Rack::Session::SessionId#private_id
ActiveSession.destroy_session(current_user, params[:id])
current_user.forget_me!
respond_to do |format|
......
......@@ -24,11 +24,6 @@ def active_session_device_type_icon(active_session)
end
def revoke_session_path(active_session)
if active_session.session_private_id
profile_active_session_path(active_session.session_private_id)
else
# TODO: remove in 13.7
profile_active_session_path(active_session.public_id)
end
profile_active_session_path(active_session.session_private_id)
end
end
# frozen_string_literal: true
module WikiPageVersionHelper
def wiki_page_version_author_url(wiki_page_version)
user = wiki_page_version.author
user.nil? ? "mailto:#{wiki_page_version.author_email}" : Gitlab::UrlBuilder.build(user)
end
def wiki_page_version_author_avatar(wiki_page_version)
image_tag(avatar_icon_for_email(wiki_page_version.author_email, 24), class: "avatar s24 float-none gl-mr-0!")
end
def wiki_page_version_author_header(wiki_page_version)
avatar = wiki_page_version_author_avatar(wiki_page_version)
name = "<strong>".html_safe + wiki_page_version.author_name + "</strong>".html_safe
link_start = "<a href='".html_safe + wiki_page_version_author_url(wiki_page_version) + "'>".html_safe
html_escape(_("Last edited by %{link_start}%{avatar} %{name}%{link_end}")) % { avatar: avatar, name: name, link_start: link_start, link_end: '</a>'.html_safe }
end
end
......@@ -42,13 +42,6 @@ def human_device_type
device_type&.titleize
end
# This is not the same as Rack::Session::SessionId#public_id, but we
# need to preserve this for backwards compatibility.
# TODO: remove in 13.7
def public_id
Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
end
def self.set(user, request)
Gitlab::Redis::SharedState.with do |redis|
session_private_id = request.session.id.private_id
......@@ -63,8 +56,6 @@ def self.set(user, request)
device_type: client.device_type,
created_at: user.current_sign_in_at || timestamp,
updated_at: timestamp,
# TODO: remove in 13.7
session_id: request.session.id.public_id,
session_private_id: session_private_id,
is_impersonated: request.session[:impersonator_id].present?
)
......@@ -80,20 +71,10 @@ def self.set(user, request)
lookup_key_name(user.id),
session_private_id
)
# We remove the ActiveSession stored by using public_id to avoid
# duplicate entries
remove_deprecated_active_sessions_with_public_id(redis, user.id, request.session.id.public_id)
end
end
end
# TODO: remove in 13.7
private_class_method def self.remove_deprecated_active_sessions_with_public_id(redis, user_id, rack_session_public_id)
redis.srem(lookup_key_name(user_id), rack_session_public_id)
redis.del(key_name(user_id, rack_session_public_id))
end
def self.list(user)
Gitlab::Redis::SharedState.with do |redis|
cleaned_up_lookup_entries(redis, user).map do |raw_session|
......@@ -109,18 +90,6 @@ def self.cleanup(user)
end
end
# TODO: remove in 13.7
# After upgrade there might be a duplicate ActiveSessions:
# - one with the public_id stored in #session_id
# - another with private_id stored in #session_private_id
def self.destroy_with_rack_session_id(user, rack_session_id)
return unless rack_session_id
Gitlab::Redis::SharedState.with do |redis|
destroy_sessions(redis, user, [rack_session_id.public_id, rack_session_id.private_id])
end
end
def self.destroy_sessions(redis, user, session_ids)
key_names = session_ids.map { |session_id| key_name(user.id, session_id) }
......@@ -132,19 +101,11 @@ def self.destroy_sessions(redis, user, session_ids)
end
end
# TODO: remove in 13.7
# After upgrade, .destroy might be called with the session id encrypted
# by .public_id.
def self.destroy_with_deprecated_encryption(user, session_id)
def self.destroy_session(user, session_id)
return unless session_id
decrypted_session_id = decrypt_public_id(session_id)
rack_session_private_id = if decrypted_session_id
Rack::Session::SessionId.new(decrypted_session_id).private_id
end
Gitlab::Redis::SharedState.with do |redis|
destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact)
destroy_sessions(redis, user, [session_id].compact)
end
end
......@@ -275,11 +236,4 @@ def self.cleaned_up_lookup_entries(redis, user)
entries.compact
end
# TODO: remove in 13.7
private_class_method def self.decrypt_public_id(public_id)
Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id)
rescue
nil
end
end
......@@ -7,7 +7,7 @@
.nav-text.flex-fill
%span.wiki-last-edit-by
- if @page.last_version
= html_escape(_("Last edited by %{link_start}%{avatar} %{name}%{link_end}")) % { avatar: image_tag(avatar_icon_for_email(@page.last_version.author_email, 24), class: "avatar s24 float-none gl-mr-0!"), name: "<strong>#{@page.last_version.author_name}</strong>".html_safe, link_start: "<a href='#{@page.last_version.author_url}'>".html_safe, link_end: '</a>'.html_safe }
= wiki_page_version_author_header(@page.last_version)
= time_ago_with_tooltip(@page.last_version.authored_date)
.nav-controls.pb-md-3.pb-lg-0
......
......@@ -42,8 +42,7 @@
activity = Gitlab::Auth::Activity.new(opts)
tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth)
# TODO: switch to `auth.request.session.id.private_id` in 13.7
ActiveSession.destroy_with_rack_session_id(user, auth.request.session.id)
ActiveSession.destroy_session(user, auth.request.session.id.private_id) if auth.request.session.id
activity.user_session_destroyed!
##
......
......@@ -5,8 +5,7 @@ class GroupVariables < ::API::Base
include PaginationParams
before { authenticate! }
before { authorize! :admin_build, user_group }
before { authorize! :admin_group, user_group }
feature_category :continuous_integration
params do
......
......@@ -3,6 +3,8 @@
module Gitlab
module Git
class WikiPageVersion
include Gitlab::Utils::StrongMemoize
attr_reader :commit, :format
def initialize(commit, format)
......@@ -12,9 +14,10 @@ def initialize(commit, format)
delegate :message, :sha, :id, :author_name, :author_email, :authored_date, to: :commit
def author_url
user = ::User.find_by_any_email(author_email)
user.nil? ? "mailto:#{author_email}" : Gitlab::UrlBuilder.build(user)
def author
strong_memoize(:author) do
::User.find_by_any_email(author_email)
end
end
end
end
......
......@@ -5,26 +5,35 @@
RSpec.describe Groups::VariablesController do
include ExternalAuthorizationServiceHelpers
let(:group) { create(:group) }
let(:user) { create(:user) }
let_it_be(:group) { create(:group) }
let_it_be(:user) { create(:user) }
let_it_be(:variable) { create(:ci_group_variable, group: group) }
let(:access_level) { :owner }
before do
sign_in(user)
group.add_maintainer(user)
group.add_user(user, access_level)
end
describe 'GET #show' do
let!(:variable) { create(:ci_group_variable, group: group) }
subject do
get :show, params: { group_id: group }, format: :json
end
include_examples 'GET #show lists all variables'
context 'when the user is a maintainer' do
let(:access_level) { :maintainer }
it 'returns not found response' do
subject
expect(response).to have_gitlab_http_status(:not_found)
end
end
end
describe 'PATCH #update' do
let!(:variable) { create(:ci_group_variable, group: group) }
let(:owner) { group }
subject do
......@@ -37,6 +46,19 @@
end
include_examples 'PATCH #update updates variables'
context 'when the user is a maintainer' do
let(:access_level) { :maintainer }
let(:variables_attributes) do
[{ id: variable.id, key: 'new_key' }]
end
it 'returns not found response' do
subject
expect(response).to have_gitlab_http_status(:not_found)
end
end
end
context 'with external authorization enabled' do
......@@ -45,8 +67,6 @@
end
describe 'GET #show' do
let!(:variable) { create(:ci_group_variable, group: group) }
it 'is successful' do
get :show, params: { group_id: group }, format: :json
......@@ -55,9 +75,6 @@
end
describe 'PATCH #update' do
let!(:variable) { create(:ci_group_variable, group: group) }
let(:owner) { group }
it 'is successful' do
patch :update,
params: {
......
......@@ -12,7 +12,7 @@
it 'invalidates all remember user tokens' do
ActiveSession.set(user, request)
session_id = request.session.id.public_id
session_id = request.session.id.private_id
user.remember_me!
delete :destroy, params: { id: session_id }
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe WikiPageVersionHelper do
let_it_be(:project) { create(:project, :public, :repository) }
let_it_be(:user) { create(:user, username: 'foo') }
let(:commit_with_user) { create(:commit, project: project, author: user)}
let(:commit_without_user) { create(:commit, project: project, author_name: 'Foo', author_email: 'foo@example.com')}
let(:wiki_page_version) { Gitlab::Git::WikiPageVersion.new(commit, nil) }
describe '#wiki_page_version_author_url' do
subject { helper.wiki_page_version_author_url(wiki_page_version) }
context 'when user exists' do
let(:commit) { commit_with_user }
it 'returns the link to the user profile' do
expect(subject).to eq('http://localhost/foo')
end
end
context 'when user does not exist' do
let(:commit) { commit_without_user }
it 'returns the mailto link' do
expect(subject).to eq "mailto:#{commit_without_user.author_email}"
end
end
end
describe '#wiki_page_version_author_avatar' do
let(:commit) { commit_with_user }
subject { helper.wiki_page_version_author_avatar(wiki_page_version) }
it 'returns the user avatar', :aggregate_failures do
avatar = Nokogiri::HTML.parse(subject)
expect(avatar.css('img')[0].attr('class')).to eq('avatar s24 float-none gl-mr-0! lazy')
expect(avatar.css('img')[0].attr('data-src')).not_to be_empty
expect(avatar.css('img')[0].attr('src')).not_to be_empty
end
end
describe '#wiki_page_version_author_header', :aggregate_failures do
let(:commit_with_xss) { create(:commit, project: project, author_email: "#' style=animation-name:blinking-dot onanimationstart=alert(document.domain) other", author_name: "<i>foo</i>") }
let(:header) { Nokogiri::HTML.parse(subject) }
subject { helper.wiki_page_version_author_header(wiki_page_version) }
context 'when user exists' do
let(:commit) { commit_with_user }
it 'renders commit header with user info' do
expect(header.css('a')[0].attr('href')).to eq("http://localhost/foo")
expect(header.css('a')[0].children[2].to_s).to eq("<strong>#{user.name}</strong>")
end
end
context 'when user does not exist' do
let(:commit) { commit_without_user }
it 'renders commit header with info from commit' do
expect(header.css('a')[0].attr('href')).to eq("mailto:#{commit.author_email}")
expect(header.css('a')[0].children[2].to_s).to eq("<strong>#{wiki_page_version.author_name}</strong>")
end
end
context 'when user info has XSS' do
let(:commit) { commit_with_xss }
it 'sets the right href and escapes HTML chars' do
expect(header.css('a')[0].attr('href')).to eq("mailto:#{commit.author_email}")
expect(header.css('a')[0].children[2].to_s).to eq("<strong>&lt;i&gt;foo&lt;/i&gt;</strong>")
end
end
end
end
......@@ -4,24 +4,24 @@
RSpec.describe Gitlab::Git::WikiPageVersion do
let_it_be(:project) { create(:project, :public, :repository) }
let(:user) { create(:user, username: 'someone') }
let_it_be(:user) { create(:user, username: 'someone') }
describe '#author_url' do
subject(:author_url) { described_class.new(commit, nil).author_url }
describe '#author' do
subject(:author) { described_class.new(commit, nil).author }
context 'user exists in gitlab' do
let(:commit) { create(:commit, project: project, author: user) }
it 'returns the profile link of the user' do
expect(author_url).to eq('http://localhost/someone')
it 'returns the user' do
expect(author).to eq user
end
end
context 'user does not exist in gitlab' do
let(:commit) { create(:commit, project: project, author_email: "someone@somewebsite.com") }
it 'returns a mailto: url' do
expect(author_url).to eq('mailto:someone@somewebsite.com')
it 'returns nil' do
expect(author).to be_nil
end
end
end
......
......@@ -42,17 +42,6 @@
end
end
describe '#public_id' do
it 'returns an encrypted, url-encoded session id' do
original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8")
active_session = ActiveSession.new(session_id: original_session_id.public_id)
encrypted_id = active_session.public_id
derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id)
expect(original_session_id.public_id).to eq derived_session_id
end
end
describe '.list' do
it 'returns all sessions by user' do
Gitlab::Redis::SharedState.with do |redis|
......@@ -207,89 +196,9 @@
end
end
end
context 'ActiveSession stored by deprecated rack_session_public_key' do
let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
let(:deprecated_active_session_lookup_key) { rack_session.public_id }
before do
Gitlab::Redis::SharedState.with do |redis|
redis.set("session:user:gitlab:#{user.id}:#{deprecated_active_session_lookup_key}",
'')
redis.sadd(described_class.lookup_key_name(user.id),
deprecated_active_session_lookup_key)
end
end
it 'removes deprecated key and stores only new one' do
expected_session_keys = ["session:user:gitlab:#{user.id}:#{rack_session.private_id}",
"session:lookup:user:gitlab:#{user.id}"]
ActiveSession.set(user, request)
Gitlab::Redis::SharedState.with do |redis|
actual_session_keys = redis.scan_each(match: 'session:*').to_a
expect(actual_session_keys).to(match_array(expected_session_keys))
expect(redis.smembers("session:lookup:user:gitlab:#{user.id}")).to eq [rack_session.private_id]
end
end
end
end
describe '.destroy_with_rack_session_id' do
it 'gracefully handles a nil session ID' do
expect(described_class).not_to receive(:destroy_sessions)
ActiveSession.destroy_with_rack_session_id(user, nil)
end
it 'removes the entry associated with the currently killed user session' do
Gitlab::Redis::SharedState.with do |redis|
redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
redis.set("session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d", '')
redis.set("session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358", '')
end
ActiveSession.destroy_with_rack_session_id(user, request.session.id)
Gitlab::Redis::SharedState.with do |redis|
expect(redis.scan_each(match: "session:user:gitlab:*")).to match_array [
"session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d",
"session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358"
]
end
end
it 'removes the lookup entry' do
Gitlab::Redis::SharedState.with do |redis|
redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
redis.sadd("session:lookup:user:gitlab:#{user.id}", '6919a6f1bb119dd7396fadc38fd18d0d')
end
ActiveSession.destroy_with_rack_session_id(user, request.session.id)
Gitlab::Redis::SharedState.with do |redis|
expect(redis.scan_each(match: "session:lookup:user:gitlab:#{user.id}").to_a).to be_empty
end
end
it 'removes the devise session' do
Gitlab::Redis::SharedState.with do |redis|
redis.set("session:user:gitlab:#{user.id}:#{rack_session.private_id}", '')
# Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
redis.set("session:gitlab:#{rack_session.private_id}", '')
end
ActiveSession.destroy_with_rack_session_id(user, request.session.id)
Gitlab::Redis::SharedState.with do |redis|
expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty
end