From 5b75b8c135187a11604c5108e86e0240033f3f16 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Tue, 7 Jan 2020 14:02:17 +1000
Subject: [PATCH] Still redirect to signed URL for secure uploads

if SiteSetting.secure_media is disabled we still want to
redirect to the signed url for uploads that are marked as
secure because their ACLs are probably still private
---
 app/controllers/uploads_controller.rb    | 11 ++++---
 spec/requests/uploads_controller_spec.rb | 37 +++++++++++++++++++-----
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index d5db4cbb23f..fa172664a38 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -126,14 +126,17 @@ class UploadsController < ApplicationController
     upload = Upload.find_by(sha1: sha1)
     return render_404 if upload.blank?
 
-    if SiteSetting.secure_media?
-      return redirect_to Discourse.store.signed_url_for_path(path_with_ext)
-    end
+    signed_secure_url = Discourse.store.signed_url_for_path(path_with_ext)
+    return redirect_to signed_secure_url if SiteSetting.secure_media?
 
     # we don't want to 404 here if secure media gets disabled
     # because all posts with secure uploads will show broken media
     # until rebaked, which could take some time
-    redirect_to Discourse.store.cdn_url(upload.url)
+    #
+    # if the upload is still secure, that means the ACL is probably still
+    # private, so we don't want to go to the CDN url just yet otherwise we
+    # will get a 403. if the upload is not secure we assume the ACL is public
+    redirect_to upload.secure? ? signed_secure_url : Discourse.store.cdn_url(upload.url)
   end
 
   def metadata
diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb
index 8e88f75244f..665fa90cfef 100644
--- a/spec/requests/uploads_controller_spec.rb
+++ b/spec/requests/uploads_controller_spec.rb
@@ -437,15 +437,38 @@ describe UploadsController do
           SiteSetting.secure_media = false
         end
 
-        it "should redirect to the regular show route" do
-          secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
-          sign_in(user)
-          stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+        context "if the upload is secure false, meaning the ACL is probably public" do
+          before do
+            upload.update(secure: false)
+          end
 
-          get secure_url
+          it "should redirect to the regular show route" do
+            secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
+            sign_in(user)
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
 
-          expect(response.status).to eq(302)
-          expect(response.redirect_url).to eq(Discourse.store.cdn_url(upload.url))
+            get secure_url
+
+            expect(response.status).to eq(302)
+            expect(response.redirect_url).to eq(Discourse.store.cdn_url(upload.url))
+          end
+        end
+
+        context "if the upload is secure true, meaning the ACL is probably private" do
+          before do
+            upload.update(secure: true)
+          end
+
+          it "should redirect to the presigned URL still otherwise we will get a 403" do
+            secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
+            sign_in(user)
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+
+            get secure_url
+
+            expect(response.status).to eq(302)
+            expect(response.redirect_url).to match("Amz-Expires")
+          end
         end
       end
     end