From 742cee756bf39d93fe5c7f207f8a54143ae6a384 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Mon, 7 Nov 2016 17:09:22 +0000
Subject: [PATCH] Merge branch 'jej-22869' into 'security'
Fix information disclosure in `Projects::BlobController#update`
It was possible to discover private project names by modifying `from_merge_request`parameter in `Projects::BlobController#update`. This fixes that.
- [ ] [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)
https://gitlab.com/gitlab-org/gitlab-ce/issues/22869
See merge request !2023
---
app/controllers/projects/blob_controller.rb | 20 +++-----
app/views/projects/blob/edit.html.haml | 2 +-
app/views/projects/diffs/_file.html.haml | 2 +-
changelogs/unreleased/jej-22869.yml | 4 ++
.../projects/blob_controller_spec.rb | 49 +++++++++++++++++++
spec/features/projects/blobs/edit_spec.rb | 45 +++++++++++++++++
6 files changed, 108 insertions(+), 14 deletions(-)
create mode 100644 changelogs/unreleased/jej-22869.yml
create mode 100644 spec/features/projects/blobs/edit_spec.rb
diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb
index 56ced786311..9940263ae24 100644
--- a/app/controllers/projects/blob_controller.rb
+++ b/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) +
- "##{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) +
+ "##{hexdigest(@path)}"
+ else
+ namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path))
+ end
end
def editor_variables
diff --git a/app/views/projects/blob/edit.html.haml b/app/views/projects/blob/edit.html.haml
index 2a0352a71b7..a5dcd93f42e 100644
--- a/app/views/projects/blob/edit.html.haml
+++ b/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)
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index 120ba9ffcd2..6c33d80becd 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -9,7 +9,7 @@
= icon('comment')
\
- 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)
diff --git a/changelogs/unreleased/jej-22869.yml b/changelogs/unreleased/jej-22869.yml
new file mode 100644
index 00000000000..9d2edcfee42
--- /dev/null
+++ b/changelogs/unreleased/jej-22869.yml
@@ -0,0 +1,4 @@
+---
+title: Fix information disclosure in `Projects::BlobController#update`
+merge_request:
+author:
diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb
index 52d13fb6f9e..1c2b0a4a45c 100644
--- a/spec/controllers/projects/blob_controller_spec.rb
+++ b/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
diff --git a/spec/features/projects/blobs/edit_spec.rb b/spec/features/projects/blobs/edit_spec.rb
new file mode 100644
index 00000000000..a820d07ab3b
--- /dev/null
+++ b/spec/features/projects/blobs/edit_spec.rb
@@ -0,0 +1,45 @@
+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
--
GitLab