Copilot commented on code in PR #8035:
URL: https://github.com/apache/incubator-seata/pull/8035#discussion_r3025608641
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -57,13 +57,57 @@ public class ConsoleRemotingFilter implements Filter {
private final Pattern urlPattern = Pattern.compile(CONSOLE_PATTERN);
- private final Logger logger =
LoggerFactory.getLogger(ConsoleRemotingFilter.class);
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ConsoleRemotingFilter.class);
public ConsoleRemotingFilter(NamingManager namingManager, RestTemplate
restTemplate) {
this.namingManager = namingManager;
this.restTemplate = restTemplate;
}
+ /**
+ * Check whether the proxied Content-Type is safe (will not be rendered as
+ * HTML / XML by the browser). Only allow known-safe MIME types through
+ * (allowlist approach); everything else is replaced with
+ * {@code application/json}.
+ */
+ private static boolean isSafeContentType(String contentType) {
+ if (contentType == null) {
+ return false;
+ }
+ String lower = contentType.toLowerCase();
+ // Extract the primary MIME type (ignore parameters such as charset)
+ int semicolonIdx = lower.indexOf(';');
+ String mimeType = (semicolonIdx >= 0 ? lower.substring(0,
semicolonIdx) : lower).trim();
+ return "application/json".equals(mimeType)
Review Comment:
`String lower = contentType.toLowerCase()` uses the default JVM locale,
which can cause incorrect case-folding for MIME types in certain locales (e.g.,
Turkish). Use `toLowerCase(Locale.ROOT)` (and import `java.util.Locale`) to
make the Content-Type allowlist check locale-independent.
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +152,75 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
HttpMethod httpMethod;
try {
httpMethod =
HttpMethod.valueOf(request.getMethod());
} catch (IllegalArgumentException ex) {
- logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+ LOGGER.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
return;
}
+
+ // GET/HEAD methods should not have a body; other
methods may include a body as needed.
+ HttpEntity<byte[]> httpEntity;
+ if (HttpMethod.GET.equals(httpMethod) ||
HttpMethod.HEAD.equals(httpMethod)) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+ headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ byte[] body = request.getCachedBody();
+ if (body == null || body.length == 0) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+
headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only for empty body
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ httpEntity = new HttpEntity<>(body,
headers);
+ }
+ }
+
try {
- ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(
- URI.create(targetUrl), httpMethod,
httpEntity, byte[].class);
+ ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(URI.create(targetUrl), httpMethod, httpEntity,
byte[].class);
+ //Copy headers from proxied response, skipping
hop-by-hop and headers we manage ourselves to mitigate
+ // security risks from Content-Type
manipulation
responseEntity.getHeaders().forEach((key,
value) -> {
- value.forEach(v -> response.addHeader(key,
v));
+ if
(!HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(key)
+ &&
!HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(key)
+ &&
!HttpHeaders.TRANSFER_ENCODING.equalsIgnoreCase(key)
+ &&
!"X-Content-Type-Options".equalsIgnoreCase(key)) {
+ value.forEach(v ->
response.addHeader(key, v));
+ }
});
- response.setStatus(
-
responseEntity.getStatusCode().value());
-
Optional.ofNullable(responseEntity.getBody()).ifPresent(body -> {
+ // Force a safe Content-Type: reject HTML/XML
types that could
+ // execute scripts; fall back to
application/json
+ String proxiedContentType =
responseEntity.getHeaders().getFirst(HttpHeaders.CONTENT_TYPE);
+ String safeContentType;
+ if (proxiedContentType != null &&
isSafeContentType(proxiedContentType)) {
+ safeContentType = proxiedContentType;
+ } else {
+ safeContentType =
"application/json;charset=UTF-8";
+ }
+ response.setContentType(safeContentType);
+ response.setHeader("X-Content-Type-Options",
"nosniff");
+
response.setStatus(responseEntity.getStatusCode().value());
+ byte[] responseBody = responseEntity.getBody();
+ if (responseBody != null &&
responseBody.length > 0) {
+ // For JSON content type, validate that
the body actually looks
+ // like JSON to prevent XSS via crafted
upstream responses
+ if
(safeContentType.toLowerCase().contains("application/json")
+ && !looksLikeJson(responseBody)) {
+
response.setContentType("application/json;charset=UTF-8");
Review Comment:
`safeContentType.toLowerCase().contains("application/json")` also relies on
the default locale. Use `toLowerCase(Locale.ROOT)` (or reuse the
already-normalized MIME type) to avoid locale-dependent behavior when deciding
whether to run JSON validation.
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +152,75 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
HttpMethod httpMethod;
try {
httpMethod =
HttpMethod.valueOf(request.getMethod());
} catch (IllegalArgumentException ex) {
- logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+ LOGGER.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
return;
}
+
+ // GET/HEAD methods should not have a body; other
methods may include a body as needed.
+ HttpEntity<byte[]> httpEntity;
+ if (HttpMethod.GET.equals(httpMethod) ||
HttpMethod.HEAD.equals(httpMethod)) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+ headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ byte[] body = request.getCachedBody();
+ if (body == null || body.length == 0) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+
headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only for empty body
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ httpEntity = new HttpEntity<>(body,
headers);
+ }
+ }
+
try {
- ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(
- URI.create(targetUrl), httpMethod,
httpEntity, byte[].class);
+ ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(URI.create(targetUrl), httpMethod, httpEntity,
byte[].class);
+ //Copy headers from proxied response, skipping
hop-by-hop and headers we manage ourselves to mitigate
+ // security risks from Content-Type
manipulation
responseEntity.getHeaders().forEach((key,
value) -> {
- value.forEach(v -> response.addHeader(key,
v));
+ if
(!HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(key)
+ &&
!HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(key)
+ &&
!HttpHeaders.TRANSFER_ENCODING.equalsIgnoreCase(key)
+ &&
!"X-Content-Type-Options".equalsIgnoreCase(key)) {
+ value.forEach(v ->
response.addHeader(key, v));
+ }
});
- response.setStatus(
-
responseEntity.getStatusCode().value());
-
Optional.ofNullable(responseEntity.getBody()).ifPresent(body -> {
+ // Force a safe Content-Type: reject HTML/XML
types that could
+ // execute scripts; fall back to
application/json
+ String proxiedContentType =
responseEntity.getHeaders().getFirst(HttpHeaders.CONTENT_TYPE);
+ String safeContentType;
+ if (proxiedContentType != null &&
isSafeContentType(proxiedContentType)) {
+ safeContentType = proxiedContentType;
+ } else {
+ safeContentType =
"application/json;charset=UTF-8";
+ }
+ response.setContentType(safeContentType);
+ response.setHeader("X-Content-Type-Options",
"nosniff");
+
response.setStatus(responseEntity.getStatusCode().value());
+ byte[] responseBody = responseEntity.getBody();
+ if (responseBody != null &&
responseBody.length > 0) {
+ // For JSON content type, validate that
the body actually looks
+ // like JSON to prevent XSS via crafted
upstream responses
+ if
(safeContentType.toLowerCase().contains("application/json")
+ && !looksLikeJson(responseBody)) {
+
response.setContentType("application/json;charset=UTF-8");
+ responseBody = "{\"error\":\"Upstream
returned invalid response body\"}"
+
.getBytes(StandardCharsets.UTF_8);
+ }
try (ServletOutputStream outputStream =
response.getOutputStream()) {
- outputStream.write(body);
+ outputStream.write(responseBody);
outputStream.flush();
- } catch (IOException e) {
- logger.error(e.getMessage(), e);
}
- });
+ }
} catch (Exception ex) {
- logger.error(ex.getMessage(), ex);
+ LOGGER.error(ex.getMessage(), ex);
response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
}
Review Comment:
`IOException` while writing to `response.getOutputStream()` (e.g., client
disconnect / broken pipe) will now be caught by the outer `catch (Exception
...)` and triggers `sendError(500)`, which can itself fail if the response is
already committed and may generate noisy error logs. Consider catching
`IOException` around the write separately and handling client-abort cases
without attempting `sendError`.
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +152,75 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
HttpMethod httpMethod;
try {
httpMethod =
HttpMethod.valueOf(request.getMethod());
} catch (IllegalArgumentException ex) {
- logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+ LOGGER.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
return;
}
+
+ // GET/HEAD methods should not have a body; other
methods may include a body as needed.
+ HttpEntity<byte[]> httpEntity;
+ if (HttpMethod.GET.equals(httpMethod) ||
HttpMethod.HEAD.equals(httpMethod)) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+ headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ byte[] body = request.getCachedBody();
+ if (body == null || body.length == 0) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+
headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only for empty body
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ httpEntity = new HttpEntity<>(body,
headers);
+ }
+ }
+
try {
- ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(
- URI.create(targetUrl), httpMethod,
httpEntity, byte[].class);
+ ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(URI.create(targetUrl), httpMethod, httpEntity,
byte[].class);
+ //Copy headers from proxied response, skipping
hop-by-hop and headers we manage ourselves to mitigate
+ // security risks from Content-Type
manipulation
responseEntity.getHeaders().forEach((key,
value) -> {
- value.forEach(v -> response.addHeader(key,
v));
+ if
(!HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(key)
+ &&
!HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(key)
+ &&
!HttpHeaders.TRANSFER_ENCODING.equalsIgnoreCase(key)
Review Comment:
The comment says “skipping hop-by-hop” headers, but the filter currently
only excludes Content-Type/Length/Transfer-Encoding/X-Content-Type-Options.
Hop-by-hop headers like `Connection`, `Keep-Alive`, `Proxy-Authenticate`,
`Proxy-Authorization`, `TE`, `Trailer`, and `Upgrade` should also be stripped
(or the comment should be adjusted) to avoid proxying invalid/unsafe headers.
```suggestion
&&
!HttpHeaders.TRANSFER_ENCODING.equalsIgnoreCase(key)
&&
!HttpHeaders.CONNECTION.equalsIgnoreCase(key)
&&
!HttpHeaders.KEEP_ALIVE.equalsIgnoreCase(key)
&&
!HttpHeaders.PROXY_AUTHENTICATE.equalsIgnoreCase(key)
&&
!HttpHeaders.PROXY_AUTHORIZATION.equalsIgnoreCase(key)
&&
!HttpHeaders.TE.equalsIgnoreCase(key)
&&
!HttpHeaders.TRAILER.equalsIgnoreCase(key)
&&
!HttpHeaders.UPGRADE.equalsIgnoreCase(key)
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +152,75 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
HttpMethod httpMethod;
try {
httpMethod =
HttpMethod.valueOf(request.getMethod());
} catch (IllegalArgumentException ex) {
- logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+ LOGGER.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
return;
}
+
+ // GET/HEAD methods should not have a body; other
methods may include a body as needed.
+ HttpEntity<byte[]> httpEntity;
+ if (HttpMethod.GET.equals(httpMethod) ||
HttpMethod.HEAD.equals(httpMethod)) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+ headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only
+ httpEntity = new HttpEntity<>(headers);
+ } else {
Review Comment:
This change fixes GET/HEAD proxying by stripping request bodies (to avoid
OkHttp’s IllegalArgumentException), but there’s no automated test covering the
regression. Adding a unit/integration test for `ConsoleRemotingFilter` that
exercises a GET request with a non-empty body would help prevent reintroducing
this issue.
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -108,33 +152,75 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
.forEach(headerName ->
headers.add(headerName, request.getHeader(headerName)));
// Create the HttpEntity with headers and body
- HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
HttpMethod httpMethod;
try {
httpMethod =
HttpMethod.valueOf(request.getMethod());
} catch (IllegalArgumentException ex) {
- logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+ LOGGER.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
return;
}
+
+ // GET/HEAD methods should not have a body; other
methods may include a body as needed.
+ HttpEntity<byte[]> httpEntity;
+ if (HttpMethod.GET.equals(httpMethod) ||
HttpMethod.HEAD.equals(httpMethod)) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+ headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ byte[] body = request.getCachedBody();
+ if (body == null || body.length == 0) {
+ headers.remove(HttpHeaders.CONTENT_LENGTH);
+
headers.remove(HttpHeaders.TRANSFER_ENCODING);
+ // headers-only for empty body
+ httpEntity = new HttpEntity<>(headers);
+ } else {
+ httpEntity = new HttpEntity<>(body,
headers);
+ }
+ }
+
try {
- ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(
- URI.create(targetUrl), httpMethod,
httpEntity, byte[].class);
+ ResponseEntity<byte[]> responseEntity =
restTemplate.exchange(URI.create(targetUrl), httpMethod, httpEntity,
byte[].class);
+ //Copy headers from proxied response, skipping
hop-by-hop and headers we manage ourselves to mitigate
+ // security risks from Content-Type
manipulation
responseEntity.getHeaders().forEach((key,
value) -> {
- value.forEach(v -> response.addHeader(key,
v));
+ if
(!HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(key)
+ &&
!HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(key)
+ &&
!HttpHeaders.TRANSFER_ENCODING.equalsIgnoreCase(key)
+ &&
!"X-Content-Type-Options".equalsIgnoreCase(key)) {
+ value.forEach(v ->
response.addHeader(key, v));
+ }
});
- response.setStatus(
-
responseEntity.getStatusCode().value());
-
Optional.ofNullable(responseEntity.getBody()).ifPresent(body -> {
+ // Force a safe Content-Type: reject HTML/XML
types that could
+ // execute scripts; fall back to
application/json
+ String proxiedContentType =
responseEntity.getHeaders().getFirst(HttpHeaders.CONTENT_TYPE);
+ String safeContentType;
+ if (proxiedContentType != null &&
isSafeContentType(proxiedContentType)) {
+ safeContentType = proxiedContentType;
+ } else {
+ safeContentType =
"application/json;charset=UTF-8";
+ }
+ response.setContentType(safeContentType);
+ response.setHeader("X-Content-Type-Options",
"nosniff");
+
response.setStatus(responseEntity.getStatusCode().value());
+ byte[] responseBody = responseEntity.getBody();
+ if (responseBody != null &&
responseBody.length > 0) {
+ // For JSON content type, validate that
the body actually looks
+ // like JSON to prevent XSS via crafted
upstream responses
+ if
(safeContentType.toLowerCase().contains("application/json")
+ && !looksLikeJson(responseBody)) {
+
response.setContentType("application/json;charset=UTF-8");
+ responseBody = "{\"error\":\"Upstream
returned invalid response body\"}"
+
.getBytes(StandardCharsets.UTF_8);
+ }
Review Comment:
When `looksLikeJson(...)` fails, the code replaces the upstream body with an
error JSON but keeps the original upstream HTTP status code (potentially 200).
Consider setting an appropriate error status (e.g., 502 Bad Gateway) and/or
logging a warning so clients don’t treat the response as successful while
receiving an injected error payload.
--
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]
