Commit 92e2edec authored by Mario de la Ossa's avatar Mario de la Ossa

Fix IDOR in draft notes publishing

parent 7e45c1ac
......@@ -8,6 +8,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
before_action :check_draft_notes_available!, except: [:index]
before_action :authorize_create_draft!, only: [:create]
before_action :authorize_admin_draft!, only: [:update, :destroy]
before_action :authorize_admin_draft!, only: [:publish], if: -> { params[:id].present? }
def index
drafts = prepare_notes_for_rendering(draft_notes)
......@@ -40,7 +41,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
end
def publish
DraftNotes::PublishService.new(merge_request, current_user).execute(params[:id])
DraftNotes::PublishService.new(merge_request, current_user).execute(draft_note(allow_nil: true))
head :ok
end
......@@ -53,10 +54,13 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
private
def draft_note
def draft_note(allow_nil: false)
strong_memoize(:draft_note) do
draft_notes.try(:find, params[:id])
draft_notes.find(params[:id])
end
rescue ActiveRecord::RecordNotFound => ex
# draft_note is allowed to be nil in #publish
raise ex unless allow_nil
end
def draft_notes
......
......@@ -2,9 +2,9 @@
module DraftNotes
class PublishService < DraftNotes::BaseService
def execute(draft_id = nil)
if draft_id
publish_draft_note(draft_id)
def execute(draft = nil)
if draft
publish_draft_note(draft)
else
publish_draft_notes
end
......@@ -12,9 +12,7 @@ module DraftNotes
private
def publish_draft_note(draft_id)
draft = DraftNote.find(draft_id)
def publish_draft_note(draft)
create_note_from_draft(draft)
draft.delete
......
---
title: Fix IDOR at /drafts/publish
merge_request:
author:
type: security
......@@ -153,6 +153,7 @@ describe Projects::MergeRequests::DraftsController do
context 'without permissions' do
before do
sign_in(user2)
project.add_developer(user2)
end
it 'does not allow editing draft note belonging to someone else' do
......@@ -176,6 +177,22 @@ describe Projects::MergeRequests::DraftsController do
end
describe 'POST #publish' do
context 'without permissions' do
before do
sign_in(user2)
project.add_developer(user2)
end
it 'does not allow publishing draft note belonging to someone else' do
draft = create(:draft_note, merge_request: merge_request, author: user)
expect { post :publish, params.merge(id: draft.id) }.to change { Note.count }.by(0)
.and change { DraftNote.count }.by(0)
expect(response).to have_gitlab_http_status(404)
end
end
it 'publishes draft notes with position' do
diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs)
......
......@@ -6,14 +6,14 @@ describe DraftNotes::PublishService do
let(:project) { merge_request.target_project }
let(:user) { merge_request.author }
def publish(id: nil)
DraftNotes::PublishService.new(merge_request, user).execute(id)
def publish(draft: nil)
DraftNotes::PublishService.new(merge_request, user).execute(draft)
end
it 'publishes a single draft note' do
drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user)
expect { publish(id: drafts.first.id) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
expect(DraftNote.count).to eq(1)
end
......@@ -58,7 +58,7 @@ describe DraftNotes::PublishService do
let(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) }
it 'resolves the discussion' do
publish(id: draft_note.id)
publish(draft: draft_note)
expect(note.discussion.resolved?).to be true
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment