From e7c7a0509749c24e17cc786d7c78594acaf03824 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 28 Nov 2019 07:32:17 +1000 Subject: [PATCH] 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 | 12 +++++++ app/jobs/regular/rebake_posts_for_upload.rb | 11 +++++++ app/models/upload.rb | 4 +-- app/models/user_history.rb | 6 ++-- app/services/staff_action_logger.rb | 11 +++++++ config/locales/client.en.yml | 1 + config/locales/server.en.yml | 3 ++ spec/models/upload_spec.rb | 12 +++++++ spec/requests/admin/themes_controller_spec.rb | 31 +++++++++++++++++++ 9 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 app/jobs/regular/rebake_posts_for_upload.rb diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index 116ed1b3224..43df7efca48 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/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 diff --git a/app/jobs/regular/rebake_posts_for_upload.rb b/app/jobs/regular/rebake_posts_for_upload.rb new file mode 100644 index 00000000000..a56b4336432 --- /dev/null +++ b/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 diff --git a/app/models/upload.rb b/app/models/upload.rb index 189a08f325c..a6842ce9a92 100644 --- a/app/models/upload.rb +++ b/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? diff --git a/app/models/user_history.rb b/app/models/user_history.rb index dcb11d5ea60..3adbb62eb1b 100644 --- a/app/models/user_history.rb +++ b/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 diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index c6cbb48d0fe..9130f346925 100644 --- a/app/services/staff_action_logger.rb +++ b/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( diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index bbd442acd99..36fa5c10730 100644 --- a/config/locales/client.en.yml +++ b/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." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 54e8ab91608..bb82cba5b67 100644 --- a/config/locales/server.en.yml +++ b/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}" diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 9f4a52de446..5d7cca2598a 100644 --- a/spec/models/upload_spec.rb +++ b/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 } diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index 17cdf9ea9fc..4be50292ecb 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/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