Baoyuantop commented on code in PR #13268:
URL: https://github.com/apache/apisix/pull/13268#discussion_r3122238734
##########
apisix/plugins/forward-auth.lua:
##########
@@ -184,14 +218,31 @@ function _M.access(conf, ctx)
return res.status, res.body
end
- -- set headers from the auth response, clearing any client-supplied values
- -- for configured headers not present in the auth response
+ local code, body = attach_consumer_by_header(conf, ctx, res)
+ if code or body then
+ return code, body
+ end
+
+ -- append headers that need to be get from the auth response header
for _, header in ipairs(conf.upstream_headers) do
local header_value = res.headers[header]
- -- if header_value is nil, the client header's value will be removed
if it exists
- core.request.set_header(ctx, header, header_value)
+ if header_value then
+ core.request.set_header(ctx, header, header_value)
Review Comment:
**`upstream_headers` security regression**: The original code intentionally
clears client-supplied headers when the auth response does not include a
configured upstream header (prevents header spoofing). The new `if header_value
then` guard in the loop breaks this behavior — a malicious client can now
inject headers that the upstream trusts (e.g., `X-User-Id`). Please revert to
the original unconditional `set_header` logic.
##########
apisix/plugins/forward-auth.lua:
##########
@@ -184,14 +218,31 @@ function _M.access(conf, ctx)
return res.status, res.body
end
- -- set headers from the auth response, clearing any client-supplied values
- -- for configured headers not present in the auth response
+ local code, body = attach_consumer_by_header(conf, ctx, res)
+ if code or body then
+ return code, body
+ end
+
+ -- append headers that need to be get from the auth response header
for _, header in ipairs(conf.upstream_headers) do
local header_value = res.headers[header]
- -- if header_value is nil, the client header's value will be removed
if it exists
- core.request.set_header(ctx, header, header_value)
+ if header_value then
+ core.request.set_header(ctx, header, header_value)
+ end
end
end
+function _M.rewrite(conf, ctx)
+ return do_auth(conf, ctx)
Review Comment:
**Moving auth to `rewrite` phase**: Running the full HTTP auth subrequest in
`rewrite` is unconventional for APISIX plugins. Please clarify: (a) Why can't
this be done in `access`? (b) How does this interact with other `rewrite`-phase
plugins like `proxy-rewrite`? (c) What happens when another auth plugin (e.g.,
`key-auth`) is also configured on the same route — does it overwrite the
Consumer set by forward-auth?
--
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]
