Copilot commented on code in PR #2945:
URL:
https://github.com/apache/incubator-hugegraph/pull/2945#discussion_r2740417738
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -225,6 +247,34 @@ public Map<String, Object> okMap(String k, Object v) {
return map;
}
+ public Map<String, Object> forbiddenMap(String k, Object v) {
+ HashMap<String, Object> map = new HashMap<>();
+ map.put("status", 403);
+ map.put(k, v);
+ return map;
+ }
+
+ private String getClientIp(HttpServletRequest request) {
+ String ip = request.getHeader("X-Forwarded-For");
+ if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) {
+ ip = request.getHeader("Proxy-Client-IP");
+ }
+ if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) {
+ ip = request.getHeader("WL-Proxy-Client-IP");
+ }
+ if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) {
+ ip = request.getHeader("X-Real-IP");
+ }
+ if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) {
+ ip = request.getRemoteAddr();
+ }
+
+ if (ip != null && ip.contains(",")) {
+ ip = ip.split(",")[0].trim();
+ }
+ return ip;
Review Comment:
The X-Forwarded-For header can be spoofed by clients to bypass the localhost
restriction. An attacker could send a request with "X-Forwarded-For: 127.0.0.1"
to gain unauthorized access to the arthasstart endpoint. For security-critical
endpoints like this one that start diagnostic tools, you should only rely on
request.getRemoteAddr() and not check forwarded headers, or implement
additional authentication mechanisms.
```suggestion
/*
* For security-sensitive operations, do not trust client-controlled
* headers such as X-Forwarded-For or X-Real-IP, since they can be
* spoofed to bypass localhost or other IP-based restrictions.
* Instead, rely solely on the remote address reported by the
* servlet container.
*/
return request.getRemoteAddr();
```
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -168,27 +172,45 @@ public Map<String, Object>
dumpPartition(@PathVariable(value = "id") int id) thr
}
cfIterator.close();
});
- return okMap("ok", null);
+ return ResponseEntity.status(HttpStatus.OK).body(okMap("ok", null));
}
/**
* Print all keys in the partition
*/
@GetMapping(value = "/partition/clean/{id}", produces =
MediaType.APPLICATION_JSON_VALUE)
- public Map<String, Object> cleanPartition(@PathVariable(value = "id") int
id) throws
-
PDException {
+ public ResponseEntity<Map<String, Object>> cleanPartition(
+ @PathVariable(value = "id") int id) throws PDException {
HgStoreEngine storeEngine = nodeService.getStoreEngine();
BusinessHandler handler = storeEngine.getBusinessHandler();
storeEngine.getPartitionEngine(id).getPartitions().forEach((graph,
partition) -> {
handler.cleanPartition(graph, id);
});
- return okMap("ok", null);
+ return ResponseEntity.status(HttpStatus.OK).body(okMap("ok", null));
}
@GetMapping(value = "/arthasstart", produces = "application/json")
- public Map<String, Object> arthasstart(
- @RequestParam(required = false, defaultValue = "") String flags) {
+ public ResponseEntity<Map<String, Object>> arthasstart(
+ @RequestParam(required = false, defaultValue = "") String flags,
+ HttpServletRequest request) {
+ if (request == null) {
+ log.error("Security Alert: HttpServletRequest is NULL when
accessing /arthasstart. " +
+ "Access denied.");
+ Map<String, Object> err = new HashMap<>();
+ err.put("status", "ERROR");
+ err.put("message", "Internal Error: Cannot determine client IP.");
+ return
ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(err);
+ }
+ String remoteAddr = getClientIp(request);
+ boolean isLocalRequest =
+ "127.0.0.1".equals(remoteAddr) ||
"[0:0:0:0:0:0:0:1]".equals(remoteAddr);
Review Comment:
The IPv6 localhost address check is incorrect. The actual IPv6 localhost
address returned by getRemoteAddr() is "0:0:0:0:0:0:0:1" (without brackets),
but this code checks for "[0:0:0:0:0:0:0:1]" (with brackets). This means IPv6
localhost requests will be incorrectly blocked. Change the check to use
"0:0:0:0:0:0:0:1" without brackets, or alternatively use "::1" which is the
standard IPv6 localhost representation.
```suggestion
"127.0.0.1".equals(remoteAddr) ||
"0:0:0:0:0:0:0:1".equals(remoteAddr) ||
"::1".equals(remoteAddr);
```
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -168,27 +172,45 @@ public Map<String, Object>
dumpPartition(@PathVariable(value = "id") int id) thr
}
cfIterator.close();
});
- return okMap("ok", null);
+ return ResponseEntity.status(HttpStatus.OK).body(okMap("ok", null));
}
/**
* Print all keys in the partition
*/
@GetMapping(value = "/partition/clean/{id}", produces =
MediaType.APPLICATION_JSON_VALUE)
- public Map<String, Object> cleanPartition(@PathVariable(value = "id") int
id) throws
-
PDException {
+ public ResponseEntity<Map<String, Object>> cleanPartition(
+ @PathVariable(value = "id") int id) throws PDException {
HgStoreEngine storeEngine = nodeService.getStoreEngine();
BusinessHandler handler = storeEngine.getBusinessHandler();
storeEngine.getPartitionEngine(id).getPartitions().forEach((graph,
partition) -> {
handler.cleanPartition(graph, id);
});
- return okMap("ok", null);
+ return ResponseEntity.status(HttpStatus.OK).body(okMap("ok", null));
}
@GetMapping(value = "/arthasstart", produces = "application/json")
- public Map<String, Object> arthasstart(
- @RequestParam(required = false, defaultValue = "") String flags) {
+ public ResponseEntity<Map<String, Object>> arthasstart(
+ @RequestParam(required = false, defaultValue = "") String flags,
+ HttpServletRequest request) {
+ if (request == null) {
+ log.error("Security Alert: HttpServletRequest is NULL when
accessing /arthasstart. " +
+ "Access denied.");
+ Map<String, Object> err = new HashMap<>();
+ err.put("status", "ERROR");
+ err.put("message", "Internal Error: Cannot determine client IP.");
+ return
ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(err);
+ }
Review Comment:
The HttpServletRequest null check is unnecessary in a Spring MVC controller.
Spring will never inject a null HttpServletRequest in a properly configured
application. If this null check is for defensive programming, it should be
documented why this unusual case is being handled, otherwise it can be removed
to simplify the code.
```suggestion
```
##########
hugegraph-server/hugegraph-dist/src/assembly/static/conf/rest-server.properties:
##########
@@ -12,10 +12,10 @@ batch.max_write_ratio=80
batch.max_write_threads=0
# configuration of arthas
-arthas.telnet_port=8562
-arthas.http_port=8561
+arthas.telnetPort=8562
+arthas.httpPort=8561
arthas.ip=127.0.0.1
-arthas.disabled_commands=jad
+arthas.disabledCommands=jad,ognl,vmtool
Review Comment:
The naming pattern inconsistency fix is incomplete. The docker configuration
files still use the old underscore naming pattern (arthas.telnet_port,
arthas.http_port, arthas.disabled_commands) instead of the new camelCase
pattern. These files need to be updated to use arthas.telnetPort,
arthas.httpPort, and arthas.disabledCommands to be consistent with the changes
in the main configuration files.
--
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]
