Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Only mark attachments as secure media if SiteSetting.secure_media? #9009

Merged
merged 2 commits into from Feb 20, 2020
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -21,8 +21,9 @@ def initialize(upload, opts = {})
end

def should_be_secure?
return false if !SiteSetting.secure_media?
return false if uploading_in_public_context?
secure_attachment? || secure_media?
(secure_attachment? || supported_media?) && uploading_in_secure_context?
end

private
@@ -39,10 +40,6 @@ def secure_attachment?
!supported_media? && SiteSetting.prevent_anons_from_downloading_files
end

def secure_media?
SiteSetting.secure_media? && supported_media? && uploading_in_secure_context?
end

def uploading_in_secure_context?
return true if SiteSetting.login_required?
if @upload.access_control_post_id.present?
@@ -173,14 +173,17 @@
describe 'secure attachments' do
let(:filename) { "small.pdf" }
let(:file) { file_from_fixtures(filename, "pdf") }
let(:opts) { { type: "composer" } }

before do
enable_s3_uploads
SiteSetting.secure_media = true
SiteSetting.prevent_anons_from_downloading_files = true
SiteSetting.authorized_extensions = 'pdf|svg|jpg'
end

it 'should mark attachments as secure' do
upload = UploadCreator.new(file, filename).create_for(user.id)
upload = UploadCreator.new(file, filename, opts).create_for(user.id)
stored_upload = Upload.last

expect(stored_upload.secure?).to eq(true)
@@ -208,6 +211,7 @@
let(:file) { file_from_fixtures(filename) }
let(:pdf_filename) { "small.pdf" }
let(:pdf_file) { file_from_fixtures(pdf_filename, "pdf") }
let(:opts) { { type: "composer" } }

before do
enable_s3_uploads
@@ -226,8 +230,9 @@
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

upload = UploadCreator.new(pdf_file, pdf_filename).create_for(user.id)
upload = UploadCreator.new(pdf_file, pdf_filename, opts).create_for(user.id)
stored_upload = Upload.last
signed_url = Discourse.store.url_for(stored_upload)

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

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) }
let(:type) { nil }
let(:opts) { { type: type } }
subject { described_class.new(upload, opts) }

context "when uploading in public context" do
describe "for a public type avatar" do
let(:type) { 'avatar' }
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end
describe "for a public type custom_emoji" do
let(:type) { 'custom_emoji' }
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end
describe "for a public type profile_background" do
let(:type) { 'profile_background' }
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end
describe "for a public type avatar" do
let(:type) { 'avatar' }
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end

describe "for_theme" do
before do
upload.stubs(:for_theme).returns(true)
end
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end
describe "for_site_setting" do
before do
upload.stubs(:for_site_setting).returns(true)
end
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end
describe "for_gravatar" do
before do
upload.stubs(:for_gravatar).returns(true)
end
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end

describe "when the upload is used for a custom emoji" do
it "returns false" do
CustomEmoji.create(name: 'meme', upload: upload)
expect(subject.should_be_secure?).to eq(false)
end
end

describe "when it is based on a regular emoji" do
it "returns false" do
falafel = Emoji.all.find { |e| e.url == '/images/emoji/twitter/falafel.png?v=9' }
upload.update!(origin: "http://localhost:3000#{falafel.url}")
expect(subject.should_be_secure?).to eq(false)
end
end
end

context "when secure media is enabled" do
before do
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"
SiteSetting.secure_media = true
end

context "when login_required" do
before do
SiteSetting.login_required = true
end
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
end

context "when the access control post has_secure_media?" do
before do
upload.update(access_control_post: post_in_secure_context)
end
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
end

context "when uploading in the composer" do
let(:type) { "composer" }
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
end
context "when uploading for a group message" do
before do
upload.stubs(:for_group_message).returns(true)
end
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
end
context "when uploading for a PM" do
before do
upload.stubs(:for_private_message).returns(true)
end
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
end
context "when upload is already secure" do
before do
upload.update(secure: true)
end
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
end

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

context "when the access control post has_secure_media?" do
before do
upload.update(access_control_post: post_in_secure_context)
end
it "returns true" do
expect(subject.should_be_secure?).to eq(true)
end
end
end
end

context "when secure media is disabled" do
before do
SiteSetting.secure_media = false
end
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end

context "when prevent_anons_from_downloading_files enabled for attachment" do
before do
SiteSetting.prevent_anons_from_downloading_files = true
upload.update(original_filename: 'test.pdf')
end
it "returns false" do
expect(subject.should_be_secure?).to eq(false)
end
end
end
end
@@ -359,7 +359,8 @@
it 'marks a local attachment as secure if prevent_anons_from_downloading_files is enabled' do
SiteSetting.prevent_anons_from_downloading_files = true
SiteSetting.authorized_extensions = "pdf"
upload.update!(original_filename: "small.pdf", extension: "pdf")
upload.update!(original_filename: "small.pdf", extension: "pdf", secure: false, access_control_post: Fabricate(:private_message_post))
enable_secure_media

expect { upload.update_secure_status }
.to change { upload.secure }
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.