Security fix sentry issue leaks and access level check

Merge branch 'security-fix-sentry-issue-leaks-and-access-level-check-14-10' into '14-10-stable-ee'

See merge request gitlab-org/security/gitlab!2501

Changelog: security

app/assets/javascripts/error_tracking_settings/utils.js

-export const projectKeys = ['name', 'organizationName', 'organizationSlug', 'slug'];
+export const projectKeys = ['id', 'name', 'organizationName', 'organizationSlug', 'slug'];
 
 export const transformFrontendSettings = ({
   apiHost,
@@ -9,6 +9,7 @@ export const transformFrontendSettings = ({
 }) => {
   const project = selectedProject
     ? {
+        sentry_project_id: selectedProject.id,
         slug: selectedProject.slug,
         name: selectedProject.name,
         organization_name: selectedProject.organizationName,

app/controllers/projects/error_tracking_controller.rb

@@ -4,6 +4,7 @@ class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseControlle
   respond_to :json
 
   before_action :authorize_read_sentry_issue!
+  before_action :authorize_update_sentry_issue!, only: %i[update]
   before_action :set_issue_id, only: :details
 
   before_action only: [:index] do

app/controllers/projects/settings/operations_controller.rb

@@ -143,7 +143,7 @@ module Projects
             :integrated,
             :api_host,
             :token,
-            project: [:slug, :name, :organization_slug, :organization_name]
+            project: [:slug, :name, :organization_slug, :organization_name, :sentry_project_id]
           ],
 
           grafana_integration_attributes: [:token, :grafana_url, :enabled],

app/helpers/projects_helper.rb

@@ -298,6 +298,7 @@ module ProjectsHelper
         setting.organization_slug.blank?
 
     {
+      sentry_project_id: setting.sentry_project_id,
       name: setting.project_name,
       organization_name: setting.organization_name,
       organization_slug: setting.organization_slug,

app/models/error_tracking/project_error_tracking_setting.rb

@@ -125,17 +125,22 @@ module ErrorTracking
 
     def issue_details(opts = {})
       with_reactive_cache('issue_details', opts.stringify_keys) do |result|
+        ensure_issue_belongs_to_project!(result[:issue].project_id)
         result
       end
     end
 
     def issue_latest_event(opts = {})
       with_reactive_cache('issue_latest_event', opts.stringify_keys) do |result|
+        ensure_issue_belongs_to_project!(result[:latest_event].project_id)
         result
       end
     end
 
     def update_issue(opts = {})
+      issue_to_be_updated = sentry_client.issue_details(issue_id: opts[:issue_id])
+      ensure_issue_belongs_to_project!(issue_to_be_updated.project_id)
+
       handle_exceptions do
         { updated: sentry_client.update_issue(opts) }
       end
@@ -177,6 +182,25 @@ module ErrorTracking
 
     private
 
+    def ensure_issue_belongs_to_project!(project_id_from_api)
+      raise 'The Sentry issue appers to be outside of the configured Sentry project' if Integer(project_id_from_api) != ensure_sentry_project_id!
+    end
+
+    def ensure_sentry_project_id!
+      return sentry_project_id if sentry_project_id.present?
+
+      raise("Couldn't find project: #{organization_name} / #{project_name} on Sentry") if sentry_project.nil?
+
+      update!(sentry_project_id: sentry_project.id)
+      sentry_project_id
+    end
+
+    def sentry_project
+      strong_memoize(:sentry_project) do
+        sentry_client.projects.find { |project| project.name == project_name && project.organization_name == organization_name }
+      end
+    end
+
     def add_gitlab_issue_details(issue)
       issue.gitlab_commit = match_gitlab_commit(issue.first_release_version)
       issue.gitlab_commit_path = project_commit_path(project, issue.gitlab_commit) if issue.gitlab_commit

app/policies/project_policy.rb

@@ -297,7 +297,6 @@ class ProjectPolicy < BasePolicy
     enable :read_deployment
     enable :read_merge_request
     enable :read_sentry_issue
-    enable :update_sentry_issue
     enable :read_prometheus
     enable :read_metrics_dashboard_annotation
     enable :metrics_dashboard
@@ -413,6 +412,7 @@ class ProjectPolicy < BasePolicy
     enable :admin_feature_flags_user_lists
     enable :update_escalation_status
     enable :read_secure_files
+    enable :update_sentry_issue
   end
 
   rule { can?(:developer_access) & user_confirmed? }.policy do

app/services/projects/operations/update_service.rb

@@ -90,7 +90,8 @@ module Projects
             api_url: api_url,
             enabled: settings[:enabled],
             project_name: settings.dig(:project, :name),
-            organization_name: settings.dig(:project, :organization_name)
+            organization_name: settings.dig(:project, :organization_name),
+            sentry_project_id: settings.dig(:project, :sentry_project_id)
           }
         }
         params[:error_tracking_setting_attributes][:token] = settings[:token] unless /\A\*+\z/.match?(settings[:token]) # Don't update token if we receive masked value

db/migrate/20220525084153_add_sentry_project_id_to_project_error_tracking_settings.rb

+# frozen_string_literal: true
+
+class AddSentryProjectIdToProjectErrorTrackingSettings < Gitlab::Database::Migration[2.0]
+  def change
+    add_column :project_error_tracking_settings, :sentry_project_id, :bigint
+  end
+end

db/schema_migrations/20220525084153

+1f03beba0775e2a4eead512819592f590b02b70096cee250dfcdf426440cb5f5
\ No newline at end of file

db/structure.sql

@@ -19156,7 +19156,8 @@ CREATE TABLE project_error_tracking_settings (
     encrypted_token_iv character varying,
     project_name character varying,
     organization_name character varying,
-    integrated boolean DEFAULT true NOT NULL
+    integrated boolean DEFAULT true NOT NULL,
+    sentry_project_id bigint
 );
 
 CREATE TABLE project_export_jobs (

doc/operations/error_tracking.md

@@ -106,7 +106,7 @@ button and a link to the GitLab issue displays within the error detail section.
 
 ## Taking Action on errors
 
-You can take action on Sentry Errors from within the GitLab UI.
+You can take action on Sentry Errors from within the GitLab UI. Marking errors ignored or resolved require at least Developer role.
 
 ### Ignoring errors
 

lib/error_tracking/sentry_client/event.rb

@@ -15,6 +15,7 @@ module ErrorTracking
         stack_trace = parse_stack_trace(event)
 
         Gitlab::ErrorTracking::ErrorEvent.new(
+          project_id: event['projectID'],
           issue_id: event['groupID'],
           date_received: event['dateReceived'],
           stack_trace_entries: stack_trace

lib/gitlab/error_tracking/error_event.rb

@@ -7,7 +7,7 @@ module Gitlab
     class ErrorEvent
       include ActiveModel::Model
 
-      attr_accessor :issue_id, :date_received, :stack_trace_entries, :gitlab_project
+      attr_accessor :issue_id, :date_received, :stack_trace_entries, :gitlab_project, :project_id
 
       def self.declarative_policy_class
         'ErrorTracking::BasePolicy'

spec/controllers/projects/error_tracking_controller_spec.rb

@@ -297,40 +297,54 @@ RSpec.describe Projects::ErrorTrackingController do
       put :update, params: issue_params(issue_id: issue_id, status: 'resolved', format: :json)
     end
 
-    before do
-      expect(ErrorTracking::IssueUpdateService)
-        .to receive(:new).with(project, user, permitted_params)
-        .and_return(issue_update_service)
-    end
-
     describe 'format json' do
-      context 'update result is successful' do
+      context 'when user is a reporter' do
         before do
-          expect(issue_update_service).to receive(:execute)
-            .and_return(status: :success, updated: true, closed_issue_iid: non_existing_record_iid)
+          project.add_reporter(user)
+        end
 
+        it 'returns 404 error' do
           update_issue
-        end
 
-        it 'returns a success' do
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(response).to match_response_schema('error_tracking/update_issue')
+          expect(response).to have_gitlab_http_status(:not_found)
         end
       end
 
-      context 'update result is erroneous' do
-        let(:error_message) { 'error message' }
-
+      context 'when user has access to update' do
         before do
-          expect(issue_update_service).to receive(:execute)
-            .and_return(status: :error, message: error_message)
+          expect(ErrorTracking::IssueUpdateService)
+            .to receive(:new).with(project, user, permitted_params)
+            .and_return(issue_update_service)
+        end
 
-          update_issue
+        context 'when update result is successful' do
+          before do
+            expect(issue_update_service).to receive(:execute)
+              .and_return(status: :success, updated: true, closed_issue_iid: non_existing_record_iid)
+
+            update_issue
+          end
+
+          it 'returns a success' do
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(response).to match_response_schema('error_tracking/update_issue')
+          end
         end
 
-        it 'returns 400 with message' do
-          expect(response).to have_gitlab_http_status(:bad_request)
-          expect(json_response['message']).to eq(error_message)
+        context 'update result is erroneous' do
+          let(:error_message) { 'error message' }
+
+          before do
+            expect(issue_update_service).to receive(:execute)
+              .and_return(status: :error, message: error_message)
+
+            update_issue
+          end
+
+          it 'returns 400 with message' do
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(json_response['message']).to eq(error_message)
+          end
         end
       end
     end

spec/db/schema_spec.rb

@@ -72,6 +72,7 @@ RSpec.describe 'Database schema' do
     oauth_applications: %w[owner_id],
     product_analytics_events_experimental: %w[event_id txn_id user_id],
     project_build_artifacts_size_refreshes: %w[last_job_artifact_id],
+    project_error_tracking_settings: %w[sentry_project_id],
     project_group_links: %w[group_id],
     project_statistics: %w[namespace_id],
     projects: %w[creator_id ci_id mirror_user_id],

spec/factories/error_tracking/error.rb

@@ -13,7 +13,7 @@ FactoryBot.define do
     message { 'message' }
     culprit { 'culprit' }
     external_url { 'http://example.com/id' }
-    project_id { 'project1' }
+    project_id { '111111' }
     project_name { 'project name' }
     project_slug { 'project_name' }
     short_id { 'ID' }

spec/factories/project_error_tracking_settings.rb

@@ -9,6 +9,7 @@ FactoryBot.define do
     project_name { 'Sentry Project' }
     organization_name { 'Sentry Org' }
     integrated { false }
+    sentry_project_id { 10 }
 
     trait :disabled do
       enabled { false }

spec/helpers/projects_helper_spec.rb

@@ -52,6 +52,7 @@ RSpec.describe ProjectsHelper do
       context 'api_url present' do
         let(:json) do
           {
+            sentry_project_id: error_tracking_setting.sentry_project_id,
             name: error_tracking_setting.project_name,
             organization_name: error_tracking_setting.organization_name,
             organization_slug: error_tracking_setting.organization_slug,

spec/models/error_tracking/project_error_tracking_setting_spec.rb

@@ -10,7 +10,9 @@ RSpec.describe ErrorTracking::ProjectErrorTrackingSetting do
 
   let(:sentry_client) { instance_double(ErrorTracking::SentryClient) }
 
-  subject(:setting) { build(:project_error_tracking_setting, project: project) }
+  let(:sentry_project_id) { 10 }
+
+  subject(:setting) { build(:project_error_tracking_setting, project: project, sentry_project_id: sentry_project_id) }
 
   describe 'Associations' do
     it { is_expected.to belong_to(:project) }
@@ -270,7 +272,7 @@ RSpec.describe ErrorTracking::ProjectErrorTrackingSetting do
   end
 
   describe '#issue_details' do
-    let(:issue) { build(:error_tracking_sentry_detailed_error) }
+    let(:issue) { build(:error_tracking_sentry_detailed_error, project_id: sentry_project_id) }
     let(:commit_id) { issue.first_release_version }
     let(:result) { subject.issue_details(opts) }
     let(:opts) { { issue_id: 1 } }
@@ -317,12 +319,33 @@ RSpec.describe ErrorTracking::ProjectErrorTrackingSetting do
     end
   end
 
+  describe '#issue_latest_event' do
+    let(:error_event) { build(:error_tracking_sentry_error_event, project_id: sentry_project_id) }
+    let(:result) { subject.issue_latest_event(opts) }
+    let(:opts) { { issue_id: 1 } }
+
+    before do
+      stub_reactive_cache(subject, error_event, {})
+      synchronous_reactive_cache(subject)
+
+      allow(subject).to receive(:sentry_client).and_return(sentry_client)
+      allow(sentry_client).to receive(:issue_latest_event).with(opts).and_return(error_event)
+    end
+
+    it 'returns the error event' do
+      expect(result[:latest_event].project_id).to eq(sentry_project_id)
+    end
+  end
+
   describe '#update_issue' do
     let(:result) { subject.update_issue(**opts) }
     let(:opts) { { issue_id: 1, params: {} } }
 
     before do
       allow(subject).to receive(:sentry_client).and_return(sentry_client)
+      allow(sentry_client).to receive(:issue_details)
+        .with({ issue_id: 1 })
+        .and_return(Gitlab::ErrorTracking::DetailedError.new(project_id: sentry_project_id))
     end
 
     context 'when sentry response is successful' do
@@ -344,6 +367,56 @@ RSpec.describe ErrorTracking::ProjectErrorTrackingSetting do
         expect(result).to eq(error: 'Unexpected Error')
       end
     end
+
+    context 'when sentry_project_id is not set' do
+      let(:sentry_projects) do
+        [
+          Gitlab::ErrorTracking::Project.new(
+            id: 1111,
+            name: 'Some Project',
+            organization_name: 'Org'
+          ),
+          Gitlab::ErrorTracking::Project.new(
+            id: sentry_project_id,
+            name: setting.project_name,
+            organization_name: setting.organization_name
+          )
+        ]
+      end
+
+      context 'when sentry_project_id is not set' do
+        before do
+          setting.update!(sentry_project_id: nil)
+
+          allow(sentry_client).to receive(:projects).and_return(sentry_projects)
+          allow(sentry_client).to receive(:update_issue).with(opts).and_return(true)
+        end
+
+        it 'tries to backfill it from sentry API' do
+          expect(result).to eq(updated: true)
+
+          expect(setting.reload.sentry_project_id).to eq(sentry_project_id)
+        end
+
+        context 'when the project cannot be found on sentry' do
+          before do
+            sentry_projects.pop
+          end
+
+          it 'raises error' do
+            expect { result }.to raise_error(/Couldn't find project/)
+          end
+        end
+      end
+
+      context 'when mismatching sentry_project_id is detected' do
+        it 'raises error' do
+          setting.update!(sentry_project_id: sentry_project_id + 1)
+
+          expect { result }.to raise_error(/The Sentry issue appers to be outside/)
+        end
+      end
+    end
   end
 
   describe 'slugs' do

spec/policies/project_policy_spec.rb

@@ -2102,4 +2102,25 @@ RSpec.describe ProjectPolicy do
       it { is_expected.to be_disallowed(:register_project_runners) }
     end
   end
+
+  describe 'update_sentry_issue' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:role, :allowed) do
+      :owner      | true
+      :maintainer | true
+      :developer  | true
+      :reporter   | false
+      :guest      | false
+    end
+
+    let(:project) { public_project }
+    let(:current_user) { public_send(role) }
+
+    with_them do
+      it do
+        expect(subject.can?(:update_sentry_issue)).to be(allowed)
+      end
+    end
+  end
 end

spec/services/issues/create_service_spec.rb

@@ -294,7 +294,7 @@ RSpec.describe Issues::CreateService do
 
         context 'user is reporter or above' do
           before do
-            project.add_reporter(user)
+            project.add_developer(user)
           end
 
           it 'assigns the sentry error' do

spec/support/shared_contexts/sentry_error_tracking_shared_context.rb

@@ -16,6 +16,6 @@ RSpec.shared_context 'sentry error tracking context' do
   before do
     expect(project).to receive(:error_tracking_setting).at_least(:once).and_return(error_tracking_setting)
 
-    project.add_reporter(user)
+    project.add_developer(user)
   end
 end