Repository navigation
fix(s3): honor If-None-Match on PutObject and CompleteMultipartUpload - #1636
Open
vignesh-chaturvedi wants to merge 1 commit into
Open
vignesh-chaturvedi wants to merge 1 commit into
vignesh-chaturvedi wants to merge 1 commit into
Conversation
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
Coverage Report for CI Build 41110765871Coverage increased (+0.003%) to 86.951%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
If-None-Match: *on S3PutObjectandCompleteMultipartUpload: the write fails with 412PreconditionFailedwhen the key already has an objectIf-None-Matchvalues with 400, since S3 accepts only*on writesProblem
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 withIf-None-Match: *, such as Terraform's S3 backend withuse_lockfile, could all take the same lock.Root cause
Neither route declared or forwarded the header, and
S3ProtocolHandler.putObjectandcompleteMultiPartUploadalways called the uploader withisUpsert: true.Solution
Both routes declare
if-none-matchwith the pattern^\*$and forward it. For*, the handler calls the uploader withisUpsert: false, the path a REST upload withoutx-upsertuses. The permission check before the upload fails fast when a live object exists.completeUploadchecks 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 asKeyAlreadyExists(409); the handler rethrows it as a newERRORS.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
s3-protocol.test.tscases:*passes validation and that"etag"and an empty value fail it.master(9d15cdc), all 4 news3-protocol.test.tscases 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 passednpm run test:integration: 1,547 passed, 1 skipped, 1 failed. The failure istesting EMPTY bucket > user is able to empty a bucket with a service key, which fails the same way on unmodifiedmaster(9d15cdc) in my local environment and passes in CInpm run lint: passednpm run build: passednpm run acceptance:typecheck: passedgit diff --check: passedIntegration ran against the repository's compose stack (Postgres 15 + RustFS).
Scope
If-Matchon writes andIf-None-MatchonCopyObjectare 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