Skip to content

S3: fix casing of PreSignedPost validation #12449

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 1, 2025
Merged

S3: fix casing of PreSignedPost validation #12449

merged 1 commit into from
Apr 1, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 28, 2025 •
edited
Loading

Motivation

As reported by #12448, we had an issue with the JS AWS SDK v3 accepting some conditions that shouldn't have been accepted. This is because we skipped validating the request if the request did not have a policy field, lower-cased. But the JS SDK sends it as Policy.

The Gist sent by the user now works with the fix, and an AWS validated fix asserts that this is the right behavior.

Changes

  • update the logic to be case insensitive while validating certain fields of the Post request

fixes #12448

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Mar 28, 2025
@bentsku bentsku added this to the 4.4 milestone Mar 28, 2025
@bentsku bentsku self-assigned this Mar 28, 2025
@bentsku bentsku requested a review from k-a-il March 28, 2025 19:18
@github-actions GitHub Actions
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   8m 37s ⏱️
488 tests 438 ✅  50 💤 0 ❌
976 runs  876 ✅ 100 💤 0 ❌

Results for commit 7a1ed5f.

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 0m 10s ⏱️ - 51m 17s
1 743 tests  - 2 563  1 581 ✅  - 2 405  162 💤  - 158  0 ❌ ±0 
1 745 runs   - 2 563  1 581 ✅  - 2 405  164 💤  - 158  0 ❌ ±0 

Results for commit 7a1ed5f. ± Comparison against base commit ccddefd.

This pull request removes 2565 and adds 2 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.s3.test_s3.TestS3PresignedPost ‑ test_post_object_policy_casing[s3]
tests.aws.services.s3.test_s3.TestS3PresignedPost ‑ test_post_object_policy_casing[s3v4]

k-a-il
k-a-il approved these changes Apr 1, 2025
View reviewed changes
Copy link
Contributor

@k-a-il k-a-il left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix :) all keys used to query the dictionary were lower cased

@bentsku bentsku merged commit a6be285 into master Apr 1, 2025
47 checks passed
@bentsku bentsku deleted the fix-s3-presigned-post-conditions branch April 1, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k-a-il k-a-il k-a-il approved these changes

Assignees

@bentsku bentsku

Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

bug: content-length-range not respected
2 participants
@bentsku @k-a-il