Skip to content

fix: fix inconsistent naming pattern#2945

Open
JisoLya wants to merge 5 commits intoapache:masterfrom
JisoLya:arthas-sec
Open

fix: fix inconsistent naming pattern#2945
JisoLya wants to merge 5 commits intoapache:masterfrom
JisoLya:arthas-sec

Conversation

@JisoLya
Copy link
Contributor

@JisoLya JisoLya commented Jan 27, 2026

Currently, the configuration keys in rest-server.properties use snake_case (e.g., server_port), which is inconsistent with the naming convention expected by ServerOptions.java. This mismatch causes the following issues:

  • User-defined configurations are ignored at startup.
  • The server defaults to hardcoded values in ServerOptions.java.

Terminal logs show warnings such as: "arthas.xxxx is redundant ...", indicating that the properties are not being recognized or registered.
image
image
image

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. store Store module labels Jan 27, 2026
@codecov
Copy link

codecov bot commented Jan 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 1.57%. Comparing base (fc391a7) to head (bda5d52).

❗ There is a different number of reports uploaded between BASE (fc391a7) and HEAD (bda5d52). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (fc391a7) HEAD (bda5d52)
3 2
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #2945       +/-   ##
============================================
- Coverage     35.61%   1.57%   -34.04%     
+ Complexity      333      43      -290     
============================================
  Files           801     779       -22     
  Lines         67533   65018     -2515     
  Branches       8780    8332      -448     
============================================
- Hits          24053    1026    -23027     
- Misses        40916   63906    +22990     
+ Partials       2564      86     -2478     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances security for Arthas debugging endpoints by restricting remote access and standardizing configuration naming patterns across the codebase.

Changes:

  • Added localhost-only access restriction to the store node's arthasstart endpoint
  • Standardized Arthas configuration property names from snake_case to camelCase (e.g., arthas.telnet_portarthas.telnetPort)
  • Changed default Arthas IP binding from 0.0.0.0 to 127.0.0.1 and expanded disabled commands to include jad,ognl,vmtool

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
hugegraph-store/hg-store-node/src/main/resources/application.yml Added Arthas configuration with localhost-only IP binding and expanded disabled commands
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java Added remote access check to arthasstart endpoint and new forbiddenMap helper method
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/AppConfig.java Updated default values for Arthas IP and disabled commands
hugegraph-server/hugegraph-dist/src/assembly/static/conf/rest-server.properties Renamed Arthas properties to camelCase and updated default values
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java Updated default values for Arthas IP binding and disabled commands

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@JisoLya JisoLya changed the title sec(store, server): disable remote access for arthasstart and fix inconsistent naming pattern fix: fix inconsistent naming pattern Jan 29, 2026
@JisoLya JisoLya requested a review from imbajin January 30, 2026 08:02
@github-actions
Copy link

github-actions bot commented Mar 1, 2026

Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label


return raft;
//return okMap("partition", rafts);
return ok("raft", raft);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: /v1/partition/{id} response schema changed unexpectedly

Before this PR, this endpoint serialized the Raft object directly, so callers could read fields like groupId, leader, and partitions from the top level. Wrapping it as { "status": ..., "raft": ... } is a wire-format change unrelated to the Arthas hardening, and it will break existing scripts/tools that already consume the old shape.

If the goal here is only to return a proper HTTP status for /arthasstart, I'd keep the existing payload for /partition/{id} and avoid broad response-shape changes in the same patch.

"compaction task fail to submit, and there could be another task in progress");
}
return map;
return ok("body", map);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: This wraps /compat in a new body envelope

Previously the response was the compaction result itself: { "code": ..., "msg": ... }. After this change it becomes { "status": 200, "body": { "code": ..., "msg": ... } }, which silently breaks any caller reading code/msg from the top level.

If we only need an HTTP 200 transport status here, we can keep the original JSON contract:

Suggested change
return ok("body", map);
return ResponseEntity.ok(map);

map.put(k, v);
return map;
public ResponseEntity<Map<String, Object>> ok(String k, Object v) {
return ResponseEntity.ok(Map.of("status", 200, k, v));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Important: Store APIs currently use status = 0 for success, not 200

IndexAPI.okMap() and the pre-change PartitionAPI.okMap() both expose success as status: 0. Switching the JSON payload to status: 200 changes the existing API contract for every endpoint routed through ok(...) (/v1/partitions, /v1/partition/*, /v1/arthasstart, etc.), even though the HTTP status is already conveyed by ResponseEntity.

To keep the transport semantics improvement without breaking body-level consumers, the helper should preserve the old success code:

Suggested change
return ResponseEntity.ok(Map.of("status", 200, k, v));
return ResponseEntity.ok(Map.of("status", 0, k, v));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inactive size:M This PR changes 30-99 lines, ignoring generated files. store Store module

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants