[riyaa14]

Context

This PR introduces a JEXL allowlist mechanism to restrict which Java classes and methods can be invoked within JEXL expressions evaluated by Wrangler directives (e.g., filter, set-column, send-to-error). This enhances the security of JEXL evaluations by preventing arbitrary code execution.

The allowlist enforcement is controlled via the Feature.WRANGLER_JEXL_ALLOWLIST feature flag.

Key Changes

API Additions:
Introduced JexlAllowlist, DefaultJexlAllowlist, and DirectiveJexlAllowlist in wrangler-api to define the schema and default rules for allowed JEXL accesses.

Expression Compilation:
Added CompileOptions to wrangler-core to pass allowlist context and restrictions during JEXL expression compilation in EL.java.

Config Store Initialization: Updated ConfigStore with an initialize() method to populate the store with a default directive config (including DefaultJexlAllowlist) if none exists. Added custom GSON deserializers for parsing JexlAllowlist.

Version Bump: JEXL 3.0 to 3.1, this is compatible change, needed because 3.0 didn't support allowlist mode, we would have need to define a exclusion and inclusion list.

[gemini-code-assist]

Code Review

This pull request introduces a secure JEXL execution sandbox in Wrangler to restrict allowed classes, methods, and properties, updating various directives to compile expressions with sandbox rules and adding a comprehensive performance benchmark. The review feedback identifies several critical improvement opportunities, primarily focusing on preventing potential NullPointerExceptions in EL.java and JexlInclusion.java (especially when handling null JexlInfo or deserialized null fields). Additionally, the feedback points out a logic bug where the sandbox is incorrectly enforced even when the secure JEXL feature is disabled, and advises against committing user-specific absolute paths in .vscode/settings.json.

I am having trouble creating individual review comments. Click here to see my feedback.

wrangler-core/src/main/java/io/cdap/wrangler/expression/EL.java (156-175)

The info object returned by jexlEx.getInfo() can be null in some JexlException scenarios. Accessing info.getDetail(), info.getLine(), or info.getColumn() without a null check will throw a NullPointerException. Please add a null check for info before accessing its properties, similar to how it is handled in handleExecutionException.

    if (ex instanceof JexlException) {
      JexlException jexlEx = (JexlException) ex;
      JexlInfo info = jexlEx.getInfo();
      String detail = (info == null || info.getDetail() == null) ? expression : info.getDetail().toString();
      int line = info == null ? 0 : info.getLine();
      int column = info == null ? 0 : info.getColumn();
      String errorMessage = jexlEx.getMessage();
      if (errorMessage != null && (errorMessage.contains("unsolvable function/method")
          || errorMessage.contains("unsolvable property"))) {
        return new ELException(
            String.format("Security violation: Access to JEXL component '%s' is not "
                    + "permitted by wrangler. Hence, this JEXL expression '%s' can't be resolved.",
                detail, expression),
            jexlEx);
      }
      return new ELException(
          String.format("Error encountered while compiling '%s' at line '%d' "
              + "and column '%d'. Make sure a valid jexl "
              + "transformation is provided.",
              detail, line, column),
          jexlEx);
    }

wrangler-core/src/main/java/io/cdap/wrangler/expression/EL.java (189-191)

If allowlistEnabled is false, the sandbox should not be created or enforced at all. However, the current condition !allowlistEnabled && (inclusions == null || inclusions.isEmpty()) means that if allowlistEnabled is false but inclusions is not empty, a sandbox will still be created and enforced. This violates the feature flag and can break existing JEXL expressions in pipelines where the secure JEXL feature is disabled. Please simplify the check to return null immediately if allowlistEnabled is false.

    if (!allowlistEnabled) {
      return null;
    }

.vscode/settings.json (3-13)

Committing user-specific absolute paths (such as /usr/local/google/home/riyagarg/...) in shared configuration files is a bad practice. It will cause build or IDE configuration issues for other developers who do not have the same directory structure. Consider removing this file from the repository, adding .vscode/ to .gitignore, or using standard/relative paths.

wrangler-core/src/main/java/io/cdap/wrangler/expression/EL.java (215-217)

If inclusions contains a null element, iterating over it and calling applyInclusionRule will result in a NullPointerException when calling rule.getClassName(). Adding a null check for rule prevents this potential crash.

    if (rule == null || rule.getClassName() == null || rule.getClassName().trim().isEmpty()) {
      return;
    }

wrangler-api/src/main/java/io/cdap/wrangler/api/JexlInclusion.java (86-96)

If JexlInclusion is deserialized from JSON and the JSON explicitly contains "methods": null or "properties": null, the fields will be set to null (bypassing constructor defaults). Returning null from these getters can cause NullPointerExceptions downstream. Returning an empty list when they are null is much safer.

  public List<String> getMethods() {
    return methods == null ? Collections.emptyList() : methods;
  }

  /**
   * Gets the list of allowed properties.
   *
   * @return the allowed properties
   */
  public List<String> getProperties() {
    return properties == null ? Collections.emptyList() : properties;
  }

wrangler-api/src/main/java/io/cdap/wrangler/api/JexlInclusion.java (103-114)

If methods or properties is null due to deserialization, calling methods.isEmpty() or properties.isEmpty() will throw a NullPointerException. Adding null checks here ensures robust and defensive execution.

  public boolean isAllMethods() {
    return methods == null || methods.isEmpty() || methods.contains("*");
  }

  /**
   * Checks if all properties are allowed.
   *
   * @return true if all properties are allowed
   */
  public boolean isAllProperties() {
    return properties == null || properties.isEmpty() || properties.contains("*");
  }

[vsethi09]

Why is this needed?

(Throwable) null

[vsethi09]

Why is this needed?

(Throwable) null

[vsethi09]

Move this to a separate file.

[vsethi09]

Simply use the error message suggested previously:

Updates to JEXL allowlist in the wrangler directives config is not allowed.

No need to mention that it is disabled by the administrator.

[vsethi09]

Is this comment correct or misplaced?
Seems like this was checked above.

[riyaa14]

Yes, this is correct

[vsethi09]

What is this piece of code trying to do? Is there a better way to do so?

Can the parameterized constructor of DirectiveConfig with new getter functions be used?

[riyaa14]

We are trying to persist jexlAllowlist as it is before the feature was disabled. When feature is disabled, users don't provide jexlAllowlist field, and because the API is not additive, the payload with only exclusions and aliases makes jexlAllowlist field null (it gets removed). To persist this value, we want to add exclusions and aliases to existing config containing jexlAllowlist rather than overriding jexlAllowlist to null.

jexlAllowlist in this case stays in database, it doesn't gets used until feature flag in enabled again and users get the same jexlAllowlist they configured before.

[vsethi09]

Does this need to be public?

Also, these same constants are defined in JexlAllowlist class. Can they be reused?

[vsethi09]

Why is this annotation needed?

Why is new deprecated class being added?

[vsethi09]

Standard Java and Google naming conventions favor concise, descriptive identifiers over verbose "spoken-out" prepositions like OF, TO, and redundant suffixes like TYPE.

Suggestions:

private static final Type STRING_SET_TYPE = new TypeToken<Set<String>>() {}.getType();
private static final Type STRING_MAP_TYPE = new TypeToken<Map<String, String>>() {}.getType();
private static final Type JEXL_ALLOWLIST_LIST_TYPE = new TypeToken<List<JexlAllowlist>>() {}.getType();

https://cdap.atlassian.net/wiki/spaces/CE/pages/1594098623/Java+Coding+Standards

Google Coding Standards

[vsethi09]

Standard Java and Google naming conventions favor concise, descriptive identifiers over verbose "spoken-out" prepositions like OF, TO, and redundant suffixes like TYPE.

Suggestions:

private static final Type STRING_LIST_TYPE = new TypeToken<List<String>>() {}.getType();

https://cdap.atlassian.net/wiki/spaces/CE/pages/1594098623/Java+Coding+Standards

Google Coding Standards

[vsethi09]

Add one more test case with specific like of method and properties instead of * wildcard.

[vsethi09]

Using null checks with Java Optional<T> is bit of anti-pattern.

Simplify:

return row.map(r -> r.getString(VAL_COL))
          .map(str -> GSON.fromJson(str, DirectiveConfig.class))
          .orElse(null);