lukaszlenart commented on PR #1815:
URL: https://github.com/apache/struts/pull/1815#issuecomment-5088015503
Thanks for this — the underlying problem is real and you diagnosed it
correctly. I traced it independently and it holds up:
`LazyParamInjector#injectParams` (`WithLazyParams.java:78-84`) writes
resolved values straight onto the interceptor, and that interceptor is a
config-time singleton (`InterceptorBuilder.java:73-74`, `:175-177`). It's worse
than same-action concurrency: `InterceptorBuilder.java:79` hands out the same
`InterceptorMapping` objects to every action that references a stack without
overriding params, so the instance is shared across actions too. Between
`DefaultActionInvocation.java:269` (resolve, write shared fields) and `:275` →
`acceptFile` (read shared fields) there is no guard at all. Good catch.
I don't want to merge this particular fix though, for a few reasons:
1. `static ThreadLocal PARAM_INJECTION_IN_PROGRESS` makes
`setAllowedTypes`/`setAllowedExtensions`/`setMaximumSize` behave differently
depending on hidden thread state. A setter whose target depends on an ambient
flag is hard to reason about and hard to test.
2. `clearRequestScopedUploadValidationPolicy()` is only called from
`ActionFileUploadInterceptor#intercept`'s `finally`. `WithLazyParams` is public
API — any other implementer that forgets that call leaks the `ThreadLocal`.
3. The unsafe write path still exists; it's conditionally bypassed rather
than removed. The next `WithLazyParams` implementer gets the original bug back
by default.
4. It doesn't cover `DefaultActionInvocation.java:262-267`, which does
`params.putAll(...)` on the live shared map returned by
`InterceptorMapping#getParams` (`:60-62`) — an unsynchronised write to a shared
`HashMap` on every request.
So rather than patch the one implementer, I'd like to fix the
`WithLazyParams` contract so the failure mode can't be expressed. The direction
is to have resolved params written into a per-invocation object that the
interceptor supplies and receives back, leaving the singleton immutable after
`init()`:
```java
public interface WithLazyParams<P> {
P newLazyParams();
String intercept(ActionInvocation invocation, P lazyParams) throws
Exception;
}
```
That keeps OGNL type conversion (the holder has typed setters, so
`maximumSize` still converts to `Long`), removes the mutation entirely, and
makes the data flow an explicit argument instead of ambient state. It's an
interface change, so it targets the next minor rather than 7.2.x.
I've filed [WW-5659](https://issues.apache.org/jira/browse/WW-5659) to track
it and I'm writing up the design now. Your regression test is the valuable part
of this PR — the scenario it pins down is exactly what the new design has to
keep passing, and I'd like to carry it over (with attribution) once the
interface lands. If you'd like to take the implementation on the new contract,
say so and it's yours; otherwise I'll pick it up and credit you on the ticket.
One process note for next time, not a criticism of this one: when a change
touches something that could weaken a security control — upload validation,
parameter filtering, OGNL access — please email [email protected]
before opening a public PR. `SECURITY.md` covers why. We looked at this one and
concluded it's a thread-safety defect rather than a vulnerability (it needs the
opt-in `${...}` config, and `struts-default.xml:60` ships `actionFileUpload`
with no params, so default setups write nothing), which is why it's fine to
keep the discussion 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]
