From 6552197eecfd355041ecb99f48a48efb93c5ffab Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Thu, 14 Mar 2019 12:40:10 -0700
Subject: [PATCH] Fix error creating a merge request when diff includes a null
 byte

If a diff happened to include a single null byte anywhere, insertion
into the database would fail with an Error 500 since the column is text
and not a byte array. To fix this, we mark the diff as binary if we
detect a single null byte and Base64-encode it.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/57710
---
 app/models/merge_request_diff.rb              |  7 ++++++-
 ...ndle-null-bytes-in-merge-request-diffs.yml |  5 +++++
 spec/models/merge_request_diff_spec.rb        | 21 +++++++++++++++++++
 spec/support/helpers/repo_helpers.rb          | 14 +++++++++++++
 4 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml

diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index e53e2c8fc43..98db1bf7de7 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -296,6 +296,11 @@ class MergeRequestDiff < ActiveRecord::Base
 
   private
 
+  def encode_in_base64?(diff_text)
+    (diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?) ||
+      diff_text.include?("\0")
+  end
+
   def create_merge_request_diff_files(diffs)
     rows =
       if has_attribute?(:external_diff) && Gitlab.config.external_diffs.enabled
@@ -348,7 +353,7 @@ class MergeRequestDiff < ActiveRecord::Base
       diff_hash.tap do |hash|
         diff_text = hash[:diff]
 
-        if diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?
+        if encode_in_base64?(diff_text)
           hash[:binary] = true
           hash[:diff] = [diff_text].pack('m0')
         end
diff --git a/changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml b/changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml
new file mode 100644
index 00000000000..01b6b08b61b
--- /dev/null
+++ b/changelogs/unreleased/sh-handle-null-bytes-in-merge-request-diffs.yml
@@ -0,0 +1,5 @@
+---
+title: Fix error creating a merge request when diff includes a null byte
+merge_request: 26190
+author:
+type: fixed
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index e530e0302f5..53f5307ea0b 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe MergeRequestDiff do
+  include RepoHelpers
+
   let(:diff_with_commits) { create(:merge_request).merge_request_diff }
 
   describe 'validations' do
@@ -194,6 +196,25 @@ describe MergeRequestDiff do
         expect(diff_file).to be_binary
         expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [path]).to_a.first.diff)
       end
+
+      context 'with diffs that contain a null byte' do
+        let(:filename) { 'test-null.txt' }
+        let(:content) { "a" * 10000 + "\x00" }
+        let(:project) { create(:project, :repository) }
+        let(:branch) { 'null-data' }
+        let(:target_branch) { 'master' }
+
+        it 'saves diffs correctly' do
+          create_file_in_repo(project, target_branch, branch, filename, content)
+
+          mr_diff = create(:merge_request, target_project: project, source_project: project, source_branch: branch, target_branch: target_branch).merge_request_diff
+          diff_file = mr_diff.merge_request_diff_files.find_by(new_path: filename)
+
+          expect(diff_file).to be_binary
+          expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [filename]).to_a.first.diff)
+          expect(diff_file.diff).to include(content)
+        end
+      end
     end
   end
 
diff --git a/spec/support/helpers/repo_helpers.rb b/spec/support/helpers/repo_helpers.rb
index 3c6956cf5e0..4af90f4af79 100644
--- a/spec/support/helpers/repo_helpers.rb
+++ b/spec/support/helpers/repo_helpers.rb
@@ -115,4 +115,18 @@ eos
       commits: commits
     )
   end
+
+  def create_file_in_repo(
+        project, start_branch, branch_name, filename, content,
+        commit_message: 'Add new content')
+    Files::CreateService.new(
+      project,
+      project.owner,
+      commit_message: commit_message,
+      start_branch: start_branch,
+      branch_name: branch_name,
+      file_path: filename,
+      file_content: content
+    ).execute
+  end
 end