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

CHANGELOG.md

@@ -2,6 +2,18 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.9.2 (2021-03-04)
+
+### Security (6 changes)
+
+- Bump thrift gem to 0.14.0.
+- Allow only owners to manage group variables.
+- Do not store marshalled sessions ids in Redis.
+- Fix XSS in wiki author email and name.
+- Workhorse: prevent escaped router path traversal.
+- Fix XSS vulnerability for swagger file viewer.
+
+
 ## 13.9.1 (2021-02-23)
 
 ### Fixed (6 changes, 1 of them is from the community)

GITALY_SERVER_VERSION

-13.9.1
+13.9.2
\ No newline at end of file

GITLAB_WORKHORSE_VERSION

-8.63.0
+8.63.2

Gemfile

@@ -310,6 +310,9 @@ gem 'premailer-rails', '~> 1.10.3'
 
 # LabKit: Tracing and Correlation
 gem 'gitlab-labkit', '0.14.0'
+# Thrift is a dependency of gitlab-labkit, we want a version higher than 0.14.0
+# because of https://gitlab.com/gitlab-org/gitlab/-/issues/321900
+gem 'thrift', '>= 0.14.0'
 
 # I18n
 gem 'ruby_parser', '~> 3.15', require: false

Gemfile.lock

@@ -1186,7 +1186,7 @@ GEM
       rack (>= 1, < 3)
     thor (1.1.0)
     thread_safe (0.3.6)
-    thrift (0.13.0)
+    thrift (0.14.0)
     tilt (2.0.10)
     timecop (0.9.1)
     timeliness (0.3.10)
@@ -1535,6 +1535,7 @@ DEPENDENCIES
   terser (= 1.0.2)
   test-prof (~> 0.12.0)
   thin (~> 1.8.0)
+  thrift (>= 0.14.0)
   timecop (~> 0.9.1)
   toml-rb (~> 1.0.0)
   truncato (~> 0.7.11)

VERSION

-13.9.1
+13.9.2
\ No newline at end of file

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
 

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|

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
   end
 end

app/helpers/wiki_page_version_helper.rb

+# 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

app/models/active_session.rb

@@ -42,13 +42,6 @@ class ActiveSession
     device_type&.titleize
   end
 
-  # This is not the same as Rack::Session::SessionId#public_id, but we
-  # need to preserve this for backwards compatibility.
-  # TODO: remove in 13.7
-  def public_id
-    Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
-  end
-
   def self.set(user, request)
     Gitlab::Redis::SharedState.with do |redis|
       session_private_id = request.session.id.private_id
@@ -63,8 +56,6 @@ class ActiveSession
         device_type: client.device_type,
         created_at: user.current_sign_in_at || timestamp,
         updated_at: timestamp,
-        # TODO: remove in 13.7
-        session_id: request.session.id.public_id,
         session_private_id: session_private_id,
         is_impersonated: request.session[:impersonator_id].present?
       )
@@ -80,20 +71,10 @@ class ActiveSession
           lookup_key_name(user.id),
           session_private_id
         )
-
-        # We remove the ActiveSession stored by using public_id to avoid
-        # duplicate entries
-        remove_deprecated_active_sessions_with_public_id(redis, user.id, request.session.id.public_id)
       end
     end
   end
 
-  # TODO: remove in 13.7
-  private_class_method def self.remove_deprecated_active_sessions_with_public_id(redis, user_id, rack_session_public_id)
-    redis.srem(lookup_key_name(user_id), rack_session_public_id)
-    redis.del(key_name(user_id, rack_session_public_id))
-  end
-
   def self.list(user)
     Gitlab::Redis::SharedState.with do |redis|
       cleaned_up_lookup_entries(redis, user).map do |raw_session|
@@ -109,18 +90,6 @@ class ActiveSession
     end
   end
 
-  # TODO: remove in 13.7
-  # After upgrade there might be a duplicate ActiveSessions:
-  # - one with the public_id stored in #session_id
-  # - another with private_id stored in #session_private_id
-  def self.destroy_with_rack_session_id(user, rack_session_id)
-    return unless rack_session_id
-
-    Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, [rack_session_id.public_id, rack_session_id.private_id])
-    end
-  end
-
   def self.destroy_sessions(redis, user, session_ids)
     key_names = session_ids.map { |session_id| key_name(user.id, session_id) }
 
@@ -132,19 +101,11 @@ class ActiveSession
     end
   end
 
-  # TODO: remove in 13.7
-  # After upgrade, .destroy might be called with the session id encrypted
-  # by .public_id.
-  def self.destroy_with_deprecated_encryption(user, session_id)
+  def self.destroy_session(user, session_id)
     return unless session_id
 
-    decrypted_session_id = decrypt_public_id(session_id)
-    rack_session_private_id = if decrypted_session_id
-                                Rack::Session::SessionId.new(decrypted_session_id).private_id
-                              end
-
     Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact)
+      destroy_sessions(redis, user, [session_id].compact)
     end
   end
 
@@ -275,11 +236,4 @@ class ActiveSession
 
     entries.compact
   end
-
-  # TODO: remove in 13.7
-  private_class_method def self.decrypt_public_id(public_id)
-    Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id)
-  rescue
-    nil
-  end
 end

app/views/shared/wikis/show.html.haml

@@ -7,7 +7,7 @@
   .nav-text.flex-fill
     %span.wiki-last-edit-by
       - if @page.last_version
-        = html_escape(_("Last edited by %{link_start}%{avatar} %{name}%{link_end}")) % { avatar: image_tag(avatar_icon_for_email(@page.last_version.author_email, 24), class: "avatar s24 float-none gl-mr-0!"), name: "<strong>#{@page.last_version.author_name}</strong>".html_safe, link_start: "<a href='#{@page.last_version.author_url}'>".html_safe, link_end: '</a>'.html_safe }
+        = wiki_page_version_author_header(@page.last_version)
         = time_ago_with_tooltip(@page.last_version.authored_date)
 
   .nav-controls.pb-md-3.pb-lg-0

config/initializers/warden.rb

@@ -42,8 +42,7 @@ Rails.application.configure do |config|
     activity = Gitlab::Auth::Activity.new(opts)
     tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth)
 
-    # TODO: switch to `auth.request.session.id.private_id` in 13.7
-    ActiveSession.destroy_with_rack_session_id(user, auth.request.session.id)
+    ActiveSession.destroy_session(user, auth.request.session.id.private_id) if auth.request.session.id
     activity.user_session_destroyed!
 
     ##

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

lib/gitlab/git/wiki_page_version.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module Git
     class WikiPageVersion
+      include Gitlab::Utils::StrongMemoize
+
       attr_reader :commit, :format
 
       def initialize(commit, format)
@@ -12,9 +14,10 @@ module Gitlab
 
       delegate :message, :sha, :id, :author_name, :author_email, :authored_date, to: :commit
 
-      def author_url
-        user = ::User.find_by_any_email(author_email)
-        user.nil? ? "mailto:#{author_email}" : Gitlab::UrlBuilder.build(user)
+      def author
+        strong_memoize(:author) do
+          ::User.find_by_any_email(author_email)
+        end
       end
     end
   end

package.json

@@ -126,7 +126,7 @@
     "sql.js": "^0.4.0",
     "string-hash": "1.1.3",
     "style-loader": "^1.3.0",
-    "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",

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: {

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 }

spec/helpers/wiki_page_version_helper_spec.rb

+# 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

spec/lib/gitlab/git/wiki_page_version_spec.rb

@@ -4,24 +4,24 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Git::WikiPageVersion do
   let_it_be(:project) { create(:project, :public, :repository) }
-  let(:user) { create(:user, username: 'someone') }
+  let_it_be(:user) { create(:user, username: 'someone') }
 
-  describe '#author_url' do
-    subject(:author_url) { described_class.new(commit, nil).author_url }
+  describe '#author' do
+    subject(:author) { described_class.new(commit, nil).author }
 
     context 'user exists in gitlab' do
       let(:commit) { create(:commit, project: project, author: user) }
 
-      it 'returns the profile link of the user' do
-        expect(author_url).to eq('http://localhost/someone')
+      it 'returns the user' do
+        expect(author).to eq user
       end
     end
 
     context 'user does not exist in gitlab' do
       let(:commit) { create(:commit, project: project, author_email: "someone@somewebsite.com") }
 
-      it 'returns a mailto: url' do
-        expect(author_url).to eq('mailto:someone@somewebsite.com')
+      it 'returns nil' do
+        expect(author).to be_nil
       end
     end
   end

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, [encrypted_active_session_id, rack_session.public_id, rack_session.private_id]))
-
-        subject
-      end
-
-      context 'ActiveSession with session_id (deprecated)' do
-        include_examples 'removes all session data'
-      end
-    end
   end
 
   describe '.destroy_all_but_current' do

spec/requests/api/group_variables_spec.rb

@@ -3,17 +3,20 @@
 require 'spec_helper'
 
 RSpec.describe API::GroupVariables do
-  let(:group) { create(:group) }
-  let(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:variable) { create(:ci_group_variable, group: group) }
 
-  describe 'GET /groups/:id/variables' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
+  let(:access_level) {}
 
-    context 'authorized user with proper permissions' do
   before do
-        group.add_maintainer(user)
+    group.add_user(user, access_level) if access_level
   end
 
+  describe 'GET /groups/:id/variables' do
+    context 'authorized user with proper permissions' do
+      let(:access_level) { :owner }
+
       it 'returns group variables' do
         get api("/groups/#{group.id}/variables", user)
 
@@ -23,6 +26,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not return group variables' do
         get api("/groups/#{group.id}/variables", user)
 
@@ -40,12 +45,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'GET /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'returns group variable details' do
         get api("/groups/#{group.id}/variables/#{variable.key}", user)
@@ -64,6 +65,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not return group variable details' do
         get api("/groups/#{group.id}/variables/#{variable.key}", user)
 
@@ -82,11 +85,7 @@ RSpec.describe API::GroupVariables do
 
   describe 'POST /groups/:id/variables' do
     context 'authorized user with proper permissions' do
-      let!(:variable) { create(:ci_group_variable, group: group) }
-
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'creates variable' do
         expect do
@@ -124,6 +123,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not create variable' do
         post api("/groups/#{group.id}/variables", user)
 
@@ -141,12 +142,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'PUT /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'updates variable data' do
         initial_variable = group.variables.reload.first
@@ -180,6 +177,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not update variable' do
         put api("/groups/#{group.id}/variables/#{variable.key}", user)
 
@@ -197,12 +196,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'DELETE /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'deletes variable' do
         expect do
@@ -224,6 +219,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not delete variable' do
         delete api("/groups/#{group.id}/variables/#{variable.key}", user)
 

workhorse/CHANGELOG

 # Changelog for gitlab-workhorse
 
+## v8.63.2
+
+### Security
+- Stop logging when path is excluded
+  https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
+
+## v8.63.1
+
+### Security
+- Use URL.EscapePath() in upstream router
+  https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
+
 ## v8.63.0
 
 ### Added

workhorse/VERSION

-8.63.0
+8.63.2

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)

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()
 

workhorse/internal/staticpages/servefile.go

 package staticpages
 
 import (
+	"errors"
+	"fmt"
 	"net/http"
 	"os"
 	"path/filepath"
 	"strings"
 	"time"
 
-	"gitlab.com/gitlab-org/labkit/log"
 	"gitlab.com/gitlab-org/labkit/mask"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/log"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix"
 )
 
@@ -26,21 +28,28 @@ const (
 // upstream.
 func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoundHandler http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		file := filepath.Join(s.DocumentRoot, prefix.Strip(r.URL.Path))
+		if notFoundHandler == nil {
+			notFoundHandler = http.HandlerFunc(http.NotFound)
+		}
+
+		// We intentionally use r.URL.Path instead of r.URL.EscaptedPath() below.
+		// This is to make it possible to serve static files with e.g. a space
+		// %20 in their name.
+		relativePath, err := s.validatePath(prefix.Strip(r.URL.Path))
+		if err != nil {
+			notFoundHandler.ServeHTTP(w, r)
+			return
+		}
 
-		// The filepath.Join does Clean traversing directories up
+		file := filepath.Join(s.DocumentRoot, relativePath)
 		if !strings.HasPrefix(file, s.DocumentRoot) {
-			helper.Fail500(w, r, &os.PathError{
-				Op:   "open",
-				Path: file,
-				Err:  os.ErrInvalid,
-			})
+			log.WithRequest(r).WithError(errPathTraversal).Error()
+			notFoundHandler.ServeHTTP(w, r)
 			return
 		}
 
 		var content *os.File
 		var fi os.FileInfo
-		var err error
 
 		// Serve pre-gzipped assets
 		if acceptEncoding := r.Header.Get("Accept-Encoding"); strings.Contains(acceptEncoding, "gzip") {
@@ -55,11 +64,7 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun
 			content, fi, err = helper.OpenFile(file)
 		}
 		if err != nil {
-			if notFoundHandler != nil {
 			notFoundHandler.ServeHTTP(w, r)
-			} else {
-				http.NotFound(w, r)
-			}
 			return
 		}
 		defer content.Close()
@@ -82,3 +87,17 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun
 		http.ServeContent(w, r, filepath.Base(file), fi.ModTime(), content)
 	})
 }
+
+var errPathTraversal = errors.New("path traversal")
+
+func (s *Static) validatePath(filename string) (string, error) {
+	filename = filepath.Clean(filename)
+
+	for _, exc := range s.Exclude {
+		if strings.HasPrefix(filename, exc) {
+			return "", fmt.Errorf("file is excluded: %s", exc)
+		}
+	}
+
+	return filename, nil
+}

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 {

workhorse/internal/staticpages/static.go

@@ -2,4 +2,5 @@ package staticpages
 
 type Static struct {
 	DocumentRoot string
+	Exclude      []string
 }

workhorse/internal/staticpages/testdata/file1

+contents1
\ No newline at end of file

workhorse/internal/staticpages/testdata/uploads/file2

+contents2
\ No newline at end of file

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,
@@ -306,12 +314,6 @@ func (u *upstream) configureRoutes() {
 		u.route("POST", snippetUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)),
 		u.route("POST", userUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)),
 
-		// For legacy reasons, user uploads are stored under the document root.
-		// To prevent anybody who knows/guesses the URL of a user-uploaded file
-		// from downloading it we make sure requests to /uploads/ do _not_ pass
-		// through static.ServeExisting.
-		u.route("", `^/uploads/`, static.ErrorPagesUnless(u.DevelopmentMode, staticpages.ErrorFormatHTML, proxy)),
-
 		// health checks don't intercept errors and go straight to rails
 		// TODO: We should probably not return a HTML deploy page?
 		//       https://gitlab.com/gitlab-org/gitlab-workhorse/issues/230

workhorse/internal/upstream/upstream.go

@@ -41,6 +41,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,
@@ -57,7 +61,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 {
@@ -96,7 +100,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)

workhorse/internal/upstream/upstream_test.go

+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))
+		})
+	}
+}

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",
 	} {
+		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)
+		})
 	}
 }
 

yarn.lock

@@ -11421,10 +11421,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"