github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2024-11-22 09:42:02 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Martin Brennan 2020-03-26 07:16:02 +10:00 committed by GitHub
parent 4fa580fbd1
commit 097851c135
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 106 additions and 127 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
app/controllers/uploads_controller.rb
View File

@ -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 @@ class UploadsController < ApplicationController
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 @@ class UploadsController < ApplicationController
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,

17
app/jobs/regular/update_private_uploads_acl.rb
View File

@ -1,17 +0,0 @@
# frozen_string_literal: true
module Jobs
class UpdatePrivateUploadsAcl < ::Jobs::Base
# only runs when SiteSetting.prevent_anons_from_downloading_files is updated
def execute(args)
return if !SiteSetting.Upload.enable_s3_uploads
Upload.find_each do |upload|
if !FileHelper.is_supported_media?(upload.original_filename)
upload.update_secure_status
end
end
end
end
end

2
config/initializers/014-track-setting-changes.rb
View File

@ -35,8 +35,6 @@ DiscourseEvent.on(:site_setting_changed) do |name, old_value, new_value|
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)

2
config/locales/server.en.yml
View File

@ -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"

5
lib/post_creator.rb
View File

@ -379,9 +379,8 @@ class PostCreator
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

3
lib/tasks/s3.rake
View File

@ -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,

10
lib/upload_security.rb
View File

@ -23,7 +23,7 @@ class UploadSecurity
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 @@ class UploadSecurity
@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?

54
spec/jobs/regular/update_private_uploads_acl_spec.rb
View File

@ -1,54 +0,0 @@
# frozen_string_literal: true
require 'rails_helper'
describe Jobs::UpdatePrivateUploadsAcl do
let(:args) { [] }
before do
SiteSetting.authorized_extensions = "pdf"
end
describe '#execute' do
context "if not SiteSetting.Upload.enable_s3_uploads" do
before do
SiteSetting.Upload.stubs(:enable_s3_uploads).returns(false)
end
it "returns early and changes no uploads" do
Upload.expects(:find_each).never
subject.execute(args)
end
end
context "if SiteSetting.Upload.enable_s3_uploads" do
let!(:upload) { Fabricate(:upload_s3, extension: 'pdf', original_filename: "watchmen.pdf", secure: false) }
before do
SiteSetting.login_required = true
SiteSetting.prevent_anons_from_downloading_files = true
Discourse.stubs(:store).returns(stub(external?: false))
enable_s3_uploads([upload])
SiteSetting.secure_media = true
end
it "changes the upload to secure" do
subject.execute(args)
expect(upload.reload.secure).to eq(true)
end
end
end
def enable_s3_uploads(uploads)
SiteSetting.enable_s3_uploads = true
SiteSetting.s3_upload_bucket = "s3-upload-bucket"
SiteSetting.s3_access_key_id = "some key"
SiteSetting.s3_secret_access_key = "some secrets3_region key"
stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
uploads.each do |upload|
stub_request(
:put,
"https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/original/1X/#{upload.sha1}.#{upload.extension}?acl"
)
end
end
end

11
spec/lib/upload_creator_spec.rb
View File

@ -178,7 +178,6 @@ RSpec.describe UploadCreator do
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 @@ RSpec.describe UploadCreator do
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 @@ RSpec.describe UploadCreator do
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

8
spec/lib/upload_security_spec.rb
View File

@ -5,7 +5,7 @@ require 'rails_helper'
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 @@ RSpec.describe UploadSecurity do
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 @@ RSpec.describe UploadSecurity do
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

6
spec/models/post_spec.rb
View File

@ -1415,13 +1415,14 @@ describe Post do
)
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 @@ describe Post do
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))

8
spec/models/upload_spec.rb
View File

@ -356,8 +356,7 @@ describe Upload do
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 @@ describe Upload do
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 @@ describe Upload do
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")

1
spec/multisite/s3_store_spec.rb
View File

@ -134,7 +134,6 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
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

35
spec/requests/uploads_controller_multisite_spec.rb Normal file
View File

@ -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

57
spec/requests/uploads_controller_spec.rb
View File

@ -352,7 +352,7 @@ describe UploadsController do
end
it "returns the right response when anon tries to download a file " \
"when prevent_anons_from_downloading_files is true" do
"when prevent_anons_from_downloading_files is true" do
delete "/session/#{user.username}.json"
SiteSetting.prevent_anons_from_downloading_files = true
@ -386,6 +386,7 @@ describe UploadsController do
end
it "redirects to the signed_url_for_path" do
sign_in(user)
freeze_time
get upload.short_path
@ -393,6 +394,7 @@ describe UploadsController do
end
it "raises invalid access if the user cannot access the upload access control post" do
sign_in(user)
post = Fabricate(:post)
post.topic.change_category_to_id(Fabricate(:private_category, group: Fabricate(:group)).id)
upload.update(access_control_post: post)
@ -400,14 +402,6 @@ describe UploadsController do
get upload.short_path
expect(response.code).to eq("403")
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
freeze_time
get upload.short_path
expect(response.body).to include(RailsMultisite::ConnectionManagement.current_db)
end
end
end
end
end
@ -430,10 +424,15 @@ describe UploadsController do
def sign_in_and_stub_head
sign_in(user)
stub_head
end
def stub_head
stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
end
before do
SiteSetting.authorized_extensions = "*"
SiteSetting.enable_s3_uploads = true
SiteSetting.s3_upload_bucket = "s3-upload-bucket"
SiteSetting.s3_access_key_id = "fakeid7974664"
@ -508,6 +507,46 @@ describe UploadsController do
end
end
context "when the upload is an attachment file" do
before do
upload.update(original_filename: 'test.pdf')
end
it "redirects to the signed_url_for_path" do
sign_in_and_stub_head
get secure_url
expect(response.status).to eq(302)
expect(response.redirect_url).to match("Amz-Expires")
end
context "when the user does not have access to the access control post via guardian" do
let(:post) { Fabricate(:post) }
let!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
before do
post.topic.change_category_to_id(private_category.id)
upload.update(access_control_post_id: post.id)
end
it "returns a 403" do
sign_in_and_stub_head
get secure_url
expect(response.status).to eq(403)
end
end
context "when the prevent_anons_from_downloading_files setting is enabled and the user is anon" do
before do
SiteSetting.prevent_anons_from_downloading_files = true
end
it "returns a 404" do
stub_head
delete "/session/#{user.username}.json"
get secure_url
expect(response.status).to eq(404)
end
end
end
context "when secure media is disabled" do
before do
SiteSetting.secure_media = false

9
spec/tasks/uploads_spec.rb
View File

@ -14,13 +14,14 @@ RSpec.describe "tasks/uploads" do
[
multi_post_upload1,
upload1,
upload2
upload2,
upload3
]
end
let(:multi_post_upload1) { Fabricate(:upload_s3) }
let(:upload1) { Fabricate(:upload_s3) }
let(:upload2) { Fabricate(:upload_s3) }
let(:upload3) { Fabricate(:upload_s3, original_filename: 'test.pdf') }
let(:upload3) { Fabricate(:upload_s3, original_filename: 'test.pdf', extension: 'pdf') }
let!(:post1) { Fabricate(:post) }
let!(:post2) { Fabricate(:post) }
@ -65,12 +66,12 @@ RSpec.describe "tasks/uploads" do
expect(upload3.reload.access_control_post).to eq(post3)
end
it "sets the upload in the read restricted topic category to secure" do
it "sets the uploads that are media and attachments in the read restricted topic category to secure" do
post3.topic.update(category: Fabricate(:private_category, group: Fabricate(:group)))
invoke_task
expect(upload2.reload.secure).to eq(true)
expect(upload1.reload.secure).to eq(false)
expect(upload3.reload.secure).to eq(false)
expect(upload3.reload.secure).to eq(true)
end
it "sets the upload in the PM topic to secure" do