Skip to content

[cherry-pick]][6.10]fix: handle JEXL security violations and fail pipeline immediately - #1057

Merged
riyaa14 merged 1 commit into
release/4.10from
cherry_fix/jexl-error-handling
Sep 25, 2026
Merged

riyaa14 merged 1 commit into
release/4.10from
cherry_fix/jexl-error-handling

Conversation

@sahusanket

Copy link
Copy Markdown
Contributor

original pr : #1056

Difference from develop (#1055)

  • WranglerErrorUtil.java & Wrangler.java: WranglerErrorUtil (introduced in 4.11/develop via f26ecd28) did not exist in release/4.10 because it relies on io.cdap.cdap.api.exception.ErrorUtils / ProgramFailureException, which are only available in CDAP 6.11+ (release/4.10 builds against CDAP 6.10.8-SNAPSHOT and throws a standard Exception on pipeline failure).
  • For release/4.10, WranglerErrorUtil is added with only isCriticalException(Throwable) (to check for ELPermissionException in the causal chain), and Wrangler.java continues to throw the existing Exception when shouldFailPipeline(e) evaluates to true.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces ELPermissionException to handle JEXL sandbox permission violations, ensuring that critical sandbox violations fail the pipeline immediately regardless of the configured on-error strategy. It updates exception handling in EL and Wrangler, and adds helper utilities and unit tests. The review feedback suggests restoring a helpful hint to the ELPermissionException error message to assist users in diagnosing sandbox violations, and replacing the use of Throwables.getCausalChain in WranglerErrorUtil with a safer, more efficient iterative loop to avoid deprecation issues and potential crashes from circular causal chains.

Comment on lines +200 to +202
if (isJexlPermissionViolation(e, allowlistEnabled)) {
throw new ELPermissionException(baseErrorMessage, e);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The error message for ELPermissionException has lost the helpful hint regarding allowlisted classes, methods, and properties. Without this context, users encountering sandbox violations will only see a generic evaluation error, making it difficult to diagnose permission issues. Adding the descriptive suffix back for permission violations restores this critical context.

Suggested change
if (isJexlPermissionViolation(e, allowlistEnabled)) {
throw new ELPermissionException(baseErrorMessage, e);
}
if (isJexlPermissionViolation(e, allowlistEnabled)) {
throw new ELPermissionException(
baseErrorMessage + " Make sure the JEXL transformation is valid and uses only allowlisted classes, methods, and properties.", e);
}

Comment on lines +46 to +49
public static boolean isCriticalException(Throwable e) {
return e != null && Throwables.getCausalChain(e).stream()
.anyMatch(ELPermissionException.class::isInstance);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using Throwables.getCausalChain(e) is inefficient as it builds a full list of throwables and creates stream overhead on every exception check. Additionally, Throwables.getCausalChain is deprecated in newer Guava versions and can throw an IllegalArgumentException if there is a loop in the causal chain, which would crash the pipeline with an unhandled exception. A simple iterative loop is more efficient, avoids memory allocation, and is safer.

  public static boolean isCriticalException(Throwable e) {
    Throwable cause = e;
    while (cause != null) {
      if (cause instanceof ELPermissionException) {
        return true;
      }
      cause = cause.getCause();
    }
    return false;
  }

@riyaa14
riyaa14 self-requested a review September 25, 2026 04:46
@riyaa14
riyaa14 merged commit e76aa0b into release/4.10 Sep 25, 2026
2 checks passed
@riyaa14
riyaa14 deleted the cherry_fix/jexl-error-handling branch September 25, 2026 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants