Commit 1aa8f3f4 authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge remote-tracking branch 'dev/master'

parents f0337d33 c5cab5c3
Please view this file on the master branch, on stable branches it's out of date.
## 13.9.2 (2021-03-04)
- No changes.
## 13.9.1 (2021-02-23)
- No changes.
......@@ -158,6 +162,10 @@ Please view this file on the master branch, on stable branches it's out of date.
- Review UI text - repo push rules settings. !52797
## 13.8.5 (2021-03-04)
- No changes.
## 13.8.4 (2021-02-11)
### Security (1 change)
......@@ -300,6 +308,10 @@ Please view this file on the master branch, on stable branches it's out of date.
- Enable DevOps Adoption Report feature flag if any Segments already exist. !51602
## 13.7.8 (2021-03-04)
- No changes.
## 13.7.7 (2021-02-11)
### Security (1 change)
......
......@@ -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)
......@@ -588,6 +600,18 @@ entry.
- Apply new GitLab UI for buttons in pipeline schedules.
## 13.8.5 (2021-03-04)
### Security (6 changes)
- Fix XSS in wiki author email and name.
- Bump thrift gem to 0.14.0.
- Allow only owners to manage group variables.
- Do not store marshalled sessions ids in Redis.
- Workhorse: prevent escaped router path traversal.
- Fix XSS vulnerability for swagger file viewer.
## 13.8.4 (2021-02-11)
### Security (9 changes)
......@@ -988,6 +1012,17 @@ entry.
- Add verbiage + link sast to show it's in core. !51935
## 13.7.8 (2021-03-04)
### Security (5 changes)
- Bump thrift gem to 0.14.0.
- Allow only owners to manage group variables.
- Do not store marshalled sessions ids in Redis.
- Workhorse: prevent escaped router path traversal.
- Fix XSS vulnerability for swagger file viewer.
## 13.7.7 (2021-02-11)
### Security (9 changes)
......
......@@ -314,6 +314,9 @@ gem 'premailer-rails', '~> 1.10.3'
# LabKit: Tracing and Correlation
gem 'gitlab-labkit', '~> 0.16.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
......
......@@ -1578,6 +1578,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)
......
......@@ -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 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController
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 @@ module ActiveSessionsHelper
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
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 @@ class ActiveSession
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 @@ class ActiveSession
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 @@ class ActiveSession
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 @@ class ActiveSession
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 @@ class ActiveSession
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 @@ class ActiveSession
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
......
---
title: 'Workhorse: prevent escaped router path traversal'
merge_request:
author:
type: security
---
title: 'Workhorse: Stop logging when path is excluded'
merge_request:
author:
type: security
......@@ -42,8 +42,7 @@ Rails.application.configure do |config|
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 @@ module API
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 @@ module Gitlab
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 @@ require 'spec_helper'
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 @@ RSpec.describe Groups::VariablesController do
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 @@ RSpec.describe Groups::VariablesController do
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 @@ RSpec.describe Groups::VariablesController do
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 @@ RSpec.describe Profiles::ActiveSessionsController do
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 @@ require 'spec_helper'
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 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
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 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
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
end
end
describe '.destroy_with_deprecated_encryption' do
describe '.destroy_session' do
shared_examples 'removes all session data' do
before do
Gitlab::Redis::SharedState.with do |redis|
......@@ -330,7 +239,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
end
context 'destroy called with Rack::Session::SessionId#private_id' do
subject { ActiveSession.destroy_with_deprecated_encryption(user, rack_session.private_id) }
subject { ActiveSession.destroy_session(user, rack_session.private_id) }
it 'calls .destroy_sessions' do
expect(ActiveSession).to(
......@@ -347,26 +256,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
include_examples 'removes all session data'
end
end
context 'destroy called with ActiveSession#public_id (deprecated)' do
let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
let(:encrypted_active_session_id) { active_session.public_id }
let(:active_session_lookup_key) { rack_session.public_id }
subject { ActiveSession.destroy_with_deprecated_encryption(user, encrypted_active_session_id) }
it 'calls .destroy_sessions' do
expect(ActiveSession).to(
receive(:destroy_sessions)
.with(anything, user, [encrypted_active_session_id, rack_session.public_id, rack_session.private_id]))
subject
end
context 'ActiveSession with session_id (deprecated)' do
include_examples 'removes all session data'
end
end
end
describe '.destroy_all_but_current' do
......
......@@ -3,17 +3,20 @@
require 'spec_helper'
RSpec.describe API::GroupVariables do
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) }
describe 'GET /groups/:id/variables' do
let!(:variable) { create(:ci_group_variable, group: group) }
let(:access_level) {}
context 'authorized user with proper permissions' do
before do
group.add_maintainer(user)
group.add_user(user, access_level) if access_level
end
describe 'GET /groups/:id/variables' do
context 'authorized user with proper permissions' do
let(:access_level) { :owner }
it 'returns group variables' do
get api("/groups/#{group.id}/variables", user)
......@@ -23,6 +26,8 @@ RSpec.describe API::GroupVariables do
end
context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not return group variables' do
get api("/groups/#{group.id}/variables", user)
......@@ -40,12 +45,8 @@ RSpec.describe API::GroupVariables do
end
describe 'GET /groups/:id/variables/:key' do
let!(:variable) { create(:ci_group_variable, group: group) }
context 'authorized user with proper permissions' do
before do
group.add_maintainer(user)
end
let(:access_level) { :owner }
it 'returns group variable details' do
get api("/groups/#{group.id}/variables/#{variable.key}", user)
......@@ -64,6 +65,8 @@ RSpec.describe API::GroupVariables do
end
context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not return group variable details' do
get api("/groups/#{group.id}/variables/#{variable.key}", user)
......@@ -82,11 +85,7 @@ RSpec.describe API::GroupVariables do
describe 'POST /groups/:id/variables' do
context 'authorized user with proper permissions' do
let!(:variable) { create(:ci_group_variable, group: group) }
before do
group.add_maintainer(user)
end
let(:access_level) { :owner }
it 'creates variable' do
expect do
......@@ -124,6 +123,8 @@ RSpec.describe API::GroupVariables do
end
context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not create variable' do
post api("/groups/#{group.id}/variables", user)
......@@ -141,12 +142,8 @@ RSpec.describe API::GroupVariables do
end
describe 'PUT /groups/:id/variables/:key' do
let!(:variable) { create(:ci_group_variable, group: group) }
context 'authorized user with proper permissions' do
before do
group.add_maintainer(user)
end
let(:access_level) { :owner }
it 'updates variable data' do
initial_variable = group.variables.reload.first
......@@ -180,6 +177,8 @@ RSpec.describe API::GroupVariables do
end
context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not update variable' do
put api("/groups/#{group.id}/variables/#{variable.key}", user)
......@@ -197,12 +196,8 @@ RSpec.describe API::GroupVariables do
end
describe 'DELETE /groups/:id/variables/:key' do
let!(:variable) { create(:ci_group_variable, group: group) }
context 'authorized user with proper permissions' do
before do
group.add_maintainer(user)
end
let(:access_level) { :owner }
it 'deletes variable' do
expect do
......@@ -224,6 +219,8 @@ RSpec.describe API::GroupVariables do
end
context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not delete variable' do
delete api("/groups/#{group.id}/variables/#{variable.key}", user)
......
# Changelog for gitlab-workhorse
## v8.64.2
### Security
- Stop logging when path is excluded
https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
## v8.64.1
### Security
- Use URL.EscapePath() in upstream router
https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
## v8.64.0
### Other
......
......@@ -23,7 +23,7 @@ func TestIfNoDeployPageExist(t *testing.T) {
w := httptest.NewRecorder()
executed := false
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
executed = true
})).ServeHTTP(w, nil)
......@@ -45,7 +45,7 @@ func TestIfDeployPageExist(t *testing.T) {
w := httptest.NewRecorder()
executed := false
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
executed = true
})).ServeHTTP(w, nil)
......
......@@ -32,7 +32,7 @@ func TestIfErrorPageIsPresented(t *testing.T) {
require.NoError(t, err)
require.Equal(t, len(upstreamBody), n, "bytes written")
})
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush()
......@@ -54,7 +54,7 @@ func TestIfErrorPassedIfNoErrorPageIsFound(t *testing.T) {
w.WriteHeader(404)
fmt.Fprint(w, errorResponse)
})
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush()
......@@ -78,7 +78,7 @@ func TestIfErrorPageIsIgnoredInDevelopment(t *testing.T) {
w.WriteHeader(500)
fmt.Fprint(w, serverError)
})
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(true, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush()
require.Equal(t, 500, w.Code)
......@@ -102,7 +102,7 @@ func TestIfErrorPageIsIgnoredIfCustomError(t *testing.T) {
w.WriteHeader(500)
fmt.Fprint(w, serverError)
})
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush()
require.Equal(t, 500, w.Code)
......@@ -137,7 +137,7 @@ func TestErrorPageInterceptedByContentType(t *testing.T) {
w.WriteHeader(500)
fmt.Fprint(w, serverError)
})
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush()
require.Equal(t, 500, w.Code)
......@@ -161,7 +161,7 @@ func TestIfErrorPageIsPresentedJSON(t *testing.T) {
require.NoError(t, err)
require.Equal(t, len(upstreamBody), n, "bytes written")
})
st := &Static{""}
st := &Static{}
st.ErrorPagesUnless(false, ErrorFormatJSON, h).ServeHTTP(w, nil)
w.Flush()
......@@ -181,7 +181,7 @@ func TestIfErrorPageIsPresentedText(t *testing.T) {
require.NoError(t, err)
require.Equal(t, len(upstreamBody), n, "bytes written")
})
st := &Static{""}
st := &Static{}
st.ErrorPagesUnless(false, ErrorFormatText, h).ServeHTTP(w, nil)
w.Flush()
......
package staticpages
import (
"errors"
"fmt"
"net/http"
"os"
"path/filepath"
"strings"
"time"
"gitlab.com/gitlab-org/labkit/log"
"gitlab.com/gitlab-org/labkit/mask"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/log"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix"
)
......@@ -26,21 +28,28 @@ const (
// upstream.
func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoundHandler http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
file := filepath.Join(s.DocumentRoot, prefix.Strip(r.URL.Path))
if notFoundHandler == nil {
notFoundHandler = http.HandlerFunc(http.NotFound)
}
// We intentionally use r.URL.Path instead of r.URL.EscaptedPath() below.
// This is to make it possible to serve static files with e.g. a space
// %20 in their name.
relativePath, err := s.validatePath(prefix.Strip(r.URL.Path))
if err != nil {
notFoundHandler.ServeHTTP(w, r)
return
}
// The filepath.Join does Clean traversing directories up
file := filepath.Join(s.DocumentRoot, relativePath)
if !strings.HasPrefix(file, s.DocumentRoot) {
helper.Fail500(w, r, &os.PathError{
Op: "open",
Path: file,
Err: os.ErrInvalid,
})
log.WithRequest(r).WithError(errPathTraversal).Error()
notFoundHandler.ServeHTTP(w, r)
return
}
var content *os.File
var fi os.FileInfo
var err error
// Serve pre-gzipped assets
if acceptEncoding := r.Header.Get("Accept-Encoding"); strings.Contains(acceptEncoding, "gzip") {
......@@ -55,11 +64,7 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun
content, fi, err = helper.OpenFile(file)
}
if err != nil {
if notFoundHandler != nil {
notFoundHandler.ServeHTTP(w, r)
} else {
http.NotFound(w, r)
}
return
}
defer content.Close()
......@@ -82,3 +87,17 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun
http.ServeContent(w, r, filepath.Base(file), fi.ModTime(), content)
})
}
var errPathTraversal = errors.New("path traversal")
func (s *Static) validatePath(filename string) (string, error) {
filename = filepath.Clean(filename)
for _, exc := range s.Exclude {
if strings.HasPrefix(filename, exc) {
return "", fmt.Errorf("file is excluded: %s", exc)
}
}
return filename, nil
}
......@@ -20,7 +20,7 @@ func TestServingNonExistingFile(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/file", nil)
w := httptest.NewRecorder()
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 404, w.Code)
}
......@@ -34,7 +34,7 @@ func TestServingDirectory(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/file", nil)
w := httptest.NewRecorder()
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 404, w.Code)
}
......@@ -44,7 +44,7 @@ func TestServingMalformedUri(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/../../../static/file", nil)
w := httptest.NewRecorder()
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 404, w.Code)
}
......@@ -54,7 +54,7 @@ func TestExecutingHandlerWhenNoFileFound(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/file", nil)
executed := false
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
executed = (r == httpRequest)
})).ServeHTTP(nil, httpRequest)
......@@ -76,7 +76,7 @@ func TestServingTheActualFile(t *testing.T) {
ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600)
w := httptest.NewRecorder()
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 200, w.Code)
if w.Body.String() != fileContent {
......@@ -84,6 +84,40 @@ func TestServingTheActualFile(t *testing.T) {
}
}
func TestExcludedPaths(t *testing.T) {
testCases := []struct {
desc string
path string
found bool
contents string
}{
{"allowed file", "/file1", true, "contents1"},
{"path traversal is allowed", "/uploads/../file1", true, "contents1"},
{"files in /uploads/ are invisible", "/uploads/file2", false, ""},
{"cannot use path traversal to get to /uploads/", "/foobar/../uploads/file2", false, ""},
{"cannot use escaped path traversal to get to /uploads/", "/foobar%2f%2e%2e%2fuploads/file2", false, ""},
{"cannot use double escaped path traversal to get to /uploads/", "/foobar%252f%252e%252e%252fuploads/file2", false, ""},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
httpRequest, err := http.NewRequest("GET", tc.path, nil)
require.NoError(t, err)
w := httptest.NewRecorder()
st := &Static{DocumentRoot: "testdata", Exclude: []string{"/uploads/"}}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
if tc.found {
require.Equal(t, 200, w.Code)
require.Equal(t, tc.contents, w.Body.String())
} else {
require.Equal(t, 404, w.Code)
}
})
}
}
func testServingThePregzippedFile(t *testing.T, enableGzip bool) {
dir, err := ioutil.TempDir("", "deploy")
if err != nil {
......@@ -108,7 +142,7 @@ func testServingThePregzippedFile(t *testing.T, enableGzip bool) {
ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600)
w := httptest.NewRecorder()
st := &Static{dir}
st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 200, w.Code)
if enableGzip {
......
......@@ -2,4 +2,5 @@ package staticpages
type Static struct {
DocumentRoot string
Exclude []string
}
contents1
\ No newline at end of file
contents2
\ No newline at end of file
......@@ -62,6 +62,14 @@ const (
importPattern = `^/import/`
)
var (
// For legacy reasons, user uploads are stored in public/uploads. To
// prevent anybody who knows/guesses the URL of a user-uploaded file
// from downloading it we configure static.ServeExisting to treat files
// under public/uploads/ as if they do not exist.
staticExclude = []string{"/uploads/"}
)
func compileRegexp(regexpStr string) *regexp.Regexp {
if len(regexpStr) == 0 {
return nil
......@@ -181,20 +189,20 @@ func buildProxy(backend *url.URL, version string, rt http.RoundTripper, cfg conf
// We match against URI not containing the relativeUrlRoot:
// see upstream.ServeHTTP
func (u *upstream) configureRoutes() {
func configureRoutes(u *upstream) {
api := apipkg.NewAPI(
u.Backend,
u.Version,
u.RoundTripper,
)
static := &staticpages.Static{DocumentRoot: u.DocumentRoot}
static := &staticpages.Static{DocumentRoot: u.DocumentRoot, Exclude: staticExclude}
proxy := buildProxy(u.Backend, u.Version, u.RoundTripper, u.Config)
cableProxy := proxypkg.NewProxy(u.CableBackend, u.Version, u.CableRoundTripper)
assetsNotFoundHandler := NotFoundUnless(u.DevelopmentMode, proxy)
if u.AltDocumentRoot != "" {
altStatic := &staticpages.Static{DocumentRoot: u.AltDocumentRoot}
altStatic := &staticpages.Static{DocumentRoot: u.AltDocumentRoot, Exclude: staticExclude}
assetsNotFoundHandler = altStatic.ServeExisting(
u.URLPrefix,
staticpages.CacheExpireMax,
......@@ -306,12 +314,6 @@ func (u *upstream) configureRoutes() {
u.route("POST", snippetUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)),
u.route("POST", userUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)),
// For legacy reasons, user uploads are stored under the document root.
// To prevent anybody who knows/guesses the URL of a user-uploaded file
// from downloading it we make sure requests to /uploads/ do _not_ pass
// through static.ServeExisting.
u.route("", `^/uploads/`, static.ErrorPagesUnless(u.DevelopmentMode, staticpages.ErrorFormatHTML, proxy)),
// health checks don't intercept errors and go straight to rails
// TODO: We should probably not return a HTML deploy page?
// https://gitlab.com/gitlab-org/gitlab-workhorse/issues/230
......
......@@ -40,6 +40,10 @@ type upstream struct {
}
func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler {
return newUpstream(cfg, accessLogger, configureRoutes)
}
func newUpstream(cfg config.Config, accessLogger *logrus.Logger, routesCallback func(*upstream)) http.Handler {
up := upstream{
Config: cfg,
accessLogger: accessLogger,
......@@ -56,7 +60,7 @@ func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler {
up.RoundTripper = roundtripper.NewBackendRoundTripper(up.Backend, up.Socket, up.ProxyHeadersTimeout, cfg.DevelopmentMode)
up.CableRoundTripper = roundtripper.NewBackendRoundTripper(up.CableBackend, up.CableSocket, up.ProxyHeadersTimeout, cfg.DevelopmentMode)
up.configureURLPrefix()
up.configureRoutes()
routesCallback(&up)
var correlationOpts []correlation.InboundHandlerOption
if cfg.PropagateCorrelationID {
......@@ -95,7 +99,7 @@ func (u *upstream) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}
// Check URL Root
URIPath := urlprefix.CleanURIPath(r.URL.Path)
URIPath := urlprefix.CleanURIPath(r.URL.EscapedPath())
prefix := u.URLPrefix
if !prefix.Match(URIPath) {
helper.HTTPError(w, r, fmt.Sprintf("Not found %q", URIPath), http.StatusNotFound)
......
package upstream
import (
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
)
func TestRouting(t *testing.T) {
handle := func(u *upstream, regex string) routeEntry {
handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
io.WriteString(w, regex)
})
return u.route("", regex, handler)
}
const (
foobar = `\A/foobar\z`
quxbaz = `\A/quxbaz\z`
main = ""
)
u := newUpstream(config.Config{}, logrus.StandardLogger(), func(u *upstream) {
u.Routes = []routeEntry{
handle(u, foobar),
handle(u, quxbaz),
handle(u, main),
}
})
ts := httptest.NewServer(u)
defer ts.Close()
testCases := []struct {
desc string
path string
route string
}{
{"main route works", "/", main},
{"foobar route works", "/foobar", foobar},
{"quxbaz route works", "/quxbaz", quxbaz},
{"path traversal works, ends up in quxbaz", "/foobar/../quxbaz", quxbaz},
{"escaped path traversal does not match any route", "/foobar%2f%2e%2e%2fquxbaz", main},
{"double escaped path traversal does not match any route", "/foobar%252f%252e%252e%252fquxbaz", main},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
resp, err := http.Get(ts.URL + tc.path)
require.NoError(t, err)
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)
require.NoError(t, err)
require.Equal(t, 200, resp.StatusCode, "response code")
require.Equal(t, tc.route, string(body))
})
}
}
......@@ -222,12 +222,15 @@ func TestDeniedPublicUploadsFile(t *testing.T) {
for _, resource := range []string{
"/uploads/static.txt",
"/uploads%2Fstatic.txt",
"/foobar%2F%2E%2E%2Fuploads/static.txt",
} {
t.Run(resource, func(t *testing.T) {
resp, body := httpGet(t, ws.URL+resource, nil)
require.Equal(t, 404, resp.StatusCode, "GET %q: status code", resource)
require.Equal(t, "", body, "GET %q: response body", resource)
require.True(t, proxied, "GET %q: never made it to backend", resource)
})
}
}
......
......@@ -11415,10 +11415,10 @@ svg-tags@^1.0.0:
resolved "https://registry.yarnpkg.com/svg-tags/-/svg-tags-1.0.0.tgz#58f71cee3bd519b59d4b2a843b6c7de64ac04764"
integrity sha1-WPcc7jvVGbWdSyqEO2x95krAR2Q=
swagger-ui-dist@^3.32.4:
version "3.32.4"
resolved "https://registry.yarnpkg.com/swagger-ui-dist/-/swagger-ui-dist-3.32.4.tgz#6fa920a99e38eaaf129580ac158cf730494a2190"
integrity sha512-3qUqK131a5nqGdDJhLflTNzvrjZgjBlINYNx+Jm5lw/Va88Lcu5iyjUupY3Js/Kf326z1XtXkrr6TbvE6r925g==
swagger-ui-dist@^3.43.0:
version "3.43.0"
resolved "https://registry.yarnpkg.com/swagger-ui-dist/-/swagger-ui-dist-3.43.0.tgz#b064a2cec1d27776f9a124bc70423cfa0bbc0d3f"
integrity sha512-PtE+g23bNbYv8qqAVoPBqNQth8hU5Sl5ZsQ7gHXlO5jlCt31dVTiKI9ArHIT1b23ZzUYTnKsFgPYYFoiWyNCAw==
symbol-observable@^1.0.2:
version "1.2.0"
......
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