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

@aws-sdk/s3-presigned-post: generated policy contains duplicate fields, making it longer than necessary #5153

Closed
3 tasks done
stefansundin opened this issue Aug 30, 2023 · 2 comments
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet. response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.

Comments

@stefansundin
Copy link

stefansundin commented Aug 30, 2023

Checkboxes for prior research

Describe the bug

The examples on the NPM page (https://www.npmjs.com/package/@aws-sdk/s3-presigned-post) generates a policy that is longer than necessary.

Here's the code:

const Conditions = [{ acl: "public-read" }, { bucket: "johnsmith" }, ["starts-with", "$key", "user/eric/"]];
const client = new S3Client({region: 'us-west-2', credentials: {accessKeyId: 'test', secretAccessKey: 'test'}}); // I added credentials to this line for this example
const Bucket = "johnsmith";
const Key = "user/eric/${filename}"; // This line was modified to have ${filename} at the end
const Fields = {
  acl: "public-read",
};
const { url, fields } = await createPresignedPost(client, {
  Bucket,
  Key,
  Conditions,
  Fields,
  Expires: 600,
});
console.log(JSON.stringify(fields, undefined, 2));

The fields that are generated are:

{
  "acl": "public-read",
  "bucket": "johnsmith",
  "X-Amz-Algorithm": "AWS4-HMAC-SHA256",
  "X-Amz-Credential": "test/20230830/us-west-2/s3/aws4_request",
  "X-Amz-Date": "20230830T041048Z",
  "key": "user/eric/${filename}",
  "Policy": "eyJleHBpcmF0aW9uIjoiMjAyMy0wOC0zMFQwNDoyMDo0OFoiLCJjb25kaXRpb25zIjpbeyJhY2wiOiJwdWJsaWMtcmVhZCJ9LHsiYnVja2V0Ijoiam9obnNtaXRoIn0sWyJzdGFydHMtd2l0aCIsIiRrZXkiLCJ1c2VyL2VyaWMvIl0seyJhY2wiOiJwdWJsaWMtcmVhZCJ9LHsiYnVja2V0Ijoiam9obnNtaXRoIn0seyJYLUFtei1BbGdvcml0aG0iOiJBV1M0LUhNQUMtU0hBMjU2In0seyJYLUFtei1DcmVkZW50aWFsIjoidGVzdC8yMDIzMDgzMC91cy13ZXN0LTIvczMvYXdzNF9yZXF1ZXN0In0seyJYLUFtei1EYXRlIjoiMjAyMzA4MzBUMDQxMDQ4WiJ9LFsic3RhcnRzLXdpdGgiLCIka2V5IiwidXNlci9lcmljLyJdXX0=",
  "X-Amz-Signature": "111b06c1aefd4f46af80a20d349f4b37944fb811f24c9ea5b1e58ae4554694b1"
}

If you decode the Policy you get:

{
  "expiration": "2023-08-30T04:20:48Z",
  "conditions": [
    {
      "acl": "public-read"
    },
    {
      "bucket": "johnsmith"
    },
    [
      "starts-with",
      "$key",
      "user/eric/"
    ],
    {
      "acl": "public-read"
    },
    {
      "bucket": "johnsmith"
    },
    {
      "X-Amz-Algorithm": "AWS4-HMAC-SHA256"
    },
    {
      "X-Amz-Credential": "test/20230830/us-west-2/s3/aws4_request"
    },
    {
      "X-Amz-Date": "20230830T041048Z"
    },
    [
      "starts-with",
      "$key",
      "user/eric/"
    ]
  ]
}

It is not expected that there are duplicate conditions here (acl, bucket, and starts-with). This makes the policy longer than necessary and wastes bandwidth.

SDK version number

@aws-sdk/[email protected]

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v20.5.1

Reproduction Steps

I made a code sandbox with the code above which reproduces the bug: https://codesandbox.io/p/sandbox/aws-sdk-s3-presigned-post-bug-jd8rrj?file=/index.js

Observed Behavior

There are duplicate conditions in the policy which makes it longer than necessary. This wastes bandwidth and confuses developers.

This is happening because conditions are automatically added based on things other than the conditions you give it. The package is trying to be smart.

Expected Behavior

There should not be any duplicate fields.

Possible Solution

Add something that dedupes this conditions array:

const conditions: PolicyEntry[] = [
...Conditions,
...Object.entries(fields).map(([k, v]) => ({ [k]: v })),
Key.endsWith("${filename}")
? ["starts-with", "$key", Key.substring(0, Key.lastIndexOf("${filename}"))]
: { key: Key },
];

Additional Information/Context

No response

@stefansundin stefansundin added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 30, 2023
@RanVaknin RanVaknin self-assigned this Aug 31, 2023
@RanVaknin
Copy link
Contributor

Hi @stefansundin ,

Thanks for the report. I have pushed a PR that tries to address this.
Ran~

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. pending-release This issue will be fixed by an approved PR that hasn't been released yet. p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Sep 6, 2023
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet. response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.
Projects
None yet
Development

No branches or pull requests

2 participants