Commit 5fb29e5d authored by Robert Speicher's avatar Robert Speicher

Merge branch 'master' of gitlab.com:gitlab-org/security/gitlab

parents 1d17649c 8c55c6eb
Please view this file on the master branch, on stable branches it's out of date. 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) ## 13.9.1 (2021-02-23)
- No changes. - No changes.
...@@ -158,6 +162,10 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -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 - Review UI text - repo push rules settings. !52797
## 13.8.5 (2021-03-04)
- No changes.
## 13.8.4 (2021-02-11) ## 13.8.4 (2021-02-11)
### Security (1 change) ### Security (1 change)
...@@ -300,6 +308,10 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -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 - 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) ## 13.7.7 (2021-02-11)
### Security (1 change) ### Security (1 change)
......
...@@ -2,6 +2,18 @@ ...@@ -2,6 +2,18 @@
documentation](doc/development/changelog.md) for instructions on adding your own documentation](doc/development/changelog.md) for instructions on adding your own
entry. 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) ## 13.9.1 (2021-02-23)
### Fixed (6 changes, 1 of them is from the community) ### Fixed (6 changes, 1 of them is from the community)
...@@ -588,6 +600,18 @@ entry. ...@@ -588,6 +600,18 @@ entry.
- Apply new GitLab UI for buttons in pipeline schedules. - 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) ## 13.8.4 (2021-02-11)
### Security (9 changes) ### Security (9 changes)
...@@ -988,6 +1012,17 @@ entry. ...@@ -988,6 +1012,17 @@ entry.
- Add verbiage + link sast to show it's in core. !51935 - 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) ## 13.7.7 (2021-02-11)
### Security (9 changes) ### Security (9 changes)
......
...@@ -314,6 +314,9 @@ gem 'premailer-rails', '~> 1.10.3' ...@@ -314,6 +314,9 @@ gem 'premailer-rails', '~> 1.10.3'
# LabKit: Tracing and Correlation # LabKit: Tracing and Correlation
gem 'gitlab-labkit', '~> 0.16.0' 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 # I18n
gem 'ruby_parser', '~> 3.15', require: false gem 'ruby_parser', '~> 3.15', require: false
......
...@@ -1578,6 +1578,7 @@ DEPENDENCIES ...@@ -1578,6 +1578,7 @@ DEPENDENCIES
terser (= 1.0.2) terser (= 1.0.2)
test-prof (~> 0.12.0) test-prof (~> 0.12.0)
thin (~> 1.8.0) thin (~> 1.8.0)
thrift (>= 0.14.0)
timecop (~> 0.9.1) timecop (~> 0.9.1)
toml-rb (~> 1.0.0) toml-rb (~> 1.0.0)
truncato (~> 0.7.11) truncato (~> 0.7.11)
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
module Groups module Groups
class VariablesController < Groups::ApplicationController class VariablesController < Groups::ApplicationController
before_action :authorize_admin_build! before_action :authorize_admin_group!
skip_cross_project_access_check :show, :update skip_cross_project_access_check :show, :update
......
...@@ -8,9 +8,8 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController ...@@ -8,9 +8,8 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController
end end
def destroy def destroy
# params[:id] can be either an Rack::Session::SessionId#private_id # params[:id] can be an Rack::Session::SessionId#private_id
# or an encrypted Rack::Session::SessionId#public_id ActiveSession.destroy_session(current_user, params[:id])
ActiveSession.destroy_with_deprecated_encryption(current_user, params[:id])
current_user.forget_me! current_user.forget_me!
respond_to do |format| respond_to do |format|
......
...@@ -24,11 +24,6 @@ module ActiveSessionsHelper ...@@ -24,11 +24,6 @@ module ActiveSessionsHelper
end end
def revoke_session_path(active_session) def revoke_session_path(active_session)
if active_session.session_private_id profile_active_session_path(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
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 ...@@ -42,13 +42,6 @@ class ActiveSession
device_type&.titleize device_type&.titleize
end 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) def self.set(user, request)
Gitlab::Redis::SharedState.with do |redis| Gitlab::Redis::SharedState.with do |redis|
session_private_id = request.session.id.private_id session_private_id = request.session.id.private_id
...@@ -63,8 +56,6 @@ class ActiveSession ...@@ -63,8 +56,6 @@ class ActiveSession
device_type: client.device_type, device_type: client.device_type,
created_at: user.current_sign_in_at || timestamp, created_at: user.current_sign_in_at || timestamp,
updated_at: timestamp, updated_at: timestamp,
# TODO: remove in 13.7
session_id: request.session.id.public_id,
session_private_id: session_private_id, session_private_id: session_private_id,
is_impersonated: request.session[:impersonator_id].present? is_impersonated: request.session[:impersonator_id].present?
) )
...@@ -80,20 +71,10 @@ class ActiveSession ...@@ -80,20 +71,10 @@ class ActiveSession
lookup_key_name(user.id), lookup_key_name(user.id),
session_private_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 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) def self.list(user)
Gitlab::Redis::SharedState.with do |redis| Gitlab::Redis::SharedState.with do |redis|
cleaned_up_lookup_entries(redis, user).map do |raw_session| cleaned_up_lookup_entries(redis, user).map do |raw_session|
...@@ -109,18 +90,6 @@ class ActiveSession ...@@ -109,18 +90,6 @@ class ActiveSession
end end
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) def self.destroy_sessions(redis, user, session_ids)
key_names = session_ids.map { |session_id| key_name(user.id, session_id) } key_names = session_ids.map { |session_id| key_name(user.id, session_id) }
...@@ -132,19 +101,11 @@ class ActiveSession ...@@ -132,19 +101,11 @@ class ActiveSession
end end
end end
# TODO: remove in 13.7 def self.destroy_session(user, session_id)
# After upgrade, .destroy might be called with the session id encrypted
# by .public_id.
def self.destroy_with_deprecated_encryption(user, session_id)
return unless 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| 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
end end
...@@ -275,11 +236,4 @@ class ActiveSession ...@@ -275,11 +236,4 @@ class ActiveSession
entries.compact entries.compact
end 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 end
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
.nav-text.flex-fill .nav-text.flex-fill
%span.wiki-last-edit-by %span.wiki-last-edit-by
- if @page.last_version - 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) = time_ago_with_tooltip(@page.last_version.authored_date)
.nav-controls.pb-md-3.pb-lg-0 .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| ...@@ -42,8 +42,7 @@ Rails.application.configure do |config|
activity = Gitlab::Auth::Activity.new(opts) activity = Gitlab::Auth::Activity.new(opts)
tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth) tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth)
# TODO: switch to `auth.request.session.id.private_id` in 13.7 ActiveSession.destroy_session(user, auth.request.session.id.private_id) if auth.request.session.id
ActiveSession.destroy_with_rack_session_id(user, auth.request.session.id)
activity.user_session_destroyed! activity.user_session_destroyed!
## ##
......
...@@ -5,8 +5,7 @@ module API ...@@ -5,8 +5,7 @@ module API
include PaginationParams include PaginationParams
before { authenticate! } before { authenticate! }
before { authorize! :admin_build, user_group } before { authorize! :admin_group, user_group }
feature_category :continuous_integration feature_category :continuous_integration
params do params do
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
module Gitlab module Gitlab
module Git module Git
class WikiPageVersion class WikiPageVersion
include Gitlab::Utils::StrongMemoize
attr_reader :commit, :format attr_reader :commit, :format
def initialize(commit, format) def initialize(commit, format)
...@@ -12,9 +14,10 @@ module Gitlab ...@@ -12,9 +14,10 @@ module Gitlab
delegate :message, :sha, :id, :author_name, :author_email, :authored_date, to: :commit delegate :message, :sha, :id, :author_name, :author_email, :authored_date, to: :commit
def author_url def author
user = ::User.find_by_any_email(author_email) strong_memoize(:author) do
user.nil? ? "mailto:#{author_email}" : Gitlab::UrlBuilder.build(user) ::User.find_by_any_email(author_email)
end
end end
end end
end end
......
...@@ -5,26 +5,35 @@ require 'spec_helper' ...@@ -5,26 +5,35 @@ require 'spec_helper'
RSpec.describe Groups::VariablesController do RSpec.describe Groups::VariablesController do
include ExternalAuthorizationServiceHelpers include ExternalAuthorizationServiceHelpers
let(:group) { create(:group) } let_it_be(:group) { create(:group) }
let(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:variable) { create(:ci_group_variable, group: group) }
let(:access_level) { :owner }
before do before do
sign_in(user) sign_in(user)
group.add_maintainer(user) group.add_user(user, access_level)
end end
describe 'GET #show' do describe 'GET #show' do
let!(:variable) { create(:ci_group_variable, group: group) }
subject do subject do
get :show, params: { group_id: group }, format: :json get :show, params: { group_id: group }, format: :json
end end
include_examples 'GET #show lists all variables' 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 end
describe 'PATCH #update' do describe 'PATCH #update' do
let!(:variable) { create(:ci_group_variable, group: group) }
let(:owner) { group } let(:owner) { group }
subject do subject do
...@@ -37,6 +46,19 @@ RSpec.describe Groups::VariablesController do ...@@ -37,6 +46,19 @@ RSpec.describe Groups::VariablesController do
end end
include_examples 'PATCH #update updates variables' 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 end
context 'with external authorization enabled' do context 'with external authorization enabled' do
...@@ -45,8 +67,6 @@ RSpec.describe Groups::VariablesController do ...@@ -45,8 +67,6 @@ RSpec.describe Groups::VariablesController do
end end
describe 'GET #show' do describe 'GET #show' do
let!(:variable) { create(:ci_group_variable, group: group) }
it 'is successful' do it 'is successful' do
get :show, params: { group_id: group }, format: :json get :show, params: { group_id: group }, format: :json
...@@ -55,9 +75,6 @@ RSpec.describe Groups::VariablesController do ...@@ -55,9 +75,6 @@ RSpec.describe Groups::VariablesController do
end end
describe 'PATCH #update' do describe 'PATCH #update' do
let!(:variable) { create(:ci_group_variable, group: group) }
let(:owner) { group }
it 'is successful' do it 'is successful' do
patch :update, patch :update,
params: { params: {
......
...@@ -12,7 +12,7 @@ RSpec.describe Profiles::ActiveSessionsController do ...@@ -12,7 +12,7 @@ RSpec.describe Profiles::ActiveSessionsController do
it 'invalidates all remember user tokens' do it 'invalidates all remember user tokens' do
ActiveSession.set(user, request) ActiveSession.set(user, request)
session_id = request.session.id.public_id session_id = request.session.id.private_id
user.remember_me! user.remember_me!
delete :destroy, params: { id: session_id } 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' ...@@ -4,24 +4,24 @@ require 'spec_helper'
RSpec.describe Gitlab::Git::WikiPageVersion do RSpec.describe Gitlab::Git::WikiPageVersion do
let_it_be(:project) { create(:project, :public, :repository) } 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 describe '#author' do
subject(:author_url) { described_class.new(commit, nil).author_url } subject(:author) { described_class.new(commit, nil).author }
context 'user exists in gitlab' do context 'user exists in gitlab' do
let(:commit) { create(:commit, project: project, author: user) } let(:commit) { create(:commit, project: project, author: user) }
it 'returns the profile link of the user' do it 'returns the user' do
expect(author_url).to eq('http://localhost/someone') expect(author).to eq user
end end
end end
context 'user does not exist in gitlab' do context 'user does not exist in gitlab' do
let(:commit) { create(:commit, project: project, author_email: "someone@somewebsite.com") } let(:commit) { create(:commit, project: project, author_email: "someone@somewebsite.com") }
it 'returns a mailto: url' do it 'returns nil' do
expect(author_url).to eq('mailto:someone@somewebsite.com') expect(author).to be_nil
end end
end end
end end
......
...@@ -42,17 +42,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do ...@@ -42,17 +42,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
end end
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 describe '.list' do
it 'returns all sessions by user' do it 'returns all sessions by user' do
Gitlab::Redis::SharedState.with do |redis| Gitlab::Redis::SharedState.with do |redis|
...@@ -207,89 +196,9 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do ...@@ -207,89 +196,9 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
end end
end 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 end
describe '.destroy_with_rack_session_id' do describe '.destroy_session' 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
shared_examples 'removes all session data' do shared_examples 'removes all session data' do
before do before do
Gitlab::Redis::SharedState.with do |redis| Gitlab::Redis::SharedState.with do |redis|
...@@ -330,7 +239,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do ...@@ -330,7 +239,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
end end
context 'destroy called with Rack::Session::SessionId#private_id' do 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 it 'calls .destroy_sessions' do
expect(ActiveSession).to( expect(ActiveSession).to(
...@@ -347,26 +256,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do ...@@ -347,26 +256,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
include_examples 'removes all session data' include_examples 'removes all session data'
end end
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 end
describe '.destroy_all_but_current' do describe '.destroy_all_but_current' do
......
...@@ -3,16 +3,19 @@ ...@@ -3,16 +3,19 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe API::GroupVariables do RSpec.describe API::GroupVariables do
let(:group) { create(:group) } let_it_be(:group) { create(:group) }
let(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:variable) { create(:ci_group_variable, group: group) }
describe 'GET /groups/:id/variables' do let(:access_level) {}
let!(:variable) { create(:ci_group_variable, group: group) }
before do
group.add_user(user, access_level) if access_level
end
describe 'GET /groups/:id/variables' do
context 'authorized user with proper permissions' do context 'authorized user with proper permissions' do
before do let(:access_level) { :owner }
group.add_maintainer(user)
end
it 'returns group variables' do it 'returns group variables' do
get api("/groups/#{group.id}/variables", user) get api("/groups/#{group.id}/variables", user)
...@@ -23,6 +26,8 @@ RSpec.describe API::GroupVariables do ...@@ -23,6 +26,8 @@ RSpec.describe API::GroupVariables do
end end
context 'authorized user with invalid permissions' do context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not return group variables' do it 'does not return group variables' do
get api("/groups/#{group.id}/variables", user) get api("/groups/#{group.id}/variables", user)
...@@ -40,12 +45,8 @@ RSpec.describe API::GroupVariables do ...@@ -40,12 +45,8 @@ RSpec.describe API::GroupVariables do
end end
describe 'GET /groups/:id/variables/:key' do describe 'GET /groups/:id/variables/:key' do
let!(:variable) { create(:ci_group_variable, group: group) }
context 'authorized user with proper permissions' do context 'authorized user with proper permissions' do
before do let(:access_level) { :owner }
group.add_maintainer(user)
end
it 'returns group variable details' do it 'returns group variable details' do
get api("/groups/#{group.id}/variables/#{variable.key}", user) get api("/groups/#{group.id}/variables/#{variable.key}", user)
...@@ -64,6 +65,8 @@ RSpec.describe API::GroupVariables do ...@@ -64,6 +65,8 @@ RSpec.describe API::GroupVariables do
end end
context 'authorized user with invalid permissions' do context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not return group variable details' do it 'does not return group variable details' do
get api("/groups/#{group.id}/variables/#{variable.key}", user) get api("/groups/#{group.id}/variables/#{variable.key}", user)
...@@ -82,11 +85,7 @@ RSpec.describe API::GroupVariables do ...@@ -82,11 +85,7 @@ RSpec.describe API::GroupVariables do
describe 'POST /groups/:id/variables' do describe 'POST /groups/:id/variables' do
context 'authorized user with proper permissions' do context 'authorized user with proper permissions' do
let!(:variable) { create(:ci_group_variable, group: group) } let(:access_level) { :owner }
before do
group.add_maintainer(user)
end
it 'creates variable' do it 'creates variable' do
expect do expect do
...@@ -124,6 +123,8 @@ RSpec.describe API::GroupVariables do ...@@ -124,6 +123,8 @@ RSpec.describe API::GroupVariables do
end end
context 'authorized user with invalid permissions' do context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not create variable' do it 'does not create variable' do
post api("/groups/#{group.id}/variables", user) post api("/groups/#{group.id}/variables", user)
...@@ -141,12 +142,8 @@ RSpec.describe API::GroupVariables do ...@@ -141,12 +142,8 @@ RSpec.describe API::GroupVariables do
end end
describe 'PUT /groups/:id/variables/:key' do describe 'PUT /groups/:id/variables/:key' do
let!(:variable) { create(:ci_group_variable, group: group) }
context 'authorized user with proper permissions' do context 'authorized user with proper permissions' do
before do let(:access_level) { :owner }
group.add_maintainer(user)
end
it 'updates variable data' do it 'updates variable data' do
initial_variable = group.variables.reload.first initial_variable = group.variables.reload.first
...@@ -180,6 +177,8 @@ RSpec.describe API::GroupVariables do ...@@ -180,6 +177,8 @@ RSpec.describe API::GroupVariables do
end end
context 'authorized user with invalid permissions' do context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not update variable' do it 'does not update variable' do
put api("/groups/#{group.id}/variables/#{variable.key}", user) put api("/groups/#{group.id}/variables/#{variable.key}", user)
...@@ -197,12 +196,8 @@ RSpec.describe API::GroupVariables do ...@@ -197,12 +196,8 @@ RSpec.describe API::GroupVariables do
end end
describe 'DELETE /groups/:id/variables/:key' do describe 'DELETE /groups/:id/variables/:key' do
let!(:variable) { create(:ci_group_variable, group: group) }
context 'authorized user with proper permissions' do context 'authorized user with proper permissions' do
before do let(:access_level) { :owner }
group.add_maintainer(user)
end
it 'deletes variable' do it 'deletes variable' do
expect do expect do
...@@ -224,6 +219,8 @@ RSpec.describe API::GroupVariables do ...@@ -224,6 +219,8 @@ RSpec.describe API::GroupVariables do
end end
context 'authorized user with invalid permissions' do context 'authorized user with invalid permissions' do
let(:access_level) { :maintainer }
it 'does not delete variable' do it 'does not delete variable' do
delete api("/groups/#{group.id}/variables/#{variable.key}", user) delete api("/groups/#{group.id}/variables/#{variable.key}", user)
......
# Changelog for gitlab-workhorse # 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 ## v8.64.0
### Other ### Other
......
...@@ -23,7 +23,7 @@ func TestIfNoDeployPageExist(t *testing.T) { ...@@ -23,7 +23,7 @@ func TestIfNoDeployPageExist(t *testing.T) {
w := httptest.NewRecorder() w := httptest.NewRecorder()
executed := false executed := false
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
executed = true executed = true
})).ServeHTTP(w, nil) })).ServeHTTP(w, nil)
...@@ -45,7 +45,7 @@ func TestIfDeployPageExist(t *testing.T) { ...@@ -45,7 +45,7 @@ func TestIfDeployPageExist(t *testing.T) {
w := httptest.NewRecorder() w := httptest.NewRecorder()
executed := false executed := false
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { st.DeployPage(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
executed = true executed = true
})).ServeHTTP(w, nil) })).ServeHTTP(w, nil)
......
...@@ -32,7 +32,7 @@ func TestIfErrorPageIsPresented(t *testing.T) { ...@@ -32,7 +32,7 @@ func TestIfErrorPageIsPresented(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
require.Equal(t, len(upstreamBody), n, "bytes written") require.Equal(t, len(upstreamBody), n, "bytes written")
}) })
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush() w.Flush()
...@@ -54,7 +54,7 @@ func TestIfErrorPassedIfNoErrorPageIsFound(t *testing.T) { ...@@ -54,7 +54,7 @@ func TestIfErrorPassedIfNoErrorPageIsFound(t *testing.T) {
w.WriteHeader(404) w.WriteHeader(404)
fmt.Fprint(w, errorResponse) fmt.Fprint(w, errorResponse)
}) })
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush() w.Flush()
...@@ -78,7 +78,7 @@ func TestIfErrorPageIsIgnoredInDevelopment(t *testing.T) { ...@@ -78,7 +78,7 @@ func TestIfErrorPageIsIgnoredInDevelopment(t *testing.T) {
w.WriteHeader(500) w.WriteHeader(500)
fmt.Fprint(w, serverError) fmt.Fprint(w, serverError)
}) })
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(true, ErrorFormatHTML, h).ServeHTTP(w, nil) st.ErrorPagesUnless(true, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush() w.Flush()
require.Equal(t, 500, w.Code) require.Equal(t, 500, w.Code)
...@@ -102,7 +102,7 @@ func TestIfErrorPageIsIgnoredIfCustomError(t *testing.T) { ...@@ -102,7 +102,7 @@ func TestIfErrorPageIsIgnoredIfCustomError(t *testing.T) {
w.WriteHeader(500) w.WriteHeader(500)
fmt.Fprint(w, serverError) fmt.Fprint(w, serverError)
}) })
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush() w.Flush()
require.Equal(t, 500, w.Code) require.Equal(t, 500, w.Code)
...@@ -137,7 +137,7 @@ func TestErrorPageInterceptedByContentType(t *testing.T) { ...@@ -137,7 +137,7 @@ func TestErrorPageInterceptedByContentType(t *testing.T) {
w.WriteHeader(500) w.WriteHeader(500)
fmt.Fprint(w, serverError) fmt.Fprint(w, serverError)
}) })
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil) st.ErrorPagesUnless(false, ErrorFormatHTML, h).ServeHTTP(w, nil)
w.Flush() w.Flush()
require.Equal(t, 500, w.Code) require.Equal(t, 500, w.Code)
...@@ -161,7 +161,7 @@ func TestIfErrorPageIsPresentedJSON(t *testing.T) { ...@@ -161,7 +161,7 @@ func TestIfErrorPageIsPresentedJSON(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
require.Equal(t, len(upstreamBody), n, "bytes written") require.Equal(t, len(upstreamBody), n, "bytes written")
}) })
st := &Static{""} st := &Static{}
st.ErrorPagesUnless(false, ErrorFormatJSON, h).ServeHTTP(w, nil) st.ErrorPagesUnless(false, ErrorFormatJSON, h).ServeHTTP(w, nil)
w.Flush() w.Flush()
...@@ -181,7 +181,7 @@ func TestIfErrorPageIsPresentedText(t *testing.T) { ...@@ -181,7 +181,7 @@ func TestIfErrorPageIsPresentedText(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
require.Equal(t, len(upstreamBody), n, "bytes written") require.Equal(t, len(upstreamBody), n, "bytes written")
}) })
st := &Static{""} st := &Static{}
st.ErrorPagesUnless(false, ErrorFormatText, h).ServeHTTP(w, nil) st.ErrorPagesUnless(false, ErrorFormatText, h).ServeHTTP(w, nil)
w.Flush() w.Flush()
......
package staticpages package staticpages
import ( import (
"errors"
"fmt"
"net/http" "net/http"
"os" "os"
"path/filepath" "path/filepath"
"strings" "strings"
"time" "time"
"gitlab.com/gitlab-org/labkit/log"
"gitlab.com/gitlab-org/labkit/mask" "gitlab.com/gitlab-org/labkit/mask"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper" "gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/log"
"gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix" "gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix"
) )
...@@ -26,21 +28,28 @@ const ( ...@@ -26,21 +28,28 @@ const (
// upstream. // upstream.
func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoundHandler http.Handler) http.Handler { func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoundHandler http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { 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) { if !strings.HasPrefix(file, s.DocumentRoot) {
helper.Fail500(w, r, &os.PathError{ log.WithRequest(r).WithError(errPathTraversal).Error()
Op: "open", notFoundHandler.ServeHTTP(w, r)
Path: file,
Err: os.ErrInvalid,
})
return return
} }
var content *os.File var content *os.File
var fi os.FileInfo var fi os.FileInfo
var err error
// Serve pre-gzipped assets // Serve pre-gzipped assets
if acceptEncoding := r.Header.Get("Accept-Encoding"); strings.Contains(acceptEncoding, "gzip") { 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 ...@@ -55,11 +64,7 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun
content, fi, err = helper.OpenFile(file) content, fi, err = helper.OpenFile(file)
} }
if err != nil { if err != nil {
if notFoundHandler != nil { notFoundHandler.ServeHTTP(w, r)
notFoundHandler.ServeHTTP(w, r)
} else {
http.NotFound(w, r)
}
return return
} }
defer content.Close() defer content.Close()
...@@ -82,3 +87,17 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun ...@@ -82,3 +87,17 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun
http.ServeContent(w, r, filepath.Base(file), fi.ModTime(), content) 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) { ...@@ -20,7 +20,7 @@ func TestServingNonExistingFile(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/file", nil) httpRequest, _ := http.NewRequest("GET", "/file", nil)
w := httptest.NewRecorder() w := httptest.NewRecorder()
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 404, w.Code) require.Equal(t, 404, w.Code)
} }
...@@ -34,7 +34,7 @@ func TestServingDirectory(t *testing.T) { ...@@ -34,7 +34,7 @@ func TestServingDirectory(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/file", nil) httpRequest, _ := http.NewRequest("GET", "/file", nil)
w := httptest.NewRecorder() w := httptest.NewRecorder()
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 404, w.Code) require.Equal(t, 404, w.Code)
} }
...@@ -44,7 +44,7 @@ func TestServingMalformedUri(t *testing.T) { ...@@ -44,7 +44,7 @@ func TestServingMalformedUri(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/../../../static/file", nil) httpRequest, _ := http.NewRequest("GET", "/../../../static/file", nil)
w := httptest.NewRecorder() w := httptest.NewRecorder()
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 404, w.Code) require.Equal(t, 404, w.Code)
} }
...@@ -54,7 +54,7 @@ func TestExecutingHandlerWhenNoFileFound(t *testing.T) { ...@@ -54,7 +54,7 @@ func TestExecutingHandlerWhenNoFileFound(t *testing.T) {
httpRequest, _ := http.NewRequest("GET", "/file", nil) httpRequest, _ := http.NewRequest("GET", "/file", nil)
executed := false executed := false
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { st.ServeExisting("/", CacheDisabled, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
executed = (r == httpRequest) executed = (r == httpRequest)
})).ServeHTTP(nil, httpRequest) })).ServeHTTP(nil, httpRequest)
...@@ -76,7 +76,7 @@ func TestServingTheActualFile(t *testing.T) { ...@@ -76,7 +76,7 @@ func TestServingTheActualFile(t *testing.T) {
ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600) ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600)
w := httptest.NewRecorder() w := httptest.NewRecorder()
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 200, w.Code) require.Equal(t, 200, w.Code)
if w.Body.String() != fileContent { if w.Body.String() != fileContent {
...@@ -84,6 +84,40 @@ func TestServingTheActualFile(t *testing.T) { ...@@ -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) { func testServingThePregzippedFile(t *testing.T, enableGzip bool) {
dir, err := ioutil.TempDir("", "deploy") dir, err := ioutil.TempDir("", "deploy")
if err != nil { if err != nil {
...@@ -108,7 +142,7 @@ func testServingThePregzippedFile(t *testing.T, enableGzip bool) { ...@@ -108,7 +142,7 @@ func testServingThePregzippedFile(t *testing.T, enableGzip bool) {
ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600) ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600)
w := httptest.NewRecorder() w := httptest.NewRecorder()
st := &Static{dir} st := &Static{DocumentRoot: dir}
st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest) st.ServeExisting("/", CacheDisabled, nil).ServeHTTP(w, httpRequest)
require.Equal(t, 200, w.Code) require.Equal(t, 200, w.Code)
if enableGzip { if enableGzip {
......
...@@ -2,4 +2,5 @@ package staticpages ...@@ -2,4 +2,5 @@ package staticpages
type Static struct { type Static struct {
DocumentRoot string DocumentRoot string
Exclude []string
} }
contents1
\ No newline at end of file
contents2
\ No newline at end of file
...@@ -62,6 +62,14 @@ const ( ...@@ -62,6 +62,14 @@ const (
importPattern = `^/import/` 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 { func compileRegexp(regexpStr string) *regexp.Regexp {
if len(regexpStr) == 0 { if len(regexpStr) == 0 {
return nil return nil
...@@ -181,20 +189,20 @@ func buildProxy(backend *url.URL, version string, rt http.RoundTripper, cfg conf ...@@ -181,20 +189,20 @@ func buildProxy(backend *url.URL, version string, rt http.RoundTripper, cfg conf
// We match against URI not containing the relativeUrlRoot: // We match against URI not containing the relativeUrlRoot:
// see upstream.ServeHTTP // see upstream.ServeHTTP
func (u *upstream) configureRoutes() { func configureRoutes(u *upstream) {
api := apipkg.NewAPI( api := apipkg.NewAPI(
u.Backend, u.Backend,
u.Version, u.Version,
u.RoundTripper, 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) proxy := buildProxy(u.Backend, u.Version, u.RoundTripper, u.Config)
cableProxy := proxypkg.NewProxy(u.CableBackend, u.Version, u.CableRoundTripper) cableProxy := proxypkg.NewProxy(u.CableBackend, u.Version, u.CableRoundTripper)
assetsNotFoundHandler := NotFoundUnless(u.DevelopmentMode, proxy) assetsNotFoundHandler := NotFoundUnless(u.DevelopmentMode, proxy)
if u.AltDocumentRoot != "" { if u.AltDocumentRoot != "" {
altStatic := &staticpages.Static{DocumentRoot: u.AltDocumentRoot} altStatic := &staticpages.Static{DocumentRoot: u.AltDocumentRoot, Exclude: staticExclude}
assetsNotFoundHandler = altStatic.ServeExisting( assetsNotFoundHandler = altStatic.ServeExisting(
u.URLPrefix, u.URLPrefix,
staticpages.CacheExpireMax, staticpages.CacheExpireMax,
...@@ -306,12 +314,6 @@ func (u *upstream) configureRoutes() { ...@@ -306,12 +314,6 @@ func (u *upstream) configureRoutes() {
u.route("POST", snippetUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)), u.route("POST", snippetUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)),
u.route("POST", userUploadPattern, 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 // health checks don't intercept errors and go straight to rails
// TODO: We should probably not return a HTML deploy page? // TODO: We should probably not return a HTML deploy page?
// https://gitlab.com/gitlab-org/gitlab-workhorse/issues/230 // https://gitlab.com/gitlab-org/gitlab-workhorse/issues/230
......
...@@ -40,6 +40,10 @@ type upstream struct { ...@@ -40,6 +40,10 @@ type upstream struct {
} }
func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler { 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{ up := upstream{
Config: cfg, Config: cfg,
accessLogger: accessLogger, accessLogger: accessLogger,
...@@ -56,7 +60,7 @@ func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler { ...@@ -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.RoundTripper = roundtripper.NewBackendRoundTripper(up.Backend, up.Socket, up.ProxyHeadersTimeout, cfg.DevelopmentMode)
up.CableRoundTripper = roundtripper.NewBackendRoundTripper(up.CableBackend, up.CableSocket, up.ProxyHeadersTimeout, cfg.DevelopmentMode) up.CableRoundTripper = roundtripper.NewBackendRoundTripper(up.CableBackend, up.CableSocket, up.ProxyHeadersTimeout, cfg.DevelopmentMode)
up.configureURLPrefix() up.configureURLPrefix()
up.configureRoutes() routesCallback(&up)
var correlationOpts []correlation.InboundHandlerOption var correlationOpts []correlation.InboundHandlerOption
if cfg.PropagateCorrelationID { if cfg.PropagateCorrelationID {
...@@ -95,7 +99,7 @@ func (u *upstream) ServeHTTP(w http.ResponseWriter, r *http.Request) { ...@@ -95,7 +99,7 @@ func (u *upstream) ServeHTTP(w http.ResponseWriter, r *http.Request) {
} }
// Check URL Root // Check URL Root
URIPath := urlprefix.CleanURIPath(r.URL.Path) URIPath := urlprefix.CleanURIPath(r.URL.EscapedPath())
prefix := u.URLPrefix prefix := u.URLPrefix
if !prefix.Match(URIPath) { if !prefix.Match(URIPath) {
helper.HTTPError(w, r, fmt.Sprintf("Not found %q", URIPath), http.StatusNotFound) 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) { ...@@ -222,12 +222,15 @@ func TestDeniedPublicUploadsFile(t *testing.T) {
for _, resource := range []string{ for _, resource := range []string{
"/uploads/static.txt", "/uploads/static.txt",
"/uploads%2Fstatic.txt", "/uploads%2Fstatic.txt",
"/foobar%2F%2E%2E%2Fuploads/static.txt",
} { } {
resp, body := httpGet(t, ws.URL+resource, nil) 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, 404, resp.StatusCode, "GET %q: status code", resource)
require.Equal(t, "", body, "GET %q: response body", resource) require.Equal(t, "", body, "GET %q: response body", resource)
require.True(t, proxied, "GET %q: never made it to backend", resource) require.True(t, proxied, "GET %q: never made it to backend", resource)
})
} }
} }
......
...@@ -11415,10 +11415,10 @@ svg-tags@^1.0.0: ...@@ -11415,10 +11415,10 @@ svg-tags@^1.0.0:
resolved "https://registry.yarnpkg.com/svg-tags/-/svg-tags-1.0.0.tgz#58f71cee3bd519b59d4b2a843b6c7de64ac04764" resolved "https://registry.yarnpkg.com/svg-tags/-/svg-tags-1.0.0.tgz#58f71cee3bd519b59d4b2a843b6c7de64ac04764"
integrity sha1-WPcc7jvVGbWdSyqEO2x95krAR2Q= integrity sha1-WPcc7jvVGbWdSyqEO2x95krAR2Q=
swagger-ui-dist@^3.32.4: swagger-ui-dist@^3.43.0:
version "3.32.4" version "3.43.0"
resolved "https://registry.yarnpkg.com/swagger-ui-dist/-/swagger-ui-dist-3.32.4.tgz#6fa920a99e38eaaf129580ac158cf730494a2190" resolved "https://registry.yarnpkg.com/swagger-ui-dist/-/swagger-ui-dist-3.43.0.tgz#b064a2cec1d27776f9a124bc70423cfa0bbc0d3f"
integrity sha512-3qUqK131a5nqGdDJhLflTNzvrjZgjBlINYNx+Jm5lw/Va88Lcu5iyjUupY3Js/Kf326z1XtXkrr6TbvE6r925g== integrity sha512-PtE+g23bNbYv8qqAVoPBqNQth8hU5Sl5ZsQ7gHXlO5jlCt31dVTiKI9ArHIT1b23ZzUYTnKsFgPYYFoiWyNCAw==
symbol-observable@^1.0.2: symbol-observable@^1.0.2:
version "1.2.0" 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