Skip to content

fix(s3): honor If-Match and If-Unmodified-Since on GetObject - #1446

Merged
ferhatelmas merged 2 commits into
supabase:masterfrom
Shub3am:fix/s3-get-object-if-match
Oct 1, 2026
Merged

ferhatelmas merged 2 commits into
supabase:masterfrom
Shub3am:fix/s3-get-object-if-match

Conversation

@Shub3am

@Shub3am Shub3am commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

What broke

S3 GetObject ignores If-Match and If-Unmodified-Since. A read with a stale ETag or an old date gets a 200 (or a 206 for a range) instead of a 412:

GET /s3/bucket/file.txt  If-Match: "stale-etag"                               -> 200
GET /s3/bucket/file.txt  Range: bytes=0-4  If-Match: "stale-etag"             -> 206
GET /s3/bucket/file.txt  If-Unmodified-Since: Sat, 01 Jan 2000 00:00:00 GMT   -> 200

This matters in practice. Recent boto3 and AWS CLI versions (s3transfer) send If-Match on every ranged GET of a multipart download so they can tell when the object changes mid-download. Because we drop the header, an overwrite during a download silently gives you a file that is half the old object and half the new one, with no error.

Why

The route schema only accepted range, if-none-match and if-modified-since, so the other two headers never reached the handler. BrowserCacheHeaders had no field for them, and neither backend checked them.

What changed

  • get-object.ts: accept if-match and if-unmodified-since on both GetObject routes. The date goes through the same "ignore invalid dates" parser as If-Modified-Since, which I renamed to parseConditionalDate since it now handles both.
  • s3-handler.ts / adapter.ts: pass ifMatch and ifUnmodifiedSince through to the backend.
  • S3 backend: forward them as IfMatch / IfUnmodifiedSince. The upstream 412 already maps to a proper S3 error response.
  • File backend: check them before the 304 checks, the same way the copy source preconditions already do (If-Unmodified-Since is ignored when If-Match is present), and throw the same 412 PreconditionFailed.

How I tested it

  • New integration tests in s3-protocol.test.ts (If-Match mismatch on a ranged GET, If-Unmodified-Since in the past) and file backend unit tests. They fail on master and pass with the fix.
  • I also ran real requests against a local server, which now return 412 with the usual S3 error XML:
    --- GET If-Match: "stale-etag"
    <?xml version="1.0" encoding="UTF-8" standalone="yes"?><Error ...><Code>S3Error</Code><Message>At least one of the preconditions you specified did not hold.</Message></Error>
    status 412
    
  • s3-protocol.test.ts: 127/128 pass. The one failure (CopyObjectCommand > will not preserve omitted metadata when replacing it) also fails on master for me, because I ran against rustfs locally instead of minio and it defaults the content type differently.
  • Unit suite, npm run lint and tsc --noEmit are all clean.

@Shub3am
Shub3am requested a review from a team as a code owner September 26, 2026 18:49
@Shub3am
Shub3am force-pushed the fix/s3-get-object-if-match branch from 5d46400 to 68f471c Compare October 1, 2026 03:47
ShubTvaram and others added 2 commits October 1, 2026 11:30
The S3 GetObject route dropped If-Match and If-Unmodified-Since, so a
read with a stale ETag or an old date returned 200 or 206 instead of
412. Clients such as boto3 and the AWS CLI send If-Match on the ranged
GETs of a multipart download to detect the object changing mid-download,
so an overwrite during a download could silently produce a mixed file.

Accept both headers on the route, pass them through the S3 handler and
evaluate them in both backends. Invalid dates are ignored like
If-Modified-Since, and If-Unmodified-Since is ignored when If-Match is
present.
Signed-off-by: Ferhat Elmas <[email protected]>
@ferhatelmas
ferhatelmas force-pushed the fix/s3-get-object-if-match branch from 68f471c to ba0c931 Compare October 1, 2026 11:31
@ferhatelmas
ferhatelmas merged commit 76b70ba into supabase:master Oct 1, 2026
30 checks passed
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 36855870214

Coverage increased (+0.08%) to 83.903%

Details

  • Coverage increased (+0.08%) from the base build.
  • Patch coverage: 17 of 17 lines across 4 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: 14298
Covered Lines: 12422
Line Coverage: 86.88%
Relevant Branches: 8775
Covered Branches: 6937
Branch Coverage: 79.05%
Branches in Coverage %: Yes
Coverage Strength: 775.5 hits per line

💛 - Coveralls

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