Drop If-None-Match tokens the server could never have issued - #204
Conversation
A client-chosen token with a PSR-6 reserved character ({}()/\@:) reached
the ETag pool as a cache key and Symfony threw: one request header turned
into a 500, logged as if the pool were down. No ETag setter can emit such
a token - EtagSetter/MobileEtagSetter issue crc32 decimals, DevEtagSetter
the sanitized URI tag - so it can never match, and the honest answer is
an ordinary miss.
EntityTags now drops unpoolable tokens per-token, so a list with one
valid validator still revalidates. `*` is dropped too: it is not an
opaque tag but RFC 9110 §13.1.2 existence semantics, tracked in bearsunday#203.
The observable answer for `*` is unchanged (always a full response); the
pool just is not asked.
Closes bearsunday#202
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change filters client-supplied ChangesETag token filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Invalid client validators now produce a normal full response instead of a server error, while valid validators continue to revalidate normally. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #204 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 511 513 +2
===========================================
Files 97 97
Lines 1365 1365
===========================================
Hits 1365 1365 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Closes #202 (#202).
EntityTags::of()handed every parsed opaque-tag to the ETag pool as a cache key. PSR-6 reserves{}()/\@:in keys, soIf-None-Match: "x:y"threwInvalidArgumentExceptionfrom insideisNotModified()/isNotModifiedFor(): one request header became a 500, and the scoped path logged acache_errorthat reads as a pool outage.No ETag setter can issue such a token (
EtagSetter/MobileEtagSetter: crc32 decimal;DevEtagSetter: sanitized URI tag), so it can never match.EntityTagsdrops it and the request is answered in full. Dropping is per-token:"good", "x:y"still revalidates on"good".*is dropped for the same reason on top of not being an opaque tag at all - RFC 9110 §13.1.2 gives it existence semantics, tracked separately in #203 (#203). Its observable answer is unchanged (always a full response); the pool just is not asked.Tests
EntityTagsTest: the drop contract per grammar (reserved chars,*, per-token dropping, both server-issued grammars pass).HttpCacheTest::testAClientChosenUnpoolableTokenIsAMissNotAnError: the 500 reproduction through both transfer boundaries now answers false, closes as an ordinary miss, and records nocache_error.