mirror of
https://github.com/discourse/discourse.git
synced 2026-09-05 04:40:41 -05:00
SECURITY: Regular users can route multipart uploads into the admin backup store
## Summary Applies security patch from triage. ## Security advisory https://github.com/discourse/discourse/security/advisories/GHSA-3mvf-q9rg-w6m7 ## Source - Patch Triage: https://patch.discourse.org/patch-triage/1068 - Original commit: --- 🤖 Auto-generated from the patch diff via Patch Triage. Review carefully before merging.
This commit is contained in:
committed by
Loïc Guitaut
parent
5418e3027d
commit
1f1ded8dd3
@@ -30,7 +30,7 @@ class ExternalUploadManager
|
||||
end
|
||||
|
||||
def self.create_direct_upload(current_user:, file_name:, file_size:, upload_type:, metadata: {})
|
||||
store = store_for_upload_type(upload_type)
|
||||
store = store_for_upload_type(upload_type, guardian: current_user.guardian)
|
||||
url, signed_headers = store.signed_request_for_temporary_upload(file_name, metadata: metadata)
|
||||
key = store.s3_helper.path_from_url(url)
|
||||
|
||||
@@ -59,7 +59,7 @@ class ExternalUploadManager
|
||||
metadata: {}
|
||||
)
|
||||
content_type = MiniMime.lookup_by_filename(file_name)&.content_type
|
||||
store = store_for_upload_type(upload_type)
|
||||
store = store_for_upload_type(upload_type, guardian: current_user.guardian)
|
||||
multipart_upload = store.create_multipart(file_name, content_type, metadata: metadata)
|
||||
|
||||
upload_stub =
|
||||
@@ -80,9 +80,9 @@ class ExternalUploadManager
|
||||
}
|
||||
end
|
||||
|
||||
def self.store_for_upload_type(upload_type)
|
||||
def self.store_for_upload_type(upload_type, guardian:)
|
||||
if upload_type == "backup"
|
||||
if !SiteSetting.enable_backups? ||
|
||||
if !guardian.is_admin? || !SiteSetting.enable_backups? ||
|
||||
SiteSetting.backup_location != BackupLocationSiteSetting::S3
|
||||
raise Discourse::InvalidAccess.new
|
||||
end
|
||||
@@ -95,7 +95,11 @@ class ExternalUploadManager
|
||||
def initialize(external_upload_stub, upload_create_opts = {})
|
||||
@external_upload_stub = external_upload_stub
|
||||
@upload_create_opts = upload_create_opts
|
||||
@store = ExternalUploadManager.store_for_upload_type(external_upload_stub.upload_type)
|
||||
@store =
|
||||
ExternalUploadManager.store_for_upload_type(
|
||||
external_upload_stub.upload_type,
|
||||
guardian: external_upload_stub.created_by.guardian,
|
||||
)
|
||||
end
|
||||
|
||||
def can_promote?
|
||||
|
||||
@@ -366,7 +366,7 @@ module ExternalUploadHelpers
|
||||
end
|
||||
|
||||
def multipart_store(upload_type)
|
||||
ExternalUploadManager.store_for_upload_type(upload_type)
|
||||
ExternalUploadManager.store_for_upload_type(upload_type, guardian: guardian)
|
||||
end
|
||||
|
||||
def external_store_check
|
||||
|
||||
@@ -1146,6 +1146,37 @@ RSpec.describe UploadsController do
|
||||
).to_return({ status: 200, body: create_multipart_result })
|
||||
end
|
||||
|
||||
def stub_create_multipart_backup_request
|
||||
SiteSetting.s3_backup_bucket = "s3-backup-bucket"
|
||||
SiteSetting.backup_location = BackupLocationSiteSetting::S3
|
||||
BackupRestore::S3BackupStore
|
||||
.any_instance
|
||||
.stubs(:temporary_upload_path)
|
||||
.returns(
|
||||
"temp/default/#{test_bucket_prefix}/28fccf8259bbe75b873a2bd2564b778c/test.tar.gz",
|
||||
)
|
||||
stub_request(
|
||||
:head,
|
||||
"https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/",
|
||||
).to_return(status: 200, body: "", headers: {})
|
||||
stub_request(
|
||||
:head,
|
||||
"https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/default/test.tar.gz",
|
||||
).to_return(status: 404)
|
||||
create_multipart_result = <<~XML
|
||||
<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n
|
||||
<InitiateMultipartUploadResult>
|
||||
<Bucket>s3-backup-bucket</Bucket>
|
||||
<Key>temp/default/#{test_bucket_prefix}/28fccf8259bbe75b873a2bd2564b778c/test.tar.gz</Key>
|
||||
<UploadId>#{mock_multipart_upload_id}</UploadId>
|
||||
</InitiateMultipartUploadResult>
|
||||
XML
|
||||
stub_request(
|
||||
:post,
|
||||
"https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/temp/default/#{test_bucket_prefix}/28fccf8259bbe75b873a2bd2564b778c/test.tar.gz?uploads",
|
||||
).to_return({ status: 200, body: create_multipart_result })
|
||||
end
|
||||
|
||||
it "creates a multipart upload and creates an external upload stub that is marked as multipart" do
|
||||
stub_create_multipart_request
|
||||
post "/uploads/create-multipart.json",
|
||||
@@ -1171,6 +1202,22 @@ RSpec.describe UploadsController do
|
||||
expect(result["key"]).to eq(external_upload_stub.last.key)
|
||||
end
|
||||
|
||||
it "does not allow backup multipart uploads through the public uploads endpoint" do
|
||||
stub_create_multipart_backup_request
|
||||
|
||||
expect do
|
||||
post "/uploads/create-multipart.json",
|
||||
params: {
|
||||
file_name: "test.tar.gz",
|
||||
file_size: 1024,
|
||||
upload_type: "backup",
|
||||
}
|
||||
end.not_to change { ExternalUploadStub.count }
|
||||
|
||||
expect(response.status).to eq(403)
|
||||
expect(response.body).to include(I18n.t("invalid_access"))
|
||||
end
|
||||
|
||||
it "includes accepted metadata when calling the store to create_multipart, but only allowed keys" do
|
||||
stub_create_multipart_request
|
||||
FileStore::S3Store
|
||||
|
||||
@@ -225,12 +225,16 @@ RSpec.describe ExternalUploadManager do
|
||||
end
|
||||
|
||||
context "when the upload type is backup" do
|
||||
fab!(:admin)
|
||||
|
||||
subject(:manager) { ExternalUploadManager.new(external_upload_stub, {}) }
|
||||
|
||||
let(:object_size) { 200.megabytes }
|
||||
let(:object_file) { file_from_fixtures("backup_since_v1.6.tar.gz", "backups") }
|
||||
let!(:external_upload_stub) do
|
||||
Fabricate(
|
||||
:attachment_external_upload_stub,
|
||||
created_by: user,
|
||||
created_by: admin,
|
||||
filesize: object_size,
|
||||
upload_type: "backup",
|
||||
original_filename: "backup_since_v1.6.tar.gz",
|
||||
|
||||
Reference in New Issue
Block a user