Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee

13 files changed, 94 insertions, 26 deletions

diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue
index d4d3038f066..5a6b1c19027 100644
--- a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue
+++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue
@@ -1,5 +1,5 @@
 <template>
   <div class="nothing-here-block">
-    {{ __('This diff was suppressed by a .gitattributes entry.') }}
+    {{ __("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") }}
   </div>
 </template>
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index ea51dca8a42..5248834a2f2 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -20,7 +20,7 @@ class ProtectedBranch::PushAccessLevel < ApplicationRecord
 
   def check_access(user)
     if user && deploy_key.present?
-      return true if user.can?(:read_project, project) && enabled_deploy_key_for_user?(deploy_key, user)
+      return user.can?(:read_project, project) && enabled_deploy_key_for_user?(deploy_key, user)
     end
 
     super
diff --git a/app/views/projects/diffs/viewers/_not_diffable.html.haml b/app/views/projects/diffs/viewers/_not_diffable.html.haml
index 7c55e272f56..63034331f6a 100644
--- a/app/views/projects/diffs/viewers/_not_diffable.html.haml
+++ b/app/views/projects/diffs/viewers/_not_diffable.html.haml
@@ -1,2 +1,2 @@
 .nothing-here-block
-  = _("This diff was suppressed by a .gitattributes entry.")
+  = _("File suppressed by a .gitattributes entry or the file's encoding is unsupported.")
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index dcd4bbdabf5..35581952f4a 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -250,7 +250,7 @@ module Gitlab
       end
 
       def diffable?
-        repository.attributes(file_path).fetch('diff') { true }
+        diffable_by_attribute? && !text_with_binary_notice?
       end
 
       def binary_in_repo?
@@ -366,6 +366,15 @@ module Gitlab
 
       private
 
+      def diffable_by_attribute?
+        repository.attributes(file_path).fetch('diff') { true }
+      end
+
+      # NOTE: Files with unsupported encodings (e.g. UTF-16) are treated as binary by git, but they are recognized as text files during encoding detection. These files have `Binary files a/filename and b/filename differ' as their raw diff content which cannot be used. We need to handle this special case and avoid displaying incorrect diff.
+      def text_with_binary_notice?
+        text? && has_binary_notice?
+      end
+
       def fetch_blob(sha, path)
         return unless sha
 
diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb
index 4a47e4b80b6..adb711ca89f 100644
--- a/lib/gitlab/diff/parser.rb
+++ b/lib/gitlab/diff/parser.rb
@@ -6,7 +6,7 @@ module Gitlab
       include Enumerable
 
       def parse(lines, diff_file: nil)
-        return [] if lines.blank?
+        return [] if lines.blank? || Git::Diff.has_binary_notice?(lines.first)
 
         @lines = lines
         line_obj_index = 0
diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb
index 53df0b7b389..8325eadce2f 100644
--- a/lib/gitlab/git/diff.rb
+++ b/lib/gitlab/git/diff.rb
@@ -33,6 +33,8 @@ module Gitlab
 
       SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze
 
+      BINARY_NOTICE_PATTERN = %r(Binary files a\/(.*) and b\/(.*) differ).freeze
+
       class << self
         def between(repo, head, base, options = {}, *paths)
           straight = options.delete(:straight) || false
@@ -131,8 +133,13 @@ module Gitlab
         def patch_hard_limit_bytes
           Gitlab::CurrentSettings.diff_max_patch_bytes
         end
-      end
 
+        def has_binary_notice?(text)
+          return false unless text.present?
+
+          text.start_with?(BINARY_NOTICE_PATTERN)
+        end
+      end
       def initialize(raw_diff, expanded: true)
         @expanded = expanded
 
@@ -215,7 +222,7 @@ module Gitlab
       end
 
       def has_binary_notice?
-        @diff.start_with?('Binary')
+        self.class.has_binary_notice?(@diff)
       end
 
       private
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index a64a2cdb714..1b2f0b26cda 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -14015,6 +14015,9 @@ msgstr ""
 msgid "File renamed with no changes."
 msgstr ""
 
+msgid "File suppressed by a .gitattributes entry or the file's encoding is unsupported."
+msgstr ""
+
 msgid "File synchronization concurrency limit"
 msgstr ""
 
@@ -33097,9 +33100,6 @@ msgstr ""
 msgid "This diff is collapsed."
 msgstr ""
 
-msgid "This diff was suppressed by a .gitattributes entry."
-msgstr ""
-
 msgid "This directory"
 msgstr ""
 
diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb
index cbd1ae628d1..add4af2bcdb 100644
--- a/spec/features/expand_collapse_diffs_spec.rb
+++ b/spec/features/expand_collapse_diffs_spec.rb
@@ -253,7 +253,7 @@ RSpec.describe 'Expand and collapse diffs', :js do
       click_link('Expand all')
 
       # Wait for elements to appear to ensure full page reload
-      expect(page).to have_content('This diff was suppressed by a .gitattributes entry')
+      expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.")
       expect(page).to have_content('This source diff could not be displayed because it is too large.')
       expect(page).to have_content('too_large_image.jpg')
       find('.note-textarea')
diff --git a/spec/features/projects/diffs/diff_show_spec.rb b/spec/features/projects/diffs/diff_show_spec.rb
index e47f36c4b7a..56506ada3ce 100644
--- a/spec/features/projects/diffs/diff_show_spec.rb
+++ b/spec/features/projects/diffs/diff_show_spec.rb
@@ -174,4 +174,14 @@ RSpec.describe 'Diff file viewer', :js, :with_clean_rails_cache do
       end
     end
   end
+
+  context 'when the the encoding of the file is unsupported' do
+    before do
+      visit_commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3')
+    end
+
+    it 'shows it is not diffable' do
+      expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.")
+    end
+  end
 end
diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
index 03a9b9bd21e..d401c42fed7 100644
--- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
+++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
@@ -23,7 +23,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiff do
 
     it 'does not highlight files marked as undiffable in .gitattributes' do
       allow_next_instance_of(Gitlab::Diff::File) do |instance|
-        allow(instance).to receive(:diffable?).and_return(false)
+        allow(instance).to receive(:diffable_by_attribute?).and_return(false)
       end
 
       expect_next_instance_of(Gitlab::Diff::File) do |instance|
diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index 78be89c449b..1800d2d6b60 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -186,26 +186,46 @@ RSpec.describe Gitlab::Diff::File do
   end
 
   describe '#diffable?' do
-    let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }
-    let(:diffs) { commit.diffs }
+    context 'when attributes exist' do
+      let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') }
+      let(:diffs) { commit.diffs }
 
-    before do
-      info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-        File.join(project.repository.path_to_repo, 'info')
+      before do
+        info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+          File.join(project.repository.path_to_repo, 'info')
+        end
+
+        FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path)
+        File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n")
       end
 
-      FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path)
-      File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n")
+      it "returns true for files that do not have attributes" do
+        diff_file = diffs.diff_file_with_new_path('LICENSE')
+        expect(diff_file.diffable?).to be_truthy
+      end
+
+      it "returns false for files that have been marked as not being diffable in attributes" do
+        diff_file = diffs.diff_file_with_new_path('README.md')
+        expect(diff_file.diffable?).to be_falsey
+      end
     end
 
-    it "returns true for files that do not have attributes" do
-      diff_file = diffs.diff_file_with_new_path('LICENSE')
-      expect(diff_file.diffable?).to be_truthy
+    context 'when the text has binary notice' do
+      let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') }
+      let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') }
+
+      it "returns false" do
+        expect(diff_file.diffable?).to be_falsey
+      end
     end
 
-    it "returns false for files that have been marked as not being diffable in attributes" do
-      diff_file = diffs.diff_file_with_new_path('README.md')
-      expect(diff_file.diffable?).to be_falsey
+    context 'when the content is binary' do
+      let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') }
+      let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') }
+
+      it "returns true" do
+        expect(diff_file.diffable?).to be_truthy
+      end
     end
   end
 
@@ -729,6 +749,18 @@ RSpec.describe Gitlab::Diff::File do
     end
   end
 
+  context 'when the the encoding of the file is unsupported' do
+    let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') }
+    let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') }
+
+    it 'returns a Not Diffable viewer' do
+      expect(diff_file.simple_viewer).to be_a(DiffViewer::NotDiffable)
+    end
+
+    it { expect(diff_file.highlighted_diff_lines).to eq([]) }
+    it { expect(diff_file.parallel_diff_lines).to eq([]) }
+  end
+
   describe '#diff_hunk' do
     context 'when first line is a match' do
       let(:raw_diff) do
diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb
index 7448ae0b2ea..c8069f82f04 100644
--- a/spec/lib/gitlab/diff/parser_spec.rb
+++ b/spec/lib/gitlab/diff/parser_spec.rb
@@ -146,6 +146,16 @@ eos
     it { expect(parser.parse(nil)).to eq([]) }
   end
 
+  context 'when it is a binary notice' do
+    let(:diff)  do
+      <<~END
+        Binary files a/test and b/test differ
+      END
+    end
+
+    it { expect(parser.parse(diff.each_line)).to eq([]) }
+  end
+
   describe 'tolerates special diff markers in a content' do
     it "counts lines correctly" do
       diff = <<~END
diff --git a/spec/models/protected_branch/push_access_level_spec.rb b/spec/models/protected_branch/push_access_level_spec.rb
index 17a589f0485..fa84cd660cb 100644
--- a/spec/models/protected_branch/push_access_level_spec.rb
+++ b/spec/models/protected_branch/push_access_level_spec.rb
@@ -44,7 +44,7 @@ RSpec.describe ProtectedBranch::PushAccessLevel do
     let(:can_push) { true }
 
     before_all do
-      project.add_guest(user)
+      project.add_maintainer(user)
     end
 
     context 'when this push_access_level is tied to a deploy key' do