Lukasz Lenart created WW-5662:
---------------------------------
Summary: Core tests run with the OGNL allowlist disabled, hiding a
class of regression from CI
Key: WW-5662
URL: https://issues.apache.org/jira/browse/WW-5662
Project: Struts 2
Issue Type: Improvement
Reporter: Lukasz Lenart
Fix For: 7.3.0
h3. Problem
{{struts.allowlist.enable}} ships as {{true}}
({{core/src/main/resources/org/apache/struts2/default.properties:261}}), but
every test that
boots a {{Dispatcher}} through the shared helper gets it forced to {{false}}:
{code:java}
// StrutsTestCaseHelper.java:41-44
public static Dispatcher initDispatcher(ServletContext ctx, Map<String, String>
params) {
Map<String, String> finalParams = params != null ? new HashMap<>(params) :
new HashMap<>();
finalParams.putIfAbsent(STRUTS_ALLOWLIST_ENABLE, "false");
...
}
{code}
The result is that the core suite - 3104 tests - validates the framework under
a configuration
that no production deployment uses. Exactly two test classes in the whole
repository opt back
in: {{OgnlUtilTest}} and {{LazyParamsAllowlistTest}}.
Any framework code path that writes to an object via OGNL is therefore untested
against the
allowlist. If the target class is not allowlisted, the write is blocked at
runtime and no core
test can observe it.
h3. Concrete incident
WW-5659 moved lazily resolved interceptor params off the shared interceptor
singleton and into
a per-invocation holder ({{UploadPolicy}}). The old write target - the
interceptor - is
allowlisted at configuration time via
{{XmlDocConfigurationProvider.verifyInterceptor}}; the
new holder was registered nowhere.
Consequences under the shipped default:
* {{SecurityMemberAccess}} blocked the setter
* {{OgnlUtil}} threw {{ReflectionException}}
* the fail-closed path marked the policy unusable
* *every file upload was rejected*
The full core suite passed - 3104 tests, zero failures - because the allowlist
was off. The
break was caught by {{it.org.apache.struts2.showcase.DynamicFileUploadTest}} (4
of 7 tests
failing), after it had already failed every "Build and Test" job on the pull
request. The
showcase integration tests were the only thing standing between this and a
release.
h3. Why the default is off, and why that is not simply reversible
Turning the flag on for the existing suite would require allowlisting every
test fixture class
reachable from an OGNL expression - {{TestBean}}, {{TestBean2}} and many
others. That is
presumably why it was disabled wholesale in the first place. A naive flip would
produce a large
volume of unrelated failures, so this needs a deliberate approach rather than a
one-line change.
h3. Options
Worth weighing during design; not a settled decision.
# *A dedicated allowlist-enabled slice.* Keep the default off, and add a test
base class (or
surefire execution) that boots with the allowlist enabled, used by any test
covering a code
path that writes to a framework-internal object via OGNL. Cheapest, and
matches what
{{LazyParamsAllowlistTest}} already does by hand.
# *Flip the default in {{StrutsTestCaseHelper}} and fix the fallout.* Highest
value - it makes
the whole suite exercise the shipped configuration - and the largest effort.
Could be staged
package by package.
# *Treat the showcase integration tests as the canary and extend them.* Weakest
option: it
catches breakage late, and only for features the showcase happens to
demonstrate.
Option 1 plus a documented convention ("if your change writes via OGNL to a
framework class,
add an allowlist-enabled test") would have caught the WW-5659 regression at the
point it was
introduced.
h3. Non-goals
* Not a change to the shipped {{struts.allowlist.enable}} default, which is
already {{true}}
and correct.
* Not a rewrite of the allowlist mechanism itself.
h3. References
* {{StrutsTestCaseHelper.java:41-44}} - where the flag is forced off
* {{core/src/main/resources/org/apache/struts2/default.properties:261}} - the
shipped default
*
{{core/src/test/java/org/apache/struts2/interceptor/LazyParamsAllowlistTest.java}}
- the
pattern a slice could generalise
* WW-5659 - the regression that motivated this issue
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
