Copilot commented on code in PR #8077:
URL: https://github.com/apache/incubator-seata/pull/8077#discussion_r3152838427
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -277,6 +277,24 @@ public Result<String> removeGroup(Unit unit, String
vGroup, String clusterName,
return new Result<>("200", "remove group in old cluster
successfully!");
}
+ private int executeControlRequest(String httpUrl, Map<String, String>
params) {
+ URI targetUri = buildControlUri(httpUrl, params);
+ Integer statusCode = restClient
+ .get()
+ .uri(targetUri)
+ .header("Content-Type", "application/x-www-form-urlencoded")
+ .exchange((request, response) ->
response.getStatusCode().value());
+ return Objects.requireNonNull(statusCode);
+ }
+
+ private URI buildControlUri(String httpUrl, Map<String, String> params) {
+ String rawUrl = Objects.requireNonNull(httpUrl);
+ String baseUrl = rawUrl.endsWith("?") ? rawUrl.substring(0,
rawUrl.length() - 1) : rawUrl;
+ UriComponentsBuilder builder =
UriComponentsBuilder.fromUriString(baseUrl);
+ params.forEach(builder::queryParam);
+ return Objects.requireNonNull(builder.build(true).toUri());
Review Comment:
`buildControlUri` calls `builder.build(true)`, which treats query parameters
as already-encoded. Previously `HttpClientUtil` URL-encoded keys/values; with
`build(true)` special characters in `vGroup`/`unit` (spaces, `/`, `+`, `&`,
non-ASCII) can be sent unencoded and break request parsing. Build/encode the
URI instead (e.g., `builder.build().encode().toUri()`), or only use
`build(true)` if you pre-encode params.
```suggestion
return Objects.requireNonNull(builder.build().encode().toUri());
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -271,4 +287,24 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
}
filterChain.doFilter(servletRequest, servletResponse);
}
+
+ private ResponseEntity<byte[]> executeProxyRequest(
+ URI targetUrl, HttpMethod httpMethod, HttpEntity<byte[]>
httpEntity) {
+ RestClient.RequestBodySpec requestSpec = restClient
+ .method(Objects.requireNonNull(httpMethod))
+ .uri(Objects.requireNonNull(targetUrl))
+ .headers(headers -> headers.addAll(httpEntity.getHeaders()));
+ RestClient.RequestHeadersSpec<?> exchangeSpec = requestSpec;
+ byte[] requestBody = httpEntity.getBody();
+ if (requestBody != null
+ && requestBody.length > 0
+ && !HttpMethod.GET.equals(httpMethod)
+ && !HttpMethod.HEAD.equals(httpMethod)) {
+ exchangeSpec = requestSpec.body(requestBody);
+ }
+ return exchangeSpec.exchange((request, response) -> new
ResponseEntity<>(
+ HttpMethod.HEAD.equals(httpMethod) ? null :
StreamUtils.copyToByteArray(response.getBody()),
+ response.getHeaders(),
+ response.getStatusCode()));
Review Comment:
`executeProxyRequest` unconditionally does
`StreamUtils.copyToByteArray(response.getBody())` for non-HEAD requests. If the
upstream response has no body (e.g., 204/304) and `getBody()` is null, this
will throw and the filter will return 500 instead of proxying the
status/headers. Guard against a null body stream (treat it as empty) before
copying.
##########
namingserver/src/test/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilterTest.java:
##########
@@ -211,24 +220,38 @@ void nonJsonBodyWithJsonContentTypeShouldReturn502()
throws Exception {
MockHttpServletResponse response = new MockHttpServletResponse();
- HttpHeaders responseHeaders = new HttpHeaders();
- responseHeaders.set(HttpHeaders.CONTENT_TYPE, "application/json");
- // Upstream sends HTML disguised as JSON
- byte[] htmlBody =
"<html><script>alert('xss')</script></html>".getBytes(StandardCharsets.UTF_8);
- ResponseEntity<byte[]> upstreamResponse = new ResponseEntity<>(
- htmlBody, responseHeaders, HttpStatus.OK);
-
- when(restTemplate.exchange(any(URI.class), eq(HttpMethod.GET),
any(HttpEntity.class), eq(byte[].class)))
- .thenReturn(upstreamResponse);
+ server.expect(once(),
requestTo("http://127.0.0.1:7091/api/v1/console/globalSession/query";))
+ .andExpect(method(HttpMethod.GET))
+ .andRespond(withSuccess(
+ "<html><script>alert('xss')</script></html>",
+ org.springframework.http.MediaType.APPLICATION_JSON));
filter.doFilter(request, response, filterChain);
+ server.verify();
- assertEquals(502, response.getStatus(),
- "Should return 502 when upstream body is not valid JSON");
+ assertEquals(502, response.getStatus(), "Should return 502 when
upstream body is not valid JSON");
String body = response.getContentAsString();
assertEquals("{\"error\":\"Upstream returned invalid response
body\"}", body);
}
+ @Test
+ void non2xxResponseShouldStillBeProxied() throws Exception {
+ MockHttpServletRequest request = createConsoleRequest("GET");
+ MockHttpServletResponse response = new MockHttpServletResponse();
+
+ server.expect(once(),
requestTo("http://127.0.0.1:7091/api/v1/console/globalSession/query";))
+ .andExpect(method(HttpMethod.GET))
+ .andRespond(withStatus(HttpStatus.BAD_GATEWAY)
+
.contentType(org.springframework.http.MediaType.APPLICATION_JSON)
+ .body("{\"error\":\"upstream failed\"}"));
+
+ filter.doFilter(request, response, filterChain);
+ server.verify();
+
+ assertEquals(502, response.getStatus(), "代理模式下应保留上游非 2xx 状态码");
Review Comment:
The assertion message is in Chinese while the rest of this test class uses
English messages. For consistency/readability across the test suite, please
change this message to English.
```suggestion
assertEquals(502, response.getStatus(), "Proxy mode should preserve
upstream non-2xx status codes");
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
