lukaszlenart commented on PR #1815:
URL: https://github.com/apache/struts/pull/1815#issuecomment-5092389995
Thanks for the rework — I read it properly and it's a real step up. You
independently found several things that took my side multiple review rounds to
catch:
- the `ConditionalInterceptor` interaction. `AbstractInterceptor` implements
`ConditionalInterceptor`, so a lazy interceptor reaches `intercept` through
`executeConditional`, and leaving the two branches as siblings silently voids
the dynamic policy. You merged them.
- the shared `InterceptorMapping` param map, now copied rather than mutated
per request — and with `LinkedHashMap`, preserving configuration order.
- the abstract-method problem, handled correctly with the explicit
`WithLazyParams.InvocationScoped.super.intercept(invocation)` delegation. An
interface `default` alone would not have satisfied `AbstractInterceptor`'s
abstract declaration.
- **the OGNL allowlist.** `ce7c3fcaa` is the one that impressed me. Moving
the write target off the interceptor means the new holder isn't allowlisted,
and with `struts.allowlist.enable=true` — the shipped default — every upload
gets rejected. I hit exactly the same trap and it broke CI on every job in
#1816; the showcase integration tests were the only thing that caught it. You
found it yourself and your `ThreadAllowlist` route is sound
(`ModelDrivenInterceptor` sets the precedent, and `java.lang.Object` is
excluded independently at `SecurityMemberAccess:86`, so the hierarchy walk
doesn't widen anything).
## Where this is going
I'm going to land #1816 rather than this PR, and I want to be straight with
you about why, because your design had one genuine advantage over mine.
Your `WithLazyParams.InvocationScoped<P>` is an *optional* sub-interface, so
`WithLazyParams` stays source-compatible and no third-party implementer breaks.
That's better than my approach on that axis, and it would have allowed a 7.2.x
patch for a bug that's live in 7.2.0 and 7.2.1. I weighed that seriously.
What settles it is release planning rather than design: 7.3.0 already
carries other compatibility-breaking changes, so there is no 7.2.2 to target.
Once the fix ships in a version that breaks compatibility anyway, the
compat-preserving shape stops buying anything, and the tradeoff inverts — a
single mandatory contract is easier to reason about than a mandatory one plus
an optional one, and it removes the unsafe path instead of letting implementers
opt out of it. In your version the legacy `WithLazyParams` branch still writes
to the interceptor, so the race remains for anyone who doesn't adopt
`InvocationScoped`.
Alongside that, #1816 carries work this PR doesn't: fail-closed handling
when an expression can't be resolved (previously an unresolvable `${...}`
produced an empty allowlist, which reads as "no restriction" — validation
silently off), configuration-time validation of param names so a typo fails at
startup instead of rejecting uploads in production, and `InterceptorParams
extends Serializable` because `Interceptor` is.
## One bug worth knowing about, wherever it lands
In this PR a lazily resolved `disabled` is silently ignored.
`UploadValidationPolicy` has no `disabled` property, `filterFactoryTimeParams`
withholds it from the interceptor at build time, and `ognlUtil.setProperty`
without `throwPropertyExceptions` no-ops rather than complaining — so `<param
name="disabled">${expr}</param>` does nothing at all and `shouldIntercept`
reads the interceptor's permanently-`false` field. Before your change it was
racy but functional; after, it's quietly dead. That's what pushed me toward
making the holder's `disabled` support explicit (`DisableParams`) and
validating the param names at configuration time.
## What I've taken from your PR
`filterFactoryTimeParams` is a better idea than what I had, and it's now in
#1816 as `83d2b61dd` with attribution to you and a `Co-Authored-By` trailer. My
version seeded the interceptor with the raw `${...}` literal, so `allowedTypes`
held the string `"${uploadConfig.allowedMimeTypes}"` — meaningless as a policy
value — and `maximumSize` failed conversion outright, since a `Long` can't hold
that text. Withholding those params at build time is simply correct, and it
also makes the "what value survives a failed resolution" documentation honest
instead of hand-wavy.
Your concurrency regression test from the first round is also in #1816,
carried over with attribution — the scenario it pins is the acceptance
criterion for the whole change, and reverting the fix still makes it fail
exactly as the bug describes.
So: I'm closing this one, but two pieces of it ship, and you're credited on
the ticket and in the commits. The allowlist catch in particular was
independent work on a trap that had already bitten me — thank you for it.
One process note, same as last time and still worth repeating: when a change
touches upload validation, parameter filtering, or OGNL access, please email
[email protected] before opening a public PR. `SECURITY.md` covers the
reasoning. This one is a thread-safety defect rather than a vulnerability — it
needs the opt-in `${...}` config, and `struts-default.xml:60` ships
`actionFileUpload` with no params — which is why the discussion stayed here.
--
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]
