aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.43k stars 2.13k forks source link

Allow multipart upload to work with only write access #13381

Open mohammedsahl opened 5 months ago

mohammedsahl commented 5 months ago

Before opening, please confirm:

JavaScript Framework

Angular

Amplify APIs

Storage

Amplify Version

v6

Amplify Categories

storage

Backend

Amplify CLI

Environment information

``` System: OS: macOS 14.4.1 CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz Memory: 13.75 MB / 32.00 GB Shell: 5.9 - /bin/zsh Binaries: Node: 20.12.1 - ~/.nvm/versions/node/v20.12.1/bin/node Yarn: 1.22.22 - ~/.nvm/versions/node/v20.12.1/bin/yarn npm: 10.5.0 - ~/.nvm/versions/node/v20.12.1/bin/npm pnpm: 9.1.1 - ~/.nvm/versions/node/v20.12.1/bin/pnpm Browsers: Chrome: 124.0.6367.202 Safari: 17.4.1 npmPackages: graphql-ttl-transformer-v2-beta: ^2.1.1 => 2.1.1 prettier: ^3.2.5 => 3.2.5 prettier-plugin-organize-imports: ^3.2.4 => 3.2.4 npmGlobalPackages: @angular/cli: 17.3.0 @aws-amplify/cli: 12.12.0 corepack: 0.17.0 eslint: 8.57.0 http-server: 14.1.1 karma-cli: 2.0.0 npm: 10.5.0 prettier: 2.8.8 yarn: 1.22.19 ```

Describe the bug

Uploading images (or files too I'm not sure) greater than 5MB ends in a "403 Forbidden". The file uploads successfully (I can go to my bucket on S3 and download the uploaded file) but then the last HEAD method call ends in a 403. This doesn't happen with file sizes <5MB.

Expected behavior

Image upload should respond with 2xx

Reproduction steps

  1. Spin up a sample angular app
  2. Setup backend
  3. Make a request using uploadData({...}) with an image larger than 5MB
  4. See error

Code Snippet

// Put your code below this line.
return new Observable((subscriber) => {
      fetchAuthSession().then((cred) => {

        event.s3Object.identityId = cred.identityId;
        event.request = uploadData({
          path: `${keyPrefix}/${cred.identityId}/${randomFilename}`,
          data: file,
          options: {
            useAccelerateEndpoint: true,
            metadata: {
              fileName: json_encode(file.name),
              ...metadata,
            },
            contentType: file.type,
            onProgress(transferProgressEvent: TransferProgressEvent) {
              event.progressEvent = transferProgressEvent;
              subscriber.next(event);
            },
          },
        });

        event.request.result
          .then((result) => {
            event.progressEvent = null;
            event.s3Object.key = result.path;
            subscriber.next(event);
            subscriber.complete();
          })
          .catch((error: AxiosError | Error) => {
            // Log error using a logger :/ ...
            event.progressEvent = null;
            event.error = error;
            subscriber.error(event);
          });
      });
    })

Log output

This shows up in the chrome dev console ``` // Put your logs below this line zone.js:2685 HEAD https://{bucket}.s3-accelerate.amazonaws.com/uploads/us-east-1%3A4cbb3bda-c2f5-4733-8e36-c6106b3bf0ea/7c30fde3-aa1f-464f-ba03-52102c31c499.jpeg 403 (Forbidden) ```

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

Note how every request before the HEAD was successful

Screenshot 2024-05-14 at 5 42 01 PM
cwomack commented 5 months ago

Hello, @mohammedsahl and thanks for opening this issue. What's interesting here is that the multi-part upload is going to be triggered when the file size is larger than 5 MB. It's possible that the headObject call (here) may be failing due to the finalKey potentially missing the prefixes. We're going to proactively mark this as a bug and investigate further.

ashika112 commented 5 months ago

@mohammedsahl As followup can check the network tab between the two calls and see if there is a difference in requestURL between them?

mohammedsahl commented 5 months ago

Hey @ashika112, I'm not sure which two calls you're referring to. Do you mean:

1) The difference in requestURL between the 4MB upload and 6MB upload or 2) the difference in requestURL between the last two network calls of the 6MB upload (I see an OPTIONS before the failing HEAD).

If it's case 1: I see that the requestURL has the ?uploads suffix for the 6MB upload and no suffix for the 4MB one. Also I notice for the 4MB upload the first two calls are OPTIONS and PUT. For the 6MB upload the first two calls are OPTIONS and POST

If it's case 2: I see no difference between the last two network calls of the 6MB upload OPTIONS HEAD
Screenshot 2024-05-21 at 2 03 41 PM Screenshot 2024-05-21 at 2 03 46 PM
ashika112 commented 5 months ago

Hey @mohammedsahl Thanks, Sorry for not being clear. I wanted to know about difference in Request URL between the last PUT request and HEAD. I tried uploading a 10MB file and i dont see the issue happening. I tried both jpeg and video.

ashika112 commented 5 months ago
Screenshot 2024-05-21 at 11 34 00 PM
ashika112 commented 5 months ago

@mohammedsahl Which version are you using? Also, just wanted to let you know in case you didnt know this, for path construction you could alternatively use the below (this is sample from my test code)

  const res = uploadData(
    {
      path: ({ identityId }) => `protected/${identityId}/${event.target.files[0].name}`,
      data: event.target.files[0]
    })
mohammedsahl commented 5 months ago

Hey @ashika112 thanks for the tip. We already fetch the creds for other purposes so we'll go with that. But I'll keep it in mind :)

I have ~6.3.0 downloaded. But from my package-lock it seems I have 6.4.1

Screenshot 2024-05-22 at 9 27 20 AM

Also no difference between the last PUT request and HEAD besides PUT having partNumber and uploadId query params.

Screenshot 2024-05-22 at 9 25 42 AM

I guess one difference is that I'm using angular not vue but I doubt that's the root cause

ashika112 commented 5 months ago

@mohammedsahl Thanks for the version. You are right Vue vs Angular should not make a difference here.

The only other thing i could think about after looking into all screenshot is , IAM permission might be messed up here. It looks like you are using your own custom prefix, my guess would be in IAM opermission put might have been allowed but not HEAD which is need to for multipart validation. Can you check that and see how ur permission is setup?

Also, are you using Amplify CLI or Amplify Gen2 for your backend? Just wanted to double check here.

If you are interested in us taking a look at ur policy, can you redact and put ur policy created so i can help further :) Another thing that could be helpful would be knowing your custom prefix usage.

ashika112 commented 5 months ago

I am removing bug label on this since we can reproduce and 403 shouldn't happen unless there is a issue in IAM policy. If as we deep dive we find a bug i will bring it back in.

mohammedsahl commented 5 months ago

Hey @ashika112 you're right it was the IAM permissions. We solved the 403 the issue by adding "s3:GetObject" to the policy. However that's not a viable fix for us since we only want users to add objects to the bucket and not read them. We noticed "s3:GetObjectAttributes" exists too but I get a 403 forbidden with that too.

ashika112 commented 5 months ago

@mohammedsahl Good to know that helped. I see your point with the permission. Let me look into the code and put some thought into this, see if we can change something here.

cwomack commented 5 months ago

@mohammedsahl, marking this as a feature request at this point and updating title to better reflect the core issue/ask here to allow for users to add/write objects to the bucket and not read them.

hisham commented 4 months ago

Thanks @cwomack - you might want to also mark this as a "VP" (version parity) issue between v5 and v6, as this was working just fine in v5.