FEATURE: Add support for secure media (#7888)

This PR introduces a new secure media setting. When enabled, it prevent unathorized access to media uploads (files of type image, video and audio). When the `login_required` setting is enabled, then all media uploads will be protected from unauthorized (anonymous) access. When `login_required`is disabled, only media in private messages will be protected from unauthorized access. 

A few notes: 

- the `prevent_anons_from_downloading_files` setting no longer applies to audio and video uploads
- the `secure_media` setting can only be enabled if S3 uploads are already enabled and configured
- upload records have a new column, `secure`, which is a boolean `true/false` of the upload's secure status
- when creating a public post with an upload that has already been uploaded and is marked as secure, the post creator will raise an error
- when enabling or disabling the setting on a site with existing uploads, the rake task `uploads:ensure_correct_acl` should be used to update all uploads' secure status and their ACL on S3
This commit is contained in:
Penar Musaraj
2019-11-17 20:25:42 -05:00
committed by Martin Brennan
parent 56b19ba740
commit 102909edb3
40 changed files with 1157 additions and 153 deletions

View File

@@ -242,7 +242,6 @@ export function getUploadMarkdown(upload) {
upload.thumbnail_height
}](${upload.short_url || upload.url})`;
} else if (
!Discourse.SiteSettings.prevent_anons_from_downloading_files &&
/\.(mov|mp4|webm|ogv|mp3|ogg|wav|m4a)$/i.test(upload.original_filename)
) {
return uploadLocation(upload.url);

View File

@@ -5,7 +5,7 @@ require "mini_mime"
class UploadsController < ApplicationController
requires_login except: [:show, :show_short]
skip_before_action :preload_json, :check_xhr, :redirect_to_login_if_required, only: [:show, :show_short]
skip_before_action :preload_json, :check_xhr, :redirect_to_login_if_required, only: [:show, :show_short, :show_secure]
protect_from_forgery except: :show
def create
@@ -110,6 +110,17 @@ class UploadsController < ApplicationController
end
end
def show_secure
# do not serve uploads requested via XHR to prevent XSS
return xhr_not_allowed if request.xhr?
if SiteSetting.secure_media?
redirect_to Discourse.store.signed_url_for_path("#{params[:path]}.#{params[:extension]}")
else
render_404
end
end
def metadata
params.require(:url)
upload = Upload.get_from_url(params[:url])

View File

@@ -7,8 +7,8 @@ module Jobs
return if !SiteSetting.enable_s3_uploads
Upload.find_each do |upload|
if !FileHelper.is_supported_image?(upload.original_filename)
Discourse.store.update_upload_ACL(upload)
if !FileHelper.is_supported_media?(upload.original_filename)
upload.update_secure_status
end
end
end

View File

@@ -349,13 +349,26 @@ class UserNotifications < ActionMailer::Base
end
def email_post_markdown(post, add_posted_by = false)
result = +"#{post.raw}\n\n"
result = +"#{post.with_secure_media? ? strip_secure_urls(post.raw) : post.raw}\n\n"
if add_posted_by
result << "#{I18n.t('user_notifications.posted_by', username: post.username, post_date: post.created_at.strftime("%m/%d/%Y"))}\n\n"
end
result
end
def strip_secure_urls(raw)
urls = Set.new
raw.scan(URI.regexp(%w{http https})) { urls << $& }
urls.each do |url|
if (url.start_with?(Discourse.store.s3_upload_host) && FileHelper.is_supported_media?(url))
raw = raw.sub(url, "<p class='secure-media-notice'>#{I18n.t("emails.secure_media_placeholder")}</p>")
end
end
raw
end
def self.get_context_posts(post, topic_user, user)
if (user.user_option.email_previous_replies == UserOption.previous_replies_type[:never]) ||
SiteSetting.private_email?

View File

@@ -300,6 +300,15 @@ class Post < ActiveRecord::Base
options[:user_id] = post_user.id if post_user
options[:omit_nofollow] = true if omit_nofollow?
if self.with_secure_media?
each_upload_url do |url|
uri = URI.parse(url)
if FileHelper.is_supported_media?(File.basename(uri.path))
raw = raw.sub(Discourse.store.s3_upload_host, "#{Discourse.base_url}/secure-media-uploads")
end
end
end
cooked = post_analyzer.cook(raw, options)
new_cooked = Plugin::Filter.apply(:after_post_cook, self, cooked)
@@ -492,6 +501,11 @@ class Post < ActiveRecord::Base
ReviewableFlaggedPost.pending.find_by(target: self)
end
def with_secure_media?
return false unless SiteSetting.secure_media?
topic&.private_message? || SiteSetting.login_required?
end
def hide!(post_action_type_id, reason = nil)
return if hidden?
@@ -882,6 +896,13 @@ class Post < ActiveRecord::Base
end
upload_ids |= Upload.where(id: downloaded_images.values).pluck(:id)
disallowed_uploads = []
if SiteSetting.secure_media? && !topic&.private_message?
disallowed_uploads = Upload.where(id: upload_ids, secure: true).pluck(:original_filename)
end
return disallowed_uploads if disallowed_uploads.count > 0
values = upload_ids.map! { |upload_id| "(#{self.id},#{upload_id})" }.join(",")
PostUpload.transaction do
@@ -893,6 +914,12 @@ class Post < ActiveRecord::Base
end
end
def update_uploads_secure_status
if Discourse.store.external?
self.uploads.each { |upload| upload.update_secure_status }
end
end
def downloaded_images
JSON.parse(self.custom_fields[Post::DOWNLOADED_IMAGES].presence || "{}")
rescue JSON::ParserError
@@ -909,6 +936,7 @@ class Post < ActiveRecord::Base
]
fragments ||= Nokogiri::HTML::fragment(self.cooked)
links = fragments.css("a/@href", "img/@src").map do |media|
src = media.value
next if src.blank?

View File

@@ -30,9 +30,9 @@ class TopicConverter
)
update_user_stats
update_post_uploads_secure_status
Jobs.enqueue(:topic_action_converter, topic_id: @topic.id)
Jobs.enqueue(:delete_inaccessible_notifications, topic_id: @topic.id)
watch_topic(topic)
end
@topic
@@ -49,6 +49,7 @@ class TopicConverter
)
add_allowed_users
update_post_uploads_secure_status
Jobs.enqueue(:topic_action_converter, topic_id: @topic.id)
Jobs.enqueue(:delete_inaccessible_notifications, topic_id: @topic.id)
@@ -97,4 +98,11 @@ class TopicConverter
end
end
def update_post_uploads_secure_status
@topic.posts.each do |post|
next if post.uploads.empty?
post.update_uploads_secure_status
post.rebake!
end
end
end

View File

@@ -176,7 +176,7 @@ class TopicLink < ActiveRecord::Base
if upload = Upload.get_from_url(url)
internal = Discourse.store.internal?
# Store the same URL that will be used in the cooked version of the post
url = UrlHelper.cook_url(upload.url)
url = UrlHelper.cook_url(upload.url, secure: upload.secure?)
elsif route = Discourse.route_for(parsed)
internal = true

View File

@@ -140,11 +140,6 @@ class Upload < ActiveRecord::Base
!(url =~ /^(https?:)?\/\//)
end
def private?
return false if self.for_theme || self.for_site_setting
SiteSetting.prevent_anons_from_downloading_files && !FileHelper.is_supported_image?(self.original_filename)
end
def fix_dimensions!
return if !FileHelper.is_supported_image?("image.#{extension}")
@@ -235,6 +230,34 @@ class Upload < ActiveRecord::Base
self.posts.where("cooked LIKE '%/_optimized/%'").find_each(&:rebake!)
end
def update_secure_status
return false if self.for_theme || self.for_site_setting
mark_secure = should_be_secure?
self.update_column("secure", mark_secure)
Discourse.store.update_upload_ACL(self) if Discourse.store.external?
end
def should_be_secure?
mark_secure = false
if FileHelper.is_supported_media?(self.original_filename)
if SiteSetting.secure_media?
mark_secure = true if SiteSetting.login_required?
unless SiteSetting.login_required?
# first post associated with upload determines secure status
# i.e. an already public upload will stay public even if added to a new PM
first_post_with_upload = self.posts.order(sort_order: :asc).first
mark_secure = first_post_with_upload ? first_post_with_upload.with_secure_media? : false
end
else
mark_secure = false
end
else
mark_secure = SiteSetting.prevent_anons_from_downloading_files?
end
mark_secure
end
def self.migrate_to_new_scheme(limit: nil)
problems = []
@@ -385,6 +408,7 @@ end
# thumbnail_width :integer
# thumbnail_height :integer
# etag :string
# secure :boolean default(FALSE), not null
#
# Indexes
#