Commit aaf27d14 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch '9224/create-mr-from-vulnerability-feedback' into 'master'

Create Merge Request From Remediated Vulnerability Solution

Closes #9224

See merge request gitlab-org/gitlab-ee!9833
parents e915e9a4 3ba0cbf9
......@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20190301081611) do
ActiveRecord::Schema.define(version: 20190301182031) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -3185,8 +3185,10 @@ ActiveRecord::Schema.define(version: 20190301081611) do
t.integer "pipeline_id"
t.integer "issue_id"
t.string "project_fingerprint", limit: 40, null: false
t.integer "merge_request_id"
t.index ["author_id"], name: "index_vulnerability_feedback_on_author_id", using: :btree
t.index ["issue_id"], name: "index_vulnerability_feedback_on_issue_id", using: :btree
t.index ["merge_request_id"], name: "index_vulnerability_feedback_on_merge_request_id", using: :btree
t.index ["pipeline_id"], name: "index_vulnerability_feedback_on_pipeline_id", using: :btree
t.index ["project_id", "category", "feedback_type", "project_fingerprint"], name: "vulnerability_feedback_unique_idx", unique: true, using: :btree
end
......@@ -3604,6 +3606,7 @@ ActiveRecord::Schema.define(version: 20190301081611) do
add_foreign_key "users_star_projects", "projects", name: "fk_22cd27ddfc", on_delete: :cascade
add_foreign_key "vulnerability_feedback", "ci_pipelines", column: "pipeline_id", on_delete: :nullify
add_foreign_key "vulnerability_feedback", "issues", on_delete: :nullify
add_foreign_key "vulnerability_feedback", "merge_requests", name: "fk_563ff1912e", on_delete: :nullify
add_foreign_key "vulnerability_feedback", "projects", on_delete: :cascade
add_foreign_key "vulnerability_feedback", "users", column: "author_id", on_delete: :cascade
add_foreign_key "vulnerability_identifiers", "projects", on_delete: :cascade
......
......@@ -102,6 +102,7 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
severity
solution
sourceid
target_branch
title
tool
tools
......@@ -129,6 +130,9 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
links: %i[
name
url
],
remediations: %i[
diff
]
]
end
......
......@@ -7,25 +7,27 @@ module Vulnerabilities
belongs_to :project
belongs_to :author, class_name: "User"
belongs_to :issue
belongs_to :merge_request
belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
attr_accessor :vulnerability_data
enum feedback_type: { dismissal: 0, issue: 1 }
enum feedback_type: { dismissal: 0, issue: 1, merge_request: 2 }
enum category: { sast: 0, dependency_scanning: 1, container_scanning: 2, dast: 3 }
validates :project, presence: true
validates :author, presence: true
validates :issue, presence: true, if: :issue?
validates :vulnerability_data, presence: true, if: :issue?
validates :merge_request, presence: true, if: :merge_request?
validates :vulnerability_data, presence: true, unless: :dismissal?
validates :feedback_type, presence: true
validates :category, presence: true
validates :project_fingerprint, presence: true, uniqueness: { scope: [:project_id, :category, :feedback_type] }
scope :with_associations, -> { includes(:pipeline, :issue, :author) }
scope :with_associations, -> { includes(:pipeline, :issue, :merge_request, :author) }
scope :all_preloaded, -> do
preload(:author, :project, :issue, :pipeline)
preload(:author, :project, :issue, :merge_request, :pipeline)
end
end
end
......@@ -138,6 +138,10 @@ module Vulnerabilities
feedback(feedback_type: 'issue')
end
def merge_request_feedback
feedback(feedback_type: 'merge_request')
end
def metadata
strong_memoize(:metadata) do
begin
......@@ -163,5 +167,9 @@ module Vulnerabilities
def links
metadata.fetch('links', [])
end
def remediations
metadata.dig('remediations')
end
end
end
......@@ -18,13 +18,21 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
end
expose :issue_iid, if: -> (feedback, _) { feedback.issue? } do |feedback|
feedback.issue&.iid
feedback.issue.iid
end
expose :issue_url, if: -> (feedback, _) { feedback.issue? } do |feedback|
project_issue_url(feedback.project, feedback.issue)
end
expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
feedback.merge_request.iid
end
expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
project_merge_request_path(feedback.project, feedback.merge_request)
end
expose :category
expose :feedback_type
expose :branch do |feedback|
......
......@@ -8,16 +8,19 @@ class Vulnerabilities::OccurrenceEntity < Grape::Entity
expose :identifiers, using: Vulnerabilities::IdentifierEntity
expose :project_fingerprint
expose :vulnerability_feedback_path, as: :vulnerability_feedback_issue_path, if: ->(_, _) { can_admin_vulnerability_feedback? && can_create_issue? }
expose :vulnerability_feedback_path, as: :vulnerability_feedback_merge_request_path, if: ->(_, _) { can_admin_vulnerability_feedback? && can_create_merge_request? }
expose :vulnerability_feedback_path, as: :vulnerability_feedback_dismissal_path, if: ->(_, _) { can_admin_vulnerability_feedback? }
expose :project, using: ::ProjectEntity
expose :dismissal_feedback, using: Vulnerabilities::FeedbackEntity
expose :issue_feedback, using: Vulnerabilities::FeedbackEntity
expose :merge_request_feedback, using: Vulnerabilities::FeedbackEntity
expose :metadata, merge: true, if: ->(occurrence, _) { occurrence.raw_metadata } do
expose :description
expose :solution
expose :location
expose :links
expose :location
expose :remediations
expose :solution
end
alias_method :occurrence, :object
......@@ -35,4 +38,8 @@ class Vulnerabilities::OccurrenceEntity < Grape::Entity
def can_create_issue?
can?(request.current_user, :create_issue, occurrence.project)
end
def can_create_merge_request?
can?(request.current_user, :create_merge_request_in, occurrence.project)
end
end
# frozen_string_literal: true
module MergeRequests
class CreateFromVulnerabilityDataService < ::BaseService
def execute
vulnerability = create_vulnerability
return error('Invalid vulnerability category') unless vulnerability
title_slug = Gitlab::Utils.slugify(vulnerability.title)
source_branch = "remediate/%s-%s" % [
title_slug[0..74],
Time.now.strftime("D%Y%m%dT%H%M%S")
]
target_branch = vulnerability.target_branch || @project.default_branch
return error("Can't create merge_request") unless can_create_merge_request?(source_branch)
return error("Can't create merge request") if vulnerability.remediations.empty?
patch_result = create_patch(vulnerability, source_branch, target_branch)
return error('Unable to apply patch') unless patch_result[:status] == :success
merge_request_params = {
title: "Resolve vulnerability: #{vulnerability.title}",
description: render_description(vulnerability),
source_branch: source_branch,
target_branch: target_branch,
force_remove_source_branch: "1"
}
merge_request = MergeRequests::CreateService.new(@project, @current_user, merge_request_params).execute
if merge_request.valid?
success(merge_request)
else
error(merge_request.errors)
end
end
private
def create_vulnerability
case @params[:category]
when 'sast', 'dependency_scanning', 'dast'
Gitlab::Vulnerabilities::StandardVulnerability.new(@params)
when 'container_scanning'
Gitlab::Vulnerabilities::ContainerScanningVulnerability.new(@params)
end
end
def create_patch(vulnerability, source_branch, target_branch)
diffs = vulnerability.data[:remediations].map { |remediation| Base64.decode64(remediation[:diff]) }
head_commit = project.repository.find_branch(target_branch).dereferenced_target
new_commit = render_commit(diffs, head_commit, vulnerability)
commit_patch_params = {
branch_name: source_branch,
start_branch: target_branch,
patches: [
new_commit
]
}
Commits::CommitPatchService.new(@project, @current_user, commit_patch_params).execute
end
def success(merge_request)
super(merge_request: merge_request)
end
def render_commit(diffs, head_commit, vulnerability)
git_user = Gitlab::Git::User.from_gitlab(@current_user)
# This currently applies only the first diff
render_template(
file: 'vulnerabilities/remediation.patch.erb',
locals: {
diff: diffs.first,
head_commit: head_commit,
user: git_user,
vulnerability: vulnerability
}
)
end
def render_description(vulnerability)
render_template(
file: 'vulnerabilities/merge_request_description.md.erb',
locals: { vulnerability: vulnerability }
)
end
def render_template(file:, locals:)
view = ActionView::Base.new(ActionController::Base.view_paths, {})
view.render(file: file, locals: locals)
end
def can_create_merge_request?(source_branch)
can?(@current_user, :create_merge_request_in, @project) &&
can?(@current_user, :create_merge_request_from, @project) &&
::Gitlab::UserAccess.new(@current_user, project: @project).can_push_to_branch?(source_branch)
end
end
end
......@@ -6,31 +6,23 @@ module VulnerabilityFeedbackModule
vulnerability_feedback = @project.vulnerability_feedback.new(@params)
vulnerability_feedback.author = @current_user
# Wrap Feedback and Issue creation in the same transaction
ActiveRecord::Base.transaction do
if vulnerability_feedback.issue? && # (feedback_type == 'issue')
!vulnerability_feedback.vulnerability_data.blank?
if vulnerability_feedback.issue? &&
!vulnerability_feedback.vulnerability_data.blank?
result = Issues::CreateFromVulnerabilityDataService
.new(@project, @current_user, vulnerability_feedback.vulnerability_data)
.execute
create_issue_for(vulnerability_feedback)
elsif vulnerability_feedback.merge_request? &&
!vulnerability_feedback.vulnerability_data.blank?
if result[:status] == :error
vulnerability_feedback.errors[:issue] << result[:message]
raise ActiveRecord::Rollback
end
issue = result[:issue]
vulnerability_feedback.issue = issue
end
# Ensure created issue is rolled back if feedback can't be saved
raise ActiveRecord::Rollback unless vulnerability_feedback.save
create_merge_request_for(vulnerability_feedback)
else
vulnerability_feedback.save
end
if vulnerability_feedback.persisted?
success(vulnerability_feedback)
else
rollback_merge_request(vulnerability_feedback.merge_request) if vulnerability_feedback.merge_request
error(vulnerability_feedback.errors)
end
......@@ -44,5 +36,49 @@ module VulnerabilityFeedbackModule
def success(vulnerability_feedback)
super().merge(vulnerability_feedback: vulnerability_feedback)
end
def create_issue_for(vulnerability_feedback)
# Wrap Feedback and Issue creation in the same transaction
ActiveRecord::Base.transaction do
result = Issues::CreateFromVulnerabilityDataService
.new(@project, @current_user, vulnerability_feedback.vulnerability_data)
.execute
if result[:status] == :error
vulnerability_feedback.errors[:issue] << result[:message]
raise ActiveRecord::Rollback
end
issue = result[:issue]
vulnerability_feedback.issue = issue
# Ensure created association is rolled back if feedback can't be saved
raise ActiveRecord::Rollback unless vulnerability_feedback.save
end
end
def create_merge_request_for(vulnerability_feedback)
result = MergeRequests::CreateFromVulnerabilityDataService
.new(@project, @current_user, vulnerability_feedback.vulnerability_data)
.execute
if result[:status] == :success
merge_request = result[:merge_request]
vulnerability_feedback.merge_request = merge_request
vulnerability_feedback.save
else
vulnerability_feedback.errors[:merge_request] << result[:message]
end
end
# Gitaly RPCs cannot occur within a transaction so we must manually
# rollback MR and branch creation
def rollback_merge_request(merge_request)
branch_name = merge_request.source_branch
merge_request&.destroy &&
DeleteBranchService.new(project, current_user).execute(branch_name)
end
end
end
### Description:
<%= vulnerability.description %>
<% if vulnerability.severity.present? %>
* Severity: <%= vulnerability.severity %>
<% end %>
<% if vulnerability.confidence.present? %>
* Confidence: <%= vulnerability.confidence %>
<% end %>
<% if defined?(vulnerability.file) && vulnerability.file.present? %>
* Location: [<%= vulnerability.location_text %>](<%= vulnerability.location_link %>)
<% end %>
<% if vulnerability.solution.present? %>
### Solution:
<%= vulnerability.solution %>
<% end %>
<% if vulnerability.identifiers.present? %>
### Identifiers:
<% vulnerability.identifiers.each do |identifier| %>
<% if identifier[:url].present? %>
* [<%= identifier[:name] %>](<%= identifier[:url] %>)
<% else %>
* <%= identifier[:name] %>
<% end %>
<% end %>
<% end %>
<% if vulnerability.links.present? %>
### Links:
<% vulnerability.links.each do |link| %>
<% if link[:name].present? %>
* [<%= link[:name] %>](<%= link[:url] %>)
<% else %>
* <%= link[:url] %>
<% end %>
<% end %>
<% end %>
From <%= head_commit.sha %> <%= head_commit.date.strftime("%a %b %e %H:%M:%S %Y") %>
From: <%= user.name %> <<%= user.email %>>
Date: <%= Time.now.strftime("%a, %e %b %Y %H:%M:%S %z") %>
Subject: Fix Vulnerability - <%= vulnerability.title %>
<%= diff %>
--
2.19.1
\ No newline at end of file
---
title: Create MR from Vulnerability Solution
merge_request: 9326
author:
type: added
# frozen_string_literal: true
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddMergeRequestIdToVulnerabilityFeedback < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def up
add_column :vulnerability_feedback, :merge_request_id, :integer, null: true
end
def down
remove_column :vulnerability_feedback, :merge_request_id, :integer
end
end
# frozen_string_literal: true
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddMergeRequestIdIndexOnVulnerabilityFeedback < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index :vulnerability_feedback, :merge_request_id
add_concurrent_foreign_key :vulnerability_feedback, :merge_requests, column: :merge_request_id, on_delete: :nullify
end
def down
# We need to remove the foreign key first, otherwise Mysql will fail with:
# Mysql2::Error: Cannot drop index 'index_vulnerability_feedback_on_merge_request_id': needed in a foreign key constraint: ALTER TABLE `vulnerability_feedback` DROP `merge_request_id`
remove_foreign_key :vulnerability_feedback, column: :merge_request_id
remove_concurrent_index :vulnerability_feedback, :merge_request_id
end
end
......@@ -11,7 +11,7 @@ module Gitlab
report_data = parse_report(json_data)
raise SecurityReportParserError, "Invalid report format" unless report_data.is_a?(Hash)
report_data["vulnerabilities"].each do |vulnerability|
collate_remediations(report_data).each do |vulnerability|
create_vulnerability(report, vulnerability, report_data["version"])
end
rescue JSON::ParserError
......@@ -27,6 +27,20 @@ module Gitlab
JSON.parse!(json_data)
end
# map remediations to relevant vulnerabilities
def collate_remediations(report_data)
return report_data["vulnerabilities"] unless report_data["remediations"]
report_data["vulnerabilities"].map do |vulnerability|
# Grab the first available remediation.
remediation = report_data["remediations"].find do |remediation|
remediation["fixes"].any? { |fix| fix["cve"] == vulnerability["cve"] }
end
vulnerability.merge("remediations" => [remediation])
end
end
def create_vulnerability(report, data, version)
scanner = create_scanner(report, data['scanner'] || mutate_scanner_tool(data['tool']))
identifiers = create_identifiers(report, data['identifiers'])
......
......@@ -10,6 +10,8 @@ module Gitlab
solution
identifiers
links
remediations
target_branch
].each do |method_name|
define_method(method_name) do
@data[method_name]
......
......@@ -14,10 +14,13 @@ describe Projects::VulnerabilityFeedbackController do
let(:pipeline_1) { create(:ci_pipeline, project: project) }
let(:pipeline_2) { create(:ci_pipeline, project: project) }
let(:merge_request) { create(:merge_request, source_project: project) }
let!(:vuln_feedback_1) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline_1) }
let!(:vuln_feedback_2) { create(:vulnerability_feedback, :issue, :sast, project: project, author: user, pipeline: pipeline_1) }
let!(:vuln_feedback_3) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline_2) }
let!(:vuln_feedback_4) { create(:vulnerability_feedback, :dismissal, :dependency_scanning, project: project, author: user, pipeline: pipeline_2) }
let!(:vuln_feedback_5) { create(:vulnerability_feedback, :merge_request, :dependency_scanning, project: project, author: user, pipeline: pipeline_1, merge_request: merge_request) }
context '@vulnerability_feedback' do
it 'returns a successful 200 response' do
......@@ -30,7 +33,7 @@ describe Projects::VulnerabilityFeedbackController do
list_feedbacks
expect(response).to match_response_schema('vulnerability_feedback_list', dir: 'ee')
expect(json_response.length).to eq 4
expect(json_response.length).to eq 5
end
context 'with filter params' do
......
......@@ -92,6 +92,16 @@ FactoryBot.define do
end
end
trait :dependency_scanning_remediation do
file_format :raw
file_type :dependency_scanning
after(:build) do |artifact, _|
artifact.file = fixture_file_upload(
Rails.root.join('spec/fixtures/security-reports/remediations/gl-dependency-scanning-report.json'), 'text/plain')
end
end
trait :dependency_scanning_deprecated do
file_format :raw
file_type :dependency_scanning
......
......@@ -9,6 +9,7 @@ FactoryBot.define do
project
author
issue nil
merge_request nil
association :pipeline, factory: :ci_pipeline
feedback_type 'dismissal'
category 'sast'
......@@ -24,6 +25,11 @@ FactoryBot.define do
issue { create(:issue, project: project) }
end
trait :merge_request do
feedback_type 'merge_request'
merge_request { create(:merge_request, source_project: project) }
end
trait :sast do
category 'sast'
end
......
......@@ -13,6 +13,7 @@
"name" : { "type": "string" },
"project_fingerprint": { "type": "string" },
"vulnerability_feedback_issue_path": { "type": "string" },
"vulnerability_feedback_merge_request_path": { "type": "string" },
"vulnerability_feedback_dismissal_path": { "type": "string" },
"confidence" : {
"type": "string",
......@@ -46,6 +47,10 @@
{ "type": "null" },
{ "$ref": "../vulnerability_feedback.json" }
]},
"merge_request_feedback" : { "oneOf": [
{ "type": "null" },
{ "$ref": "../vulnerability_feedback.json" }
]},
"dismissal_feedback" : { "oneOf": [
{ "type": "null" },
{ "$ref": "../vulnerability_feedback.json" }
......
......@@ -18,9 +18,11 @@
},
"issue_iid": { "type": ["integer", "null"] },
"issue_url": { "type": ["string", "null"] },
"merge_request_iid": { "type": ["integer", "null"] },
"merge_request_path": { "type": ["string", "null"] },
"feedback_type": {
"type": "string",
"enum": ["dismissal", "issue"]
"enum": ["dismissal", "issue", "merge_request"]
},
"category": {
"type": "string",
......
......@@ -3,6 +3,8 @@
require 'spec_helper'
describe Gitlab::Ci::Parsers::Security::DependencyScanning do
using RSpec::Parameterized::TableSyntax
describe '#parse!' do
let(:project) { artifact.project }
let(:pipeline) { artifact.job.pipeline }
......@@ -10,7 +12,11 @@ describe Gitlab::Ci::Parsers::Security::DependencyScanning do
let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type) }
let(:parser) { described_class.new }
where(report_format: %i(dependency_scanning dependency_scanning_deprecated))
where(:report_format, :occurrence_count, :identifier_count, :scanner_count, :fingerprint, :version) do
:dependency_scanning | 4 | 7 | 2 | '2773f8cc955346ab1f756b94aa310db8e17c0944' | '1.3'
:dependency_scanning_deprecated | 4 | 7 | 2 | '2773f8cc955346ab1f756b94aa310db8e17c0944' | '1.3'
:dependency_scanning_remediation | 2 | 3 | 1 | '228998b5db51d86d3b091939e2f5873ada0a14a1' | '2.0'
end
with_them do
let(:artifact) { create(:ee_ci_job_artifact, report_format) }
......@@ -22,17 +28,36 @@ describe Gitlab::Ci::Parsers::Security::DependencyScanning do
end
it "parses all identifiers and occurrences" do
expect(report.occurrences.length).to eq(4)
expect(report.identifiers.length).to eq(7)
expect(report.scanners.length).to eq(2)
expect(report.occurrences.length).to eq(occurrence_count)
expect(report.identifiers.length).to eq(identifier_count)
expect(report.scanners.length).to eq(scanner_count)
end
it "generates expected location fingerprint" do
expect(report.occurrences.first[:location_fingerprint]).to eq('2773f8cc955346ab1f756b94aa310db8e17c0944')
expect(report.occurrences.first[:location_fingerprint]).to eq(fingerprint)
end
it "generates expected metadata_version" do
expect(report.occurrences.first[:metadata_version]).to eq('1.3')
expect(report.occurrences.first[:metadata_version]).to eq(version)
end
end
context "when vulnerabilities have remediations" do
let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning_remediation) }
before do
artifact.each_blob do |blob|
parser.parse!(blob, report)
end
end
it "generates occurrence with expected remediation" do
occurrence = report.occurrences.last
raw_metadata = JSON.parse!(occurrence[:raw_metadata])
expect(occurrence[:name]).to eq("Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js")
expect(raw_metadata["remediations"].first["summary"]).to eq("Upgrade saml2-js")
expect(raw_metadata["remediations"].first["diff"]).to start_with("ZGlmZiAtLWdpdCBhL3lhcm4")
end
end
end
......
......@@ -10,6 +10,7 @@ describe Vulnerabilities::Feedback do
it { is_expected.to belong_to(:project) }
it { is_expected.to belong_to(:author).class_name('User') }
it { is_expected.to belong_to(:issue) }
it { is_expected.to belong_to(:merge_request) }
it { is_expected.to belong_to(:pipeline).class_name('Ci::Pipeline').with_foreign_key('pipeline_id') }
end
......
......@@ -230,4 +230,81 @@ describe Vulnerabilities::Occurrence do
is_expected.to eq({ 4 => 1, 5 => 2, 6 => 3 })
end
end
describe 'feedback' do
set(:project) { create(:project) }
let(:occurrence) do
create(
:vulnerabilities_occurrence,
report_type: :dependency_scanning,
project: project
)
end
describe '#issue_feedback' do
let(:issue) { create(:issue, project: project) }
let!(:issue_feedback) do
create(
:vulnerability_feedback,
:dependency_scanning,
:issue,
issue: issue,
project: project,
project_fingerprint: occurrence.project_fingerprint
)
end
it 'returns associated feedback' do
feedback = occurrence.issue_feedback
expect(feedback).to be_present
expect(feedback[:project_id]).to eq project.id
expect(feedback[:feedback_type]).to eq 'issue'
expect(feedback[:issue_id]).to eq issue.id
end
end
describe '#dismissal_feedback' do
let!(:dismissal_feedback) do
create(
:vulnerability_feedback,
:dependency_scanning,
:dismissal,
project: project,
project_fingerprint: occurrence.project_fingerprint
)
end
it 'returns associated feedback' do
feedback = occurrence.dismissal_feedback
expect(feedback).to be_present
expect(feedback[:project_id]).to eq project.id
expect(feedback[:feedback_type]).to eq 'dismissal'
end
end
describe '#merge_request_feedback' do
let(:merge_request) { create(:merge_request, source_project: project) }
let!(:merge_request_feedback) do
create(
:vulnerability_feedback,
:dependency_scanning,
:merge_request,
merge_request: merge_request,
project: project,
project_fingerprint: occurrence.project_fingerprint
)
end
it 'returns associated feedback' do
feedback = occurrence.merge_request_feedback
expect(feedback).to be_present
expect(feedback[:project_id]).to eq project.id
expect(feedback[:feedback_type]).to eq 'merge_request'
expect(feedback[:merge_request_id]).to eq merge_request.id
end
end
end
end
......@@ -52,7 +52,7 @@ describe Vulnerabilities::OccurrenceEntity do
expect(subject).to include(:name, :report_type, :severity, :confidence, :project_fingerprint)
expect(subject).to include(:scanner, :project, :identifiers)
expect(subject).to include(:dismissal_feedback, :issue_feedback)
expect(subject).to include(:description, :solution, :location, :links)
expect(subject).to include(:description, :links, :location, :remediations, :solution)
end
context 'when not allowed to admin vulnerability feedback' do
......@@ -62,6 +62,7 @@ describe Vulnerabilities::OccurrenceEntity do
it 'does not contain vulnerability feedback paths' do
expect(subject).not_to include(:vulnerability_feedback_issue_path)
expect(subject).not_to include(:vulnerability_feedback_merge_request_path)
expect(subject).not_to include(:vulnerability_feedback_dismissal_path)
end
end
......@@ -79,6 +80,10 @@ describe Vulnerabilities::OccurrenceEntity do
expect(subject).to include(:vulnerability_feedback_issue_path)
end
it 'contains vulnerability feedback merge_request path' do
expect(subject).to include(:vulnerability_feedback_merge_request_path)
end
context 'when disallowed to create issue' do
let(:project) { create(:project, issues_access_level: ProjectFeature::DISABLED) }
......@@ -89,6 +94,26 @@ describe Vulnerabilities::OccurrenceEntity do
it 'contains vulnerability feedback dismissal path' do
expect(subject).to include(:vulnerability_feedback_dismissal_path)
end
it 'contains vulnerability feedback merge_request path' do
expect(subject).to include(:vulnerability_feedback_merge_request_path)
end
end
context 'when disallowed to create merge_request' do
let(:project) { create(:project, merge_requests_access_level: ProjectFeature::DISABLED) }
it 'does not contain vulnerability feedback merge_request path' do
expect(subject).not_to include(:vulnerability_feedback_merge_request_path)
end
it 'contains vulnerability feedback issue path' do
expect(subject).to include(:vulnerability_feedback_issue_path)
end
it 'contains vulnerability feedback dismissal path' do
expect(subject).to include(:vulnerability_feedback_dismissal_path)
end
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe MergeRequests::CreateFromVulnerabilityDataService, '#execute' do
let(:remediations_folder) { Rails.root.join('spec/fixtures/security-reports/remediations') }
let(:yarn_lock_content) { File.read(File.join(remediations_folder, "yarn.lock")) }
let(:remediation_patch_content) { File.read(File.join(remediations_folder, "remediation.patch")) }
let(:group) { create(:group) }
let(:user) { create(:user) }
let(:project) do
create(:project, :custom_repo, namespace: group, files: { 'yarn.lock' => yarn_lock_content })
end
before do
group.add_developer(user)
end
shared_examples 'a created merge_request' do
let(:result) { described_class.new(project, user, params).execute }
it 'creates the merge_request with the given params' do
expect(result[:status]).to eq(:success)
merge_request = result[:merge_request]
expect(merge_request).to be_persisted
expect(merge_request.project).to eq(project)
expect(merge_request.author).to eq(user)
expect(merge_request.title).to eq(expected_title)
expect(merge_request.description).to eq(expected_description)
expect(merge_request.target_branch).to eq(project.default_branch)
expect(merge_request.source_branch).to start_with('remediate/authentication-bypass-via-incorrect-dom-traversal-and-canonical')
end
end
context 'when user does not have permission to create merge_request' do
let(:result) { described_class.new(project, user, { title: 'title', category: 'dependency_scanning' }).execute }
before do
allow_any_instance_of(described_class).to receive(:can?).with(user, :create_merge_request_in, project).and_return(false)
allow_any_instance_of(described_class).to receive(:can?).with(user, :create_merge_request_from, project).and_return(false)
end
it 'returns expected error' do
expect(result[:status]).to eq(:error)
expect(result[:message]).to eq("Can't create merge_request")
end
end
context 'when merge_requests are disabled on project' do
let(:result) { described_class.new(project, user, { title: 'title', category: 'dependency_scanning' }).execute }
let(:project) { create(:project, namespace: group, merge_requests_access_level: ProjectFeature::DISABLED) }
it 'returns expected error' do
expect(result[:status]).to eq(:error)
expect(result[:message]).to eq("Can't create merge_request")
end
end
context 'when params are valid' do
context 'when category is dependency scanning' do
let(:diff) do
Base64.encode64(remediation_patch_content)
end
context 'when a description is present' do
let(:params) do
{
category: "dependency_scanning",
name: "Authentication bypass via incorrect DOM traversal and canonicalization",
message: "Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js",
description: "Some XML DOM traversal and canonicalization APIs may be inconsistent in handling of comments within XML nodes. Incorrect use of these APIs by some SAML libraries results in incorrect parsing of the inner text of XML nodes such that any inner text after the comment is lost prior to cryptographically signing the SAML message. Text after the comment therefore has no impact on the signature on the SAML message.\n\nA remote attacker can modify SAML content for a SAML service provider without invalidating the cryptographic signature, which may allow attackers to bypass primary authentication for the affected SAML service provider.",
cve: "yarn.lock:saml2-js:gemnasium:9952e574-7b5b-46fa-a270-aeb694198a98",
severity: "Unknown",
solution: "Upgrade to fixed version.",
scanner: {
id: "gemnasium",
name: "Gemnasium"
},
location: {
file: "yarn.lock",
dependency: {
package: {
name: "saml2-js"
},
version: "1.5.0"
}
},
identifiers: [
{
type: "gemnasium",
name: "Gemnasium-9952e574-7b5b-46fa-a270-aeb694198a98",
value: "9952e574-7b5b-46fa-a270-aeb694198a98",
url: "https://deps.sec.gitlab.com/packages/npm/saml2-js/versions/1.5.0/advisories"
},
{
type: "cve",
name: "CVE-2017-11429",
value: "CVE-2017-11429",
url: "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-11429"
}
],
links: [
{
url: "https://github.com/Clever/saml2/commit/3546cb61fd541f219abda364c5b919633609ef3d#diff-af730f9f738de1c9ad87596df3f6de84R279"
},
{
url: "https://github.com/Clever/saml2/issues/127"
},
{
url: "https://www.kb.cert.org/vuls/id/475445"
}
],
project_fingerprint: "fa6f5b6c5d240b834ac5e901dc69f9484cef89ec",
title: "Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js",
remediations: [
{
fixes: [
{
cve: "yarn.lock:saml2-js:gemnasium:9952e574-7b5b-46fa-a270-aeb694198a98"
}
],
summary: "Upgrade saml2-js",
diff: diff
}
],
path: "yarn.lock",
urlPath: "/root/yarn-remediation/blob/9bade3fef995a9e2ca1de44d396b433864990ab9/yarn.lock"
}
end
let(:expected_title) { 'Resolve vulnerability: Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js' }
let(:expected_description) do
<<~DESC.chomp
### Description:
Some XML DOM traversal and canonicalization APIs may be inconsistent in handling of comments within XML nodes. Incorrect use of these APIs by some SAML libraries results in incorrect parsing of the inner text of XML nodes such that any inner text after the comment is lost prior to cryptographically signing the SAML message. Text after the comment therefore has no impact on the signature on the SAML message.\n\nA remote attacker can modify SAML content for a SAML service provider without invalidating the cryptographic signature, which may allow attackers to bypass primary authentication for the affected SAML service provider.
* Severity: Unknown
* Location: [yarn.lock](yarn.lock)
### Solution:
Upgrade to fixed version.
### Identifiers:
* [Gemnasium-9952e574-7b5b-46fa-a270-aeb694198a98](https://deps.sec.gitlab.com/packages/npm/saml2-js/versions/1.5.0/advisories)
* [CVE-2017-11429](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-11429)
### Links:
* https://github.com/Clever/saml2/commit/3546cb61fd541f219abda364c5b919633609ef3d#diff-af730f9f738de1c9ad87596df3f6de84R279
* https://github.com/Clever/saml2/issues/127
* https://www.kb.cert.org/vuls/id/475445
DESC
end
it_behaves_like 'a created merge_request'
end
context 'when a description is NOT present' do
let(:params) do
{
category: 'dependency_scanning',
severity: "Unknown",
solution: "Upgrade to fixed version.",
file: "yarn.lock",
cve: "yarn.lock:saml2-js:gemnasium:9952e574-7b5b-46fa-a270-aeb694198a98",
title: 'Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js',
tool: 'find_sec_bugs',
remediations: [
{ diff: diff }
]
}
end
let(:expected_title) { 'Resolve vulnerability: Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js' }
let(:expected_description) do
<<~DESC.chomp
### Description:
Authentication bypass via incorrect DOM traversal and canonicalization in saml2-js
* Severity: Unknown
* Location: [yarn.lock](yarn.lock)
### Solution:
Upgrade to fixed version.
DESC
end
it_behaves_like 'a created merge_request'
end
end
end
context 'when params are invalid' do
context 'when category is unknown' do
let(:params) { { category: 'foo' } }
let(:result) { described_class.new(project, user, params).execute }
it 'return expected error' do
expect(result[:status]).to eq(:error)
expect(result[:message]).to eq('Invalid vulnerability category')
end
end
context 'when remediations are missing' do
let(:params) { { title: 'title', category: 'dependency_scanning', remediations: [] } }
let(:result) { described_class.new(project, user, params).execute }
it 'return expected error' do
expect(result[:status]).to eq(:error)
expect(result[:message]).to eq("Can't create merge request")
end
end
end
end
......@@ -2,7 +2,7 @@ require 'spec_helper'
describe VulnerabilityFeedbackModule::CreateService, '#execute' do
let(:group) { create(:group) }
let(:project) { create(:project, :public, namespace: group) }
let(:project) { create(:project, :public, :repository, namespace: group) }
let(:user) { create(:user) }
let(:pipeline) { create(:ci_pipeline, project: project) }
......@@ -43,6 +43,8 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
expect(feedback.dismissal?).to eq(true)
expect(feedback.issue?).to eq(false)
expect(feedback.issue).to be_nil
expect(feedback.merge_request?).to eq(false)
expect(feedback.merge_request).to be_nil
end
end
......@@ -68,6 +70,8 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
expect(feedback.dismissal?).to eq(false)
expect(feedback.issue?).to eq(true)
expect(feedback.issue).to be_an(Issue)
expect(feedback.merge_request?).to eq(false)
expect(feedback.merge_request).to be_nil
end
it 'delegates the Issue creation to CreateFromVulnerabilityDataService' do
......@@ -77,6 +81,82 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
expect(result[:status]).to eq(:success)
end
end
context 'when feedback_type is merge_request' do
let(:remediations_folder) { Rails.root.join('spec/fixtures/security-reports/remediations') }
let(:yarn_lock_content) do
File.read(
File.join(remediations_folder, "yarn.lock")
)
end
let(:project) do
create(:project, :custom_repo, namespace: group, files: { 'yarn.lock' => yarn_lock_content })
end
let(:remediation_diff) do
Base64.encode64(
File.read(
File.join(remediations_folder, "remediation.patch")
)
)
end
let(:result) do
params = feedback_params.merge(
feedback_type: 'merge_request',
category: 'dependency_scanning'
)
params[:vulnerability_data][:category] = 'dependency_scanning'
params[:vulnerability_data][:remediations] = [
{ diff: remediation_diff }
]
described_class.new(
project,
user,
params
).execute
end
it 'creates the feedback with the given params' do
expect(result[:status]).to eq(:success)
feedback = result[:vulnerability_feedback]
expect(feedback).to be_persisted
expect(feedback.project).to eq(project)
expect(feedback.author).to eq(user)
expect(feedback.feedback_type).to eq('merge_request')
expect(feedback.pipeline_id).to eq(pipeline.id)
expect(feedback.category).to eq('dependency_scanning')
expect(feedback.project_fingerprint).to eq('418291a26024a1445b23fe64de9380cdcdfd1fa8')
expect(feedback.dismissal?).to eq(false)
expect(feedback.issue?).to eq(false)
expect(feedback.issue).to be_nil
expect(feedback.merge_request?).to eq(true)
expect(feedback.merge_request).to be_an(MergeRequest)
end
it 'delegates the MergeRequest creation to CreateFromVulnerabilityDataService' do
expect_next_instance_of(MergeRequests::CreateFromVulnerabilityDataService) do |service|
expect(service).to receive(:execute).once.and_call_original
end
expect(result[:status]).to eq(:success)
end
it 'destroys merge_request and branch if feedback fails to persist' do
expect_next_instance_of(Vulnerabilities::Feedback) do |feedback|
expect(feedback).to receive(:save).and_return(false)
end
expect(result[:status]).to eq(:error)
expect(Vulnerabilities::Feedback.count).to eq 0
expect(MergeRequest.count).to eq 0
branches = BranchesFinder.new(project.repository, {}).execute
expect(branches.length).to eq 1
end
end
end
context 'when params are invalid' do
......
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