[sahusanket]

original PR : #1040

[gemini-code-assist]

Code Review

This pull request introduces a JEXL sandbox allowlist feature to restrict the classes, methods, and properties executed in JEXL expressions. The review feedback highlights several critical issues that must be addressed: compilation errors in EL.java due to incorrect usage of the JEXL 3 Permissions API, a bootstrapping bug in ConfigStore.java that prevents existing installations from receiving the default allowlist, missing essential utility and collection classes in the default allowlist, an overly restrictive validation in JexlAllowlist.java that blocks method-only or property-only rules, and a potential NullPointerException in DirectivesHandler.java.

[gemini-code-assist]

In Apache Commons JEXL 3, JexlSandbox.Permissions does not have an execute(String) method. Instead, execute() returns a Names object, on which you must call add(String) to allow a method. Calling permissions.execute(method) will cause a compilation error. Please update it to use permissions.execute().add(method).

Suggested change

	rule.getMethods().forEach(method -> permissions.execute(method));
	rule.getMethods().forEach(method -> permissions.execute().add(method));

[gemini-code-assist]

Similarly, JexlSandbox.Permissions does not have read(String) or write(String) methods. You must call read().add(property) and write().add(property) to allow property access. Calling them with arguments will cause a compilation error.

Suggested change

	rule.getProperties().forEach(property -> {
	permissions.read(property);
	permissions.write(property);
	});
	rule.getProperties().forEach(property -> {
	permissions.read().add(property);
	permissions.write().add(property);
	});

[gemini-code-assist]

The initialize() method only bootstraps the default JEXL allowlist if no configuration exists in the database (config == null). For existing installations, config will not be null, meaning the default JEXL allowlist will never be initialized. If the JEXL allowlist feature is enabled, all JEXL expressions will fail because the allowlist is empty. Please update the initialization logic to check if config.getJexlAllowlist() == null and, if so, initialize it with the default values.

  public void initialize() throws IOException {
    DirectiveConfig config = getConfig();

    if (config == null) {
      LOG.info("Initializing Directive config with default values");
      JsonObject configJson = new JsonObject();
      configJson.add(DirectiveConfig.JEXL_ALLOWLIST_KEY, GSON.toJsonTree(DefaultJexlAllowlist.get()));
      updateConfig(GSON.fromJson(configJson, DirectiveConfig.class));
      return;
    }

    if (config.getJexlAllowlist() == null) {
      LOG.info("Existing Directive config found without JEXL allowlist. Initializing JEXL allowlist with default values");
      JsonObject configJson = config.toJson().getAsJsonObject();
      configJson.add(DirectiveConfig.JEXL_ALLOWLIST_KEY, GSON.toJsonTree(DefaultJexlAllowlist.get()));
      updateConfig(GSON.fromJson(configJson, DirectiveConfig.class));
    } else {
      LOG.info("Directive config already exists with JEXL allowlist");
    }
  }

[gemini-code-assist]

The default JEXL allowlist is missing essential classes for standard Wrangler functions and collection operations. For example, standard namespaces like string (resolving to org.apache.commons.lang.StringUtils), json (resolving to io.cdap.functions.JsonFunctions), and standard collection classes like java.util.Map, java.util.HashMap, java.util.List, java.util.ArrayList are not included. When the JEXL allowlist is enabled, any expression using these namespaces or collections will fail. Please consider adding these classes to the default allowlist.

[gemini-code-assist]

The sanitizeList method throws an IllegalArgumentException if the list is empty. However, it is a very common and valid use case to allow a class with only methods and no properties (or vice versa). Forcing both lists to be non-empty prevents users from defining secure, method-only or property-only allowlist rules. Consider allowing empty lists and defaulting null lists to an empty list.

Suggested change

	private static List<String> sanitizeList(List<String> list, String type) {
	if (list == null || list.isEmpty()) {
	throw new IllegalArgumentException("The " + type + " list cannot be null or empty.");
	}
	private static List<String> sanitizeList(List<String> list, String type) {
	if (list == null) {
	return Collections.emptyList();
	}

[gemini-code-assist]

If existingConfig is null (e.g., if the database has not been initialized or the row was deleted), calling existingConfig.toJson() will throw a NullPointerException. Consider adding a null check for existingConfig before calling methods on it.

Suggested change

	if (!jexlAllowlistEnabled) {
	DirectiveConfig existingConfig = configStore.getConfig();
	JsonObject existingJson = existingConfig.toJson().getAsJsonObject();
	JsonObject newJson = config.toJson().getAsJsonObject();
	existingJson.add(DirectiveConfig.EXCLUSIONS_KEY, newJson.get(DirectiveConfig.EXCLUSIONS_KEY));
	existingJson.add(DirectiveConfig.ALIASES_KEY, newJson.get(DirectiveConfig.ALIASES_KEY));
	config = GSON.fromJson(existingJson, DirectiveConfig.class);
	}
	if (!jexlAllowlistEnabled) {
	DirectiveConfig existingConfig = configStore.getConfig();
	if (existingConfig != null) {
	JsonObject existingJson = existingConfig.toJson().getAsJsonObject();
	JsonObject newJson = config.toJson().getAsJsonObject();
	existingJson.add(DirectiveConfig.EXCLUSIONS_KEY, newJson.get(DirectiveConfig.EXCLUSIONS_KEY));
	existingJson.add(DirectiveConfig.ALIASES_KEY, newJson.get(DirectiveConfig.ALIASES_KEY));
	config = GSON.fromJson(existingJson, DirectiveConfig.class);
	}
	}