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"/>

Normal Tiles startup no longer uses DispatcherListener. The no-argument factory registers a lazy configured OGNL: evaluator. On its first evaluation, the evaluator obtains the Dispatcher from the current request's ServletContext, reads struts.tiles.ognl.legacy.enabled, and caches the resulting delegate for that evaluator instance.

A false or missing flag, missing Dispatcher, or non-servlet Request fails closed with EvaluationException. Only an explicit true constructs the raw evaluator and installs the global OgnlRuntime Tiles Request property accessor. Legacy mode preserves the existing behavior and emits one migration warning when first used. Public boolean constructors support custom initializers that need to select disabled or legacy behavior explicitly.

The compatibility constant and legacy evaluator are deprecated in Struts 7.4.0 and targeted for removal in Struts 8.0.0.

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 (561 tests, all passing)
• ran the complete Tiles module verify lifecycle (561 tests, all passing), including Apache RAT and JAR packaging
• tested per-webapp isolation, one-time cached configuration resolution, non-servlet and missing-Dispatcher handling, construction gating, concurrent first evaluation, explicit public constructors, and real servlet-backed S2: evaluation
• inspected compiled bytecode to confirm the normal listener contains no DispatcherListener integration and raw evaluator construction remains unreachable until explicit true is established
• ran the default and Jakarta EE 11 reactors; all modules through Tiles passed, after which Showcase encountered two external gitbox.apache.org download timeouts

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.

[lukaszlenart]

Sorry for the delay — yes, that direction addresses all of it. Please go ahead.

Resolving the Dispatcher from the current request's ServletContext is the right key: Dispatcher.getInstance(ServletContext) (Dispatcher.java:280) reads the SERVLET_DISPATCHER attribute, which is per-webapp and cleared on cleanup, so no static state is involved and a neighbouring application can't opt this one in. Constructing the raw evaluator lazily and only after that check keeps the OgnlRuntime.setPropertyAccessor(Request.class, ...) mutation off the default path, which was the main thing. Public constructors for the explicit opt-in cover the custom-initializer case.

Two small things while you're in there:

• Resolve the flag once per evaluator instance and hold the result, rather than looking it up from the container on every evaluation. The constant can't change at runtime, and there is one evaluator per Tiles container, so caching the decision is safe and avoids a container lookup per rendered attribute.
• ServletUtil.getServletRequest(request) throws NotAServletEnvironmentException for a non-servlet Request. Treat that the same as "no Dispatcher": fail closed with the migration message, not a stack trace from the lookup.

A separate PR for support/struts-6-x-x afterwards is fine — plain @Deprecated there, as discussed.

[kuranikaran]

Implemented the approved redesign in 917ce740.

The normal Tiles bootstrap no longer uses DispatcherListener. On the first OGNL: evaluation, the evaluator resolves the Dispatcher from that request’s ServletContext, reads struts.tiles.ognl.legacy.enabled once, and caches the resulting delegate for its lifetime.

A missing Dispatcher, false or missing flag, or non-servlet request now fails closed with the migration EvaluationException. The raw evaluator and global Request accessor are constructed only after an application-local explicit opt-in.

The public boolean constructors remain available for custom initializers. Tiles validation passes with 561/561 tests, including per-webapp isolation, cached resolution, non-servlet handling, construction gating, concurrency, and unchanged S2: behavior.

The new GitHub Actions runs are awaiting repository approval.