Merge branch 'bwill/escape-newlines-from-vulnerability-export' into 'master'

Remove newlines from vulnerability export CSV

Closes #472930

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/160439



Merged-by: Michael Becker <11881043-wandering_person@users.noreply.gitlab.com>
Approved-by: Mehmet Emin INAC <minac@gitlab.com>
Approved-by: Michael Becker <11881043-wandering_person@users.noreply.gitlab.com>
Co-authored-by: Brian Williams <bwilliams@gitlab.com>

ee/app/services/vulnerability_exports/exporters/csv_service.rb

@@ -38,7 +38,7 @@ def preloads
       end
 
       def csv_builder
-        @csv_builder ||= CsvBuilder.new(vulnerabilities, mapping, preloads)
+        @csv_builder ||= CsvBuilder.new(vulnerabilities, mapping, preloads, replace_newlines: true)
       end
 
       def mapping

ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb

@@ -48,7 +48,7 @@
 
       context 'when a project belongs to a group' do
         let_it_be(:vulnerability) { create(:vulnerability, :with_findings, project: project) }
-        let_it_be(:note) { create(:note, project: project, noteable: vulnerability) }
+        let_it_be(:note) { create(:note, project: project, noteable: vulnerability, note: "a\nb") }
 
         it 'includes proper values for each column type', :aggregate_failures do
           expect(csv[0]['Group Name']).to eq group.name
@@ -66,7 +66,7 @@
           expect(csv[0]['Detected At']).to eq vulnerability.created_at.to_s
           expect(csv[0]['Location']).to eq vulnerability.location.to_s
           expect(csv[0]['Activity']).to eq vulnerability.resolved_on_default_branch.to_s
-          expect(csv[0]['Comments']).to include(note.note)
+          expect(csv[0]['Comments']).to include('a\nb')
           expect(csv[0]['Full Path']).to eq vulnerability.full_path
           expect(csv[0]['CVSS Vectors']).to eq "GitLab=CVSS:3.1/AV:N/AC:L/PR:H/UI:N/S:U/C:L/I:L/A:N"
           expect(csv[0]['Dismissal Reason']).to be_nil
@@ -76,7 +76,7 @@
       context 'when a project belongs to a user' do
         let_it_be(:project) { project_with_valid_findings(namespace: user.namespace) }
         let_it_be(:vulnerability) { create(:vulnerability, :with_findings, project: project) }
-        let_it_be(:note) { create(:note, project: project, noteable: vulnerability) }
+        let_it_be(:note) { create(:note, project: project, noteable: vulnerability, note: "a\nb") }
 
         it 'includes proper values for each column except group name' do
           expect(csv[0]['Group Name']).to be_nil
@@ -94,7 +94,7 @@
           expect(csv[0]['Detected At']).to eq vulnerability.created_at.to_s
           expect(csv[0]['Location']).to eq vulnerability.location.to_s
           expect(csv[0]['Activity']).to eq vulnerability.resolved_on_default_branch.to_s
-          expect(csv[0]['Comments']).to include(note.note)
+          expect(csv[0]['Comments']).to include('a\nb')
           expect(csv[0]['Full Path']).to eq vulnerability.full_path
           expect(csv[0]['CVSS Vectors']).to eq "GitLab=CVSS:3.1/AV:N/AC:L/PR:H/UI:N/S:U/C:L/I:L/A:N"
           expect(csv[0]['Dismissal Reason']).to be_nil

gems/csv_builder/lib/csv_builder.rb

@@ -28,11 +28,17 @@ module CsvBuilder
   # * +collection+ - The data collection to be used
   # * +header_to_value_hash+ - A hash of 'Column Heading' => 'value_method'.
   # * +associations_to_preload+ - An array of records to preload with a batch of records.
+  # * +replace_newlines+ - default: false - If true, replaces newline characters with a literal "\n"
   #
   # The value method will be called once for each object in the collection, to
   # determine the value for that row. It can either be the name of a method on
   # the object, or a lamda to call passing in the object.
-  def self.new(collection, header_to_value_hash, associations_to_preload = [])
-    CsvBuilder::Builder.new(collection, header_to_value_hash, associations_to_preload)
+  def self.new(collection, header_to_value_hash, associations_to_preload = [], replace_newlines: false)
+    CsvBuilder::Builder.new(
+      collection,
+      header_to_value_hash,
+      associations_to_preload,
+      replace_newlines: replace_newlines
+    )
   end
 end

gems/csv_builder/lib/csv_builder/builder.rb

@@ -6,12 +6,13 @@ class Builder
 
     attr_reader :rows_written
 
-    def initialize(collection, header_to_value_hash, associations_to_preload = [])
+    def initialize(collection, header_to_value_hash, associations_to_preload = [], replace_newlines: false)
       @header_to_value_hash = header_to_value_hash
       @collection = collection
       @truncated = false
       @rows_written = 0
       @associations_to_preload = associations_to_preload
+      @replace_newlines = replace_newlines
     end
 
     # Renders the csv to a string
@@ -78,13 +79,19 @@ def attributes
 
     def row(object)
       attributes.map do |attribute|
-        if attribute.respond_to?(:call)
-          excel_sanitize(attribute.call(object))
-        elsif object.is_a?(Hash)
-          excel_sanitize(object[attribute])
-        else
-          excel_sanitize(object.public_send(attribute)) # rubocop:disable GitlabSecurity/PublicSend
-        end
+        data = if attribute.respond_to?(:call)
+                 attribute.call(object)
+               elsif object.is_a?(Hash)
+                 object[attribute]
+               else
+                 object.public_send(attribute) # rubocop:disable GitlabSecurity/PublicSend -- Not user input
+               end
+
+        next if data.nil?
+
+        data = data.gsub("\n", '\n') if data.is_a?(String) && @replace_newlines
+
+        excel_sanitize(data)
       end
     end
 
@@ -104,7 +111,6 @@ def write_csv(csv, until_condition:)
     end
 
     def excel_sanitize(line)
-      return if line.nil?
       return line unless line.is_a?(String) && line.match?(UNSAFE_EXCEL_PREFIX)
 
       ["'", line].join

gems/csv_builder/spec/csv_builder/gzip_spec.rb

@@ -7,7 +7,7 @@
   let(:event_2) { double(title: 'Added sugar', description: 'Just a pinch') }
   let(:items) { [event_1, event_2] }
 
-  subject(:builder) { described_class.new(items, 'Title' => 'title', 'Description' => 'description') }
+  subject(:builder) { described_class.new(items, { 'Title' => 'title', 'Description' => 'description' }) }
 
   describe '#render' do
     it 'returns yields a tempfile' do

gems/csv_builder/spec/csv_builder/stream_spec.rb

@@ -7,7 +7,7 @@
   let(:event_2) { double(title: 'Added sugar', description: 'Just a pinch') }
   let(:fake_relation) { described_class::FakeRelation.new([event_1, event_2]) }
 
-  subject(:builder) { described_class.new(fake_relation, 'Title' => 'title', 'Description' => 'description') }
+  subject(:builder) { described_class.new(fake_relation, { 'Title' => 'title', 'Description' => 'description' }) }
 
   describe '#render' do
     before do

gems/csv_builder/spec/csv_builder_spec.rb

@@ -7,7 +7,7 @@
   end
 
   let(:subject) do
-    described_class.new(enumerable, **header_to_value_hash)
+    described_class.new(enumerable, header_to_value_hash)
   end
 
   shared_examples 'csv builder examples' do
@@ -113,6 +113,41 @@
     end
   end
 
+  shared_examples 'builder that replaces newlines' do
+    let(:object) { double(title: "title", description: "Line 1\n\nLine 2") }
+    let(:header_to_value_hash) { { 'Title' => 'title', 'Description' => 'description' } }
+    let(:items) { [object] }
+
+    it 'does not replace newlines by default' do
+      expect(csv_data).to eq("Title,Description\ntitle,\"Line 1\n\nLine 2\"\n")
+    end
+
+    context 'when replace_newlines is set to true' do
+      let(:subject) { described_class.new(enumerable, header_to_value_hash, replace_newlines: true) }
+
+      it 'replaces newlines with a literal "\n"' do
+        expect(csv_data).to eq("Title,Description\ntitle,Line 1\\n\\nLine 2\n")
+      end
+    end
+
+    context 'when line is nil' do
+      let(:object) { double(title: "title", description: nil) }
+
+      it 'gracefully generates CSV' do
+        expect(csv_data).to eq("Title,Description\ntitle,\n")
+      end
+    end
+
+    context 'when data is not a string' do
+      let(:object) { double(title: "title", created_at: Date.new(2001, 2, 3)) }
+      let(:header_to_value_hash) { { 'Title' => 'title', 'Created At' => 'created_at' } }
+
+      it 'gracefully generates CSV' do
+        expect(csv_data).to eq("Title,Created At\ntitle,2001-02-03\n")
+      end
+    end
+  end
+
   context 'when ActiveRecord::Relation like object is given' do
     let(:object) { double(question: :answer) }
     let(:enumerable) { described_class::FakeRelation.new(items) }
@@ -129,6 +164,7 @@ def find_each(&block)
 
     it_behaves_like 'csv builder examples'
     it_behaves_like 'excel sanitization'
+    it_behaves_like 'builder that replaces newlines'
     it_behaves_like 'csv builder with truncation ability' do
       let(:big_object) { double(question: 'Long' * 1024) }
       let(:question_value) { big_object.question }
@@ -141,6 +177,7 @@ def find_each(&block)
 
     it_behaves_like 'csv builder examples'
     it_behaves_like 'excel sanitization'
+    it_behaves_like 'builder that replaces newlines'
     it_behaves_like 'csv builder with truncation ability' do
       let(:big_object) { double(question: 'Long' * 1024) }
       let(:question_value) { big_object.question }
@@ -155,6 +192,7 @@ def find_each(&block)
     end
 
     it_behaves_like 'csv builder examples'
+    it_behaves_like 'builder that replaces newlines'
 
     it_behaves_like 'excel sanitization' do
       let(:dangerous_title) { { title: "=cmd|' /C calc'!A0 title", description: "*safe_desc" } }