HY-love-sleep opened a new pull request, #7534:
URL: https://github.com/apache/shenyu/pull/7534
Fixes #7459
### Problem
`AiRequestTransformerPlugin.rewriteRequestPath()` extracted the generated
request target and
validated it as a whole against the path character set:
if (!newPath.matches("^/[a-zA-Z0-9/_\\-]*$"))
An origin-form target may carry a query string, so a legitimate model answer
such as
`POST /rewritten?new=2 HTTP/1.1` failed this check. The rewrite was dropped
with a warning
while the generated body and headers were still applied, leaving the
downstream URI on the
original path and query.
### Fix
- Split the extracted target into its path and query components at the first
`?`.
- Validate the path with the existing rule (unchanged) and the query against
RFC 3986 query
syntax, requiring well-formed percent-encoding (`%` + two hex digits) so
the target is
always a legal URI reference for `URI.resolve`.
- Apply both components to the request URI (`originalUri.resolve(path + "?"
+ query)`). A
target without `?` behaves exactly as before.
- Fail-safe behaviour is unchanged: if either component is invalid, the
whole rewrite is
skipped and the original target is kept.
### Not in this PR
- **Routing-context synchronisation.** The mutated request URI still
diverges from
`ShenyuContext.path/realUrl`, which `RequestUrlUtils.buildRequestUri()`
uses to build the
outbound request -- that is #7460, kept out on purpose. The in-repo
mechanism for it is
`Constants.REWRITE_URI` (set today by `RewritePlugin`, not by this
plugin), and it changes
forwarding behaviour rather than parsing, so it deserves its own review.
- **Partial application.** I kept the existing all-or-nothing semantics
(invalid generated
component -> keep the original target). If per-component fallback is
preferred, I can change
it.
### Verification
./mvnw -pl
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-request-transformer -am \
-Djacoco.skip=true -Dtest=AiRequestTransformerPluginTest \
-DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false test
- With this change: `Tests run: 11, Failures: 0, Errors: 0, Skipped: 0`
- Same test class against unpatched `380afb559`: `Tests run: 11, Failures:
2` -- both
query-string cases fail with `expected: </rewritten> but was: </original>`
Three tests were added; all of them drive `doExecute()` end to end with a
mocked model answer
and assert the URI handed to the plugin chain: a query-bearing target is
accepted, a
percent-encoded query is preserved, and a malformed query (`%zz`) keeps the
original target.
The module's surefire is not skipped, so these counts are real test
execution.
### Checklist
- [x] I have read the [contribution
guidelines](https://shenyu.apache.org/community/contributor-guide).
- [x] I submit test cases (unit or integration tests) that back your changes.
- [x] My local test passed `./mvnw clean install -Dmaven.javadoc.skip=true`.
--
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]
