FIX: Respect force download when downloading secure media via lightbox (#10769)

FIX: Respect force download when downloading secure media via lightbox (#10769)

The download link on the lightbox for images was not downloading the image if the upload was marked secure, because the code in the upload controller route was not respecting the dl=1 param for force download.

This PR fixes this so the download link works for secure images as well as regular ligthboxed images.

diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index 366c065..814b65a 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -111,7 +111,7 @@ class UploadsController < ApplicationController
       if Discourse.store.internal?
         send_file_local_upload(upload)
       else
-        redirect_to Discourse.store.url_for(upload, force_download: params[:dl] == "1")
+        redirect_to Discourse.store.url_for(upload, force_download: force_download?)
       end
     else
       render_404
@@ -160,11 +160,13 @@ class UploadsController < ApplicationController
     # url_for figures out the full URL, handling multisite DBs,
     # and will return a presigned URL for the upload
     if path_with_ext.blank?
-      return redirect_to Discourse.store.url_for(upload)
+      return redirect_to Discourse.store.url_for(upload, force_download: force_download?)
     end
 
     redirect_to Discourse.store.signed_url_for_path(
-      path_with_ext, expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS
+      path_with_ext,
+      expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS,
+      force_download: force_download?
     )
   end
 
@@ -183,6 +185,10 @@ class UploadsController < ApplicationController
 
   protected
 
+  def force_download?
+    params[:dl] == "1"
+  end
+
   def xhr_not_allowed
     raise Discourse::InvalidParameters.new("XHR not allowed")
   end
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index d80e1ff..fcc2891 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -169,9 +169,9 @@ module FileStore
       url.sub(File.join("#{schema}#{absolute_base_url}", folder), File.join(SiteSetting.Upload.s3_cdn_url, "/"))
     end
 
-    def signed_url_for_path(path, expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS)
+    def signed_url_for_path(path, expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS, force_download: false)
       key = path.sub(absolute_base_url + "/", "")
-      presigned_url(key, expires_in: expires_in)
+      presigned_url(key, expires_in: expires_in, force_download: force_download)
     end
 
     def cache_avatar(avatar, user_id)
diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb
index 02b4c03..ca4d257 100644
--- a/spec/requests/uploads_controller_spec.rb
+++ b/spec/requests/uploads_controller_spec.rb
@@ -402,6 +402,14 @@ describe UploadsController do
           get upload.short_path
 
           expect(response).to redirect_to(Discourse.store.signed_url_for_path(Discourse.store.get_path_for_upload(upload)))
+          expect(response.header['Location']).not_to include('response-content-disposition=attachment')
+        end
+
+        it "respects the force download (dl) param" do
+          sign_in(user)
+          freeze_time
+          get upload.short_path, params: { dl: '1' }
+          expect(response.header['Location']).to include('response-content-disposition=attachment')
         end
 
         it "has the correct caching header" do

GitHub sha: 4193eb04

This commit appears in #10769 which was approved by pmusaraj. It was merged by martin.