From f52fdb94fb75ed43d0cb487df50bf16802a24715 Mon Sep 17 00:00:00 2001
From: Brian Williams <bwilliams@gitlab.com>
Date: Tue, 23 Jul 2024 11:16:29 -0500
Subject: [PATCH] Remove newlines from vulnerability export CSV
The export finalization process expects that each CSV entry is on a
newline. As a quick fix, this change replaces newline characters with
literal "\n" values.
---
.../exporters/csv_service.rb | 2 +-
.../exporters/csv_service_spec.rb | 8 ++--
gems/csv_builder/lib/csv_builder.rb | 10 ++++-
gems/csv_builder/lib/csv_builder/builder.rb | 24 ++++++-----
.../csv_builder/spec/csv_builder/gzip_spec.rb | 2 +-
.../spec/csv_builder/stream_spec.rb | 2 +-
gems/csv_builder/spec/csv_builder_spec.rb | 40 ++++++++++++++++++-
7 files changed, 69 insertions(+), 19 deletions(-)
diff --git a/ee/app/services/vulnerability_exports/exporters/csv_service.rb b/ee/app/services/vulnerability_exports/exporters/csv_service.rb
index e73ba991b83ed..f522f5323fcd1 100644
--- a/ee/app/services/vulnerability_exports/exporters/csv_service.rb
+++ b/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
diff --git a/ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb b/ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb
index ffd45b88521bb..c9621143806ff 100644
--- a/ee/spec/services/vulnerability_exports/exporters/csv_service_spec.rb
+++ b/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
diff --git a/gems/csv_builder/lib/csv_builder.rb b/gems/csv_builder/lib/csv_builder.rb
index 86b682939dcc7..0f5aa5386332e 100644
--- a/gems/csv_builder/lib/csv_builder.rb
+++ b/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
diff --git a/gems/csv_builder/lib/csv_builder/builder.rb b/gems/csv_builder/lib/csv_builder/builder.rb
index 51aaf2132cf48..c270db77f8474 100644
--- a/gems/csv_builder/lib/csv_builder/builder.rb
+++ b/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
diff --git a/gems/csv_builder/spec/csv_builder/gzip_spec.rb b/gems/csv_builder/spec/csv_builder/gzip_spec.rb
index 22462d8dd0ae4..8b05b0e23377a 100644
--- a/gems/csv_builder/spec/csv_builder/gzip_spec.rb
+++ b/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
diff --git a/gems/csv_builder/spec/csv_builder/stream_spec.rb b/gems/csv_builder/spec/csv_builder/stream_spec.rb
index d23e63520afff..1866885661e87 100644
--- a/gems/csv_builder/spec/csv_builder/stream_spec.rb
+++ b/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
diff --git a/gems/csv_builder/spec/csv_builder_spec.rb b/gems/csv_builder/spec/csv_builder_spec.rb
index ccae8365174f0..94abb0aaa9418 100644
--- a/gems/csv_builder/spec/csv_builder_spec.rb
+++ b/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" } }
--
GitLab