Merge remote-tracking branch 'security/master'

app/controllers/oauth/authorized_applications_controller.rb

@@ -5,6 +5,13 @@ class Oauth::AuthorizedApplicationsController < Doorkeeper::AuthorizedApplicatio
 
   layout 'profile'
 
+  def index
+    respond_to do |format|
+      format.html { render "errors/not_found", layout: "errors", status: :not_found }
+      format.json { render json: "", status: :not_found }
+    end
+  end
+
   def destroy
     if params[:token_id].present?
       current_resource_owner.oauth_authorized_tokens.find(params[:token_id]).revoke

app/helpers/projects_helper.rb

@@ -626,6 +626,7 @@ module ProjectsHelper
 
   def find_file_path
     return unless @project && !@project.empty_repo?
+    return unless can?(current_user, :download_code, @project)
 
     ref = @ref || @project.repository.root_ref
 

app/serializers/remote_mirror_entity.rb

@@ -2,7 +2,7 @@
 
 class RemoteMirrorEntity < Grape::Entity
   expose :id
-  expose :url
+  expose :safe_url, as: :url
   expose :enabled
 
   expose :auth_method

changelogs/unreleased/bug-codeowner-diffs.yml

+---
+title: Ensure MR diff exists before codeowner check
+merge_request:
+author:
+type: security

changelogs/unreleased/security-apply-codeowners-checks-to-web_ui.yml

+---
+title: Apply CODEOWNERS validations to web requests
+merge_request:
+author:
+type: security

changelogs/unreleased/security-branch-permissions.yml

+---
+title: Prevent unauthorized access to default branch
+merge_request:
+author:
+type: security

changelogs/unreleased/security-file-template-project.yml

+---
+title: Do not return private project ID without permission
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fix-CVE-2020-10187.yml

+---
+title: Fix doorkeeper CVE-2020-10187
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fix-es-credentials-leak.yml

+---
+title: Prevent ES credentials leak
+merge_request:
+author:
+type: security

changelogs/unreleased/security-mask-gh-service-password.yml

+---
+title: Change GitHub service integration token input to password
+merge_request:
+author:
+type: security

changelogs/unreleased/security-mirror-urls.yml

+---
+title: Return only safe urls for mirrors
+merge_request:
+author:
+type: security

changelogs/unreleased/security-validate-use-propery-workhorse-rewritten-fields-for-multipart-up.yml

+---
+title: Validate workhorse 'rewritten_fields' and properly use them during multipart
+  uploads
+merge_request:
+author:
+type: security

config/application.rb

@@ -131,6 +131,7 @@ module Gitlab
       encrypted_key
       hook
       import_url
+      elasticsearch_url
       otp_attempt
       sentry_dsn
       trace

ee/app/models/packages/package.rb

@@ -30,7 +30,7 @@ class Packages::Package < ApplicationRecord
   validate :valid_conan_package_recipe, if: :conan?
   validate :valid_npm_package_name, if: :npm?
   validate :package_already_taken, if: :npm?
-  validates :version, format: { with: Gitlab::Regex.semver_regex }, if: :npm?
+  validates :version, format: { with: Gitlab::Regex.semver_regex }, if: -> { npm? || nuget? }
   validates :name, format: { with: Gitlab::Regex.conan_recipe_component_regex }, if: :conan?
   validates :version, format: { with: Gitlab::Regex.conan_recipe_component_regex }, if: :conan?
 

ee/app/models/project_services/github_service.rb

@@ -38,7 +38,7 @@ class GithubService < Service
 
   def fields
     [
-      { type: 'text',
+      { type: 'password',
         name: "token",
         required: true,
         placeholder: "e.g. 8d3f016698e...",

ee/app/serializers/ee/project_mirror_entity.rb

@@ -6,7 +6,7 @@ module EE
 
     prepended do
       expose :mirror
-      expose :import_url
+      expose :safe_import_url, as: :import_url
       expose :username_only_import_url
       expose :mirror_user_id
       expose :mirror_trigger_builds

ee/app/services/ee/audit_events/release_audit_event_service.rb

@@ -9,7 +9,6 @@ module EE
         @release = release
 
         super(author, entity, {
-          action: :custom,
           custom_message: message,
           ip_address: ip_address,
           target_id: release.id,

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -9,11 +9,12 @@ module EE
 
       override :refresh_merge_requests!
       def refresh_merge_requests!
-        update_approvers
-        reset_approvals_for_merge_requests(push.ref, push.newrev)
         check_merge_train_status
 
         super
+
+        update_approvers
+        reset_approvals_for_merge_requests(push.ref, push.newrev)
       end
 
       # Note: Closed merge requests also need approvals reset.

ee/app/services/packages/nuget/update_package_from_metadata_service.rb

@@ -15,16 +15,18 @@ module Packages
         raise InvalidMetadataError.new('package name and/or package version not found in metadata') unless valid_metadata?
 
         @package_file.transaction do
-          @package_file.update!(
-            file_name: package_filename,
-            file: @package_file.file
-          )
-
           if existing_package_id
             link_to_existing_package
           else
             update_linked_package
           end
+
+          # Updating file_name updates the path where the file is stored.
+          # We must pass the file again so that CarrierWave can handle the update
+          @package_file.update!(
+            file_name: package_filename,
+            file: @package_file.file
+          )
         end
       end
 
@@ -36,7 +38,12 @@ module Packages
 
       def link_to_existing_package
         package_to_destroy = @package_file.package
-        @package_file.update!(package_id: existing_package_id)
+        # Updating package_id updates the path where the file is stored.
+        # We must pass the file again so that CarrierWave can handle the update
+        @package_file.update!(
+          package_id: existing_package_id,
+          file: @package_file.file
+        )
         package_to_destroy.destroy!
       end
 

ee/changelogs/unreleased/security-admin-audit-log-dos.yml

+---
+title: Fix rendering failure of Audit Event generated by Releases API
+merge_request:
+author:
+type: security

ee/changelogs/unreleased/security-fix-path-traversal-in-nuget-package-repository.yml

+---
+title: Ensure that NuGet package versions are SemVer compliant
+merge_request:
+author:
+type: security

ee/changelogs/unreleased/security-validate-nuget-pkg-version-before-updating-uploaded-file.yml

+---
+title: Ensure that NuGet package versions are validated before updating the stored
+  file path
+merge_request:
+author:
+type: security

ee/lib/audit/details.rb

@@ -29,9 +29,9 @@ module Audit
     end
 
     def action_text
-      action = @details.slice(*ACTIONS)
+      action_name, action_info = @details.slice(*ACTIONS).first
 
-      case action.each_key.first
+      case action_name
       when :add
         "Added #{target_name}#{@details[:as] ? " as #{@details[:as]}" : ''}"
       when :remove
@@ -45,7 +45,7 @@ module Audit
 
         "Updated ref #{target_ref} from #{from_sha} to #{to_sha}"
       when :custom_message
-        detail_value
+        action_info
       else
         text_for_change(target_name)
       end

ee/lib/ee/api/entities/group.rb

@@ -14,7 +14,11 @@ module EE
 
           expose :checked_file_template_project_id,
                  as: :file_template_project_id,
-                 if: ->(group, options) { group.feature_available?(:custom_file_templates_for_namespace) }
+                 if: ->(group, options) {
+                       group.feature_available?(:custom_file_templates_for_namespace) &&
+                       Ability.allowed?(options[:current_user], :read_project, group.checked_file_template_project)
+                     }
+
           expose :marked_for_deletion_on, if: ->(group, _) { group.feature_available?(:adjourned_deletion_for_projects_and_groups) }
         end
       end

ee/lib/ee/gitlab/checks/diff_check.rb

@@ -12,7 +12,7 @@ module EE
         def path_validations
           validations = [super].flatten
 
-          if !updated_from_web? && project.branch_requires_code_owner_approval?(branch_name)
+          if project.branch_requires_code_owner_approval?(branch_name)
             validations << validate_code_owners
           end
 
@@ -34,11 +34,13 @@ module EE
           matched_rules = loader.entries.collect { |e| "- #{e.pattern}" }
           code_owner_path = project.repository.code_owners_blob(ref: branch_name).path || "CODEOWNERS"
 
-          "Pushes to protected branches that contain changes to files that\n" \
+          msg = "Pushes to protected branches that contain changes to files that\n" \
             "match patterns defined in `#{code_owner_path}` are disabled for\n" \
             "this project. Please submit these changes via a merge request.\n\n" \
             "The following pattern(s) from `#{code_owner_path}` were matched:\n" \
             "#{matched_rules.join('\n')}\n"
+
+          updated_from_web? ? msg.tr("\n", " ") : msg
         end
 
         def validate_path_locks?

ee/spec/features/admin/admin_audit_logs_spec.rb

@@ -36,6 +36,21 @@ describe 'Admin::AuditLogs', :js do
       expect(page).to have_link('Audit Log', href: admin_audit_logs_path)
     end
 
+    describe 'release created events' do
+      let(:project) { create(:project) }
+      let(:release) { create(:release, project: project, tag: 'v0.1', author: user) }
+
+      before do
+        EE::AuditEvents::ReleaseCreatedAuditEventService.new(user, project, '127.0.0.1', release).security_event
+      end
+
+      it 'shows the related audit event' do
+        visit admin_audit_logs_path
+
+        expect(page).to have_content('Created Release')
+      end
+    end
+
     describe 'user events' do
       before do
         AuditEventService.new(user, user, with: :ldap)

ee/spec/features/projects/services/user_activates_github_spec.rb

@@ -39,6 +39,10 @@ describe 'User activates GitHub Service' do
       expect(page).to have_content('GitHub activated.')
     end
 
+    it 'renders a token field of type `password` for masking input' do
+      expect(find('#service_token')['type']).to eq('password')
+    end
+
     context 'with pipelines', :js do
       let(:pipeline) { create(:ci_pipeline) }
       let(:project) { create(:project, ci_pipelines: [pipeline])}

ee/spec/lib/gitlab/checks/diff_check_spec.rb

@@ -89,8 +89,22 @@ describe Gitlab::Checks::DiffCheck do
             stub_feature_flags(sectional_codeowners: false)
           end
 
-          it "returns an error message" do
-            expect(validation_result).to include("Pushes to protected branches")
+          context "for a non-web-based request" do
+            it "returns an error message" do
+              expect(validation_result).to include("Pushes to protected branches")
+              expect(validation_result).to include("\n")
+            end
+          end
+
+          context "for a web-based request" do
+            before do
+              expect(subject).to receive(:updated_from_web?).and_return(true)
+            end
+
+            it "returns an error message with newline chars removed" do
+              expect(validation_result).to include("Pushes to protected branches")
+              expect(validation_result).not_to include("\n")
+            end
           end
         end
 
@@ -128,30 +142,16 @@ describe Gitlab::Checks::DiffCheck do
       end
 
       context "when the feature is enabled on the project" do
-        context "updated_from_web? == false" do
-          before do
-            expect(subject).to receive(:updated_from_web?).and_return(false)
-            expect(project).to receive(:branch_requires_code_owner_approval?)
-              .once.and_return(true)
-          end
-
-          it "returns an array of Proc(s)" do
-            validations = subject.send(:path_validations)
-
-            expect(validations.any?).to be_truthy
-            expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
-          end
+        before do
+          expect(project).to receive(:branch_requires_code_owner_approval?)
+            .once.and_return(true)
         end
 
-        context "updated_from_web? == true" do
-          before do
-            expect(subject).to receive(:updated_from_web?).and_return(true)
-            expect(project).not_to receive(:branch_requires_code_owner_approval?)
-          end
+        it "returns an array of Proc(s)" do
+          validations = subject.send(:path_validations)
 
-          it "returns an empty array" do
-            expect(subject.send(:path_validations)).to eq([])
-          end
+          expect(validations.any?).to be_truthy
+          expect(validations.any? { |v| !v.is_a? Proc }).to be_falsy
         end
       end
     end

ee/spec/models/packages/package_spec.rb

@@ -95,17 +95,19 @@ RSpec.describe Packages::Package, type: :model do
     end
 
     describe '#version' do
-      context 'npm package' do
-        subject { create(:npm_package) }
-
-        it { is_expected.to allow_value('1.2.3').for(:version) }
-        it { is_expected.to allow_value('1.2.3-beta').for(:version) }
-        it { is_expected.to allow_value('1.2.3-alpha.3').for(:version) }
-        it { is_expected.not_to allow_value('1').for(:version) }
-        it { is_expected.not_to allow_value('1.2').for(:version) }
-        it { is_expected.not_to allow_value('1./2.3').for(:version) }
-        it { is_expected.not_to allow_value('../../../../../1.2.3').for(:version) }
-        it { is_expected.not_to allow_value('%2e%2e%2f1.2.3').for(:version) }
+      RSpec.shared_examples 'validating version to be SemVer compliant for' do |factory_name|
+        context "for #{factory_name}" do
+          subject { create(factory_name) }
+
+          it { is_expected.to allow_value('1.2.3').for(:version) }
+          it { is_expected.to allow_value('1.2.3-beta').for(:version) }
+          it { is_expected.to allow_value('1.2.3-alpha.3').for(:version) }
+          it { is_expected.not_to allow_value('1').for(:version) }
+          it { is_expected.not_to allow_value('1.2').for(:version) }
+          it { is_expected.not_to allow_value('1./2.3').for(:version) }
+          it { is_expected.not_to allow_value('../../../../../1.2.3').for(:version) }
+          it { is_expected.not_to allow_value('%2e%2e%2f1.2.3').for(:version) }
+        end
       end
 
       context 'conan package' do
@@ -123,6 +125,9 @@ RSpec.describe Packages::Package, type: :model do
         it { is_expected.not_to allow_value('+1.2.3').for(:version) }
         it { is_expected.not_to allow_value('%2e%2e%2f1.2.3').for(:version) }
       end
+
+      it_behaves_like 'validating version to be SemVer compliant for', :npm_package
+      it_behaves_like 'validating version to be SemVer compliant for', :nuget_package
     end
 
     describe '#package_already_taken' do
@@ -218,10 +223,12 @@ RSpec.describe Packages::Package, type: :model do
     end
 
     describe '.has_version' do
-      let!(:package4) { create(:nuget_package, version: nil) }
-
       subject { described_class.has_version }
 
+      before do
+        create(:maven_metadatum).package.update!(version: nil)
+      end
+
       it 'includes only packages with version attribute' do
         is_expected.to match_array([package1, package2, package3])
       end

ee/spec/requests/api/groups_spec.rb

@@ -106,6 +106,41 @@ describe API::Groups do
         end
       end
     end
+
+    context 'file_template_project_id is a private project' do
+      let_it_be(:private_project) { create(:project, :private, group: group) }
+
+      before do
+        stub_licensed_features(custom_file_templates_for_namespace: true)
+        group.update_attribute(:file_template_project_id, private_project.id)
+      end
+
+      context 'user has permission to private project' do
+        it 'returns file_template_project_id' do
+          private_project.add_maintainer(user)
+
+          get api("/groups/#{group.id}", user)
+
+          expect(json_response).to have_key 'file_template_project_id'
+        end
+      end
+
+      context 'user does not have permission to private project' do
+        it 'does not return file_template_project_id' do
+          get api("/groups/#{group.id}", another_user)
+
+          expect(json_response).not_to have_key 'file_template_project_id'
+        end
+      end
+
+      context 'user is not logged in' do
+        it 'does not return file_template_project_id' do
+          get api("/groups/#{group.id}")
+
+          expect(json_response).not_to have_key 'file_template_project_id'
+        end
+      end
+    end
   end
 
   describe 'PUT /groups/:id' do

ee/spec/serializers/project_mirror_entity_spec.rb

@@ -18,7 +18,7 @@ describe ProjectMirrorEntity do
         is_expected.to eq(
           id: project.id,
           mirror: true,
-          import_url: project.import_url,
+          import_url: project.safe_import_url,
           username_only_import_url: project.username_only_import_url,
           mirror_user_id: project.mirror_user_id,
           mirror_trigger_builds: project.mirror_trigger_builds,
@@ -36,6 +36,10 @@ describe ProjectMirrorEntity do
           remote_mirrors_attributes: []
         )
       end
+
+      it 'excludes password information' do
+        expect(subject[:import_url]).not_to include('password')
+      end
     end
 
     context 'SSH public-key authentication' do

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -61,7 +61,7 @@ describe MergeRequests::RefreshService do
     end
 
     describe '#update_approvers' do
-      let(:owner) { create(:user) }
+      let(:owner) { create(:user, username: 'default-codeowner') }
       let(:current_user) { merge_request.author }
       let(:service) { described_class.new(project, current_user) }
       let(:enable_code_owner) { true }
@@ -86,6 +86,31 @@ describe MergeRequests::RefreshService do
         forked_merge_request
       end
 
+      it 'gets called in a specific order' do
+        allow_any_instance_of(MergeRequests::BaseService).to receive(:inspect).and_return(true)
+        expect(service).to receive(:reload_merge_requests).ordered
+        expect(service).to receive(:update_approvers).ordered
+        expect(service).to receive(:reset_approvals_for_merge_requests).ordered
+
+        subject
+      end
+
+      context 'when :sectional_codeowners is disabled' do
+        before do
+          stub_feature_flags(sectional_codeowners: false)
+        end
+
+        it 'creates an approval rule based on current diff' do
+          file = File.read(Rails.root.join('ee', 'spec', 'fixtures', 'codeowners_example'))
+          project.repository.create_file(owner, 'CODEOWNERS', file, { branch_name: 'test', message: 'codeowners' })
+
+          subject
+
+          expect(another_merge_request.approval_rules.size).to eq(3)
+          expect(another_merge_request.approval_rules.first.rule_type).to eq('code_owner')
+        end
+      end
+
       context 'when code owners disabled' do
         let(:enable_code_owner) { false }
 

ee/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb

@@ -66,5 +66,25 @@ describe Packages::Nuget::UpdatePackageFromMetadataService do
         expect { subject }.to raise_error(::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError)
       end
     end
+
+    context 'with an invalid package version' do
+      invalid_versions = [
+        '555',
+        '1.2',
+        '1./2.3',
+        '../../../../../1.2.3',
+        '%2e%2e%2f1.2.3'
+      ]
+
+      invalid_versions.each do |invalid_version|
+        it "raises an error for version #{invalid_version}" do
+          allow(service).to receive(:package_version).and_return(invalid_version)
+
+          expect { subject }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Version is invalid')
+          expect(package_file.file_name).not_to include(invalid_version)
+          expect(package_file.file.file.path).not_to include(invalid_version)
+        end
+      end
+    end
   end
 end

ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb

@@ -89,7 +89,6 @@ RSpec.shared_examples 'logs the release audit event' do
     expect(logger).to receive(:info).with(author_id: user.id,
                                           entity_id: entity.id,
                                           entity_type: entity_type,
-                                          action: :custom,
                                           ip_address: ip_address,
                                           custom_message: custom_message,
                                           target_details: target_details,
@@ -102,7 +101,6 @@ RSpec.shared_examples 'logs the release audit event' do
 
     expect(security_event.details).to eq(custom_message: custom_message,
                                          ip_address: ip_address,
-                                         action: :custom,
                                          target_details: target_details,
                                          target_id: target_id,
                                          target_type: target_type)

lib/gitlab/middleware/multipart.rb

@@ -43,11 +43,13 @@ module Gitlab
             raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
 
             key, value = parsed_field.first
-            if value.nil?
-              value = open_file(@request.params, key)
+            if value.nil? # we have a top level param, eg. field = 'foo' and not 'foo[bar]'
+              raise "invalid field: #{field.inspect}" if field != key
+
+              value = open_file(@request.params, key, tmp_path.presence)
               @open_files << value
             else
-              value = decorate_params_value(value, @request.params[key])
+              value = decorate_params_value(value, @request.params[key], tmp_path.presence)
             end
 
             update_param(key, value)
@@ -59,7 +61,7 @@ module Gitlab
         end
 
         # This function calls itself recursively
-        def decorate_params_value(path_hash, value_hash)
+        def decorate_params_value(path_hash, value_hash, path_override = nil)
           unless path_hash.is_a?(Hash) && path_hash.count == 1
             raise "invalid path: #{path_hash.inspect}"
           end
@@ -72,19 +74,19 @@ module Gitlab
 
           case path_value
           when nil
-            value_hash[path_key] = open_file(value_hash.dig(path_key), '')
+            value_hash[path_key] = open_file(value_hash.dig(path_key), '', path_override)
             @open_files << value_hash[path_key]
             value_hash
           when Hash
-            decorate_params_value(path_value, value_hash[path_key])
+            decorate_params_value(path_value, value_hash[path_key], path_override)
             value_hash
           else
             raise "unexpected path value: #{path_value.inspect}"
           end
         end
 
-        def open_file(params, key)
-          ::UploadedFile.from_params(params, key, allowed_paths)
+        def open_file(params, key, path_override = nil)
+          ::UploadedFile.from_params(params, key, allowed_paths, path_override)
         end
 
         # update_params ensures that both rails controllers and rack middleware can find

lib/uploaded_file.rb

@@ -42,13 +42,14 @@ class UploadedFile
     @remote_id = remote_id
   end
 
-  def self.from_params(params, field, upload_paths)
-    path = params["#{field}.path"]
+  def self.from_params(params, field, upload_paths, path_override = nil)
+    path = path_override || params["#{field}.path"]
     remote_id = params["#{field}.remote_id"]
     return if path.blank? && remote_id.blank?
 
-    file_path = nil
-    if path.present?
+    if remote_id.present? # don't use file_path if remote_id is set
+      file_path = nil
+    elsif path.present?
       file_path = File.realpath(path)
 
       paths = Array(upload_paths) << Dir.tmpdir

spec/controllers/oauth/authorized_applications_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Oauth::AuthorizedApplicationsController do
+  let(:user) { create(:user) }
+  let(:guest) { create(:user) }
+  let(:application) { create(:oauth_application, owner: guest) }
+
+  before do
+    sign_in(user)
+  end
+
+  describe 'GET #index' do
+    it 'responds with 404' do
+      get :index
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+end

spec/helpers/application_helper_spec.rb

@@ -280,11 +280,16 @@ describe ApplicationHelper do
     end
 
     context 'when @project is set' do
-      it 'includes all possible body data elements and associates the project elements with project' do
-        project = create(:project)
+      let_it_be(:project) { create(:project, :repository) }
+      let_it_be(:user) { create(:user) }
 
+      before do
         assign(:project, project)
+        allow(helper).to receive(:current_user).and_return(nil)
+      end
 
+      it 'includes all possible body data elements and associates the project elements with project' do
+        expect(helper).to receive(:can?).with(nil, :download_code, project)
         expect(helper.body_data).to eq(
           {
             page: 'application',
@@ -305,12 +310,11 @@ describe ApplicationHelper do
 
         context 'when params[:id] is present and the issue exsits and action_name is show' do
           it 'sets all project and id elements correctly related to the issue' do
-            issue = create(:issue)
+            issue = create(:issue, project: project)
             stub_controller_method(:action_name, 'show')
             stub_controller_method(:params, { id: issue.id })
 
-            assign(:project, issue.project)
-
+            expect(helper).to receive(:can?).with(nil, :download_code, project).and_return(false)
             expect(helper.body_data).to eq(
               {
                 page: 'projects:issues:show',
@@ -325,6 +329,15 @@ describe ApplicationHelper do
           end
         end
       end
+
+      context 'when current_user has download_code permission' do
+        it 'returns find_file with the default branch' do
+          allow(helper).to receive(:current_user).and_return(user)
+
+          expect(helper).to receive(:can?).with(user, :download_code, project).and_return(true)
+          expect(helper.body_data[:find_file]).to end_with(project.default_branch)
+        end
+      end
     end
 
     def stub_controller_method(method_name, value)

spec/lib/gitlab/middleware/multipart_spec.rb

@@ -7,11 +7,11 @@ require 'tempfile'
 describe Gitlab::Middleware::Multipart do
   include_context 'multipart middleware context'
 
-  shared_examples_for 'multipart upload files' do
+  RSpec.shared_examples_for 'multipart upload files' do
     it 'opens top-level files' do
       Tempfile.open('top-level') do |tempfile|
         rewritten = { 'file' => tempfile.path }
-        in_params = { 'file.name' => original_filename, 'file.path' => tempfile.path, 'file.remote_id' => remote_id }
+        in_params = { 'file.name' => original_filename, 'file.path' => file_path, 'file.remote_id' => remote_id, 'file.size' => file_size }
         env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
         expect_uploaded_file(tempfile, %w(file))
@@ -22,8 +22,8 @@ describe Gitlab::Middleware::Multipart do
 
     it 'opens files one level deep' do
       Tempfile.open('one-level') do |tempfile|
-        in_params = { 'user' => { 'avatar' => { '.name' => original_filename, '.path' => tempfile.path, '.remote_id' => remote_id } } }
         rewritten = { 'user[avatar]' => tempfile.path }
+        in_params = { 'user' => { 'avatar' => { '.name' => original_filename, '.path' => file_path, '.remote_id' => remote_id, '.size' => file_size } } }
         env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
         expect_uploaded_file(tempfile, %w(user avatar))
@@ -34,7 +34,7 @@ describe Gitlab::Middleware::Multipart do
 
     it 'opens files two levels deep' do
       Tempfile.open('two-levels') do |tempfile|
-        in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename, '.path' => tempfile.path, '.remote_id' => remote_id } } } }
+        in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename, '.path' => file_path, '.remote_id' => remote_id, '.size' => file_size } } } }
         rewritten = { 'project[milestone][themesong]' => tempfile.path }
         env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
@@ -44,13 +44,61 @@ describe Gitlab::Middleware::Multipart do
       end
     end
 
-    def expect_uploaded_file(tempfile, path, remote: false)
+    def expect_uploaded_file(tempfile, path)
       expect(app).to receive(:call) do |env|
         file = get_params(env).dig(*path)
         expect(file).to be_a(::UploadedFile)
-        expect(file.path).to eq(tempfile.path)
         expect(file.original_filename).to eq(original_filename)
-        expect(file.remote_id).to eq(remote_id)
+
+        if remote_id
+          expect(file.remote_id).to eq(remote_id)
+          expect(file.path).to be_nil
+        else
+          expect(file.path).to eq(File.realpath(tempfile.path))
+          expect(file.remote_id).to be_nil
+        end
+      end
+    end
+  end
+
+  RSpec.shared_examples_for 'handling CI artifact upload' do
+    it 'uploads both file and metadata' do
+      Tempfile.open('file') do |file|
+        Tempfile.open('metadata') do |metadata|
+          rewritten = { 'file' => file.path, 'metadata' => metadata.path }
+          in_params = { 'file.name' => 'file.txt', 'file.path' => file_path, 'file.remote_id' => file_remote_id, 'file.size' => file_size, 'metadata.name' => 'metadata.gz' }
+          env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+          with_expected_uploaded_artifact_files(file, metadata) do |uploaded_file, uploaded_metadata|
+            expect(uploaded_file).to be_a(::UploadedFile)
+            expect(uploaded_file.original_filename).to eq('file.txt')
+
+            if file_remote_id
+              expect(uploaded_file.remote_id).to eq(file_remote_id)
+              expect(uploaded_file.size).to eq(file_size)
+              expect(uploaded_file.path).to be_nil
+            else
+              expect(uploaded_file.path).to eq(File.realpath(file.path))
+              expect(uploaded_file.remote_id).to be_nil
+            end
+
+            expect(uploaded_metadata).to be_a(::UploadedFile)
+            expect(uploaded_metadata.original_filename).to eq('metadata.gz')
+            expect(uploaded_metadata.path).to eq(File.realpath(metadata.path))
+            expect(uploaded_metadata.remote_id).to be_nil
+          end
+
+          middleware.call(env)
+        end
+      end
+    end
+
+    def with_expected_uploaded_artifact_files(file, metadata)
+      expect(app).to receive(:call) do |env|
+        file = get_params(env).dig('file')
+        metadata = get_params(env).dig('metadata')
+
+        yield file, metadata
       end
     end
   end
@@ -67,18 +115,65 @@ describe Gitlab::Middleware::Multipart do
     expect { middleware.call(env) }.to raise_error(JWT::InvalidIssuerError)
   end
 
+  context 'with invalid rewritten field' do
+    invalid_field_names = [
+      '[file]',
+      ';file',
+      'file]',
+      ';file]',
+      'file]]',
+      'file;;'
+    ]
+
+    invalid_field_names.each do |invalid_field_name|
+      it "rejects invalid rewritten field name #{invalid_field_name}" do
+        env = post_env({ invalid_field_name => nil }, {}, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+        expect { middleware.call(env) }.to raise_error(RuntimeError, "invalid field: \"#{invalid_field_name}\"")
+      end
+    end
+  end
+
   context 'with remote file' do
     let(:remote_id) { 'someid' }
+    let(:file_size) { 300 }
+    let(:file_path) { '' }
+
+    it_behaves_like 'multipart upload files'
+  end
+
+  context 'with remote file and a file path set' do
+    let(:remote_id) { 'someid' }
+    let(:file_size) { 300 }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
 
     it_behaves_like 'multipart upload files'
   end
 
   context 'with local file' do
     let(:remote_id) { nil }
+    let(:file_size) { nil }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
 
     it_behaves_like 'multipart upload files'
   end
 
+  context 'with remote CI artifact upload' do
+    let(:file_remote_id) { 'someid' }
+    let(:file_size) { 300 }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
+
+    it_behaves_like 'handling CI artifact upload'
+  end
+
+  context 'with local CI artifact upload' do
+    let(:file_remote_id) { nil }
+    let(:file_size) { nil }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
+
+    it_behaves_like 'handling CI artifact upload'
+  end
+
   it 'allows files in uploads/tmp directory' do
     with_tmp_dir('public/uploads/tmp') do |dir, env|
       expect(app).to receive(:call) do |env|

spec/lib/uploaded_file_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe UploadedFile do
   let(:temp_dir) { Dir.tmpdir }
-  let(:temp_file) { Tempfile.new("test", temp_dir) }
+  let(:temp_file) { Tempfile.new(%w[test test], temp_dir) }
 
   before do
     FileUtils.touch(temp_file)
@@ -16,13 +16,14 @@ describe UploadedFile do
 
   describe ".from_params" do
     let(:upload_path) { nil }
+    let(:file_path_override) { nil }
 
     after do
       FileUtils.rm_r(upload_path) if upload_path
     end
 
     subject do
-      described_class.from_params(params, :file, upload_path)
+      described_class.from_params(params, :file, upload_path, file_path_override)
     end
 
     context 'when valid file is specified' do
@@ -31,9 +32,7 @@ describe UploadedFile do
           { 'file.path' => temp_file.path }
         end
 
-        it "succeeds" do
-          is_expected.not_to be_nil
-        end
+        it { is_expected.not_to be_nil }
 
         it "generates filename from path" do
           expect(subject.original_filename).to eq(::File.basename(temp_file.path))
@@ -41,33 +40,153 @@ describe UploadedFile do
       end
 
       context 'all parameters are specified' do
-        let(:params) do
-          { 'file.path' => temp_file.path,
-            'file.name' => 'dir/my file&.txt',
-            'file.type' => 'my/type',
-            'file.sha256' => 'sha256',
-            'file.remote_id' => 'remote_id' }
+        RSpec.shared_context 'filepath override' do
+          let(:temp_file_override) { Tempfile.new(%w[override override], temp_dir) }
+          let(:file_path_override) { temp_file_override.path }
+
+          before do
+            FileUtils.touch(temp_file_override)
+          end
+
+          after do
+            FileUtils.rm_f(temp_file_override)
+          end
         end
 
-        it "succeeds" do
-          is_expected.not_to be_nil
+        RSpec.shared_examples 'using the file path' do |filename:, content_type:, sha256:, path_suffix:|
+          it 'sets properly the attributes' do
+            expect(subject.original_filename).to eq(filename)
+            expect(subject.content_type).to eq(content_type)
+            expect(subject.sha256).to eq(sha256)
+            expect(subject.remote_id).to be_nil
+            expect(subject.path).to end_with(path_suffix)
+          end
+
+          it 'handles a blank path' do
+            params['file.path'] = ''
+
+            # Not a real file, so can't determine size itself
+            params['file.size'] = 1.byte
+
+            expect { described_class.from_params(params, :file, upload_path) }
+              .not_to raise_error
+          end
         end
 
-        it "generates filename from path" do
-          expect(subject.original_filename).to eq('my_file_.txt')
-          expect(subject.content_type).to eq('my/type')
-          expect(subject.sha256).to eq('sha256')
-          expect(subject.remote_id).to eq('remote_id')
+        RSpec.shared_examples 'using the remote id' do |filename:, content_type:, sha256:, size:, remote_id:|
+          it 'sets properly the attributes' do
+            expect(subject.original_filename).to eq(filename)
+            expect(subject.content_type).to eq('application/octet-stream')
+            expect(subject.sha256).to eq('sha256')
+            expect(subject.path).to be_nil
+            expect(subject.size).to eq(123456)
+            expect(subject.remote_id).to eq('1234567890')
+          end
+        end
+
+        context 'with a filepath' do
+          let(:params) do
+            { 'file.path' => temp_file.path,
+              'file.name' => 'dir/my file&.txt',
+              'file.type' => 'my/type',
+              'file.sha256' => 'sha256' }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the file path',
+                          filename: 'my_file_.txt',
+                          content_type: 'my/type',
+                          sha256: 'sha256',
+                          path_suffix: 'test'
+        end
+
+        context 'with a filepath override' do
+          include_context 'filepath override'
+
+          let(:params) do
+            { 'file.path' => temp_file.path,
+              'file.name' => 'dir/my file&.txt',
+              'file.type' => 'my/type',
+              'file.sha256' => 'sha256' }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the file path',
+                          filename: 'my_file_.txt',
+                          content_type: 'my/type',
+                          sha256: 'sha256',
+                          path_suffix: 'override'
         end
 
-        it 'handles a blank path' do
-          params['file.path'] = ''
+        context 'with a remote id' do
+          let(:params) do
+            {
+              'file.name' => 'dir/my file&.txt',
+              'file.sha256' => 'sha256',
+              'file.remote_url' => 'http://localhost/file',
+              'file.remote_id' => '1234567890',
+              'file.etag' => 'etag1234567890',
+              'file.size' => '123456'
+            }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the remote id',
+                          filename: 'my_file_.txt',
+                          content_type: 'application/octet-stream',
+                          sha256: 'sha256',
+                          size: 123456,
+                          remote_id: '1234567890'
+        end
 
-          # Not a real file, so can't determine size itself
-          params['file.size'] = 1.byte
+        context 'with a path and a remote id' do
+          let(:params) do
+            {
+              'file.path' => temp_file.path,
+              'file.name' => 'dir/my file&.txt',
+              'file.sha256' => 'sha256',
+              'file.remote_url' => 'http://localhost/file',
+              'file.remote_id' => '1234567890',
+              'file.etag' => 'etag1234567890',
+              'file.size' => '123456'
+            }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the remote id',
+                          filename: 'my_file_.txt',
+                          content_type: 'application/octet-stream',
+                          sha256: 'sha256',
+                          size: 123456,
+                          remote_id: '1234567890'
+        end
 
-          expect { described_class.from_params(params, :file, upload_path) }
-            .not_to raise_error
+        context 'with a path override and a remote id' do
+          include_context 'filepath override'
+
+          let(:params) do
+            {
+              'file.name' => 'dir/my file&.txt',
+              'file.sha256' => 'sha256',
+              'file.remote_url' => 'http://localhost/file',
+              'file.remote_id' => '1234567890',
+              'file.etag' => 'etag1234567890',
+              'file.size' => '123456'
+            }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the remote id',
+                          filename: 'my_file_.txt',
+                          content_type: 'application/octet-stream',
+                          sha256: 'sha256',
+                          size: 123456,
+                          remote_id: '1234567890'
         end
       end
     end

spec/serializers/remote_mirror_entity_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe RemoteMirrorEntity do
-  let(:project) { create(:project, :repository, :remote_mirror) }
+  let(:project) { create(:project, :repository, :remote_mirror, url: "https://test:password@gitlab.com") }
   let(:remote_mirror) { project.remote_mirrors.first }
   let(:entity) { described_class.new(remote_mirror) }
 
@@ -15,4 +15,9 @@ describe RemoteMirrorEntity do
       :ssh_known_hosts, :ssh_public_key, :ssh_known_hosts_fingerprints
     )
   end
+
+  it 'does not expose password information' do
+    expect(subject[:url]).not_to include('password')
+    expect(subject[:url]).to eq(remote_mirror.safe_url)
+  end
 end