🐛 fix(cache): detect directives with OWS and value params#4212
🐛 fix(cache): detect directives with OWS and value params#4212nickzerjeski wants to merge 1 commit intogofiber:mainfrom
Conversation
nickzerjeski
requested review from
ReneWerner87,
Copilot,
efectn,
gaby and
sixcolors
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughFixes Cache-Control directive detection in cache middleware by parsing comma-separated directives, trimming spaces/tabs, handling Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 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 refactors the hasDirective function in the cache middleware to use the parseCacheControlDirectives parser and adds unit tests. The review recommends using utils.UnsafeBytes to eliminate unnecessary allocations and notes that the parser needs to be updated to support horizontal tabs to comply with RFC 9111.
middleware/cache/cache.go
Outdated
| if i+dirLen == ccLen || cc[i+dirLen] == ',' { | ||
| return true | ||
| found := false | ||
| parseCacheControlDirectives([]byte(cc), func(key, _ []byte) { |
There was a problem hiding this comment.
The conversion []byte(cc) creates a copy of the string, leading to an unnecessary allocation. Since parseCacheControlDirectives only performs read operations on the slice, you can use utils.UnsafeBytes(cc) for a zero-allocation conversion. This is more efficient and consistent with other parts of the middleware (e.g., line 1129).
| parseCacheControlDirectives([]byte(cc), func(key, _ []byte) { | |
| parseCacheControlDirectives(utils.UnsafeBytes(cc), func(key, _ []byte) { |
| {header: "no-cache", directive: "no-cache", expect: true}, | ||
| {header: "NO-CACHE", directive: "no-cache", expect: true}, | ||
| {header: "no-cache ", directive: "no-cache", expect: true}, | ||
| {header: "no-cache\t", directive: "no-cache", expect: true}, |
There was a problem hiding this comment.
This test case for a trailing horizontal tab (\t) might fail with the current implementation of parseCacheControlDirectives. The parser (lines 952, 957, and 970 in cache.go) only trims the space character (' ') and does not account for tabs. Per RFC 9111, Optional White Space (OWS) consists of both spaces and horizontal tabs. To fully support OWS as intended by this PR, the underlying parser should be updated to handle both characters.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/cache/cache.go`:
- Around line 911-913: hasDirective fails for HTAB-terminated tokens because
parseCacheControlDirectives only trims space characters; update the parser used
by hasDirective (parseCacheControlDirectives) to trim horizontal tab characters
as well (include '\t' in the trimming/whitespace set) so that values like
"no-cache\t" match directive "no-cache"; locate parseCacheControlDirectives and
modify its trimming logic (or the utilities it calls) to treat '\t' the same as
' ' so hasDirective correctly sees HTAB-terminated directives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 033e35ad-8a75-4553-b618-d3dcf60312ee
📒 Files selected for processing (2)
middleware/cache/cache.gomiddleware/cache/cache_test.go
nickzerjeski
force-pushed
the
fix-cache-hasdirective-4143
branch
from
5c17b55 to
01b6574
Compare
There was a problem hiding this comment.
Pull request overview
Fixes directive matching in cache middleware so Cache-Control / Pragma directives are recognized when followed by optional whitespace or by = parameters (e.g., no-cache , no-cache="Set-Cookie").
Changes:
- Refactors
hasDirectiveto useparseCacheControlDirectivesfor directive-key matching. - Adds regression tests covering directive terminators, parameterized directives, and negative matches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| middleware/cache/cache.go | Switches hasDirective from substring scanning to directive parsing for more robust matching. |
| middleware/cache/cache_test.go | Adds table-driven tests to prevent regressions for directive terminators and edge cases. |
| start := 0 | ||
| for start < len(cc) { | ||
| end := start | ||
| for end < len(cc) && cc[end] != ',' { | ||
| end++ | ||
| } |
There was a problem hiding this comment.
parseCacheControlDirectives currently only treats ' ' as optional whitespace when trimming keys/parts. As a result, for inputs like "no-cache\t" the parsed key becomes "no-cache\t" and this comparison never matches, so the bug for tab-terminated directives persists (and the new regression test will fail). Update the directive parser (or this call site) to treat \t as OWS everywhere it trims/skips whitespace (leading separators, key/value trimming).
| if !utils.EqualFold(cc[i:i+dirLen], directive) { | ||
| continue | ||
| start := 0 | ||
| for start < len(cc) { |
There was a problem hiding this comment.
parseCacheControlDirectives([]byte(cc), ...) allocates a new byte slice on every call. Since the parser is read-only, prefer utils.UnsafeBytes(cc) (as used elsewhere in this file, e.g. parseMaxAge at cache.go:1086) to avoid the extra allocation in this hot path.
| for start < len(cc) { | |
| parseCacheControlDirectives(utils.UnsafeBytes(cc), func(key, _ []byte) { |
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.header+"_"+tt.directive, func(t *testing.T) { | ||
| t.Parallel() | ||
| got := hasDirective(tt.header, tt.directive) | ||
| require.Equal(t, tt.expect, got) | ||
| }) |
There was a problem hiding this comment.
Subtest names are built from the raw header value (tt.header+"_"+tt.directive). Because some cases include control characters (e.g. \t) and quotes, the test output and -run filtering can become hard to read/use. Consider using a sanitized name (e.g. fmt.Sprintf("%q_%s", tt.header, tt.directive) or an explicit name field).
|
Validation run:\n\n |
|
we already have a pr for this #4211 |
Description
Fixes directive matching in cache middleware so valid
Cache-Control/Pragmadirectives are recognized when followed by optional whitespace or by=parameters.Fixes #4143
Changes introduced
Replaced manual substring scanning in
hasDirectivewith directive-key parsing viaparseCacheControlDirectives.Added regression tests for directive terminators and edge cases:
no-cache)no-cache\t)no-cache="Set-Cookie")my-no-cache,private-ish)Benchmarks: Not applicable (logic correctness fix).
Documentation Update: Not required.
Changelog/What's New: Not required.
Migration Guide: Not required.
API Alignment with Express: Not applicable.
API Longevity: Uses existing directive parser path for more robust token matching.
Examples: Added table-driven examples in tests.
Type of change
Checklist
/docs/directory for Fiber's documentation.Commit formatting
Commit uses emoji prefix per contribution guidance.