Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WW-5340 Preliminary refactor of OgnlUtil #746

Merged
merged 4 commits into from
Sep 26, 2023
Merged

WW-5340 Preliminary refactor of OgnlUtil #746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
452cb77
WW-5340 Refactor OgnlUtil, specifically calls to Ognl#getValue,setVal…
kusalk Aug 31, 2023
13f0591
WW-5340 Remove redundant check on #setValue
kusalk Aug 31, 2023
65ff242
WW-5340 Rename functional interface
kusalk Aug 31, 2023
90adbfb
WW-5340 Fix OgnlReflectionProvider bypassing OgnlUtil
kusalk Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions core/src/main/java/com/opensymphony/xwork2/ognl/OgnlReflectionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.util.reflection.ReflectionException;
import com.opensymphony.xwork2.util.reflection.ReflectionProvider;
import ognl.Ognl;
import ognl.OgnlException;
import ognl.OgnlRuntime;

Expand All @@ -33,9 +32,9 @@
import java.util.Map;

public class OgnlReflectionProvider implements ReflectionProvider {

private OgnlUtil ognlUtil;

@Inject
public void setOgnlUtil(OgnlUtil ognlUtil) {
this.ognlUtil = ognlUtil;
Expand Down Expand Up @@ -69,7 +68,6 @@ public void setProperties(Map<String, ?> props, Object o, Map<String, Object> co

public void setProperties(Map<String, ?> props, Object o, Map<String, Object> context, boolean throwPropertyExceptions) throws ReflectionException{
ognlUtil.setProperties(props, o, context, throwPropertyExceptions);

}

public void setProperties(Map<String, ?> properties, Object o) {
Expand Down Expand Up @@ -134,7 +132,7 @@ public Object getValue(String expression, Map<String, Object> context, Object ro
public void setValue(String expression, Map<String, Object> context, Object root,
Object value) throws ReflectionException {
try {
Ognl.setValue(expression, context, root, value);
ognlUtil.setValue(expression, context, root, value);
} catch (OgnlException e) {
throw new ReflectionException(e);
}
Expand Down
99 changes: 47 additions & 52 deletions core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,8 +529,7 @@ public Object getRealTarget(String property, Map<String, Object> context, Object
}

/**
* Wrapper around Ognl.setValue() to handle type conversion for collection elements.
* Ideally, this should be handled by OGNL directly.
* Wrapper around Ognl#setValue
*
* @param name the name
* @param context context map
Expand All @@ -540,16 +539,7 @@ public Object getRealTarget(String property, Map<String, Object> context, Object
* @throws OgnlException in case of ognl errors
*/
public void setValue(final String name, final Map<String, Object> context, final Object root, final Object value) throws OgnlException {
compileAndExecute(name, context, (OgnlTask<Void>) tree -> {
if (isEvalExpression(tree, context)) {
throw new OgnlException("Eval expression/chained expressions cannot be used as parameter name");
}
if (isArithmeticExpression(tree, context)) {
throw new OgnlException("Arithmetic expressions cannot be used as parameter name");
}
Ognl.setValue(tree, context, root, value);
return null;
});
ognlSet(name, context, root, value, context, this::checkEvalExpression, this::checkArithmeticExpression);
}

private boolean isEvalExpression(Object tree, Map<String, Object> context) throws OgnlException {
Expand Down Expand Up @@ -592,58 +582,55 @@ private boolean isSimpleMethod(Object tree, Map<String, Object> context) throws
}

public Object getValue(final String name, final Map<String, Object> context, final Object root) throws OgnlException {
return compileAndExecute(name, context, tree -> Ognl.getValue(tree, context, root));
return getValue(name, context, root, null);

Check failure

Code scanning / CodeQL

OGNL Expression Language statement with user-controlled input

OGNL Expression Language statement depends on a [user-provided value](1). OGNL Expression Language statement depends on a [user-provided value](2). OGNL Expression Language statement depends on a [user-provided value](3). OGNL Expression Language statement depends on a [user-provided value](4). OGNL Expression Language statement depends on a [user-provided value](5). OGNL Expression Language statement depends on a [user-provided value](6). OGNL Expression Language statement depends on a [user-provided value](7). OGNL Expression Language statement depends on a [user-provided value](8). OGNL Expression Language statement depends on a [user-provided value](9). OGNL Expression Language statement depends on a [user-provided value](10). OGNL Expression Language statement depends on a [user-provided value](11). OGNL Expression Language statement depends on a [user-provided value](12).
}

public Object callMethod(final String name, final Map<String, Object> context, final Object root) throws OgnlException {
return compileAndExecuteMethod(name, context, tree -> Ognl.getValue(tree, context, root));
return ognlGet(name, context, root, null, context, this::checkSimpleMethod);
}

public Object getValue(final String name, final Map<String, Object> context, final Object root, final Class<?> resultType) throws OgnlException {
return compileAndExecute(name, context, tree -> Ognl.getValue(tree, context, root, resultType));
return ognlGet(name, context, root, resultType, context, this::checkEnableEvalExpression);
}


public Object compile(String expression) throws OgnlException {
return compile(expression, null);
}

private <T> Object compileAndExecute(String expression, Map<String, Object> context, OgnlTask<T> task) throws OgnlException {
Object tree;
if (enableExpressionCache) {
tree = expressionCache.get(expression);
if (tree == null) {
tree = Ognl.parseExpression(expression);
checkEnableEvalExpression(tree, context);
expressionCache.putIfAbsent(expression, tree);
}
} else {
tree = Ognl.parseExpression(expression);
checkEnableEvalExpression(tree, context);
private void ognlSet(String expr, Map<String, Object> context, Object root, Object value, Map<String, Object> checkContext, TreeValidator... treeValidators) throws OgnlException {
Object tree = toTree(expr);
for (TreeValidator validator : treeValidators) {
validator.validate(tree, checkContext);
}
Ognl.setValue(tree, context, root, value);

Check failure

Code scanning / CodeQL

OGNL Expression Language statement with user-controlled input

OGNL Expression Language statement depends on a [user-provided value](1). OGNL Expression Language statement depends on a [user-provided value](2). OGNL Expression Language statement depends on a [user-provided value](3).
}

return task.execute(tree);
private <T> T ognlGet(String expr, Map<String, Object> context, Object root, Class<T> resultType, Map<String, Object> checkContext, TreeValidator... treeValidators) throws OgnlException {
Object tree = toTree(expr);
for (TreeValidator validator : treeValidators) {
validator.validate(tree, checkContext);
}
return (T) Ognl.getValue(tree, context, root, resultType);

Check failure

Code scanning / CodeQL

OGNL Expression Language statement with user-controlled input

OGNL Expression Language statement depends on a [user-provided value](1). OGNL Expression Language statement depends on a [user-provided value](2). OGNL Expression Language statement depends on a [user-provided value](3). OGNL Expression Language statement depends on a [user-provided value](4). OGNL Expression Language statement depends on a [user-provided value](5). OGNL Expression Language statement depends on a [user-provided value](6). OGNL Expression Language statement depends on a [user-provided value](7). OGNL Expression Language statement depends on a [user-provided value](8). OGNL Expression Language statement depends on a [user-provided value](9). OGNL Expression Language statement depends on a [user-provided value](10). OGNL Expression Language statement depends on a [user-provided value](11). OGNL Expression Language statement depends on a [user-provided value](12).
}

private <T> Object compileAndExecuteMethod(String expression, Map<String, Object> context, OgnlTask<T> task) throws OgnlException {
Object tree;
private Object toTree(String expr) throws OgnlException {
Object tree = null;
if (enableExpressionCache) {
tree = expressionCache.get(expression);
if (tree == null) {
tree = Ognl.parseExpression(expression);
checkSimpleMethod(tree, context);
expressionCache.putIfAbsent(expression, tree);
tree = expressionCache.get(expr);
}
if (tree == null) {
tree = Ognl.parseExpression(expr);
if (enableExpressionCache) {
expressionCache.put(expr, tree);
}
} else {
tree = Ognl.parseExpression(expression);
checkSimpleMethod(tree, context);
}

return task.execute(tree);
return tree;
}

public Object compile(String expression, Map<String, Object> context) throws OgnlException {
return compileAndExecute(expression, context, tree -> tree);
Object tree = toTree(expression);
checkEnableEvalExpression(tree, context);
return tree;
}

private void checkEnableEvalExpression(Object tree, Map<String, Object> context) throws OgnlException {
Expand All @@ -658,6 +645,18 @@ private void checkSimpleMethod(Object tree, Map<String, Object> context) throws
}
}

private void checkEvalExpression(Object tree, Map<String, Object> context) throws OgnlException {
if (isEvalExpression(tree, context)) {
throw new OgnlException("Eval expression/chained expressions cannot be used as parameter name");
}
}

private void checkArithmeticExpression(Object tree, Map<String, Object> context) throws OgnlException {
if (isArithmeticExpression(tree, context)) {
throw new OgnlException("Arithmetic expressions cannot be used as parameter name");
}
}

/**
* Copies the properties in the object "from" and sets them in the object "to"
* using specified type converter, or {@link com.opensymphony.xwork2.conversion.impl.XWorkConverter} if none
Expand Down Expand Up @@ -732,12 +731,8 @@ public void copy(final Object from, final Object to, final Map<String, Object> c
PropertyDescriptor toPd = toPdHash.get(fromPd.getName());
if ((toPd != null) && (toPd.getWriteMethod() != null)) {
try {
compileAndExecute(fromPd.getName(), context, expr -> {
Object value = Ognl.getValue(expr, contextFrom, from);
Ognl.setValue(expr, contextTo, to, value);
return null;
});

Object value = ognlGet(fromPd.getName(), contextFrom, from, null, context, this::checkEnableEvalExpression);
ognlSet(fromPd.getName(), contextTo, to, value, context);
} catch (OgnlException e) {
LOG.debug("Got OGNL exception", e);
}
Expand Down Expand Up @@ -809,7 +804,7 @@ public Map<String, Object> getBeanMap(final Object source) throws IntrospectionE
final String propertyName = propertyDescriptor.getDisplayName();
Method readMethod = propertyDescriptor.getReadMethod();
if (readMethod != null) {
final Object value = compileAndExecute(propertyName, null, expr -> Ognl.getValue(expr, sourceMap, source));
final Object value = ognlGet(propertyName, sourceMap, source, null, null, this::checkEnableEvalExpression);
beanMap.put(propertyName, value);
} else {
beanMap.put(propertyName, "There is no read method for " + propertyName);
Expand Down Expand Up @@ -899,8 +894,8 @@ protected Map<String, Object> createDefaultContext(Object root, ClassResolver cl
return Ognl.createDefaultContext(root, memberAccess, resolver, defaultConverter);
}

private interface OgnlTask<T> {
T execute(Object tree) throws OgnlException;
@FunctionalInterface
private interface TreeValidator {
void validate(Object tree, Map<String, Object> context) throws OgnlException;
}

}
2 changes: 1 addition & 1 deletion core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1312,7 +1312,7 @@ public void testBlockSequenceOfExpressions() {
}
assertNotNull(expected);
assertSame(OgnlException.class, expected.getClass());
assertEquals(expected.getMessage(), "Eval expressions/chained expressions have been disabled!");
assertEquals("Eval expression/chained expressions cannot be used as parameter name", expected.getMessage());
}

public void testCallMethod() {
Expand Down