Skip to content

fix(S3): enforce presigned POST policy conditions#1211

Open
TylerHillery wants to merge 2 commits into
masterfrom
tyler/fix/presigned-s3-post-forms
Open

fix(S3): enforce presigned POST policy conditions#1211
TylerHillery wants to merge 2 commits into
masterfrom
tyler/fix/presigned-s3-post-forms

Conversation

@TylerHillery

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Bug fix to enforce policy conditions in S3 presigned POST url.

What is the current behavior?

Currently only verifies the HMAC over the Policy blob

What is the new behavior?

Properly validates the conditions

Additional context

@TylerHillery TylerHillery marked this pull request as ready for review July 7, 2026 16:00
@TylerHillery TylerHillery requested a review from a team as a code owner July 7, 2026 16:00
Copilot AI review requested due to automatic review settings July 7, 2026 16:00
Comment thread src/storage/protocols/s3/signature-v4.ts Outdated
Comment thread src/storage/protocols/s3/signature-v4.ts Outdated
Comment thread src/storage/protocols/s3/signature-v4.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens S3 presigned POST handling by validating POST policy conditions (not just the policy HMAC), preventing policy replay/misuse and enforcing constraint-driven uploads.

Changes:

  • Adds POST policy condition validation (e.g., eq, starts-with, content-length-range) and rejection of unconstrained submitted fields.
  • Propagates content-length-range max into the upload pipeline to enforce an effective maximum upload size.
  • Expands S3 protocol tests to cover key/bucket replay and policy condition enforcement scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
src/test/s3-protocol.test.ts Adds multipart/form-data POST policy enforcement tests (bucket/key replay, expiration, field coverage, content-length-range).
src/storage/protocols/s3/signature-v4.ts Implements policy condition parsing/normalization and validation, plus tracks submitted POST fields for coverage checks.
src/http/routes/s3/commands/put-object.ts Applies policy-derived content-length-range max by tightening the stream byte limit.
src/http/plugins/signature-v4.ts Runs POST policy condition validation after signature verification and stores derived max size on the request.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/storage/protocols/s3/signature-v4.ts Outdated
Comment thread src/storage/protocols/s3/signature-v4.ts Outdated
Comment thread src/storage/protocols/s3/signature-v4.ts Outdated
Comment thread src/storage/protocols/s3/signature-v4.ts Outdated
Comment thread src/test/s3-protocol.test.ts Outdated
Comment thread src/storage/protocols/s3/signature-v4.ts Outdated
Comment thread src/http/plugins/signature-v4.ts Outdated
@coveralls

coveralls commented Jul 7, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 28885889609

Coverage increased (+0.1%) to 79.199%

Details

  • Coverage increased (+0.1%) from the base build.
  • Patch coverage: 10 uncovered changes across 2 files (51 of 61 lines covered, 83.61%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
src/storage/protocols/s3/policy.ts 50 42 84.0%
src/storage/protocols/s3/signature-v4.ts 9 7 77.78%
Total (3 files) 61 51 83.61%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 12423
Covered Lines: 10287
Line Coverage: 82.81%
Relevant Branches: 7201
Covered Branches: 5255
Branch Coverage: 72.98%
Branches in Coverage %: Yes
Coverage Strength: 418.01 hits per line

💛 - Coveralls

@TylerHillery TylerHillery force-pushed the tyler/fix/presigned-s3-post-forms branch from c96f999 to fa82c4d Compare July 7, 2026 17:30
@TylerHillery TylerHillery requested a review from fenos July 7, 2026 21:19
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.

4 participants