fix(proxy-rewrite): query string discarded when use_real_request_uri_…#12843
Merged
AlinsRan merged 10 commits intoJul 8, 2026
Merged
Conversation
…unsafe is true When use_real_request_uri_unsafe is set to true, url that was requested with query string, arguments is not parsed. Expected to parse the arguments just like same default behavior when use_real_request_uri_unsafe is set to false. The reason this problem exist is because `if ctx.var.is_args == "?" then` statement is inside `if not conf.use_real_request_uri_unsafe then` which then bypasses args parsing.
dosubot
Bot
added
size:M
Baoyuantop
reviewed
Dec 26, 2025
Baoyuantop
left a comment
Baoyuantop
left a comment
Contributor
There was a problem hiding this comment.
If the plugin only configures "use_real_request_uri_unsafe": true, it will cause the query to be concatenated repeatedly.
Baoyuantop
added
wait for update
Contributor
Author
|
Hi @Baoyuantop alright I think I understand what you mean. I think it is better for me to debug it line by line in this case. For your side normally how do you run debugging/printing code output? |
The real issue lies in the fact that when use_real_request_uri_unsafe is set to true and conf.uri is provided, the core.utils.resolve_var output back an upstream uri that does not include query string. Therefore we need to concatenate the query string for the case if conf.uri is true. The previous commit does not solve the issue as the issue is not related in the `if not conf.use_real_request_uri_unsafe then` statement.
dosubot
Bot
added
size:L
Baoyuantop
added
awaiting review
and removed
wait for update
Baoyuantop
reviewed
Jan 26, 2026
Baoyuantop
added
wait for update
There was a problem hiding this comment.
Pull request overview
Fixes proxy-rewrite behavior where the original request query string is dropped when use_real_request_uri_unsafe=true and conf.uri is set, aligning unsafe mode with the default (safe) behavior.
Changes:
- Update
proxy-rewriterewrite logic to preserve the query string whenuse_real_request_uri_unsafe=trueanduriis configured. - Add regression tests covering query preservation in unsafe mode.
- Extend/adjust tests around URI normalization behavior when unsafe mode is disabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apisix/plugins/proxy-rewrite.lua | Appends the original query string when building upstream_uri from conf.uri in unsafe mode. |
| t/plugin/proxy-rewrite.t | Adds tests to verify query args are preserved when use_real_request_uri_unsafe=true and uri is rewritten. |
| t/plugin/proxy-rewrite3.t | Updates test descriptions and adds coverage for normalization behavior when unsafe mode is not enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Hi @YapWC, following up on the previous review comments. Please let us know if you have any updates. Thank you. |
moonming
requested changes
Mar 16, 2026
moonming
left a comment
moonming
left a comment
Member
There was a problem hiding this comment.
Hi @YapWC, thank you for fixing the query string issue in proxy-rewrite!
Query strings being discarded when using use_real_request_uri is a real bug that affects production use.
Please:
- Rebase on the latest master
- Add edge case tests: empty query strings, query strings with special characters, multiple
?in the URI - Verify the fix works correctly with both
use_real_request_uri_unsafevariants
Looking forward to the update!
Contributor
|
Hi @YapWC, following up on the previous review comments. Please let us know if you have any updates. Thank you. |
nic-6443
reviewed
Jun 11, 2026
Address review feedback: when conf.uri already carries its own query string, merge the original query with '&' instead of concatenating a second '?'. Simplify the added comments.
…e '?' and both variants Address review feedback on the query-string fix: - conf.uri carrying its own query merged with '&' (no double '?') - use_real_request_uri_unsafe only (no conf.uri): query reaches upstream once - query with special characters and multiple '?' passed through unchanged - same conf.uri with use_real_request_uri_unsafe disabled, verifying both variants keep the query string consistently
Fix non-sequential TEST numbers (45/46 -> 42/43) so the reindex lint check passes.
shreemaan-abhishek
approved these changes
Jul 8, 2026
nic-6443
approved these changes
Jul 8, 2026
membphis
approved these changes
Jul 8, 2026
moonming
approved these changes
Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…unsafe is true
When use_real_request_uri_unsafe is set to true, url that was requested with query string, arguments is not parsed. Expected to parse the arguments just like same default behavior when use_real_request_uri_unsafe is set to false.
The real issue lies in the fact that when use_real_request_uri_unsafe is set to true and conf.uri is provided, the core.utils.resolve_var output back an upstream uri that does not include query string. Therefore we need to concatenate the query string for the case if conf.uri is true.
Description
Which issue(s) this PR fixes:
Fixes #12623
Checklist