Merge remote-tracking branch 'dev/13-7-stable' into 13-7-stable

31 files changed, 275 insertions, 267 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9b5728c53a8..5de844c88a6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,17 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 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)
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 097a182f11e..9255caa9bd7 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-13.7.7
\ No newline at end of file
+13.7.8
\ No newline at end of file
diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION
index 8aa8b5f68bd..f0ccef3be90 100644
--- a/GITLAB_WORKHORSE_VERSION
+++ b/GITLAB_WORKHORSE_VERSION
@@ -1 +1 @@
-8.58.2
+8.58.4
diff --git a/Gemfile b/Gemfile
index 49d0841be3c..123cf0422ab 100644
--- a/Gemfile
+++ b/Gemfile
@@ -312,6 +312,9 @@ gem 'premailer-rails', '~> 1.10.3'
 
 # LabKit: Tracing and Correlation
 gem 'gitlab-labkit', '0.13.3'
+# 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
diff --git a/Gemfile.lock b/Gemfile.lock
index 1cd90080fd8..a54c8117fff 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -1169,7 +1169,7 @@ GEM
       rack (>= 1, < 3)
     thor (0.20.3)
     thread_safe (0.3.6)
-    thrift (0.13.0)
+    thrift (0.14.0)
     tilt (2.0.10)
     timecop (0.9.1)
     timeliness (0.3.10)
@@ -1516,6 +1516,7 @@ DEPENDENCIES
   terser (= 1.0.2)
   test-prof (~> 0.12.0)
   thin (~> 1.7.0)
+  thrift (>= 0.14.0)
   timecop (~> 0.9.1)
   toml-rb (~> 1.0.0)
   truncato (~> 0.7.11)
diff --git a/VERSION b/VERSION
index 097a182f11e..9255caa9bd7 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-13.7.7
\ No newline at end of file
+13.7.8
\ No newline at end of file
diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb
index 51670325ce3..a2289b540ec 100644
--- a/app/controllers/groups/variables_controller.rb
+++ b/app/controllers/groups/variables_controller.rb
@@ -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
 
diff --git a/app/controllers/profiles/active_sessions_controller.rb b/app/controllers/profiles/active_sessions_controller.rb
index 1233c906406..aafd7c2b65b 100644
--- a/app/controllers/profiles/active_sessions_controller.rb
+++ b/app/controllers/profiles/active_sessions_controller.rb
@@ -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|
diff --git a/app/helpers/active_sessions_helper.rb b/app/helpers/active_sessions_helper.rb
index 322c5b3b16d..cfe0b747e78 100644
--- a/app/helpers/active_sessions_helper.rb
+++ b/app/helpers/active_sessions_helper.rb
@@ -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
+    profile_active_session_path(active_session.session_private_id)
   end
 end
diff --git a/app/models/active_session.rb b/app/models/active_session.rb
index dded0eb1dc3..88a68bd27ac 100644
--- a/app/models/active_session.rb
+++ b/app/models/active_session.rb
@@ -23,13 +23,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
@@ -44,8 +37,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?
       )
@@ -61,20 +52,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|
@@ -90,18 +71,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) }
 
@@ -113,19 +82,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
 
@@ -252,11 +213,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
diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb
index 3cfab52efd1..e0a6dc79852 100644
--- a/config/initializers/warden.rb
+++ b/config/initializers/warden.rb
@@ -40,8 +40,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!
 
     ##
diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb
index 0c40db02eb5..09744fbeda2 100644
--- a/lib/api/group_variables.rb
+++ b/lib/api/group_variables.rb
@@ -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
diff --git a/package.json b/package.json
index 14c3fd8533e..996356b6a1a 100644
--- a/package.json
+++ b/package.json
@@ -132,7 +132,7 @@
     "stickyfilljs": "^2.1.0",
     "string-hash": "1.1.3",
     "style-loader": "^1.1.3",
-    "swagger-ui-dist": "^3.32.4",
+    "swagger-ui-dist": "^3.43.0",
     "three": "^0.84.0",
     "three-orbit-controls": "^82.1.0",
     "three-stl-loader": "^1.0.4",
diff --git a/spec/controllers/groups/variables_controller_spec.rb b/spec/controllers/groups/variables_controller_spec.rb
index e2a14165cb4..a450a4afb02 100644
--- a/spec/controllers/groups/variables_controller_spec.rb
+++ b/spec/controllers/groups/variables_controller_spec.rb
@@ -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: {
diff --git a/spec/controllers/profiles/active_sessions_controller_spec.rb b/spec/controllers/profiles/active_sessions_controller_spec.rb
index f54f69d853d..12cf4f982e9 100644
--- a/spec/controllers/profiles/active_sessions_controller_spec.rb
+++ b/spec/controllers/profiles/active_sessions_controller_spec.rb
@@ -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 }
diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb
index f0bae3f29c0..2fd7b127500 100644
--- a/spec/models/active_session_spec.rb
+++ b/spec/models/active_session_spec.rb
@@ -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, [active_session.public_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
diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb
index 41b013f49ee..0b6bf65ca44 100644
--- a/spec/requests/api/group_variables_spec.rb
+++ b/spec/requests/api/group_variables_spec.rb
@@ -3,16 +3,19 @@
 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) {}
+
+  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
-      before do
-        group.add_maintainer(user)
-      end
+      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)
 
diff --git a/workhorse/CHANGELOG b/workhorse/CHANGELOG
index 271928845c1..b1b404f4196 100644
--- a/workhorse/CHANGELOG
+++ b/workhorse/CHANGELOG
@@ -1,5 +1,17 @@
 # Changelog for gitlab-workhorse
 
+## v8.58.4
+
+### Security
+- Stop logging when path is excluded
+  https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
+
+## v8.58.3
+
+### Security
+- Use URL.EscapePath() in upstream router
+  https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
+
 ## v8.58.2
 
 ### Security
diff --git a/workhorse/VERSION b/workhorse/VERSION
index 8aa8b5f68bd..f0ccef3be90 100644
--- a/workhorse/VERSION
+++ b/workhorse/VERSION
@@ -1 +1 @@
-8.58.2
+8.58.4
diff --git a/workhorse/internal/staticpages/deploy_page_test.go b/workhorse/internal/staticpages/deploy_page_test.go
index 4b081e73a97..9bc0a082144 100644
--- a/workhorse/internal/staticpages/deploy_page_test.go
+++ b/workhorse/internal/staticpages/deploy_page_test.go
@@ -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)
diff --git a/workhorse/internal/staticpages/error_pages_test.go b/workhorse/internal/staticpages/error_pages_test.go
index 05ec06cd429..ab29e187c8e 100644
--- a/workhorse/internal/staticpages/error_pages_test.go
+++ b/workhorse/internal/staticpages/error_pages_test.go
@@ -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()
 
diff --git a/workhorse/internal/staticpages/servefile.go b/workhorse/internal/staticpages/servefile.go
index c98bc030bc2..df3a556fc74 100644
--- a/workhorse/internal/staticpages/servefile.go
+++ b/workhorse/internal/staticpages/servefile.go
@@ -1,6 +1,8 @@
 package staticpages
 
 import (
+	"errors"
+	"fmt"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -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,
-			})
+			helper.LogError(r, errPathTraversal)
+			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)
-			}
+			notFoundHandler.ServeHTTP(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
+}
diff --git a/workhorse/internal/staticpages/servefile_test.go b/workhorse/internal/staticpages/servefile_test.go
index e136b876298..314547b8a57 100644
--- a/workhorse/internal/staticpages/servefile_test.go
+++ b/workhorse/internal/staticpages/servefile_test.go
@@ -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 {
diff --git a/workhorse/internal/staticpages/static.go b/workhorse/internal/staticpages/static.go
index b42351f15f5..5b804e4d644 100644
--- a/workhorse/internal/staticpages/static.go
+++ b/workhorse/internal/staticpages/static.go
@@ -2,4 +2,5 @@ package staticpages
 
 type Static struct {
 	DocumentRoot string
+	Exclude      []string
 }
diff --git a/workhorse/internal/staticpages/testdata/file1 b/workhorse/internal/staticpages/testdata/file1
new file mode 100644
index 00000000000..146edcbe0a3
--- /dev/null
+++ b/workhorse/internal/staticpages/testdata/file1
@@ -0,0 +1 @@
+contents1
\ No newline at end of file
diff --git a/workhorse/internal/staticpages/testdata/uploads/file2 b/workhorse/internal/staticpages/testdata/uploads/file2
new file mode 100644
index 00000000000..c061beb8592
--- /dev/null
+++ b/workhorse/internal/staticpages/testdata/uploads/file2
@@ -0,0 +1 @@
+contents2
\ No newline at end of file
diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go
index 5bbd245719b..3ccb7298b96 100644
--- a/workhorse/internal/upstream/routes.go
+++ b/workhorse/internal/upstream/routes.go
@@ -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,
@@ -303,12 +311,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
diff --git a/workhorse/internal/upstream/upstream.go b/workhorse/internal/upstream/upstream.go
index c81a21c0ecd..80e7d4056b6 100644
--- a/workhorse/internal/upstream/upstream.go
+++ b/workhorse/internal/upstream/upstream.go
@@ -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)
diff --git a/workhorse/internal/upstream/upstream_test.go b/workhorse/internal/upstream/upstream_test.go
new file mode 100644
index 00000000000..3afc62a7384
--- /dev/null
+++ b/workhorse/internal/upstream/upstream_test.go
@@ -0,0 +1,67 @@
+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))
+		})
+	}
+}
diff --git a/workhorse/main_test.go b/workhorse/main_test.go
index d15af1d3e4c..bcbb2f11369 100644
--- a/workhorse/main_test.go
+++ b/workhorse/main_test.go
@@ -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",
 	} {
-		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, "", body, "GET %q: response body", resource)
-		require.True(t, proxied, "GET %q: never made it to backend", resource)
+			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)
+		})
 	}
 }
 
diff --git a/yarn.lock b/yarn.lock
index 58e3a393196..e6229dfa025 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -11428,10 +11428,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"