🐛 bug: Fix Etag validation per RFC 9110#3554
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes entity tag (ETag) quoting per RFC 9110 by enforcing quoted formats in ETag comparisons, updating tests, and documenting the requirement.
- Enforce proper quoted ETags in matchEtag and matchEtagStrong functions.
- Update tests (helpers_test.go and ctx_test.go) to use correctly quoted ETags.
- Update documentation to illustrate the proper ETag quoting format.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| helpers_test.go | Added tests for ETag comparison functions to ensure RFC 9110 compliance. |
| helpers.go | Introduced normalizeEtag and updated matchEtag/matchEtagStrong for quoting. |
| docs/middleware/etag.md | Documented the requirement for ETag quoting with an example. |
| ctx_test.go | Updated test headers to use quoted ETags for consistency in comparisons. |
Comments suppressed due to low confidence (1)
helpers.go:496
- Consider adding unit tests for normalizeEtag to cover edge cases such as empty strings, strings with only one quote, or other malformed tags to ensure robust behavior.
func normalizeEtag(t string) (tag string, weak, ok bool) {
|
""" WalkthroughThe changes implement comprehensive RFC 9110-compliant handling of HTTP ETag headers. This includes new helper functions for normalizing, comparing, and validating ETags, updated and expanded unit tests, and improved documentation clarifying the requirement for quoted ETags. Test cases and benchmarks are updated to use correctly quoted ETag values. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App
participant Helpers
Client->>App: Sends request with If-None-Match header (quoted ETags)
App->>Helpers: Calls isEtagStale(etag, noneMatchBytes)
Helpers->>Helpers: normalizeEtag and matchEtag (weak comparison)
Helpers-->>App: Returns staleness result
App-->>Client: Responds based on ETag freshness
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningshelpers.go (1)⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gaby, 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 refines the handling of HTTP ETags to strictly adhere to RFC 9110, specifically regarding the requirement for entity tags to be quoted. This ensures more robust and standard-compliant ETag comparison logic within the application, improving cache validation and conditional request processing.
Highlights
- ETag Quoting Enforcement: The
matchEtagfunction now strictly enforces RFC 9110's requirement for entity tags to be quoted, ensuring proper validation ofIf-None-MatchandETagheaders. - New ETag Utility Functions: Introduced
normalizeEtagto parse and validate ETag strings, andmatchEtagStrongfor performing strong ETag comparisons as per RFC 9110. - Updated Test Coverage: Existing
ctxtests were updated to use quoted ETags, and new comprehensive tests were added formatchEtag,matchEtagStrong, andisEtagStaleto validate the new ETag logic. - Documentation Update: The ETag middleware documentation has been updated to explicitly state the requirement for entity tags in requests to be quoted according to RFC 9110.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3554 +/- ##
==========================================
+ Coverage 91.02% 91.04% +0.01%
==========================================
Files 110 110
Lines 10955 10976 +21
==========================================
+ Hits 9972 9993 +21
Misses 731 731
Partials 252 252
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.
Code Review
This pull request correctly enforces RFC 9110 for ETag quoting, which is a great improvement for correctness and interoperability. The changes in helpers.go are well-implemented, and the new tests for matchEtag and matchEtagStrong are solid. I've added a couple of suggestions to improve the test coverage for isEtagStale and to correct an example in the documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ctx_test.go (2)
1428-1433: Prefer raw-string literals for clearer ETag test dataThe header value requires heavy escaping (
\"a\") which makes the string hard to scan.
A raw-string literal keeps the visual structure and removes the escape noise:- c.Request().Header.Set(HeaderIfNoneMatch, "\"a\", \"b\", \"c\", \"d\"") + c.Request().Header.Set(HeaderIfNoneMatch, `"a", "b", "c", "d"`)Same idea for the subsequent call just below.
1462-1471: Consistently use raw literals for ETag stringsFor the ETag scenarios below the escaping can again be dropped for readability:
- c.Request().Header.Set(HeaderIfNoneMatch, "\"675af34563dc-tr34\"") + c.Request().Header.Set(HeaderIfNoneMatch, `"675af34563dc-tr34"`) - c.Request().Header.Set(HeaderIfNoneMatch, "\"a\", \"b\"") - c.Response().Header.Set(HeaderETag, "\"c\"") + c.Request().Header.Set(HeaderIfNoneMatch, `"a", "b"`) + c.Response().Header.Set(HeaderETag, `"c"`) - c.Response().Header.Set(HeaderETag, "\"a\"") + c.Response().Header.Set(HeaderETag, `"a"`)No behavioural change – just easier to read.
helpers_test.go (1)
1379-1387: Consider renaming the test function for clarity.The test function name
Test_IsEtagStale_Invalidis potentially misleading since it tests both scenarios where ETags are stale (return true) and not stale (return false). The test logic itself is correct - it properly validates that unquoted ETag values are considered stale and that proper ETag matching prevents staleness.Consider renaming to be more descriptive of what it actually tests:
-func Test_IsEtagStale_Invalid(t *testing.T) { +func Test_IsEtagStale(t *testing.T) {The test cases appropriately validate ETag staleness checking with both quoted and unquoted values, which is essential for RFC 9110 compliance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctx_test.go(2 hunks)docs/middleware/etag.md(1 hunks)helpers.go(1 hunks)helpers_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
docs/middleware/etag.md (2)
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
ctx_test.go (6)
undefined
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: gaby
PR: #3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in ctx_test.go, it is acceptable to use invalid CIDR notation such as "0.0.0.1/31junk" for testing purposes.
</retrieved_learning>
<retrieved_learning>
Learnt from: ReneWerner87
PR: #3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the Test method in app.go.
</retrieved_learning>
<retrieved_learning>
Learnt from: gaby
PR: #3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the middleware/adaptor/adaptor.go file of the Fiber framework, when updating context handling, replacing c.Context() with c.RequestCtx() is appropriate to access the fasthttp.RequestCtx.
</retrieved_learning>
<retrieved_learning>
Learnt from: efectn
PR: #3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the Test_App_ShutdownWithContext function in app_test.go, the clientDone channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional time.Sleep calls.
</retrieved_learning>
helpers_test.go (12)
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
helpers.go (1)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, ubuntu-latest)
🔇 Additional comments (7)
helpers.go (4)
494-505: Excellent RFC 9110-compliant ETag normalization implementation!The function correctly validates ETag format, handles weak indicators, and ensures proper quoting. The logic covers all edge cases and follows the RFC specification precisely.
507-518: Solid weak ETag comparison implementation!The function correctly implements RFC 9110 §8.8.3.2 weak comparison semantics by using
normalizeEtagfor validation and comparing the core values while ignoring weak indicators.
520-531: Perfect strong ETag comparison implementation!The function correctly implements RFC 9110 §8.8.3.1 strong comparison semantics by rejecting weak tags and only matching when both ETags are strong and identical.
533-560: Well-implemented ETag staleness detection!The function correctly parses If-None-Match header values and uses weak comparison semantics as required by RFC 9110. The byte-level parsing efficiently handles spaces and commas as delimiters.
docs/middleware/etag.md (1)
48-52: Excellent documentation addition!The documentation clearly explains the RFC 9110 compliance requirement for quoted entity tags and provides a helpful example. This will guide users in correctly formatting their ETag headers.
helpers_test.go (2)
1357-1366: LGTM! Comprehensive weak ETag comparison testing.The test correctly validates weak ETag comparison semantics according to RFC 9110, including proper handling of the
W/prefix for weak ETags and quoted ETag values. The test cases appropriately cover both positive and negative scenarios.
1368-1377: LGTM! Proper strong ETag comparison testing.The test correctly validates strong ETag comparison logic, ensuring that only exact matches of strong ETags return true, while weak ETags correctly fail strong comparison. This aligns with RFC 9110 requirements for strong ETag validation.
|
Will fix bot review comments in a bit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: e01159e | Previous: 55818fb | Ratio |
|---|---|---|---|
Benchmark_Ctx_BodyRaw_Immutable |
37.76 ns/op 8 B/op 1 allocs/op |
17.86 ns/op 8 B/op 1 allocs/op |
2.11 |
Benchmark_Ctx_BodyRaw_Immutable - ns/op |
37.76 ns/op |
17.86 ns/op |
2.11 |
Benchmark_SlashRecognition/forEach |
5.827 ns/op 0 B/op 0 allocs/op |
3.712 ns/op 0 B/op 0 allocs/op |
1.57 |
Benchmark_SlashRecognition/forEach - ns/op |
5.827 ns/op |
3.712 ns/op |
1.57 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctx_test.go(2 hunks)docs/middleware/etag.md(1 hunks)helpers.go(1 hunks)helpers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/middleware/etag.md
- helpers_test.go
- ctx_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
🪛 GitHub Check: unit (1.24.x, macos-latest)
helpers.go
[failure] 504-504:
cannot use t[1:len(t) - 1] (value of type string) as bool value in return statement
[failure] 502-502:
cannot use "" (untyped string constant) as bool value in return statement
🪛 GitHub Check: unit (1.24.x, ubuntu-latest)
helpers.go
[failure] 504-504:
cannot use t[1:len(t) - 1] (value of type string) as bool value in return statement
[failure] 502-502:
cannot use "" (untyped string constant) as bool value in return statement
🪛 GitHub Check: lint
helpers.go
[failure] 504-504:
cannot use t[1:len(t) - 1] (value of type string) as bool value in return statement (typecheck)
[failure] 502-502:
cannot use "" (untyped string constant) as bool value in return statement
🪛 GitHub Check: govulncheck-check
helpers.go
[failure] 504-504:
cannot use t[1:len(t) - 1] (value of type string) as bool value in return statement
[failure] 502-502:
cannot use "" (untyped string constant) as bool value in return statement
🪛 GitHub Actions: golangci-lint
helpers.go
[error] 502-502: golangci-lint: cannot use "" (untyped string constant) as bool value in return statement
🪛 GitHub Actions: Run govulncheck
helpers.go
[error] 502-502: cannot use "" (untyped string constant) as bool value in return statement
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyse
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (3)
helpers.go (3)
507-518: LGTM: Weak ETag comparison implementation is correct.The function correctly implements RFC 9110 §8.8.3.2 weak comparison semantics by:
- Using
normalizeEtagto validate and extract core values- Ignoring the weak indicator prefix
- Requiring both ETags to be properly quoted
- Returning false for any invalid ETags
520-531: LGTM: Strong ETag comparison implementation is correct.The function correctly implements RFC 9110 §8.8.3.1 strong comparison semantics by:
- Rejecting any weak ETags (w1 || w2 check)
- Requiring both ETags to be valid and properly quoted
- Only matching when both are strong and have identical values
533-560: LGTM: ETag staleness check implementation is correct.The method correctly implements weak comparison semantics for If-None-Match header processing by:
- Parsing comma-separated ETag values from the header
- Using weak comparison (
matchEtag) as required by RFC 9110 §8.8.3.2- Properly handling whitespace around comma separators
- Returning false (not stale) if any ETag matches
The implementation efficiently processes the raw header bytes without unnecessary string allocations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helpers.go(1 hunks)helpers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- helpers_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
🪛 GitHub Check: govulncheck-check
helpers.go
[failure] 505-505:
no new variables on left side of :=
[failure] 497-497:
no new variables on left side of :=
[failure] 496-496:
missing parameter type
🪛 GitHub Check: unit (1.24.x, macos-latest)
helpers.go
[failure] 496-496:
syntax error: missing parameter type
🪛 GitHub Check: unit (1.24.x, ubuntu-latest)
helpers.go
[failure] 496-496:
syntax error: missing parameter type
🪛 GitHub Actions: Run govulncheck
helpers.go
[error] 496-496: Syntax error: missing parameter type at line 496, column 60.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: Analyse
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (3)
helpers.go (3)
509-520: LGTM! RFC 9110-compliant weak ETag comparison.The implementation correctly performs weak ETag comparison by using
normalizeEtagto validate and extract tag values, then comparing the inner values while ignoring weak indicators. The error handling for invalid ETags is appropriate.
522-533: LGTM! Proper strong ETag comparison implementation.The function correctly implements strong ETag comparison per RFC 9110 §8.8.3.1, ensuring both ETags are valid, neither is weak, and the inner values match. The logic properly rejects any comparison involving weak ETags.
535-562: LGTM! Robust If-None-Match header processing.The implementation correctly handles comma-separated ETag lists in If-None-Match headers with proper whitespace handling. The logic follows RFC 9110 weak comparison semantics and handles edge cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx.go(1 hunks)helpers.go(1 hunks)helpers_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ctx.go
🚧 Files skipped from review as they are similar to previous changes (1)
- helpers_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
helpers.go (1)
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
🪛 GitHub Check: govulncheck-check
helpers.go
[failure] 542-542:
undefined: utils.TrimSpace
🪛 GitHub Check: unit (1.24.x, macos-13)
helpers.go
[failure] 542-542:
undefined: utils.TrimSpace
🪛 GitHub Check: unit (1.24.x, ubuntu-latest)
helpers.go
[failure] 542-542:
undefined: utils.TrimSpace
🪛 GitHub Check: unit (1.24.x, macos-latest)
helpers.go
[failure] 542-542:
undefined: utils.TrimSpace
🪛 GitHub Actions: Run govulncheck
helpers.go
[error] 542-542: Undefined identifier 'utils.TrimSpace' at line 542.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Compare
- GitHub Check: Analyse
- GitHub Check: lint
- GitHub Check: repeated
🔇 Additional comments (4)
helpers.go (4)
494-509: LGTM: Well-implemented ETag normalization function.The function correctly validates entity tags according to RFC 9110, properly handling weak prefixes and quote validation. The named return parameters and logic flow are clean and accurate.
514-522: LGTM: Correct weak ETag comparison implementation.The function properly implements weak ETag comparison per RFC 9110 §8.8.3.2, ignoring weak indicators while requiring both tags to be valid and properly quoted.
524-535: LGTM: Proper strong ETag comparison implementation.The function correctly implements strong ETag comparison per RFC 9110 §8.8.3.1, ensuring both tags must be strong (not weak) and properly quoted for a match.
540-570: Approve the ETag staleness logic after fixing the compilation error.The overall implementation correctly handles ETag staleness determination with proper wildcard handling and weak comparison semantics. The parsing logic for comma-separated ETags is sound.
gaby
added
the
📜 RFC Compliance
Summary
matchEtagmatchEtagandisEtagStaleRelated: #3383