imbajin commented on code in PR #2945:
URL:
https://github.com/apache/incubator-hugegraph/pull/2945#discussion_r2740396309
##########
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(
Review Comment:
⚠️ **Excessive error logging**
Logging at ERROR level when `HttpServletRequest` is null is misleading -
this would indicate a Spring framework bug, not a security issue. This case
should never occur in normal operation.
```suggestion
if (request == null) {
log.warn("Unexpected: HttpServletRequest is null in arthasstart
endpoint");
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);
}
```
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -109,7 +113,7 @@ public Map<String, Object> getPartitions(
rafts.add(raft);
}
- return okMap("partitions", rafts);
+ return ResponseEntity.status(HttpStatus.OK).body(okMap("partitions",
rafts));
Review Comment:
🧹 **Code style: Inconsistent return type usage**
All modified endpoints now return `ResponseEntity<Map<String, Object>>`, but
the implementation doesn't leverage ResponseEntity's features (e.g., headers,
caching).
**Minor suggestion:**
Since you're already using `okMap()` helper, the ResponseEntity wrapper adds
verbosity without benefit. Consider either:
1. Keep using plain `Map<String, Object>` and let Spring auto-wrap it
2. Or refactor helpers to return `ResponseEntity` directly:
```java
public ResponseEntity<Map<String, Object>> ok(String k, Object v) {
return ResponseEntity.ok(Map.of("status", 200, k, v));
}
```
Current approach: `ResponseEntity.status(HttpStatus.OK).body(okMap(...))` is
redundant since `okMap` already sets status.
##########
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
Review Comment:
⚠️ **Incomplete config migration documentation**
The property naming changed from snake_case to camelCase:
- `arthas.telnet_port` → `arthas.telnetPort`
- `arthas.http_port` → `arthas.httpPort`
- `arthas.disabled_commands` → `arthas.disabledCommands`
**This is a breaking change for existing deployments.**
Should either:
1. Add migration notes in PR description/documentation
2. Or support both naming patterns temporarily:
```java
@Value("${arthas.telnetPort:${arthas.telnet_port:8562}}")
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java:
##########
@@ -439,15 +439,15 @@ public class ServerOptions extends OptionHolder {
"arthas.ip",
"arthas bound ip",
disallowEmpty(),
- "0.0.0.0"
+ "127.0.0.1"
);
public static final ConfigOption<String> ARTHAS_DISABLED_COMMANDS =
new ConfigOption<>(
"arthas.disabledCommands",
"arthas disabled commands",
disallowEmpty(),
- "jad"
+ "jad,ognl,vmtool"
Review Comment:
‼️ **Security improvement: Good choice of disabled commands**
Disabling `ognl` and `vmtool` in addition to `jad` significantly improves
security:
- `ognl`: Can execute arbitrary Java code
- `vmtool`: Can manipulate JVM internals and create instances
However, consider also disabling:
- `sc` (search class) - can expose internal structure
- `sm` (search method) - can expose method signatures
- `dump` - can dump class bytecode
For production, you might want an allowlist instead:
```properties
arthas.disabledCommands=*
arthas.enabledCommands=thread,dashboard,jvm,sysprop
```
--
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]
