Correctly check permissions when creating snippet notes
In the Snippets::NotesController the noteable was resolved and authorized through the :snippet_id, so by passing a :target_id for a different snippet it was possible to create a note on a snippet where the user would be unauthorized to do so otherwise. This fixes the problem by ignoring the :target_id and :target_type from the request, and using the same noteable for creation and authorization.
This commit is contained in:
parent
11bb3b53bc
commit
12d7b3937f
|
@ -203,17 +203,17 @@ module NotesActions
|
|||
|
||||
# These params are also sent by the client but we need to set these based on
|
||||
# target_type and target_id because we're checking permissions based on that
|
||||
create_params[:noteable_type] = params[:target_type].classify
|
||||
create_params[:noteable_type] = noteable.class.name
|
||||
|
||||
case params[:target_type]
|
||||
when 'commit'
|
||||
create_params[:commit_id] = params[:target_id]
|
||||
when 'merge_request'
|
||||
create_params[:noteable_id] = params[:target_id]
|
||||
case noteable
|
||||
when Commit
|
||||
create_params[:commit_id] = noteable.id
|
||||
when MergeRequest
|
||||
create_params[:noteable_id] = noteable.id
|
||||
# Notes on MergeRequest can have an extra `commit_id` context
|
||||
create_params[:commit_id] = params.dig(:note, :commit_id)
|
||||
else
|
||||
create_params[:noteable_id] = params[:target_id]
|
||||
create_params[:noteable_id] = noteable.id
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -5,8 +5,8 @@ class Snippets::NotesController < ApplicationController
|
|||
include ToggleAwardEmoji
|
||||
|
||||
skip_before_action :authenticate_user!, only: [:index]
|
||||
before_action :snippet
|
||||
before_action :authorize_read_snippet!, only: [:show, :index, :create]
|
||||
before_action :authorize_read_snippet!, only: [:show, :index]
|
||||
before_action :authorize_create_note!, only: [:create]
|
||||
|
||||
private
|
||||
|
||||
|
@ -33,4 +33,8 @@ class Snippets::NotesController < ApplicationController
|
|||
def authorize_read_snippet!
|
||||
return render_404 unless can?(current_user, :read_personal_snippet, snippet)
|
||||
end
|
||||
|
||||
def authorize_create_note!
|
||||
access_denied! unless can?(current_user, :create_note, noteable)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Correctly check permissions when creating snippet notes
|
||||
merge_request:
|
||||
author:
|
||||
type: security
|
|
@ -252,7 +252,7 @@ describe Projects::NotesController do
|
|||
before do
|
||||
service_params = ActionController::Parameters.new({
|
||||
note: 'some note',
|
||||
noteable_id: merge_request.id.to_s,
|
||||
noteable_id: merge_request.id,
|
||||
noteable_type: 'MergeRequest',
|
||||
commit_id: nil,
|
||||
merge_request_diff_head_sha: 'sha'
|
||||
|
|
|
@ -119,6 +119,119 @@ describe Snippets::NotesController do
|
|||
end
|
||||
end
|
||||
|
||||
describe 'POST create' do
|
||||
context 'when a snippet is public' do
|
||||
let(:request_params) do
|
||||
{
|
||||
note: attributes_for(:note_on_personal_snippet, noteable: public_snippet),
|
||||
snippet_id: public_snippet.id
|
||||
}
|
||||
end
|
||||
|
||||
before do
|
||||
sign_in user
|
||||
end
|
||||
|
||||
it 'returns status 302' do
|
||||
post :create, params: request_params
|
||||
|
||||
expect(response).to have_gitlab_http_status(302)
|
||||
end
|
||||
|
||||
it 'creates the note' do
|
||||
expect { post :create, params: request_params }.to change { Note.count }.by(1)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when a snippet is internal' do
|
||||
let(:request_params) do
|
||||
{
|
||||
note: attributes_for(:note_on_personal_snippet, noteable: internal_snippet),
|
||||
snippet_id: internal_snippet.id
|
||||
}
|
||||
end
|
||||
|
||||
before do
|
||||
sign_in user
|
||||
end
|
||||
|
||||
it 'returns status 302' do
|
||||
post :create, params: request_params
|
||||
|
||||
expect(response).to have_gitlab_http_status(302)
|
||||
end
|
||||
|
||||
it 'creates the note' do
|
||||
expect { post :create, params: request_params }.to change { Note.count }.by(1)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when a snippet is private' do
|
||||
let(:request_params) do
|
||||
{
|
||||
note: attributes_for(:note_on_personal_snippet, noteable: private_snippet),
|
||||
snippet_id: private_snippet.id
|
||||
}
|
||||
end
|
||||
|
||||
before do
|
||||
sign_in user
|
||||
end
|
||||
|
||||
context 'when user is not the author' do
|
||||
before do
|
||||
sign_in(user)
|
||||
end
|
||||
|
||||
it 'returns status 404' do
|
||||
post :create, params: request_params
|
||||
|
||||
expect(response).to have_gitlab_http_status(404)
|
||||
end
|
||||
|
||||
it 'does not create the note' do
|
||||
expect { post :create, params: request_params }.not_to change { Note.count }
|
||||
end
|
||||
|
||||
context 'when user sends a snippet_id for a public snippet' do
|
||||
let(:request_params) do
|
||||
{
|
||||
note: attributes_for(:note_on_personal_snippet, noteable: private_snippet),
|
||||
snippet_id: public_snippet.id
|
||||
}
|
||||
end
|
||||
|
||||
it 'returns status 302' do
|
||||
post :create, params: request_params
|
||||
|
||||
expect(response).to have_gitlab_http_status(302)
|
||||
end
|
||||
|
||||
it 'creates the note on the public snippet' do
|
||||
expect { post :create, params: request_params }.to change { Note.count }.by(1)
|
||||
expect(Note.last.noteable).to eq public_snippet
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when user is the author' do
|
||||
before do
|
||||
sign_in(private_snippet.author)
|
||||
end
|
||||
|
||||
it 'returns status 302' do
|
||||
post :create, params: request_params
|
||||
|
||||
expect(response).to have_gitlab_http_status(302)
|
||||
end
|
||||
|
||||
it 'creates the note' do
|
||||
expect { post :create, params: request_params }.to change { Note.count }.by(1)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe 'DELETE destroy' do
|
||||
let(:request_params) do
|
||||
{
|
||||
|
|
Loading…
Reference in New Issue