[
https://issues.apache.org/jira/browse/WW-5713?focusedWorklogId=1039520&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1039520
]
ASF GitHub Bot logged work on WW-5713:
--------------------------------------
Author: ASF GitHub Bot
Created on: 03/Sep/26 18:24
Start Date: 03/Sep/26 18:24
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #1890:
URL: https://github.com/apache/struts/pull/1890#issuecomment-5530239495
Thanks for filing the ticket and the PR, and sorry for the slow turnaround.
The evaluator part is right, including the two things that are easy to get
wrong and that ruled out
both of the options we discussed earlier:
- it gates *construction* (`createConfiguredOgnlEvaluator`) rather than
`evaluate()`, so on the
default path we no longer perform the global
`OgnlRuntime.setPropertyAccessor(Request.class, ...)` mutation at all —
that was the main thing I
wanted and it's easy to miss;
- disabled mode throws `EvaluationException` instead of returning an empty
string, and the language
stays registered rather than being unregistered. Both alternatives fail
silently — an empty string
is indistinguishable from an empty attribute, and an unregistered language
falls through to
`DirectAttributeEvaluator` and renders `OGNL:...` as literal page text.
So the hardening itself I'm happy with. What I can't take as it stands is
the bootstrap change.
**The `StrutsTilesListener` / `DispatcherListener` bootstrap**
`Dispatcher.dispatcherListeners` is `private static final`
(`Dispatcher.java:126`) — one list per
classloader, not per web application, and `Dispatcher` clears it wholesale
at `:515`. Hooking the
Tiles lifecycle to it has three consequences in a shared-classloader
container:
- `StrutsTilesListener.java:73` — `dispatcherDestroyed` fires for *any*
Dispatcher, so undeploying
webapp A destroys webapp B's live Tiles container, and because the list is
cleared B can never
rebuild it.
- `StrutsTilesListener.java:92` — the `struts.tiles.ognl.legacy.enabled`
value is read from
whichever Dispatcher initialises first, which may belong to a different
application. A webapp that
never opted in can have the raw `OGNL:` evaluator switched back on by a
neighbour. That inverts the
control this PR adds, which is why it's the blocking one for me.
- `StrutsTilesListener.java:46` — if no Struts Dispatcher ever initialises,
Tiles is now never
initialised at all, where previously it was built at context startup.
That's a behaviour change for
setups that don't route through our Dispatcher.
Also `StrutsTilesInitializer.java:41`: both flag-carrying constructors are
package-private and only
the listener reads the constant, so an application using its own initializer
or factory — which the
factory's own javadoc suggests — can't reach the documented escape hatch and
gets no diagnostic.
None of this is you going off-piste. Getting the constant to
`createAttributeEvaluatorFactory` is
genuinely awkward: it's a Tiles `AbstractTilesContainerFactory` override
with no container access,
and container creation happens at application init when there may be no
`ActionContext`. I flagged
that as the only non-trivial part of the change when we discussed the
design, and this is where it
bites.
What I'd like instead is to decouple the two: keep the container bootstrap
exactly where it is today
and resolve the flag without adopting the Dispatcher lifecycle — reading the
constant lazily at
factory time or on first evaluation, or, if a listener really is needed,
keying it to its own
`ServletContext` identity so it ignores lifecycle events from other
applications. Happy to talk
through the options if you'd rather settle the approach before rewriting.
Two loose ends:
- The escape hatch needs a visible end date or it becomes permanent, so I've
filed
[WW-5714](https://issues.apache.org/jira/browse/WW-5714) for the removal
in 8.0.0 alongside your
WW-5713 deprecation in 7.4.0. Nothing for you to do there — it just means
the flag can't quietly
become permanent.
- Do you want `support/struts-6-x-x` as well? The same evaluator is there.
It would be plain
`@Deprecated` without `forRemoval`, since that branch is still on Java 8.
Credit is yours in the release notes either way.
Issue Time Tracking
-------------------
Worklog Id: (was: 1039520)
Time Spent: 20m (was: 10m)
> Fail closed for legacy Tiles OGNL evaluation
> --------------------------------------------
>
> Key: WW-5713
> URL: https://issues.apache.org/jira/browse/WW-5713
> Project: Struts 2
> Issue Type: Improvement
> Components: Plugin - Tiles
> Reporter: Karan Kurani
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The Struts Tiles plugin registers separate {{S2:}} and legacy {{OGNL:}}
> attribute-expression evaluators.
> The {{S2:}} evaluator processes ValueStack expressions through the Struts
> OGNL facilities. The legacy {{OGNL:}} evaluator evaluates against the Tiles
> {{Request}} and does not use the Struts OGNL security controls used by
> {{{}S2:{}}}.
> This hardening change keeps {{OGNL:}} registered but makes it fail closed by
> default. Evaluation throws Tiles {{EvaluationException}} with migration
> guidance directing users to {{{}S2:{}}}.
> Applications that temporarily require the existing raw behavior may
> explicitly set:
> {{struts.tiles.ognl.legacy.enabled=true}}
> The compatibility flag defaults to {{false}} and is targeted for removal in
> Struts 8.0.0.
> The raw evaluator and its global {{OgnlRuntime}} {{Request}}
> property-accessor setup are constructed only when legacy mode is explicitly
> enabled. This avoids mutating the shared OGNL runtime for applications using
> the secure default.
> When legacy mode is enabled, the existing raw behavior is preserved and a
> clear startup migration warning is emitted.
> {{{}S2:{}}}, {{{}I18N:{}}}, and {{EL:}} remain unchanged.
> Tests cover:
> * default fail-closed behavior;
> * exact migration guidance;
> * non-evaluation of expression markers;
> * construction gating;
> * absence of global accessor initialization under the default;
> * explicit legacy compatibility behavior;
> * startup warning behavior; and
> * real servlet-backed {{S2:}} evaluation.
> This is defense-in-depth hardening. No attacker-controlled expression source
> or concrete vulnerability impact has been demonstrated, and no security
> advisory is proposed.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
