Merge branch 'kassio/github-importer-strings-with-invalid-char' into 'master'

GithubImporter: Fix "ArgumentError: string contains null byte"

See merge request gitlab-org/gitlab!61480

changelogs/unreleased/kassio-github-importer-strings-with-invalid-char.yml

+---
+title: 'GithubImporter: Fix "ArgumentError: string contains null byte"'
+merge_request: 61480
+author:
+type: fixed

lib/gitlab/github_import/importer/diff_note_importer.rb

@@ -21,8 +21,7 @@ module Gitlab
 
           author_id, author_found = user_finder.author_id_for(note)
 
-          note_body =
-            MarkdownText.format(note.note, note.author, author_found)
+          note_body = MarkdownText.format(note.note, note.author, author_found)
 
           attributes = {
             noteable_type: 'MergeRequest',

lib/gitlab/github_import/importer/note_importer.rb

@@ -21,8 +21,7 @@ module Gitlab
 
           author_id, author_found = user_finder.author_id_for(note)
 
-          note_body =
-            MarkdownText.format(note.note, note.author, author_found)
+          note_body = MarkdownText.format(note.note, note.author, author_found)
 
           attributes = {
             noteable_type: note.noteable_type,

lib/gitlab/github_import/importer/pull_request_importer.rb

@@ -44,8 +44,7 @@ module Gitlab
         def create_merge_request
           author_id, author_found = user_finder.author_id_for(pull_request)
 
-          description = MarkdownText
-            .format(pull_request.description, pull_request.author, author_found)
+          description = MarkdownText.format(pull_request.description, pull_request.author, author_found)
 
           attributes = {
             iid: pull_request.iid,

lib/gitlab/github_import/markdown_text.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module GithubImport
     class MarkdownText
-      attr_reader :text, :author, :exists
+      include Gitlab::EncodingHelper
 
       def self.format(*args)
         new(*args).to_s
@@ -19,6 +19,15 @@ module Gitlab
       end
 
       def to_s
+        # Gitlab::EncodingHelper#clean remove `null` chars from the string
+        clean(format)
+      end
+
+      private
+
+      attr_reader :text, :author, :exists
+
+      def format
         if author&.login.present? && !exists
           "*Created by: #{author.login}*\n\n#{text}"
         else

spec/lib/gitlab/github_import/importer/note_importer_spec.rb

@@ -8,13 +8,14 @@ RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do
   let(:user) { create(:user) }
   let(:created_at) { Time.new(2017, 1, 1, 12, 00) }
   let(:updated_at) { Time.new(2017, 1, 1, 12, 15) }
+  let(:note_body) { 'This is my note' }
 
   let(:github_note) do
     Gitlab::GithubImport::Representation::Note.new(
       noteable_id: 1,
       noteable_type: 'Issue',
       author: Gitlab::GithubImport::Representation::User.new(id: 4, login: 'alice'),
-      note: 'This is my note',
+      note: note_body,
       created_at: created_at,
       updated_at: updated_at,
       github_id: 1
@@ -92,6 +93,24 @@ RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do
           importer.execute
         end
       end
+
+      context 'when the note have invalid chars' do
+        let(:note_body) { %{There were an invalid char "\u0000" <= right here} }
+
+        it 'removes invalid chars' do
+          expect(importer.user_finder)
+            .to receive(:author_id_for)
+            .with(github_note)
+            .and_return([user.id, true])
+
+          expect { importer.execute }
+            .to change(project.notes, :count)
+            .by(1)
+
+          expect(project.notes.last.note)
+            .to eq('There were an invalid char "" <= right here')
+        end
+      end
     end
 
     context 'when the noteable does not exist' do

spec/lib/gitlab/github_import/markdown_text_spec.rb

@@ -20,11 +20,25 @@ RSpec.describe Gitlab::GithubImport::MarkdownText do
       expect(text.to_s).to eq('Hello')
     end
 
+    it 'returns the text when the author has no login' do
+      author = double(:author, login: nil)
+      text = described_class.new('Hello', author, true)
+
+      expect(text.to_s).to eq('Hello')
+    end
+
     it 'returns the text with an extra header when the author was not found' do
       author = double(:author, login: 'Alice')
       text = described_class.new('Hello', author)
 
       expect(text.to_s).to eq("*Created by: Alice*\n\nHello")
     end
+
+    it 'cleans invalid chars' do
+      author = double(:author, login: 'Alice')
+      text = described_class.format("\u0000Hello", author)
+
+      expect(text.to_s).to eq("*Created by: Alice*\n\nHello")
+    end
   end
 end