diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 80b9bdc8f24fa98a7e33ad3b63a2865cdeee1a54..abc57b6ddc5fc903ac7522c53e797580e28c48c8 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -78,7 +78,7 @@ module NotesActions # rubocop:disable Gitlab/ModuleWithInstanceVariables def update - @note = Notes::UpdateService.new(project, current_user, note_params).execute(note) + @note = Notes::UpdateService.new(project, current_user, update_note_params).execute(note) prepare_notes_for_rendering([@note]) respond_to do |format| @@ -216,6 +216,10 @@ module NotesActions ) end + def update_note_params + params.require(:note).permit(:note) + end + def set_polling_interval_header Gitlab::PollingInterval.set_header(response, interval: 6_000) end diff --git a/changelogs/unreleased/security-57153-comments-on-confidential-issues.yml b/changelogs/unreleased/security-57153-comments-on-confidential-issues.yml new file mode 100644 index 0000000000000000000000000000000000000000..62ee0fddfd9e2d76afa17a29e826e0368e013dce --- /dev/null +++ b/changelogs/unreleased/security-57153-comments-on-confidential-issues.yml @@ -0,0 +1,5 @@ +--- +title: Only allow modification of content when note is edited +merge_request: +author: +type: security diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index ec91a760388dad803c363110508565301b865c74..5ea4d0b79535a39168a5cdffe6d75d84ea7c2bfb 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -431,28 +431,77 @@ describe Projects::NotesController do end describe 'PUT update' do - context "should update the note with a valid issue" do - let(:request_params) do - { - namespace_id: project.namespace, - project_id: project, - id: note, - format: :json, - note: { - note: "New comment" + context "updates the note" do + context 'with a valid issue' do + let(:request_params) do + { + namespace_id: project.namespace, + project_id: project, + id: note, + format: :json, + note: { + note: "New comment" + } } - } - end + end - before do - sign_in(note.author) - project.add_developer(note.author) + before do + sign_in(note.author) + project.add_developer(note.author) + end + + it "updates the note content" do + expect { put :update, params: request_params }.to change { note.reload.note } + end end - it "updates the note" do - expect { put :update, params: request_params }.to change { note.reload.note } + context "when the note is edited and a different issue is targeted" do + ## + # We are editing a note originally in a public issue of a public project, + # but the edit can be intercepted to change the target to a different, even confidential, issue + # see https://gitlab.com/gitlab-org/gitlab-ce/issues/57153 + ## + + let!(:confidential_issue) { create(:issue, :confidential, project: project) } + let(:new_content) { "splendiferous new content" } + let(:request_params) do + { + namespace_id: project.namespace, + project_id: project, + id: note, + format: :json, + note: { + note: new_content, + noteable_id: confidential_issue.id + } + } + end + + before do + sign_in(note.author) + project.add_developer(note.author) + + put :update, params: request_params + end + + it 'returns success' do + expect(response.status).to eq 200 + end + + it 'edits the note content' do + expect(note.reload.note).to eq new_content + end + + it 'does not create a note in the confidential issue' do + expect(confidential_issue.reload.notes).to be_empty + end + + it "does not modify the note's issue" do + expect(note.noteable_id).to match note.reload.noteable_id + end end end + context "doesnt update the note" do let(:issue) { create(:issue, :confidential, project: project) } let(:note) { create(:note, noteable: issue, project: project) }