Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FEATURE: Secure media allowing duplicated uploads with category-level privacy and post-based access rules (#8664) 

### General Changes and Duplication

* We now consider a post `with_secure_media?` if it is in a read-restricted category.
* When uploading we now set an upload's secure status straight away.
* When uploading if `SiteSetting.secure_media` is enabled, we do not check to see if the upload already exists using the `sha1` digest of the upload. The `sha1` column of the upload is filled with a `SecureRandom.hex(20)` value which is the same length as `Upload::SHA1_LENGTH`. The `original_sha1` column is filled with the _real_ sha1 digest of the file. 
* Whether an upload `should_be_secure?` is now determined by whether the `access_control_post` is `with_secure_media?` (if there is no access control post then we leave the secure status as is).
* When serializing the upload, we now cook the URL if the upload is secure. This is so it shows up correctly in the composer preview, because we set secure status on upload.

### Viewing Secure Media

* The secure-media-upload URL will take the post that the upload is attached to into account via `Guardian.can_see?` for access permissions
* If there is no `access_control_post` then we just deliver the media. This should be a rare occurrance and shouldn't cause issues as the `access_control_post` is set when `link_post_uploads` is called via `CookedPostProcessor`

### Removed

We no longer do any of these because we do not reuse uploads by sha1 if secure media is enabled.

* We no longer have a way to prevent cross-posting of a secure upload from a private context to a public context.
* We no longer have to set `secure: false` for uploads when uploading for a theme component.
This commit is contained in:
Martin Brennan 2020-01-16 13:50:27 +10:00 committed by GitHub
parent 5e3fc31f2c
commit 7c32411881
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
26 changed files with 518 additions and 193 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
app/assets/javascripts/discourse/lib/uploads.js.es6
View File

@ -225,7 +225,10 @@ function uploadLocation(url) {
url = Discourse.getURLWithCDN(url);
return /^\/\//.test(url) ? "http:" + url : url;
} else if (Discourse.S3BaseUrl) {
return "https:" + url;
if (url.indexOf("secure-media-uploads") === -1) {
return "https:" + url;
}
return window.location.protocol + url;
} else {
var protocol = window.location.protocol + "//",
hostname = window.location.hostname,

12
app/controllers/admin/themes_controller.rb
View File

@ -23,24 +23,12 @@ 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

12
app/controllers/uploads_controller.rb
View File

@ -102,6 +102,8 @@ class UploadsController < ApplicationController
sha1 = Upload.sha1_from_base62_encoded(params[:base62])
if upload = Upload.find_by(sha1: sha1)
return handle_secure_upload_request(upload, Discourse.store.get_path_for_upload(upload)) if upload.secure? && SiteSetting.secure_media?
if Discourse.store.internal?
send_file_local_upload(upload)
else
@ -127,7 +129,7 @@ class UploadsController < ApplicationController
return render_404 if upload.blank?
signed_secure_url = Discourse.store.signed_url_for_path(path_with_ext)
return redirect_to signed_secure_url if SiteSetting.secure_media?
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
# because all posts with secure uploads will show broken media
@ -139,6 +141,14 @@ class UploadsController < ApplicationController
redirect_to upload.secure? ? signed_secure_url : Discourse.store.cdn_url(upload.url)
end
def handle_secure_upload_request(upload, path_with_ext)
if upload.access_control_post_id.present?
raise Discourse::InvalidAccess if !guardian.can_see?(upload.access_control_post)
end
redirect_to Discourse.store.signed_url_for_path(path_with_ext)
end
def metadata
params.require(:url)
upload = Upload.get_from_url(params[:url])

1
app/jobs/scheduled/clean_up_uploads.rb
View File

@ -54,6 +54,7 @@ module Jobs
result = Upload.by_users
.where("uploads.retain_hours IS NULL OR uploads.created_at < current_timestamp - interval '1 hour' * uploads.retain_hours")
.where("uploads.created_at < ?", grace_period.hour.ago)
.where("uploads.access_control_post_id IS NULL")
.joins(<<~SQL)
LEFT JOIN site_settings ss
ON NULLIF(ss.value, '')::integer = uploads.id

2
app/models/optimized_image.rb
View File

@ -106,7 +106,7 @@ class OptimizedImage < ActiveRecord::Base
# store the optimized image and update its url
File.open(temp_path) do |file|
url = Discourse.store.store_optimized_image(file, thumbnail)
url = Discourse.store.store_optimized_image(file, thumbnail, nil, secure: upload.secure?)
if url.present?
thumbnail.url = url
thumbnail.save

24
app/models/post.rb
View File

@ -505,8 +505,9 @@ class Post < ActiveRecord::Base
end
def with_secure_media?
return false unless SiteSetting.secure_media?
topic&.private_message? || SiteSetting.login_required?
return false if !SiteSetting.secure_media?
SiteSetting.login_required? || \
(topic.present? && (topic.private_message? || topic.category&.read_restricted))
end
def hide!(post_action_type_id, reason = nil)
@ -899,20 +900,21 @@ class Post < ActiveRecord::Base
end
upload_ids |= Upload.where(id: downloaded_images.values).pluck(:id)
disallowed_uploads = []
if SiteSetting.secure_media? && !self.with_secure_media?
disallowed_uploads = Upload.where(id: upload_ids, secure: true).pluck(:original_filename)
post_uploads = upload_ids.map do |upload_id|
{ post_id: self.id, upload_id: upload_id }
end
return disallowed_uploads if disallowed_uploads.count > 0
values = upload_ids.map! { |upload_id| "(#{self.id},#{upload_id})" }.join(",")
PostUpload.transaction do
PostUpload.where(post_id: self.id).delete_all
if values.size > 0
DB.exec("INSERT INTO post_uploads (post_id, upload_id) VALUES #{values}")
if post_uploads.size > 0
PostUpload.insert_all(post_uploads)
end
if SiteSetting.secure_media?
Upload.where(id: upload_ids, access_control_post_id: nil).update_all(
access_control_post_id: self.id
)
end
end
end

77
app/models/upload.rb
View File

@ -11,6 +11,7 @@ class Upload < ActiveRecord::Base
URL_REGEX ||= /(\/original\/\dX[\/\.\w]*\/([a-zA-Z0-9]+)[\.\w]*)/
belongs_to :user
belongs_to :access_control_post, class_name: 'Post'
has_many :post_uploads, dependent: :destroy
has_many :posts, through: :post_uploads
@ -232,32 +233,12 @@ class Upload < ActiveRecord::Base
def update_secure_status(secure_override_value: nil)
return false if self.for_theme || self.for_site_setting
mark_secure = secure_override_value.nil? ? should_be_secure? : secure_override_value
mark_secure = secure_override_value.nil? ? UploadSecurity.new(self).should_be_secure? : secure_override_value
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 = []
@ -392,30 +373,38 @@ end
#
# Table name: uploads
#
# id :integer not null, primary key
# user_id :integer not null
# original_filename :string not null
# filesize :integer not null
# width :integer
# height :integer
# url :string not null
# created_at :datetime not null
# updated_at :datetime not null
# sha1 :string(40)
# origin :string(1000)
# retain_hours :integer
# extension :string(10)
# thumbnail_width :integer
# thumbnail_height :integer
# etag :string
# secure :boolean default(FALSE), not null
# id :integer not null, primary key
# user_id :integer not null
# original_filename :string not null
# filesize :integer not null
# width :integer
# height :integer
# url :string not null
# created_at :datetime not null
# updated_at :datetime not null
# sha1 :string(40)
# origin :string(1000)
# retain_hours :integer
# extension :string(10)
# thumbnail_width :integer
# thumbnail_height :integer
# etag :string
# secure :boolean default(FALSE), not null
# access_control_post_id :bigint
# original_sha1 :string
#
# Indexes
#
# index_uploads_on_etag (etag)
# index_uploads_on_extension (lower((extension)::text))
# index_uploads_on_id_and_url (id,url)
# index_uploads_on_sha1 (sha1) UNIQUE
# index_uploads_on_url (url)
# index_uploads_on_user_id (user_id)
# index_uploads_on_access_control_post_id (access_control_post_id)
# index_uploads_on_etag (etag)
# index_uploads_on_extension (lower((extension)::text))
# index_uploads_on_id_and_url (id,url)
# index_uploads_on_original_sha1 (original_sha1)
# index_uploads_on_sha1 (sha1) UNIQUE
# index_uploads_on_url (url)
# index_uploads_on_user_id (user_id)
#
# Foreign Keys
#
# fk_rails_... (access_control_post_id => posts.id)
#

5
app/serializers/upload_serializer.rb
View File

@ -13,4 +13,9 @@ class UploadSerializer < ApplicationSerializer
:short_url,
:retain_hours,
:human_filesize
def url
return object.url if !object.secure || !SiteSetting.secure_media?
UrlHelper.cook_url(object.url, secure: object.secure)
end
end

14
db/migrate/20191230055237_add_access_control_columns_to_upload.rb Normal file
View File

@ -0,0 +1,14 @@
# frozen_string_literal: true
class AddAccessControlColumnsToUpload < ActiveRecord::Migration[6.0]
def up
add_reference :uploads, :access_control_post, foreign_key: { to_table: :posts }, index: true, null: true
add_column :uploads, :original_sha1, :string, null: true
add_index :uploads, :original_sha1
end
def down
remove_column :uploads, :access_control_post_id
remove_column :uploads, :original_sha1
end
end

4
lib/file_helper.rb
View File

@ -14,11 +14,11 @@ class FileHelper
end
def self.is_supported_image?(filename)
filename =~ supported_images_regexp
(filename =~ supported_images_regexp).present?
end
def self.is_supported_media?(filename)
filename =~ supported_media_regexp
(filename =~ supported_media_regexp).present?
end
class FakeIO

2
lib/file_store/base_store.rb
View File

@ -9,7 +9,7 @@ module FileStore
store_file(file, path)
end
def store_optimized_image(file, optimized_image)
def store_optimized_image(file, optimized_image, content_type = nil, secure: false)
path = get_path_for_optimized_image(optimized_image)
store_file(file, path)
end

9
lib/file_store/s3_store.rb
View File

@ -21,7 +21,14 @@ module FileStore
def store_upload(file, upload, content_type = nil)
path = get_path_for_upload(upload)
url, upload.etag = store_file(file, path, filename: upload.original_filename, content_type: content_type, cache_locally: true, private_acl: upload.secure?)
url, upload.etag = store_file(
file,
path,
filename: upload.original_filename,
content_type: content_type,
cache_locally: true,
private_acl: upload.secure?
)
url
end

10
lib/post_creator.rb
View File

@ -177,7 +177,7 @@ class PostCreator
update_topic_auto_close
update_user_counts
create_embedded_topic
link_post_uploads
@post.link_post_uploads
update_uploads_secure_status
ensure_in_allowed_users if guardian.is_staff?
unarchive_message
@ -372,14 +372,6 @@ class PostCreator
rollback_from_errors!(embed) unless embed.save
end
def link_post_uploads
disallowed_uploads = @post.link_post_uploads
if disallowed_uploads.is_a? Array
@post.errors.add(:base, I18n.t('secure_upload_not_allowed_in_public_topic', upload_filenames: disallowed_uploads.join(", ")))
rollback_from_errors!(@post)
end
end
def update_uploads_secure_status
if SiteSetting.secure_media? || SiteSetting.prevent_anons_from_downloading_files?
@post.update_uploads_secure_status

8
lib/tasks/uploads.rake
View File

@ -72,7 +72,9 @@ task "uploads:backfill_shas" => :environment do
Upload.where(sha1: nil).find_each do |u|
begin
path = Discourse.store.path_for(u)
u.sha1 = Upload.generate_digest(path)
sha1 = Upload.generate_digest(path)
u.sha1 = u.secure? ? SecureRandom.hex(20) : sha1
u.original_sha1 = u.secure? ? sha1 : nil
u.save!
putc "."
rescue => e
@ -732,9 +734,7 @@ def update_acls_and_rebake_upload_posts(uploads_with_supported_media, mark_secur
upload_with_supported_media.posts.each { |post| post.rebake! }
if mark_secure_in_loop_because_no_login_required
first_post_with_upload = upload_with_supported_media.posts.order(sort_order: :asc).first
mark_secure = first_post_with_upload ? first_post_with_upload.with_secure_media? : false
upload_ids_to_mark_as_secure << upload_with_supported_media.id if mark_secure
upload_ids_to_mark_as_secure << UploadSecurity.new(upload_with_supported_media).should_be_secure?
end
rescue => e
uploads_skipped_because_of_error << "#{upload_with_supported_media.original_filename} (#{upload_with_supported_media.url}) #{e.message}"

44
lib/upload_creator.rb
View File

@ -63,22 +63,31 @@ class UploadCreator
image_type = @image_info.type.to_s
end
# compute the sha of the file
# compute the sha of the file and generate a unique hash
# which is only used for secure uploads
sha1 = Upload.generate_digest(@file)
unique_hash = SecureRandom.hex(20) if SiteSetting.secure_media
# do we already have that upload?
@upload = Upload.find_by(sha1: sha1)
# we do not check for duplicate uploads if secure media is
# enabled because we use a unique access hash to differentiate
# between uploads instead of the sha1, and to get around various
# access/permission issues for uploads
if !SiteSetting.secure_media
# make sure the previous upload has not failed
if @upload && @upload.url.blank?
@upload.destroy
@upload = nil
end
# do we already have that upload?
@upload = Upload.find_by(sha1: sha1)
# return the previous upload if any
if @upload
UserUpload.find_or_create_by!(user_id: user_id, upload_id: @upload.id) if user_id
return @upload
# make sure the previous upload has not failed
if @upload && @upload.url.blank?
@upload.destroy
@upload = nil
end
# return the previous upload if any
if @upload
UserUpload.find_or_create_by!(user_id: user_id, upload_id: @upload.id) if user_id
return @upload
end
end
fixed_original_filename = nil
@ -101,7 +110,8 @@ class UploadCreator
@upload.user_id = user_id
@upload.original_filename = fixed_original_filename || @filename
@upload.filesize = filesize
@upload.sha1 = sha1
@upload.sha1 = SiteSetting.secure_media? ? unique_hash : sha1
@upload.original_sha1 = SiteSetting.secure_media? ? sha1 : nil
@upload.url = ""
@upload.origin = @opts[:origin][0...1000] if @opts[:origin]
@upload.extension = image_type || File.extname(@filename)[1..10]
@ -117,13 +127,7 @@ class UploadCreator
@upload.for_export = true if @opts[:for_export]
@upload.for_site_setting = true if @opts[:for_site_setting]
@upload.for_gravatar = true if @opts[:for_gravatar]
if !FileHelper.is_supported_media?(@filename) &&
!@upload.for_theme &&
!@upload.for_site_setting &&
SiteSetting.prevent_anons_from_downloading_files
@upload.secure = true
end
@upload.secure = UploadSecurity.new(@upload, @opts).should_be_secure?
return @upload unless @upload.save

72
lib/upload_security.rb Normal file
View File

@ -0,0 +1,72 @@
# frozen_string_literal: true
##
# A note on determining whether an upload should be marked as secure:
#
# Some of these flags checked (e.g. all of the for_X flags and the opts[:type])
# are only set when _initially uploading_ via UploadCreator and are not present
# when an upload already exists.
#
# If the upload already exists the best way to figure out whether it should be
# secure alongside the site settings is the access_control_post_id, because the
# original post the upload is linked to has far more bearing on its security context
# post-upload. If the access_control_post_id does not exist then we just rely
# on the current secure? status, otherwise there would be a lot of additional
# complex queries and joins to perform.
class UploadSecurity
def initialize(upload, opts = {})
@upload = upload
@opts = opts
end
def should_be_secure?
return false if uploading_in_public_context?
secure_attachment? || secure_media?
end
private
def uploading_in_public_context?
@upload.for_theme || @upload.for_site_setting || avatar?
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 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?
return access_control_post_has_secure_media?
end
composer? || @upload.for_private_message || @upload.secure?
end
# whether the upload should remain secure or not after posting depends on its context,
# which is based on the post it is linked to via access_control_post_id.
# if that post is with_secure_media? then the upload should also be secure.
# this may change to false if the upload was set to secure on upload e.g. in
# a post composer then it turned out that the post itself was not in a secure context
#
# if there is no access control post id and the upload is currently secure, we
# do not want to make it un-secure to avoid unintentionally exposing it
def access_control_post_has_secure_media?
Post.find_by(id: @upload.access_control_post_id).with_secure_media?
end
def avatar?
@opts[:type] == "avatar"
end
def composer?
@opts[:type] == "composer"
end
end

43
spec/components/post_creator_spec.rb
View File

@ -1417,53 +1417,12 @@ describe PostCreator do
)
end
it "does not allow a secure image to be used in a public topic" do
it "links post uploads" do
public_post = PostCreator.create(
user,
topic_id: public_topic.id,
raw: "A public post with an image.\n![](#{image_upload.short_path})"
)
expect(public_post.errors.count).to be(1)
expect(public_post.errors.full_messages).to include(I18n.t('secure_upload_not_allowed_in_public_topic', upload_filenames: image_upload.original_filename))
# secure upload CAN be used in another PM
pm = PostCreator.create(
user,
title: 'this is another private message',
raw: "with an upload: \n![](#{image_upload.short_path})",
archetype: Archetype.private_message,
target_usernames: [user2.username].join(',')
)
expect(pm.errors).to be_blank
end
it "does not allow a secure video to be used in a public topic" do
video_upload = Fabricate(:upload_s3, extension: 'mp4', original_filename: "video.mp4", secure: true)
public_post = PostCreator.create(
user,
topic_id: public_topic.id,
raw: "A public post with a video onebox:\n#{video_upload.url}"
)
expect(public_post.errors.count).to be(1)
expect(public_post.errors.full_messages).to include(I18n.t('secure_upload_not_allowed_in_public_topic', upload_filenames: video_upload.original_filename))
end
it "allows an existing upload to be used again in nonPM topics in login_required sites" do
SiteSetting.login_required = true
public_post = PostCreator.create(
user,
topic_id: public_topic.id,
raw: "Reusing this image on a public topic in a login_required site:\n![](#{image_upload.short_path})"
)
expect(public_post.errors.count).to be(0)
end
end
end

6
spec/fabricators/upload_fabricator.rb
View File

@ -30,6 +30,12 @@ Fabricator(:video_upload, from: :upload) do
extension "mp4"
end
Fabricator(:secure_upload, from: :upload) do
secure { true }
sha1 { SecureRandom.hex(20) }
original_sha1 { sequence(:sha1) { |n| Digest::SHA1.hexdigest(n.to_s) } }
end
Fabricator(:upload_s3, from: :upload) do
url do |attrs|
sequence(:url) do |n|

9
spec/jobs/clean_up_uploads_spec.rb
View File

@ -298,6 +298,15 @@ describe Jobs::CleanUpUploads do
expect(Upload.exists?(id: upload2.id)).to eq(true)
end
it "does not delete uploads with an access control post ID (secure uploads)" do
upload = fabricate_upload(access_control_post_id: Fabricate(:post).id, secure: true)
Jobs::CleanUpUploads.new.execute(nil)
expect(Upload.exists?(id: expired_upload.id)).to eq(false)
expect(Upload.exists?(id: upload.id)).to eq(true)
end
it "does not delete custom emojis" do
upload = fabricate_upload
CustomEmoji.create!(name: 'test', upload: upload)

128
spec/lib/upload_creator_spec.rb
View File

@ -210,17 +210,7 @@ RSpec.describe UploadCreator do
let(:pdf_file) { file_from_fixtures(pdf_filename, "pdf") }
before do
SiteSetting.s3_upload_bucket = "s3-upload-bucket"
SiteSetting.s3_access_key_id = "s3-access-key-id"
SiteSetting.s3_secret_access_key = "s3-secret-access-key"
SiteSetting.s3_region = 'us-west-1'
SiteSetting.enable_s3_uploads = true
store = FileStore::S3Store.new
s3_helper = store.instance_variable_get(:@s3_helper)
client = Aws::S3::Client.new(stub_responses: true)
s3_helper.stubs(:s3_client).returns(client)
Discourse.stubs(:store).returns(store)
enable_s3_uploads
end
it 'should store the file and return etag' do
@ -246,6 +236,108 @@ RSpec.describe UploadCreator do
expect(signed_url).to match(/Amz-Credential/)
end
end
context "when the upload already exists based on the sha1" do
let(:filename) { "small.pdf" }
let(:file) { file_from_fixtures(filename, "pdf") }
let!(:existing_upload) { Fabricate(:upload, sha1: Upload.generate_digest(file)) }
let(:result) { UploadCreator.new(file, filename).create_for(user.id) }
it "returns the existing upload" do
expect(result).to eq(existing_upload)
end
it "does not set an original_sha1 normally" do
expect(result.original_sha1).to eq(nil)
end
it "creates a userupload record" do
result
expect(UserUpload.exists?(user_id: user.id, upload_id: existing_upload.id)).to eq(true)
end
context "when the existing upload URL is blank (it has failed)" do
before do
existing_upload.update(url: '')
end
it "destroys the existing upload" do
result
expect(Upload.find_by(id: existing_upload.id)).to eq(nil)
end
end
context "when SiteSetting.secure_media is enabled" do
before do
enable_s3_uploads
SiteSetting.secure_media = true
end
it "does not return the existing upload, as duplicate uploads are allowed" do
expect(result).not_to eq(existing_upload)
end
end
end
context "secure media functionality" do
let(:filename) { "logo.jpg" }
let(:file) { file_from_fixtures(filename) }
let(:opts) { {} }
let(:result) { UploadCreator.new(file, filename, opts).create_for(user.id) }
context "when SiteSetting.secure_media enabled" do
before do
enable_s3_uploads
SiteSetting.secure_media = true
end
it "sets an original_sha1 on the upload created because the sha1 column is securerandom in this case" do
expect(result.original_sha1).not_to eq(nil)
end
context "when uploading in a public context (theme, site setting, avatar)" do
it "does not set the upload to secure" do
upload = UploadCreator.new(file_from_fixtures(filename), filename, for_site_setting: true).create_for(user.id)
expect(upload.secure).to eq(false)
upload.destroy!
upload = UploadCreator.new(file_from_fixtures(filename), filename, for_theme: true).create_for(user.id)
expect(upload.secure).to eq(false)
upload.destroy!
upload = UploadCreator.new(file_from_fixtures(filename), filename, type: "avatar").create_for(user.id)
expect(upload.secure).to eq(false)
upload.destroy!
end
end
context "if type of upload is in the composer" do
let(:opts) { { type: "composer" } }
it "sets the upload to secure and sets the original_sha1 column, because we don't know the context of the composer" do
expect(result.secure).to eq(true)
expect(result.original_sha1).not_to eq(nil)
end
end
context "if the upload is for a PM" do
let(:opts) { { for_private_message: true } }
it "sets the upload to secure and sets the original_sha1" do
expect(result.secure).to eq(true)
expect(result.original_sha1).not_to eq(nil)
end
end
context "if SiteSetting.login_required" do
before do
SiteSetting.login_required = true
end
it "sets the upload to secure and sets the original_sha1" do
expect(result.secure).to eq(true)
expect(result.original_sha1).not_to eq(nil)
end
end
end
end
end
describe '#whitelist_svg!' do
@ -269,4 +361,18 @@ RSpec.describe UploadCreator do
end
end
end
def enable_s3_uploads
SiteSetting.s3_upload_bucket = "s3-upload-bucket"
SiteSetting.s3_access_key_id = "s3-access-key-id"
SiteSetting.s3_secret_access_key = "s3-secret-access-key"
SiteSetting.s3_region = 'us-west-1'
SiteSetting.enable_s3_uploads = true
store = FileStore::S3Store.new
s3_helper = store.instance_variable_get(:@s3_helper)
client = Aws::S3::Client.new(stub_responses: true)
s3_helper.stubs(:s3_client).returns(client)
Discourse.stubs(:store).returns(store)
end
end

2
spec/models/optimized_image_spec.rb
View File

@ -379,7 +379,7 @@ class FakeInternalStore
upload.url
end
def store_optimized_image(file, optimized_image)
def store_optimized_image(file, optimized_image, content_type = nil, secure: false)
"/internally/stored/optimized/image#{optimized_image.extension}"
end

71
spec/models/post_spec.rb
View File

@ -156,6 +156,45 @@ describe Post do
end
describe "with_secure_media?" do
let(:topic) { Fabricate(:topic) }
let!(:post) { Fabricate(:post, topic: topic) }
it "returns false if secure media is not enabled" do
expect(post.with_secure_media?).to eq(false)
end
context "when secure media is enabled" do
before { enable_secure_media_and_s3 }
context "if login_required" do
before { SiteSetting.login_required = true }
it "returns true" do
expect(post.with_secure_media?).to eq(true)
end
end
context "if the topic category is read_restricted" do
let(:category) { Fabricate(:private_category, group: Fabricate(:group)) }
before do
topic.change_category_to_id(category.id)
end
it "returns true" do
expect(post.with_secure_media?).to eq(true)
end
end
context "if the post is in a PM topic" do
let(:topic) { Fabricate(:private_message_topic) }
it "returns true" do
expect(post.with_secure_media?).to eq(true)
end
end
end
end
describe 'flagging helpers' do
fab!(:post) { Fabricate(:post) }
fab!(:user) { Fabricate(:coding_horror) }
@ -1311,6 +1350,23 @@ describe Post do
post_uploads_ids
)
end
context "when secure media is enabled" do
before { enable_secure_media_and_s3 }
it "sets the access_control_post_id on uploads in the post that don't already have the value set" do
other_post = Fabricate(:post)
video_upload.update(access_control_post_id: other_post.id)
audio_upload.update(access_control_post_id: other_post.id)
post.link_post_uploads
image_upload.reload
video_upload.reload
expect(image_upload.access_control_post_id).to eq(post.id)
expect(video_upload.access_control_post_id).not_to eq(post.id)
end
end
end
context '#update_uploads_secure_status' do
@ -1324,12 +1380,7 @@ describe Post do
end
before do
SiteSetting.authorized_extensions = "pdf|png|jpg|csv"
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 secret key"
SiteSetting.secure_media = true
enable_secure_media_and_s3
attachment_upload.update!(original_filename: "hello.csv")
stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
@ -1532,4 +1583,12 @@ describe Post do
end
end
def enable_secure_media_and_s3
SiteSetting.authorized_extensions = "pdf|png|jpg|csv"
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 secret key"
SiteSetting.secure_media = true
end
end

16
spec/models/upload_spec.rb
View File

@ -358,14 +358,24 @@ describe Upload do
)
end
it 'marks an image upload as not secure when not associated with a post' do
it 'does not mark an image upload as not secure when there is no access control post id, to avoid unintentional exposure' do
upload.update!(secure: true)
expect { upload.update_secure_status }
.to change { upload.secure }
upload.update_secure_status
expect(upload.secure).to eq(true)
end
it 'marks the upload as not secure if its access control post is a public post' do
upload.update!(secure: true, access_control_post: Fabricate(:post))
upload.update_secure_status
expect(upload.secure).to eq(false)
end
it 'leaves the upload as secure if its access control post is a PM post' do
upload.update!(secure: true, access_control_post: Fabricate(:private_message_post))
upload.update_secure_status
expect(upload.secure).to eq(true)
end
it 'marks an image upload as secure if login_required is enabled' do
SiteSetting.login_required = true
upload.update!(secure: false)

22
spec/requests/admin/themes_controller_spec.rb
View File

@ -47,25 +47,9 @@ describe Admin::ThemesController do
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
it "reuses the original upload" do
expect(response.status).to eq(201)
expect(response_json["upload_id"]).to eq(uploaded_file.id)
end
end
end

76
spec/requests/uploads_controller_spec.rb
View File

@ -377,6 +377,30 @@ describe UploadsController do
expect(response).to redirect_to(upload.url)
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
it "redirects to the signed_url_for_path" do
get upload.short_path
expect(response).to redirect_to(Discourse.store.signed_url_for_path(Discourse.store.get_path_for_upload(upload)))
end
it "raises invalid access if the user cannot access the upload access control post" do
post = Fabricate(:post)
post.topic.change_category_to_id(Fabricate(:private_category, group: Fabricate(:group)).id)
upload.update(access_control_post: post)
get upload.short_path
expect(response.code).to eq("403")
end
end
end
end
@ -394,6 +418,12 @@ describe UploadsController do
describe "s3 store" do
let(:upload) { Fabricate(:upload_s3) }
let(:secure_url) { upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads") }
def sign_in_and_stub_head
sign_in(user)
stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
end
before do
SiteSetting.enable_s3_uploads = true
@ -405,15 +435,12 @@ describe UploadsController do
end
it "should return 404 for anonymous requests requests" do
secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
get secure_url
expect(response.status).to eq(404)
end
it "should return signed url for legitimate request" do
secure_url = upload.url.sub(SiteSetting.Upload.absolute_base_url, "/secure-media-uploads")
sign_in(user)
stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
sign_in_and_stub_head
get secure_url
@ -432,6 +459,47 @@ describe UploadsController do
expect(result[0]["url"]).to match("secure-media-uploads")
end
context "when the upload cannot be found from the URL" do
it "returns a 404" do
sign_in_and_stub_head
upload.update(sha1: 'test')
get secure_url
expect(response.status).to eq(404)
end
end
context "when the access_control_post_id has been set for the upload" do
let(:post) { Fabricate(:post) }
let!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
before do
sign_in_and_stub_head
upload.update(access_control_post_id: post.id)
end
context "when the user has access to the post via guardian" do
it "should return signed url for legitimate request" do
sign_in_and_stub_head
get secure_url
expect(response.status).to eq(302)
expect(response.redirect_url).to match("Amz-Expires")
end
end
context "when the user does not have access to the post via guardian" do
before do
post.topic.change_category_to_id(private_category.id)
end
it "returns a 403" do
sign_in_and_stub_head
get secure_url
expect(response.status).to eq(403)
end
end
end
context "when secure media is disabled" do
before do
SiteSetting.secure_media = false

37
spec/serializers/upload_serializer_spec.rb
View File

@ -15,4 +15,41 @@ RSpec.describe UploadSerializer do
expect(json_data['thumbnail_width']).to eql upload.thumbnail_width
expect(json_data['thumbnail_height']).to eql upload.thumbnail_height
end
context "when the upload is secure" do
fab!(:upload) { Fabricate(:secure_upload) }
context "when secure media is disabled" do
it "just returns the normal URL, otherwise S3 errors are encountered" do
json_data = JSON.load(subject.to_json)
expect(json_data['url']).to eq(upload.url)
end
end
context "when secure media is enabled" do
before do
enable_s3_uploads
SiteSetting.secure_media = true
end
it "returns the cooked URL based on the upload URL" do
UrlHelper.expects(:cook_url).with(upload.url, secure: true)
subject.to_json
end
end
end
def enable_s3_uploads
SiteSetting.s3_upload_bucket = "s3-upload-bucket"
SiteSetting.s3_access_key_id = "s3-access-key-id"
SiteSetting.s3_secret_access_key = "s3-secret-access-key"
SiteSetting.s3_region = 'us-west-1'
SiteSetting.enable_s3_uploads = true
store = FileStore::S3Store.new
s3_helper = store.instance_variable_get(:@s3_helper)
client = Aws::S3::Client.new(stub_responses: true)
s3_helper.stubs(:s3_client).returns(client)
Discourse.stubs(:store).returns(store)
end
end