Commit 6552197e authored by Stan Hu's avatar Stan Hu

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
parent 1715622c
...@@ -296,6 +296,11 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -296,6 +296,11 @@ class MergeRequestDiff < ActiveRecord::Base
private 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) def create_merge_request_diff_files(diffs)
rows = rows =
if has_attribute?(:external_diff) && Gitlab.config.external_diffs.enabled if has_attribute?(:external_diff) && Gitlab.config.external_diffs.enabled
...@@ -348,7 +353,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -348,7 +353,7 @@ class MergeRequestDiff < ActiveRecord::Base
diff_hash.tap do |hash| diff_hash.tap do |hash|
diff_text = hash[:diff] diff_text = hash[:diff]
if diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only? if encode_in_base64?(diff_text)
hash[:binary] = true hash[:binary] = true
hash[:diff] = [diff_text].pack('m0') hash[:diff] = [diff_text].pack('m0')
end end
......
---
title: Fix error creating a merge request when diff includes a null byte
merge_request: 26190
author:
type: fixed
require 'spec_helper' require 'spec_helper'
describe MergeRequestDiff do describe MergeRequestDiff do
include RepoHelpers
let(:diff_with_commits) { create(:merge_request).merge_request_diff } let(:diff_with_commits) { create(:merge_request).merge_request_diff }
describe 'validations' do describe 'validations' do
...@@ -194,6 +196,25 @@ describe MergeRequestDiff do ...@@ -194,6 +196,25 @@ describe MergeRequestDiff do
expect(diff_file).to be_binary expect(diff_file).to be_binary
expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [path]).to_a.first.diff) expect(diff_file.diff).to eq(mr_diff.compare.diffs(paths: [path]).to_a.first.diff)
end 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
end end
......
...@@ -115,4 +115,18 @@ eos ...@@ -115,4 +115,18 @@ eos
commits: commits commits: commits
) )
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment