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
This commit is contained in:
parent
3d7704ae5f
commit
742cee756b
|
@ -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
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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)
|
||||
|
||||
|
|
|
@ -0,0 +1,4 @@
|
|||
---
|
||||
title: Fix information disclosure in `Projects::BlobController#update`
|
||||
merge_request:
|
||||
author:
|
|
@ -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
|
||||
|
|
|
@ -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
|
Loading…
Reference in New Issue