Merge branch 'jej-22869' into 'security'

Fix information disclosure in `Projects::BlobController#update`

## What does this MR do?

It was possible to discover private project names by modifying `from_merge_request`parameter in `Projects::BlobController#update`. This fixes that.

## Does this MR meet the acceptance criteria?

- [ ] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG.md) entry added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/22869

See merge request !2023
Signed-off-by: Rémy Coutable <remy@rymai.me>

app/controllers/projects/blob_controller.rb

@@ -13,7 +13,6 @@ class Projects::BlobController < Projects::ApplicationController
   before_action :assign_blob_vars
   before_action :commit, except: [:new, :create]
   before_action :blob, except: [:new, :create]
-  before_action :from_merge_request, only: [:edit, :update]
   before_action :require_branch_head, only: [:edit, :update]
   before_action :editor_variables, except: [:show, :preview, :diff]
   before_action :validate_diff_params, only: :diff
@@ -39,14 +38,6 @@ class Projects::BlobController < Projects::ApplicationController
 
   def update
     @path = params[:file_path] if params[:file_path].present?
-    after_edit_path =
-      if from_merge_request && @target_branch == @ref
-        diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) +
-          "#file-path-#{hexdigest(@path)}"
-      else
-        namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path))
-      end
-
     create_commit(Files::UpdateService, success_path: after_edit_path,
                                         failure_view: :edit,
                                         failure_path: namespace_project_blob_path(@project.namespace, @project, @id))
@@ -124,9 +115,14 @@ class Projects::BlobController < Projects::ApplicationController
     render_404
   end
 
-  def from_merge_request
-    # If blob edit was initiated from merge request page
-    @from_merge_request ||= MergeRequest.find_by(id: params[:from_merge_request_id])
+  def after_edit_path
+    from_merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.find_by(iid: params[:from_merge_request_iid])
+    if from_merge_request && @target_branch == @ref
+      diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) +
+        "#file-path-#{hexdigest(@path)}"
+    else
+      namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path))
+    end
   end
 
   def editor_variables

app/views/projects/blob/edit.html.haml

@@ -27,5 +27,5 @@
       = render 'shared/new_commit_form', placeholder: "Update #{@blob.name}"
       = hidden_field_tag 'last_commit_sha', @last_commit_sha
       = hidden_field_tag 'content', '', id: "file-content"
-      = hidden_field_tag 'from_merge_request_id', params[:from_merge_request_id]
+      = hidden_field_tag 'from_merge_request_iid', params[:from_merge_request_iid]
       = render 'projects/commit_button', ref: @ref, cancel_path: namespace_project_blob_path(@project.namespace, @project, @id)

app/views/projects/diffs/_file.html.haml

@@ -10,7 +10,7 @@
           \
           = clipboard_button(clipboard_text: diff_file.new_path, class: 'btn-file-option')
           - if editable_diff?(diff_file)
-            - link_opts = @merge_request.id ? { from_merge_request_id: @merge_request.id } : {}
+            - link_opts = @merge_request.persisted? ? { from_merge_request_iid: @merge_request.iid } : {}
             = edit_blob_link(@merge_request.source_project, @merge_request.source_branch, diff_file.new_path,
                              blob: blob, link_opts: link_opts)
 

changelogs/unreleased/jej-22869.yml

+---
+title: 'Fix information disclosure in `Projects::BlobController#update`'
+merge_request:
+author:

spec/controllers/projects/blob_controller_spec.rb

@@ -36,4 +36,53 @@ describe Projects::BlobController do
       end
     end
   end
+
+  describe 'PUT update' do
+    let(:default_params) do
+      {
+        namespace_id: project.namespace.to_param,
+        project_id: project.to_param,
+        id: 'master/CHANGELOG',
+        target_branch: 'master',
+        content: 'Added changes',
+        commit_message: 'Update CHANGELOG'
+      }
+    end
+
+    def blob_after_edit_path
+      namespace_project_blob_path(project.namespace, project, 'master/CHANGELOG')
+    end
+
+    it 'redirects to blob' do
+      put :update, default_params
+
+      expect(response).to redirect_to(blob_after_edit_path)
+    end
+
+    context '?from_merge_request_iid' do
+      let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+      let(:mr_params) { default_params.merge(from_merge_request_iid: merge_request.iid) }
+
+      it 'redirects to MR diff' do
+        put :update, mr_params
+
+        after_edit_path = diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
+        file_anchor = "#file-path-#{Digest::SHA1.hexdigest('CHANGELOG')}"
+        expect(response).to redirect_to(after_edit_path + file_anchor)
+      end
+
+      context "when user doesn't have access" do
+        before do
+          other_project = create(:empty_project)
+          merge_request.update!(source_project: other_project, target_project: other_project)
+        end
+
+        it "it redirect to blob" do
+          put :update, mr_params
+
+          expect(response).to redirect_to(blob_after_edit_path)
+        end
+      end
+    end
+  end
 end

spec/features/projects/blobs/edit_spec.rb

+require 'spec_helper'
+
+feature 'Editing file blob', feature: true, js: true do
+  include WaitForAjax
+
+  given(:user) { create(:user) }
+  given(:role) { :developer }
+  given(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') }
+  given(:project) { merge_request.target_project }
+
+  background do
+    login_as(user)
+    project.team << [user, role]
+  end
+
+  def edit_and_commit
+    wait_for_ajax
+    first('.file-actions').click_link 'Edit'
+    execute_script('ace.edit("editor").setValue("class NextFeature\nend\n")')
+    click_button 'Commit Changes'
+  end
+
+  context 'from MR diff' do
+    before do
+      visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
+      edit_and_commit
+    end
+
+    scenario 'returns me to the mr' do
+      expect(page).to have_content(merge_request.title)
+    end
+  end
+
+  context 'from blob file path' do
+    before do
+      visit namespace_project_blob_path(project.namespace, project, '/feature/files/ruby/feature.rb')
+      edit_and_commit
+    end
+
+    scenario 'updates content' do
+      expect(page).to have_content 'successfully committed'
+      expect(page).to have_content 'NextFeature'
+    end
+  end
+end