[
https://issues.apache.org/jira/browse/WW-5536?focusedWorklogId=997285&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-997285
]
ASF GitHub Bot logged work on WW-5536:
--------------------------------------
Author: ASF GitHub Bot
Created on: 21/Dec/25 16:56
Start Date: 21/Dec/25 16:56
Worklog Time Spent: 10m
Work Description: 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?
Issue Time Tracking
-------------------
Worklog Id: (was: 997285)
Time Spent: 1h 50m (was: 1h 40m)
> Bump ognl:ognl from 3.3.5 to 3.4.8
> ----------------------------------
>
> Key: WW-5536
> URL: https://issues.apache.org/jira/browse/WW-5536
> Project: Struts 2
> Issue Type: Dependency
> Components: Core
> Affects Versions: 6.7.0, 7.0.0
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.2.0
>
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
