mirror of
https://github.com/apache/struts.git
synced 2026-08-31 19:35:40 +00:00
Lukasz Lenart
81264063e6
* WW-5659 docs: design for request-scoped lazy interceptor params
WithLazyParams#injectParams resolves ${...} params onto the interceptor
singleton, so concurrent requests can read one another's resolved values.
For ActionFileUploadInterceptor that means allowedTypes, allowedExtensions,
maximumSize and disabled can cross between requests.
Design fixes the contract rather than the one implementer: resolved params
go into a per-invocation holder the interceptor supplies and receives back,
leaving the singleton immutable after init(). Adds InterceptorParams as the
general contract with DisableParams as opt-in support for the disabled param,
and makes unresolvable expressions fail closed instead of silently disabling
validation.
Reported via GitHub PR #1815; that approach (ThreadLocal on the interceptor)
is not adopted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 docs: implementation plan for request-scoped lazy params
Six tasks, each independently testable and compiling: new InterceptorParams
and DisableParams types, LazyParamInjector.resolveInto alongside the old
path, a pure refactor onto a single UploadPolicy value object, the contract
switch plus DefaultActionInvocation wiring, fail-closed handling, and test
migration.
Records one deviation from the spec: the unresolved-param rule is applied
unconditionally rather than by introspecting the seeded value, which is not
implementable deterministically for the Long-typed maximumSize. Task 6
updates the spec to match.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 docs: clarify test base class constraint in the plan
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 feat(core): add InterceptorParams contract and DisableParams holder
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 feat(core): resolve lazy params into a holder instead of the interceptor
* WW-5659 docs(core): correct isUnresolved javadoc and pin empty-value fail-closed behavior
isUnresolved cannot distinguish a failed ${...} resolution from an expression that
legitimately evaluates to an empty string; the parser gives no other signal. The
previous javadoc wrongly claimed the raw template let it tell the two apart. Fix
the javadoc to state the actual, intentional rule (fail-closed: treat both as
unusable), and add a test pinning that a legitimately-empty expression is treated
as unresolved rather than written.
* WW-5659 refactor(core): hold upload policy in one value object
Introduce UploadPolicy (extends DisableParams) to consolidate the three
loose maximumSize/allowedTypes/allowedExtensions fields on
AbstractFileUploadInterceptor into a single config-time value object.
acceptFile now takes the effective policy as an explicit parameter
instead of reading interceptor-level state directly.
Pure refactor, no behaviour change: the existing setters still mutate
the shared singleton via configuredPolicy, and ActionFileUploadInterceptor
copies it once per invocation via copyConfiguredPolicy() before calling
acceptFile. This groundwork lets a later change route lazily-resolved
per-request params into the copy instead of the singleton.
* WW-5659 fix(core): resolve lazy interceptor params per invocation
Co-Authored-By: deprrous <sukhbatsuugii2004@gmail.com>
* WW-5659 test(core): cover both lazy-params skip branches and per-invocation disabled
The two skip branches in DefaultActionInvocation#invokeWithLazyParams had no
coverage: deleting either left the whole suite green. The only tests reaching
that method used LazyFoo/LazyFooWithStackParams, which declare no disabled
param, and MockLazyParams did not extend DisableParams, so the holder branch
was unreachable and shouldIntercept was always true.
Make MockLazyParams extend DisableParams and add two action configs that
isolate one branch each:
- LazyFooLazilyDisabled passes disabled as an interceptor-ref param, so it
reaches InterceptorMapping#getParams(), resolves onto the holder, and
exercises the DisableParams branch.
- LazyFooStaticallyDisabled sets disabled on the interceptor definition
instead. InterceptorBuilder only puts interceptor-ref params into the
mapping, so the holder never sees it and it can only be honoured through
ConditionalInterceptor#shouldIntercept.
Verified by deleting each branch in turn: each deletion fails exactly the one
test that targets it, and no other.
Also make testDisabledIsResolvedPerInvocation earn its name. It previously
asserted only that newLazyParams() returns a fresh object, never resolving
anything, and built a MyDynamicFileUploadAction it never used. It now routes
two actions through a real LazyParamInjector#resolveInto of a
disabled=${uploadDisabled} param and pins that one invocation's resolved flag
survives the other's, and that neither reaches the interceptor singleton.
* WW-5659 fix(core): reject uploads when the policy cannot be resolved
* WW-5659 test(core): exercise real lazy param resolution in dynamic upload tests
* WW-5659 fix(core): mark params unusable when a lazy value cannot be applied
resolveInto had two failure branches behaving oppositely. An unresolvable
${...} skipped the write and notified the holder, so a fail-closed holder such
as UploadPolicy could reject the upload. A value the holder's setter could not
accept — a non-numeric String for the Long maximumSize, say — also skipped the
write but notified nothing, leaving the policy reporting isUnresolved() == false
and maximumSize null, which acceptFile reads as "no size limit". The cap was
silently off and the file accepted: fail-open, through a branch the design never
enumerated.
Notify the holder from the catch block too, and record in the spec the general
rule that every path skipping a write must notify, so a future failure mode gets
checked against it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 chore(core): harden the policy sets and tidy the lazy params dispatch
UploadPolicy handed out the mutable HashSet built by commaDelimitedStringToSet,
which the copy constructor shares by reference with the configured policy, so a
subclass overriding the protected acceptFile could have rewritten process-wide
config from a request thread. The sets are unmodifiable now.
Also document that unresolved() records `disabled` like any other param, so an
unresolvable disabled expression leaves the interceptor enabled and rejects
every upload; and in DefaultActionInvocation use normal imports for the params
types, make the interceptor local final, word the three skip logs consistently
and identify the interceptor by its mapping name, and note that the name-based
param merge is inherited behaviour whose duplicate-ref handling is questionable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): allowlist the lazy params holder for OGNL member access
Moving lazy param resolution off the interceptor and onto a per-invocation
InterceptorParams holder changed the OGNL target of the write. The interceptor
is allowlisted at configuration time by XmlDocConfigurationProvider, because it
is named in the configuration; the holder is named nowhere, so with the shipped
default struts.allowlist.enable=true SecurityMemberAccess refused every setter,
resolveInto's fail-closed handling marked every param unresolved, and
ActionFileUploadInterceptor rejected every upload.
Register the holder's own class hierarchy through ProviderAllowlist when the
interceptor is built, keyed by the holder class so repeated builds collapse onto
one entry. Only the holder's class, superclasses and interfaces are registered -
the setter may be declared on any of them and SecurityMemberAccess checks both
the target and the declaring class. Object is filtered out: it says nothing
about the holder and is excluded by default anyway. No package is allowlisted.
Also run Interceptor#init() before the holder is obtained, so newLazyParams()
sees a fully initialised interceptor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 test(core): prove lazy params resolve with the OGNL allowlist enabled
Every other core test runs with struts.allowlist.enable=false - StrutsTestCaseHelper
turns it off by default and XWorkTestCaseHelper never loads default.properties -
so no core test could see the holder being blocked by SecurityMemberAccess. Only
the showcase DynamicFileUploadTest integration test exercised the production
setting, which is why the regression reached CI.
This test boots the dispatcher with the allowlist enforced and asserts both that
the holder hierarchy is registered at configuration time and that a ${...} param
actually lands on the policy rather than being reported unresolved. Reverting the
registration in DefaultInterceptorFactory fails it with the same
"Declaring class [UploadPolicy] ... is not allowlisted" warning seen in the
showcase failure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): stop an unresolvable disabled param from voiding the upload policy
UploadPolicy#unresolved recorded every param name, disabled included, so an
unresolvable <param name="disabled">${...}</param> marked the whole policy
unusable and rejected every upload of the invocation. That is not a safe
default: disabled is not a validation dimension. Its unresolved value is simply
false, which leaves the interceptor running and the rest of the policy intact,
so it cannot relax validation - recording it only invents a second failure mode.
Exclude it from the tracking that gates isUnresolved(), via a new
DisableParams#DISABLED_PARAM constant, and replace the javadoc that defended the
old behaviour. A param that is a validation dimension still voids the policy,
including when it fails alongside disabled.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 docs(core): state what the lazy param injector actually did
The two WARN messages in resolveInto claimed consequences the injector does
not control. The ReflectionException branch said the params were 'marked
unusable', but InterceptorParams.unresolved is a defaulted no-op, so only a
holder that overrides it - UploadPolicy does - degrades at all. The
unresolved-expression branch said the configured value was kept, which reads
as a sensible fallback when it is normally the unevaluated ${...} literal
applied at build time.
Both now report only the injector's own actions: the value was not written
and the holder was notified. The nuance about what the holder retains moves
to the javadoc, where there is room to state it accurately.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 feat(core): reject unknown lazy interceptor params at configuration time
A param name that no property on the params holder can accept was only
noticed per request: resolveInto caught the ReflectionException, warned, and
notified the holder - which for UploadPolicy means rejecting every upload of
every request behind a WARN. The names are fully known when the configuration
is parsed, so DefaultInterceptorFactory now fails with a ConfigurationException
naming the interceptor, the param and the holder type.
Only the interceptor-ref params are checked. InterceptorBuilder passes that
same map on to the InterceptorMapping, and DefaultActionInvocation.mergedParams
feeds it to resolveInto, so it is exactly the set that reaches the holder.
Params on the <interceptor> definition are applied to the interceptor instance
and never reach the mapping; checking them too would reject working config,
<param name="disabled"> on a definition being the obvious case.
The runtime handling stays as defence in depth. ConfigurationException is now
rethrown rather than swallowed by the generic catch, so the operator reads the
param name instead of "Caught Exception while registering Interceptor class".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 refactor(core): drop the deprecated single-arg executeConditional
The overload lost its last caller when the mapping name became available at
the call site, so an existing subclass override would have compiled and then
never run again - silently dead code, worse than a compile error. This branch
already changes the protected acceptFile signature, so keeping the one-arg
form for source compatibility was not consistent either.
Covered by a test asserting the surviving two-arg form is the extension point
and receives the mapping name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): keep configuration order when merging lazy interceptor params
mergedParams built a HashMap, so the order the configuration carries was
discarded and the order params were applied to the per-invocation holder was
whatever hashing produced. LinkedHashMap makes it deterministic and matches
the LinkedHashMap the InterceptorBuilder already assembles.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* WW-5659 fix(core): keep interceptor params serializable
Interceptor extends Serializable, so an interceptor holding its configured
params as a field must hold something serializable. The fields UploadPolicy
replaced were a Long and two HashSets, all serializable; the holder was not,
which silently broke serialization of every file upload interceptor.
Fix it on the contract rather than the field: InterceptorParams now extends
Serializable, so every holder inherits the requirement. Marking the field
transient would instead have dropped the configured policy on deserialization.
Also renames four test locals that shadowed the interceptor field and drops
a throws clause that could not be reached, both reported by SonarCloud.
* WW-5659 test(core): hoist the params map out of the assertThatThrownBy lambdas
Each lambda called both params(...) and buildInterceptor(...), so a throw from
the helper would have satisfied the assertion just as well as one from the code
under test. Building the map first leaves one throwing call per lambda.
Reported by SonarCloud (java:S5778).
* WW-5659 fix(core): stop seeding the interceptor with raw lazy expressions
A ${...} param is resolved per invocation into the params holder, so applying
its raw text to the interceptor at configuration time only seeded an
unevaluated literal - allowedTypes held "${uploadConfig.allowedMimeTypes}",
matching no content type - or failed conversion outright for a typed property
such as the Long maximumSize.
Withhold those params at build time; static params still apply and still seed
the holder, which is what a lazy param falling back has to fall back to. Also
makes InterceptorParams.unresolved's javadoc about the retained value honest.
Idea from @deprrous in GitHub PR #1815.
Co-Authored-By: deprrous <sukhbatsuugii2004@gmail.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: deprrous <sukhbatsuugii2004@gmail.com>