Copilot commented on code in PR #2945:
URL:
https://github.com/apache/incubator-hugegraph/pull/2945#discussion_r2731205343
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -189,6 +192,16 @@ public Map<String, Object>
cleanPartition(@PathVariable(value = "id") int id) th
@GetMapping(value = "/arthasstart", produces = "application/json")
public Map<String, Object> arthasstart(
@RequestParam(required = false, defaultValue = "") String flags) {
+ String remoteAddr = ((ServletRequestAttributes) Objects.requireNonNull(
+
RequestContextHolder.getRequestAttributes())).getRequest().getRemoteAddr();
+
+ boolean isLocalRequest = "127.0.0.1".equals(remoteAddr) ||
+ "[0:0:0:0:0:0:0:1]".equals(remoteAddr);
+ if (!isLocalRequest){
+ List<String> ret = new ArrayList<>();
+ ret.add("Arthas start is ONLY allowed from localhost.");
+ return forbiddenMap("arthasstart", ret);
+ }
Review Comment:
The hugegraph-server ArthasAPI.startArthas() endpoint at
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/arthas/ArthasAPI.java
lacks the same remote access restriction that was added to the store node's
arthasstart endpoint. While the IP binding is now restricted to 127.0.0.1, the
API endpoint itself can still be invoked remotely if the server is exposed.
Consider adding similar localhost-only validation to the hugegraph-server's
ArthasAPI for consistency and defense in depth.
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -189,6 +192,16 @@ public Map<String, Object>
cleanPartition(@PathVariable(value = "id") int id) th
@GetMapping(value = "/arthasstart", produces = "application/json")
public Map<String, Object> arthasstart(
@RequestParam(required = false, defaultValue = "") String flags) {
+ String remoteAddr = ((ServletRequestAttributes) Objects.requireNonNull(
+
RequestContextHolder.getRequestAttributes())).getRequest().getRemoteAddr();
+
+ boolean isLocalRequest = "127.0.0.1".equals(remoteAddr) ||
+ "[0:0:0:0:0:0:0:1]".equals(remoteAddr);
Review Comment:
The IPv6 loopback address check may be incorrect. The standard IPv6 loopback
address returned by getRemoteAddr() is typically "0:0:0:0:0:0:0:1" without
square brackets, not "[0:0:0:0:0:0:0:1]". Spring's getRemoteAddr() does not
wrap IPv6 addresses in brackets. Consider using "0:0:0:0:0:0:0:1" or the short
form "::1" instead.
```suggestion
"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:
##########
@@ -225,6 +238,13 @@ 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);
Review Comment:
Missing space in the map initialization. There should be a space after the
comma to follow standard Java formatting conventions.
```suggestion
map.put(k, v);
```
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -189,6 +192,16 @@ public Map<String, Object>
cleanPartition(@PathVariable(value = "id") int id) th
@GetMapping(value = "/arthasstart", produces = "application/json")
public Map<String, Object> arthasstart(
@RequestParam(required = false, defaultValue = "") String flags) {
+ String remoteAddr = ((ServletRequestAttributes) Objects.requireNonNull(
+
RequestContextHolder.getRequestAttributes())).getRequest().getRemoteAddr();
+
+ boolean isLocalRequest = "127.0.0.1".equals(remoteAddr) ||
+ "[0:0:0:0:0:0:0:1]".equals(remoteAddr);
+ if (!isLocalRequest){
+ List<String> ret = new ArrayList<>();
+ ret.add("Arthas start is ONLY allowed from localhost.");
+ return forbiddenMap("arthasstart", ret);
Review Comment:
The forbiddenMap method returns a map with "status": 403, but this only sets
the status in the response body, not the HTTP response status code. The HTTP
response will still have a 200 OK status code. Consider using Spring's
ResponseEntity to properly set the HTTP status code to 403 Forbidden, or add
@ResponseStatus annotation, or inject HttpServletResponse and set the status
there.
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -189,6 +192,16 @@ public Map<String, Object>
cleanPartition(@PathVariable(value = "id") int id) th
@GetMapping(value = "/arthasstart", produces = "application/json")
public Map<String, Object> arthasstart(
@RequestParam(required = false, defaultValue = "") String flags) {
+ String remoteAddr = ((ServletRequestAttributes) Objects.requireNonNull(
+
RequestContextHolder.getRequestAttributes())).getRequest().getRemoteAddr();
+
+ boolean isLocalRequest = "127.0.0.1".equals(remoteAddr) ||
+ "[0:0:0:0:0:0:0:1]".equals(remoteAddr);
+ if (!isLocalRequest){
Review Comment:
Missing space after closing parenthesis. The code should have a space
between ')' and '{' to follow standard Java formatting conventions.
```suggestion
if (!isLocalRequest) {
```
--
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]
