Merge branch 'security-vuln-feedback-pipeline' into 'master'

Enforce feedback pipeline is in the same project

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

ee/app/controllers/projects/vulnerability_feedback_controller.rb

@@ -15,6 +15,9 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
     # TODO: Move to finder or list service
     @vulnerability_feedback = @project.vulnerability_feedback.with_associations
 
+    # TODO remove once filtered data has been cleaned
+    @vulnerability_feedback = @vulnerability_feedback.only_valid_feedback
+
     if params[:category].present?
       @vulnerability_feedback = @vulnerability_feedback
         .with_category(params[:category])

ee/app/models/vulnerabilities/feedback.rb

@@ -26,6 +26,7 @@ module Vulnerabilities
     validates :feedback_type, presence: true
     validates :category, presence: true
     validates :project_fingerprint, presence: true, uniqueness: { scope: [:project_id, :category, :feedback_type] }
+    validates :pipeline, same_project_association: true, if: :pipeline_id?
 
     scope :with_associations, -> { includes(:pipeline, :issue, :merge_request, :author, :comment_author) }
 
@@ -33,6 +34,13 @@ module Vulnerabilities
       preload(:author, :comment_author, :project, :issue, :merge_request, :pipeline)
     end
 
+    # TODO remove once filtered data has been cleaned
+    def self.only_valid_feedback
+      pipeline = Ci::Pipeline.arel_table
+      feedback = arel_table
+      joins(:pipeline).where(pipeline[:project_id].eq(feedback[:project_id]))
+    end
+
     def self.find_or_init_for(feedback_params)
       validate_enums(feedback_params)
 

ee/changelogs/unreleased/security-vuln-feedback-pipeline.yml

+---
+title: Enforce vulnerability feedback pipeline is in the same project
+merge_request:
+author:
+type: security

ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb

@@ -38,6 +38,27 @@ describe Projects::VulnerabilityFeedbackController do
         expect(json_response.length).to eq 5
       end
 
+      # TODO remove once filtered data has been cleaned
+      context 'with invalid vulnerability_feedback' do
+        let!(:vuln_feedback_in_other_proj) do
+          feedback = build(
+            :vulnerability_feedback,
+            project: project,
+            author: user,
+            pipeline: create(:ci_pipeline)
+          )
+
+          feedback.save(validate: false)
+        end
+
+        it 'ignores feedback in other project' do
+          list_feedbacks
+
+          expect(response).to match_response_schema('vulnerability_feedback_list', dir: 'ee')
+          expect(json_response.length).to eq 5
+        end
+      end
+
       context 'with filter params' do
         it 'returns project feedbacks list filtered on category' do
           list_feedbacks({ category: 'sast' })
@@ -184,6 +205,39 @@ describe Projects::VulnerabilityFeedbackController do
       end
     end
 
+    context 'with pipeline in another project' do
+      let(:pipeline) { create(:ci_pipeline) }
+
+      it 'returns a 422 response' do
+        create_feedback user: user, project: project, params: create_params
+
+        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        expect(json_response).to eq({ "pipeline" => ["must associate the same project"] })
+      end
+    end
+
+    context 'with nonexistent pipeline_id' do
+      let(:pipeline) { build(:ci_pipeline, id: -10) }
+
+      it 'returns a 422 response' do
+        create_feedback user: user, project: project, params: create_params
+
+        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        expect(json_response).to eq({ "pipeline" => ["must associate the same project"] })
+      end
+    end
+
+    context 'with nil pipeline_id' do
+      let(:pipeline) { build(:ci_pipeline, id: nil) }
+
+      it 'returns a successful response' do
+        create_feedback user: user, project: project, params: create_params
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('vulnerability_feedback', dir: 'ee')
+      end
+    end
+
     def create_feedback(user:, project:, params:)
       sign_in(user)
       post_params = {

ee/spec/factories/vulnerabilities/feedback.rb

@@ -12,7 +12,7 @@ FactoryBot.define do
     author
     issue { nil }
     merge_request { nil }
-    association :pipeline, factory: :ci_pipeline
+    pipeline { create(:ci_pipeline, project: project) }
     feedback_type { 'dismissal' }
     category { 'sast' }
     project_fingerprint { generate(:project_fingerprint) }

ee/spec/fixtures/api/schemas/vulnerability_feedback.json

@@ -35,7 +35,7 @@
       "enum": ["sast", "dependency_scanning", "container_scanning", "dast"]
     },
     "project_fingerprint": { "type": "string" },
-    "branch": { "type": "string" },
+    "branch": { "type": ["string", "null"] },
     "destroy_vulnerability_feedback_dismissal_path": { "type": "string" }
   },
   "additionalProperties": false

ee/spec/models/vulnerabilities/feedback_spec.rb

@@ -28,6 +28,42 @@ describe Vulnerabilities::Feedback do
     it { is_expected.to validate_presence_of(:category) }
     it { is_expected.to validate_presence_of(:project_fingerprint) }
 
+    context 'pipeline is nil' do
+      let(:feedback) { build(:vulnerability_feedback, pipeline_id: nil) }
+
+      it 'is valid' do
+        expect(feedback).to be_valid
+      end
+    end
+
+    context 'pipeline has the same project_id' do
+      let(:feedback) { build(:vulnerability_feedback) }
+
+      it 'is valid' do
+        expect(feedback.project_id).to eq(feedback.pipeline.project_id)
+        expect(feedback).to be_valid
+      end
+    end
+
+    context 'pipeline_id does not exist' do
+      let(:feedback) { build(:vulnerability_feedback, pipeline_id: -100) }
+
+      it 'is invalid' do
+        expect(feedback.project_id).not_to eq(feedback.pipeline_id)
+        expect(feedback).not_to be_valid
+      end
+    end
+    context 'pipeline has a different project_id' do
+      let(:project) { create(:project) }
+      let(:pipeline) { create(:ci_pipeline, project: create(:project)) }
+      let(:feedback) { build(:vulnerability_feedback, project: project, pipeline: pipeline) }
+
+      it 'is invalid' do
+        expect(feedback.project_id).not_to eq(feedback.pipeline_id)
+        expect(feedback).not_to be_valid
+      end
+    end
+
     context 'comment is set' do
       let(:feedback) { build(:vulnerability_feedback, comment: 'a comment' ) }
 
@@ -71,6 +107,26 @@ describe Vulnerabilities::Feedback do
     end
   end
 
+  # TODO remove once filtered data has been cleaned
+  describe '::only_valid_feedback' do
+    let(:project) { create(:project) }
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+
+    let!(:feedback) { create(:vulnerability_feedback, :dismissal, :sast, project: project, pipeline: pipeline) }
+    let!(:invalid_feedback) do
+      feedback = build(:vulnerability_feedback, project: project, pipeline: create(:ci_pipeline))
+
+      feedback.save(validate: false)
+    end
+
+    it 'filters out invalid feedback' do
+      feedback_records = described_class.only_valid_feedback
+
+      expect(feedback_records.length).to eq 1
+      expect(feedback_records.first).to eq feedback
+    end
+  end
+
   describe '#has_comment?' do
     let(:feedback) { build(:vulnerability_feedback, comment: comment, comment_author: comment_author) }
     let(:comment) { 'a comment' }