🧹 chore: Return generic errors in KeyAuth middleware#3692
Conversation
|
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. WalkthroughUnifies keyauth middleware error reporting by replacing multiple location-specific missing-key errors and the Config.ErrorHandler with a single exported error and a default 401 response using ErrMissingOrMalformedAPIKey ("Missing or invalid API Key"). Tests and docs updated; 404 for missing route params preserved. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant KeyAuth as KeyAuth Middleware
participant Extractor
participant Handler as Downstream Handler
Client->>Server: HTTP Request
Server->>KeyAuth: run middleware
KeyAuth->>Extractor: attempt extract (header/query/cookie/param/form)
alt key present and valid
KeyAuth->>Handler: call next handler
Handler-->>Client: 2xx response
else missing or invalid key
Note right of KeyAuth #D6EAF8: Default unified unauthorized response
KeyAuth-->>Client: 401 Unauthorized: "Missing or invalid API Key"
end
opt route param missing
Note over Server: Routing may return 404 before middleware runs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3692 +/- ##
=======================================
Coverage 91.99% 92.00%
=======================================
Files 114 114
Lines 11494 11502 +8
=======================================
+ Hits 10574 10582 +8
Misses 661 661
Partials 259 259
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.
Pull Request Overview
This PR standardizes error messaging in the KeyAuth middleware by replacing specific error messages with a generic "Invalid or expired API Key" message for all authentication failures.
- Simplifies error handling by returning a consistent generic error message
- Removes custom error handler logic that distinguished between different failure types
- Updates all tests and documentation to expect the new generic error message
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/keyauth/config.go | Replaces the complex ErrorHandler with a simple one that returns a generic error message |
| middleware/keyauth/keyauth_test.go | Updates all test expectations to use the new generic error message |
| docs/middleware/keyauth.md | Updates documentation example to show the new generic error message |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/middleware/keyauth.md (1)
270-290: Default Config snippet is stale; update ErrorHandler to the unified response.The snippet still shows the old switch on specific error types. Update it to match the simplified default handler implemented in code.
Apply this diff to the code block:
var ConfigDefault = Config{ SuccessHandler: func(c fiber.Ctx) error { return c.Next() }, - ErrorHandler: func(c fiber.Ctx, err error) error { - switch { - case errors.Is(err, ErrMissingOrMalformedAPIKey), - errors.Is(err, ErrMissingAPIKey), - errors.Is(err, ErrMissingAPIKeyInHeader), - errors.Is(err, ErrMissingAPIKeyInQuery), - errors.Is(err, ErrMissingAPIKeyInParam), - errors.Is(err, ErrMissingAPIKeyInForm), - errors.Is(err, ErrMissingAPIKeyInCookie): - return c.Status(fiber.StatusUnauthorized).SendString(err.Error()) - } - return c.Status(fiber.StatusUnauthorized).SendString("Invalid or expired API Key") - }, + ErrorHandler: func(c fiber.Ctx, _ error) error { + return c.Status(fiber.StatusUnauthorized).SendString("Invalid or expired API Key") + }, Realm: "Restricted", Extractor: FromAuthHeader(fiber.HeaderAuthorization, "Bearer"), }
🧹 Nitpick comments (4)
docs/middleware/keyauth.md (2)
66-69: Standardize capitalization: “API key”, not “api-key”.Minor wording consistency nit in the example.
Apply this diff:
-# No api-key specified -> 401 Invalid or expired API Key +# No API key specified -> 401 Invalid or expired API Key
258-266: Clarify default ErrorHandler behavior in the Config table.Reflect the unified message in the docs.
Apply this diff:
-| ErrorHandler | `fiber.ErrorHandler` | ErrorHandler defines a function which is executed for an invalid key. By default a 401 response with a `WWW-Authenticate` challenge is sent. | Default error handler | +| ErrorHandler | `fiber.ErrorHandler` | ErrorHandler defines a function executed for an invalid key. By default, a 401 response is sent with `WWW-Authenticate` (when applicable) and body: `Invalid or expired API Key`. | Default error handler |middleware/keyauth/config.go (1)
56-59: Comment contradicts behavior; adjust for clarity.The comment says “Return default config if nothing provided” but the code panics when no config is provided. Update the comment to match behavior.
- // Return default config if nothing provided + // Panic if no config is provided (a Validator is required) if len(config) < 1 { panic("fiber: keyauth middleware requires a validator function") }middleware/keyauth/keyauth_test.go (1)
174-176: Remove redundant expectedBody override.Test cases for “no key” and “wrong key” already set expectedBody to the unified string. This branch is no longer needed and increases noise.
- if test.APIKey == "" || test.APIKey == "WRONGKEY" { - expectedBody = "Invalid or expired API Key" - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/middleware/keyauth.md(1 hunks)middleware/keyauth/config.go(1 hunks)middleware/keyauth/keyauth_test.go(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (2)
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
middleware/keyauth/config.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
🧬 Code Graph Analysis (1)
middleware/keyauth/config.go (2)
app.go (1)
ErrorHandler(57-57)ctx_interface_gen.go (1)
Ctx(17-405)
⏰ 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
🔇 Additional comments (2)
middleware/keyauth/config.go (1)
47-49: Unified default error message looks good.Simplified default ErrorHandler matches the PR goal and tests expecting “Invalid or expired API Key”.
middleware/keyauth/keyauth_test.go (1)
252-252: LGTM: Assertions aligned to the unified message.Updated expectations consistently assert “Invalid or expired API Key”, matching the simplified default behavior. The 404 special-case for missing param still holds.
Also applies to: 336-336, 359-359, 486-486, 615-615, 727-727, 744-744, 764-764, 783-783
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/middleware/keyauth.md (1)
66-69: Standardize “API key” casing in the example commentMinor consistency nit: use “API key” (no hyphen) everywhere.
-# No api-key specified -> 401 Invalid or expired API Key +# No API key specified -> 401 Invalid or expired API Key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/middleware/keyauth.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (1)
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
docs/middleware/keyauth.md
⏰ 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: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
docs/middleware/keyauth.md (2)
74-76: Example updates align with unified error responseThe example correctly reflects the new generic 401 message for invalid keys. Looks good.
274-276: Documentation is accurate: header set by middleware, not in the ErrorHandler override
- The middleware’s core logic (in
middleware/keyauth/keyauth.go) sets theWWW-Authenticateheader before callingcfg.ErrorHandler.ConfigDefault.ErrorHandleronly writes the status and body—exactly as shown in the “Default Config” snippet.- The Config table correctly states that a 401 with a
WWW-Authenticatechallenge is sent by default.ErrorHandleris still part of the public API and the snippet matchesConfigDefault.No updates to the docs are needed; please disregard the original suggestion.
Likely an incorrect or invalid review comment.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements a security enhancement by returning a generic error message for all KeyAuth failures, which prevents potential information leakage. The changes are well-contained, and both the documentation and tests have been updated to reflect this new behavior. I have one suggestion regarding the use of a hardcoded string for the new error message, which could be defined as a constant to improve maintainability.
sixcolors
left a comment
There was a problem hiding this comment.
Approved, but genini-code-assist comment "error message, which could be defined as a constant" makes sense to me.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
middleware/keyauth/config.go (1)
47-49: Good: default ErrorHandler now uses the canonical error constant.This addresses prior feedback about avoiding hardcoded strings and centralizes the failure response.
🧹 Nitpick comments (3)
middleware/keyauth/config.go (1)
22-22: Keep comments consistent with error string casing.After lowercasing the error string per ST1005, mirror it in the doc to reduce confusion.
Apply this diff:
-// Optional. Default: 401 Missing or invalid API Key +// Optional. Default: 401 missing or invalid API keymiddleware/keyauth/keyauth_test.go (2)
68-69: Avoid hardcoding the error message in tests; use the canonical error value.This keeps tests resilient to future message tweaks (and to the ST1005 casing fix).
Apply this diff:
- expectedCode: 401, - expectedBody: "Missing or invalid API Key", + expectedCode: 401, + expectedBody: ErrMissingOrMalformedAPIKey.Error(),
174-176: Replace hardcoded string override with the canonical error value.Prevents drift if the message changes and aligns with other tests using ErrMissingOrMalformedAPIKey.Error().
Apply this diff:
- if test.APIKey == "" || test.APIKey == "WRONGKEY" { - expectedBody = "Missing or invalid API Key" + if test.APIKey == "" || test.APIKey == "WRONGKEY" { + expectedBody = ErrMissingOrMalformedAPIKey.Error()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
docs/middleware/keyauth.md(2 hunks)middleware/keyauth/config.go(2 hunks)middleware/keyauth/extractors.go(9 hunks)middleware/keyauth/extractors_test.go(5 hunks)middleware/keyauth/keyauth.go(1 hunks)middleware/keyauth/keyauth_test.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/middleware/keyauth.md
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code Graph Analysis (4)
middleware/keyauth/extractors_test.go (2)
middleware/keyauth/keyauth.go (1)
ErrMissingOrMalformedAPIKey(20-20)middleware/keyauth/extractors.go (3)
FromQuery(177-189)FromHeader(154-166)FromCookie(85-97)
middleware/keyauth/extractors.go (1)
middleware/keyauth/keyauth.go (1)
ErrMissingOrMalformedAPIKey(20-20)
middleware/keyauth/keyauth_test.go (1)
middleware/keyauth/keyauth.go (1)
ErrMissingOrMalformedAPIKey(20-20)
middleware/keyauth/config.go (3)
app.go (2)
ErrorHandler(57-57)Error(60-63)constants.go (1)
StatusUnauthorized(74-74)middleware/keyauth/keyauth.go (1)
ErrMissingOrMalformedAPIKey(20-20)
🪛 GitHub Check: lint
middleware/keyauth/keyauth.go
[failure] 20-20:
ST1005: error strings should not be capitalized (stylecheck)
🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth.go
[error] 20-20: ST1005: error strings should not be capitalized (stylecheck)
⏰ 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: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (13)
middleware/keyauth/extractors.go (8)
54-66: Unified error for missing/malformed Authorization header looks correct.Returning a single sentinel error keeps behavior consistent and makes downstream handling simpler. The scheme check is case-insensitive and handles extra whitespace via TrimSpace, which is solid.
83-91: Cookie extractor now consistently returns the canonical error — LGTM.
106-114: Param extractor: consistent generic error on empty param — LGTM.
129-137: Form extractor: consistent generic error on missing field — LGTM.
152-160: Header extractor: consistent generic error on missing header — LGTM.
175-183: Query extractor: consistent generic error on missing query param — LGTM.
198-206: Chain: empty extractors now fail with the canonical error — LGTM.This removes ambiguity and aligns with the unified handling across the package.
233-237: Chain: when all extractors find nothing (return "", nil), defaulting to the canonical error is correct.This prevents silent success cases and keeps error semantics uniform.
middleware/keyauth/extractors_test.go (1)
22-22: Tests uniformly assert ErrMissingOrMalformedAPIKey across sources and chain — LGTM.Comparing against the sentinel error (not just strings) is robust and validates the unification intent well.
Also applies to: 34-34, 39-39, 44-44, 49-49, 54-54, 135-135, 159-159, 161-161, 174-174
middleware/keyauth/keyauth_test.go (4)
484-487: LGTM: default error body assertions use ErrMissingOrMalformedAPIKey.Error().These will automatically adapt to the new casing; no further action needed here.
613-616: LGTM: failure assertions correctly use the canonical error string.
685-688: LGTM: unified error handling path verified with canonical error message.
725-728: LGTM: header parsing edge cases covered; error body assertions use the canonical error string.Nice coverage for missing space, missing token, no separator, and whitespace-only token.
Also applies to: 743-745, 762-764, 782-784
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: dfbd15a | Previous: 9ed35c9 | Ratio |
|---|---|---|---|
Benchmark_Ctx_SendString_B |
15.48 ns/op 0 B/op 0 allocs/op |
9.962 ns/op 0 B/op 0 allocs/op |
1.55 |
Benchmark_Ctx_SendString_B - ns/op |
15.48 ns/op |
9.962 ns/op |
1.55 |
Benchmark_Compress_Levels_Parallel/Brotli_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
|
@ReneWerner87 @sixcolors Please re-review. |
Summary