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

Address existing issues involving paths in S3 and Signature usages #104

Merged
merged 6 commits into from
May 14, 2024

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented May 14, 2024

This Pull Request results from a debugging call with one of k6's users.

Problem

They were experiencing issues relating to:

  • Putting objects whose objectKey would contain space characters.
  • Signature path argument would not automatically add a leading slash if missing, leading to erroneous URLs
  • The new Endpoint type involved in the signature process is exposed by the signature.js bundle but not the overarching aws.js one.

Solution

To address these issues, this PR applies the following changes:

  • We set the S3Client signer's uriEscapePath attribute to true, to ensure paths are always double URI encoded in S3 paths.
  • The SignatureV4.sign and SignatureV4.presign operations ensure that a leading slash is appended to the target URL (if not present) when generating the resulting URL. That way we avoid potentially generating malformed URLs such as https://s3.amazonaws.commy-bucket/object-key as we've seen happen.
  • We expose the Endpoint type from the index.ts entry point, making it available in the aws.js bundle.

Next steps

In order to unblock our user, I'll publish a v0.12.1 once this is merged.

@oleiade oleiade requested a review from a team as a code owner May 14, 2024 13:43
@oleiade oleiade requested review from codebien and olegbespalov and removed request for a team May 14, 2024 13:43
@oleiade oleiade merged commit 5073f0a into main May 14, 2024
3 checks passed
@oleiade oleiade deleted the fix/issues-with-paths branch May 14, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants