[Ankit-cs]

Description

Background & Problem:

When apisix.normalize_uri_like_servlet is enabled, $upstream_uri was materialized before route rewrite and access plugins executed.

Some plugins modify the request arguments dynamically during these later phases. For example, key-auth and jwt-auth remove query credentials via core.request.set_uri_args() when hide_credentials: true is configured. Because $upstream_uri already contained a snapshot of the earlier query string, those query modifications were not reflected in the URI forwarded upstream, causing credentials to leak and query argument changes to be lost.

Solution:

1. Store the normalized servlet path separately in api_ctx.servlet_upstream_uri instead of eagerly writing to api_ctx.var.upstream_uri.
2. Assemble $upstream_uri with the updated $is_args$args after the rewrite and access plugin phases have completed.
3. Respect plugin precedence: if a plugin like proxy-rewrite explicitly sets api_ctx.var.upstream_uri, do not overwrite it.
4. Updated proxy-mirror to also respect servlet_upstream_uri.
5. Added regression test cases in t/router/servlet-upstream-uri.t covering:
   • apisix.normalize_uri_like_servlet: true
   • query-based key-auth with hide_credentials: true
   • unrelated query arguments remaining intact
   • clean URI with no trailing ? when all arguments are stripped.

Which issue(s) this PR fixes:

Fixes #13983

Checklist

• I have explained the need for this PR and the problem it solves
• I have explained the changes or the new features added to this PR
• I have added tests corresponding to this change
• I have updated the documentation to reflect this change (Documentation unchanged: this is a bug fix restoring expected behavior)
• I have verified that this change is backward compatible

[Ankit-cs]

as i am learning lua for the first time so that is the reason i got late in solving the bug @Baoyuantop sir and looking for feedback also.

[Baoyuantop]

Early plugins lose servlet path parameters

With servlet normalization enabled, upstream_uri stays empty during rewrite and access plugins. The external pre-request runner sees a stripped path, and GraphQL introspection can target a different endpoint than the forwarded request.

Example: For /anything;jsessionid=x, the pre-request runner now receives /anything; the upstream receives /anything;jsessionid=x. A GraphQL endpoint available only at the latter path fails schema discovery at the former path.

[Ankit-cs]

OK @Baoyuantop i will solve this

[Ankit-cs]

Hi @Baoyuantop,looking for another feedback!

I've updated the PR so that api_ctx.var.upstream_uri is populated early with the safe-encoded servlet path (/anything;jsessionid=x), allowing early plugins like ext-plugin and graphql-limit-count to observe the servlet path parameters. The final query string ($is_args$args) is then synchronized after rewrite and access phases finish to preserve query modifications (e.g. hide_credentials).

I also added a regression test in t/router/servlet-upstream-uri.t covering this scenario.

[Ankit-cs]

Hi @Baoyuantop sir can you look a minute for my pr it will be really great if you provide some feedback

[Ankit-cs]

hey @Baoyuantop sir can you please look for the above pr and give me feedback for it

[ruanweihong-aaa]

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]

Ok @ruanweihong-aaa sir i will look for that and update the pr

[Ankit-cs]

Hi @ruanweihong-aaa,
I have updated the PR to handle $upstream_uri during data-mask's log phase:

1. data-mask.lua: When query_masked is active in _M.log, we now inspect ctx.var.upstream_uri. If present with query parameters, we re-serialize upstream_uri using the sanitized arguments (or strip the query delimiter ? completely if all query arguments were masked).
2. Regression Test: Added tests 8–11 in t/router/servlet-upstream-uri.t verifying that with normalize_uri_like_servlet: true, data-mask sanitizes both $request_line and $upstream_uri in the log output for both single and multiple query parameters.

Could you please take another look?

[ruanweihong-aaa]

Thanks for the update! The original masking case is fixed in targeted HTTP integration tests against 5b8b4739. Two issues remain:

1. apisix/plugins/data-mask.lua:336–340:

if next(args) then
    ctx.var.upstream_uri = upstream_path .. "?" .. core.string.encode_args(args)
else
    ctx.var.upstream_uri = upstream_path
end

This replaces the upstream query with the masked request args, dropping parameters added by proxy-rewrite. With uri: /rewritten?fixed=1 and client query password=secret&foo=1, the backend receives /rewritten?fixed=1&password=secret&foo=1, but the log contains /rewritten?foo=1. Please preserve unrelated upstream parameters, including repeated values. This affects logging, not the forwarded request.

2. t/router/servlet-upstream-uri.t:148 configures:

uri = "/data-mask/*",

But TEST 8 and TEST 10 (lines 268 and 314) request:

GET /data-mask;jsessionid=x?password=secret
GET /data-mask;jsessionid=x?password=secret&foo=1

These normalize to /data-mask, miss /data-mask/*, and hit the fallback. With the same routing configuration, both requests returned 200 but produced no file-logger output; /data-mask/test;jsessionid=x matched the intended route and produced masked logs. Please use an exact /data-mask route or update the requests and assertions to use a child path.

I verified these results with targeted HTTP integration checks comparing c1fd74f6 and 5b8b4739, beyond the earlier function-level checks. I have not run the complete Test::Nginx suite.

[Ankit-cs]

Hi @ruanweihong-aaa ,
I have updated the PR to resolve both issues:

1. Preserve unrelated upstream parameters in data-mask (apisix/plugins/data-mask.lua):

   • We now parse and decode the query string directly from ctx.var.upstream_uri via core.string.decode_args(), rather than overwriting it with client request arguments.
   • Masking rules are applied to these upstream arguments, ensuring unrelated parameters (such as fixed=1 added by proxy-rewrite) and repeated values (tag=a&tag=b) are fully preserved in $upstream_uri logs.
   • Added regression tests in t/plugin/data-mask.t (TEST 35 & 36) verifying that proxy-rewrite parameters and repeated values are preserved while sensitive query arguments are sanitized.

2. Fix route matching in t/router/servlet-upstream-uri.t:

   • Changed the data-mask-test route from uri = "/data-mask/*" to the exact path uri = "/data-mask".
   • Requests to /data-mask;jsessionid=x now accurately match the intended route instead of falling back to /*, properly triggering data-mask and file-logger.

Could you please take another look when you have a moment?

[hanzhenfang]

Hi @Ankit-cs , Could you merge the latest master into your PR branch, resolve the conflicts, and push the updated branch? This will help us continue the review.

[Ankit-cs]

OK @hanzhenfang sir .Please take another look when you have time, thanks!