Companion's URL controller should avoid getting the URL metadata if it already knows the file size

[james-tisato-kortical]

The /get/ endpoint of the URL controller within Companion (https://github.com/transloadit/uppy/blob/master/packages/%40uppy/companion/src/server/controllers/url.js#L53) initially makes a HEAD request for the URL (via getURLMeta) to find out the file size. It then triggers the upload using that file size as an input. This initial HEAD request appears to be unnecessary because the request sent to the /get endpoint already contains the size. The suggestion is that getURLMeta only be called if the size is not already present in the request to /get (in case there is some edge case where it's not available).

Why is this important?
We are trying to send a S3 presigned URL to Companion (via our own custom Uppy plugin). We have generated the presigned URL for the S3 object but it's only valid for GET requests. If you try to make a HEAD request with that URL, it will fail. From what I can tell in S3 documentation, there's no way around this - presigned S3 URLs for different HTTP verbs must use different presigned URLs. Therefore, this is a problem - we can only send a single URL to Companion and it will always try to perform both HEAD and GET requests with it.

Alternatively, since we are passing the file size in the request body for /get (just as the Uppy URL plugin does already), the HEAD request could be avoided and the URL could be used just for the GET operation.

[james-tisato-kortical]

I've tested a change locally and it seems to fix the issue:

diff --git a/packages/@uppy/companion/src/server/controllers/url.js b/packages/@uppy/companion/src/server/controllers/url.js
index 186610dd2..87da9e075 100644
--- a/packages/@uppy/companion/src/server/controllers/url.js
+++ b/packages/@uppy/companion/src/server/controllers/url.js
@@ -50,7 +50,7 @@ const get = (req, res) => {
     return res.status(400).json({ error: 'Invalid request body' })
   }
 
-  reqUtil.getURLMeta(req.body.url, !debug)
+  (!req.body.size ? reqUtil.getURLMeta(req.body.url, !debug) : Promise.resolve({size: req.body.size }))
     .then(({ size }) => {
       // @ts-ignore
       logger.debug('Instantiating uploader.', null, req.id)

[mifi]

I understand your problem and I created a PR #3048 with your change.
One thing I don't understand though, how can req.body.size ever be set without the client first running getURLMeta (which will also fail for signed S3 URLs)? If this workaround is only for a custom plugin, I'm not sure if this is the right way to do it, because it will not solve the root cause, and signed S3 urls using the Link plugin does still not work with this workaround. Maybe instead of using HEAD, we could do one of:

1. use GET with a Range: 0-0 (https://stackoverflow.com/questions/15717230/pre-signing-amazon-s3-urls-for-both-head-and-get-verbs) - I'm not sure if all servers handle this though.
2. a GET request that gets immediately terminated after response headers are received, without receiving any of the response body. I think all servers should be able to handle this.
3. Rewrite the Uploader so that it doesn't need to know the size in advance, but can instead figure out the size

If we do one of these instead, we could make a more generic way of fetching all kinds of links, even signed ones.

Another issue with using req.body.size is that it is a client provided value, so it could cause security issues down the road. better to use a trusted server side variable

[james-tisato-kortical]

Thanks for the update. I agree with your point that this fix doesn't work with the Link plugin. It only works for us because we are writing a custom plugin that can use separate HEAD and GET URLs if required. I was just looking for the simplest and most isolated workaround so we could carry on development.

Your other suggestions to improve this further make sense to me.

[mifi]

@arturi any preferences?

[mifi]

I tried to look into (3) Rewrite the Uploader so that it doesn't need to know the size in advance, but it turns out that the uppy client already asks for metadata before downloading, which also is the same HEAD request, so changing the uploader will not solve anything:

uppy/packages/@uppy/companion/src/server/controllers/url.js

Line 11 in 8e4fae9

.post('/meta', meta)

So the only solution that I can see that will work is (1) and (2). 2 should be the most compliant with servers so I'll do that.