Merge remote-tracking branch 'dev/security-9-0' into 9-0-stable

app/controllers/autocomplete_controller.rb

@@ -22,7 +22,7 @@ class AutocompleteController < ApplicationController
         @users = [current_user, *@users]
       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/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?
-    self.class.storage == 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
+
   # Reduce disk IO
   def move_to_cache
     true

app/validators/namespace_validator.rb

@@ -33,6 +33,7 @@ class NamespaceValidator < ActiveModel::EachValidator
     u
     unsubscribes
     users
+    system
   ].freeze
   WILDCARD_ROUTES = %w[tree commits wikis new edit create update logs_tree
@@ -47,9 +48,9 @@ class NamespaceValidator < ActiveModel::EachValidator
   def self.reserved?(value, strict: false)
     if strict
-      STRICT_RESERVED.include?(value)
+      STRICT_RESERVED.include?(value.to_s.downcase)
     else
-      RESERVED.include?(value)
+      RESERVED.include?(value.to_s.downcase)
     end
   end

app/validators/project_path_validator.rb

@@ -15,7 +15,7 @@ class ProjectPathValidator < ActiveModel::EachValidator
   #  'tree' as project name and 'deploy_keys' as route.
   #
   RESERVED = (NamespaceValidator::STRICT_RESERVED -
-              %w[dashboard help ci admin search notes services assets profile public]).freeze
+              %w[dashboard help ci admin search notes services assets profile public system]).freeze
   def self.valid?(value)
     !reserved?(value)

changelogs/unreleased/28917-contain-uploads-in-system-dir.yml

+---
+title: Move uploads from 'public/uploads' to 'public/uploads/system'
+merge_request:
+author:

changelogs/unreleased/dz-api-x-frame.yml

+---
+title: Restrict API X-Frame-Options to same origin
+merge_request:
+author:

changelogs/unreleased/dz-restrict-autocomplete.yml

+---
+title: Allow users autocomplete by author_id only for authenticated users
+merge_request:
+author:

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: /[^\/]+/ }
   # 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 appeareance)
+
+  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

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

@@ -37,7 +37,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

@@ -44,6 +44,7 @@ module API
     end
     before { allow_access_with_scope :api }
+    before { header['X-Frame-Options'] = 'SAMEORIGIN' }
     rescue_from Gitlab::Access::AccessDeniedError do
       rack_response({ 'message' => '403 Forbidden' }.to_json, 403)

lib/gitlab/database/migration_helpers.rb

@@ -80,7 +80,7 @@ module Gitlab
       # here is based on Rails' foreign_key_name() method, which unfortunately
       # is private so we can't rely on it directly.
       def concurrent_foreign_key_name(table, column)
-        "fk_#{Digest::SHA256.hexdigest("#{table}_#{column}_fk").first(10)}"
+        "fk_#{Digest::SHA256.hexdigest('#{table}_#{column}_fk').first(10)}"
       end
       # Long-running migrations may take more than the timeout allowed by
@@ -226,6 +226,29 @@ module Gitlab
           raise error
         end
       end
+
+      # This will replace the first occurance of a string in a column with
+      # the replacement
+      # On postgresql we can use `regexp_replace` for that.
+      # On mysql we find the location of the pattern, and overwrite it
+      # with the replacement
+      def replace_sql(column, pattern, replacement)
+        quoted_pattern = Arel::Nodes::Quoted.new(pattern.to_s)
+        quoted_replacement = Arel::Nodes::Quoted.new(replacement.to_s)
+
+        if Database.mysql?
+          locate = Arel::Nodes::NamedFunction.
+            new('locate', [quoted_pattern, column])
+          insert_in_place = Arel::Nodes::NamedFunction.
+            new('insert', [column, locate, pattern.size, quoted_replacement])
+
+          Arel::Nodes::SqlLiteral.new(insert_in_place.to_sql)
+        else
+          replace = Arel::Nodes::NamedFunction.
+            new("regexp_replace", [column, quoted_pattern, quoted_replacement])
+          Arel::Nodes::SqlLiteral.new(replace.to_sql)
+        end
+      end
     end
   end
 end

lib/tasks/gitlab/db.rake

@@ -62,7 +62,7 @@ namespace :gitlab do
       ref = Shellwords.escape(args[:ref])
-      migrations = `git diff #{ref}.. --name-only -- db/migrate`.lines
+      migrations = `git diff #{ref}.. --diff-filter=A --name-only -- db/migrate`.lines
         .map { |file| Rails.root.join(file.strip).to_s }
         .select { |file| File.file?(file) }

spec/controllers/autocomplete_controller_spec.rb

@@ -156,22 +156,32 @@ describe AutocompleteController do
     end
     context 'author of issuable included' do
-      before do
-        sign_in(user)
-      end
-
       let(:body) { JSON.parse(response.body) }
-      it 'includes the author' do
-        get(:users, author_id: non_member.id)
-        expect(body.first["username"]).to eq non_member.username
+      context 'authenticated' do
+        before do
+          sign_in(user)
+        end
+
+        it 'includes the author' do
+          get(:users, author_id: non_member.id)
+
+          expect(body.first["username"]).to eq non_member.username
+        end
+
+        it 'rejects non existent user ids' do
+          get(:users, author_id: 99999)
+          expect(body.collect { |u| u['id'] }).not_to include(99999)
+        end
       end
-      it 'rejects non existent user ids' do
-        get(:users, author_id: 99999)
-        expect(body.collect { |u| u['id'] }).not_to include(99999)
+      context 'without authenticating' do
+        it 'returns empty result' do
+          get(:users, author_id: non_member.id)
+          expect(body).to be_empty
+        end
       end
     end