Skip to content

S3: fix Access-Control-Allow-Headers handling in S3 CORS #12841

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

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jul 7, 2025

Motivation

As reported in #12835, we had bad handling of comma separated values in the Access-Control-Allow-Headers header when handling CORS.

This PR adds a small fix for that, along with a test

Changes

  • properly parse the Access-Control-Allow-Headers before checking if it matches the S3 CORS Rule
  • properly return the value in the same format as S3
  • add a test for this case

fixes #12835

@bentsku bentsku added this to the 4.7 milestone Jul 7, 2025
@bentsku bentsku self-assigned this Jul 7, 2025
@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Jul 7, 2025
Copy link

github-actions bot commented Jul 7, 2025

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0    2 suites  ±0   8m 54s ⏱️ +4s
  512 tests ±0  462 ✅ ±0   50 💤 ±0  0 ❌ ±0 
1 024 runs  ±0  924 ✅ ±0  100 💤 ±0  0 ❌ ±0 

Results for commit c3fef77. ± Comparison against base commit 655d4b2.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 7, 2025

Test Results - Preflight, Unit

21 854 tests  ±0   20 197 ✅ ±0   6m 19s ⏱️ -7s
     1 suites ±0    1 657 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit c3fef77. ± Comparison against base commit 655d4b2.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 7, 2025

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 8s ⏱️ +2s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit c3fef77. ± Comparison against base commit 655d4b2.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review July 7, 2025 17:38
@bentsku bentsku requested a review from k-a-il July 7, 2025 17:38
Copy link

github-actions bot commented Jul 7, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   1h 25m 54s ⏱️
2 276 tests 1 661 ✅ 615 💤 0 ❌
2 282 runs  1 661 ✅ 621 💤 0 ❌

Results for commit c3fef77.

♻️ This comment has been updated with latest results.

@bentsku bentsku force-pushed the fix-s3-cors-parsing branch from 8b77c66 to c3fef77 Compare July 8, 2025 10:46
Copy link

github-actions bot commented Jul 8, 2025

LocalStack Community integration with Pro

    2 files      2 suites   1h 4m 22s ⏱️
2 252 tests 1 634 ✅ 618 💤 0 ❌
2 254 runs  1 634 ✅ 620 💤 0 ❌

Results for commit c3fef77.

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.

LGTM 🚀

@bentsku bentsku merged commit 10e25f6 into master Jul 8, 2025
46 checks passed
@bentsku bentsku deleted the fix-s3-cors-parsing branch July 8, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: S3 - CORS header parsing fails if Access-Control-Request-Headers lacks space after comma
2 participants