Copilot commented on code in PR #13787:
URL: https://github.com/apache/apisix/pull/13787#discussion_r3733270805
##########
t/plugin/proxy-rewrite.t:
##########
@@ -1724,3 +1724,38 @@ GET /test/echo
x-src: from-src
--- response_body_like eval
qr/x-multi: cap-echo\nx-multi: from-src/
+
+
+
+=== TEST 66: CRLF in a reflected uri is encoded, not injected into the
upstream request line
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_PUT, {
+ uri = "/reflect*",
+ plugins = {
+ ["proxy-rewrite"] = {
+ regex_uri = {"^(/reflect.*)",
"/print_request_received?orig=$1"}
+ }
+ },
+ upstream = {
+ type = "roundrobin",
+ nodes = {["127.0.0.1:1980"] = 1}
+ }
+ })
+ if code >= 300 then ngx.status = code; ngx.say(body); return end
+
+ local http = require("resty.http")
+ local httpc = http.new()
+ local res = httpc:request_uri("http://127.0.0.1:"; ..
ngx.var.server_port
+ .. "/reflect%0d%0aX-Injected:pwn")
+ ngx.print(res.body)
+ }
+ }
+--- request
+GET /t
+--- response_body eval
+qr{request_uri: /print_request_received\?orig=/reflect%0[Dd]%0[Aa]X-Injected}
+--- no_error_log
+[error]
Review Comment:
The test verifies that CR/LF is percent-encoded in the upstream
`request_uri`, but it doesn’t assert that the CR/LF wasn’t interpreted as a
header boundary by the upstream (i.e., that no injected `x-injected` header
line was received). Adding a negative assertion makes the test robust against
alternative upstream parsing/printing behaviors.
##########
apisix/init.lua:
##########
@@ -813,8 +813,9 @@ function _M.http_access_phase()
api_ctx.var.uri = new_uri
-- forward the original uri so the servlet upstream
- -- can consume the param after ';'
- api_ctx.var.upstream_uri = uri
+ -- can consume the param after ';'. Encode control characters so a
+ -- CR/LF in the decoded uri cannot inject into the upstream
request line.
+ api_ctx.var.upstream_uri = core.utils.escape_uri_control_chars(uri)
end
Review Comment:
This change also alters the `normalize_uri_like_servlet` path by escaping
control characters before assigning `api_ctx.var.upstream_uri`, but there’s no
accompanying test to prevent regressions on that code path. Consider adding a
test case (similar to TEST 66) with `normalize_uri_like_servlet: true` that
reflects `%0d%0a` from `$uri` into the upstream request and asserts it is
encoded and not injected.
--
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]
