[
https://issues.apache.org/jira/browse/WW-5364?focusedWorklogId=893745&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-893745
]
ASF GitHub Bot logged work on WW-5364:
--------------------------------------
Author: ASF GitHub Bot
Created on: 04/Dec/23 11:44
Start Date: 04/Dec/23 11:44
Worklog Time Spent: 10m
Work Description: kusalk commented on code in PR #800:
URL: https://github.com/apache/struts/pull/800#discussion_r1413740321
##########
core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java:
##########
Review Comment:
Moved this here so that we can utilise it for tests in core
##########
apps/showcase/src/main/resources/struts.xml:
##########
@@ -34,6 +34,19 @@
<constant name="struts.custom.i18n.resources" value="globalMessages" />
<constant name="struts.action.extension" value="action,," />
+ <constant name="struts.allowlist.enable" value="true" />
Review Comment:
It works!
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -65,12 +80,14 @@ public class SecurityMemberAccess implements MemberAccess {
private Set<String> excludedPackageNames = emptySet();
private Set<String> excludedPackageExemptClasses = emptySet();
private boolean enforceAllowlistEnabled = false;
- private Set<String> allowlistClasses = emptySet();
+ private Set<Class<?>> allowlistClasses = emptySet();
Review Comment:
While we use `String` comparison for the exclusion list, we use `Class`
comparison for the allowlist to prevent accidentally allowing through another
Class with the same name (in applications with multiple classloaders)
##########
core/src/main/resources/struts-beans.xml:
##########
@@ -169,6 +169,7 @@
<bean name="struts"
class="com.opensymphony.xwork2.ognl.SecurityMemberAccess" scope="prototype"/>
<bean type="org.apache.struts2.ognl.OgnlGuard" name="struts"
class="org.apache.struts2.ognl.StrutsOgnlGuard"/>
+ <bean class="org.apache.struts2.ognl.ProviderAllowlist"/>
Review Comment:
No extension point - can't see any use for one
##########
plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java:
##########
@@ -18,74 +18,5 @@
*/
package org.apache.struts2.junit;
-import com.opensymphony.xwork2.ActionProxyFactory;
-import com.opensymphony.xwork2.config.Configuration;
-import com.opensymphony.xwork2.config.ConfigurationException;
-import com.opensymphony.xwork2.config.ConfigurationManager;
-import com.opensymphony.xwork2.config.ConfigurationProvider;
-import com.opensymphony.xwork2.inject.Container;
-import com.opensymphony.xwork2.inject.ContainerBuilder;
-import com.opensymphony.xwork2.inject.Context;
-import com.opensymphony.xwork2.inject.Factory;
-import com.opensymphony.xwork2.inject.Scope;
-import com.opensymphony.xwork2.test.StubConfigurationProvider;
-import com.opensymphony.xwork2.util.XWorkTestCaseHelper;
-import com.opensymphony.xwork2.util.location.LocatableProperties;
-import org.junit.After;
-import org.junit.Before;
-
-public abstract class XWorkJUnit4TestCase {
-
- protected ConfigurationManager configurationManager;
- protected Configuration configuration;
- protected Container container;
- protected ActionProxyFactory actionProxyFactory;
-
- @Before
- public void setUp() throws Exception {
- configurationManager = XWorkTestCaseHelper.setUp();
- configuration = configurationManager.getConfiguration();
- container = configuration.getContainer();
- actionProxyFactory = container.getInstance(ActionProxyFactory.class);
- }
-
- @After
- public void tearDown() throws Exception {
- XWorkTestCaseHelper.tearDown(configurationManager);
- configurationManager = null;
- configuration = null;
- container = null;
- actionProxyFactory = null;
- }
-
- protected void loadConfigurationProviders(ConfigurationProvider...
providers) {
- configurationManager =
XWorkTestCaseHelper.loadConfigurationProviders(configurationManager, providers);
- configuration = configurationManager.getConfiguration();
- container = configuration.getContainer();
- actionProxyFactory = container.getInstance(ActionProxyFactory.class);
- }
-
- protected void loadButAdd(final Class<?> type, final Object impl) {
- loadButAdd(type, Container.DEFAULT_NAME, impl);
- }
-
- protected void loadButAdd(final Class<?> type, final String name, final
Object impl) {
- loadConfigurationProviders(new StubConfigurationProvider() {
- @Override
- public void register(ContainerBuilder builder,
- LocatableProperties props) throws
ConfigurationException {
- builder.factory(type, name, new Factory() {
- public Object create(Context context) throws Exception {
- return impl;
- }
-
- @Override
- public Class type() {
- return impl.getClass();
- }
- }, Scope.SINGLETON);
- }
- });
- }
-
+public abstract class XWorkJUnit4TestCase extends
com.opensymphony.xwork2.XWorkJUnit4TestCase {
Review Comment:
Extended the moved class for compat
##########
core/src/main/resources/struts-default.xml:
##########
@@ -44,8 +44,6 @@
<interceptors>
<interceptor name="alias"
class="com.opensymphony.xwork2.interceptor.AliasInterceptor"/>
- <interceptor name="autowiring"
Review Comment:
This class isn't provided by core
##########
plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java:
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.junit;
+
+public abstract class XWorkTestCase extends
com.opensymphony.xwork2.XWorkTestCase {
Review Comment:
Added this for consistency
##########
core/src/main/java/com/opensymphony/xwork2/config/providers/XmlDocConfigurationProvider.java:
##########
Review Comment:
Made some changes to this file to attempt to load every class defined in a
`package` and add it to the allowlist
##########
core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java:
##########
@@ -395,6 +359,57 @@ protected Container
createBootstrapContainer(List<ContainerProvider> providers)
return builder.create(true);
}
+ public static ContainerBuilder bootstrapFactories(ContainerBuilder
builder) {
Review Comment:
Extracted code into this method for reuse by the
`StrutsDefaultConfigurationProvider`
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -57,6 +59,19 @@ public class SecurityMemberAccess implements MemberAccess {
private static final Logger LOG =
LogManager.getLogger(SecurityMemberAccess.class);
+ private static final Set<String> ALLOWLIST_REQUIRED_PACKAGES =
unmodifiableSet(new HashSet<>(Arrays.asList(
Review Comment:
Required system allowlist defined here - it's possible I've missed some
packages/classes, can add as needed
##########
core/src/test/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package com.opensymphony.xwork2.config.providers;
+
+import com.opensymphony.xwork2.XWorkJUnit4TestCase;
+import com.opensymphony.xwork2.config.ConfigurationProvider;
+import org.apache.struts2.config.StrutsXmlConfigurationProvider;
+import org.apache.struts2.ognl.ProviderAllowlist;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ConfigurationProviderOgnlAllowlistTest extends
XWorkJUnit4TestCase {
+
+ private final ConfigurationProvider testXml1 = new
StrutsXmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowlist.xml");
+ private final ConfigurationProvider testXml2 = new
StrutsXmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowlist-2.xml");
+ private ProviderAllowlist providerAllowlist;
+
+ @Before
+ public void setUp() throws Exception {
+ loadConfigurationProviders(testXml1, testXml2);
+ providerAllowlist = container.getInstance(ProviderAllowlist.class);
+ }
+
+ @Test
+ public void allowlist() throws Exception {
+ loadConfigurationProviders(testXml1, testXml2);
+ providerAllowlist = container.getInstance(ProviderAllowlist.class);
+
+
assertThat(providerAllowlist.getProviderAllowlist()).containsExactlyInAnyOrder(
+
Class.forName("com.opensymphony.xwork2.interceptor.ValidationAware"),
+ Class.forName("com.opensymphony.xwork2.LocaleProvider"),
+ Class.forName("java.io.Serializable"),
+ Class.forName("com.opensymphony.xwork2.mock.MockResult"),
+
Class.forName("com.opensymphony.xwork2.interceptor.ConditionalInterceptor"),
+ Class.forName("com.opensymphony.xwork2.ActionSupport"),
+ Class.forName("com.opensymphony.xwork2.ActionChainResult"),
+ Class.forName("com.opensymphony.xwork2.TextProvider"),
+
Class.forName("org.apache.struts2.interceptor.NoOpInterceptor"),
+
Class.forName("com.opensymphony.xwork2.interceptor.Interceptor"),
+ Class.forName("java.lang.Object"),
Review Comment:
Even though `java.lang.Object` is added to the allowlist as it is a
superclass, the exclusion list always takes precedence and so this class will
not be allowed in practice
Issue Time Tracking
-------------------
Worklog Id: (was: 893745)
Time Spent: 1h 40m (was: 1.5h)
> Automatically populate OGNL allowlist
> -------------------------------------
>
> Key: WW-5364
> URL: https://issues.apache.org/jira/browse/WW-5364
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Kusal Kithul-Godage
> Priority: Minor
> Fix For: 6.4.0
>
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
