From 4193eb0419dd6dd9b3a3f23a4fa13763d518c4ae Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 29 Sep 2020 12:12:03 +1000 Subject: [PATCH] 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. --- app/controllers/uploads_controller.rb | 12 +++++++++--- lib/file_store/s3_store.rb | 4 ++-- spec/requests/uploads_controller_spec.rb | 8 ++++++++ 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 366c06548ac..814b65ac39d 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 d80e1ff5d75..fcc289123cf 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 02b4c035946..ca4d257bfa2 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