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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
