From 03805e5a76cc6bfc54cc159205ac71704ac13f04 Mon Sep 17 00:00:00 2001
From: Penar Musaraj <pmusaraj@gmail.com>
Date: Thu, 4 Jul 2019 11:32:51 -0400
Subject: [PATCH] FIX: Ensure lightbox image download has correct content
 disposition in S3 (#7845)

---
 app/controllers/uploads_controller.rb       |  2 +-
 lib/cooked_post_processor.rb                |  2 +-
 lib/file_store/s3_store.rb                  | 14 ++++++++---
 spec/components/file_store/s3_store_spec.rb | 27 +++++++++++++++++++++
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index f1841a48603..2fe9c0c9c94 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -104,7 +104,7 @@ class UploadsController < ApplicationController
       if Discourse.store.internal?
         send_file_local_upload(upload)
       else
-        redirect_to Discourse.store.url_for(upload)
+        redirect_to Discourse.store.url_for(upload, force_download: params[:dl] == "1")
       end
     else
       render_404
diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb
index d4ec97ac640..1cd4b90c05f 100644
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@@ -382,7 +382,7 @@ class CookedPostProcessor
     a = create_link_node("lightbox", img["src"])
     img.add_next_sibling(a)
 
-    if upload && Discourse.store.internal?
+    if upload
       a["data-download-href"] = Discourse.store.download_url(upload)
     end
 
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index 4389ba3d051..feb3ca8d2b1 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -99,15 +99,23 @@ module FileStore
       File.join("uploads", "tombstone", RailsMultisite::ConnectionManagement.current_db, "/")
     end
 
+    def download_url(upload)
+      return unless upload
+      "#{upload.short_path}?dl=1"
+    end
+
     def path_for(upload)
       url = upload&.url
       FileStore::LocalStore.new.path_for(upload) if url && url[/^\/[^\/]/]
     end
 
-    def url_for(upload)
-      if upload.private?
+    def url_for(upload, force_download: false)
+      if upload.private? || force_download
+        opts = { expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS }
+        opts[:response_content_disposition] = "attachment; filename=\"#{upload.original_filename}\"" if force_download
+
         obj = @s3_helper.object(get_upload_key(upload))
-        url = obj.presigned_url(:get, expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS)
+        url = obj.presigned_url(:get, opts)
       else
         url = upload.url
       end
diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb
index 0399efcedb8..f9863311360 100644
--- a/spec/components/file_store/s3_store_spec.rb
+++ b/spec/components/file_store/s3_store_spec.rb
@@ -395,4 +395,31 @@ describe FileStore::S3Store do
     end
   end
 
+  describe ".download_url" do
+    include_context "s3 helpers"
+    let(:s3_object) { stub }
+
+    it "returns correct short URL with dl=1 param" do
+      expect(store.download_url(upload)).to eq("#{upload.short_path}?dl=1")
+    end
+  end
+
+  describe ".url_for" do
+    include_context "s3 helpers"
+    let(:s3_object) { stub }
+
+    it "returns signed URL with content disposition when requesting to download image" do
+      s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+      s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object)
+      opts = {
+        expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS,
+        response_content_disposition: "attachment; filename=\"#{upload.original_filename}\""
+      }
+
+      s3_object.expects(:presigned_url).with(:get, opts)
+
+      expect(store.url_for(upload, force_download: true)).not_to eq(upload.url)
+    end
+  end
+
 end