Repository navigation
[CDAP-21270] Handle JEXL security violations and fail pipeline immediately - #1055
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated ELSecurityException to handle JEXL sandbox security violations, ensuring that security violations immediately fail the pipeline regardless of the configured on-error strategy. It updates exception handling in EL, RecipePipelineExecutor, and Wrangler to propagate and detect these security exceptions, and adds corresponding unit tests. The review feedback highlights a bug where normal runtime exceptions wrapped in JEXL exceptions could be misclassified as security violations, suggesting a check for e.getCause() == null. Additionally, it is recommended to add a serialVersionUID to ELSecurityException to prevent serialization issues in distributed environments.
| private static boolean isJexlSecurityViolation(JexlException e, boolean allowlistEnabled) { | ||
| return allowlistEnabled | ||
| && (e instanceof JexlException.Method || e instanceof JexlException.Property); | ||
| } |
There was a problem hiding this comment.
If a method or property is successfully resolved (i.e., it is allowlisted) but its invocation throws a runtime exception (e.g., NullPointerException, IllegalArgumentException, etc.), JEXL wraps it in a JexlException.Method or JexlException.Property with a non-null cause.
Without checking e.getCause() == null, these normal runtime errors will be incorrectly classified as security violations, causing the pipeline to fail immediately and bypassing the configured onError strategy (such as send-to-error-port or skip-on-error). Adding e.getCause() == null ensures we only treat actual sandbox resolution blocks (which have no cause) as security violations.
| private static boolean isJexlSecurityViolation(JexlException e, boolean allowlistEnabled) { | |
| return allowlistEnabled | |
| && (e instanceof JexlException.Method || e instanceof JexlException.Property); | |
| } | |
| private static boolean isJexlSecurityViolation(JexlException e, boolean allowlistEnabled) { | |
| return allowlistEnabled | |
| && e.getCause() == null | |
| && (e instanceof JexlException.Method || e instanceof JexlException.Property); | |
| } |
| * Exception thrown when a JEXL expression violates security restrictions | ||
| * (e.g., accessing unallowlisted classes, methods, or properties). | ||
| */ | ||
| public class ELSecurityException extends RuntimeException { |
There was a problem hiding this comment.
Since ELSecurityException extends RuntimeException (which is Serializable) and is used in a distributed environment (CDAP pipelines running on MapReduce/Spark), it is highly recommended to define a serialVersionUID to prevent serialization/deserialization issues across JVM boundaries.
public class ELSecurityException extends RuntimeException {
private static final long serialVersionUID = 1L;
99dbc6d to
8c7a775
Compare
| if (isJexlPermissionViolation(e, allowlistEnabled)) { | ||
| throw new ELPermissionException( | ||
| String.format( | ||
| "Disallowed Expression: %s Make sure the JEXL transformation uses only allowlisted " |
There was a problem hiding this comment.
Remove this from message:
"Disallowed Expression: %s Make sure the JEXL transformation uses only allowlisted classes, methods, and properties.
Propagate the error details and let the user act on it by either allowlisting or fixing expression. The cause exception already has the details.
| } | ||
|
|
||
| return new ELException( | ||
| String.format("%s Make sure the JEXL transformation is valid.", baseErrorMessage), e); |
There was a problem hiding this comment.
Don't format and remove
Make sure the JEXL transformation is valid.
Propagate the error details and let the user act on it by either allowlisting or fixing expression. The cause exception already has the details.
8c7a775 to
5ea4399
Compare
No description provided.