Skip to content
Permalink
Browse files

FIX: Change secure media to encompass attachments as well (#9271)

If the “secure media” site setting is enabled then ALL files uploaded to Discourse (images, video, audio, pdf, txt, zip etc. etc.) will follow the secure media rules. The “prevent anons from downloading files” setting will no longer have any bearing on upload security. Basically, the feature will more appropriately be called “secure uploads” instead of “secure media”.

This is being done because there are communities out there that would like all attachments and media to be secure based on category rules but still allow anonymous users to download attachments in public places, which is not possible in the current arrangement.
  • Loading branch information
martin-brennan committed Mar 25, 2020
1 parent 4fa580f commit 097851c1352210be38d78e47eae50cd68a780770
@@ -3,7 +3,7 @@
require "mini_mime"

class UploadsController < ApplicationController
requires_login except: [:show, :show_short]
requires_login except: [:show, :show_short, :show_secure]

skip_before_action :preload_json, :check_xhr, :redirect_to_login_if_required, only: [:show, :show_short, :show_secure]
protect_from_forgery except: :show
@@ -130,6 +130,7 @@ def show_secure
upload = Upload.find_by(sha1: sha1)
return render_404 if upload.blank?

return render_404 if SiteSetting.prevent_anons_from_downloading_files && current_user.nil?
return handle_secure_upload_request(upload, path_with_ext) if SiteSetting.secure_media?

# we don't want to 404 here if secure media gets disabled
@@ -146,6 +147,8 @@ def show_secure
def handle_secure_upload_request(upload, path_with_ext = nil)
if upload.access_control_post_id.present?
raise Discourse::InvalidAccess if !guardian.can_see?(upload.access_control_post)
else
return render_404 if current_user.nil?
end

# url_for figures out the full URL, handling multisite DBs,

This file was deleted.

@@ -35,8 +35,6 @@

Jobs.enqueue(:update_s3_inventory) if [:enable_s3_inventory, :s3_upload_bucket].include?(name)

Jobs.enqueue(:update_private_uploads_acl) if name == :prevent_anons_from_downloading_files

SvgSprite.expire_cache if name.to_s.include?("_icon")

if SiteIconManager::WATCHED_SETTINGS.include?(name)
@@ -2079,7 +2079,7 @@ en:
bootstrap_mode_min_users: "Minimum number of users required to disable bootstrap mode (set to 0 to disable)"

prevent_anons_from_downloading_files: "Prevent anonymous users from downloading attachments. WARNING: this will prevent any non-image site assets posted as attachments from working."
secure_media: 'Limits access to media uploads (images, video, audio). If "login required" is enabled, only logged-in users can access media uploads. Otherwise, access will be limited only for media uploads in private messages. Note: S3 uploads must be enabled prior to enabling this setting.'
secure_media: 'Limits access to ALL uploads (images, video, audio, text, pdfs, zips, and others). If login required is enabled, only logged-in users can access uploads. Otherwise, access will be limited only for media uploads in private messages and private categories. WARNING: This setting is complex and requires deep administrative understanding. See <a target="_blank" href="https://meta.discourse.org/t/secure-media-uploads/140017">the secure media topic on Meta</a> for details.'
slug_generation_method: "Choose a slug generation method. 'encoded' will generate percent encoding string. 'none' will disable slug at all."

enable_emoji: "Enable emoji"
@@ -379,9 +379,8 @@ def create_embedded_topic
end

def update_uploads_secure_status
if SiteSetting.secure_media? || SiteSetting.prevent_anons_from_downloading_files?
@post.update_uploads_secure_status
end
return if !SiteSetting.secure_media?
@post.update_uploads_secure_status
end

def handle_spam
@@ -147,7 +147,6 @@ task 's3:correct_cachecontrol' => :environment do

base_url = Discourse.store.absolute_base_url

acl = SiteSetting.prevent_anons_from_downloading_files ? 'private' : 'public-read'
cache_control = 'max-age=31556952, public, immutable'

objects = Upload.pluck(:id, :url).map { |array| array << :upload }
@@ -165,7 +164,7 @@ task 's3:correct_cachecontrol' => :environment do
object = Discourse.store.s3_helper.object(key)
object.copy_from(
copy_source: "#{object.bucket_name}/#{object.key}",
acl: acl,
acl: "public-read",
cache_control: cache_control,
content_type: object.content_type,
content_disposition: object.content_disposition,
@@ -23,7 +23,7 @@ def initialize(upload, opts = {})
def should_be_secure?
return false if !SiteSetting.secure_media?
return false if uploading_in_public_context?
(secure_attachment? || supported_media?) && uploading_in_secure_context?
uploading_in_secure_context?
end

private
@@ -32,14 +32,6 @@ def uploading_in_public_context?
@upload.for_theme || @upload.for_site_setting || @upload.for_gravatar || public_type? || used_for_custom_emoji? || based_on_regular_emoji?
end

def supported_media?
FileHelper.is_supported_media?(@upload.original_filename)
end

def secure_attachment?
!supported_media? && SiteSetting.prevent_anons_from_downloading_files
end

def uploading_in_secure_context?
return true if SiteSetting.login_required?
if @upload.access_control_post_id.present?

This file was deleted.

@@ -178,7 +178,6 @@
before do
enable_s3_uploads
SiteSetting.secure_media = true
SiteSetting.prevent_anons_from_downloading_files = true
SiteSetting.authorized_extensions = 'pdf|svg|jpg'
end

@@ -195,15 +194,6 @@

expect(upload.secure?).to eq(false)
end

it 'should not apply prevent_anons_from_downloading_files to image uploads' do
fname = "logo.jpg"
upload = UploadCreator.new(file_from_fixtures(fname), fname).create_for(user.id)
stored_upload = Upload.last

expect(stored_upload.original_filename).to eq(fname)
expect(stored_upload.secure?).to eq(false)
end
end

context 'uploading to s3' do
@@ -228,7 +218,6 @@
end

it 'should return signed URL for secure attachments in S3' do
SiteSetting.prevent_anons_from_downloading_files = true
SiteSetting.authorized_extensions = 'pdf'
SiteSetting.secure_media = true

@@ -5,7 +5,7 @@
RSpec.describe UploadSecurity do
let(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
let(:post_in_secure_context) { Fabricate(:post, topic: Fabricate(:topic, category: private_category)) }
let(:upload) { Fabricate(:upload) }
fab!(:upload) { Fabricate(:upload) }
let(:type) { nil }
let(:opts) { { type: type } }
subject { described_class.new(upload, opts) }
@@ -144,9 +144,8 @@
end
end

context "when prevent_anons_from_downloading_files enabled for attachment" do
context "for attachments" do
before do
SiteSetting.prevent_anons_from_downloading_files = true
upload.update(original_filename: 'test.pdf')
end

@@ -169,9 +168,8 @@
expect(subject.should_be_secure?).to eq(false)
end

context "when prevent_anons_from_downloading_files enabled for attachment" do
context "for attachments" do
before do
SiteSetting.prevent_anons_from_downloading_files = true
upload.update(original_filename: 'test.pdf')
end
it "returns false" do
@@ -1415,13 +1415,14 @@ def post_with_body(body, user = nil)
)
end

it "marks image uploads as secure in PMs when secure_media is ON" do
it "marks image and attachment uploads as secure in PMs when secure_media is ON" do
SiteSetting.secure_media = true
post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user))
post.link_post_uploads
post.update_uploads_secure_status

expect(PostUpload.where(post: post).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly(
[attachment_upload.id, false],
[attachment_upload.id, true],
[image_upload.id, true]
)
end
@@ -1439,7 +1440,6 @@ def post_with_body(body, user = nil)
end

it "marks attachments as secure when relevant setting is enabled" do
SiteSetting.prevent_anons_from_downloading_files = true
SiteSetting.secure_media = true
private_category = Fabricate(:private_category, group: Fabricate(:group))
post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:topic, user: user, category: private_category))
@@ -356,8 +356,7 @@
expect(upload.secure).to eq(false)
end

it 'marks a local attachment as secure if prevent_anons_from_downloading_files is enabled' do
SiteSetting.prevent_anons_from_downloading_files = true
it 'marks a local attachment as secure if secure media enabled' do
SiteSetting.authorized_extensions = "pdf"
upload.update!(original_filename: "small.pdf", extension: "pdf", secure: false, access_control_post: Fabricate(:private_message_post))
enable_secure_media
@@ -368,8 +367,7 @@
expect(upload.secure).to eq(true)
end

it 'marks a local attachment as not secure if prevent_anons_from_downloading_files is disabled' do
SiteSetting.prevent_anons_from_downloading_files = false
it 'marks a local attachment as not secure if secure media enabled' do
SiteSetting.authorized_extensions = "pdf"
upload.update!(original_filename: "small.pdf", extension: "pdf", secure: true)

@@ -379,7 +377,7 @@
expect(upload.secure).to eq(false)
end

it 'does not change secure status of a non-attachment when prevent_anons_from_downloading_files is enabled' do
it 'does not change secure status of a non-attachment when prevent_anons_from_downloading_files is enabled by itself' do
SiteSetting.prevent_anons_from_downloading_files = true
SiteSetting.authorized_extensions = "mp4"
upload.update!(original_filename: "small.mp4", extension: "mp4")
@@ -134,7 +134,6 @@ def build_upload
SiteSetting.s3_access_key_id = "s3-access-key-id"
SiteSetting.s3_secret_access_key = "s3-secret-access-key"
SiteSetting.enable_s3_uploads = true
SiteSetting.prevent_anons_from_downloading_files = true
SiteSetting.authorized_extensions = "pdf|png|jpg|gif"
end

@@ -0,0 +1,35 @@
# frozen_string_literal: true

require 'rails_helper'

describe UploadsController do
let!(:user) { Fabricate(:user) }

describe "#show_short" do
describe "s3 store" do
let(:upload) { Fabricate(:upload_s3) }

before do
SiteSetting.enable_s3_uploads = true
SiteSetting.s3_access_key_id = "fakeid7974664"
SiteSetting.s3_secret_access_key = "fakesecretid7974664"
end

context "when upload is secure and secure media enabled" do
before do
SiteSetting.secure_media = true
upload.update(secure: true)
stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
end
context "when running on a multisite connection", type: :multisite do
it "redirects to the signed_url_for_path with the multisite DB name in the url" do
sign_in(user)
freeze_time
get upload.short_path
expect(response.body).to include(RailsMultisite::ConnectionManagement.current_db)
end
end
end
end
end
end

1 comment on commit 097851c

@discoursereviewbot

This comment has been minimized.

Copy link

@discoursereviewbot discoursereviewbot commented on 097851c Mar 26, 2020

posted:

[quote="martin, post:1, topic:10087"]
ALL
[/quote]

No need for caps here :slight_smile:

Please sign in to comment.
You can’t perform that action at this time.