Merge branch 'master-security-update' into 'master'

Master security update

See merge request !12025

app/assets/javascripts/notes.js

@@ -1478,7 +1478,7 @@ const normalizeNewlines = function(str) {
       const cachedNoteBodyText = $noteBodyText.html();
       // Show updated comment content temporarily
-      $noteBodyText.html(formContent);
+      $noteBodyText.html(_.escape(formContent));
       $editingNote.removeClass('is-editing fade-in-full').addClass('being-posted fade-in-half');
       $editingNote.find('.note-headline-meta a').html('<i class="fa fa-spinner fa-spin" aria-label="Comment is being updated" aria-hidden="true"></i>');
@@ -1491,7 +1491,7 @@ const normalizeNewlines = function(str) {
         })
         .fail(() => {
           // Submission failed, revert back to original note
-          $noteBodyText.html(cachedNoteBodyText);
+          $noteBodyText.html(_.escape(cachedNoteBodyText));
           $editingNote.removeClass('being-posted fade-in');
           $editingNote.find('.fa.fa-spinner').remove();

app/controllers/autocomplete_controller.rb

@@ -21,7 +21,7 @@ class AutocompleteController < ApplicationController
         @users = [current_user, *@users].uniq
       end
-      if params[:author_id].present?
+      if params[:author_id].present? && current_user
         author = User.find_by_id(params[:author_id])
         @users = [author, *@users].uniq if author
       end

app/policies/project_snippet_policy.rb

 class ProjectSnippetPolicy < BasePolicy
   def rules
+    # We have to check both project feature visibility and a snippet visibility and take the stricter one
+    # This will be simplified - check https://gitlab.com/gitlab-org/gitlab-ce/issues/27573
+    return unless @subject.project.feature_available?(:snippets, @user)
+    return unless Ability.allowed?(@user, :read_project, @subject.project)
+
     can! :read_project_snippet if @subject.public?
     return unless @user

app/uploaders/file_uploader.rb

@@ -13,6 +13,13 @@ class FileUploader < GitlabUploader
     )
   end
+  # Not using `GitlabUploader.base_dir` because all project namespaces are in
+  # the `public/uploads` dir.
+  #
+  def self.base_dir
+    root_dir
+  end
+
   # Returns the part of `store_dir` that can change based on the model's current
   # path
   #

app/uploaders/gitlab_uploader.rb

@@ -3,16 +3,28 @@ class GitlabUploader < CarrierWave::Uploader::Base
     File.join(CarrierWave.root, upload_record.path)
   end
-  def self.base_dir
+  def self.root_dir
     'uploads'
   end
-  delegate :base_dir, to: :class
-  def file_storage?
-    storage.is_a?(CarrierWave::Storage::File)
+  # When object storage is used, keep the `root_dir` as `base_dir`.
+  # The files aren't really in folders there, they just have a name.
+  # The files that contain user input in their name, also contain a hash, so
+  # the names are still unique
+  #
+  # This method is overridden in the `FileUploader`
+  def self.base_dir
+    return root_dir unless file_storage?
+
+    File.join(root_dir, 'system')
+  end
+  def self.file_storage?
+    self.storage == CarrierWave::Storage::File
   end
+  delegate :base_dir, :file_storage?, to: :class
+
   def file_cache_storage?
     cache_storage.is_a?(CarrierWave::Storage::File)
   end

config/routes/uploads.rb

 scope path: :uploads do
   # Note attachments and User/Group/Project avatars
-  get ":model/:mounted_as/:id/:filename",
+  get "system/:model/:mounted_as/:id/:filename",
       to:           "uploads#show",
       constraints:  { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ }
@@ -15,7 +15,7 @@ scope path: :uploads do
     constraints: { filename: /[^\/]+/ }
   # Appearance
-  get ":model/:mounted_as/:id/:filename",
+  get "system/:model/:mounted_as/:id/:filename",
       to:           "uploads#show",
       constraints:  { model: /appearance/, mounted_as: /logo|header_logo/, filename: /.+/ }

db/migrate/20170316163800_rename_system_namespaces.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+class RenameSystemNamespaces < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  include Gitlab::ShellAdapter
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+  end
+
+  class Namespace < ActiveRecord::Base
+    self.table_name = 'namespaces'
+    belongs_to :parent, class_name: 'RenameSystemNamespaces::Namespace'
+    has_one :route, as: :source
+    has_many :children, class_name: 'RenameSystemNamespaces::Namespace', foreign_key: :parent_id
+    belongs_to :owner, class_name: 'RenameSystemNamespaces::User'
+
+    # Overridden to have the correct `source_type` for the `route` relation
+    def self.name
+      'Namespace'
+    end
+
+    def full_path
+      if route && route.path.present?
+        @full_path ||= route.path
+      else
+        update_route if persisted?
+
+        build_full_path
+      end
+    end
+
+    def build_full_path
+      if parent && path
+        parent.full_path + '/' + path
+      else
+        path
+      end
+    end
+
+    def update_route
+      prepare_route
+      route.save
+    end
+
+    def prepare_route
+      route || build_route(source: self)
+      route.path = build_full_path
+      route.name = build_full_name
+      @full_path = nil
+      @full_name = nil
+    end
+
+    def build_full_name
+      if parent && name
+        parent.human_name + ' / ' + name
+      else
+        name
+      end
+    end
+
+    def human_name
+      owner&.name
+    end
+  end
+
+  class Route < ActiveRecord::Base
+    self.table_name = 'routes'
+    belongs_to :source, polymorphic: true
+  end
+
+  class Project < ActiveRecord::Base
+    self.table_name = 'projects'
+
+    def repository_storage_path
+      Gitlab.config.repositories.storages[repository_storage]['path']
+    end
+  end
+
+  DOWNTIME = false
+
+  def up
+    return unless system_namespace
+
+    old_path = system_namespace.path
+    old_full_path = system_namespace.full_path
+    # Only remove the last occurrence of the path name to get the parent namespace path
+    namespace_path = remove_last_occurrence(old_full_path, old_path)
+    new_path = rename_path(namespace_path, old_path)
+    new_full_path = join_namespace_path(namespace_path, new_path)
+
+    Namespace.where(id: system_namespace).update_all(path: new_path) # skips callbacks & validations
+
+    replace_statement = replace_sql(Route.arel_table[:path], old_full_path, new_full_path)
+    route_matches = [old_full_path, "#{old_full_path}/%"]
+
+    update_column_in_batches(:routes, :path, replace_statement) do |table, query|
+      query.where(Route.arel_table[:path].matches_any(route_matches))
+    end
+
+    clear_cache_for_namespace(system_namespace)
+
+    # tasks here are based on `Namespace#move_dir`
+    move_repositories(system_namespace, old_full_path, new_full_path)
+    move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage?
+    move_namespace_folders(pages_dir, old_full_path, new_full_path)
+  end
+
+  def down
+    # nothing to do
+  end
+
+  def remove_last_occurrence(string, pattern)
+    string.reverse.sub(pattern.reverse, "").reverse
+  end
+
+  def move_namespace_folders(directory, old_relative_path, new_relative_path)
+    old_path = File.join(directory, old_relative_path)
+    return unless File.directory?(old_path)
+
+    new_path = File.join(directory, new_relative_path)
+    FileUtils.mv(old_path, new_path)
+  end
+
+  def move_repositories(namespace, old_full_path, new_full_path)
+    repo_paths_for_namespace(namespace).each do |repository_storage_path|
+      # Ensure old directory exists before moving it
+      gitlab_shell.add_namespace(repository_storage_path, old_full_path)
+
+      unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path)
+        say "Exception moving path #{repository_storage_path} from #{old_full_path} to #{new_full_path}"
+      end
+    end
+  end
+
+  def rename_path(namespace_path, path_was)
+    counter = 0
+    path = "#{path_was}#{counter}"
+
+    while route_exists?(join_namespace_path(namespace_path, path))
+      counter += 1
+      path = "#{path_was}#{counter}"
+    end
+
+    path
+  end
+
+  def route_exists?(full_path)
+    Route.where(Route.arel_table[:path].matches(full_path)).any?
+  end
+
+  def join_namespace_path(namespace_path, path)
+    if namespace_path.present?
+      File.join(namespace_path, path)
+    else
+      path
+    end
+  end
+
+  def system_namespace
+    @system_namespace ||= Namespace.where(parent_id: nil).
+                            where(arel_table[:path].matches(system_namespace_path)).
+                            first
+  end
+
+  def system_namespace_path
+    "system"
+  end
+
+  def clear_cache_for_namespace(namespace)
+    project_ids = projects_for_namespace(namespace).pluck(:id)
+
+    update_column_in_batches(:projects, :description_html, nil) do |table, query|
+      query.where(table[:id].in(project_ids))
+    end
+
+    update_column_in_batches(:issues, :description_html, nil) do |table, query|
+      query.where(table[:project_id].in(project_ids))
+    end
+
+    update_column_in_batches(:merge_requests, :description_html, nil) do |table, query|
+      query.where(table[:target_project_id].in(project_ids))
+    end
+
+    update_column_in_batches(:notes, :note_html, nil) do |table, query|
+      query.where(table[:project_id].in(project_ids))
+    end
+
+    update_column_in_batches(:milestones, :description_html, nil) do |table, query|
+      query.where(table[:project_id].in(project_ids))
+    end
+  end
+
+  def projects_for_namespace(namespace)
+    namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id])
+    namespace_or_children = Project.arel_table[:namespace_id].in(namespace_ids)
+    Project.unscoped.where(namespace_or_children)
+  end
+
+  # This won't scale to huge trees, but it should do for a handful of namespaces
+  # called `system`.
+  def child_ids_for_parent(namespace, ids: [])
+    namespace.children.each do |child|
+      ids << child.id
+      child_ids_for_parent(child, ids: ids) if child.children.any?
+    end
+    ids
+  end
+
+  def repo_paths_for_namespace(namespace)
+    projects_for_namespace(namespace).distinct.
+      select(:repository_storage).map(&:repository_storage_path)
+  end
+
+  def uploads_dir
+    File.join(Rails.root, "public", "uploads")
+  end
+
+  def pages_dir
+    Settings.pages.path
+  end
+
+  def file_storage?
+    CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
+  end
+
+  def arel_table
+    Namespace.arel_table
+  end
+end

db/migrate/20170316163845_move_uploads_to_system_dir.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MoveUploadsToSystemDir < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+  DIRECTORIES_TO_MOVE = %w(user project note group appearance).freeze
+
+  def up
+    return unless file_storage?
+
+    FileUtils.mkdir_p(new_upload_dir)
+
+    DIRECTORIES_TO_MOVE.each do |dir|
+      source = File.join(old_upload_dir, dir)
+      destination = File.join(new_upload_dir, dir)
+      next unless File.directory?(source)
+      next if File.directory?(destination)
+
+      say "Moving #{source} -> #{destination}"
+      FileUtils.mv(source, destination)
+      FileUtils.ln_s(destination, source)
+    end
+  end
+
+  def down
+    return unless file_storage?
+    return unless File.directory?(new_upload_dir)
+
+    DIRECTORIES_TO_MOVE.each do |dir|
+      source = File.join(new_upload_dir, dir)
+      destination = File.join(old_upload_dir, dir)
+      next unless File.directory?(source)
+      next if File.directory?(destination) && !File.symlink?(destination)
+
+      say "Moving #{source} -> #{destination}"
+      FileUtils.rm(destination) if File.symlink?(destination)
+      FileUtils.mv(source, destination)
+    end
+  end
+
+  def file_storage?
+    CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
+  end
+
+  def base_directory
+    Rails.root
+  end
+
+  def old_upload_dir
+    File.join(base_directory, "public", "uploads")
+  end
+
+  def new_upload_dir
+    File.join(base_directory, "public", "uploads", "system")
+  end
+end

db/post_migrate/20170317162059_update_upload_paths_to_system.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class UpdateUploadPathsToSystem < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  AFFECTED_MODELS = %w(User Project Note Namespace Appearance)
+
+  def up
+    update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], base_directory, new_upload_dir)) do |_table, query|
+      query.where(uploads_to_switch_to_new_path)
+    end
+  end
+
+  def down
+    update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], new_upload_dir, base_directory)) do |_table, query|
+      query.where(uploads_to_switch_to_old_path)
+    end
+  end
+
+  # "SELECT \"uploads\".* FROM \"uploads\" WHERE \"uploads\".\"model_type\" IN ('User', 'Project', 'Note', 'Namespace', 'Appearance') AND (\"uploads\".\"path\" ILIKE 'uploads/%' AND NOT (\"uploads\".\"path\" ILIKE 'uploads/system/%'))"
+  def uploads_to_switch_to_new_path
+    affected_uploads.and(starting_with_base_directory).and(starting_with_new_upload_directory.not)
+  end
+
+  # "SELECT \"uploads\".* FROM \"uploads\" WHERE \"uploads\".\"model_type\" IN ('User', 'Project', 'Note', 'Namespace', 'Appearance') AND (\"uploads\".\"path\" ILIKE 'uploads/%' AND \"uploads\".\"path\" ILIKE 'uploads/system/%')"
+  def uploads_to_switch_to_old_path
+    affected_uploads.and(starting_with_new_upload_directory)
+  end
+
+  def starting_with_base_directory
+    arel_table[:path].matches("#{base_directory}/%")
+  end
+
+  def starting_with_new_upload_directory
+    arel_table[:path].matches("#{new_upload_dir}/%")
+  end
+
+  def affected_uploads
+    arel_table[:model_type].in(AFFECTED_MODELS)
+  end
+
+  def base_directory
+    "uploads"
+  end
+
+  def new_upload_dir
+    File.join(base_directory, "system")
+  end
+
+  def arel_table
+    Arel::Table.new(:uploads)
+  end
+end

db/post_migrate/20170406111121_clean_upload_symlinks.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CleanUploadSymlinks < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+  DIRECTORIES_TO_MOVE = %w(user project note group appeareance)
+
+  def up
+    return unless file_storage?
+
+    DIRECTORIES_TO_MOVE.each do |dir|
+      symlink_location = File.join(old_upload_dir, dir)
+      next unless File.symlink?(symlink_location)
+      say "removing symlink: #{symlink_location}"
+      FileUtils.rm(symlink_location)
+    end
+  end
+
+  def down
+    return unless file_storage?
+
+    DIRECTORIES_TO_MOVE.each do |dir|
+      symlink = File.join(old_upload_dir, dir)
+      destination = File.join(new_upload_dir, dir)
+
+      next if File.directory?(symlink)
+      next unless File.directory?(destination)
+
+      say "Creating symlink #{symlink} -> #{destination}"
+      FileUtils.ln_s(destination, symlink)
+    end
+  end
+
+  def file_storage?
+    CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
+  end
+
+  def base_directory
+    Rails.root
+  end
+
+  def old_upload_dir
+    File.join(base_directory, "public", "uploads")
+  end
+
+  def new_upload_dir
+    File.join(base_directory, "public", "uploads", "system")
+  end
+end

db/post_migrate/20170606202615_move_appearance_to_system_dir.rb

+class MoveAppearanceToSystemDir < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+  DIRECTORY_TO_MOVE = 'appearance'.freeze
+
+  def up
+    source = File.join(old_upload_dir, DIRECTORY_TO_MOVE)
+    destination = File.join(new_upload_dir, DIRECTORY_TO_MOVE)
+
+    move_directory(source, destination)
+  end
+
+  def down
+    source = File.join(new_upload_dir, DIRECTORY_TO_MOVE)
+    destination = File.join(old_upload_dir, DIRECTORY_TO_MOVE)
+
+    move_directory(source, destination)
+  end
+
+  def move_directory(source, destination)
+    unless file_storage?
+      say 'Not using file storage, skipping'
+      return
+    end
+
+    unless File.directory?(source)
+      say "#{source} did not exist, skipping"
+      return
+    end
+
+    if File.directory?(destination)
+      say "#{destination} already existed, skipping"
+      return
+    end
+
+    say "Moving #{source} -> #{destination}"
+    FileUtils.mv(source, destination)
+  end
+
+  def file_storage?
+    CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
+  end
+
+  def base_directory
+    Rails.root
+  end
+
+  def old_upload_dir
+    File.join(base_directory, "public", "uploads")
+  end
+
+  def new_upload_dir
+    File.join(base_directory, "public", "uploads", "system")
+  end
+end

db/schema.rb

@@ -11,8 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20170603200744) do
-
+ActiveRecord::Schema.define(version: 20170606202615) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
   enable_extension "pg_trgm"

features/steps/groups.rb

@@ -81,7 +81,7 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
   step 'I should see new group "Owned" avatar' do
     expect(owned_group.avatar).to be_instance_of AvatarUploader
-    expect(owned_group.avatar.url).to eq "/uploads/group/avatar/#{Group.find_by(name: "Owned").id}/banana_sample.gif"
+    expect(owned_group.avatar.url).to eq "/uploads/system/group/avatar/#{Group.find_by(name: "Owned").id}/banana_sample.gif"
   end
   step 'I should see the "Remove avatar" button' do

features/steps/profile/profile.rb

@@ -36,7 +36,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   step 'I should see new avatar' do
     expect(@user.avatar).to be_instance_of AvatarUploader
-    expect(@user.avatar.url).to eq "/uploads/user/avatar/#{@user.id}/banana_sample.gif"
+    expect(@user.avatar.url).to eq "/uploads/system/user/avatar/#{@user.id}/banana_sample.gif"
   end
   step 'I should see the "Remove avatar" button' do

features/steps/project/project.rb

@@ -38,7 +38,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
   step 'I should see new project avatar' do
     expect(@project.avatar).to be_instance_of AvatarUploader
     url = @project.avatar.url
-    expect(url).to eq "/uploads/project/avatar/#{@project.id}/banana_sample.gif"
+    expect(url).to eq "/uploads/system/project/avatar/#{@project.id}/banana_sample.gif"
   end
   step 'I should see the "Remove avatar" button' do

lib/api/api.rb

@@ -45,6 +45,7 @@ module API
     end
     before { allow_access_with_scope :api }
+    before { header['X-Frame-Options'] = 'SAMEORIGIN' }
     before { Gitlab::I18n.locale = current_user&.preferred_language }
     after { Gitlab::I18n.use_default_locale }

lib/banzai/reference_parser/base_parser.rb

@@ -62,7 +62,7 @@ module Banzai
         nodes.select do |node|
           if node.has_attribute?(project_attr)
-            can_read_reference?(user, projects[node])
+            can_read_reference?(user, projects[node], node)
           else
             true
           end
@@ -231,7 +231,7 @@ module Banzai
       # see reference comments.
       # Override this method on subclasses
       # to check if user can read resource
-      def can_read_reference?(user, ref_project)
+      def can_read_reference?(user, ref_project, node)
         raise NotImplementedError
       end

lib/banzai/reference_parser/commit_parser.rb

@@ -32,7 +32,7 @@ module Banzai
       private
-      def can_read_reference?(user, ref_project)
+      def can_read_reference?(user, ref_project, node)
         can?(user, :download_code, ref_project)
       end
     end

lib/banzai/reference_parser/commit_range_parser.rb

@@ -36,7 +36,7 @@ module Banzai
       private
-      def can_read_reference?(user, ref_project)
+      def can_read_reference?(user, ref_project, node)
         can?(user, :download_code, ref_project)
       end
     end

lib/banzai/reference_parser/external_issue_parser.rb

@@ -23,7 +23,7 @@ module Banzai
       private
-      def can_read_reference?(user, ref_project)
+      def can_read_reference?(user, ref_project, node)
         can?(user, :read_issue, ref_project)
       end
     end