This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new 8014dd10f Harden operation class loading in take/free instance APIs
8014dd10f is described below
commit 8014dd10fcdb7fca561606862c53fdbfe5416352
Author: Junkai Xue <[email protected]>
AuthorDate: Tue Nov 3 11:39:31 2026 -0800
Harden operation class loading in take/free instance APIs
The take/free instance APIs load classes named in the "operation_list"
request field via loadClass(...).newInstance() with no validation. A
caller could name any class on the classpath and have its no-arg
constructor executed before the cast to OperationInterface failed.
Validate the type before instantiating so a class that does not
implement OperationInterface never has its constructor run, and add an
optional comma-separated allowlist (rest.operation.class.allowlist). The
allowlist is unset by default, preserving existing behavior for
operators who supply their own OperationInterface implementations.
Trim the request class name before the allowlist lookup so it matches
trimmed allowlist entries, and unwrap InvocationTargetException so a
legitimate operation constructor's real failure reason is surfaced.
---
.../MaintenanceManagementService.java | 49 ++++++++++++++++++++--
.../helix/rest/common/RestSystemPropertyKeys.java | 6 +++
.../TestMaintenanceManagementService.java | 41 +++++++++++++++++-
3 files changed, 92 insertions(+), 4 deletions(-)
diff --git
a/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
b/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
index 8d5d025b3..43edb5e89 100644
---
a/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
+++
b/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
@@ -20,6 +20,7 @@ package org.apache.helix.rest.clusterMaintenanceService;
*/
import java.io.IOException;
+import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -56,6 +57,7 @@ import org.apache.helix.rest.client.CustomRestClient;
import org.apache.helix.rest.client.CustomRestClientFactory;
import org.apache.helix.rest.clusterMaintenanceService.api.OperationInterface;
import org.apache.helix.rest.common.HelixDataAccessorWrapper;
+import org.apache.helix.rest.common.RestSystemPropertyKeys;
import org.apache.helix.rest.common.datamodel.RestSnapShot;
import org.apache.helix.rest.server.json.instance.InstanceInfo;
import org.apache.helix.rest.server.json.instance.StoppableCheck;
@@ -337,13 +339,39 @@ public class MaintenanceManagementService {
private List<OperationInterface> getAllOperationClasses(List<String>
operations) {
List<OperationInterface> operationAbstractClassList = new ArrayList<>();
- for (String operationClassName : operations) {
+ Set<String> allowlist = getOperationClassAllowlist();
+ for (String operationClassNameRaw : operations) {
+ String operationClassName =
+ operationClassNameRaw == null ? null : operationClassNameRaw.trim();
try {
+ if (!allowlist.isEmpty() && !allowlist.contains(operationClassName)) {
+ throw new HelixException(
+ String.format("Operation class is not in the configured
allowlist: %s",
+ operationClassName));
+ }
LOG.info("Loading class: " + operationClassName);
+ Class<?> operationClass = HelixUtil.loadClass(getClass(),
operationClassName);
+ // Validate the type BEFORE instantiating so that a class which does
not implement
+ // OperationInterface never has its constructor executed.
+ if (!OperationInterface.class.isAssignableFrom(operationClass)) {
+ throw new HelixException(
+ String.format("Operation class does not implement
OperationInterface: %s",
+ operationClassName));
+ }
OperationInterface userOperation =
- (OperationInterface) HelixUtil.loadClass(getClass(),
operationClassName)
- .newInstance();
+ (OperationInterface)
operationClass.getDeclaredConstructor().newInstance();
operationAbstractClassList.add(userOperation);
+ } catch (HelixException e) {
+ LOG.error("Rejected operation class: {}. message: ",
operationClassName, e);
+ throw e;
+ } catch (InvocationTargetException e) {
+ // Unwrap so the operation class constructor's real failure reason is
surfaced.
+ Throwable cause = e.getCause() == null ? e : e.getCause();
+ LOG.error("Operation class constructor failed for: {}. message: ",
operationClassName,
+ cause);
+ throw new HelixException(
+ String.format("Operation class constructor failed for: %s.
message: %s",
+ operationClassName, cause));
} catch (Exception e) {
LOG.error("No operation class found for: {}. message: ",
operationClassName, e);
throw new HelixException(
@@ -353,6 +381,21 @@ public class MaintenanceManagementService {
return operationAbstractClassList;
}
+ private static Set<String> getOperationClassAllowlist() {
+ String raw =
System.getProperty(RestSystemPropertyKeys.REST_OPERATION_CLASS_ALLOWLIST);
+ if (raw == null || raw.trim().isEmpty()) {
+ return Collections.emptySet();
+ }
+ Set<String> allowlist = new HashSet<>();
+ for (String entry : raw.split(",")) {
+ String trimmed = entry.trim();
+ if (!trimmed.isEmpty()) {
+ allowlist.add(trimmed);
+ }
+ }
+ return allowlist;
+ }
+
/**
* {@inheritDoc}
* Single instance stoppable check implementation is a special case of
diff --git
a/helix-rest/src/main/java/org/apache/helix/rest/common/RestSystemPropertyKeys.java
b/helix-rest/src/main/java/org/apache/helix/rest/common/RestSystemPropertyKeys.java
index 1d1e42ab1..5dc6dd955 100644
---
a/helix-rest/src/main/java/org/apache/helix/rest/common/RestSystemPropertyKeys.java
+++
b/helix-rest/src/main/java/org/apache/helix/rest/common/RestSystemPropertyKeys.java
@@ -22,4 +22,10 @@ package org.apache.helix.rest.common;
public class RestSystemPropertyKeys {
// System property for REST HTTP request timeout
public static final String REST_HTTP_TIMEOUT_MS = "rest.http.timeout.ms";
+
+ // Optional comma-separated allowlist of fully-qualified operation class
names that the
+ // take/free instance APIs are permitted to load via "operation_list". When
set to a non-empty
+ // value, only classes on this list may be instantiated. When unset or
empty, no name-based
+ // restriction is applied, but every operation class must still implement
OperationInterface.
+ public static final String REST_OPERATION_CLASS_ALLOWLIST =
"rest.operation.class.allowlist";
}
diff --git
a/helix-rest/src/test/java/org/apache/helix/rest/clusterMaintenanceService/TestMaintenanceManagementService.java
b/helix-rest/src/test/java/org/apache/helix/rest/clusterMaintenanceService/TestMaintenanceManagementService.java
index 5801fa231..9a4844a3f 100644
---
a/helix-rest/src/test/java/org/apache/helix/rest/clusterMaintenanceService/TestMaintenanceManagementService.java
+++
b/helix-rest/src/test/java/org/apache/helix/rest/clusterMaintenanceService/TestMaintenanceManagementService.java
@@ -352,6 +352,33 @@ public class TestMaintenanceManagementService {
Assert.assertEquals(instanceInfo2.getOperationResult(),
"DummyTakeOperationResult");
}
+ /*
+ * A class that does not implement OperationInterface must be rejected
before its constructor
+ * runs, so that arbitrary classpath classes cannot be instantiated through
"operation_list".
+ */
+ @Test
+ public void testTakeInstanceRejectsNonOperationClass() throws IOException {
+ Map<String, String> dataMap =
+ ImmutableMap.of("param1", "true", "param2", "faketype", "check1",
"false", "check2",
+ "true");
+ String jsonContent = new ObjectMapper().writeValueAsString(dataMap);
+ ZKHelixDataAccessor zkHelixDataAccessor = mock(ZKHelixDataAccessor.class);
+ MockMaintenanceManagementService service =
+ new MockMaintenanceManagementService(zkHelixDataAccessor,
_configAccessor,
+ _customRestClient, true, false,
HelixRestNamespace.DEFAULT_NAMESPACE_NAME);
+
+ ForbiddenConstructorProbe.invoked = false;
+ MaintenanceManagementInstanceInfo info =
+ service.takeInstance(TEST_CLUSTER, TEST_INSTANCE,
Collections.emptyList(),
+ MaintenanceManagementService.getMapFromJsonPayload(jsonContent),
+
Collections.singletonList(ForbiddenConstructorProbe.class.getName()),
+ Collections.EMPTY_MAP, true);
+ Assert.assertFalse(info.isSuccessful(),
+ "Taking an instance with a non-OperationInterface class must fail");
+ Assert.assertFalse(ForbiddenConstructorProbe.invoked,
+ "Constructor of a non-OperationInterface class must not run");
+ }
+
/*
* Tests stoppable check api when all checks query is enabled. After helix
own check fails,
* the subsequent checks should be performed.
@@ -538,4 +565,16 @@ public class TestMaintenanceManagementService {
verify(_customRestClient, times(1)).getInstanceStoppableCheck(any(),
any());
verify(_customRestClient, times(0)).getPartitionStoppableCheck(any(),
any(), any());
}
-}
\ No newline at end of file
+
+ /*
+ * A plain class (NOT an OperationInterface) whose constructor records if it
ever ran.
+ * Used to prove that rejected operation classes are never instantiated.
+ */
+ public static class ForbiddenConstructorProbe {
+ static volatile boolean invoked = false;
+
+ public ForbiddenConstructorProbe() {
+ invoked = true;
+ }
+ }
+}
