Neymar-10-JR commented on PR #7823:
URL: https://github.com/apache/shenyu/pull/7823#issuecomment-6260721314
> The encoding work is good — genuinely good — but there is one behavioural
regression and one hardening gap I would like addressed before this lands.
>
> ## What is well done
> * Using `FormHttpMessageWriter` with a `LinkedMultiValueMap` gets
charset-aware encoding and repeated fields for free, which is exactly what the
old string passthrough could not do.
> * Pinning the multipart boundary in
`RequestMultipartWriter.generateMultipartBoundary()` is the subtle fix that
makes this correct at all: the default `MultipartHttpMessageWriter` generates
the boundary during `write(...)`, so a caller that reads `getHeaders()` before
subscribing to the body would see a `Content-Type` boundary that does not match
the body. Pinning it in the constructor and setting it on
`cachedBodyOutputMessage` up front is the right resolution.
> * `bodyInsertion.thenMany(Flux.defer(cachedBodyOutputMessage::getBody))`
fixes an error-handling hole in the previous code: `.insert(...).subscribe()`
in the constructor detached the write from the subscriber, so any failure in
the writer was emitted into the void instead of failing the request.
> * `FILE_FIELDS.containsAll(file.keySet())` rejecting unknown keys, plus
requiring a concrete `contentType` with no line breaks, is strict in the right
direction. The explicit `{"filename","content","contentType"}` file
representation is a reasonable contract and the docs update covers it.
>
> ## 1. Content types that used to pass through now throw
> The old implementation ended with a passthrough:
>
> ```java
> private String getFormattedBody() {
> if (Objects.isNull(contentType)) { return body; }
> if (contentType.contains("application/json")) { return body; }
> else if (contentType.contains("application/x-www-form-urlencoded")) {
return body; }
> else if (contentType.contains("multipart/form-data")) { return body; }
> else { return body; } // <-- any other type was forwarded
verbatim
> }
> ```
>
> The new implementation rejects everything outside the three supported
families:
>
> ```java
> } else {
> throw new IllegalArgumentException("Unsupported MCP request body
Content-Type");
> }
> ```
>
> So a tool template configured with `text/plain`, `application/xml`,
`application/graphql` or a vendor type now fails at request time where it
previously forwarded the body unchanged. The PR description only mentions
“retaining JSON as the default”, not narrowing the accepted set.
>
> If the template schema genuinely only allows JSON / form / multipart, then
throwing is correct and this should be stated as a behaviour change (and
ideally validated when the tool template is saved, not on the first call). If
arbitrary types were reachable, please keep a raw passthrough branch for them —
the encoding work does not need that door closed.
>
> ## 2. Part names are validated for CRLF but not for quoting
> ```java
> private void validatePartName(final String name) {
> if (name.isBlank() || name.contains("\r") || name.contains("\n")) {
> throw new IllegalArgumentException("Multipart part names and
filenames must be nonblank and contain no line breaks");
> }
> }
> ```
>
> Blocking CR/LF is the critical part and it is present. But a part name or
filename is emitted inside a quoted parameter — `Content-Disposition:
form-data; name="<name>"` — and these values come straight from **tool
arguments**, i.e. from the MCP caller. A name containing a double quote can
break out of the quoted string and introduce additional parameters, for example
an attacker-supplied `filename`:
>
> ```
> name = a; filename=payload.sh
> ```
>
> That does not corrupt the gateway’s own HTTP framing (it stays inside the
multipart body), but it does hand the downstream service a
`Content-Disposition` it did not construct, which matters for any service that
persists or names uploaded files. Since you already have an exact allow-list
pattern for tool names in the JWT work (`[a-zA-Z0-9_.-]+`), applying something
similar here — or at least rejecting `"` and `\` — would close it. The same
applies to `filename`.
>
> ## Minor notes
> * `bodyJson.isEmpty() ? "" : bodyJson.toString()` changes behaviour for a
parameterless tool: a JSON endpoint that previously received `{}` now receives
an empty body. Worth confirming that is intended; some strict JSON endpoints
reject an empty body.
> * `resolveBody` lets Gson’s `JsonSyntaxException` escape for a malformed
body while every other failure path raises a descriptive
`IllegalArgumentException`. Wrapping it would keep the error contract uniform.
>
> The test coverage is substantial (`ShenyuToolCallbackBodyTest`,
`BodyWriterExchangeTest`), and covering both the form and multipart writers
plus the file representation is what makes a rewrite of this size reviewable.
Thanks for the review. I have addressed both requested changes and the minor
notes:
- Restored passthrough for other concrete media types.
- Rejected double quotes and backslashes in multipart field names and
filenames while retaining Unicode names and spaces.
- Restored {} for empty JSON bodies.
- Added descriptive IllegalArgumentException handling for malformed and
non-object form/multipart bodies.
Regression tests and both language examples are updated. The related
16-module clean verify passed, including Checkstyle and Apache RAT. All 204 MCP
Server tests passed.
Could you please re-review and approve the workflow runs if approval is
still required?
--
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]
