imbajin commented on code in PR #2944:
URL:
https://github.com/apache/incubator-hugegraph/pull/2944#discussion_r2724267506
##########
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java:
##########
@@ -88,8 +88,12 @@ private void setLayoutIfNeeded(Configuration conf) {
public <T, R> R get(TypedOption<T, R> option) {
Object value = this.getProperty(option.name());
if (value == null) {
- return option.defaultValue();
+ value = option.defaultValue();
}
+
+ // Normalize URL options if needed (add scheme like http://)
Review Comment:
**‼️ Critical: Logic Flow Issue**
The normalization logic modifies the default value before returning it, but
this happens **after** the value is already set from `defaultValue()`. This
means the normalization will apply to **both** user-provided values AND default
values.
However, if the `ConfigOption` already has a proper default like
`"http://127.0.0.1:8080"`, the normalization is redundant. More importantly,
this design couples the config retrieval logic with URL-specific assumptions.
Consider: Does this break if someone later adds a URL option that isn't in
the allowlist? The normalization silently won't apply, creating inconsistent
behavior.
##########
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/config/HugeConfig.java:
##########
@@ -213,4 +217,55 @@ private static Configuration loadConfigFile(File
configFile) {
e, configFile);
}
}
+
+ private static Object normalizeUrlOptionIfNeeded(String key, Object value)
{
+ if (value == null) {
+ return null;
+ }
+
+ String scheme = defaultSchemeFor(key);
+ if (scheme == null) {
+ return value;
+ }
+
+ // URL options are defined as ConfigOption<String> and normalized here.
+ if (value instanceof String) {
+ return prefixSchemeIfMissing((String) value, scheme);
+ }
+
+ // If it ever hits here, it means config storage returned a non-string
type;
+ // leave it unchanged (safer than forcing toString()).
+ return value;
+ }
+
+ private static String defaultSchemeFor(String key) {
+ switch (key) {
+ case "restserver.url":
+ case "gremlinserver.url":
+ case "server.urls_to_pd":
+ return "http://";;
+ case "server.k8s_url":
+ return "https://";;
+ default:
+ return null;
+ }
+ }
+
+ private static String prefixSchemeIfMissing(String raw, String scheme) {
+ if (raw == null) {
+ return null;
+ }
+ String s = raw.trim();
+ if (s.isEmpty()) {
+ return s;
+ }
+
+ // Keep original string if scheme already exists
Review Comment:
**‼️ Critical: Case Sensitivity Bug**
Using `.toLowerCase()` for comparison but returning the original string
creates a vulnerability:
```java
String lower = s.toLowerCase();
if (lower.startsWith("http://";) || lower.startsWith("https://";)) {
return s; // Returns original - might be "HTTP://" or "HtTp://"
}
```
This means `"HTTP://127.0.0.1:8080"` would pass through unchanged. While
URLs are case-insensitive for the scheme part per RFC 3986, this creates
inconsistent output where some schemes are lowercase and others preserve their
original casing.
**Suggestion:**
```suggestion
// Keep original string if scheme already exists (case-insensitive
check)
if (s.startsWith("http://";) || s.startsWith("https://";) ||
s.startsWith("HTTP://") || s.startsWith("HTTPS://")) {
return s;
}
```
Or normalize consistently:
```java
// Check if scheme exists (case-insensitive)
if (lower.startsWith("http://";) || lower.startsWith("https://";)) {
// Normalize scheme to lowercase per RFC 3986
return lower;
}
```
--
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]
