Fix 500 error when diff context line has broken encoding

Rugged sometimes chops a context line in between bytes, resulting in the context
line having an invalid encoding: https://github.com/libgit2/rugged/issues/716

When that happens, we will try to detect the encoding for the diff, and
sometimes we'll get it wrong. If that difference in encoding results in a
difference in string lengths between the diff and the underlying blobs, we'd
fail to highlight any inline diffs, and return a 500 status to the user.

As we're using the underlying blobs, the encoding is 'correct' anyway, so if the
string range is invalid, we can just discard the inline diff highlighting. We
still report to Sentry to ensure that we can investigate further in future.
This commit is contained in:
Sean McGivern 2018-02-21 13:42:11 +00:00
parent 9ff91bc481
commit cdf3ae04f8
3 changed files with 38 additions and 1 deletions

View file

@ -0,0 +1,5 @@
---
title: Fix 500 error being shown when diff has context marker with invalid encoding
merge_request:
author:
type: fixed

View file

@ -27,7 +27,17 @@ module Gitlab
rich_line = highlight_line(diff_line) || diff_line.text
if line_inline_diffs = inline_diffs[i]
rich_line = InlineDiffMarker.new(diff_line.text, rich_line).mark(line_inline_diffs)
begin
rich_line = InlineDiffMarker.new(diff_line.text, rich_line).mark(line_inline_diffs)
# This should only happen when the encoding of the diff doesn't
# match the blob, which is a bug. But we shouldn't fail to render
# completely in that case, even though we want to report the error.
rescue RangeError => e
if Gitlab::Sentry.enabled?
Gitlab::Sentry.context
Raven.capture_exception(e)
end
end
end
diff_line.text = rich_line

View file

@ -72,6 +72,28 @@ describe Gitlab::Diff::Highlight do
expect(subject[5].text).to eq(code)
expect(subject[5].text).to be_html_safe
end
context 'when the inline diff marker has an invalid range' do
before do
allow_any_instance_of(Gitlab::Diff::InlineDiffMarker).to receive(:mark).and_raise(RangeError)
end
it 'keeps the original rich line' do
code = %q{+ raise RuntimeError, "System commands must be given as an array of strings"}
expect(subject[5].text).to eq(code)
expect(subject[5].text).not_to be_html_safe
end
it 'reports to Sentry if configured' do
allow(Gitlab::Sentry).to receive(:enabled?).and_return(true)
expect(Gitlab::Sentry).to receive(:context)
expect(Raven).to receive(:capture_exception)
subject
end
end
end
end
end