Copilot commented on code in PR #12909:
URL: https://github.com/apache/apisix/pull/12909#discussion_r2918684207
##########
apisix/plugins/key-auth.lua:
##########
@@ -105,14 +124,18 @@ function _M.rewrite(conf, ctx)
local consumer, consumer_conf, err = find_consumer(ctx, conf)
if not consumer then
if not conf.anonymous_consumer then
- core.response.set_header("WWW-Authenticate", "apikey realm=\"" ..
conf.realm .. "\"")
+ local auth_scheme = conf.bearer_token and "Bearer" or "apikey"
+ local www_auth_value = auth_scheme .. " realm=\"" .. conf.realm ..
"\""
+ core.response.set_header("WWW-Authenticate", www_auth_value)
Review Comment:
The `WWW-Authenticate` header construction is duplicated. Consider
extracting a small local helper (or a shared local variable computed once per
request) to build/set this header to reduce repetition and keep behavior
consistent if the header ever needs to change (e.g., adding bearer error
fields).
##########
apisix/plugins/key-auth.lua:
##########
@@ -81,6 +91,15 @@ local function find_consumer(ctx, conf)
return nil, nil, "Missing API key in request"
end
+ if conf.bearer_token and from_header then
+ local bearer_prefix = "Bearer "
+ if key:sub(1, #bearer_prefix) == bearer_prefix then
+ key = key:sub(#bearer_prefix + 1)
+ else
+ return nil, nil, "Invalid Bearer token format"
+ end
+ end
Review Comment:
The Bearer scheme comparison is case-sensitive and only matches exactly
`"Bearer "` with a single space, but RFC 6750 treats the auth scheme as
case-insensitive and real clients may send `"bearer"` / `"BEARER"` and/or
multiple spaces. Consider parsing the scheme case-insensitively and tolerating
one-or-more whitespace characters after the scheme.
##########
docs/en/latest/plugins/key-auth.md:
##########
@@ -54,9 +54,10 @@ For Route:
| Name | Type | Required | Default | Description
|
|--------|--------|-------------|-------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| header | string | False | apikey | The header to get the key from.
|
+| header | string | False | apikey | The header to get the key from. This
parameter is ignored when `bearer_token` is set to `true`.
|
Review Comment:
This line appears to include a large amount of trailing whitespace, which
can trip markdown linting/formatting checks and adds noise in diffs. Please
trim trailing spaces on this row (and any similar ones).
```suggestion
| header | string | False | apikey | The header to get the key from. This
parameter is ignored when `bearer_token` is set to `true`. |
```
##########
apisix/plugins/key-auth.lua:
##########
@@ -81,6 +91,15 @@ local function find_consumer(ctx, conf)
return nil, nil, "Missing API key in request"
end
+ if conf.bearer_token and from_header then
+ local bearer_prefix = "Bearer "
+ if key:sub(1, #bearer_prefix) == bearer_prefix then
+ key = key:sub(#bearer_prefix + 1)
+ else
+ return nil, nil, "Invalid Bearer token format"
+ end
Review Comment:
Bearer parsing introduces new edge-cases that aren’t covered by the added
tests (e.g., `Authorization: bearer <token>`, `Authorization: Bearer
<token>`, and `Authorization: Bearer` / `Authorization: Bearer ` with a missing
token). Adding test cases for these inputs would lock in RFC-aligned behavior
and prevent regressions.
```suggestion
-- Parse Bearer token in a case-insensitive and whitespace-tolerant
way.
-- Accept headers like:
-- Authorization: Bearer <token>
-- Authorization: bearer <token>
-- Authorization: Bearer <token>
-- but reject missing-token cases such as:
-- Authorization: Bearer
-- Authorization: Bearer .
local token = key:match("^%s*[Bb][Ee][Aa][Rr][Ee][Rr]%s+(%S.*)$")
if not token then
return nil, nil, "Invalid Bearer token format"
end
key = token
```
##########
apisix/plugins/key-auth.lua:
##########
@@ -105,14 +124,18 @@ function _M.rewrite(conf, ctx)
local consumer, consumer_conf, err = find_consumer(ctx, conf)
if not consumer then
if not conf.anonymous_consumer then
- core.response.set_header("WWW-Authenticate", "apikey realm=\"" ..
conf.realm .. "\"")
+ local auth_scheme = conf.bearer_token and "Bearer" or "apikey"
+ local www_auth_value = auth_scheme .. " realm=\"" .. conf.realm ..
"\""
+ core.response.set_header("WWW-Authenticate", www_auth_value)
return 401, { message = err}
end
consumer, consumer_conf, err =
consumer_mod.get_anonymous_consumer(conf.anonymous_consumer)
if not consumer then
err = "key-auth failed to authenticate the request, code: 401.
error: " .. err
core.log.error(err)
- core.response.set_header("WWW-Authenticate", "apikey realm=\"" ..
conf.realm .. "\"")
+ local auth_scheme = conf.bearer_token and "Bearer" or "apikey"
+ local www_auth_value = auth_scheme .. " realm=\"" .. conf.realm ..
"\""
+ core.response.set_header("WWW-Authenticate", www_auth_value)
Review Comment:
The `WWW-Authenticate` header construction is duplicated. Consider
extracting a small local helper (or a shared local variable computed once per
request) to build/set this header to reduce repetition and keep behavior
consistent if the header ever needs to change (e.g., adding bearer error
fields).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
