Merge commit 'dev/security' into 'master'

Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG

@@ -86,6 +86,14 @@ v 8.12.2
   - Fix resolve discussion buttons endpoint path
   - Refactor remnants of CoffeeScript destructured opts and super !6261
 
+v 8.12.4 (unreleased)
+  - Set GitLab project exported file permissions to owner only
+  - Don't send Private-Token (API authentication) headers to Sentry
+
+v 8.12.2 (unreleased)
+  - Fix Import/Export not recognising correctly the imported services.
+  - Respect the fork_project permission when forking projects
+
 v 8.12.1
   - Fix a memory leak in HTML::Pipeline::SanitizationFilter::WHITELIST
   - Fix issue with search filter labels not displaying

Gemfile

@@ -231,7 +231,7 @@ gem 'net-ssh',            '~> 3.0.1'
 gem 'base32',             '~> 0.3.0'
 
 # Sentry integration
-gem 'sentry-raven', '~> 1.1.0'
+gem 'sentry-raven', '~> 2.0.0'
 
 gem 'premailer-rails', '~> 1.9.0'
 

Gemfile.lock

@@ -665,8 +665,8 @@ GEM
       activesupport (>= 3.1)
     select2-rails (3.5.9.3)
       thor (~> 0.14)
-    sentry-raven (1.1.0)
-      faraday (>= 0.7.6)
+    sentry-raven (2.0.2)
+      faraday (>= 0.7.6, < 0.10.x)
     settingslogic (2.0.9)
     sexp_processor (4.7.0)
     sham_rack (1.3.6)
@@ -948,7 +948,7 @@ DEPENDENCIES
   sdoc (~> 0.3.20)
   seed-fu (~> 2.3.5)
   select2-rails (~> 3.5.9)
-  sentry-raven (~> 1.1.0)
+  sentry-raven (~> 2.0.0)
   settingslogic (~> 2.0.9)
   sham_rack (~> 1.3.6)
   shoulda-matchers (~> 2.8.0)

app/assets/javascripts/api.js

@@ -5,7 +5,7 @@
     namespacesPath: "/api/:version/namespaces.json",
     groupProjectsPath: "/api/:version/groups/:id/projects.json",
     projectsPath: "/api/:version/projects.json?simple=true",
-    labelsPath: "/api/:version/projects/:id/labels",
+    labelsPath: "/:namespace_path/:project_path/labels",
     licensePath: "/api/:version/licenses/:key",
     gitignorePath: "/api/:version/gitignores/:key",
     gitlabCiYmlPath: "/api/:version/gitlab_ci_ymls/:key",
@@ -66,13 +66,14 @@
         return callback(projects);
       });
     },
-    newLabel: function(project_id, data, callback) {
+    newLabel: function(namespace_path, project_path, data, callback) {
       var url = Api.buildUrl(Api.labelsPath)
-        .replace(':id', project_id);
+        .replace(':namespace_path', namespace_path)
+        .replace(':project_path', project_path);
       return $.ajax({
         url: url,
         type: "POST",
-        data: data,
+        data: {'label': data},
         dataType: "json"
       }).done(function(label) {
         return callback(label);

app/assets/javascripts/boards/components/new_list_dropdown.js.es6

@@ -3,8 +3,7 @@ $(() => {
 
   $('.js-new-board-list').each(function () {
     const $this = $(this);
-
-    new gl.CreateLabelDropdown($this.closest('.dropdown').find('.dropdown-new-label'), $this.data('project-id'));
+    new gl.CreateLabelDropdown($this.closest('.dropdown').find('.dropdown-new-label'), $this.data('namespace-path'), $this.data('project-path'));
 
     $this.glDropdown({
       data(term, callback) {

app/assets/javascripts/create_label.js.es6

@@ -1,8 +1,9 @@
 (function (w) {
   class CreateLabelDropdown {
-    constructor ($el, projectId) {
+    constructor ($el, namespacePath, projectPath) {
       this.$el = $el;
-      this.projectId = projectId;
+      this.namespacePath = namespacePath;
+      this.projectPath = projectPath;
       this.$dropdownBack = $('.dropdown-menu-back', this.$el.closest('.dropdown'));
       this.$cancelButton = $('.js-cancel-label-btn', this.$el);
       this.$newLabelField = $('#new_label_name', this.$el);
@@ -91,8 +92,8 @@
       e.preventDefault();
       e.stopPropagation();
 
-      Api.newLabel(this.projectId, {
-        name: this.$newLabelField.val(),
+      Api.newLabel(this.namespacePath, this.projectPath, {
+        title: this.$newLabelField.val(),
         color: this.$newColorField.val()
       }, (label) => {
         this.$newLabelCreateButton.enable();

app/assets/javascripts/labels_select.js

@@ -7,7 +7,8 @@
         var $block, $colorPreview, $dropdown, $form, $loading, $selectbox, $sidebarCollapsedValue, $value, abilityName, defaultLabel, enableLabelCreateButton, issueURLSplit, issueUpdateURL, labelHTMLTemplate, labelNoneHTMLTemplate, labelUrl, projectId, saveLabelData, selectedLabel, showAny, showNo, $sidebarLabelTooltip, initialSelected, $toggleText, fieldName, useId, propertyName, showMenuAbove;
         $dropdown = $(dropdown);
         $toggleText = $dropdown.find('.dropdown-toggle-text');
-        projectId = $dropdown.data('project-id');
+        namespacePath = $dropdown.data('namespace-path');
+        projectPath = $dropdown.data('project-path');
         labelUrl = $dropdown.data('labels');
         issueUpdateURL = $dropdown.data('issueUpdate');
         selectedLabel = $dropdown.data('selected');
@@ -45,7 +46,7 @@
         $sidebarLabelTooltip.tooltip();
 
         if ($dropdown.closest('.dropdown').find('.dropdown-new-label').length) {
-          new gl.CreateLabelDropdown($dropdown.closest('.dropdown').find('.dropdown-new-label'), projectId);
+          new gl.CreateLabelDropdown($dropdown.closest('.dropdown').find('.dropdown-new-label'), namespacePath, projectPath);
         }
 
         saveLabelData = function() {

app/controllers/projects/labels_controller.rb

@@ -30,9 +30,15 @@ class Projects::LabelsController < Projects::ApplicationController
     @label = @project.labels.create(label_params)
 
     if @label.valid?
-      redirect_to namespace_project_labels_path(@project.namespace, @project)
+      respond_to do |format|
+        format.html { redirect_to namespace_project_labels_path(@project.namespace, @project) }
+        format.json { render json: @label }
+      end
     else
-      render 'new'
+      respond_to do |format|
+        format.html { render 'new' }
+        format.json { render json: { message: @label.errors.messages }, status: 400 }
+      end
     end
   end
 

app/services/projects/create_service.rb

@@ -17,6 +17,11 @@ module Projects
         return @project
       end
 
+      unless allowed_fork?(forked_from_project_id)
+        @project.errors.add(:forked_from_project_id, 'is forbidden')
+        return @project
+      end
+
       # Set project name from path
       if @project.name.present? && @project.path.present?
         # if both name and path set - everything is ok
@@ -73,6 +78,13 @@ module Projects
       @project.errors.add(:namespace, "is not valid")
     end
 
+    def allowed_fork?(source_project_id)
+      return true if source_project_id.nil?
+
+      source_project = Project.find_by(id: source_project_id)
+      current_user.can?(:fork_project, source_project)
+    end
+
     def allowed_namespace?(user, namespace_id)
       namespace = Namespace.find_by(id: namespace_id)
       current_user.can?(:create_projects, namespace)

app/services/projects/fork_service.rb

@@ -16,6 +16,8 @@ module Projects
       end
 
       new_project = CreateService.new(current_user, new_params).execute
+      return new_project unless new_project.persisted?
+
       builds_access_level = @project.project_feature.builds_access_level
       new_project.project_feature.update_attributes(builds_access_level: builds_access_level)
 

app/views/ci/lints/_create.html.haml

@@ -16,8 +16,7 @@
             %tr
               %td #{stage.capitalize} Job - #{build[:name]}
               %td
-                %pre
-                  = simple_format build[:commands]
+                %pre= build[:commands]
 
                 %br
                 %b Tag list:

app/views/shared/issuable/_filter.html.haml

@@ -38,7 +38,7 @@
               %input.pull-left.form-control{ type: "search", placeholder: "Filter by name...", "v-model" => "filters.search", "debounce" => "250" }
               - if can?(current_user, :admin_list, @project)
                 .dropdown.pull-right
-                  %button.btn.btn-create.js-new-board-list{ type: "button", data: { toggle: "dropdown", labels: labels_filter_path, project_id: @project.try(:id) } }
+                  %button.btn.btn-create.js-new-board-list{ type: "button", data: { toggle: "dropdown", labels: labels_filter_path, namespace_path: @project.try(:namespace).try(:path), project_path: @project.try(:path) } }
                     Create new list
                   .dropdown-menu.dropdown-menu-paging.dropdown-menu-align-right.dropdown-menu-issues-board-new.dropdown-menu-selectable
                     = render partial: "shared/issuable/label_page_default", locals: { show_footer: true, show_create: true, show_boards_content: true, title: "Create a new list" }

app/views/shared/issuable/_label_dropdown.html.haml

@@ -8,7 +8,7 @@
 - classes = local_assigns.fetch(:classes, [])
 - selected = local_assigns.fetch(:selected, nil)
 - selected_toggle = local_assigns.fetch(:selected_toggle, nil)
-- dropdown_data = {toggle: 'dropdown', field_name: "label_name[]", show_no: "true", show_any: "true", project_id: project.try(:id), labels: labels_filter_path, default_label: "Labels"}
+- dropdown_data = {toggle: 'dropdown', field_name: "label_name[]", show_no: "true", show_any: "true", namespace_path: @project.try(:namespace).try(:path), project_path: @project.try(:path), labels: labels_filter_path, default_label: "Labels"}
 - dropdown_data.merge!(data_options)
 - classes << 'js-extra-options' if extra_options
 - classes << 'js-filter-submit' if filter_submit

app/views/shared/issuable/_sidebar.html.haml

@@ -129,7 +129,7 @@
             - selected_labels.each do |label|
               = hidden_field_tag "#{issuable.to_ability_name}[label_names][]", label.id, id: nil
             .dropdown
-              %button.dropdown-menu-toggle.js-label-select.js-multiselect.js-label-sidebar-dropdown{type: "button", data: {toggle: "dropdown", default_label: "Labels", field_name: "#{issuable.to_ability_name}[label_names][]", ability_name: issuable.to_ability_name, show_no: "true", show_any: "true", project_id: (@project.id if @project), issue_update: issuable_json_path(issuable), labels: (namespace_project_labels_path(@project.namespace, @project, :json) if @project)}}
+              %button.dropdown-menu-toggle.js-label-select.js-multiselect.js-label-sidebar-dropdown{type: "button", data: {toggle: "dropdown", default_label: "Labels", field_name: "#{issuable.to_ability_name}[label_names][]", ability_name: issuable.to_ability_name, show_no: "true", show_any: "true", namespace_path: @project.try(:namespace).try(:path), project_path: @project.try(:path), issue_update: issuable_json_path(issuable), labels: (namespace_project_labels_path(@project.namespace, @project, :json) if @project)}}
                 %span.dropdown-toggle-text{ class: ("is-default" if selected_labels.empty?)}
                   = multi_label_name(selected_labels, "Labels")
                 = icon('chevron-down')

config/application.rb

@@ -50,6 +50,7 @@ module Gitlab
     # - Build variables (:variables)
     # - GitLab Pages SSL cert/key info (:certificate, :encrypted_key)
     # - Webhook URLs (:hook)
+    # - GitLab-shell secret token (:secret_token)
     # - Sentry DSN (:sentry_dsn)
     # - Deploy keys (:key)
     config.filter_parameters += %i(
@@ -62,6 +63,7 @@ module Gitlab
       password
       password_confirmation
       private_token
+      secret_token
       sentry_dsn
       variables
     )

config/initializers/sentry.rb

@@ -18,6 +18,8 @@ if Rails.env.production?
       
       # Sanitize fields based on those sanitized from Rails.
       config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s)
+      # Sanitize authentication headers
+      config.sanitize_http_headers = %w[Authorization Private-Token]
       config.tags = { program: Gitlab::Sentry.program_context }
     end
   end

features/steps/project/fork.rb

@@ -70,6 +70,7 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps
 
   step 'There is an existent fork of the "Shop" project' do
     user = create(:user, name: 'Mike')
+    @project.team << [user, :reporter]
     @forked_project = Projects::ForkService.new(@project, user).execute
   end
 

lib/api/helpers.rb

@@ -21,8 +21,11 @@ module API
     end
 
     # Check the Rails session for valid authentication details
+    #
+    # Until CSRF protection is added to the API, disallow this method for
+    # state-changing endpoints
     def find_user_from_warden
-      warden ? warden.authenticate : nil
+      warden.try(:authenticate) if request.get? || request.head?
     end
 
     def find_user_by_private_token

lib/gitlab/import_export/command_line_util.rb

@@ -1,6 +1,8 @@
 module Gitlab
   module ImportExport
     module CommandLineUtil
+      DEFAULT_MODE = 0700
+
       def tar_czf(archive:, dir:)
         tar_with_options(archive: archive, dir: dir, options: 'czf')
       end
@@ -21,6 +23,11 @@ module Gitlab
         execute(%W(#{Gitlab.config.gitlab_shell.path}/bin/create-hooks) + repository_storage_paths_args)
       end
 
+      def mkdir_p(path)
+        FileUtils.mkdir_p(path, mode: DEFAULT_MODE)
+        FileUtils.chmod(DEFAULT_MODE, path)
+      end
+
       private
 
       def tar_with_options(archive:, dir:, options:)
@@ -45,7 +52,7 @@ module Gitlab
         # if we are copying files, create the destination folder
         destination_folder = File.file?(source) ? File.dirname(destination) : destination
 
-        FileUtils.mkdir_p(destination_folder)
+        mkdir_p(destination_folder)
         FileUtils.copy_entry(source, destination)
         true
       end

lib/gitlab/import_export/file_importer.rb

@@ -15,7 +15,7 @@ module Gitlab
       end
 
       def import
-        FileUtils.mkdir_p(@shared.export_path)
+        mkdir_p(@shared.export_path)
 
         wait_for_archived_file do
           decompress_archive

lib/gitlab/import_export/project_tree_saver.rb

@@ -1,6 +1,8 @@
 module Gitlab
   module ImportExport
     class ProjectTreeSaver
+      include Gitlab::ImportExport::CommandLineUtil
+
       attr_reader :full_path
 
       def initialize(project:, shared:)
@@ -10,7 +12,7 @@ module Gitlab
       end
 
       def save
-        FileUtils.mkdir_p(@shared.export_path)
+        mkdir_p(@shared.export_path)
 
         File.write(full_path, project_json_tree)
         true

lib/gitlab/import_export/repo_restorer.rb

@@ -12,7 +12,7 @@ module Gitlab
       def restore
         return true unless File.exist?(@path_to_bundle)
 
-        FileUtils.mkdir_p(path_to_repo)
+        mkdir_p(path_to_repo)
 
         git_unbundle(repo_path: path_to_repo, bundle_path: @path_to_bundle) && repo_restore_hooks
       rescue => e

lib/gitlab/import_export/repo_saver.rb

@@ -20,7 +20,7 @@ module Gitlab
       private
 
       def bundle_to_disk
-        FileUtils.mkdir_p(@shared.export_path)
+        mkdir_p(@shared.export_path)
         git_bundle(repo_path: path_to_repo, bundle_path: @full_path)
       rescue => e
         @shared.error(e)

lib/gitlab/import_export/version_saver.rb

@@ -1,12 +1,14 @@
 module Gitlab
   module ImportExport
     class VersionSaver
+      include Gitlab::ImportExport::CommandLineUtil
+
       def initialize(shared:)
         @shared = shared
       end
 
       def save
-        FileUtils.mkdir_p(@shared.export_path)
+        mkdir_p(@shared.export_path)
 
         File.write(version_file, Gitlab::ImportExport.version, mode: 'w')
       rescue => e

lib/gitlab/import_export/wiki_repo_saver.rb

@@ -9,7 +9,7 @@ module Gitlab
       end
 
       def bundle_to_disk(full_path)
-        FileUtils.mkdir_p(@shared.export_path)
+        mkdir_p(@shared.export_path)
         git_bundle(repo_path: path_to_repo, bundle_path: full_path)
       rescue => e
         @shared.error(e)

spec/controllers/users_controller_spec.rb

@@ -73,8 +73,8 @@ describe UsersController do
     end
 
     context 'forked project' do
-      let!(:project) { create(:project) }
-      let!(:forked_project) { Projects::ForkService.new(project, user).execute }
+      let(:project) { create(:project) }
+      let(:forked_project) { Projects::ForkService.new(project, user).execute }
 
       before do
         sign_in(user)

spec/features/projects/import_export/export_file_spec.rb

@@ -47,6 +47,8 @@ feature 'Import/Export - project export integration test', feature: true, js: tr
 
       expect(page).to have_content('Download export')
 
+      expect(file_permissions(project.export_path)).to eq(0700)
+
       in_directory_with_expanded_export(project) do |exit_status, tmpdir|
         expect(exit_status).to eq(0)
 

spec/helpers/projects_helper_spec.rb

@@ -11,7 +11,7 @@ describe ProjectsHelper do
 
   describe "can_change_visibility_level?" do
     let(:project) { create(:project) }
-    let(:user) { create(:user) }
+    let(:user) { create(:project_member, :reporter, user: create(:user), project: project).user }
     let(:fork_project) { Projects::ForkService.new(project, user).execute }
 
     it "returns false if there are no appropriate permissions" do

spec/models/forked_project_link_spec.rb

@@ -6,6 +6,7 @@ describe ForkedProjectLink, "add link on fork" do
   let(:user) { create(:user, namespace: namespace) }
 
   before do
+    create(:project_member, :reporter, user: user, project: project_from)
     @project_to = fork_project(project_from, user)
   end
 

spec/requests/api/api_helpers_spec.rb

@@ -10,7 +10,8 @@ describe API::Helpers, api: true do
   let(:key) { create(:key, user: user) }
 
   let(:params) { {} }
-  let(:env) { {} }
+  let(:env) { { 'REQUEST_METHOD' => 'GET' } }
+  let(:request) { Rack::Request.new(env) }
 
   def set_env(token_usr, identifier)
     clear_env
@@ -52,17 +53,43 @@ describe API::Helpers, api: true do
   describe ".current_user" do
     subject { current_user }
 
-    describe "when authenticating via Warden" do
+    describe "Warden authentication" do
       before { doorkeeper_guard_returns false }
 
-      context "fails" do
-        it { is_expected.to be_nil }
+      context "with invalid credentials" do
+        context "GET request" do
+          before { env['REQUEST_METHOD'] = 'GET' }
+          it { is_expected.to be_nil }
+        end
       end
 
-      context "succeeds" do
+      context "with valid credentials" do
         before { warden_authenticate_returns user }
 
-        it { is_expected.to eq(user) }
+        context "GET request" do
+          before { env['REQUEST_METHOD'] = 'GET' }
+          it { is_expected.to eq(user) }
+        end
+
+        context "HEAD request" do
+          before { env['REQUEST_METHOD'] = 'HEAD' }
+          it { is_expected.to eq(user) }
+        end
+
+        context "PUT request" do
+          before { env['REQUEST_METHOD'] = 'PUT' }
+          it { is_expected.to be_nil }
+        end
+
+        context "POST request" do
+          before { env['REQUEST_METHOD'] = 'POST' }
+          it { is_expected.to be_nil }
+        end
+
+        context "DELETE request" do
+          before { env['REQUEST_METHOD'] = 'DELETE' }
+          it { is_expected.to be_nil }
+        end
       end
     end
 

spec/requests/api/fork_spec.rb

@@ -18,7 +18,7 @@ describe API::API, api: true  do
   end
 
   let(:project_user2) do
-    create(:project_member, :guest, user: user2, project: project)
+    create(:project_member, :reporter, user: user2, project: project)
   end
 
   describe 'POST /projects/fork/:id' do

spec/services/projects/fork_service_spec.rb

@@ -12,12 +12,26 @@ describe Projects::ForkService, services: true do
                              description: 'wow such project')
       @to_namespace = create(:namespace)
       @to_user = create(:user, namespace: @to_namespace)
+      @from_project.add_user(@to_user, :developer)
     end
 
     context 'fork project' do
+      context 'when forker is a guest' do
+        before do
+          @guest = create(:user)
+          @from_project.add_user(@guest, :guest)
+        end
+        subject { fork_project(@from_project, @guest) }
+
+        it { is_expected.not_to be_persisted }
+        it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) }
+      end
+
       describe "successfully creates project in the user namespace" do
         let(:to_project) { fork_project(@from_project, @to_user) }
 
+        it { expect(to_project).to be_persisted }
+        it { expect(to_project.errors).to be_empty }
         it { expect(to_project.owner).to eq(@to_user) }
         it { expect(to_project.namespace).to eq(@to_user.namespace) }
         it { expect(to_project.star_count).to be_zero }
@@ -29,7 +43,9 @@ describe Projects::ForkService, services: true do
       it "fails due to validation, not transaction failure" do
         @existing_project = create(:project, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace)
         @to_project = fork_project(@from_project, @to_user)
-        expect(@existing_project.persisted?).to be_truthy
+        expect(@existing_project).to be_persisted
+
+        expect(@to_project).not_to be_persisted
         expect(@to_project.errors[:name]).to eq(['has already been taken'])
         expect(@to_project.errors[:path]).to eq(['has already been taken'])
       end
@@ -81,18 +97,23 @@ describe Projects::ForkService, services: true do
       @group = create(:group)
       @group.add_user(@group_owner, GroupMember::OWNER)
       @group.add_user(@developer,   GroupMember::DEVELOPER)
+      @project.add_user(@developer,   :developer)
+      @project.add_user(@group_owner, :developer)
       @opts = { namespace: @group }
     end
 
     context 'fork project for group' do
       it 'group owner successfully forks project into the group' do
         to_project = fork_project(@project, @group_owner, @opts)
+
+        expect(to_project).to             be_persisted
+        expect(to_project.errors).to      be_empty
         expect(to_project.owner).to       eq(@group)
         expect(to_project.namespace).to   eq(@group)
         expect(to_project.name).to        eq(@project.name)
         expect(to_project.path).to        eq(@project.path)
         expect(to_project.description).to eq(@project.description)
-        expect(to_project.star_count).to     be_zero
+        expect(to_project.star_count).to  be_zero
       end
     end
 

spec/services/system_note_service_spec.rb

@@ -451,7 +451,7 @@ describe SystemNoteService, services: true do
     end
 
     context 'commit with cross-reference from fork' do
-      let(:author2) { create(:user) }
+      let(:author2) { create(:project_member, :reporter, user: create(:user), project: project).user }
       let(:forked_project) { Projects::ForkService.new(project, author2).execute }
       let(:commit2) { forked_project.commit }
 

spec/support/import_export/export_file_helper.rb

@@ -130,4 +130,8 @@ module ExportFileHelper
 
     (parsed_model_attributes - parent.keys - excluded_attributes).empty?
   end
+
+  def file_permissions(file)
+    File.stat(file).mode & 0777
+  end
 end

spec/views/ci/lints/show.html.haml_spec.rb

@@ -1,6 +1,52 @@
 require 'spec_helper'
 
 describe 'ci/lints/show' do
+  include Devise::TestHelpers
+
+  describe 'XSS protection' do
+    let(:config_processor) { Ci::GitlabCiYamlProcessor.new(YAML.dump(content)) }
+    before do
+      assign(:status, true)
+      assign(:builds, config_processor.builds)
+      assign(:stages, config_processor.stages)
+      assign(:jobs, config_processor.jobs)
+    end
+
+    context 'when builds attrbiutes contain HTML nodes' do
+      let(:content) do
+        {
+          rspec: {
+            script: '<h1>rspec</h1>',
+            stage: 'test'
+          }
+        }
+      end
+
+      it 'does not render HTML elements' do
+        render
+
+        expect(rendered).not_to have_css('h1', text: 'rspec')
+      end
+    end
+
+    context 'when builds attributes do not contain HTML nodes' do
+      let(:content) do
+        {
+          rspec: {
+            script: 'rspec',
+            stage: 'test'
+          }
+        }
+      end
+
+      it 'shows configuration in the table' do
+        render
+
+        expect(rendered).to have_css('td pre', text: 'rspec')
+      end
+    end
+  end
+
   let(:content) do
     {
       build_template: {