Skip to content

Conversation

@gaby
Copy link
Member

@gaby gaby commented May 27, 2025

Summary

  • make authorization header parsing RFC 6750 compliant
  • return standard WWW-Authenticate challenge when authentication fails
  • add optional Realm config
  • improve test-cases for the middleware
  • document Realm option

Copilot AI review requested due to automatic review settings May 27, 2025 01:38
@gaby gaby requested a review from a team as a code owner May 27, 2025 01:38
@gaby gaby requested review from ReneWerner87, efectn and sixcolors May 27, 2025 01:38
@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 27, 2025

Warning

Rate limit exceeded

@gaby has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 18 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 52756ed and cdc54d0.

📒 Files selected for processing (1)
  • middleware/keyauth/keyauth_test.go (2 hunks)

"""

Walkthrough

The keyauth middleware was updated to introduce a configurable Realm field for the WWW-Authenticate header, clarify and adjust the default error handling logic, and improve API key extraction from headers. Documentation was updated, and new tests were added to verify case-insensitive scheme handling and the default challenge response.

Changes

Files/Paths Change Summary
docs/middleware/keyauth.md Updated documentation to describe new Realm option, clarify ErrorHandler default, and adjust formatting.
middleware/keyauth/config.go Added Realm field to Config, set default ErrorHandler to nil, updated default error handler logic.
middleware/keyauth/keyauth.go Improved API key extraction in KeyFromHeader by trimming whitespace and enforcing scheme prefix checks.
middleware/keyauth/keyauth_test.go Added tests for case-insensitive scheme handling and default error handler challenge response behavior.
docs/whats_new.md Added KeyAuth middleware Realm configuration details to "What's New" documentation.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Middleware
    participant Validator

    Client->>Middleware: Sends request with Authorization header
    Middleware->>Middleware: Extracts key using KeyFromHeader (trims, checks scheme)
    Middleware->>Validator: Validates API key
    alt Key valid
        Middleware->>Client: Proceeds with request
    else Key invalid or missing
        Middleware->>Client: Responds 401, sets WWW-Authenticate: <scheme> realm="<Realm>"
    end
Loading

Suggested reviewers

  • sixcolors
  • efectn
  • ReneWerner87

Poem

In the meadow of code, a new realm appears,
With headers and schemes, it calms all our fears.
Now errors are clearer, and tests hop along,
With "Restricted" protection, the challenge is strong.
🐇✨
A bunny approves—your middleware’s robust,
For keys and for headers, in code we trust!
"""

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the keyauth middleware by making the authorization header parsing RFC 6750 compliant and improving error responses with a standard WWW-Authenticate challenge.

  • Implements case-insensitive header scheme comparison and robust whitespace handling in key parsing.
  • Adds an optional Realm configuration and updates the default error handler accordingly.
  • Updates relevant test cases and documentation to reflect these changes.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
middleware/keyauth/keyauth_test.go Added tests to verify case-insensitive handling and default challenge error responses.
middleware/keyauth/keyauth.go Improved header parsing to support RFC 6750, including trimming and case-insensitive checks.
middleware/keyauth/config.go Adjusted default configuration to include a configurable Realm and updated the error handler.
docs/middleware/keyauth.md Documentation updated to reflect the new behavior and configuration options.
Comments suppressed due to low confidence (1)

middleware/keyauth/config.go:90

  • [nitpick] The variable name 'localCfg' could be more descriptive to indicate that it's a configuration copy used in the error handler. Consider renaming it to something like 'cfgCopy' for improved clarity.
if localCfg.AuthScheme != "" {

@gaby gaby changed the title Improve keyauth middleware 🧹 chore: Improve keyauth middleware May 27, 2025
@codecov
Copy link

codecov bot commented May 27, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.78%. Comparing base (d756ec1) to head (8ff31cb).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
middleware/keyauth/keyauth.go 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3482      +/-   ##
==========================================
- Coverage   84.35%   83.78%   -0.58%     
==========================================
  Files         120      120              
  Lines       12190    12253      +63     
==========================================
- Hits        10283    10266      -17     
- Misses       1477     1562      +85     
+ Partials      430      425       -5     
Flag Coverage Δ
unittests 83.78% <90.90%> (-0.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
docs/middleware/keyauth.md (1)

220-220: Fix minor grammatical issue in ErrorHandler description.

-| 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. | `nil`  |
+| 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. | `nil`  |
🧰 Tools
🪛 LanguageTool

[uncategorized] ~220-~220: Did you mean: “By default,”?
Context: ...n which is executed for an invalid key. By default a 401 response with a `WWW-Authenticate...

(BY_DEFAULT_COMMA)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d756ec1 and f0b5978.

📒 Files selected for processing (4)
  • docs/middleware/keyauth.md (2 hunks)
  • middleware/keyauth/config.go (3 hunks)
  • middleware/keyauth/keyauth.go (1 hunks)
  • middleware/keyauth/keyauth_test.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
middleware/keyauth/config.go (2)
constants.go (3)
  • HeaderAuthorization (165-165)
  • HeaderWWWAuthenticate (168-168)
  • StatusUnauthorized (76-76)
middleware/keyauth/keyauth.go (1)
  • ErrMissingOrMalformedAPIKey (23-23)
🪛 LanguageTool
docs/middleware/keyauth.md

[uncategorized] ~220-~220: Did you mean: “By default,”?
Context: ...n which is executed for an invalid key. By default a 401 response with a `WWW-Authenticate...

(BY_DEFAULT_COMMA)

🪛 GitHub Check: codecov/patch
middleware/keyauth/keyauth.go

[warning] 141-142: middleware/keyauth/keyauth.go#L141-L142
Added lines #L141 - L142 were not covered by tests

middleware/keyauth/config.go

[warning] 96-96: middleware/keyauth/config.go#L96
Added line #L96 was not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: unit (1.23.x, macos-13)
  • GitHub Check: repeated
  • GitHub Check: Compare
🔇 Additional comments (5)
middleware/keyauth/config.go (1)

47-49: LGTM: Well-documented Realm field addition.

The new Realm field is properly documented and follows the existing configuration pattern with a sensible default value.

middleware/keyauth/keyauth_test.go (2)

618-638: Excellent test for case-insensitive scheme handling.

This test properly verifies that the middleware correctly handles authorization schemes in a case-insensitive manner (e.g., "bearer" vs "Bearer"), which is important for RFC 6750 compliance.


640-653: Well-implemented test for default error handler WWW-Authenticate behavior.

This test effectively validates that the default error handler:

  • Returns a 401 Unauthorized status
  • Sets the correct WWW-Authenticate header with the default realm
  • Uses a validator that always fails to properly trigger the error path

The test design is clean and focused on the specific functionality.

docs/middleware/keyauth.md (2)

224-224: Well-documented Realm configuration option.

The new Realm field is clearly documented with appropriate description and default value, making it easy for users to understand its purpose in WWW-Authenticate headers.


234-239: ConfigDefault example properly updated.

The default configuration example accurately reflects the new Realm field and the changed ErrorHandler default value, maintaining consistency with the actual implementation.

@gaby gaby changed the title 🧹 chore: Improve keyauth middleware 🧹 chore: Improve KeyAuth middleware compliance May 27, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (1)
middleware/keyauth/keyauth_test.go (1)

3-13: ⚠️ Potential issue

Fix missing import statement.

The test file is missing the errors package import, which is causing compilation failures across all CI environments.

Add the missing import:

 import (
 	"context"
+	"errors"
 	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
 
 	"github.com/gofiber/fiber/v3"
 	"github.com/stretchr/testify/require"
 )
🧹 Nitpick comments (2)
middleware/keyauth/keyauth_test.go (2)

656-662: Consider improving test readability with better formatting.

The indentation appears inconsistent compared to other test functions in the file. Consider standardizing the indentation to match the existing code style.

Apply this formatting fix:

 func Test_DefaultErrorHandlerGenericError(t *testing.T) {
-    app := fiber.New()
-    app.Use(New(Config{
-        AuthScheme: "Bearer",
-        Validator: func(_ fiber.Ctx, _ string) (bool, error) {
-            return false, errors.New("token expired")
-        },
-    }))
-    app.Get("/", func(c fiber.Ctx) error { return c.SendString("OK") })
+	app := fiber.New()
+	app.Use(New(Config{
+		AuthScheme: "Bearer",
+		Validator: func(_ fiber.Ctx, _ string) (bool, error) {
+			return false, errors.New("token expired")
+		},
+	}))
+	app.Get("/", func(c fiber.Ctx) error { return c.SendString("OK") })
🧰 Tools
🪛 GitHub Check: unit (1.23.x, macos-latest)

[failure] 660-660:
undefined: errors

🪛 GitHub Check: unit (1.23.x, ubuntu-latest)

[failure] 660-660:
undefined: errors


[failure] 660-660:
undefined: errors

🪛 GitHub Check: unit (1.23.x, macos-13)

[failure] 660-660:
undefined: errors

🪛 GitHub Check: lint

[failure] 660-660:
undefined: errors (typecheck)

🪛 GitHub Actions: golangci-lint

[error] 660-660: golangci-lint typecheck error: undefined 'errors' identifier


665-672: Improve test readability and error handling.

The remaining part of the test function also has inconsistent indentation and could benefit from proper error handling for the body reading operation.

Apply this formatting and error handling improvement:

-    req := httptest.NewRequest(fiber.MethodGet, "/", nil)
-    res, err := app.Test(req)
-    require.NoError(t, err)
-    require.Equal(t, http.StatusUnauthorized, res.StatusCode)
-    body, _ := io.ReadAll(res.Body)
-    require.Equal(t, "Invalid or expired API Key", string(body))
-    require.Equal(t, `Bearer realm="Restricted"`, res.Header.Get("WWW-Authenticate"))
+	req := httptest.NewRequest(fiber.MethodGet, "/", nil)
+	res, err := app.Test(req)
+	require.NoError(t, err)
+	require.Equal(t, http.StatusUnauthorized, res.StatusCode)
+	body, err := io.ReadAll(res.Body)
+	require.NoError(t, err)
+	require.Equal(t, "Invalid or expired API Key", string(body))
+	require.Equal(t, `Bearer realm="Restricted"`, res.Header.Get("WWW-Authenticate"))
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f0b5978 and 3ab85a4.

📒 Files selected for processing (1)
  • middleware/keyauth/keyauth_test.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
middleware/keyauth/keyauth_test.go (4)
middleware/keyauth/keyauth.go (2)
  • New (33-67)
  • ErrMissingOrMalformedAPIKey (23-23)
middleware/keyauth/config.go (1)
  • Config (13-50)
ctx_interface_gen.go (1)
  • Ctx (17-365)
constants.go (3)
  • MethodGet (5-5)
  • StatusOK (54-54)
  • StatusUnauthorized (76-76)
🪛 GitHub Check: unit (1.23.x, macos-latest)
middleware/keyauth/keyauth_test.go

[failure] 660-660:
undefined: errors

🪛 GitHub Check: unit (1.23.x, ubuntu-latest)
middleware/keyauth/keyauth_test.go

[failure] 660-660:
undefined: errors


[failure] 660-660:
undefined: errors

🪛 GitHub Check: unit (1.23.x, macos-13)
middleware/keyauth/keyauth_test.go

[failure] 660-660:
undefined: errors

🪛 GitHub Check: lint
middleware/keyauth/keyauth_test.go

[failure] 660-660:
undefined: errors (typecheck)

🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth_test.go

[error] 660-660: golangci-lint typecheck error: undefined 'errors' identifier

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Compare
  • GitHub Check: repeated
🔇 Additional comments (2)
middleware/keyauth/keyauth_test.go (2)

618-638: LGTM! Excellent test for case-insensitive scheme handling.

This test correctly verifies that the middleware treats the authorization header scheme case-insensitively, which aligns with RFC 6750 compliance mentioned in the PR objectives. The test uses "bearer" in lowercase and expects successful authentication.


640-653: Good test for WWW-Authenticate header verification.

This test properly verifies that the default error handler sets the correct WWW-Authenticate header when the standard ErrMissingOrMalformedAPIKey error is returned. The expected header value Bearer realm="Restricted" aligns with the new Realm configuration mentioned in the PR objectives.

github-actions[bot]

This comment was marked as off-topic.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ab85a4 and e3848c3.

📒 Files selected for processing (1)
  • middleware/keyauth/keyauth_test.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
middleware/keyauth/keyauth_test.go

[failure] 670-670:
Error return value of io.ReadAll is not checked (errcheck)


[failure] 657-657:
File is not properly formatted (gofmt)

🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth_test.go

[error] 657-657: File is not properly formatted (gofmt)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Compare
  • GitHub Check: repeated
🔇 Additional comments (3)
middleware/keyauth/keyauth_test.go (3)

5-5: LGTM! Import issue resolved.

The errors package import has been correctly added to fix the compilation error identified in previous reviews.


619-639: Excellent test for RFC 6750 compliance.

This test correctly verifies that the middleware handles authorization schemes case-insensitively by using "bearer" in lowercase, which aligns with the PR objective of improving RFC 6750 compliance.


641-654: Great test for WWW-Authenticate header validation.

This test correctly verifies that the default error handler returns the proper WWW-Authenticate challenge header with the expected format, which aligns with the PR objective of improving HTTP authentication protocol compliance.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e3848c3 and 51ef602.

📒 Files selected for processing (2)
  • middleware/keyauth/keyauth.go (1 hunks)
  • middleware/keyauth/keyauth_test.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
middleware/keyauth/keyauth.go

[warning] 148-149: middleware/keyauth/keyauth.go#L148-L149
Added lines #L148 - L149 were not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: unit (1.24.x, macos-latest)
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: Compare
🔇 Additional comments (7)
middleware/keyauth/keyauth.go (1)

127-152:

✅ Verification successful

Excellent RFC 6750 compliance improvements with minor test coverage gap.

The enhanced KeyFromHeader function significantly improves authorization header parsing:

Strengths:

  • Proper whitespace trimming prevents parsing issues
  • Case-insensitive scheme matching aligns with HTTP standards
  • Validates space/tab separator after scheme per RFC requirements
  • Clear error handling for various malformed header scenarios

⚠️ Test Coverage Gap:
The empty token validation logic (lines 148-149) isn't covered by existing tests.

Please add a test case for the empty token scenario to ensure complete coverage:


🏁 Script executed:

#!/bin/bash
# Description: Verify that the empty token case (lines 148-149) is covered by tests

# Search for existing tests that cover empty token after space
rg -A 10 -B 5 "Bearer\s+$|Token\s+$" middleware/keyauth/keyauth_test.go

# Check if Test_HeaderSchemeNoToken covers this case adequately
ast-grep --pattern 'func Test_HeaderSchemeNoToken($_) {
  $$$
}'

Length of output: 1366


All set – empty token validation is already covered by tests

The existing Test_HeaderSchemeNoToken in middleware/keyauth/keyauth_test.go (lines 723–730) explicitly tests the "Authorization: Bearer " scenario, ensuring lines 148–149 are exercised and returning ErrMissingOrMalformedAPIKey as expected.

No further action required.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 148-149: middleware/keyauth/keyauth.go#L148-L149
Added lines #L148 - L149 were not covered by tests

middleware/keyauth/keyauth_test.go (6)

5-5: Good addition of errors package import.

The import of the errors package is properly added to support the new test functions that use errors.New().


619-639: Excellent test for case-insensitive scheme handling.

This test correctly verifies that the authorization scheme matching is case-insensitive (using "bearer" instead of "Bearer"), which aligns with HTTP standards and the enhanced KeyFromHeader implementation.


641-654: Good test for default WWW-Authenticate challenge header.

This test verifies that the default error handler properly sets the WWW-Authenticate header with the realm "Restricted" when ErrMissingOrMalformedAPIKey occurs, which is essential for RFC 6750 compliance.


656-674: Well-structured test for generic error handling.

This test correctly validates that generic validation errors (non-ErrMissingOrMalformedAPIKey) still receive the appropriate WWW-Authenticate header and error message, ensuring consistent authentication challenge behavior.


676-696: Excellent coverage for multiple spaces handling.

This test verifies that the enhanced header parsing correctly handles multiple spaces between the scheme and token ("Bearer " + token), demonstrating the improved whitespace handling in the KeyFromHeader function.


698-713: Perfect test for missing space validation.

This test covers the important scenario where the scheme is immediately followed by the token without a space ("Bearer" + token), ensuring the validation logic properly rejects malformed headers. This addresses the past review comment about testing the space validation logic.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 51ef602 and 3b1c3d1.

📒 Files selected for processing (1)
  • middleware/keyauth/keyauth_test.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
middleware/keyauth/keyauth_test.go

[failure] 733-733:
File is not properly formatted (gofmt)

🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth_test.go

[error] 733-733: File is not properly formatted (gofmt)

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: unit (1.24.x, macos-13)
  • GitHub Check: unit (1.23.x, macos-latest)
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: unit (1.23.x, macos-13)
  • GitHub Check: repeated
  • GitHub Check: Compare
🔇 Additional comments (5)
middleware/keyauth/keyauth_test.go (5)

619-639: Excellent test for case-insensitive scheme handling.

This test properly validates RFC 6750 compliance by ensuring the middleware accepts lowercase "bearer" scheme, which is an important improvement for standard compliance.


641-654: Good validation of WWW-Authenticate header in default error handler.

This test confirms that the default error handler properly sets the WWW-Authenticate header with the expected realm, which aligns with RFC 6750 requirements for proper authentication challenges.


656-674: Comprehensive test for invalid API key error handling.

This test validates both the error message and WWW-Authenticate header for invalid keys, ensuring proper RFC 6750 compliance in error scenarios.


676-696: Well-designed test for multiple spaces handling.

This test ensures the middleware correctly handles multiple spaces between the scheme and token, demonstrating robust parsing logic.


698-730: Thorough coverage of malformed header scenarios.

These tests (Test_HeaderSchemeMissingSpace and Test_HeaderSchemeNoToken) properly validate edge cases where headers are malformed, ensuring the middleware correctly rejects invalid formats.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b1c3d1 and 542db23.

📒 Files selected for processing (1)
  • middleware/keyauth/keyauth_test.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: unit (1.24.x, ubuntu-latest)
middleware/keyauth/keyauth_test.go

[failure] 756-756:
expected statement, found ','

🪛 GitHub Check: unit (1.23.x, macos-13)
middleware/keyauth/keyauth_test.go

[failure] 756-756:
expected statement, found ','

🪛 GitHub Check: unit (1.23.x, ubuntu-latest)
middleware/keyauth/keyauth_test.go

[failure] 756-756:
expected statement, found ','


[failure] 756-756:
expected statement, found ','

🪛 GitHub Check: lint
middleware/keyauth/keyauth_test.go

[failure] 769-769:
missing ',' in composite literal (typecheck)


[failure] 758-758:
missing ',' in composite literal (typecheck)


[failure] 756-756:
expected statement, found ',' (typecheck)


[failure] 756-756:
syntax error: unexpected comma, expected } (typecheck)

🪛 GitHub Check: unit (1.23.x, macos-latest)
middleware/keyauth/keyauth_test.go

[failure] 756-756:
expected statement, found ','

🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth_test.go

[error] 756-756: syntax error: unexpected comma, expected } (typecheck)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Compare
  • GitHub Check: repeated
🔇 Additional comments (7)
middleware/keyauth/keyauth_test.go (7)

619-639: Good test for case-insensitive scheme handling.

This test correctly verifies that the middleware accepts both uppercase and lowercase schemes, which aligns with RFC standards for case-insensitive header scheme handling.


641-654: Excellent test for WWW-Authenticate challenge response.

This test validates the new default error handler behavior that sets the WWW-Authenticate header with the proper realm and scheme, ensuring RFC 6750 compliance.


656-674: Good test for generic error handling.

This test properly validates that generic validation errors are handled consistently, returning the standard error message while still setting the appropriate WWW-Authenticate header.


676-696: Good test for multiple spaces handling.

This test correctly verifies that the middleware handles multiple spaces between the scheme and token, ensuring robust parsing of Authorization headers.


698-713: Good test for missing separator detection.

This test correctly validates that headers without a space separator between the scheme and token are properly rejected with the appropriate error.


715-730: Good test for empty token detection.

This test correctly validates that headers with only a scheme followed by whitespace are properly rejected. This ensures the middleware doesn't accept empty tokens.


732-749: Good test for malformed header detection.

This test correctly validates that headers with no separator between scheme and token are properly rejected, addressing the coverage gap identified in previous reviews.

@gaby gaby changed the title 🧹 chore: Improve KeyAuth middleware compliance 🧹 chore: Improve KeyAuth middleware RFC 6750 compliance May 27, 2025
@gaby gaby added this to v3 May 27, 2025
@gaby gaby moved this to In Progress in v3 May 27, 2025
@gaby gaby added this to the v3 milestone May 27, 2025
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@gaby

  • add this to Whatsnew.md

@gaby gaby changed the title 🧹 chore: Improve KeyAuth middleware RFC 6750 compliance 🧹 chore: Enhance KeyAuth middleware to better comply with RFC 6750 May 27, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 542db23 and 921857d.

📒 Files selected for processing (3)
  • docs/whats_new.md (2 hunks)
  • middleware/keyauth/keyauth.go (1 hunks)
  • middleware/keyauth/keyauth_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • middleware/keyauth/keyauth_test.go
🧰 Additional context used
🪛 GitHub Check: unit (1.24.x, macos-13)
middleware/keyauth/keyauth.go

[failure] 137-137:
undefined: utils


[failure] 127-127:
undefined: utils

🪛 GitHub Check: unit (1.24.x, ubuntu-latest)
middleware/keyauth/keyauth.go

[failure] 137-137:
undefined: utils


[failure] 127-127:
undefined: utils

🪛 GitHub Check: lint
middleware/keyauth/keyauth.go

[failure] 137-137:
undefined: utils (typecheck)


[failure] 127-127:
undefined: utils

🪛 GitHub Check: govulncheck-check
middleware/keyauth/keyauth.go

[failure] 137-137:
undefined: utils


[failure] 127-127:
undefined: utils

🪛 GitHub Actions: Run govulncheck
middleware/keyauth/keyauth.go

[error] 127-127: Undefined identifier 'utils' at line 127:11.

🪛 GitHub Actions: golangci-lint
middleware/keyauth/keyauth.go

[error] 127-127: golangci-lint error: undefined identifier 'utils' at line 127, column 11

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Compare
  • GitHub Check: repeated
  • GitHub Check: unit (1.24.x, macos-latest)
🔇 Additional comments (2)
docs/whats_new.md (2)

39-39: LGTM!

Good addition of KeyAuth to the middleware list for proper navigation.


1024-1027: LGTM!

The documentation accurately describes the KeyAuth middleware enhancement with the configurable Realm field for the WWW-Authenticate header. The content is concise and appropriate for the "What's New" document.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 921857d and 52756ed.

📒 Files selected for processing (1)
  • middleware/keyauth/keyauth.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
middleware/keyauth/keyauth.go

[warning] 149-150: middleware/keyauth/keyauth.go#L149-L150
Added lines #L149 - L150 were not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Compare
  • GitHub Check: unit (1.24.x, windows-latest)
  • GitHub Check: repeated
🔇 Additional comments (1)
middleware/keyauth/keyauth.go (1)

11-11: Import addition resolves compilation errors.

Good fix! The utils package import addresses the compilation errors mentioned in previous reviews.

@ReneWerner87 ReneWerner87 merged commit 804a2b9 into main May 27, 2025
16 of 19 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in v3 May 27, 2025
@gaby gaby deleted the codex/refactor-keyauth-middleware-for-rfc-compliance branch May 27, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants