shreemaan-abhishek commented on code in PR #13468:
URL: https://github.com/apache/apisix/pull/13468#discussion_r3371976502
##########
apisix/plugins/key-auth.lua:
##########
@@ -108,6 +108,21 @@ function _M.rewrite(conf, ctx)
core.response.set_header("WWW-Authenticate", "apikey realm=\"" ..
conf.realm .. "\"")
return 401, { message = err}
end
+ -- Strip credentials before falling back to the anonymous consumer when
+ -- hide_credentials is enabled. find_consumer() only strips on the
+ -- successful-auth path, so without this an invalid credential would be
+ -- forwarded upstream during anonymous fallback. A request may carry
the
+ -- credential in both the header and the query string, so clean up
both.
+ if conf.hide_credentials then
+ if core.request.header(ctx, conf.header) then
+ core.request.set_header(ctx, conf.header, nil)
+ end
+ local args = core.request.get_uri_args(ctx) or {}
+ if args[conf.query] then
+ args[conf.query] = nil
+ core.request.set_uri_args(ctx, args)
+ end
+ end
Review Comment:
Thanks @nic-6443. The two blocks look duplicated but they intentionally
implement different behaviors, so collapsing them into one place would change
documented behavior.
- `find_consumer` (auth succeeded) strips **only** the credential location
that was actually used (`from_header ? header : query`) and leaves the other
location untouched. This is asserted by `key-auth.t` TEST 19 ("when apikey both
in header and query string, verify apikey request header is hidden but request
args is NOT hidden") and TEST 23 (the mirror for the query case).
- The `rewrite` anonymous-fallback path strips **both** locations, because
auth failed: there is no single "used" credential, and the request may carry a
credential-looking value in both the header and the query, either of which
would otherwise be forwarded upstream under the anonymous identity (the leak
this PR closes).
Concretely, for `GET /echo?apikey=Q` with header `apikey: H` and
`hide_credentials=true`:
- success (`H` valid): strip header, keep `?apikey=Q` (TEST 19);
- fallback (`H` invalid): strip both.
Same request, opposite handling of the query location. Moving the logic to
the end of `rewrite` and stripping both in one place would drop `?apikey=Q` on
the success path and break TEST 19; conversely, stripping only the used
location everywhere would re-introduce the leak on the fallback path. So a
single shared block cannot satisfy both without a deliberate behavior change. I
am keeping the two paths separate for that reason.
--
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]
