Commit 0dc6871f authored by Cindy Pallares's avatar Cindy Pallares

Merge branch 'security-IDOR-note-drafts-publish' into 'master'

[master] Fix IDOR at /drafts/publish/

Closes #356

See merge request gitlab/gitlab-ee!710
parents b22df50e 92e2edec
...@@ -8,6 +8,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli ...@@ -8,6 +8,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
before_action :check_draft_notes_available!, except: [:index] before_action :check_draft_notes_available!, except: [:index]
before_action :authorize_create_draft!, only: [:create] before_action :authorize_create_draft!, only: [:create]
before_action :authorize_admin_draft!, only: [:update, :destroy] before_action :authorize_admin_draft!, only: [:update, :destroy]
before_action :authorize_admin_draft!, only: [:publish], if: -> { params[:id].present? }
def index def index
drafts = prepare_notes_for_rendering(draft_notes) drafts = prepare_notes_for_rendering(draft_notes)
...@@ -40,7 +41,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli ...@@ -40,7 +41,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
end end
def publish 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 head :ok
end end
...@@ -53,10 +54,13 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli ...@@ -53,10 +54,13 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
private private
def draft_note def draft_note(allow_nil: false)
strong_memoize(:draft_note) do strong_memoize(:draft_note) do
draft_notes.try(:find, params[:id]) draft_notes.find(params[:id])
end end
rescue ActiveRecord::RecordNotFound => ex
# draft_note is allowed to be nil in #publish
raise ex unless allow_nil
end end
def draft_notes def draft_notes
......
...@@ -2,9 +2,9 @@ ...@@ -2,9 +2,9 @@
module DraftNotes module DraftNotes
class PublishService < DraftNotes::BaseService class PublishService < DraftNotes::BaseService
def execute(draft_id = nil) def execute(draft = nil)
if draft_id if draft
publish_draft_note(draft_id) publish_draft_note(draft)
else else
publish_draft_notes publish_draft_notes
end end
...@@ -12,9 +12,7 @@ module DraftNotes ...@@ -12,9 +12,7 @@ module DraftNotes
private private
def publish_draft_note(draft_id) def publish_draft_note(draft)
draft = DraftNote.find(draft_id)
create_note_from_draft(draft) create_note_from_draft(draft)
draft.delete draft.delete
......
---
title: Fix IDOR at /drafts/publish
merge_request:
author:
type: security
...@@ -153,6 +153,7 @@ describe Projects::MergeRequests::DraftsController do ...@@ -153,6 +153,7 @@ describe Projects::MergeRequests::DraftsController do
context 'without permissions' do context 'without permissions' do
before do before do
sign_in(user2) sign_in(user2)
project.add_developer(user2)
end end
it 'does not allow editing draft note belonging to someone else' do it 'does not allow editing draft note belonging to someone else' do
...@@ -176,6 +177,22 @@ describe Projects::MergeRequests::DraftsController do ...@@ -176,6 +177,22 @@ describe Projects::MergeRequests::DraftsController do
end end
describe 'POST #publish' do 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 it 'publishes draft notes with position' do
diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs) diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs)
......
...@@ -6,14 +6,14 @@ describe DraftNotes::PublishService do ...@@ -6,14 +6,14 @@ describe DraftNotes::PublishService do
let(:project) { merge_request.target_project } let(:project) { merge_request.target_project }
let(:user) { merge_request.author } let(:user) { merge_request.author }
def publish(id: nil) def publish(draft: nil)
DraftNotes::PublishService.new(merge_request, user).execute(id) DraftNotes::PublishService.new(merge_request, user).execute(draft)
end end
it 'publishes a single draft note' do it 'publishes a single draft note' do
drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user) 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) expect(DraftNote.count).to eq(1)
end end
...@@ -58,7 +58,7 @@ describe DraftNotes::PublishService do ...@@ -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) } 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 it 'resolves the discussion' do
publish(id: draft_note.id) publish(draft: draft_note)
expect(note.discussion.resolved?).to be true expect(note.discussion.resolved?).to be true
end 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