Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@uppy/aws-s3-multipart: setting companionHeaders in pre-processor fails first upload #4262

Closed
2 tasks done
tpluscode opened this issue Jan 4, 2023 · 14 comments · Fixed by #4687
Closed
2 tasks done
Labels
AWS S3 Plugin that handles uploads to Amazon AWS S3 Bug

Comments

@tpluscode
Copy link

tpluscode commented Jan 4, 2023

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

I am trying to upgrade to uppy v2 to v3 and now it fails to upload to out s3 multipart endpoint

We used the following code to set auth header right before uploading

uppy.use(AwsS3Multipart, { companionUrl: uploadURL })
uppy.addPreProcessor(async () => {
  // Hack to set fresh auth token before each upload
  const plugin = uppy.getPlugin<AwsS3Multipart>('AwsS3Multipart')
  plugin?.setOptions({
    companionHeaders: prepareHeaders(uploadURL, this.$store)
  })
})

Now the header is not being sent. I found the related issue #2465 but there appears to be more to it

I see the OPTIONS preflight request and it does come back with the expected headers

Screenshot 2023-01-04 at 10 47 31

Still, Authorization is not included in the POST.

Expected behavior

Authorization header should be sent

Actual behavior

Authorization header is sent only on the second upload attempt

Thus, I compared the preflight between the:

+access-control-request-headers: authorization,content-type

No access-control-request-headers is present on the OPTIONS when upload fails. This may be a red herring but I found that setting the companion headers in the initial plugin setup mitigates the problem

uppy.use(AwsS3Multipart, {
  companionUrl: uploadURL,
+ companionHeaders: prepareHeaders(uploadURL, this.$store)
})
@tpluscode
Copy link
Author

Speaking of setting the headers. I also looked at #3770 but I did not understand it

What is the right way to achieve what I do in the addPreProcessor above?

@Murderlon
Copy link
Member

Am I understanding correctly that you don't experience problems if you also set companionHeaders initially and with your pre-processor? Or does it still occur when setting both?

@tpluscode
Copy link
Author

Sorry if I expressed myself poorly.

Yes, when set initially, the upload works fine right away

@Murderlon
Copy link
Member

Alright good. I still expected that only the pre-processor should have been enough so something might be going wrong indeed.

@Murderlon Murderlon changed the title Authorization not being sent @uppy/aws-s3-multipart: setting companionHeaders in pre-processor fails first upload Jan 4, 2023
@Murderlon Murderlon added AWS S3 Plugin that handles uploads to Amazon AWS S3 and removed Triage labels Jan 4, 2023
@aarongundel
Copy link

I am running into a similar issue where the companion headers are not being sent to my custom companion endpoints. In my case I need a CSRF header to be sent along with the request. Here's how I configure the multipart plugin:

           .use(AwsS3Multipart, {
                companionUrl: "/",
                companionHeaders: {
                    'X-CSRFToken': Cookies.get('csrftoken')
                }
            })

Unfortunately, the X-CSRFToken I provide is not included in the request headers. If I manually patch the post method of the companion client and concatenate the base headers with the companion headers, this works and the header is forwarded along with the request. That is, changing

this.preflightAndHeaders(path).then(headers => fetchWithNetworkError(_classPrivateFieldLooseBase(this, _getUrl)[_getUrl](path), {
      method,
      headers, 

to

this.preflightAndHeaders(path).then(headers => fetchWithNetworkError(_classPrivateFieldLooseBase(this, _getUrl)[_getUrl](path), {
      method,
      headers: {...headers, ...this.opts.companionHeaders},

But I am not sure if this is sufficient for all users or if I'm even using the companion headers correctly. I am happy to help on this fix, as I'm currently having to run on a fork until I can find a way to get these headers passed to my companion endpoint.

@Murderlon
Copy link
Member

Hi @aarongundel, if you would be willing to provide a PR that would be great!

Take a look at this PR, which seems very relevant to problem/fix: #3770

@jur-ng
Copy link
Contributor

jur-ng commented Sep 19, 2023

What is the status on this? The workaround mentioned here is not very reliable. The idea is to refresh the token just before uploading so we don't start the upload with an expired token. When setting the companionHeaders initially, we still occasionally run in to the issue that we are trying to prevent in the first place.

Using just the preprocessor, the first upload always fails, after which it succeeds on retry. The preprocessor executes and updates the options correctly, but in the initial createMultipartUpload the newly set companionHeaders are missing.

@jur-ng
Copy link
Contributor

jur-ng commented Sep 19, 2023

I had a theory that this could be related to the implementation of the setCompanionHeaders functionality. We have this method:

  #setCompanionHeaders = () => {
    this.#client.setCompanionHeaders(this.opts.companionHeaders)
  }

which is registered as a preprocessor:

  install () {
  this.#setResumableUploadsCapability(true)
  this.uppy.addPreProcessor(this.#setCompanionHeaders)
  this.uppy.addUploader(this.#upload)
  this.uppy.on('cancel-all', this.#resetResumableCapability)
}

According to the docs, install() is called whenever the plugin is .use'd, what we do in our preprocessor is just call setOptions, which did not call setCompanionHeaders and thus did not update the companionHeaders of the client. Then for some reason which I cannot yet explain as soon as we retry the headers do get updated.

I made a simple test case resembling the situation described in this issue which would indeed fail, and then altered the setOptions method to also call #setCompanionHeaders:

  setOptions (newOptions) {
   this.#companionCommunicationQueue.setOptions(newOptions)
   super.setOptions(newOptions)
   this.#setCompanionHeaders()
 }

The test now does succeed, making me hopeful that this could indeed work...

The only issue is that I've never worked with a monorepo like this before and I'm having a really hard time getting yarn to use my fork... I will submit an MR anyways so you can have a look and maybe test it out.

EDIT: see #4687

@shopmatechat
Copy link

I'm using the latest version of Uppy and I'm still getting the same issue.

setUppy(
        new Uppy().use(AwsS3, {
          companionUrl: API_URL + 'upload',
          shouldUseMultipart: false,
          companionHeaders: {
            Authorization: `Bearer ${token}`
          }
        })
      );

The request doesn't include the Authorization token.

@jur-ng
Copy link
Contributor

jur-ng commented Nov 27, 2023

Did you call addPreProcessor like so?

    const uppy = new Uppy({
        onBeforeFileAdded: (file) => {
            file.meta.lastModified = file.data.lastModified;
            file.meta.name = file.data.name;
            file.meta.size = file.data.size;
            file.meta.type = file.data.type;

            return file;
        },
        restrictions: {
            allowedFileTypes: ["image/*", "video/*"],
        },
    }).use(AwsS3Multipart, {
        companionUrl: `${getApiUrl()}/media/upload/`,
    });

    uppy.addPreProcessor(async () => {
        return new Promise((resolve, reject) => {
            /* Refresh current token */
            if (!TokenService.refreshToken()) reject();
            /* Use updated token for s3 multipart plugin */
            const plugin = uppy.getPlugin("AwsS3Multipart");
            plugin.setOptions({
                companionHeaders: {
                    authorization: `Bearer ${TokenService.getAccessToken()}`,
                },
            });
            resolve();
        });
    });

    return uppy;

@shopmatechat
Copy link

Just tried that without success. The request still doesn't include the header.

@jur-ng
Copy link
Contributor

jur-ng commented Nov 28, 2023

I have no idea then, we are using it without any issues:)
EDIT: from your code it seems like you are not using the aws-s3-multipart plugin, but the regular s3 one

@shopmatechat
Copy link

shopmatechat commented Nov 30, 2023

I changed to aws-s3-multipart to try your solution:

import AwsS3Multipart from '@uppy/aws-s3-multipart';

const uppy = new Uppy().use(AwsS3Multipart, {
  companionUrl: API_URL + 'upload/'
});

uppy.addPreProcessor(async () => {
  return new Promise((resolve, reject) => {
    const token = localStorage.getItem('serviceToken');
    if (!token) reject();
    const plugin = uppy.getPlugin('AwsS3Multipart');
    plugin!.setOptions({
      companionHeaders: {
        authorization: `Bearer ${token}`
      }
    });
    resolve();
  });
});

However here are the request headers:

:method:
POST
:path:
/prod/upload/s3/multipart
:scheme:
https
Accept:
application/json
Accept-Encoding:
gzip, deflate, br
Accept-Language:
en-GB,en;q=0.9,es-ES;q=0.8,es;q=0.7,en-US;q=0.6
Content-Length:
154
Content-Type:
application/json
Origin:
http://localhost:3000
Referer:
http://localhost:3000/
Sec-Ch-Ua:
"Chromium";v="116", "Not)A;Brand";v="24", "Google Chrome";v="116"
Sec-Ch-Ua-Mobile:
?0
Sec-Ch-Ua-Platform:
"Linux"
Sec-Fetch-Dest:
empty
Sec-Fetch-Mode:
cors
Sec-Fetch-Site:
cross-site
User-Agent:
Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/116.0.0.0 Safari/537.36

As you can see "Authorization" is still not being included...

Uppy versions:

    "@uppy/aws-s3": "^3.5.0",
    "@uppy/dashboard": "^3.7.1",
    "@uppy/drag-drop": "^3.0.3",
    "@uppy/file-input": "^3.0.4",
    "@uppy/progress-bar": "^3.0.4",
    "@uppy/react": "^3.2.1",

@shopmatechat
Copy link

I created a separate issue as this is not really related to addPreprocessor: #4807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWS S3 Plugin that handles uploads to Amazon AWS S3 Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants