Skip to content

fix(router): preserve rewrite-phase query argument changes during servlet URI normalization #14040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ankit-cs wants to merge 2 commits into
base: master
Choose a base branch
from
+364 −12
Open

fix(router): preserve rewrite-phase query argument changes during servlet URI normalization #14040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
155d6a0
fix(router): preserve rewrite-phase query argument changes during ser…
Ankit-cs Oct 15, 2026
cfb07d4
Merge branch 'master' of https://github.com/apache/apisix into fix/pr…
Ankit-cs Oct 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions apisix/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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

@ruanweihong-aaa ruanweihong-aaa Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!
The equality check upstream_uri == servlet_upstream_uri cannot distinguish an untouched URI from one explicitly rewritten to the same path without a query string.

With normalize_uri_like_servlet: true and this configuration:
{
"proxy-rewrite": {
"use_real_request_uri_unsafe": true,
"regex_uri": ["^.*$", "/anything;jsessionid=x"]
}
}
For /anything;jsessionid=x?foo=1, proxy-rewrite explicitly sets the upstream URI to /anything;jsessionid=x. This condition evaluates to true, and the following concatenation re-appends ?foo=1 from var.is_args and var.args.

I verified this with real HTTP requests using the APISIX 3.19.0 container runtime with each source tree mounted:

  • Base 4a080d0: upstream receives /anything;jsessionid=x.
  • Head cfb07d4: upstream receives /anything;jsessionid=x?foo=1.
    Could you preserve the explicit plugin URI override and add a regression test for this case, while keeping the existing hide_credentials tests passing?

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 "")

@ruanweihong-aaa ruanweihong-aaa Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I compared base e14c6e93 with head c1fd74f6: query preservation and hide_credentials work as expected, but appending the query here introduces a data-mask interaction.

With normalize_uri_like_servlet enabled, configure data-mask to remove the password query parameter and log $request_line together with $upstream_uri. For /anything;jsessionid=x?password=secret, I get:

base: request_line="GET /anything HTTP/1.1" upstream_uri="/anything;jsessionid=x"
head: request_line="GET /anything HTTP/1.1" upstream_uri="/anything;jsessionid=x?password=secret"

data-mask does not sanitize $upstream_uri during its log phase, so the password remains in the log even when $request has been replaced with the masked $request_line. This is separate from the existing raw $request behavior.

Could you handle this field during log masking and add a regression test for this combination?

@Ankit-cs Ankit-cs Oct 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do it by today I did not get the eye on that

end
end

_M.handle_upstream(api_ctx, route, enable_websocket)
end

Expand Down
26 changes: 25 additions & 1 deletion apisix/plugins/data-mask.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions apisix/plugins/proxy-mirror.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@ruanweihong-aaa ruanweihong-aaa Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same equality-check issue as the final URI assembly in init.lua: it can re-append query arguments that proxy-rewrite explicitly removed.

With normalize_uri_like_servlet: true, this configuration makes rewriting run before mirroring:
{
"proxy-rewrite": {
"_meta": {
"priority": 1100
},
"use_real_request_uri_unsafe": true,
"regex_uri": ["^.*$", "/anything;jsessionid=x"]
},
"proxy-mirror": {
"host": "http://127.0.0.1:1981",
"sample_ratio": 1
}
}
For /anything;jsessionid=x?foo=1, I verified the actual URI received by the mirror service using the APISIX 3.19.0 container runtime with each source tree mounted:

  • Base 4a080d0: /anything;jsessionid=x.
  • Head cfb07d4: /anything;jsessionid=x?foo=1.
    With default priorities, mirroring runs before rewriting, and both versions retain the query; that is not a new regression. The regression above occurs when rewriting runs first.

Could you also preserve the explicit plugin URI override here and add a regression test for this execution order?

(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
Expand Down
105 changes: 105 additions & 0 deletions t/plugin/data-mask.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading
Loading