fix: match weak, list and wildcard for if-none-match file backend - #1424
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good, straightforward and well-tested RFC-compliance fix.
What was reviewed: the new ifNoneMatchMatches() helper (wildcard, comma-separated list splitting, W/ weak-prefix stripping) in src/storage/backend/file.ts, its single call site replacing the old strict-equality check, and the corresponding parametrized tests in file.test.ts covering weak tag, multi-tag list, wildcard, and no-match cases. Confirmed the object's eTag is always quoted (via the private etag() method), so comparisons against header tags are apples-to-apples and commas can't appear inside an eTag value, so the naive split(',') is safe for this codebase.
Extended reasoning...
Overview
The PR modifies FileBackend's If-None-Match handling in src/storage/backend/file.ts, replacing a naive strict string-equality check with a small helper ifNoneMatchMatches() that correctly implements RFC 9110 §13.1.2 weak-comparison semantics: bare * matches anything, comma-separated tag lists are split and trimmed, and W/ weak-validator prefixes are stripped before comparing each tag to the object's eTag. The test file adds four new cases (weak tag, tag list containing a match, wildcard, and a list with no match) alongside the existing conditional-read test suite.
Security risks
None identified. This is a caching/conditional-request correctness fix, not an auth or data-exposure path — at worst a wrong 304 vs 200 status affects cache freshness, not access control. The change only affects response status code selection for GET/HEAD reads and doesn't touch write paths, path resolution, or credentials.
Level of scrutiny
Light-to-moderate scrutiny is appropriate: the change is small (about 10 lines of logic), self-contained to a single method, has a clear RFC citation backing the correctness claim, and ships with targeted unit tests exercising every new branch. I verified the object's eTag format (always wrapped in quotes by the private etag() method for both md5 and mtime algorithms) to confirm the new comparison logic is consistent with existing eTag values, and confirmed other backends (S3) don't share this code path since they delegate If-None-Match handling to the AWS SDK/S3 service directly, so this fix is correctly scoped to FileBackend only.
Other factors
No CLAUDE.md conventions apply. No bug-hunting findings were reported, and I found no additional concerns beyond what's already tested. The change is mechanical enough, and sufficiently well-tested, that I'm comfortable approving without requiring further human review.
Coverage Report for CI Build 35700558832Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.02%) to 83.352%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
583f348 to
26aab97
Compare
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
304 only when the whole header equals the strong etag, anything else is a miss.
What is the new behavior?
304 when the header matches under RFC 9110 §13.1.2 weak comparison.
Additional context
Related to #1422
https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.2