fix(cache): recognize space, tab, and = as directive terminators in hasDirective#4211
fix(cache): recognize space, tab, and = as directive terminators in hasDirective#4211Yanhu007 wants to merge 2 commits intogofiber:mainfrom
Conversation
…asDirective The hasDirective function only checked for ',' and end-of-string after a matched directive token. This caused it to miss directives followed by space, tab, or '=' (for directives with arguments): - 'no-cache ' (trailing space) → not detected - 'no-cache="Set-Cookie"' (directive with value) → not detected - 'private\t' (trailing tab) → not detected This could cause the cache middleware to serve cached responses when it shouldn't (e.g. a private response leaking into shared cache). Also accept tab as a valid separator before a directive (in addition to space and comma). Fixes gofiber#4143
Yanhu007
requested review from
ReneWerner87,
Copilot,
efectn,
gaby and
sixcolors
and removed request for
Copilot
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdated cache middleware token-matching: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the hasDirective function in the cache middleware to align with RFC 9111 §5.2. The logic now correctly handles tabs as separators and recognizes directives followed by an equals sign, supporting directives with values. A new test suite, Test_hasDirective, has been added to cover various edge cases including whitespace and case sensitivity. I have no feedback to provide.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4211 +/- ##
==========================================
+ Coverage 91.15% 91.24% +0.08%
==========================================
Files 123 123
Lines 11860 11863 +3
==========================================
+ Hits 10811 10824 +13
+ Misses 660 652 -8
+ Partials 389 387 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the cache middleware’s directive detection to comply more closely with RFC 9111 Cache-Control directive syntax, preventing cache directives from being missed when followed by whitespace or =.
Changes:
- Extend
hasDirectiveterminator handling to include space, tab, and=(in addition to comma/end-of-string). - Allow tab as a valid separator before a directive in
hasDirective. - Add unit tests covering whitespace and
=edge cases for directive matching.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
middleware/cache/cache.go |
Expands hasDirective matching rules for RFC-compliant terminators/separators. |
middleware/cache/cache_test.go |
Adds a focused test suite validating hasDirective behavior across edge cases. |
| if i > 0 { | ||
| prev := cc[i-1] | ||
| if prev != ' ' && prev != ',' { | ||
| if prev != ' ' && prev != ',' && prev != '\t' { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
hasDirective now treats '\t' as a valid separator/terminator, but parseCacheControlDirectives (used by parseRequestCacheControl/parseResponseCacheControl) still only skips/trims spaces. This means Cache-Control directives preceded/followed by tabs (valid OWS) can still be missed in the main parsing path, potentially reintroducing incorrect caching decisions. Consider extending parseCacheControlDirectives to treat '\t' like ' ' when skipping leading separators and trimming key/value.
| tests := []struct { | ||
| name string | ||
| cc string | ||
| directive string | ||
| want bool | ||
| }{ | ||
| // Basic matches |
There was a problem hiding this comment.
PR description mentions 16 hasDirective test cases, but this table currently defines 15. Either add the missing edge case (if intended) or update the PR description/test output snippet to match the actual count.
|
@Yanhu007 check the comments above and the failing spell-check workflow. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Fixes #4143
The
hasDirectivefunction in the cache middleware only recognized,and end-of-string as valid terminators after a matched directive token. Per RFC 9111 §5.2, directives can also be followed by:no-cacheorno-cache , max-age=0\t) — whitespace in headers=— directives with arguments, e.g.no-cache="Set-Cookie",max-age=3600Missing these terminators could cause the cache middleware to serve cached responses that should not be cached (e.g.
privateorno-cachedirectives being ignored).Changes
\t, and=as valid terminators after a directive match\tas a valid separator before a directive (alongside space and comma)hasDirectivecovering all edge casesTests
All 16 test cases pass: