Skip to content

fix(s3): honor If-None-Match on PutObject and CompleteMultipartUpload - #1636

Open
vignesh-chaturvedi wants to merge 1 commit into
supabase:masterfrom
vignesh-chaturvedi:fix/s3-put-object-if-none-match
Open

vignesh-chaturvedi wants to merge 1 commit into
supabase:masterfrom
vignesh-chaturvedi:fix/s3-put-object-if-none-match

Conversation

@vignesh-chaturvedi

@vignesh-chaturvedi vignesh-chaturvedi commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • honor If-None-Match: * on S3 PutObject and CompleteMultipartUpload: the write fails with 412 PreconditionFailed when the key already has an object
  • treat a key whose current version is a delete marker as having no object, as S3 does in versioned buckets
  • reject other If-None-Match values with 400, since S3 accepts only * on writes

Problem

Both operations ignored If-None-Match. A create-only write over an existing key returned 200 and replaced the object, and concurrent create-only writes to a new key all succeeded. Clients that create a lock file with If-None-Match: *, such as Terraform's S3 backend with use_lockfile, could all take the same lock.

Root cause

Neither route declared or forwarded the header, and S3ProtocolHandler.putObject and completeMultiPartUpload always called the uploader with isUpsert: true.

Solution

Both routes declare if-none-match with the pattern ^\*$ and forward it. For *, the handler calls the uploader with isUpsert: false, the path a REST upload without x-upsert uses. The permission check before the upload fails fast when a live object exists. completeUpload checks again while holding the object lock and the row lock, so only one of several concurrent writes can create the object, and the uploader's existing cleanup queues the bytes of the rejected ones for deletion. A current delete marker counts as no object. The uploader reports an existing key as KeyAlreadyExists (409); the handler rethrows it as a new ERRORS.PreconditionFailed (412), the status S3 returns.

For multipart uploads the first check runs before the parts are assembled, so a completion rejected by that check leaves the upload open: the client can abort it, or complete it without the condition. Permission checks are the same as for a REST upload without x-upsert.

Tests

  • New s3-protocol.test.ts cases:
    • a create-only write to a new key, then to the same key
    • five concurrent create-only writes to one new key: exactly one gets 200, and the stored body is the winner's
    • a create-only write over a delete marker in a versioned bucket
    • a multipart completion that gets 412 and can still complete without the condition
  • New route cases check that * passes validation and that "etag" and an empty value fail it.
  • Regression first: with only the source changes reverted on current master (9d15cdc), all 4 new s3-protocol.test.ts cases fail, and so do the 4 route cases that expect "etag" and an empty value to fail validation.
  • npm run test:unit: 182 files, 3,428 passed
  • npm run test:integration: 1,547 passed, 1 skipped, 1 failed. The failure is testing EMPTY bucket > user is able to empty a bucket with a service key, which fails the same way on unmodified master (9d15cdc) in my local environment and passes in CI
  • npm run lint: passed
  • npm run build: passed
  • npm run acceptance:typecheck: passed
  • git diff --check: passed

Integration ran against the repository's compose stack (Postgres 15 + RustFS).

Scope

If-Match on writes and If-None-Match on CopyObject are still ignored. Iceberg bucket uploads share these route schemas, so they also reject values other than *, but they write straight to the backend and still skip the check.

Closes #1635

S3 PutObject and CompleteMultipartUpload ignored If-None-Match, so a
create-only write replaced an existing object and concurrent create-only
writes all succeeded. Use the uploader's non-upsert path for
If-None-Match: *, report an existing object as 412 PreconditionFailed,
and reject other If-None-Match values with 400.

Closes supabase#1635
@vignesh-chaturvedi
vignesh-chaturvedi requested a review from a team as a code owner October 30, 2026 18:53
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 41110765871

Coverage increased (+0.003%) to 86.951%

Details

  • Coverage increased (+0.003%) from the base build.
  • Patch coverage: 7 of 7 lines across 2 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 15596
Covered Lines: 13999
Line Coverage: 89.76%
Relevant Branches: 9954
Covered Branches: 8217
Branch Coverage: 82.55%
Branches in Coverage %: Yes
Coverage Strength: 1228.32 hits per line

💛 - Coveralls

This branch has not been deployed

No deployments
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.

S3 PutObject and CompleteMultipartUpload ignore If-None-Match

2 participants