kusalk commented on code in PR #1405:
URL: https://github.com/apache/struts/pull/1405#discussion_r2637949160
##########
core/src/main/java/org/apache/struts2/ognl/StrutsContext.java:
##########
Review Comment:
What purpose does this class serve? Seems to be unused
##########
core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java:
##########
@@ -41,40 +40,40 @@
*/
public class XWorkMethodAccessor extends ObjectMethodAccessor {
- private static final Logger LOG =
LogManager.getLogger(XWorkMethodAccessor.class);
+ private static final Logger LOG =
LogManager.getLogger(XWorkMethodAccessor.class);
@Override
- public Object callMethod(Map context, Object object, String string,
Object[] objects) throws MethodFailedException {
+ public Object callMethod(OgnlContext context, Object object, String
string, Object[] objects) throws MethodFailedException {
//Collection property accessing
//this if statement ensures that ognl
//statements of the form someBean.mySet('keyPropVal')
//return the set element with value of the keyProp given
- if (objects.length == 1 && context instanceof OgnlContext) {
+ if (objects.length == 1) {
try {
- OgnlContext ogContext=(OgnlContext)context;
- if (OgnlRuntime.hasSetProperty(ogContext, object, string)) {
- PropertyDescriptor
descriptor=OgnlRuntime.getPropertyDescriptor(object.getClass(), string);
- Class propertyType=descriptor.getPropertyType();
- if ((Collection.class).isAssignableFrom(propertyType)) {
- //go directly through OgnlRuntime here
- //so that property strings are not cleared
- //i.e. OgnlUtil should be used initially,
OgnlRuntime
- //thereafter
+ OgnlContext ogContext = context;
Review Comment:
Nit: Unnecessary assignment
##########
core/src/test/java/org/apache/struts2/ognl/OgnlUtilTest.java:
##########
@@ -1647,21 +1646,46 @@ public void testOgnlDefaultCacheFactoryCoverage() {
assertEquals("Eviction limit for cache mismatches limit for factory
?", 15, ognlCache.getEvictionLimit());
}
- public void testCustomOgnlMapBlocked() throws Exception {
- String vulnerableExpr =
"#@org.apache.struts2.ognl.MyCustomMap@{}.get(\"ye\")";
- assertEquals("System compromised", ognlUtil.getValue(vulnerableExpr,
ognlUtil.createDefaultContext(null), null));
+ public void testAllowCustomOgnlMap() throws Exception {
+ String vulnerableExpr = "#@org.test.MyCustomMap@{}.get(\"ye\")";
+ Object result = ognlUtil.getValue(vulnerableExpr,
ognlUtil.createDefaultContext(null), null);
+ assertNull("System compromised", result);
Review Comment:
It shouldn't be `null` when custom map impls are allowed. `System
compromised` wasn't the assertion message, it's the literal value expected and
returned by `MyCustomMap`.
Does the new OGNL not support custom map implementations at all? This is
better from a security standpoint, but will break applications that rely on
this behaviour
##########
pom.xml:
##########
Review Comment:
This PR breaks API compatibility with a number of extension points. To abide
by SemVer we would bump the SNAPSHOT version by a major. Otherwise, bump by at
least a minor and call out the breaking changes in the release notes
##########
core/src/main/java/org/apache/struts2/ognl/XWorkTypeConverterWrapper.java:
##########
@@ -36,6 +37,11 @@ public XWorkTypeConverterWrapper(ognl.TypeConverter conv) {
@Override
public Object convertValue(Map context, Object target, Member member,
String propertyName, Object value, Class toType) {
- return typeConverter.convertValue(context, target, member,
propertyName, value, toType);
+ // Cast context to OgnlContext for OGNL 3.4.8+ compatibility
+ OgnlContext ognlContext = (context instanceof OgnlContext oc) ? oc :
null;
+ if (ognlContext == null) {
+ throw new IllegalArgumentException("Context must be an OgnlContext
for OGNL 3.4.8+");
+ }
+ return typeConverter.convertValue(ognlContext, target, member,
propertyName, value, toType);
Review Comment:
Slightly cleaner (remove unnecessary ternary)
```suggestion
if (!(context instanceof OgnlContext ognlContext)) {
throw new IllegalArgumentException("Context must be an
OgnlContext for OGNL 3.4.8+");
}
return typeConverter.convertValue(ognlContext, target, member,
propertyName, value, toType);
```
##########
core/src/main/java/org/apache/struts2/ognl/OgnlUtil.java:
##########
@@ -198,13 +197,32 @@ public void clearBeanInfoCache() {
* Check the size of the BeanInfo cache (current number of elements).
*
* @return current number of elements in the BeanInfo cache.
- *
* @since 2.5.21
*/
public int beanInfoCacheSize() {
return beanInfoCache.size();
}
+ /**
+ * Ensures that the given context is an OgnlContext. If it's already an
OgnlContext, returns it as-is.
+ * If it's a plain Map (like HashMap), wraps it in an OgnlContext to
ensure compatibility with OGNL 3.4.8+.
+ *
+ * @param context the context map that may or may not be an OgnlContext
+ * @return an OgnlContext instance
+ * @since 7.2.0
+ */
+ private OgnlContext ensureOgnlContext(Map<String, Object> context) {
+ if (context instanceof OgnlContext ognlContext) {
+ return ognlContext;
+ }
+ // Create a new OgnlContext and copy the Map contents
+ OgnlContext ognlContext = createDefaultContext(null);
+ if (context != null) {
Review Comment:
Is this null check necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
