janko / uppy-s3_multipart

Provides Ruby endpoints for AWS S3 multipart uploads in Uppy
https://uppy.io/docs/aws-s3/
MIT License
65 stars 21 forks source link

Missing "parts" parameter for /complete #26

Closed sliminas closed 1 year ago

sliminas commented 1 year ago

I'm in the process of switching from the presign_endpoint plugin to multipart uploads, but during the last step when calling /complete the parts parameter is missing:

response request
Screenshot_20221114_154442 Screenshot_20221114_154431

I found that the parameter is encoded in the request body but does not get parsed into the request.params object as the content type is not set to application/json which is required to add the request payload to the params (see Jeremy Evans' comment here).

Screenshot_20221114_154927

As described in the Uppy AWS S3 Multipart plugin docs I tried setting the companionHeaders like this:

.use(AwsS3Multipart, {
  companionUrl: '/', // Will call the presign endpoint on `/s3/multipart`
  companionHeaders: { 'Content-Type': 'application/json' },
})

However this header gets ignored because the OPTIONS /s3/multipart response headers contain Access-Control-Allow-Headers: *:

response Uppy logs
image image

Uppy excludes headers that are not in this list of allowed headers if it's set, see https://github.com/transloadit/uppy/blob/main/packages/@uppy/companion-client/src/RequestClient.js#L115-L126 and https://github.com/transloadit/uppy/blob/main/packages/@uppy/companion-client/src/RequestClient.js#L145-L146, so since it's set to * which is not a valid header key it excludes all the headers as in the logs screenshot above.

I could make it work by setting the Content-Type as allowed request headers in the OPTIONS endpoint here.

image

How could I solve this?

I think a way to set the response headers via a configuration option like e.g. for the create_multipart_upload endpoint would solve the issue.

I imagine other people would have encountered this already if this would be a general problem. Do I have my Rails app configured incorrectly so that it returns the * in the Access-Control-Allow-Headers header?

janko commented 1 year ago

Thanks for the detailed report. Do you know what might have changed since the initial creation of the gem? Are you using Rack 2 or 3? Is this something that stopped working in Uppy 3, or did it also not work on Uppy 2?

I don't know why Uppy Companion is intrepreting * as a header name, according to the specification, * should mean "all headers" 🤷🏻‍♂️

sliminas commented 1 year ago

I'm using uppy-s3_multipart (1.2.1), rack (2.2.4) and @uppy/aws-s3-multipart@^3.0.2. I didn't use any older version but only started implementing direct uploads within the last few weeks.

I guess then this is rather sounds like bug in Uppys handling of the Access-Control-Allow-Headers, as there is only a special case for when the header is not set at all. I'll have a look there. Maybe there is already an open issue or so. Thanks for the hints to the specification. :+1:

sliminas commented 1 year ago

I opened an issue in the Uppy project: https://github.com/transloadit/uppy/issues/4219

sliminas commented 1 year ago

The issue was fixed in https://github.com/transloadit/uppy/pull/4221 and is part of @uppy/companion-client 3.1.1.