🩹 fix: Cache middleware hasDirective misses directives with trailing space/tab/=#4144
🩹 fix: Cache middleware hasDirective misses directives with trailing space/tab/=#4144mango766 wants to merge 2 commits intogofiber:mainfrom
Conversation
…space/tab/=
The hasDirective function only checked for comma or end-of-string after a
matched directive name. This caused it to miss directives followed by a space,
tab, or '=' (for directives with arguments like no-cache="field").
For example, `Pragma: no-cache ` (with trailing whitespace) or
`Cache-Control: no-cache="Set-Cookie"` were not recognized, potentially
causing private or no-cache responses to be incorrectly cached.
The fix adds space (' '), tab ('\t'), and equals ('=') as valid directive
terminators, aligning with RFC 9111 Cache-Control syntax.
Fixes gofiber#4143
Co-Authored-By: Claude (claude-opus-4-6) <noreply@anthropic.com>
WalkthroughThe PR fixes Cache-Control directive detection in the cache middleware by treating directives as standalone tokens, recognizing directives followed by whitespace, tabs, or Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical caching bug by enhancing the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 correctly fixes an issue in the hasDirective function where it failed to recognize certain valid terminators for cache directives, such as trailing spaces, tabs, or equals signs. This is a crucial fix to prevent incorrect caching behavior. The addition of a comprehensive unit test suite for hasDirective is also a significant improvement. I've identified a related edge case where a tab character preceding a directive is not handled correctly, and I've suggested an additional test case to highlight this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/cache/cache.go (1)
920-923:⚠️ Potential issue | 🟡 MinorAllow HTAB as a valid preceding boundary for directives.
hasDirectivestill rejects directives preceded by a tab, so valid inputs like"public,\tno-cache"or"\tno-cache"are missed (Line 922). That keeps a false-negative path for cache-control detection.🐛 Proposed fix
if i > 0 { prev := cc[i-1] - if prev != ' ' && prev != ',' { + if prev != ' ' && prev != '\t' && prev != ',' { continue } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/cache/cache.go` around lines 920 - 923, In hasDirective (middleware/cache/cache.go) the check that skips matches when the preceding character isn't a space or comma erroneously rejects HTAB; update the preceding-boundary check around variable cc and prev in that loop to treat '\t' as a valid boundary as well (i.e., allow prev == '\t' in addition to ' ' and ',') so directives like "\tno-cache" or "public,\tno-cache" are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@middleware/cache/cache.go`:
- Around line 920-923: In hasDirective (middleware/cache/cache.go) the check
that skips matches when the preceding character isn't a space or comma
erroneously rejects HTAB; update the preceding-boundary check around variable cc
and prev in that loop to treat '\t' as a valid boundary as well (i.e., allow
prev == '\t' in addition to ' ' and ',') so directives like "\tno-cache" or
"public,\tno-cache" are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa26c701-10ea-46b8-b5de-6e7fa4174bb2
📒 Files selected for processing (2)
middleware/cache/cache.gomiddleware/cache/cache_test.go
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4144 +/- ##
==========================================
+ Coverage 91.07% 91.12% +0.05%
==========================================
Files 122 122
Lines 11550 11551 +1
==========================================
+ Hits 10519 10526 +7
+ Misses 651 647 -4
+ Partials 380 378 -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:
|
|
@mango766 check the review comments |
|
@mango766 pls add the tab case and then its ready for a merge |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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_test.go`:
- Line 5115: The test fails because hasDirective only treats space and comma as
valid preceding characters; modify the preceding-character check inside
hasDirective to also accept tab ('\t') so strings like "public,\tno-cache"
match; locate the condition that currently reads "if prev != ' ' && prev != ','
{ continue }" and add '\t' to that allowed set (or equivalently accept
unicode.IsSpace(prev)), then run tests to confirm the "tab before directive"
case passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51216787-c0ae-4063-86d2-dc72cce1c0b8
📒 Files selected for processing (1)
middleware/cache/cache_test.go
| {name: "last in list", cc: "public, no-cache", directive: "no-cache", expected: true}, | ||
| {name: "middle in list", cc: "public, no-cache, no-store", directive: "no-cache", expected: true}, | ||
| {name: "no spaces", cc: "public,no-cache,no-store", directive: "no-cache", expected: true}, | ||
| {name: "tab before directive", cc: "public,\tno-cache", directive: "no-cache", expected: true}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if the implementation supports tab as a preceding character
rg -n "prev != " middleware/cache/cache.go -A2 -B2Repository: gofiber/fiber
Length of output: 161
🏁 Script executed:
# Get the complete hasDirective function
rg -n "func.*hasDirective" middleware/cache/cache.go -A 50Repository: gofiber/fiber
Length of output: 1377
🏁 Script executed:
# Get the complete test case context around line 5115
sed -n '5110,5120p' middleware/cache/cache_test.goRepository: gofiber/fiber
Length of output: 859
🏁 Script executed:
# Check if there are any updates to the preceding character validation logic
rg -n "prev.*==" middleware/cache/cache.goRepository: gofiber/fiber
Length of output: 39
Test case "tab before directive" will fail against the current implementation.
The hasDirective implementation (line 922) only allows space ' ' or comma ',' as valid preceding characters:
if prev != ' ' && prev != ',' {
continue
}For the test case cc: "public,\tno-cache" with directive: "no-cache", the tab '\t' at position 7 (before the directive) will not match this condition, causing the function to return false instead of the expected true.
While the implementation does support tab as a following character (line 927: cc[end] == '\t'), tabs are not accepted as preceding characters. Either:
- Add
'\t'to the preceding character check (line 922) to accept tabs before directives, or - Update the test expectation to
false
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware/cache/cache_test.go` at line 5115, The test fails because
hasDirective only treats space and comma as valid preceding characters; modify
the preceding-character check inside hasDirective to also accept tab ('\t') so
strings like "public,\tno-cache" match; locate the condition that currently
reads "if prev != ' ' && prev != ',' { continue }" and add '\t' to that allowed
set (or equivalently accept unicode.IsSpace(prev)), then run tests to confirm
the "tab before directive" case passes.
3 participants
Summary
Fixes #4143
The
hasDirectivefunction in the cache middleware only treated,and end-of-string as valid terminators after matching a directive name. This caused it to miss directives followed by:Pragma: no-cacheorCache-Control: private(common with some HTTP servers/proxies that pad headers)no-cache\tno-cache="Set-Cookie"(RFC 9111 §5.2.2.4 allows no-cache with field names)This could result in private or no-cache responses being incorrectly cached and served to other users.
Changes
middleware/cache/cache.go: UpdatedhasDirectiveto also accept' ','\t', and'='as valid directive terminators after a matchmiddleware/cache/cache_test.go: Added comprehensiveTest_hasDirectiveunit test with 19 test cases covering all the edge casesBefore (bug)
After (fix)
Test plan
Test_hasDirectivetest cases passgo test ./middleware/cache/ -count=1)