[
https://issues.apache.org/jira/browse/WW-5662?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5662:
------------------------------
Fix Version/s: 7.4.0
(was: 7.3.0)
> 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
> Priority: Major
> Fix For: 7.4.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)
