Commit aee1cf9a authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

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

Security fix sentry issue leaks and access level check

See merge request gitlab-org/security/gitlab!2501
parents 08b3c7ea c644d94f
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,
......
......@@ -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
......
......@@ -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],
......
......@@ -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,
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
# 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
1f03beba0775e2a4eead512819592f590b02b70096cee250dfcdf426440cb5f5
\ No newline at end of file
......@@ -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 (
......@@ -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
......
......@@ -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
......
......@@ -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'
......
......@@ -297,14 +297,27 @@ RSpec.describe Projects::ErrorTrackingController do
put :update, params: issue_params(issue_id: issue_id, status: 'resolved', format: :json)
end
describe 'format json' do
context 'when user is a reporter' do
before do
project.add_reporter(user)
end
it 'returns 404 error' do
update_issue
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when user has access to update' do
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 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)
......@@ -335,6 +348,7 @@ RSpec.describe Projects::ErrorTrackingController do
end
end
end
end
private
......
......@@ -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],
......
......@@ -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' }
......
......@@ -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 }
......
......@@ -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,
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
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