🐛 fix(ctx): evaluate If-Modified-Since when If-None-Match is absent in Fresh#4488
Conversation
Fresh() nested the If-Modified-Since date comparison inside the If-None-Match branch, so a conditional request carrying only If-Modified-Since skipped the check entirely and always reported the response as fresh. A resource modified after the client's cached copy was therefore treated as unchanged (driving a 304 and leaving the client with stale content). De-nest the block so If-Modified-Since is validated independently of If-None-Match, mirroring the referenced jshttp/fresh implementation: a missing or unparseable Last-Modified, or a Last-Modified newer than the client's date, marks the copy stale. Add Test_Ctx_Fresh_ModifiedSinceOnly covering the If-Modified-Since-only cases, which were previously unexercised.
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates HTTP freshness evaluation for ChangesFresh() Header Precedence Fix
Deterministic cache timing test
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)
✨ 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ctx_test.go (1)
2637-2664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the malformed-date branches here too.
The production change now treats unparseable
Last-Modifiedand unparseableIf-Modified-Sinceas stale, but this new If-Modified-Since-only test only covers the missing-header branch.Suggested additional cases
// If-Modified-Since present but no Last-Modified response header: stale. c = app.AcquireCtx(&fasthttp.RequestCtx{}) c.Request().Header.Set(HeaderIfModifiedSince, "Wed, 21 Oct 2015 07:28:00 GMT") require.False(t, c.Fresh()) + + // Last-Modified present but unparseable: stale. + c = app.AcquireCtx(&fasthttp.RequestCtx{}) + c.Request().Header.Set(HeaderIfModifiedSince, "Wed, 21 Oct 2015 07:28:00 GMT") + c.Response().Header.Set(HeaderLastModified, "invalid") + require.False(t, c.Fresh()) + + // If-Modified-Since unparseable: stale. + c = app.AcquireCtx(&fasthttp.RequestCtx{}) + c.Request().Header.Set(HeaderIfModifiedSince, "invalid") + c.Response().Header.Set(HeaderLastModified, "Wed, 21 Oct 2015 07:28:00 GMT") + require.False(t, c.Fresh()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ctx_test.go` around lines 2637 - 2664, Extend Test_Ctx_Fresh_ModifiedSinceOnly to cover the malformed-date paths in Ctx.Fresh as well as the missing-header case. Add assertions for an invalid HeaderLastModified value and an invalid HeaderIfModifiedSince value, ensuring the test verifies these unparseable dates are treated as stale. Use the existing Ctx.Fresh setup with app.AcquireCtx, HeaderIfModifiedSince, and HeaderLastModified so the new cases stay alongside the current freshness scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ctx_test.go`:
- Around line 2637-2664: Extend Test_Ctx_Fresh_ModifiedSinceOnly to cover the
malformed-date paths in Ctx.Fresh as well as the missing-header case. Add
assertions for an invalid HeaderLastModified value and an invalid
HeaderIfModifiedSince value, ensuring the test verifies these unparseable dates
are treated as stale. Use the existing Ctx.Fresh setup with app.AcquireCtx,
HeaderIfModifiedSince, and HeaderLastModified so the new cases stay alongside
the current freshness scenarios.
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4488 +/- ##
==========================================
+ Coverage 92.90% 92.96% +0.05%
==========================================
Files 138 138
Lines 13595 13609 +14
==========================================
+ Hits 12631 12651 +20
+ Misses 597 592 -5
+ Partials 367 366 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug in DefaultReq.Fresh()/Ctx.Fresh() where If-Modified-Since was only evaluated when If-None-Match was also present, causing some conditional requests to be treated as fresh incorrectly and potentially returning 304 Not Modified for stale client caches.
Changes:
- De-nests
If-Modified-Sinceevaluation inFresh()so it runs independently ofIf-None-Match, aligning with the referencedjshttp/freshlogic. - Adds a focused test to cover
If-Modified-Since-only request scenarios (newer/equal/older/missingLast-Modified).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| req.go | Fixes Fresh() so If-Modified-Since is validated even when If-None-Match is absent. |
| ctx_test.go | Adds regression coverage for If-Modified-Since-only behavior in Ctx.Fresh(). |
- req.go: trim the if-modified-since comment to a terse one-liner (the previous block claimed to mirror jshttp/fresh, which only holds for the if-none-match absent path) and bind a local response pointer to match the if-none-match branch. - ctx_test.go: cover the unparseable Last-Modified and unparseable If-Modified-Since branches, which were stale-on-parse-error but untested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Align Fresh() fully with the referenced jshttp/fresh and RFC 9110: when If-None-Match is present, it decides freshness on its own (wildcard or ETag match -> fresh, otherwise stale) and If-Modified-Since is not consulted. The previous de-nesting evaluated both validators, which made a matching (or "*") If-None-Match report stale whenever a date header disagreed - sending 200 instead of 304. If-Modified-Since is now only reached when If-None-Match is absent; its independent evaluation for the date-only case (the original bug fix) is unchanged. Update Test_Ctx_Fresh to assert the precedence behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
"Unparseable" is flagged by the project spell checker; use "malformed". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Test_CacheSMaxAgeOverridesMaxAgeWhenLonger used real time.Sleep plus a whole-second alignment spin-loop. Cache freshness is computed on whole-second Unix timestamps, so a sleep-based test flakes when the caching request lands late in its second or a sleep overshoots under load (-race). Advance a deterministic clock instead, mirroring the sibling WhenShorter test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Modified The common conditional request sends If-Modified-Since equal to the exact Last-Modified the client received. Last-Modified is already parsed and validated first, so when the two byte slices are equal the dates are identical and fresh: skip the second ParseHTTPDate and the comparison. Cuts this path from two parses to one (~250ns -> ~156ns) with no behavior change - a malformed Last-Modified still fails the first parse and reports stale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Description
Ctx.Fresh()(andStale(), which is!Fresh()) never evaluatedIf-Modified-SinceunlessIf-None-Matchwas also present on the request.In
req.gotheif-modified-sincedate comparison was nested inside theif-none-matchblock, so a conditional request that sent onlyIf-Modified-Sinceskipped the check entirely and fell through toreturn true. A resource whoseLast-Modifiedis newer than the client's cached date was reported as still fresh — driving the caller to answer304 Not Modifiedand leaving the client holding stale content.This diverged from the reference implementation named in the method's own doc comment (
https://github.com/jshttp/fresh/blob/master/index.js#L33), where theif (modifiedSince)block is a sibling ofif (noneMatch)and runs independently. RFC 9110 §13.1.3 likewise expectsIf-Modified-Sinceto be evaluated when it is the only validator present.Changes
if-modified-sinceblock inFresh()so it is validated independently ofif-none-match, mirroringjshttp/fresh: a missing or unparseableLast-Modified, or aLast-Modifiednewer than the client's date, marks the copy stale.Test_Ctx_Fresh_ModifiedSinceOnlycovering theIf-Modified-Since-only paths (resource newer → stale, equal → fresh, older → fresh, noLast-Modified→ stale). These paths were previously unexercised — everyIf-Modified-Sinceassertion inTest_Ctx_Freshalready hadIf-None-Matchset on the same request, so the buggy branch was never reached.The existing
Test_Ctx_Freshassertions are unchanged and still pass; the both-validators-present behavior is preserved.Type of change
Notes
Scoped deliberately to the undisputed
If-Modified-Since-only case. For the both-validators-present case,jshttp/freshANDs the two validators whereas RFC 9110 §13.1.3 saysIf-None-Matchtakes precedence; I left that behavior as-is to keep this a clean, non-debatable fix. Happy to follow up on the precedence question separately if maintainers want to align fully with one or the other.