diff --git a/apisix/init.lua b/apisix/init.lua index 57898b69a5fa..ecaa2f3dcf8c 100644 --- a/apisix/init.lua +++ b/apisix/init.lua @@ -556,13 +556,13 @@ end -- Forward the original path so servlet upstreams can consume params after ';'. -- URI-encode it before proxying to keep delimiters as path data. -- --- Setting $upstream_uri at all turns off the "empty proxy_pass URI" passthrough --- that otherwise hands the client's request line -- query string included -- to --- the upstream unchanged, so the query string has to be re-appended here. +-- We set api_ctx.var.upstream_uri early so early plugins (like ext-plugin, +-- graphql-limit-count) observe the servlet path parameters. +-- Query arguments are appended after rewrite and access plugin phases complete. local function set_servlet_upstream_uri(api_ctx, uri) - api_ctx.var.upstream_uri = core.utils.uri_safe_encode(uri) - .. (api_ctx.var.is_args or "") - .. (api_ctx.var.args or "") + local servlet_uri = core.utils.uri_safe_encode(uri) + api_ctx.var.upstream_uri = servlet_uri + api_ctx.servlet_upstream_uri = servlet_uri end @@ -1080,6 +1080,15 @@ function _M.http_access_phase() end span:finish(ngx_ctx) + if api_ctx.servlet_upstream_uri then + if api_ctx.var.upstream_uri == api_ctx.servlet_upstream_uri or + not api_ctx.var.upstream_uri or api_ctx.var.upstream_uri == "" then + api_ctx.var.upstream_uri = api_ctx.servlet_upstream_uri + .. (api_ctx.var.is_args or "") + .. (api_ctx.var.args or "") + end + end + _M.handle_upstream(api_ctx, route, enable_websocket) end diff --git a/apisix/plugins/data-mask.lua b/apisix/plugins/data-mask.lua index 3f49fee98fc0..fad35ca13541 100644 --- a/apisix/plugins/data-mask.lua +++ b/apisix/plugins/data-mask.lua @@ -22,6 +22,8 @@ local t_remove = table.remove local re_sub = ngx.re.sub local str_lower = string.lower local str_gsub = string.gsub +local str_find = string.find +local str_sub = string.sub local core = require("apisix.core") local jp = require("jsonpath") @@ -232,12 +234,27 @@ function _M.log(conf, ctx) local body_masked = false local masked_headers + local upstream_args + local upstream_path + if ctx.var.upstream_uri and ctx.var.upstream_uri ~= "" then + local idx = str_find(ctx.var.upstream_uri, "?", 1, true) + if idx then + upstream_path = str_sub(ctx.var.upstream_uri, 1, idx - 1) + local raw_query = str_sub(ctx.var.upstream_uri, idx + 1) + upstream_args = core.string.decode_args(raw_query) + end + end + local upstream_query_masked = false + if conf.request then for _, item in ipairs(conf.request) do if item.type == "query" then if mask_table(args, item) then query_masked = true end + if upstream_args and mask_table(upstream_args, item) then + upstream_query_masked = true + end end if item.type == "header" then @@ -327,7 +344,14 @@ function _M.log(conf, ctx) -- for access log ctx.var.request_line = core.request.get_method() .. " " .. ctx.var.request_uri .. " HTTP/" .. core.request.get_http_version() - -- TODO: handle upstream_uri in access log when enable proxy-rewrite + end + + if upstream_query_masked then + if next(upstream_args) then + ctx.var.upstream_uri = upstream_path .. "?" .. core.string.encode_args(upstream_args) + else + ctx.var.upstream_uri = upstream_path + end end if post_args_masked then diff --git a/apisix/plugins/proxy-mirror.lua b/apisix/plugins/proxy-mirror.lua index 8ffb534ec43d..342e3220e94d 100644 --- a/apisix/plugins/proxy-mirror.lua +++ b/apisix/plugins/proxy-mirror.lua @@ -91,8 +91,12 @@ end local function enable_mirror(ctx, conf) local uri = (ctx.var.upstream_uri and ctx.var.upstream_uri ~= "") and - ctx.var.upstream_uri or - ctx.var.uri .. ctx.var.is_args .. (ctx.var.args or '') + (ctx.servlet_upstream_uri and ctx.var.upstream_uri == ctx.servlet_upstream_uri and + (ctx.servlet_upstream_uri .. (ctx.var.is_args or "") .. (ctx.var.args or "")) or + ctx.var.upstream_uri) or + (ctx.servlet_upstream_uri and + (ctx.servlet_upstream_uri .. (ctx.var.is_args or "") .. (ctx.var.args or "")) or + (ctx.var.uri .. ctx.var.is_args .. (ctx.var.args or ""))) if conf.path then if conf.path_concat_mode == "prefix" then diff --git a/t/plugin/data-mask.t b/t/plugin/data-mask.t index 21d471015c88..65a33a198004 100644 --- a/t/plugin/data-mask.t +++ b/t/plugin/data-mask.t @@ -1489,3 +1489,108 @@ success } --- response_body success + + + +=== TEST 35: data-mask with proxy-rewrite preserves unrelated upstream parameters and repeated values in upstream_uri +--- 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, + [[{ + "plugins": { + "proxy-rewrite": { + "uri": "/echo?fixed=1" + }, + "data-mask": { + "request": [ + { + "action": "remove", + "name": "password", + "type": "query" + } + ] + }, + "file-logger": { + "path": "mask-proxy-rewrite-upstream.log", + "log_format": { + "upstream_uri": "$upstream_uri" + } + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/hello" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } + + + +=== TEST 36: verify upstream_uri preserves unrelated upstream parameters and repeated values +--- config + location /t { + content_by_lua_block { + local core = require("apisix.core") + local t = require("lib.test_admin").test + + t("/hello?password=secret&foo=1&tag=a&tag=b", ngx.HTTP_GET) + ngx.sleep(0.5) + + local fd, err = io.open("mask-proxy-rewrite-upstream.log", "r") + if not fd then + core.log.error("failed to open file: ", err) + return + end + local line = fd:read() + fd:close() + os.remove("mask-proxy-rewrite-upstream.log") + + local log = core.json.decode(line) + local idx = string.find(log.upstream_uri, "?", 1, true) + if not idx then + ngx.say("missing query in upstream_uri: ", log.upstream_uri) + return + end + + local path = string.sub(log.upstream_uri, 1, idx - 1) + if path ~= "/echo" then + ngx.say("unexpected path in upstream_uri: ", path) + return + end + + local args = core.string.decode_args(string.sub(log.upstream_uri, idx + 1)) + if args.password then + ngx.say("password was not masked in upstream_uri: ", log.upstream_uri) + return + end + if args.fixed ~= "1" then + ngx.say("fixed param missing in upstream_uri: ", log.upstream_uri) + return + end + if args.foo ~= "1" then + ngx.say("foo param missing in upstream_uri: ", log.upstream_uri) + return + end + if type(args.tag) ~= "table" or args.tag[1] ~= "a" or args.tag[2] ~= "b" then + ngx.say("repeated tag param missing or corrupt in upstream_uri: ", log.upstream_uri) + return + end + + ngx.say("success") + } + } +--- response_body +success diff --git a/t/router/servlet-upstream-uri.t b/t/router/servlet-upstream-uri.t index 0a5b3efe0608..7b0766490f9c 100644 --- a/t/router/servlet-upstream-uri.t +++ b/t/router/servlet-upstream-uri.t @@ -43,6 +43,7 @@ add_block_preprocessor(sub { content_by_lua_block { ngx.say("protected-handler") ngx.say("route-marker=", ngx.var.http_x_route_marker or "") + ngx.say("request-uri=", ngx.var.request_uri) } } @@ -53,6 +54,20 @@ add_block_preprocessor(sub { ngx.say("request-uri=", ngx.var.request_uri) } } + + location /early-plugin { + content_by_lua_block { + ngx.say("observed-upstream-uri=", ngx.var.http_x_observed_upstream_uri or "") + ngx.say("request-uri=", ngx.var.request_uri) + } + } + + location /data-mask { + content_by_lua_block { + ngx.say("data-mask-handler") + ngx.say("request-uri=", ngx.var.request_uri) + } + } _EOC_ } }); @@ -66,6 +81,23 @@ __DATA__ location /t { content_by_lua_block { local t = require("lib.test_admin").test + local code, body = t("/apisix/admin/consumers", + ngx.HTTP_PUT, + { + username = "jack", + plugins = { + ["key-auth"] = { + key = "secret-key", + }, + }, + } + ) + if code >= 300 then + ngx.status = code + ngx.say(body) + return + end + local routes = { { id = "protected-exact-path", @@ -73,7 +105,9 @@ __DATA__ priority = 100000, uri = "/anything", plugins = { - ["key-auth"] = {}, + ["key-auth"] = { + hide_credentials = true, + }, }, }, { @@ -93,6 +127,44 @@ __DATA__ }, }, }, + { + id = "early-plugin-test", + host = "servlet-uri.test", + priority = 100001, + uri = "/early-plugin/*", + plugins = { + ["serverless-pre-function"] = { + phase = "rewrite", + functions = { + "return function(conf, ctx) ngx.req.set_header('X-Observed-Upstream-Uri', ctx.var.upstream_uri or '') end", + }, + }, + }, + }, + { + id = "data-mask-test", + host = "servlet-uri.test", + priority = 100002, + uri = "/data-mask", + plugins = { + ["data-mask"] = { + request = { + { + action = "remove", + name = "password", + type = "query", + }, + }, + }, + ["file-logger"] = { + path = "mask-servlet-upstream.log", + log_format = { + request_line = "$request_line", + upstream_uri = "$upstream_uri", + }, + }, + }, + }, } for _, route in ipairs(routes) do @@ -101,7 +173,7 @@ __DATA__ type = "roundrobin", } - local code, body = t("/apisix/admin/routes/" .. route.id, + code, body = t("/apisix/admin/routes/" .. route.id, ngx.HTTP_PUT, route) if code >= 300 then ngx.status = code @@ -155,7 +227,135 @@ qr/^path-handler\nroute-marker=[a-f]{6}\nrequest-uri=\/anything\/sub;jsessionid= -=== TEST 5: delete routes +=== TEST 5: query modifications (key-auth hide_credentials) are preserved during servlet normalization +--- request +GET /anything;jsessionid=x?apikey=secret-key&foo=1 +--- more_headers +Host: servlet-uri.test +--- response_body eval +"path-handler\nroute-marker=\nrequest-uri=/anything;jsessionid=x?foo=1\n" +--- no_error_log +[error] + + + +=== TEST 6: when all query args are stripped by hide_credentials, no trailing question mark is sent upstream +--- request +GET /anything;jsessionid=x?apikey=secret-key +--- more_headers +Host: servlet-uri.test +--- response_body eval +"path-handler\nroute-marker=\nrequest-uri=/anything;jsessionid=x\n" +--- no_error_log +[error] + + + +=== TEST 7: early plugins (rewrite/access phase) observe servlet path parameters in upstream_uri +--- request +GET /early-plugin/test;jsessionid=x?foo=1 +--- more_headers +Host: servlet-uri.test +--- response_body eval +"observed-upstream-uri=/early-plugin/test;jsessionid=x\nrequest-uri=/early-plugin/test;jsessionid=x?foo=1\n" +--- no_error_log +[error] + + + +=== TEST 8: data-mask sanitizes upstream_uri and request_line when servlet normalization is on +--- request +GET /data-mask;jsessionid=x?password=secret +--- more_headers +Host: servlet-uri.test +--- response_body eval +"data-mask-handler\nrequest-uri=/data-mask;jsessionid=x?password=secret\n" +--- no_error_log +[error] + + + +=== TEST 9: verify log contains masked request_line and sanitized upstream_uri +--- config + location /t { + content_by_lua_block { + local core = require("apisix.core") + local fd, err = io.open("mask-servlet-upstream.log", "r") + if not fd then + core.log.error("failed to open file: ", err) + return + end + local line = fd:read() + fd:close() + os.remove("mask-servlet-upstream.log") + + local log = core.json.decode(line) + if log.request_line ~= "GET /data-mask HTTP/1.1" then + ngx.say("unexpected request_line: ", log.request_line) + return + end + if log.upstream_uri ~= "/data-mask;jsessionid=x" then + ngx.say("unexpected upstream_uri: ", log.upstream_uri) + return + end + + ngx.say("passed") + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 10: data-mask preserves remaining query args in upstream_uri during servlet normalization +--- request +GET /data-mask;jsessionid=x?password=secret&foo=1 +--- more_headers +Host: servlet-uri.test +--- response_body eval +"data-mask-handler\nrequest-uri=/data-mask;jsessionid=x?password=secret&foo=1\n" +--- no_error_log +[error] + + + +=== TEST 11: verify log preserves remaining query args in request_line and upstream_uri +--- config + location /t { + content_by_lua_block { + local core = require("apisix.core") + local fd, err = io.open("mask-servlet-upstream.log", "r") + if not fd then + core.log.error("failed to open file: ", err) + return + end + local line = fd:read() + fd:close() + os.remove("mask-servlet-upstream.log") + + local log = core.json.decode(line) + if log.request_line ~= "GET /data-mask?foo=1 HTTP/1.1" then + ngx.say("unexpected request_line: ", log.request_line) + return + end + if log.upstream_uri ~= "/data-mask;jsessionid=x?foo=1" then + ngx.say("unexpected upstream_uri: ", log.upstream_uri) + return + end + + ngx.say("passed") + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 12: delete routes and consumer --- config location /t { content_by_lua_block { @@ -163,6 +363,8 @@ qr/^path-handler\nroute-marker=[a-f]{6}\nrequest-uri=\/anything\/sub;jsessionid= local route_ids = { "protected-exact-path", "public-wildcard-fallback", + "early-plugin-test", + "data-mask-test", } for _, route_id in ipairs(route_ids) do @@ -175,6 +377,14 @@ qr/^path-handler\nroute-marker=[a-f]{6}\nrequest-uri=\/anything\/sub;jsessionid= end end + local code, body = t("/apisix/admin/consumers/jack", + ngx.HTTP_DELETE) + if code >= 300 then + ngx.status = code + ngx.say(body) + return + end + ngx.say("passed") } }