Fix saving diffs that are not valid UTF-835539-can-t-create-a-merge-request-containing-a-binary-file-with-non-utf-8-characters

Previously, we used Psych, which would:

1. Check if a string was encoded as binary, and not ASCII-compatible.
2. Add the !binary tag in that case.
3. Convert to base64.

We need to do the same thing, using a new column in place of the tag.

10 files changed, 79 insertions, 9 deletions

diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 4b141945ab4..ec87aee9310 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -236,10 +236,21 @@ class MergeRequestDiff < ActiveRecord::Base
 
   def create_merge_request_diff_files(diffs)
     rows = diffs.map.with_index do |diff, index|
-      diff.to_hash.merge(
+      diff_hash = diff.to_hash.merge(
+        binary: false,
         merge_request_diff_id: self.id,
         relative_order: index
       )
+
+      # Compatibility with old diffs created with Psych.
+      diff_hash.tap do |hash|
+        diff_text = hash[:diff]
+
+        if diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?
+          hash[:binary] = true
+          hash[:diff] = [diff_text].pack('m0')
+        end
+      end
     end
 
     Gitlab::Database.bulk_insert('merge_request_diff_files', rows)
@@ -268,9 +279,7 @@ class MergeRequestDiff < ActiveRecord::Base
           st_diffs
         end
       elsif merge_request_diff_files.present?
-        merge_request_diff_files
-          .as_json(only: Gitlab::Git::Diff::SERIALIZE_KEYS)
-          .map(&:with_indifferent_access)
+        merge_request_diff_files.map(&:to_hash)
       end
   end
 
diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb
index 598ebd4d829..1199ff5af22 100644
--- a/app/models/merge_request_diff_file.rb
+++ b/app/models/merge_request_diff_file.rb
@@ -8,4 +8,14 @@ class MergeRequestDiffFile < ActiveRecord::Base
 
     encode_utf8(diff) if diff.respond_to?(:encoding)
   end
+
+  def diff
+    binary? ? super.unpack('m0').first : super
+  end
+
+  def to_hash
+    keys = Gitlab::Git::Diff::SERIALIZE_KEYS - [:diff]
+
+    as_json(only: keys).merge(diff: diff).with_indifferent_access
+  end
 end
diff --git a/changelogs/unreleased/35539-can-t-create-a-merge-request-containing-a-binary-file-with-non-utf-8-characters.yml b/changelogs/unreleased/35539-can-t-create-a-merge-request-containing-a-binary-file-with-non-utf-8-characters.yml
new file mode 100644
index 00000000000..8d92aacc9ef
--- /dev/null
+++ b/changelogs/unreleased/35539-can-t-create-a-merge-request-containing-a-binary-file-with-non-utf-8-characters.yml
@@ -0,0 +1,5 @@
+---
+title: Fix creating merge request diffs when diff contains bytes that are invalid
+  in UTF-8
+merge_request:
+author:
diff --git a/db/migrate/20170725145659_add_binary_to_merge_request_diff_files.rb b/db/migrate/20170725145659_add_binary_to_merge_request_diff_files.rb
new file mode 100644
index 00000000000..1f5fa7e3d49
--- /dev/null
+++ b/db/migrate/20170725145659_add_binary_to_merge_request_diff_files.rb
@@ -0,0 +1,9 @@
+class AddBinaryToMergeRequestDiffFiles < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :merge_request_diff_files, :binary, :boolean
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 61bcd8c7e95..1ec25c7d46f 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170724214302) do
+ActiveRecord::Schema.define(version: 20170725145659) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -748,6 +748,7 @@ ActiveRecord::Schema.define(version: 20170724214302) do
     t.text "new_path", null: false
     t.text "old_path", null: false
     t.text "diff", null: false
+    t.boolean "binary"
   end
 
   add_index "merge_request_diff_files", ["merge_request_diff_id", "relative_order"], name: "index_merge_request_diff_files_on_mr_diff_id_and_order", unique: true, using: :btree
diff --git a/spec/features/projects/ref_switcher_spec.rb b/spec/features/projects/ref_switcher_spec.rb
index 31c7b492ab7..9f5544ac43e 100644
--- a/spec/features/projects/ref_switcher_spec.rb
+++ b/spec/features/projects/ref_switcher_spec.rb
@@ -19,14 +19,14 @@ feature 'Ref switcher', feature: true, js: true do
       input.set 'binary'
       wait_for_requests
 
-      expect(find('.dropdown-content ul')).to have_selector('li', count: 6)
+      expect(find('.dropdown-content ul')).to have_selector('li', count: 7)
 
       page.within '.dropdown-content ul' do
         input.native.send_keys :enter
       end
     end
 
-    expect(page).to have_title 'binary-encoding'
+    expect(page).to have_title 'add-pdf-text-binary'
   end
 
   it "user selects ref with special characters" do
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 0f2db3380a7..11f4c16ff96 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -195,6 +195,7 @@ MergeRequestDiffFile:
 - a_mode
 - b_mode
 - too_large
+- binary
 Ci::Pipeline:
 - id
 - project_id
diff --git a/spec/models/merge_request_diff_file_spec.rb b/spec/models/merge_request_diff_file_spec.rb
index 7276f5b5061..239620ef4fc 100644
--- a/spec/models/merge_request_diff_file_spec.rb
+++ b/spec/models/merge_request_diff_file_spec.rb
@@ -1,8 +1,33 @@
 require 'rails_helper'
 
 describe MergeRequestDiffFile, type: :model do
+  describe '#diff' do
+    let(:unpacked) { 'unpacked' }
+    let(:packed) { [unpacked].pack('m0') }
+
+    before do
+      subject.diff = packed
+    end
+
+    context 'when the diff is marked as binary' do
+      before do
+        subject.binary = true
+      end
+
+      it 'unpacks from base 64' do
+        expect(subject.diff).to eq(unpacked)
+      end
+    end
+
+    context 'when the diff is not marked as binary' do
+      it 'returns the raw diff' do
+        expect(subject.diff).to eq(packed)
+      end
+    end
+  end
+
   describe '#utf8_diff' do
-    it 'does not raise error when a hash value is in binary' do
+    it 'does not raise error when the diff is binary' do
       subject.diff = "\x05\x00\x68\x65\x6c\x6c\x6f"
 
       expect { subject.utf8_diff }.not_to raise_error
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index edc2f4bb9f0..0e77752bccc 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -105,6 +105,15 @@ describe MergeRequestDiff, models: true do
 
       expect(mr_diff.empty?).to be_truthy
     end
+
+    it 'saves binary diffs correctly' do
+      path = 'files/images/icn-time-tracking.pdf'
+      mr_diff = create(:merge_request, source_branch: 'add-pdf-text-binary', target_branch: 'master').merge_request_diff
+      diff_file = mr_diff.merge_request_diff_files.find_by(new_path: path)
+
+      expect(diff_file).to be_binary
+      expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [path]).to_a.first.diff)
+    end
   end
 
   describe '#commit_shas' do
diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb
index 0a194ca4c90..c32c05b03e2 100644
--- a/spec/support/test_env.rb
+++ b/spec/support/test_env.rb
@@ -41,7 +41,8 @@ module TestEnv
     'csv'                                => '3dd0896',
     'v1.1.0'                             => 'b83d6e3',
     'add-ipython-files'                  => '93ee732',
-    'add-pdf-file'                       => 'e774ebd'
+    'add-pdf-file'                       => 'e774ebd',
+    'add-pdf-text-binary'                => '79faa7b'
   }.freeze
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily