Merge remote-tracking branch 'dev/master'

CHANGELOG.md

@@ -2,6 +2,15 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 10.8.2 (2018-05-28)
+
+### Security (3 changes)
+
+- Prevent user passwords from being changed without providing the previous password.
+- Fix API to remove deploy key from project instead of deleting it entirely.
+- Fixed bug that allowed importing arbitrary project attributes.
+
+
 ## 10.8.1 (2018-05-23)
 
 ### Fixed (9 changes)
@@ -193,6 +202,15 @@ entry.
 - Gitaly handles repository forks by default.
 
 
+## 10.7.5 (2018-05-28)
+
+### Security (3 changes)
+
+- Prevent user passwords from being changed without providing the previous password.
+- Fix API to remove deploy key from project instead of deleting it entirely.
+- Fixed bug that allowed importing arbitrary project attributes.
+
+
 ## 10.7.4 (2018-05-21)
 
 ### Fixed (1 change)
@@ -457,6 +475,16 @@ entry.
 - Upgrade Gitaly to upgrade its charlock_holmes.
 
 
+## 10.6.6 (2018-05-28)
+
+### Security (4 changes)
+
+- Do not allow non-members to create MRs via forked projects when MRs are private.
+- Prevent user passwords from being changed without providing the previous password.
+- Fix API to remove deploy key from project instead of deleting it entirely.
+- Fixed bug that allowed importing arbitrary project attributes.
+
+
 ## 10.6.5 (2018-04-24)
 
 ### Security (1 change)

app/controllers/profiles_controller.rb

@@ -93,8 +93,6 @@ class ProfilesController < Profiles::ApplicationController
       :linkedin,
       :location,
       :name,
-      :password,
-      :password_confirmation,
       :public_email,
       :skype,
       :twitter,

changelogs/unreleased/security-dm-delete-deploy-key.yml

+---
+title: Fix API to remove deploy key from project instead of deleting it entirely
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fj-import-export-assignment.yml

+---
+title: Fixed bug that allowed importing arbitrary project attributes
+merge_request:
+author:
+type: security

changelogs/unreleased/security-users-can-update-their-password-without-entering-current-password.yml

+---
+title: Prevent user passwords from being changed without providing the previous password
+merge_request:
+author:
+type: security

lib/api/deploy_keys.rb

@@ -148,10 +148,10 @@ module API
         requires :key_id, type: Integer, desc: 'The ID of the deploy key'
       end
       delete ":id/deploy_keys/:key_id" do
-        key = user_project.deploy_keys.find(params[:key_id])
-        not_found!('Deploy Key') unless key
+        deploy_key_project = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
+        not_found!('Deploy Key') unless deploy_key_project
 
-        destroy_conditionally!(key)
+        destroy_conditionally!(deploy_key_project)
       end
     end
   end

lib/gitlab/import_export/attribute_cleaner.rb

@@ -7,14 +7,15 @@ module Gitlab
         new(*args).clean
       end
 
-      def initialize(relation_hash:, relation_class:)
+      def initialize(relation_hash:, relation_class:, excluded_keys: [])
         @relation_hash = relation_hash
         @relation_class = relation_class
+        @excluded_keys = excluded_keys
       end
 
       def clean
         @relation_hash.reject do |key, _value|
-          prohibited_key?(key) || !@relation_class.attribute_method?(key)
+          prohibited_key?(key) || !@relation_class.attribute_method?(key) || excluded_key?(key)
         end.except('id')
       end
 
@@ -23,6 +24,12 @@ module Gitlab
       def prohibited_key?(key)
         key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
       end
+
+      def excluded_key?(key)
+        return false if @excluded_keys.empty?
+
+        @excluded_keys.include?(key)
+      end
     end
   end
 end

lib/gitlab/import_export/attributes_finder.rb

@@ -32,6 +32,10 @@ module Gitlab
         @methods[key].nil? ? {} : { methods: @methods[key] }
       end
 
+      def find_excluded_keys(klass_name)
+        @excluded_attributes[klass_name.to_sym]&.map(&:to_s) || []
+      end
+
       private
 
       def find_attributes_only(value)

lib/gitlab/import_export/import_export.yml

@@ -98,8 +98,6 @@ excluded_attributes:
     - :import_jid
     - :created_at
     - :updated_at
-    - :import_jid
-    - :import_jid
     - :id
     - :star_count
     - :last_activity_at

lib/gitlab/import_export/project_tree_restorer.rb

@@ -88,16 +88,18 @@ module Gitlab
       end
 
       def project_params
-        @project_params ||= json_params.merge(override_params)
+        @project_params ||= begin
+          attrs = json_params.merge(override_params)
+
+          # Cleaning all imported and overridden params
+          Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: attrs,
+                                                       relation_class: Project,
+                                                       excluded_keys: excluded_keys_for_relation(:project))
+        end
       end
 
       def override_params
-        return {} unless params = @project.import_data&.data&.fetch('override_params', nil)
-
-        @override_params ||= params.select do |key, _value|
-          Project.column_names.include?(key.to_s) &&
-            !reader.project_tree[:except].include?(key.to_sym)
-        end
+        @override_params ||= @project.import_data&.data&.fetch('override_params', nil) || {}
       end
 
       def json_params
@@ -171,7 +173,8 @@ module Gitlab
                                                        relation_hash: parsed_relation_hash(relation_hash, relation.to_sym),
                                                        members_mapper: members_mapper,
                                                        user: @user,
-                                                       project: @restored_project)
+                                                       project: @restored_project,
+                                                       excluded_keys: excluded_keys_for_relation(relation))
         end.compact
 
         relation_hash_list.is_a?(Array) ? relation_array : relation_array.first
@@ -192,6 +195,10 @@ module Gitlab
       def reader
         @reader ||= Gitlab::ImportExport::Reader.new(shared: @shared)
       end
+
+      def excluded_keys_for_relation(relation)
+        @reader.attributes_finder.find_excluded_keys(relation)
+      end
     end
   end
 end

lib/gitlab/import_export/reader.rb

 module Gitlab
   module ImportExport
     class Reader
-      attr_reader :tree
+      attr_reader :tree, :attributes_finder
 
       def initialize(shared:)
         @shared = shared

lib/gitlab/import_export/relation_factory.rb

@@ -36,13 +36,21 @@ module Gitlab
         new(*args).create
       end
 
-      def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project:)
+      def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project:, excluded_keys: [])
         @relation_name = OVERRIDES[relation_sym] || relation_sym
         @relation_hash = relation_hash.except('noteable_id')
         @members_mapper = members_mapper
         @user = user
         @project = project
         @imported_object_retries = 0
+
+        # Remove excluded keys from relation_hash
+        # We don't do this in the parsed_relation_hash because of the 'transformed attributes'
+        # For example, MergeRequestDiffFiles exports its diff attribute as utf8_diff. Then,
+        # in the create method that attribute is renamed to diff. And because diff is an excluded key,
+        # if we clean the excluded keys in the parsed_relation_hash, it will be removed
+        # from the object attributes and the export will fail.
+        @relation_hash.except!(*excluded_keys)
       end
 
       # Creates an object from an actual model with name "relation_sym" with params from

spec/controllers/profiles_controller_spec.rb

@@ -3,6 +3,19 @@ require('spec_helper')
 describe ProfilesController, :request_store do
   let(:user) { create(:user) }
 
+  describe 'POST update' do
+    it 'does not update password' do
+      sign_in(user)
+
+      expect do
+        post :update,
+             user: { password: 'hello12345', password_confirmation: 'hello12345' }
+      end.not_to change { user.reload.encrypted_password }
+
+      expect(response.status).to eq(302)
+    end
+  end
+
   describe 'PUT update' do
     it 'allows an email update from a user without an external email address' do
       sign_in(user)

spec/lib/gitlab/import_export/attribute_cleaner_spec.rb

@@ -15,7 +15,10 @@ describe Gitlab::ImportExport::AttributeCleaner do
       'project_id' => 99,
       'user_id' => 99,
       'random_id_in_the_middle' => 99,
-      'notid' => 99
+      'notid' => 99,
+      'import_source' => 'whatever',
+      'import_type' => 'whatever',
+      'non_existent_attr' => 'whatever'
     }
   end
 
@@ -28,10 +31,30 @@ describe Gitlab::ImportExport::AttributeCleaner do
     }
   end
 
+  let(:excluded_keys) { %w[import_source import_type] }
+
+  subject { described_class.clean(relation_hash: unsafe_hash, relation_class: relation_class, excluded_keys: excluded_keys) }
+
+  before do
+    allow(relation_class).to receive(:attribute_method?).and_return(true)
+    allow(relation_class).to receive(:attribute_method?).with('non_existent_attr').and_return(false)
+  end
+
   it 'removes unwanted attributes from the hash' do
-    # allow(relation_class).to receive(:attribute_method?).and_return(true)
+    expect(subject).to eq(post_safe_hash)
+  end
+
+  it 'removes attributes not present in relation_class' do
+    expect(subject.keys).not_to include 'non_existent_attr'
+  end
+
+  it 'removes excluded keys from the hash' do
+    expect(subject.keys).not_to include excluded_keys
+  end
+
+  it 'does not remove excluded key if not listed' do
     parsed_hash = described_class.clean(relation_hash: unsafe_hash, relation_class: relation_class)
 
-    expect(parsed_hash).to eq(post_safe_hash)
+    expect(parsed_hash.keys).to eq post_safe_hash.keys + excluded_keys
   end
 end

spec/lib/gitlab/import_export/project.json

 {
   "description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
+  "import_type": "gitlab_project",
+  "creator_id": 123,
   "visibility_level": 10,
   "archived": false,
   "labels": [

spec/lib/gitlab/import_export/project.light.json

 {
   "description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
+  "import_type": "gitlab_project",
+  "creator_id": 123,
   "visibility_level": 10,
   "archived": false,
   "milestones": [

spec/lib/gitlab/import_export/project_tree_restorer_spec.rb

@@ -23,6 +23,10 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
         allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch)
 
         project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project)
+
+        expect(Gitlab::ImportExport::RelationFactory).to receive(:create).with(hash_including(excluded_keys: ['whatever'])).and_call_original.at_least(:once)
+        allow(project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(['whatever'])
+
         @restored_project_json = project_tree_restorer.restore
       end
     end
@@ -248,6 +252,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
       expect(labels.where(type: "ProjectLabel").count).to eq(results.fetch(:first_issue_labels, 0))
       expect(labels.where(type: "ProjectLabel").where.not(group_id: nil).count).to eq(0)
     end
+
+    it 'does not set params that are excluded from import_export settings' do
+      expect(project.import_type).to be_nil
+      expect(project.creator_id).not_to eq 123
+    end
   end
 
   shared_examples 'restores group correctly' do |**results|

spec/lib/gitlab/import_export/relation_factory_spec.rb

@@ -4,12 +4,14 @@ describe Gitlab::ImportExport::RelationFactory do
   let(:project) { create(:project) }
   let(:members_mapper) { double('members_mapper').as_null_object }
   let(:user) { create(:admin) }
+  let(:excluded_keys) { [] }
   let(:created_object) do
     described_class.create(relation_sym: relation_sym,
                            relation_hash: relation_hash,
                            members_mapper: members_mapper,
                            user: user,
-                           project: project)
+                           project: project,
+                           excluded_keys: excluded_keys)
   end
 
   context 'hook object' do
@@ -67,6 +69,14 @@ describe Gitlab::ImportExport::RelationFactory do
         expect(created_object.service_id).not_to eq(service_id)
       end
     end
+
+    context 'excluded attributes' do
+      let(:excluded_keys) { %w[url] }
+
+      it 'are removed from the imported object' do
+        expect(created_object.url).to be_nil
+      end
+    end
   end
 
   # Mocks an ActiveRecordish object with the dodgy columns

spec/requests/api/deploy_keys_spec.rb

@@ -171,7 +171,7 @@ describe API::DeployKeys do
       deploy_key
     end
 
-    it 'deletes existing key' do
+    it 'removes existing key from project' do
       expect do
         delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin)
 
@@ -179,6 +179,44 @@ describe API::DeployKeys do
       end.to change { project.deploy_keys.count }.by(-1)
     end
 
+    context 'when the deploy key is public' do
+      it 'does not delete the deploy key' do
+        expect do
+          delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin)
+
+          expect(response).to have_gitlab_http_status(204)
+        end.not_to change { DeployKey.count }
+      end
+    end
+
+    context 'when the deploy key is not public' do
+      let!(:deploy_key) { create(:deploy_key, public: false) }
+
+      context 'when the deploy key is only used by this project' do
+        it 'deletes the deploy key' do
+          expect do
+            delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin)
+
+            expect(response).to have_gitlab_http_status(204)
+          end.to change { DeployKey.count }.by(-1)
+        end
+      end
+
+      context 'when the deploy key is used by other projects' do
+        before do
+          create(:deploy_keys_project, project: project2, deploy_key: deploy_key)
+        end
+
+        it 'does not delete the deploy key' do
+          expect do
+            delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin)
+
+            expect(response).to have_gitlab_http_status(204)
+          end.not_to change { DeployKey.count }
+        end
+      end
+    end
+
     it 'returns 404 Not Found with invalid ID' do
       delete api("/projects/#{project.id}/deploy_keys/404", admin)