WW-5713 Fail closed for legacy Tiles OGNL evaluation

[kuranikaran]

Summary

The Tiles plugin registers separate S2: and legacy OGNL: attribute-expression evaluators. The legacy evaluator operates directly against the Tiles Request and does not use the Struts OGNL controls used by S2:.

This change keeps OGNL: registered but makes it fail closed by default. Evaluation throws Tiles EvaluationException with migration guidance and does not parse or evaluate the expression.

Applications that temporarily require the existing raw behavior can explicitly set:

<constant name="struts.tiles.ognl.legacy.enabled" value="true"/>

The compatibility constant and legacy evaluator are deprecated in Struts 7.4.0 and targeted for removal in Struts 8.0.0. The raw evaluator and its global OgnlRuntime Tiles Request property-accessor setup are constructed only when legacy mode is explicitly enabled. Legacy mode preserves the existing behavior and emits one startup/container-construction warning.

S2:, I18N:, and EL: remain unchanged.

Testing

• compiled the Tiles plugin
• ran the focused evaluator, factory, listener-lifecycle, integration, and legacy compatibility tests in both class orders
• ran the complete Tiles module test suite (558 tests)
• ran the complete Tiles module verify lifecycle, including Apache RAT and JAR packaging
• inspected compiled bytecode to confirm the disabled evaluator contains no OGNL reference and the default factory branch does not construct the raw evaluator

Classification

This is defense-in-depth hardening. No attacker-controlled expression source or concrete vulnerability impact has been demonstrated, and no security advisory is proposed.

Issue: WW-5713

[lukaszlenart]

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 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.

[kuranikaran]

Thanks for the detailed review. Your shared-classloader concerns make sense, especially the possibility of one application’s Dispatcher lifecycle or legacy setting affecting another application. I agree that the current DispatcherListener wiring should be removed.

Before rewriting it, I propose this approach:

• Restore StrutsTilesListener and the normal Tiles bootstrap lifecycle to their existing main-branch behavior, with no DispatcherListener registration.
• Keep OGNL: registered with a lazy gating evaluator.
• At evaluation time, derive the ServletContext from the current Tiles Request/ApplicationContext and resolve only that context’s Dispatcher.
• If its struts.tiles.ognl.legacy.enabled value is absent or false, throw EvaluationException before constructing the raw evaluator or changing the global Request property accessor.
• If it is explicitly true, construct/cache the legacy evaluator and emit the migration warning once.
• Continue checking the flag against the current request’s ServletContext so another webapp cannot opt this one into legacy behavior.
• If no Dispatcher exists, fail closed by default.
• Make the explicit boolean constructors public for applications using their own StrutsTilesInitializer or StrutsTilesContainerFactory, including direct Tiles setups without a Dispatcher.

This should preserve the original bootstrap behavior, remove the static cross-application lifecycle dependency, and ensure raw OGNL construction remains reachable only through an explicit application-local opt-in.

Would this direction address your concerns before I revise the patch?

I’m also happy to prepare the support/struts-6-x-x backport as a separate PR after we settle and merge the main-line design, using plain @deprecated as noted.