Copilot commented on code in PR #6295:
URL: https://github.com/apache/shenyu/pull/6295#discussion_r2785461817
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/McpServerPlugin.java:
##########
@@ -579,11 +619,34 @@ private Mono<Void> handleMessageEndpoint(final
ServerWebExchange exchange,
* @param exchange the server web exchange
*/
private void setCorsHeaders(final ServerWebExchange exchange) {
- exchange.getResponse().getHeaders().set("Access-Control-Allow-Origin",
"*");
- exchange.getResponse().getHeaders().set("Access-Control-Allow-Headers",
- "Content-Type, Mcp-Session-Id, Authorization, Last-Event-ID,
Mcp-Protocol-Version");
- exchange.getResponse().getHeaders().set("Access-Control-Allow-Methods",
- "GET, POST, OPTIONS");
+ exchange.getResponse().getHeaders().set("Access-Control-Allow-Origin",
resolveAllowOrigin(exchange));
+
exchange.getResponse().getHeaders().set("Access-Control-Allow-Headers",
resolveAllowHeaders(exchange));
+
exchange.getResponse().getHeaders().set("Access-Control-Allow-Methods",
CORS_ALLOW_METHODS);
+ exchange.getResponse().getHeaders().set("Vary", "Origin,
Access-Control-Request-Headers");
+ }
+
+ private String resolveAllowOrigin(final ServerWebExchange exchange) {
+ final String origin =
exchange.getRequest().getHeaders().getFirst("Origin");
+ return Objects.nonNull(origin) && !origin.isBlank() ? origin : "*";
+ }
+
+ private String resolveAllowHeaders(final ServerWebExchange exchange) {
+ final Set<String> allowedHeaders = new LinkedHashSet<>();
+ final String allowHeaders =
Objects.nonNull(configuredCorsAllowHeaders) &&
!configuredCorsAllowHeaders.isBlank()
+ ? configuredCorsAllowHeaders : CORS_FALLBACK_ALLOW_HEADERS;
+ for (String header : allowHeaders.split(",")) {
+ allowedHeaders.add(header.trim());
+ }
+ final String requestedHeaders =
exchange.getRequest().getHeaders().getFirst("Access-Control-Request-Headers");
+ if (Objects.nonNull(requestedHeaders) && !requestedHeaders.isBlank()) {
+ for (String requestedHeader : requestedHeaders.split(",")) {
+ final String header = requestedHeader.trim();
+ if (!header.isEmpty()) {
+ allowedHeaders.add(header);
+ }
Review Comment:
This logic appends all client-supplied `Access-Control-Request-Headers` into
the allow-list, which effectively allows any requested header and makes
`shenyu.cross.allowedHeaders` non-enforcing. Consider responding with only the
configured allow-list (or intersecting configured vs requested) so disallowed
requested headers fail preflight as intended.
```suggestion
final String trimmed = header.trim();
if (!trimmed.isEmpty()) {
allowedHeaders.add(trimmed);
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/McpServerPlugin.java:
##########
@@ -579,11 +619,34 @@ private Mono<Void> handleMessageEndpoint(final
ServerWebExchange exchange,
* @param exchange the server web exchange
*/
private void setCorsHeaders(final ServerWebExchange exchange) {
- exchange.getResponse().getHeaders().set("Access-Control-Allow-Origin",
"*");
- exchange.getResponse().getHeaders().set("Access-Control-Allow-Headers",
- "Content-Type, Mcp-Session-Id, Authorization, Last-Event-ID,
Mcp-Protocol-Version");
- exchange.getResponse().getHeaders().set("Access-Control-Allow-Methods",
- "GET, POST, OPTIONS");
+ exchange.getResponse().getHeaders().set("Access-Control-Allow-Origin",
resolveAllowOrigin(exchange));
+
exchange.getResponse().getHeaders().set("Access-Control-Allow-Headers",
resolveAllowHeaders(exchange));
+
exchange.getResponse().getHeaders().set("Access-Control-Allow-Methods",
CORS_ALLOW_METHODS);
+ exchange.getResponse().getHeaders().set("Vary", "Origin,
Access-Control-Request-Headers");
Review Comment:
`set("Vary", ...)` overwrites any existing `Vary` header that may already be
set by the framework (e.g., `Accept-Encoding`), which can break caching
semantics. Prefer adding/merging the `Vary` values instead of replacing the
header entirely.
```suggestion
// Merge CORS-related Vary values with any existing Vary header
entries
final List<String> existingVary =
exchange.getResponse().getHeaders().getVary();
final Set<String> varyValues = new LinkedHashSet<>(existingVary);
varyValues.add("Origin");
varyValues.add("Access-Control-Request-Headers");
exchange.getResponse().getHeaders().setVary(new
java.util.ArrayList<>(varyValues));
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/transport/ShenyuStreamableHttpServerTransportProvider.java:
##########
@@ -643,6 +661,40 @@ private Object createJsonRpcError(final Object id, final
int code, final String
return response;
}
+ private ServerResponse.BodyBuilder applyCorsHeaders(final ServerRequest
request,
+ final
ServerResponse.BodyBuilder builder,
+ final String
allowMethods) {
+ return builder
+ .header("Access-Control-Allow-Origin",
resolveAllowOrigin(request))
+ .header("Access-Control-Allow-Headers",
resolveAllowHeaders(request))
+ .header("Access-Control-Allow-Methods", allowMethods)
+ .header("Vary", "Origin, Access-Control-Request-Headers");
+ }
+
+ private String resolveAllowOrigin(final ServerRequest request) {
+ final String origin = request.headers().firstHeader("Origin");
+ return Objects.nonNull(origin) && !origin.isBlank() ? origin : "*";
+ }
+
+ private String resolveAllowHeaders(final ServerRequest request) {
+ final Set<String> allowedHeaders = new LinkedHashSet<>();
+ final String allowHeaders =
Objects.nonNull(configuredCorsAllowHeaders) &&
!configuredCorsAllowHeaders.isBlank()
+ ? configuredCorsAllowHeaders : CORS_FALLBACK_ALLOW_HEADERS;
+ for (String header : allowHeaders.split(",")) {
+ allowedHeaders.add(header.trim());
+ }
+ final String requestedHeaders =
request.headers().firstHeader("Access-Control-Request-Headers");
+ if (Objects.nonNull(requestedHeaders) && !requestedHeaders.isBlank()) {
+ for (String requestedHeader : requestedHeaders.split(",")) {
+ final String header = requestedHeader.trim();
+ if (!header.isEmpty()) {
+ allowedHeaders.add(header);
+ }
+ }
Review Comment:
This logic unions the configured allow-list with all client-supplied
`Access-Control-Request-Headers`, which effectively allows any requested header
and makes the configuration non-enforcing. Consider returning only the
configured allow-list (or intersecting configured vs requested) so disallowed
requested headers fail preflight.
```suggestion
final String trimmed = header.trim();
if (!trimmed.isEmpty()) {
allowedHeaders.add(trimmed);
}
}
final String requestedHeaders =
request.headers().firstHeader("Access-Control-Request-Headers");
if (Objects.nonNull(requestedHeaders) &&
!requestedHeaders.isBlank()) {
final Set<String> effectiveHeaders = new LinkedHashSet<>();
for (String requestedHeader : requestedHeaders.split(",")) {
final String header = requestedHeader.trim();
if (!header.isEmpty() && allowedHeaders.contains(header)) {
effectiveHeaders.add(header);
}
}
if (!effectiveHeaders.isEmpty()) {
return String.join(", ", effectiveHeaders);
}
```
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/transport/ShenyuStreamableHttpServerTransportProvider.java:
##########
@@ -195,18 +221,12 @@ public Mono<ServerResponse> handleUnifiedEndpoint(final
ServerRequest request) {
}
if ("OPTIONS".equalsIgnoreCase(request.methodName())) {
// Handle CORS preflight requests
- return ServerResponse.ok()
- .header("Access-Control-Allow-Origin", "*")
- .header("Access-Control-Allow-Headers", "Content-Type,
Mcp-Session-Id, Authorization, Mcp-Protocol-Version")
- .header("Access-Control-Allow-Methods", "GET, POST,
OPTIONS")
+ return applyCorsHeaders(request, ServerResponse.ok(),
CORS_ALLOW_METHODS)
.header("Access-Control-Max-Age", "3600")
.build();
Review Comment:
This transport returns 405 for GET, but the CORS preflight response
advertises `Access-Control-Allow-Methods: GET, POST, OPTIONS`. Align the
advertised methods with what the endpoint actually supports (likely `POST,
OPTIONS`) to avoid misleading clients and incorrect CORS behavior.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/McpServerPlugin.java:
##########
@@ -276,6 +303,19 @@ private boolean isSseProtocol(final String uri) {
return uri.contains(SSE_PATH) || uri.endsWith(SSE_PATH) ||
uri.endsWith(MESSAGE_ENDPOINT);
}
+ /**
+ * Handles CORS preflight (OPTIONS) requests.
+ *
+ * @param exchange the server web exchange
+ * @return a Mono representing completion
+ */
+ private Mono<Void> handleCorsPreflight(final ServerWebExchange exchange) {
+ exchange.getResponse().setStatusCode(HttpStatus.OK);
+ setCorsHeaders(exchange);
+ exchange.getResponse().getHeaders().set("Access-Control-Max-Age",
"3600");
+ return exchange.getResponse().setComplete();
Review Comment:
CORS behavior was added/changed (preflight handling + configurable allowed
headers), but there are no corresponding tests covering OPTIONS requests and
the resulting `Access-Control-*` headers. Please add unit/integration tests
that assert the preflight response headers for both configured and default
allowed-headers scenarios.
--
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]
