FIX: Mark secure media upload insecure automatically if used for theme component (#8413)

When uploading a file to a theme component, and that file is existing and has already been marked as secure, we now automatically mark the file as secure: false, change the ACL, and log the action as the user (also rebake the posts for the upload)

app/controllers/admin/themes_controller.rb

@@ -23,12 +23,24 @@ class Admin::ThemesController < Admin::AdminController
         if upload.errors.count > 0
           render_json_error upload
         else
+          # we assume a user intends to make some media public
+          # if they are uploading it to a theme component
+          mark_upload_insecure(upload) if upload.secure?
           render json: { upload_id: upload.id }, status: :created
         end
       end
     end
   end
 
+  def mark_upload_insecure(upload)
+    upload.update_secure_status(secure_override_value: false)
+    StaffActionLogger.new(current_user).log_change_upload_secure_status(
+      upload_id: upload.id,
+      new_value: false
+    )
+    Jobs.enqueue(:rebake_posts_for_upload, id: upload.id)
+  end
+
   def generate_key_pair
     require 'sshkey'
     k = SSHKey.generate

app/jobs/regular/rebake_posts_for_upload.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Jobs
+  class RebakePostsForUpload < ::Jobs::Base
+    def execute(args)
+      upload = Upload.find_by(id: args[:id])
+      return if upload.blank?
+      upload.posts.find_each(&:rebake!)
+    end
+  end
+end

app/models/upload.rb

@@ -230,9 +230,9 @@ class Upload < ActiveRecord::Base
     self.posts.where("cooked LIKE '%/_optimized/%'").find_each(&:rebake!)
   end
 
-  def update_secure_status
+  def update_secure_status(secure_override_value: nil)
     return false if self.for_theme || self.for_site_setting
-    mark_secure = should_be_secure?
+    mark_secure = secure_override_value.nil? ? should_be_secure? : secure_override_value
 
     self.update_column("secure", mark_secure)
     Discourse.store.update_upload_ACL(self) if Discourse.store.external?

app/models/user_history.rb

@@ -102,7 +102,8 @@ class UserHistory < ActiveRecord::Base
       api_key_update: 81,
       api_key_destroy: 82,
       revoke_title: 83,
-      change_title: 84
+      change_title: 84,
+      override_upload_secure_status: 85
     )
   end
 
@@ -181,7 +182,8 @@ class UserHistory < ActiveRecord::Base
       :change_title,
       :api_key_create,
       :api_key_update,
-      :api_key_destroy
+      :api_key_destroy,
+      :override_upload_secure_status
     ]
   end
 

app/services/staff_action_logger.rb

@@ -373,6 +373,17 @@ class StaffActionLogger
     ))
   end
 
+  def log_change_upload_secure_status(opts = {})
+    UserHistory.create!(params(opts).merge(
+      action: UserHistory.actions[:override_upload_secure_status],
+      details: [
+        "upload_id: #{opts[:upload_id]}",
+        "reason: #{I18n.t("uploads.marked_insecure_from_theme_component_reason")}"
+      ].join("\n"),
+      new_value: opts[:new_value]
+    ))
+  end
+
   def log_check_email(user, opts = {})
     raise Discourse::InvalidParameters.new(:user) unless user
     UserHistory.create!(params(opts).merge(

config/locales/client.en.yml

@@ -3936,6 +3936,7 @@ en:
             api_key_create: "api key create"
             api_key_update: "api key update"
             api_key_destroy: "api key destroy"
+            override_upload_secure_status: "override upload secure status"
         screened_emails:
           title: "Screened Emails"
           description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed."

config/locales/server.en.yml

@@ -971,6 +971,9 @@ en:
     topic_description: "To re-subscribe to %{link}, use the notification control at the bottom or right of the topic."
     private_topic_description: "To re-subscribe, use the notification control at the bottom or right of the topic."
 
+  uploads:
+    marked_insecure_from_theme_component_reason: "upload used in theme component"
+
   unsubscribe:
     title: "Unsubscribe"
     stop_watching_topic: "Stop watching this topic, %{link}"

spec/models/upload_spec.rb

@@ -289,6 +289,18 @@ describe Upload do
   end
 
   describe '.update_secure_status' do
+    it "respects the secure_override_value parameter if provided" do
+      upload.update!(secure: true)
+
+      upload.update_secure_status(secure_override_value: true)
+
+      expect(upload.secure).to eq(true)
+
+      upload.update_secure_status(secure_override_value: false)
+
+      expect(upload.secure).to eq(false)
+    end
+
     it 'marks a local upload as not secure with default settings' do
       upload.update!(secure: true)
       expect { upload.update_secure_status }

spec/requests/admin/themes_controller_spec.rb

@@ -37,6 +37,37 @@ describe Admin::ThemesController do
       expect(upload.id).not_to be_nil
       expect(JSON.parse(response.body)["upload_id"]).to eq(upload.id)
     end
+
+    context "when trying to upload an existing file" do
+      let(:uploaded_file) { Upload.find_by(original_filename: "fake.woff2") }
+      let(:response_json) { JSON.parse(response.body) }
+
+      before do
+        post "/admin/themes/upload_asset.json", params: { file: upload }
+        expect(response.status).to eq(201)
+      end
+
+      context "if the file is secure media" do
+        before do
+          uploaded_file.update_secure_status(secure_override_value: true)
+          upload.rewind
+        end
+
+        it "marks the upload as not secure" do
+          post "/admin/themes/upload_asset.json", params: { file: upload }
+          expect(response.status).to eq(201)
+          expect(response_json["upload_id"]).to eq(uploaded_file.id)
+          uploaded_file.reload
+          expect(uploaded_file.secure).to eq(false)
+        end
+
+        it "enqueues a job to rebake the posts for the upload" do
+          Jobs.expects(:enqueue).with(:rebake_posts_for_upload, id: uploaded_file.id)
+          post "/admin/themes/upload_asset.json", params: { file: upload }
+          expect(response.status).to eq(201)
+        end
+      end
+    end
   end
 
   describe '#export' do