From 0fdfd2dd6e01648f4daf6853f11a3ffc9a678a55 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 23 May 2016 22:59:35 -0700 Subject: [PATCH] Fix Error 500 when viewing a blob with binary characters after the 1024-byte mark Here was the problem: 1. When determining whether a given blob is viewable text, gitlab_git reads the first 1024 bytes and checks with Linguist whether it is a text or binary file. 2. If the blob is text, GitLab will attempt to display it. 3. However, if the text has binary characters after the first 1024 bytes, then GitLab will attempt to load the entire contents, but the encoding will be ASCII-8BIT since there are binary characters. 4. The Error 500 results when GitLab attempts to display a mix UTF-8 and ASCII-8BIT. To fix this, we load as much data as we are willing to display so that the detection will work properly. Requires an update to gitlab_git: gitlab-org/gitlab_git!86 Closes #13826 --- CHANGELOG | 1 + Gemfile.lock | 4 ++-- app/helpers/blob_helper.rb | 2 +- app/models/blob.rb | 2 +- app/models/repository.rb | 2 +- app/views/projects/diffs/_diffs.html.haml | 1 + app/views/projects/diffs/_file.html.haml | 2 ++ spec/controllers/blob_controller_spec.rb | 5 +++++ spec/controllers/projects/commit_controller_spec.rb | 12 ++++++++++++ spec/support/test_env.rb | 1 + 10 files changed, 27 insertions(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 8f897b4a34..acb349594a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -18,6 +18,7 @@ v 8.9.0 (unreleased) - Reduce number of fog gem dependencies - Remove project notification settings associated with deleted projects - Fix 404 page when viewing TODOs that contain milestones or labels in different projects + - Fix Error 500 when viewing a blob with binary characters after the 1024-byte mark - Redesign navigation for project pages - Fix groups API to list only user's accessible projects - Redesign account and email confirmation emails diff --git a/Gemfile.lock b/Gemfile.lock index dfc1570049..15b3158c63 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -284,7 +284,7 @@ GEM posix-spawn (~> 0.3) gitlab_emoji (0.3.1) gemojione (~> 2.2, >= 2.2.1) - gitlab_git (10.1.0) + gitlab_git (10.1.3) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -408,7 +408,7 @@ GEM mime-types (>= 1.16, < 4) mail_room (0.7.0) method_source (0.8.2) - mime-types (2.99.1) + mime-types (2.99.2) mimemagic (0.3.0) mini_portile2 (2.1.0) minitest (5.7.0) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index cec2dc753f..85559fbc5f 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -116,7 +116,7 @@ module BlobHelper end def blob_text_viewable?(blob) - blob && blob.text? && !blob.lfs_pointer? + blob && blob.text? && !blob.lfs_pointer? && !blob.only_display_raw? end def blob_size(blob) diff --git a/app/models/blob.rb b/app/models/blob.rb index 0fea6b7f57..4279ea2ce5 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -24,7 +24,7 @@ class Blob < SimpleDelegator end def only_display_raw? - size && size > 5.megabytes + size && truncated? end def svg? diff --git a/app/models/repository.rb b/app/models/repository.rb index 1ab163510b..e5b277cb19 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -446,7 +446,7 @@ class Repository def blob_at(sha, path) unless Gitlab::Git.blank_ref?(sha) - Gitlab::Git::Blob.find(self, sha, path) + Blob.decorate(Gitlab::Git::Blob.find(self, sha, path)) end end diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index d9c4b410d3..6c11afbe42 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -24,6 +24,7 @@ - diff_commit = commit_for_diff(diff_file) - blob = project.repository.blob_for_diff(diff_commit, diff_file) - next unless blob + - blob.load_all_data!(project.repository) unless blob.only_display_raw? = render 'projects/diffs/file', i: index, project: project, diff_file: diff_file, diff_commit: diff_commit, blob: blob, diff_refs: diff_refs diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index e5983c5803..2395ea3c27 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -49,6 +49,8 @@ = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i - else = render "projects/diffs/text_file", diff_file: diff_file, index: i + - elsif blob.only_display_raw? + .nothing-here-block This file is too large to display. - elsif blob.image? - old_file = project.repository.prev_blob_for_diff(diff_commit, diff_file) = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i, diff_refs: diff_refs diff --git a/spec/controllers/blob_controller_spec.rb b/spec/controllers/blob_controller_spec.rb index eb91e577b8..465013231f 100644 --- a/spec/controllers/blob_controller_spec.rb +++ b/spec/controllers/blob_controller_spec.rb @@ -38,6 +38,11 @@ describe Projects::BlobController do let(:id) { 'invalid-branch/README.md' } it { is_expected.to respond_with(:not_found) } end + + context "binary file" do + let(:id) { 'binary-encoding/encoding/binary-1.bin' } + it { is_expected.to respond_with(:success) } + end end describe 'GET show with tree path' do diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index 438e776ec4..6e3db10e45 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -2,6 +2,8 @@ require 'rails_helper' describe Projects::CommitController do describe 'GET show' do + render_views + let(:project) { create(:project) } before do @@ -27,6 +29,16 @@ describe Projects::CommitController do end end + it 'handles binary files' do + get(:show, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: TestEnv::BRANCH_SHA['binary-encoding'], + format: "html") + + expect(response).to be_success + end + def go(id:) get :show, namespace_id: project.namespace.to_param, diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 71664bb192..498bd4bf80 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -16,6 +16,7 @@ module TestEnv 'master' => '5937ac0', "'test'" => 'e56497b', 'orphaned-branch' => '45127a9', + 'binary-encoding' => '7b1cf43', } # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily