refactor: update keyauth middleware to use shared extractors #3747
Conversation
…ance token validation
|
Warning Rate limit exceeded@sixcolors has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR migrates KeyAuth extractor types and constructors to the shared Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Fiber
participant KeyAuth
participant Extractors
participant Validator
Client->>Fiber: HTTP Request
Fiber->>KeyAuth: invoke KeyAuth middleware
KeyAuth->>Extractors: cfg.Extractor.Extract(ctx)
alt Extractor returns ErrNotFound
Extractors-->>KeyAuth: ErrNotFound
Note over KeyAuth: map to ErrMissingOrMalformedAPIKey
KeyAuth-->>Fiber: 401 / missing or malformed
else token returned
KeyAuth->>KeyAuth: compute auth schemes (from extractor metadata)
alt source == AuthHeader AND AuthScheme != ""
KeyAuth->>Extractors: (implicit) token68 validation
alt invalid token68
KeyAuth-->>Fiber: 401 / missing or malformed
else valid
KeyAuth->>Validator: Validator(ctx, token)
alt valid
KeyAuth-->>Fiber: set locals & next handler
else invalid
KeyAuth-->>Fiber: 401
end
end
else other sources / no scheme
KeyAuth->>Validator: Validator(ctx, token)
alt valid
KeyAuth-->>Fiber: set locals & next handler
else invalid
KeyAuth-->>Fiber: 401
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Summary of Changes
Hello @sixcolors, 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 significantly refactors the keyauth middleware by integrating a new, shared extractors package. This change centralizes the logic for extracting API keys from various request sources, eliminating redundant custom implementations. It also introduces more robust validation for tokens found in authorization headers, aligning with RFC standards and enhancing the overall security and maintainability of the middleware.
Highlights
- Migration to Shared Extractors: The keyauth middleware has been refactored to utilize the new centralized
github.com/gofiber/fiber/v3/extractorspackage for API key extraction, promoting code reuse and consistency across Fiber middleware. - Removal of Redundant Code: The custom
keyauth/extractors.gofile and its associated test file (extractors_test.go) have been removed, as their functionality is now provided by the shared extractors package. - Enhanced Token Validation: Stricter
token68validation, compliant with RFC 7235/9110, has been implemented and applied specifically to tokens extracted from authorization headers, improving security. - Documentation and Configuration Updates: The
keyauth.mddocumentation has been updated to reflect the new usage of shared extractors, including new examples and an updated configuration table. TheConfigstruct andConfigDefaulthave also been updated to use the new extractor types.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| 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 pull request 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 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
-
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. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the keyauth middleware to use shared extractors from the github.com/gofiber/fiber/v3/extractors package instead of local implementations. The changes improve code consistency and maintainability by consolidating extractor functionality.
Key changes:
- Replaced local extractor implementations with shared extractors package
- Updated Config struct to use extractors.Extractor type
- Enhanced token validation for authorization headers with stricter token68 checks
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/keyauth/keyauth.go | Updated imports, enhanced token validation logic with isValidToken68 function, and modified getAuthSchemes to use extractors package |
| middleware/keyauth/keyauth_test.go | Updated all tests to use extractors package functions and added comprehensive isValidToken68 validation tests |
| middleware/keyauth/config.go | Changed Extractor field type to extractors.Extractor and updated default configuration |
| middleware/keyauth/config_test.go | Updated test to use extractors.FromHeader instead of local implementation |
| middleware/keyauth/extractors.go | Removed entire file as functionality moved to shared extractors package |
| middleware/keyauth/extractors_test.go | Removed entire file as tests moved to shared extractors package |
| docs/middleware/keyauth.md | Updated documentation to reference shared extractors package and improved usage examples |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3747 +/- ##
==========================================
- Coverage 91.98% 91.87% -0.12%
==========================================
Files 114 113 -1
Lines 11753 11654 -99
==========================================
- Hits 10811 10707 -104
- Misses 682 686 +4
- Partials 260 261 +1
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 is a great step towards improving code reuse by refactoring the keyauth middleware to use the shared extractors package. The documentation and test updates are also well-aligned with these changes. I've identified a critical issue with the new token68 validation logic, which doesn't correctly handle base64 padding and would reject valid tokens. Additionally, I've suggested a refactoring to reduce some code duplication in the main middleware handler to improve maintainability. Please see my detailed comments.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/middleware/keyauth.md (2)
25-31: Examples must import and use shared extractors (blocking docs correctness).
Current sample uses keyauth.FromCookie; update to extractors.FromCookie and add import.import ( "crypto/sha256" "crypto/subtle" "github.com/gofiber/fiber/v3" "github.com/gofiber/fiber/v3/middleware/keyauth" + "github.com/gofiber/fiber/v3/extractors" ) ... app.Use(keyauth.New(keyauth.Config{ - Extractor: keyauth.FromCookie("access_token"), + Extractor: extractors.FromCookie("access_token"), Validator: validateAPIKey, }))Also applies to: 49-53
88-96: Same extractor import/use fix in “Authenticate only certain endpoints” example.
Keep docs consistent with the code.import ( "crypto/sha256" "crypto/subtle" "github.com/gofiber/fiber/v3" "github.com/gofiber/fiber/v3/middleware/keyauth" + "github.com/gofiber/fiber/v3/extractors" "regexp" "strings" ) ... app.Use(keyauth.New(keyauth.Config{ Next: authFilter, - Extractor: keyauth.FromCookie("access_token"), + Extractor: extractors.FromCookie("access_token"), Validator: validateAPIKey, }))Also applies to: 131-135
🧹 Nitpick comments (5)
middleware/keyauth/config.go (1)
103-106: Comment is misleading vs behavior (nit).
The comment says “Return default config if nothing provided” but the code panics. Adjust the comment to reflect current behavior.- // Return default config if nothing provided + // If no config is provided, panic because a Validator is requiredmiddleware/keyauth/keyauth.go (1)
105-135: Allow multiple '=' at end per token68; tighten ordering after '='.
Permits 0+ '=' only at the tail; any non-'=' after first '=' is invalid.-// isValidToken68 checks if a string is a valid token68 per RFC 7235/9110. -func isValidToken68(token string) bool { - if token == "" { - return false - } - eqIdx := -1 - for i := 0; i < len(token); i++ { - c := token[i] - if !((c >= 'A' && c <= 'Z') || - (c >= 'a' && c <= 'z') || - (c >= '0' && c <= '9') || - c == '-' || c == '.' || c == '_' || c == '~' || c == '+' || c == '/' || c == '=') { - return false - } - if c == '=' { - if i == 0 { - return false - } - if eqIdx != -1 { - // Multiple equals - return false - } - eqIdx = i - } - } - if eqIdx != -1 && eqIdx != len(token)-1 { - // Equals not at end - return false - } - return true -} +// isValidToken68 checks token68 syntax per RFC 7235/9110: +// 1+ of ALPHA/DIGIT/-._~+/ followed by 0+ '=' (padding) at the end only. +func isValidToken68(s string) bool { + if s == "" { + return false + } + seenEq := false + for i := 0; i < len(s); i++ { + c := s[i] + switch { + case c >= 'A' && c <= 'Z', + c >= 'a' && c <= 'z', + c >= '0' && c <= '9', + c == '-' || c == '.' || c == '_' || c == '~' || c == '+' || c == '/': + if seenEq { + // Non '=' after '=' is invalid + return false + } + case c == '=': + // '=' cannot be first and once seen, only '=' may follow + if i == 0 { + return false + } + seenEq = true + default: + return false + } + } + return true +}docs/middleware/keyauth.md (1)
224-225: Fix markdownlint: remove extra blank lines.
Unblocks the MD012 check.- - @@ -Also applies to: 290-290
middleware/keyauth/keyauth_test.go (2)
1114-1124: Align tests with token68 spec if function is updated.
If you accept allowing multiple '=' at end, update expectations for these cases.- {token: "token==", want: false, name: "multiple equals"}, + {token: "token==", want: true, name: "multiple equals at end"}, @@ - {token: "token68==", want: false, name: "multiple equals at end"}, + {token: "token68==", want: true, name: "multiple equals at end"},
1118-1119: golangci-lint dupword: avoid literal “token token” in strings (test-only).
Either add nolint or build the string via concatenation to keep intent explicit.- {token: "token token", want: false, name: "space in token"}, - {token: "token\ttoken", want: false, name: "tab character in token"}, + // nolint:dupword // intentional duplicate for validation + {token: "token" + " token", want: false, name: "space in token"}, + // nolint:dupword // intentional duplicate for validation + {token: "token" + "\t" + "token", want: false, name: "tab character in token"},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/middleware/keyauth.md(3 hunks)middleware/keyauth/config.go(3 hunks)middleware/keyauth/config_test.go(2 hunks)middleware/keyauth/extractors.go(0 hunks)middleware/keyauth/extractors_test.go(0 hunks)middleware/keyauth/keyauth.go(4 hunks)middleware/keyauth/keyauth_test.go(17 hunks)
💤 Files with no reviewable changes (2)
- middleware/keyauth/extractors_test.go
- middleware/keyauth/extractors.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/keyauth/config_test.gomiddleware/keyauth/config.gomiddleware/keyauth/keyauth.gomiddleware/keyauth/keyauth_test.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/keyauth.md
🧠 Learnings (5)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/keyauth/config_test.gomiddleware/keyauth/keyauth_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
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.
Applied to files:
middleware/keyauth/config_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
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`.
Applied to files:
middleware/keyauth/config_test.go
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
middleware/keyauth/config_test.gomiddleware/keyauth/config.godocs/middleware/keyauth.mdmiddleware/keyauth/keyauth_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/keyauth/keyauth_test.go
🧬 Code graph analysis (4)
middleware/keyauth/config_test.go (1)
extractors/extractors.go (1)
FromHeader(303-315)
middleware/keyauth/config.go (1)
extractors/extractors.go (2)
Extractor(67-73)FromAuthHeader(101-154)
middleware/keyauth/keyauth.go (2)
extractors/extractors.go (3)
Extractor(67-73)Source(38-38)SourceAuthHeader(45-45)req.go (1)
Locals(520-532)
middleware/keyauth/keyauth_test.go (1)
extractors/extractors.go (8)
Extractor(67-73)FromHeader(303-315)FromAuthHeader(101-154)FromCookie(181-193)FromQuery(344-356)FromParam(221-237)FromForm(264-276)Chain(463-504)
🪛 GitHub Check: markdownlint
docs/middleware/keyauth.md
[failure] 225-225: Multiple consecutive blank lines
docs/middleware/keyauth.md:225 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md012.md
[failure] 224-224: Multiple consecutive blank lines
docs/middleware/keyauth.md:224 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md012.md
[failure] 290-290: Multiple consecutive blank lines
docs/middleware/keyauth.md:290 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md012.md
🪛 GitHub Actions: markdownlint
docs/middleware/keyauth.md
[error] 224-224: markdownlint-cli2-action failed: MD012/no-multiple-blanks Multiple consecutive blank lines. (Expected: 1; Actual: 2)
🪛 GitHub Check: lint
middleware/keyauth/keyauth_test.go
[failure] 1119-1119:
Duplicate words (token) found (dupword)
[failure] 1118-1118:
Duplicate words (token) found (dupword)
🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth_test.go
[error] 1118-1118: golangci-lint: Duplicate words (token) found (dupword). Step: golangci-lint run.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
🔇 Additional comments (5)
middleware/keyauth/config_test.go (2)
8-8: LGTM: migrated to shared extractors import.
Consistent with the PR’s API shift.
53-53: LGTM: using extractors.FromHeader in tests.
Matches the new public API.middleware/keyauth/config.go (1)
9-10: LGTM: Config.Extractor migrated to extractors.Extractor and default set to FromAuthHeader("Bearer").
Types and docs align with shared extractors.Also applies to: 85-87, 98-99
middleware/keyauth/keyauth.go (1)
150-159: LGTM: getAuthSchemes updated to extractors.Extractor and recurses chain correctly.
Accurately collects schemes for challenges.middleware/keyauth/keyauth_test.go (1)
17-18: LGTM: tests migrated to shared extractors API.
Covers all constructors and chains; consistent with runtime changes.Also applies to: 90-104, 200-205, 265-266, 279-280, 823-824, 845-846, 856-856, 1015-1018, 1031-1034
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/keyauth/keyauth_test.go (1)
191-196: Fix compile error: unused constant ‘scheme’The constant ‘scheme’ is declared but not used; Go will fail to compile. Either remove it or use it in the extractor chain.
Apply this diff to use the constant and keep intent clear:
const ( desc = "auth with correct key" success = "Success!" scheme = "Bearer" ) // setup the fiber endpoint app := fiber.New() -customExtractor := extractors.Chain( - extractors.FromAuthHeader("Bearer"), +customExtractor := extractors.Chain( + extractors.FromAuthHeader(scheme), extractors.FromHeader("key"), extractors.FromCookie("key"), extractors.FromQuery("key"), )Also applies to: 200-205
🧹 Nitpick comments (1)
middleware/keyauth/keyauth_test.go (1)
151-153: Prefer fiber.HeaderAuthorization constantMinor consistency/readability: use fiber.HeaderAuthorization instead of hard-coded "Authorization" in tests.
Example:
-req.Header.Set("Authorization", "Bearer "+testKey) +req.Header.Set(fiber.HeaderAuthorization, "Bearer "+testKey)Also applies to: 370-371, 425-426, 491-492, 573-574, 620-621, 682-683, 705-706, 722-723, 739-740, 757-758, 778-779, 916-918, 935-937, 954-956, 1007-1008
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/keyauth/keyauth.go(4 hunks)middleware/keyauth/keyauth_test.go(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/keyauth/keyauth.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/keyauth/keyauth_test.go
🧠 Learnings (3)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/keyauth/keyauth_test.go
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
middleware/keyauth/keyauth_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/keyauth/keyauth_test.go
🧬 Code graph analysis (1)
middleware/keyauth/keyauth_test.go (1)
extractors/extractors.go (8)
Extractor(67-73)FromHeader(303-315)FromAuthHeader(101-154)FromCookie(181-193)FromQuery(344-356)FromParam(221-237)FromForm(264-276)Chain(463-504)
🪛 GitHub Check: lint
middleware/keyauth/keyauth_test.go
[failure] 1119-1119:
Duplicate words (token) found (dupword)
[failure] 1118-1118:
Duplicate words (token) found (dupword)
🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth_test.go
[error] 1118-1118: golangci-lint: Duplicate words (token) found (dupword) in middleware/keyauth/keyauth_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (3)
middleware/keyauth/keyauth_test.go (3)
17-18: Importing shared extractors looks goodMigration to github.com/gofiber/fiber/v3/extractors is correct and aligns tests with the new API.
90-107: Config.Extractor migration to shared extractors is correctSwitch/case maps cleanly to extractors.FromHeader/FromAuthHeader/FromCookie/FromQuery/FromParam/FromForm. Good coverage of all sources.
823-826: WWW-Authenticate challenge behavior tests are solidNice assertions for default non-auth extractor (“ApiKey …”), chained extractors with/without auth header, and realm customization.
Also applies to: 844-847, 855-858
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
middleware/keyauth/keyauth_test.go (1)
1101-1135: Token68 tests look solid; minor wording nit in names.Test matrix correctly accepts multiple trailing ‘=’. Consider clearer names.
- {token: "token==", want: true, name: "multiple equals"}, + {token: "token==", want: true, name: "multiple equal signs"}, ... - {token: "token68==", want: true, name: "multiple equals at end"}, + {token: "token68==", want: true, name: "multiple equal signs at end"},
🧹 Nitpick comments (3)
middleware/keyauth/keyauth.go (2)
101-126: RFC-compliant token68; drop alloc in loop.Validation looks correct (allows contiguous trailing ‘=’ only). Avoid
[]byte(token)allocation.- for i, c := range []byte(token) { + for i := 0; i < len(token); i++ { + c := token[i] switch {
141-149: Deduplicate auth schemes to avoid repeated WWW-Authenticate challenges.If the chain contains the same scheme multiple times, header may duplicate entries.
-func getAuthSchemes(e extractors.Extractor) []string { - var schemes []string - if e.Source == extractors.SourceAuthHeader && e.AuthScheme != "" { - schemes = append(schemes, e.AuthScheme) - } - for _, ex := range e.Chain { - schemes = append(schemes, getAuthSchemes(ex)...) - } - return schemes -} +func getAuthSchemes(e extractors.Extractor) []string { + seen := make(map[string]struct{}) + var out []string + var walk func(ex extractors.Extractor) + walk = func(ex extractors.Extractor) { + if ex.Source == extractors.SourceAuthHeader && ex.AuthScheme != "" { + if _, ok := seen[ex.AuthScheme]; !ok { + seen[ex.AuthScheme] = struct{}{} + out = append(out, ex.AuthScheme) + } + } + for _, ch := range ex.Chain { + walk(ch) + } + } + walk(e) + return out +}middleware/keyauth/keyauth_test.go (1)
862-884: Parallel subtests: avoid loop variable capture (pre-Go 1.22).If CI ever runs on older Go, capture
ttbeforet.Run.- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt // avoid loop var capture in parallel subtests on pre-Go 1.22 + t.Run(tt.name, func(t *testing.T) { t.Parallel() app := fiber.New() app.Use(New(tt.config))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/middleware/keyauth.md(3 hunks)middleware/keyauth/keyauth.go(4 hunks)middleware/keyauth/keyauth_test.go(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/middleware/keyauth.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/keyauth/keyauth.gomiddleware/keyauth/keyauth_test.go
🧠 Learnings (3)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/keyauth/keyauth_test.go
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
middleware/keyauth/keyauth_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/keyauth/keyauth_test.go
🧬 Code graph analysis (2)
middleware/keyauth/keyauth.go (2)
extractors/extractors.go (3)
Extractor(67-73)Source(38-38)SourceAuthHeader(45-45)req.go (1)
Locals(520-532)
middleware/keyauth/keyauth_test.go (1)
extractors/extractors.go (8)
Extractor(67-73)FromHeader(303-315)FromAuthHeader(101-154)FromCookie(181-193)FromQuery(344-356)FromParam(221-237)FromForm(264-276)Chain(463-504)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
middleware/keyauth/keyauth.go (1)
42-46: Good: normalize extractor ErrNotFound to middleware error.This keeps error semantics stable across sources.
middleware/keyauth/keyauth_test.go (1)
90-107: Migration to shared extractors is correct.Factory switch returns
extractors.Extractorvariants as expected.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/keyauth/keyauth_test.go (1)
704-712: Update expectation: multiple spaces after scheme should fail with strict extractorFromAuthHeader("Bearer") enforces token68 and rejects tokens containing spaces — "Authorization: Bearer " must return 401 with ErrMissingOrMalformedAPIKey.
File: middleware/keyauth/keyauth_test.go Lines: 704-712
- require.Equal(t, http.StatusOK, res.StatusCode) - require.Equal(t, "OK", string(body)) + require.Equal(t, http.StatusUnauthorized, res.StatusCode) + require.Equal(t, ErrMissingOrMalformedAPIKey.Error(), string(body))
🧹 Nitpick comments (4)
extractors/extractors_test.go (1)
401-404: Prefer ErrorIs for error checksUsing require.ErrorIs is more robust for sentinel errors and future wrapping.
Example change:
- require.Equal(t, ErrNotFound, err) + require.ErrorIs(t, err, ErrNotFound)Also applies to: 422-424, 439-441, 449-451, 457-459, 465-467, 472-475, 496-498, 535-537
extractors/extractors.go (2)
79-83: Doc mismatch: code enforces exactly one SP, not 1*SPFromAuthHeader requires a single ASCII space after the scheme; multiple spaces are rejected by token68 validation. Update the RFC notes accordingly.
-// - Follows RFC 9110 Section 11.6.2 for Authorization header format -// - Enforces 1*SP (one or more spaces) between auth-scheme and credentials +// - Follows RFC 9110 Section 11.6.2 for Authorization header format +// - Enforces exactly one SP (single ASCII space) between auth-scheme and credentials +// Note: multiple spaces are intentionally rejected for consistency and simplicity
505-530: Avoid []byte allocation in isValidToken68 loopTiny micro‑opt: iterate by index over the string to skip a []byte conversion.
-func isValidToken68(token string) bool { - if token == "" { - return false - } - paddingStarted := false - for i, c := range []byte(token) { +func isValidToken68(token string) bool { + if token == "" { + return false + } + paddingStarted := false + for i := 0; i < len(token); i++ { + c := token[i]docs/guide/extractors.md (1)
283-287: Clarify whitespace rule: single SP requiredDocs claim “1*SP” (one or more) but the implementation requires exactly one space after the scheme. Align the guide with code and tests.
-- **Section 11.6.2 Format**: Enforces `credentials = auth-scheme 1*SP token68` structure -- **1*SP Requirement**: Validates exactly one or more spaces between auth-scheme and token +- **Section 11.6.2 Format**: Enforces `credentials = auth-scheme SP token68` structure +- **SP Requirement**: Requires exactly one ASCII space between auth-scheme and token
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/guide/extractors.md(1 hunks)extractors/extractors.go(5 hunks)extractors/extractors_test.go(3 hunks)middleware/keyauth/keyauth.go(3 hunks)middleware/keyauth/keyauth_test.go(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/keyauth/keyauth.go
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/guide/extractors.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
extractors/extractors.goextractors/extractors_test.gomiddleware/keyauth/keyauth_test.go
🧠 Learnings (3)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/keyauth/keyauth_test.go
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
middleware/keyauth/keyauth_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
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.
Applied to files:
middleware/keyauth/keyauth_test.go
🧬 Code graph analysis (2)
extractors/extractors_test.go (2)
constants.go (1)
HeaderAuthorization(163-163)extractors/extractors.go (2)
FromAuthHeader(115-153)ErrNotFound(64-64)
middleware/keyauth/keyauth_test.go (1)
extractors/extractors.go (8)
Extractor(67-73)FromHeader(302-314)FromAuthHeader(115-153)FromCookie(180-192)FromQuery(343-355)FromParam(220-236)FromForm(263-275)Chain(462-503)
🪛 GitHub Check: lint
extractors/extractors_test.go
[failure] 486-486:
Test_Extractor_FromAuthHeader_Token68_Validation should use t.Cleanup instead of defer (tparallel)
🪛 GitHub Actions: golangci-lint
extractors/extractors_test.go
[error] 486-486: golangci-lint: Test_Extractor_FromAuthHeader_Token68_Validation should use t.Cleanup instead of defer (tparallel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Compare
- GitHub Check: repeated
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
extractors/extractors_test.go(12 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
extractors/extractors_test.go
🧠 Learnings (3)
📚 Learning: 2024-09-25T15:57:10.221Z
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.
Applied to files:
extractors/extractors_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#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.
Applied to files:
extractors/extractors_test.go
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
extractors/extractors_test.go
🧬 Code graph analysis (1)
extractors/extractors_test.go (2)
extractors/extractors.go (18)
ErrNotFound(64-64)FromForm(263-275)FromQuery(343-355)FromHeader(302-314)FromAuthHeader(115-153)FromCookie(180-192)FromParam(220-236)Chain(462-503)Extractor(67-73)Source(38-38)SourceCustom(60-60)FromCustom(406-415)SourceAuthHeader(45-45)SourceHeader(42-42)SourceForm(48-48)SourceQuery(51-51)SourceParam(54-54)SourceCookie(57-57)constants.go (2)
MIMEApplicationForm(28-28)HeaderAuthorization(163-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (3)
extractors/extractors_test.go (3)
219-258: Auth header edge-cases coverage looks solid.Good negative/positive assertions for wrong scheme, missing space, and case-insensitive matching aligned with the extractor behavior.
148-217: Chain behavior and error mapping verified well.The tests for empty chains, success short‑circuiting, and empty‑string extractor returning ErrNotFound match the implementation semantics.
848-899: URL decoding scenarios are appropriate.Query/form/param decoding expectations match fasthttp/Fiber behavior, including percent-encoded slashes in params.
If CI runs against multiple Go versions, please confirm these pass under the lowest supported Go version as URL/param decoding behaviors can subtly differ across fasthttp releases.
|
@coderabbitai a manual copy of a loop variable is no longer necessary and can be removed, thanks to a change introduced in Go 1.22. This was part of a language modernization effort to fix a common bug related to variable capture in closures. |
|
Tip For best results, initiate chat on the files or code changes. However, after searching through the modified files in this PR, I don't see any obvious manual loop variable copies (like Could you point me to the specific file and lines where you see manual loop variable copies that can now be removed? This will help me provide more targeted feedback and potentially suggest the cleanup you're referring to. ✏️ Learnings added
|
It was related to your code suggestion which I applied, but then reverted in a later commit; here is a link to the comment #3747 (comment) |
…#3747) * refactor: integrate shared extractors into KeyAuth middleware and enhance token validation * refactor: simplify token validation logic and update test cases for isValidToken68 * refactor: error handling for missing API key in KeyAuth middleware * refactor: update KeyAuth documentation and improve test cases for token validation * refactor: move token validation to extractors and update related tests * refactor: enhance extractor tests * refactor: update error assertions in extractor tests to use require.ErrorIs * refactor: update test assertions for HeaderSchemeMultipleSpaces to check for unauthorized status * refactor: capture loop variable in test cases for parallel execution * refactor: rm capture loop variable in test cases for parallel execution
Changes to KeyAuth
Change to extractors