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

Location field in Upload utility in lib-storage is wrong #4109

Closed
3 tasks done
yoniheib opened this issue Oct 27, 2022 · 6 comments
Closed
3 tasks done

Location field in Upload utility in lib-storage is wrong #4109

yoniheib opened this issue Oct 27, 2022 · 6 comments
Assignees
Labels
bug This issue is a bug. needs-review This issue/pr needs review from an internal developer. p0 This issue is the highest priority

Comments

@yoniheib
Copy link

Checkboxes for prior research

Describe the bug

The calculated Location field (based on Bucket and endpoint) is wrong.

In this line:

: `${endpoint.protocol}//${locationBucket}.${endpoint.hostname}/${locationKey}`;

The locationBucket is appended with endpoint.hostname, but endpoint.hostname already contains the bucket.
This causes the locationBucket to appear twice and creates an invalid URI.

SDK version number

@aws-sdk/[email protected]

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

Node.JS 16

Reproduction Steps

I used the example for the lib-storage library and the Location field was wrong.

Observed Behavior

For bucket name: test-bucket-staging

Location field is:
https://test-bucket-staging.test-bucket-staging.s3.us-west-1.amazonaws.com/test-image.jpeg

Expected Behavior

For bucket name: test-bucket-staging

I would expect the field to be:
https://test-bucket-staging.s3.us-west-1.amazonaws.com/test-image.jpeg

Possible Solution

I'm assuming that only endpoint.hostname should be used, but unsure about the other possible implications of this change.

Additional Information/Context

No response

@yoniheib yoniheib added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 27, 2022
@RanVaknin
Copy link
Contributor

Hi @yoniheib ,

Can you include your code?

Thanks,
Ran~

@RanVaknin RanVaknin removed the needs-triage This issue or PR still needs to be triaged. label Oct 27, 2022
@RanVaknin RanVaknin self-assigned this Oct 27, 2022
@RanVaknin RanVaknin added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Oct 27, 2022
@yoniheib
Copy link
Author

yoniheib commented Oct 27, 2022

Sure:

import {
  S3Client, 
  type CompleteMultipartUploadCommandOutput
} from '@aws-sdk/client-s3';
import { Upload } from '@aws-sdk/lib-storage';
const s3 = new S3Client({ region: 'us-west-1' });

const S3DefaultUploadParams = {
  Bucket: process.env.AWS_S3_BUCKET as string,
  ACL: 'public-read',
};

export async function uploadFileToS3()
 {
  const params = {
    ...S3DefaultUploadParams,
    Key: 'test',
    Body: 'TEST',
  };
  const upload = new Upload({ client: s3, params});
  const result = await upload.done() as CompleteMultipartUploadCommandOutput;
  console.log(result.Location);
  
  return result.Location;
};

When I run this the Location value is:
https://<BUCKET_NAME>.<BUCKET_NAME>.s3.us-west-1.amazonaws.com/test

@RanVaknin
Copy link
Contributor

Hi @yoniheib ,

I'm able to confirm that this is incorrect behavior. Will assign it to the dev team to fix.

Thanks!
Ran~

@RanVaknin RanVaknin added needs-review This issue/pr needs review from an internal developer. p1 This is a high priority issue and removed response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. labels Oct 27, 2022
@wallind
Copy link

wallind commented Oct 28, 2022

I also just ran into the same thing on 3.198.0

I20221027-22:04:40.294(-4)? result {
I20221027-22:04:40.295(-4)?   '$metadata': {
I20221027-22:04:40.296(-4)?     httpStatusCode: 200,
I20221027-22:04:40.296(-4)?     requestId: 'CF383A1WTT6C1FKD',
I20221027-22:04:40.297(-4)?     extendedRequestId: 'bR7vd7l0VfC/vF0FN5j+xf2ZpWd2Ip8L/GClA3Ef1LiXvSI+B0r4cQ1eFU9bLHWI49/qD+PdygY=',
I20221027-22:04:40.297(-4)?     cfId: undefined,
I20221027-22:04:40.298(-4)?     attempts: 1,
I20221027-22:04:40.298(-4)?     totalRetryDelay: 0
I20221027-22:04:40.298(-4)?   },
I20221027-22:04:40.299(-4)?   ETag: '"9e69702a37e16f8af6e30332b07bd281"',
I20221027-22:04:40.299(-4)?   Bucket: 'kbi-dev-photo-uploads',
I20221027-22:04:40.300(-4)?   Key: 'uploads/profileImages/1666922680038',
I20221027-22:04:40.301(-4)?   Location: 'https://kbi-dev-photo-uploads.kbi-dev-photo-uploads.s3.us-east-2.amazonaws.com/uploads/profileImages/1666922680038'
I20221027-22:04:40.302(-4)? }

@wallind
Copy link

wallind commented Oct 28, 2022

not gonna pretend to be qualified on the full implications of the setting but forcePathStyle seems to fix the issue at least for a simple photo upload use-case.

// define a shared client for the various functions
const s3Client = new S3Client({
        ...otherConfig
        
	forcePathStyle: true,
})

result:

I20221027-22:17:20.922(-4)? result {
I20221027-22:17:20.924(-4)?   '$metadata': {
I20221027-22:17:20.925(-4)?     httpStatusCode: 200,
I20221027-22:17:20.925(-4)?     requestId: 'P2NSBZVXDT753M3H',
I20221027-22:17:20.926(-4)?     extendedRequestId: 'SvMrs1RmhlO4YcPMN5glkprdNnV+TqI1Ksp8rIqeNBknLPoiZKD81lCISzavhZtbariJ20nmRc8=',
I20221027-22:17:20.927(-4)?     cfId: undefined,
I20221027-22:17:20.927(-4)?     attempts: 1,
I20221027-22:17:20.928(-4)?     totalRetryDelay: 0
I20221027-22:17:20.929(-4)?   },
I20221027-22:17:20.929(-4)?   ETag: '"cdd1701eccfcab7ecdbb5f9b136af71f"',
I20221027-22:17:20.929(-4)?   Bucket: 'kbi-dev-photo-uploads',
I20221027-22:17:20.930(-4)?   Key: 'uploads/profileImages/1666923440713',
I20221027-22:17:20.930(-4)?   Location: 'https://s3.us-east-2.amazonaws.com/kbi-dev-photo-uploads/uploads/profileImages/1666923440713'
I20221027-22:17:20.931(-4)? }

https://s3.us-east-2.amazonaws.com/kbi-dev-photo-uploads/uploads/profileImages/1666923440713. The URL format is a bit different than I'm used to but it renders my image so yeah seems like a temporary workaround.

@RanVaknin RanVaknin added p0 This issue is the highest priority and removed p1 This is a high priority issue labels Nov 8, 2022
@kuhe kuhe self-assigned this Nov 8, 2022
@kuhe kuhe closed this as completed Nov 11, 2022
@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 Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. needs-review This issue/pr needs review from an internal developer. p0 This issue is the highest priority
Projects
None yet
Development

No branches or pull requests

4 participants