diff --git a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js index 824a9a6c5d..815c8e4355 100644 --- a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js +++ b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js @@ -133,16 +133,6 @@ export default Mixin.create({ }, }); - this._uppyInstance.use(DropTarget, { target: this.element }); - this._uppyInstance.use(UppyChecksum, { capabilities: this.capabilities }); - - // TODO (martin) Need a more automatic way to do this for preprocessor - // plugins like UppyChecksum and UppyMediaOptimization so people don't - // have to remember to do this, also want to wrap this.uppy.emit in those - // classes so people don't have to remember to pass through the plugin class - // name for the preprocess-X events. - this._trackPreProcessorStatus(UppyChecksum); - // hidden setting like enable_experimental_image_uploader if (this.siteSettings.enable_direct_s3_uploads) { this._useS3MultipartUploads(); @@ -233,6 +223,20 @@ export default Mixin.create({ }); this._setupPreprocessing(); + + // It is important that the UppyChecksum preprocessor is the last one to + // be added; the preprocessors are run in order and since other preprocessors + // may modify the file (e.g. the UppyMediaOptimization one), we need to + // checksum once we are sure the file data has "settled". + this._uppyInstance.use(UppyChecksum, { capabilities: this.capabilities }); + this._uppyInstance.use(DropTarget, { target: this.element }); + + // TODO (martin) Need a more automatic way to do this for preprocessor + // plugins like UppyChecksum and UppyMediaOptimization so people don't + // have to remember to do this, also want to wrap this.uppy.emit in those + // classes so people don't have to remember to pass through the plugin class + // name for the preprocess-X events. + this._trackPreProcessorStatus(UppyChecksum); }, _handleUploadError(file, error, response) { @@ -373,19 +377,30 @@ export default Mixin.create({ limit: 10, createMultipartUpload(file) { + const data = { + file_name: file.name, + file_size: file.size, + upload_type: file.meta.upload_type, + metadata: file.meta, + }; + + // the sha1 checksum is set by the UppyChecksum plugin, except + // for in cases where the browser does not support the required + // crypto mechanisms or an error occurs. it is an additional layer + // of security, and not required. + if (file.meta.sha1_checksum) { + data.metadata = { "sha1-checksum": file.meta.sha1_checksum }; + } + return ajax("/uploads/create-multipart.json", { type: "POST", - data: { - file_name: file.name, - file_size: file.size, - upload_type: file.meta.upload_type, - }, + data, // uppy is inconsistent, an error here fires the upload-error event - }).then((data) => { - file.meta.unique_identifier = data.unique_identifier; + }).then((responseData) => { + file.meta.unique_identifier = responseData.unique_identifier; return { - uploadId: data.external_upload_identifier, - key: data.key, + uploadId: responseData.external_upload_identifier, + key: responseData.key, }; }); }, diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 0a5c0bb056..c1c124ece5 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -224,19 +224,7 @@ class UploadsController < ApplicationController ) end - # don't want people posting arbitrary S3 metadata so we just take the - # one we need. all of these will be converted to x-amz-meta- metadata - # fields in S3 so it's best to use dashes in the names for consistency - # - # this metadata is baked into the presigned url and is not altered when - # sending the PUT from the clientside to the presigned url - metadata = if params[:metadata].present? - meta = {} - if params[:metadata]["sha1-checksum"].present? - meta["sha1-checksum"] = params[:metadata]["sha1-checksum"] - end - meta - end + metadata = parse_allowed_metadata(params[:metadata]) url = Discourse.store.signed_url_for_temporary_upload( file_name, metadata: metadata @@ -313,9 +301,11 @@ class UploadsController < ApplicationController ) end + metadata = parse_allowed_metadata(params[:metadata]) + begin multipart_upload = Discourse.store.create_multipart( - file_name, content_type + file_name, content_type, metadata: metadata ) rescue Aws::S3::Errors::ServiceError => err debug_upload_error(err, "upload.create_mutlipart_failure") @@ -579,4 +569,15 @@ class UploadsController < ApplicationController return if !SiteSetting.enable_upload_debug_mode Discourse.warn_exception(err, message: I18n.t(translation_key, translation_params)) end + + # don't want people posting arbitrary S3 metadata so we just take the + # one we need. all of these will be converted to x-amz-meta- metadata + # fields in S3 so it's best to use dashes in the names for consistency + # + # this metadata is baked into the presigned url and is not altered when + # sending the PUT from the clientside to the presigned url + def parse_allowed_metadata(metadata) + return if metadata.blank? + metadata.permit("sha1-checksum").to_h + end end diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 53c54e69c8..82fadc4bc2 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -319,13 +319,14 @@ module FileStore ) end - def create_multipart(file_name, content_type) + def create_multipart(file_name, content_type, metadata: {}) key = temporary_upload_path(file_name) response = s3_helper.s3_client.create_multipart_upload( acl: "private", bucket: s3_bucket_name, key: key, - content_type: content_type + content_type: content_type, + metadata: metadata ) { upload_id: response.upload_id, key: key } end diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index c50711a952..5463faa7df 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/spec/requests/uploads_controller_spec.rb @@ -802,8 +802,6 @@ describe UploadsController do expect(response.status).to eq(400) post "/uploads/create-multipart.json", params: { upload_type: "composer" } expect(response.status).to eq(400) - post "/uploads/create-multipart.json", params: { content_type: "image/jpeg" } - expect(response.status).to eq(400) end it "returns 422 when the create request errors" do @@ -813,7 +811,6 @@ describe UploadsController do file_name: "test.png", file_size: 1024, upload_type: "composer", - content_type: "image/png" } } expect(response.status).to eq(422) @@ -826,7 +823,6 @@ describe UploadsController do file_name: "test.zip", file_size: 9999999, upload_type: "composer", - content_type: "application/zip" } } expect(response.status).to eq(422) @@ -855,7 +851,6 @@ describe UploadsController do file_name: "test.png", file_size: 1024, upload_type: "composer", - content_type: "image/png" } } @@ -878,6 +873,27 @@ describe UploadsController do expect(result["key"]).to eq(external_upload_stub.last.key) end + it "includes accepted metadata when calling the store to create_multipart, but only allowed keys" do + stub_create_multipart_request + FileStore::S3Store.any_instance.expects(:create_multipart).with( + "test.png", "image/png", metadata: { "sha1-checksum" => "testing" } + ).returns({ key: "test" }) + + post "/uploads/create-multipart.json", { + params: { + file_name: "test.png", + file_size: 1024, + upload_type: "composer", + metadata: { + "sha1-checksum" => "testing", + "blah" => "wontbeincluded" + } + } + } + + expect(response.status).to eq(200) + end + it "rate limits" do RateLimiter.enable RateLimiter.clear_all! @@ -887,7 +903,6 @@ describe UploadsController do post "/uploads/create-multipart.json", params: { file_name: "test.png", upload_type: "composer", - content_type: "image/png", file_size: 1024 } expect(response.status).to eq(200) @@ -895,7 +910,6 @@ describe UploadsController do post "/uploads/create-multipart.json", params: { file_name: "test.png", upload_type: "composer", - content_type: "image/png", file_size: 1024 } expect(response.status).to eq(429) @@ -912,7 +926,6 @@ describe UploadsController do post "/uploads/create-multipart.json", params: { file_name: "test.png", upload_type: "composer", - content_type: "image/png", file_size: 1024 } expect(response.status).to eq(404)