Merge branch 'issue_7105' into 'master'

Allow pdf files to be opened on browser

See merge request gitlab-org/gitlab!21272

app/controllers/concerns/uploads_actions.rb

@@ -44,15 +44,14 @@ module UploadsActions
 
     expires_in ttl, directives
 
-    disposition = uploader.embeddable? ? 'inline' : 'attachment'
-
-    uploaders = [uploader, *uploader.versions.values]
-    uploader = uploaders.find { |version| version.filename == params[:filename] }
+    file_uploader = [uploader, *uploader.versions.values].find do |version|
+      version.filename == params[:filename]
+    end
 
-    return render_404 unless uploader
+    return render_404 unless file_uploader
 
     workhorse_set_content_type!
-    send_upload(uploader, attachment: uploader.filename, disposition: disposition)
+    send_upload(file_uploader, attachment: file_uploader.filename, disposition: content_disposition)
   end
 
   def authorize
@@ -83,6 +82,14 @@ module UploadsActions
     end
   end
 
+  def content_disposition
+    if uploader.embeddable? || uploader.pdf?
+      'inline'
+    else
+      'attachment'
+    end
+  end
+
   def uploader_class
     raise NotImplementedError
   end

changelogs/unreleased/issue_7105.yml

+---
+title: Allow PDF attachments to be opened on browser
+merge_request: 21272
+author:
+type: added

lib/gitlab/file_type_detection.rb

@@ -20,6 +20,7 @@
 module Gitlab
   module FileTypeDetection
     SAFE_IMAGE_EXT = %w[png jpg jpeg gif bmp tiff ico].freeze
+    PDF_EXT = 'pdf'
     # We recommend using the .mp4 format over .mov. Videos in .mov format can
     # still be used but you really need to make sure they are served with the
     # proper MIME type video/mp4 and not video/quicktime or your videos won't play
@@ -46,6 +47,10 @@ module Gitlab
       extension_match?(SAFE_AUDIO_EXT)
     end
 
+    def pdf?
+      extension_match?([PDF_EXT])
+    end
+
     def embeddable?
       image? || video? || audio?
     end

spec/controllers/uploads_controller_spec.rb

@@ -196,24 +196,39 @@ describe UploadsController do
 
   describe "GET show" do
     context 'Content-Disposition security measures' do
+      let(:expected_disposition) { 'inline;' }
       let(:project) { create(:project, :public) }
 
-      context 'for PNG files' do
-        it 'returns Content-Disposition: inline' do
-          note = create(:note, :with_attachment, project: project)
-          get :show, params: { model: 'note', mounted_as: 'attachment', id: note.id, filename: 'dk.png' }
+      shared_examples_for 'uploaded file with disposition' do
+        it 'returns correct Content-Disposition' do
+          get :show, params: { model: 'note', mounted_as: 'attachment', id: note.id, filename: filename }
 
-          expect(response['Content-Disposition']).to start_with('inline;')
+          expect(response['Content-Disposition']).to start_with(expected_disposition)
         end
       end
 
+      context 'for PNG files' do
+        let(:filename) { 'dk.png' }
+        let(:expected_disposition) { 'inline;' }
+        let(:note) { create(:note, :with_attachment, project: project) }
+
+        it_behaves_like 'uploaded file with disposition'
+      end
+
+      context 'for PDF files' do
+        let(:filename) { 'git-cheat-sheet.pdf' }
+        let(:expected_disposition) { 'inline;' }
+        let(:note) { create(:note, :with_pdf_attachment, project: project) }
+
+        it_behaves_like 'uploaded file with disposition'
+      end
+
       context 'for SVG files' do
-        it 'returns Content-Disposition: attachment' do
-          note = create(:note, :with_svg_attachment, project: project)
-          get :show, params: { model: 'note', mounted_as: 'attachment', id: note.id, filename: 'unsanitized.svg' }
+        let(:filename) { 'unsanitized.svg' }
+        let(:expected_disposition) { 'attachment;' }
+        let(:note) { create(:note, :with_svg_attachment, project: project) }
 
-          expect(response['Content-Disposition']).to start_with('attachment;')
-        end
+        it_behaves_like 'uploaded file with disposition'
       end
     end
 

spec/factories/notes.rb

@@ -167,6 +167,10 @@ FactoryBot.define do
       attachment { fixture_file_upload("spec/fixtures/unsanitized.svg", "image/svg+xml") }
     end
 
+    trait :with_pdf_attachment do
+      attachment { fixture_file_upload("spec/fixtures/git-cheat-sheet.pdf", "application/pdf") }
+    end
+
     transient do
       in_reply_to { nil }
     end