mirror of
https://github.com/apache/struts.git
synced 2026-08-05 06:36:58 +00:00
main
3948 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Lukasz Lenart
|
702280f48f |
WW-5656 docs(conversion): mark ConversionRule.COLLECTION and the Collection_ prefix as deprecated (#1825)
Both have been documented as deprecated since WebWork 2.1.x, but neither carried an actual @Deprecated annotation, so users only ever learned about it from prose or from an INFO log line that fires solely when the fallback is hit. Runtime behaviour is unchanged - the Collection_ fallback keeps working, and COLLECTION remains a legal @TypeConversion rule handled identically to ELEMENT. Call sites that reference either element deliberately carry a suppression and a note saying why. Also deprecates XWorkConverter.CONVERSION_COLLECTION_PREFIX, a second public spelling of the same Collection_ prefix that the framework itself never reads. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
687436f9bf |
WW-5668 Keep the localized-text providers deserializable across a version upgrade (#1824)
Follow-up to #1821. Pins serialVersionUID to the value implicitly computed for the Struts 7.2.1 class shape instead of 1L, so a session serialized by a 7.2.1 node still loads on a 7.3.0 one during a rolling upgrade rather than failing with InvalidClassException. Such a stream carries no value for the new cache settings, and field initialisers do not run during deserialization, so readObject restores their defaults before rebuilding the caches - without that guard it failed with a NullPointerException. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
a6570b769b |
WW-5668 Bound the localized-text provider caches and align request-locale resolution (#1821)
* WW-5668 docs: design spec for bounded i18n caches and request-locale resolution Follow-up to WW-5540. Bound the AbstractLocalizedTextProvider caches via the existing OgnlCache abstraction (configurable struts.i18n.cacheType/cacheMaxSize), and add opt-in request-locale resolution consistency between Dispatcher and I18nInterceptor (struts.locale.validateRequestLocale, default off). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 docs: implementation plan for bounded i18n caches and request-locale resolution Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add remove(key) to the OgnlCache abstraction Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Bound the localized-text provider caches with configurable size Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Fix reassignable-lock hazard and add volatile to i18n cache fields synchronized (bundlesMap) locked on a monitor that rebuildI18nCaches() can reassign; introduce a dedicated bundlesMapLock and lock on that instead. Mark the five i18n cache fields volatile for safe publication across the reassignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add opt-in request-locale resolution consistency to Dispatcher Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Keep the localized-text caches transient so the provider stays serializable Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add missing Javadoc and cache-rebuild coverage for i18n provider Add the one-line Javadoc that sibling fields/setters carry to validateRequestLocale and its @Inject setter in Dispatcher, and add two tests covering StrutsLocalizedTextProvider's serialize/deserialize cache rebuild and cacheType selection behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Pin explicit serialVersionUID on the localized-text providers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Suppress false-positive Sonar S3077 on the thread-safe i18n caches Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Drop the unused throws Exception from the new Dispatcher locale tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
1218c49224 |
WW-5666 Apply input length limits consistently when reading request bodies (#1819)
* WW-5666 fix(json): apply the input length limit while reading The configured JSON input length limit was evaluated after accumulating each line of input. It is now evaluated as the input is read, in fixed-size chunks, so enforcement no longer varies with the structure of the input. Line terminators are no longer stripped while reading. They are insignificant whitespace between tokens, but an unescaped control character inside a string value is now preserved rather than silently removed. * WW-5666 fix(core): bound the CSP report body read and make the limit configurable CspReportAction read the submitted report body with a single readLine() and had no limit of its own. Read it up to a limit instead, defaulting to 8192 characters and configurable through struts.csp.report.maxSize. A body above the limit is discarded with a warning rather than processed. The limit is injected when the action is built, before the interceptor stack runs, because withServletRequest is invoked by the servletConfig interceptor ahead of staticParams and params. Values that are not usable as a buffer size are ignored with a warning. |
||
|
Lukasz Lenart
|
81264063e6 |
WW-5659 Resolve lazy interceptor params per invocation (#1816)
* 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>
|
||
|
Lukasz Lenart
|
94a8fcb26c |
WW-3784 Order annotated wildcard actions most-specific-first (#1813)
* WW-3784 docs: design for specificity-ordered wildcard matching in annotated actions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: implementation plan for annotated wildcard specificity ordering Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(convention): add action-name specificity comparator Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 fix(convention): add Apache License header to test file Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(core): add PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: add javadoc for PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(convention): order annotated wildcard actions most-specific-first Sorts each convention-built package's action configs by pattern specificity so a specific pattern (some/usefull/*) is matched before a general one (some/*), regardless of class-scan order. Also makes convention action ordering deterministic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: correct wildcard cross-segment claims and note comparator limitations The spec incorrectly stated that WildcardHelper's single `*` is greedy and crosses `/`, and that `some/*` shadows `some/usefull/*`. Verified against WildcardHelper.java and NamedVariablePatternMatcher.java: only `**` crosses `/`, so those two patterns are actually disjoint (different segment counts) and never compete for the same request. Correct the Problem narrative, ticket example, and matcher bullets to state this accurately, and document two known limitations of the specificity comparator (raw wildcard-token-count key can misrank `**` ahead of narrower multi-token patterns; parent-package actions bypass sorting). Also add a test asserting the natural-order alphabetical tiebreak key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 test(convention): prove specificity ordering fixes wildcard shadowing end-to-end Adds an end-to-end routing test driving the production reorder (PackageConfig.Builder.reorderActionConfigs + ActionNameSpecificityComparator) through the real ActionConfigMatcher/WildcardHelper. some/** and some/usefull/* genuinely overlap for some/usefull/sleeping (** crosses '/'), so the test asserts the general pattern shadows the specific one when registered first, and that specificity ordering makes the specific action reachable again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
6f802987f5 |
WW-3871 Derive ConversionRule prefixes for @TypeConversion keys (#1812)
* WW-3871 docs: add design spec for @TypeConversion key derivation Specifies deriving the ConversionRule prefix for @TypeConversion keys at class, method and field level via a single resolver, adds ElementType.FIELD as a target, and records the break/continue and empty-key fixes in the same code block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs: note interaction with the 7.3.0 converter mapping cache Records that addConverterMapping runs inside the computeMappingIfAbsent builder introduced by WW-5539, which executes outside any lock, so the new field pass adds no deadlock risk but must stay side-effect free. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs: add implementation plan for @TypeConversion key derivation Seven TDD tasks covering ConversionRule#prefix(), the shared resolveKey helper, class- and field-level derivation, the break/continue and empty-key fixes, an end-to-end binding proof and the Javadoc updates. Refines the spec's resolveKey signature to take the two annotation attributes rather than the annotation instance, so it can be unit tested directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 feat(core): add ConversionRule#prefix() owning the rule-to-prefix table * WW-3871 refactor(core): split addConverterMapping into per-source passes * WW-3871 feat(core): derive conversion mapping keys through a single resolver * WW-3871 fix(core): derive class level conversion keys and stop dropping later entries * WW-3871 feat(core): support @TypeConversion on fields * WW-3871 test(core): assert bare conversion keys bind through the action lifecycle Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs(core): document conversion key derivation and field level support * WW-3871 docs(core): add deprecated Collection_ prefix to parameter table * WW-3871 fix(core): widen resolveKey idempotence guard against any rule prefix resolveKey only recognized a key as already-prefixed if it started with its own declared rule's prefix. COLLECTION and ELEMENT are interchangeable throughout the conversion pipeline (DefaultConversionAnnotationProcessor handles them in the same branch, DefaultObjectTypeDeterminer.getElementClass reads Element_ then falls back to the deprecated Collection_), so key="Element_users" with rule=COLLECTION silently doubled to Collection_Element_users instead of being left alone, losing the mapping. Match against every known rule's prefix instead. Also documents two related precedence subtleties surfaced during review: processFieldAnnotations' Javadoc now notes that an inherited method can claim a key before a subclass's own field annotation is considered, since getMethods() includes inherited methods and runs first; and the unresolvable-key WARN in processMethodAnnotations now names the method's declaring class rather than the class being scanned, since getMethods() can surface the same inherited method at every level of the hierarchy. Design spec section 2 updated to match the implementation. * WW-3871 docs(core): correct TypeConversion Javadoc property attribute and determiner package Two pre-existing errors in the block this ticket's commits already touch: the APPLICATION example used a non-existent "property" attribute where "key" is the working form (see ConversionTestAction.java:97), and the rule() Javadoc pointed at org.apache.struts2.util.DefaultObjectTypeDeterminer instead of the actual org.apache.struts2.conversion.impl package. * WW-3871 test(core): cover key-prefix crossover, empty class-level key, and KeyProperty_ end-to-end binding - testResolveKeyLeavesAnAlreadyPrefixedKeyAlone: add the COLLECTION/ELEMENT crossover cases that demonstrate the resolveKey guard fix (fail before, pass after). - New EmptyKeyConversionAction fixture plus testClassLevelEmptyKeyRegistersNoMapping: a class-level @TypeConversion with no key must be skipped, not registered under "". This was the one behavioural bullet in the spec's test plan with no coverage. - MyBeanActionTest.testBareConversionKeysBindTheSameWayAsPrefixedOnes: add an assertion that the bare KeyProperty_ derivation actually binds the list index onto the created bean's id property end to end, not just that a converter mapping exists. * WW-3871 fix(core): skip APPLICATION-scoped @TypeConversion with no explicit key Method- and field-level @TypeConversion(type = APPLICATION) with no key previously derived a member name (e.g. a setter's property name) and registered it in the global default converter map via addDefaultMapping. That map is only ever read by class name (lookup(String, boolean) and lookup(Class)), so the entry was permanently unreachable. Skip it before deriving a name, logging a WARN naming the declaring class and member; the class-level pass already handled this correctly via resolveKey returning null. Adds a fixture and tests proving no default mapping is registered under the derived member name in either pass. * WW-3871 docs(core): fix broken TypeConversion Javadoc example and align spec TypeConversion's example class declared `users` twice (once unannotated, once again at its annotated field), so the sample no longer compiled as written; drop the earlier, redundant declaration. The same example's setConvertInt showed @TypeConversion(type = APPLICATION) with no key - exactly the case the previous commit's XWorkConverter fix now skips. Drop the type attribute so it reads as a class-scoped conversion, matching the corrected ConversionTestAction fixture. The correct APPLICATION example further down (execute(), key = "java.util.Date") is untouched. Also records the APPLICATION no-key skip rule in the design spec's carve-out paragraph so spec and code agree. * WW-3871 fix(core): dedupe method-pass WARN logging for inherited @TypeConversion processMethodAnnotations iterates clazz.getMethods(), which includes inherited public methods, and buildConverterMapping calls it once per class in the hierarchy. A single misconfigured @TypeConversion on a base class method was therefore logging its WARN once per subclass level. Gate both WARN call sites on method.getDeclaringClass() == clazz so each fires exactly once, at the level that owns the method; the derivation/registration logic keeps running on every visit unchanged. Adds a small permanent test proving the gate is logging-only: an inherited annotated setter still resolves and registers through a subclass that overrides nothing. * WW-3871 docs(core): clarify field-name key default and dedicated-annotation precedence Two gaps in the @TypeConversion Javadoc, both newly relevant now that the annotation targets fields: - The key() default on a field is the field name, not the JavaBean property name (processFieldAnnotations uses field.getName()). A field like _users backing property users would otherwise derive CreateIfNull__users, a key DefaultObjectTypeDeterminer never looks up. - org.apache.struts2.util's dedicated field annotations (@Key, @Element, @KeyProperty, @CreateIfNull) are consulted by DefaultObjectTypeDeterminer before it falls back to the converter mapping @TypeConversion populates, so a dedicated annotation silently wins over an equivalent @TypeConversion on the same property. Verified against getAnnotation/getElementClass/ getKeyProperty in DefaultObjectTypeDeterminer before documenting it. * WW-3871 docs(core): note COLLECTION derives the deprecated Collection_ prefix ConversionRule.COLLECTION.prefix() intentionally returns Collection_, the spelling DefaultObjectTypeDeterminer treats as deprecated and logs an INFO about on every fallback hit, kept for compatibility with existing annotations. Document that the derivation is deliberate and point readers at ELEMENT as the current form. * WW-3871 refactor(core): extract shared annotation-registration pipeline processMethodAnnotations and processFieldAnnotations were the same five-step pipeline (skip non-@TypeConversion, skip APPLICATION-scoped without a key, derive the name, resolve the key, register unless already mapped) written twice, driving SonarCloud S3776 cognitive complexity to 26 and 21 respectively and triggering three S135 multiple-break/continue findings. Extract steps 2-5 into a private registerAnnotatedMember(mapping, tc, Member, fallbackName, logSkips) helper that both passes delegate to. Each pass is now just its loop plus one instanceof check. The method pass keeps its per-declaring-class log gate (getMethods() revisits inherited methods once per hierarchy level); the field pass always logs, since getDeclaredFields() is visited once per class. The two WARN wordings, which differed only in a trailing clause, are merged into one message accurate for both a method and a field. No change to the registered mapping, pass order, or precedence for any class - verified via the existing XWorkConverterTest, AnnotationXWorkConverterTest, MyBeanActionTest, and ConversionRuleTest suites (92 tests, same count and same triggering warnings before and after) plus the full core module suite (3043 tests). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * WW-3871 docs(core): fix inaccurate and self-contradicting TypeConversion key Javadoc @Key, @Element, @KeyProperty and @CreateIfNull are @Target({FIELD, METHOD}), not field-only, and the same paragraph already says they are read from the field, setter and getter - drop "field" from "dedicated field annotations". Fold the field-vs-property-name correction into key()'s opening sentence instead of stating "defaults to the property name" and rebutting it three lines later, and align the parameters table row for key with the same rule. * WW-3871 docs(core): clarify XWorkConverter annotation-registration logging Give the success DEBUG the same [declaringClass#member] shape the three skip messages already use, instead of logging the bare member name that identifies neither the class nor whether it was a method or a field. Reword the "already mapped" DEBUG so it covers its commonest trigger - the same annotation seen one hierarchy level down, not just a genuinely higher-precedence source. Note in the logSkips comment that buildConverterMapping only visits each class' direct interfaces, so a misconfigured annotation declared on a super-interface method never gets logged at all, even though registration is unaffected. Also drop a stray extra blank line. No behavioural change: registration/derivation logic is untouched. * WW-3871 test(core): make inherited-method-annotation test diagnostic testInheritedMethodAnnotationStillRegistersThroughASubclass previously asserted nothing the logSkips gate could break: the hierarchy walk always reaches InheritedMethodConversionAction itself, where declaringClass == clazz, so the key registers there regardless of whether registration is (wrongly) gated alongside logging. The test passed identically with logSkips hardcoded true or false. Give InheritedMethodConversionSubAction a contesting field annotation for the same property the inherited setter claims. The inherited method annotation registers at the subclass level - before the subclass's own field pass runs - so its value must keep winning; that is the invariant documented on processFieldAnnotations, and it is exactly what gating registration would break, since the subclass field would start winning over the inherited method annotation instead. Verified: temporarily wrapping the registerAnnotatedMember call in processMethodAnnotations with `if (logSkips)` makes this test fail (expected:<true> but was:<false>); reverting it passes again. Mutation was not committed. Corrected both Javadocs, which overclaimed what the old assertion proved. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
d906f23448 |
WW-3530 Fix visitor-validator cache-key collision under wildcard actions (#1811)
* WW-3530 docs: add design spec for visitor-validator cache-key fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: add implementation plan for visitor-validator cache-key fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 test(core): cover visitor-validator cache-key context handling under wildcard actions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 fix(core): keep visitor-validator context in cache key under wildcard actions Apply the wildcard config-name substitution only when validating the action's own class. Visited objects carry a stable, explicit visitor context that must remain part of the cache key, otherwise two visitor validators on one field with different contexts collide and the second is silently dropped. Fixes https://issues.apache.org/jira/browse/WW-3530 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: document <s:form> render-path caching limitation Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: correct WW-2996 scope claim and note default-context visitor limitation Final-review finding: default-context visitor validators under wildcard actions key on the volatile resolved action name for the visited class, reintroducing bounded WW-2996-style cache growth (memory only; correct validators still load). Correct the 'WW-2996 untouched' wording to 'untouched for the action's own class', document the subpath as an accepted limitation folded into the follow-up ticket, and clarify that end-to-end visitor execution is covered by existing visitor suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 chore(core): add DEBUG logging for validator cache-key branch decision Log the built key together with clazz, context, validatingActionClass, wildcard, and the action config name, so the wildcard-vs-visited-object branch taken in buildValidatorKey can be diagnosed at runtime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 test(core): use assertNotEquals for cache-key inequality; fix comment grammar Address SonarCloud S5785 (assertFalse+equals -> assertNotEquals) and a Copilot grammar nit in the WW-4536 comment. DEBUG logging kept as-is per author decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
532ca7f864 |
WW-2934 Skip field validators when a field has a conversion error (opt-in) (#1810)
* WW-2934 docs: add design spec for skipping validators on conversion error Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 docs: add implementation plan for skipping validators on conversion error Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 feat(core): skip field validators on conversion error behind opt-in flag Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 test(core): cover annotation manager + document conversion-error skip exemption Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 test(core): assert custom conversion message survives and cover nested field skip Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
0b2bc2be14 |
WW-3427 Add regression test for conversion errors on aliased properties (#1814)
* WW-3427 test(core): cover conversion errors on aliased properties Reproduce the WW-3427 scenario: an aliased property whose custom TypeConverter throws TypeConversionException. AliasInterceptor already reports such errors (setReportingConversionErrors on the secure child stack, then copies conversion errors back to the original ActionContext), but nothing exercised the alias + conversion-error path. The test drives an action through params -> alias -> conversionError and asserts the failure surfaces both in ActionContext.getConversionErrors() and as a field error, confirming WW-3427 is fixed. Removing the copy-back in AliasInterceptor makes it fail with "swallowed", proving it guards the behavior. Test-only; no production changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3427 test(core): add Apache license header to conversion.properties RAT flagged the new test resource as having an unapproved license. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
12015d0bf5 |
WW-5540 Add caching to AbstractLocalizedTextProvider (#1808)
* WW-5540 docs: add caching design spec for AbstractLocalizedTextProvider Design for caching the class/package hierarchy traversal result in findText, keyed on (classloader, class name, textKey, locale). Caches the raw resolved pattern (or a NOT_FOUND marker) only; translation and formatting stay per-call. Wires invalidation into the existing reloadBundles/clearBundle/clearMissingBundlesCache sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: add implementation plan and refine spec Add the 3-task TDD implementation plan and record the formatWithNullDetection fall-through decision in the spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: deprecate+delegate findMessage/getMessage in plan Resolve pre-flight duplication/dead-code finding: old traversal helpers delegate to the raw twins and are marked @Deprecated instead of being duplicated. Add a direct characterization test for the findMessage delegator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 refactor(core): split raw message resolution from formatting Add getRawMessage/formatMessage and a raw twin findMessageRaw. Re-express getMessage via formatMessage and make findMessage delegate to findMessageRaw + formatMessage; deprecate both as legacy extension points superseded by the raw-resolution path. The deprecated findMessage triggers the bundle reload on entry, preserving the reload side effect the old getMessage-per-probe walk provided. Groundwork for the traversal caches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: refine Task 1 plan (deprecate/delegate + reload-on-entry) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 perf(core): cache class-hierarchy text resolution Cache the class/interface/superclass traversal in findText keyed on (classloader, class name, key, locale), storing the raw pattern or a NOT_FOUND marker. Formatting stays per call and falls through to the next tier when a cached pattern formats to null. Invalidated on reloadBundles/clearBundle/clearMissingBundlesCache; reload is hoisted to the top of findText so caches are cleared before they are read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: draft follow-up ticket for null-control-flow cleanup Capture the deferred result-wrapper refactor (raised during WW-5540) as a ready-to-file Jira draft; keep WW-5540 focused on caching. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 perf(core): cache package-hierarchy text resolution Cache the *.package traversal in findText the same way as the class hierarchy, with the same keying, fall-through, and invalidation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 test(core): tighten localized-text cache tests Assert single cache entry in the per-call-format tests (proves the raw pattern is cached, not the formatted result), and mirror the package-cache clearBundle/clearMissingBundlesCache invalidation test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: note devMode null-valueStack eager-reload edge Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: link follow-up doc to filed ticket WW-5655 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 chore(core): add ASF license header to CacheFixture.properties Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 chore(core): add since/forRemoval to @Deprecated annotations Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: drop follow-up draft superseded by WW-5655 The ticket is filed; the draft's content now lives in WW-5655 itself. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): address fresh-eyes review findings - Document that the deprecated getMessage/findMessage are no longer invoked by findText, and name formatMessage as the override point - Fall back to the ActionContext-based reloadBundles() when findText is called without a value stack, so the RELOADED flag is tracked and the caches can warm on that path in reload/devMode - Narrow resolveClassHierarchyRaw/resolvePackageHierarchyRaw to package-private (the cache key omits indexedKey, which is safe only when derived from textKey as the internal call sites do) - Suppress java:S2129 on the NOT_FOUND identity sentinel Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs: strip stray NUL bytes from design spec Two literal NUL bytes in the sentinel example made git/GitHub treat the whole markdown file as binary and unreviewable in the PR UI; align the example with the shipped sentinel name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 test(core): cover ModelDriven tier, per-locale keys, indexed keys Close the review-noted coverage gaps: the ModelDriven tier resolves via the shared class-hierarchy cache (action miss + model hit), each locale gets its own cache entry backed by a new _de fixture bundle, and indexed keys (name[N] -> name[*]) resolve and cache per full textKey. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): address Copilot review comments - Partition the caches by System.identityHashCode of the context classloader so a custom ClassLoader overriding hashCode() cannot collide or collapse the per-loader partitions - Derive the indexed key inside the resolvers (miss-only) instead of accepting it as a parameter, so the cache key trivially covers every input that influences the resolution result Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): resolve SonarCloud quality-gate findings - Suppress S4973 on isNotFound: the identity comparison against the non-interned NOT_FOUND sentinel is the design, not a bug - Reduce findMessageRaw cognitive complexity (S3776) by extracting getRawMessageWithAlternate, reused by the package walk - Add missing @Override annotations and suppress the deliberate deprecated-delegator call in the test helper (S1161, S5738) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 fix(core): make findMessageRaw cycle guard effective The `checked` set tested `contains(clazz.getName())` but never added the class, so the diamond-interface cycle guard was a no-op (a latent issue inherited from the original findMessage). Add the class name after the contains-check so repeated interface branches aren't re-traversed. Behavior is unchanged (lookups are idempotent); this only avoids redundant recursion. The deprecated findMessage delegates here, so it's fixed too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5540 docs(core): reference WW-5658 in deprecated method javadoc Point the @deprecated javadoc of getMessage and findMessage at WW-5658, the ticket tracking their removal in the next major release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
833220346c |
WW-5474 Count files only for maxFiles, add maxParameterCount (#1806)
* WW-5474 docs(multipart): design for files-only maxFiles + maxParameterCount Spec for correcting struts.multipart.maxFiles to count file parts only (consistently across the jakarta and jakarta-stream parsers) and adding struts.multipart.maxParameterCount to cap non-file form fields, restoring the DoS guard the old accidental total-part cap provided. Fail-closed on breach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 docs(multipart): implementation plan for maxFiles/maxParameterCount Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): count files only for maxFiles, add maxParameterCount (jakarta) The jakarta parser passed maxFiles to commons-fileupload2 setMaxFileCount, which counts every part (fields + files), so maxFiles wrongly limited total parameters. Enforce a files-only count and non-file field count in Struts, failing closed on breach; keep a total-parts commons backstop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): honor -1 unlimited sentinel in total-parts backstop prepareServletFileUpload applied the total-parts backstop whenever both maxFiles and maxParameterCount were non-null, without checking for the -1 "unlimited" sentinel already honored by enforceMaxFiles/enforceMaxParameterCount. With maxFiles=-1 and maxParameterCount=256, maxParts computed to 255 and was passed to commons-fileupload2's setMaxFileCount (which counts ALL parts), wrongly rejecting large file-only uploads. Only apply the backstop when both limits are finite (non-null and >= 0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): apply files-only maxFiles + maxParameterCount to stream parser Replace the field-name-based exceedsMaxFiles with the shared files-only enforcement and add parameter-count enforcement, matching the jakarta parser and failing closed on breach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): track all parsed items to avoid temp-file leak on fail-closed breach servletFileUpload.parseRequest() fully materializes every part - spilling large ones to disk - before processUpload() iterates over the result. The loop only added each DiskFileItem to diskFileItems as it was reached, so when enforceMaxFiles/enforceMaxParameterCount threw mid-loop on a breach, every item positioned after the breaching one was never registered for cleanup. With no FileCleaningTracker on the factory, cleanUp() had no way to reclaim those temp files, leaking disk space on the hardening path. Materialize the parsed list once and register all items for cleanup before processing so cleanUp() reclaims every temp file regardless of where enforcement aborts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): guard debug logging in enforce helpers (Sonar S2629) Wrap the LOG.debug calls in enforceMaxFiles/enforceMaxParameterCount with isDebugEnabled() so normalizeSpace() is not evaluated when debug is disabled, matching the exceedsMaxStringLength pattern in the same class. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): address Copilot review - parser parity + overflow guard - JakartaMultiPartRequest: only count/enforce a file part toward maxFiles when it has a non-null field name, matching JakartaStreamMultiPartRequest's accept criteria (defensive: commons-fileupload2 already drops parts without a name attribute before parseRequest returns, so the two parsers stay consistent regardless). - AbstractMultiPartRequest: compute the total-parts backstop with Math.addExact and clamp to Long.MAX_VALUE on overflow, so extremely large configured limits cannot wrap negative and silently disable the commons backstop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): log "processing a form field" only for form fields Move the debug log into the isFormField branch so file parts are not mislabelled; the file branch already logs "Processing a file". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
c3e9db1b21 |
WW-5413 Avoid writing small in-memory multipart uploads to disk (#1805)
* WW-5413 docs(core): design for in-memory multipart upload optimization Lazy-materializing UploadedFile plus a new getInputStream() accessor so small (in-memory) uploads no longer eagerly write a temp file, while getContent() keeps returning a File for backward compatibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 docs(core): implementation plan for in-memory upload optimization Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 feat(core): add UploadedFile.getInputStream() streaming accessor Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 feat(core): add lazily-materializing StrutsInMemoryUploadedFile Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 fix(core): make StrutsInMemoryUploadedFile serializable and thread-safe * WW-5413 refactor(core): drop eager temp-file write for in-memory uploads * WW-5413 test(core): cover deferred-write behavior for in-memory uploads Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 perf(core): avoid materializing in-memory uploads during interceptor validation * WW-5413 chore(core): clean up partial materialization and cover isMissing() * WW-5413 docs(core): sync design/plan with interceptor fix and deviations Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 chore(core): deprecate now-unused STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY Mark the orphaned constant @Deprecated(forRemoval = true) instead of leaving it silently unused. The message key it referenced was only emitted from an unreachable block in acceptFile() that was removed with the in-memory upload optimization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 test(core): cover materialization failure and getInputStream default branches Address review follow-ups on PR #1805: - document that processFileField's retained 'throws IOException' is intentional (subclass source compatibility), not an oversight - add a negative test: getContent() on an unwritable save dir throws StrutsException, stays unmaterialized, and leaves no partial file behind - cover the UploadedFile.getInputStream() default File branch and the no-content IOException branch Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5413 fix(core): address SonarCloud and Copilot review findings - materialize() now writes with StandardOpenOption.CREATE_NEW and fails closed if the target already exists, so a pre-planted file/symlink is never overwritten or followed (Copilot security note) + regression test - defensively copy the content byte array on construction and reject null content, so the instance owns its bytes and cannot observe caller mutation (Copilot / review) - delete() uses Files.deleteIfExists and logs the real cause on failure instead of a silent File.delete() boolean (Sonar MAJOR) - reorder field modifiers to JLS order 'transient volatile' (Sonar) - tests: assertThat(dir).isEmptyDirectory() instead of listFiles().isEmpty() (Sonar); drop unused DiskFileItem import (Sonar) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
8a5323fbdf |
WW-5539 Concurrency performance enhancements (#1799)
* WW-5539 docs: add concurrency performance enhancements design Design for removing coarse locks from XWorkConverter, DefaultActionValidatorManager and StrutsTypeConverterHolder in favour of concurrent collections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 docs: add concurrency performance implementation plan Five tasks derived from the approved design: make StrutsTypeConverterHolder concurrent, add the computeMappingIfAbsent SPI method, remove the locks from XWorkConverter and DefaultActionValidatorManager, then benchmark and raise the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 docs: make conditionalReload store its rebuilt mapping buildConverterMapping no longer stores its result, so reload mode would have rebuilt from disk on every request without ever caching. * WW-5539 Make StrutsTypeConverterHolder collections concurrent The holder is a container singleton whose HashMaps were read without any lock by XWorkConverter.lookup() while being written elsewhere, risking lost updates and torn reads during resize. Null TypeConverters are now ignored with a warning rather than stored, since ConcurrentHashMap forbids null values and a null converter left the holder in an inconsistent state. * WW-5539 Rename test to match what it actually covers The method exercised only the unknown-mapping cache, not noMapping. * WW-5539 Add TypeConverterHolder#computeMappingIfAbsent Adds an atomic build-once-and-cache operation so callers no longer need check-then-act around the class mapping cache, and deprecates the three primitives it subsumes: getMapping, addMapping and containsNoMapping. The method is a default method delegating to those primitives, so third-party TypeConverterHolder implementations keep working unchanged. * WW-5539 Deduplicate the no-mapping path in computeMappingIfAbsent ConcurrentHashMap.computeIfAbsent stores nothing when the mapping function returns null, so every concurrent caller re-ran the builder for a class with no conversion mapping - the common case for an ordinary action, and the exact thundering herd this method exists to prevent. Negative results now store a sentinel in the same map, so the builder runs once per class either way. getMapping and containsNoMapping translate the sentinel, preserving their existing contracts. * WW-5539 docs: sync plan with negative-cache sentinel fix * WW-5539 Pin down addNoMapping's override semantics Storing the no-mapping sentinel deliberately replaces any mapping cached for the class, matching the pre-7.3.0 effective behaviour where such a class was short-circuited before its cached mapping was ever read. putIfAbsent would instead serve a stale mapping after a failed build. Also asserts the sentinel translation in getMapping directly, and stops the interface javadoc promising a specific empty-map instance that implementations are not required to return. * WW-5539 Document that addNoMapping may replace a cached mapping The behaviour was documented only on the Struts implementation, but addNoMapping stays a non-deprecated SPI primitive that third parties both call and implement, so the contract belongs on the interface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Remove coarse locks from XWorkConverter getConverter() synchronized on the Class object being converted, which is a globally visible monitor any other library may contend on, and which serialised every conversion for a given action class including cache hits. It now delegates to TypeConverterHolder#computeMappingIfAbsent. registerConverter and registerConverterNotFound drop their synchronized modifier; they are single delegations to a concurrent map, and the lock never covered the readers in lookup() in any case. buildConverterMapping no longer stores its result - storage is owned by computeMappingIfAbsent. * WW-5539 Remove global lock from DefaultActionValidatorManager getValidators() was synchronized on the singleton manager, so every validated request in the application serialised on it - and the lock covered the per-request Validator construction loop, which operates on per-request objects and never needed mutual exclusion. Both caches become ConcurrentHashMap and cached config lists are wrapped unmodifiable, since several threads now iterate them concurrently. * WW-5539 Make the validator concurrency test race a cold cache The test computed its expected count with a getValidators call before starting the threads, which warmed the cache and left all 16 workers on the fast path - never exercising first-build contention, the race the test is named for. Also drops an unused import and awaits executor termination. * WW-5539 Address final review findings Restores the protected unknownMappings field verbatim as a deprecated, unused vestige: retyping it changed the field descriptor, so a subclass compiled against 7.2.0 would have hit NoSuchFieldError on upgrade without recompiling. Real storage moves to a private concurrent set. Also stops conditionalReload running for negative-cached classes, which had been costing a failed classloader resource scan per property per request in devMode, and restores the unknown-mapping clearing that the null-converter guard was skipping. * WW-5539 Fix concurrency regressions from coarse-lock removal Four correctness fixes surfaced in PR review of the concurrent-collections refactor: - StrutsTypeConverterHolder.addDefaultMapping: restore put-before-remove ordering. The inverted order let a concurrent XWorkConverter.lookup observe (unknown=false, default=false), sending it into lookupSuper() and letting it overwrite the more specific converter being registered. - StrutsTypeConverterHolder.computeMappingIfAbsent: stop building inside a ConcurrentHashMap bin lock. The builder reaches ObjectFactory.buildConverter, which can autowire arbitrary user TypeConverters; running that under a CHM bin lock risked a recursive-update exception or self-deadlock. Callers now only get the guarantee that they converge on the same cached instance, not that the builder runs exactly once - documented on the interface and reflected in the concurrency tests. - DefaultValidatorFactory.validators: switch to ConcurrentHashMap now that DefaultActionValidatorManager.getValidators is no longer synchronized, so runtime registerValidator() calls no longer race unsynchronized reads of a plain HashMap. - XWorkConverter.conditionalReload: route empty devMode rebuilds through addNoMapping instead of addMapping, so an empty reload result is stored as the NO_MAPPING sentinel rather than a plain empty map that would silently disable further reloads for the class. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Add tests closing coverage gaps from coarse-lock removal SonarCloud's quality gate failed at 59.8% coverage on new code (need >=80%). Adds tests for the specific lines JaCoCo identified as uncovered, without touching production code: - TypeConverterHolder.computeMappingIfAbsent's default method body (the SPI compatibility fallback for third-party holders that predate 7.3.0 and don't override it) - new TypeConverterHolderTest against a minimal non-overriding implementation. - StrutsTypeConverterHolder.getMapping/containsNoMapping's remaining non-sentinel branch. - XWorkConverter.conditionalReload's reloadingConfigs==true path (both the addMapping and addNoMapping outcomes), buildConverterMappingUnchecked's checked-to-IllegalStateException wrapping, and getConverter's catch(Throwable) negative-caching. - DefaultActionValidatorManager's else-if(reloadingConfigs) cache rebuild, loadFile's checkFile&&fileNeedsReloading re-parse, and buildValidatorConfigs' already-checked short-circuit. TypeConverterHolder.java and StrutsTypeConverterHolder.java are now at 0 missed lines/branches. XWorkConverter.java and DefaultActionValidatorManager.java have all requested target lines covered; remaining misses are pre-existing, unrelated gaps left alone per scope. Full core suite: 3026 tests (3015 + 11 new), 0 failures/errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Fix SonarCloud deprecation and test-hygiene issues - Add since/forRemoval attributes to the 7 @Deprecated elements on TypeConverterHolder.getMapping/addMapping/containsNoMapping and StrutsTypeConverterHolder's overrides plus the unknownMappings field (java:S6355). - Add the missing @deprecated Javadoc tag to the three StrutsTypeConverterHolder overrides, pointing at computeMappingIfAbsent as the replacement (java:S1123). - Remove the unused throws Exception from testGetConverterBuildsMappingExactlyOncePerClass (java:S1130). - Document why StubFileManager.setReloadingConfigs/monitorFile are intentionally empty no-ops (java:S1186). - Rename a local variable that shadowed the converter field in testConditionalReloadRebuildsEmptyMappingAndStoresItViaAddNoMapping (java:S1117). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Suppress removal warnings for the deprecated holder primitives javac treats [removal] as a category separate from [deprecation], so marking the three primitives forRemoval left four warnings behind: the deliberate addMapping call in conditionalReload, and the three overrides that must exist for as long as the interface declares them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Address Copilot review comments Fixes getMapping's @return (it returns a Map, not a TypeConverter) and drops the "atomically" wording from its @deprecated tag, which no longer matches computeMappingIfAbsent's contract now that the builder may run more than once under concurrent first access. Syncs the design and plan docs with the shipped approach: the unknownMappings field is kept for binary compatibility rather than retyped, and the override uses get/build/putIfAbsent rather than computeIfAbsent. * WW-5539 docs: correct the classloader out-of-scope note The conversion caches are container-scoped singletons with no external references, so their Class keys do not independently pin the webapp classloader - that is governed by whatever retains the container (WW-5537). Reframed as optional defense-in-depth cache clearing, folded into WW-5537 Task 5b, rather than a standalone leak fix. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
f32270cdb0 |
WW-5580 chore(core): use Configuration.VERSION_2_3_34 for FreeMarker config (#1798)
Aligns the FreeMarker incompatible_improvements setting with the FreeMarker 2.3.34 dependency already declared in the build. FreeMarker 2.3.34 declares VERSION_2_3_34 as an incompatible improvements break-point but does not gate any behaviour on it, so this is a no-op at runtime and purely keeps the setting in sync with the dependency. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
f5880be2ef |
WW-5591 chore(core): mark XWorkObjectPropertyAccessor as deprecated (#1797)
The class is no longer used by the framework and can be removed in a future version. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
b2548542ec |
WW-5640 refactor(webjars): rename DefaultWebJarUrlProvider to StrutsWebJarUrlProvider (#1795)
Use the Struts* prefix convention for the framework's default WebJarUrlProvider implementation instead of Default*. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
11c10ee8f9 |
WW-5620 Standardize logging on Log4j2 (#1794)
* WW-5620 docs: add Log4j2 logging standardization design spec Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 docs: add Log4j2 logging standardization implementation plan Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Migrate FinalizableReferenceQueue to Log4j2 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Migrate AbstractDefaultToStringRenderable to Log4j2 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Remove unused injectable j.u.l.Logger DI factory from ContainerBuilder Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5620 Remove dead first-party SLF4J dependency declarations Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Arun
|
de599c91d3 |
WW-5646 Modernize path normalization in Include component (#1780)
* Modernize path normalization in Include component and handle edge cases * Add tests for edge case dot-dot handling in Include path normalization * Fix reversed path segment order in getContextRelativePath() The for-each loop iterated the ArrayDeque head-to-tail (most recently pushed first), which is the reverse of the old Stack's insertion-order iteration. This caused rebuilt paths like "car/view.jsp" to come out as "view.jsp/car". Use descendingIterator() to restore the original oldest-first ordering when rebuilding the flat path string. |
||
|
ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻
|
e5eb01abda |
WW-5642 fix(rest): authorize @StrutsParameter on record/creator-bound REST body properties (#1774)
* fix(rest): authorize @StrutsParameter on record/creator-bound REST body properties ParameterAuthorizingModule enforces @StrutsParameter on REST/JSON body deserialization by wrapping each property's deserializeAndSet/ deserializeSetAndReturn. Jackson never calls either method for creator-bound properties (Java records, @JsonCreator constructors, @ConstructorProperties) — it calls SettableBeanProperty#deserialize directly, which is declared final and bypasses the wrapper entirely. With struts.parameters.requireAnnotations enabled, any record-typed field anywhere in a REST action's request body was populated with no authorization check at all. Add AuthorizingValueDeserializer, which wraps the property's value deserializer instead of the property itself, and install it from AuthorizingSettableBeanProperty#withValueDeserializer — scoped to CreatorProperty so ordinary setter/field/builder properties, already authorized via the existing wrapper, aren't checked twice. * fix(rest): treat redaction-induced construction failures as unauthorized, not fatal AuthorizingValueDeserializer substitutes null for a rejected creator-bound property (record component, @JsonCreator/@ConstructorProperties param). For reference-typed, unvalidated components this is a harmless stand-in for "not set" -- but two cases turn that substitution into an unhandled exception that crashes deserialization of the entire request body instead of just dropping the unauthorized subtree: - A record/constructor with its own non-null validation (e.g. a compact constructor doing Objects.requireNonNull) throws ValueInstantiationException when the redacted component reaches it. - With DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES enabled, Jackson itself throws MismatchedInputException when a primitive-typed creator component is redacted to null. Add RedactionAwareDeserializer, wrapping every bean-type deserializer via a new BeanDeserializerModifier#modifyDeserializer hook. It tracks (via a new redaction-scope stack in ParameterAuthorizationContext) whether the object currently under construction had a property redacted by authorization; if construction then throws, the object is treated as unauthorized (returns null) instead of propagating the raw exception -- matching the same fail-closed outcome already used when a non-creator nested property is rejected outright. A guard test confirms genuine, unrelated validation failures (nothing redacted) still propagate normally, so real client errors aren't masked. Also verified (and added regression coverage for) the other gaps raised in review: static factory-method @JsonCreator, @ConstructorProperties, top-level records, 3-level nested creator chains, and List/Map creator params whose elements are further creator-bound or plain-POJO types -- all of these were already handled correctly by the existing withValueDeserializer interception. * test(rest): cover array creator param; document redaction edge cases Addresses the three non-blocking review notes on WW-5642: - Add testArrayOfRecordsAsCreatorParam_elementsAuthorizedByIndexedPath and a WithArray fixture, exercising the type.isArray() branch of AuthorizingValueDeserializer#prefixForNested so the collection matrix (List/Map/array) is fully covered. - Document in AuthorizingValueDeserializer that redacting a primitive creator component becomes the type default (0/false) when FAIL_ON_NULL_FOR_PRIMITIVES is off -- a deliberate choice, the client value never lands either way. - Document in RedactionAwareDeserializer that a redaction co-located with an unrelated mapping error is folded into "object dropped" -- a deliberate fail-closed trade-off, never exposing a partial object. --------- Co-authored-by: g0w6y <g0w6y@users.noreply.github.com> |
||
|
Arun
|
b70ecc8e15 |
WW-5645 Canonicalise static content paths and remove redundant URL decode (#1777)
* Add path segment validation utility to StaticContentLoader * Remove redundant URL decode in buildPath and reject malformed path segments early * Use shared path segment validation in WebJar URL provider * Document encoding contract on RequestUtils.getServletPath * Add tests for path segment validation in static content loader * Add encoded traversal test for WebJar static content serving * Add encoded traversal tests for WebJar URL provider * Fix missing closing brace in StaticContentLoader causing compile failure * Use per-segment matching in containsMalformedPathSegment to avoid false positives * Remove redundant dot-segment check now handled by containsMalformedPathSegment * Fix indentation on validateStaticContentPath closing brace * Remove unused encoding field and setter from DefaultStaticContentLoader * Replace denylist with path canonicalisation in Validator * Wire canonicalisePath into static content serving * Wire canonicalisePath into WebJar URL provider * Update tests for canonicalise approach and remove unused setEncoding call * Remove setEncoding calls from tests to match updated DefaultStaticContentLoader * Remove setEncoding calls from tests to match updated DefaultStaticContentLoader * Remove redundant encoded-traversal tests per maintainer review — end-to-end 404 already covered |
||
|
Lukasz Lenart
|
789dbf3cd2 |
WW-5640 Add WebJars support to Struts core (#1765)
* WW-5640 docs: design for WebJars support in Struts core Adds first-class WebJars support so client-side libraries can be referenced by a version-less logical path and served through the existing static-content pipeline. Grounded against 7.2.x source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 docs: implementation plan for WebJars support Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 build: add webjars-locator-lite dependency Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: add webjars config constants and defaults Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 docs: correct plan test framework to JUnit 4 core uses JUnit 4 + AssertJ + Mockito, not JUnit 5 Jupiter (no Jupiter engine on the classpath). Test tasks translate accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: add WebJarUrlProvider resolution seam Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: register WebJarUrlProvider bean Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: extend static content-type map for webjar assets Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: serve webjar assets via static content loader Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 feat: add <s:webjar> tag and <@s.webjar> macro Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 docs: add generated tag reference for <s:webjar> Annotation-processor-generated tag reference (attributes + description), tracked like every other tag's docs under core/src/site/resources/tags/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 fix: address final review (log level, resolveUrl traversal test, javadoc) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5640 refactor: address SonarCloud code smells - getContentType: replace long if/else chain with a static extension-> MIME map (S3776 cognitive complexity) - DefaultWebJarUrlProvider.split: return Optional<String[]> instead of a null sentinel (S1168; Optional fits the reject semantics, empty-array would not) - serving tests: rename local 'loader' -> 'webJarLoader' to stop hiding the ContentTypeProbe field (S1117) - WebJarTest: use assertThat(writer).hasToString(...) (S5838) S110 (WebJarTag inheritance depth) is inherent to the Struts tag base class hierarchy shared by every tag; left as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
50c16e93b1 |
Release 7.2.1-RC1 (#1744)
* [maven-release-plugin] prepare release STRUTS_7_2_1 * [maven-release-plugin] prepare for next development iteration |
||
|
Lukasz Lenart
|
59e342488d |
WW-5256 Decouple FreeMarker whitespace stripping from devMode (#1743)
* WW-5256 docs: design to decouple FreeMarker whitespace stripping from devMode Fixes s:textarea rendering blank lines and HTML whitespace bloat in devMode by honoring struts.freemarker.whitespaceStripping unconditionally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5256 docs: implementation plan to decouple whitespace stripping from devMode Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5256 test: prove whitespace stripping wrongly disabled in devMode * WW-5256 fix(freemarker): honor whitespaceStripping regardless of devMode * WW-5256 docs: drop devMode note from whitespaceStripping constant --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
489445c78d |
Release 7.2.0-RC1 (#1742)
* [maven-release-plugin] prepare release STRUTS_7_2_0 * [maven-release-plugin] prepare for next development iteration |
||
|
Arun
|
cc3ebc3c1f |
WW-5635 Avoid logging sensitive token values in TokenHelper (#1738)
* Avoid logging sensitive token values in TokenHelper Redact form and session token values from WARN-level log output in TokenHelper.validToken() and update corresponding i18n message properties. Detailed diagnostics moved to DEBUG level with sanitized input. * Update struts-messages.properties * Update invalid token error message for clarity * Update struts-messages_da.properties * Update struts-messages_de.properties * Update Polish translation for invalid token message * Update invalid token message in Portuguese properties * Improve token mismatch warning logging Updated warning message to include the form token in the log. * Update struts-messages.properties * Update invalid token message format in properties file * Update invalid token message for clarity * Update struts-messages_de.properties * Update struts-messages_pl.properties * Update invalid token message format in properties file * Update TokenHelper.java * Refactor token mismatch logging for development mode |
||
|
Arun
|
8f9b4b8a90 |
WW-5636 Harden redirect URL escaping in non-302 response body (#1737)
* Implement test for status code 200 with HTML escaping * Escape HTML in ServletRedirectResult response Escape HTML in the final location before writing to the response. |
||
|
Lukasz Lenart
|
dd830dca80 |
WW-5630 test: streamline ConfigParseUtilTest and convert to JUnit 4 (#1740)
Collapse 12 overlapping cache tests to 5 focused ones, replace the ~80-entry JDK class-name literal with a synthetic-name loop bounded by the inner-cache limit, and drop reflection from the behavioral tests (load-count assertions only). Reflection is retained solely in the two size-bound tests, where Caffeine exposes no public seam. Production ConfigParseUtil caching logic is unchanged. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
4f3fd69aa6 |
WW-5632 Harden commons-fileupload2 dependency against milestone binary-incompatibility (#1735)
* WW-5632 docs: add commons-fileupload2 milestone-hardening design spec Design for hardening the commons-fileupload2 dependency against milestone binary-incompatibility (manage -core, activate a scoped enforcer rule, add a runtime API guard in AbstractMultiPartRequest). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 docs: add implementation plan for fileupload2 milestone hardening Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 build(deps): manage commons-fileupload2-core alongside jakarta-servlet6 Pin both commons-fileupload2 artifacts to a single commons-fileupload2.version property so the volatile -core API can no longer skew from -jakarta-servlet6 in the reactor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 build: enforce a single commons-fileupload2 version Activate maven-enforcer-plugin (previously dormant in pluginManagement) with a fileupload-scoped bannedDependencies rule so any divergent commons-fileupload2 version fails the build early. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 fix(fileupload): fail fast on incompatible commons-fileupload2 API Verify once per JVM that the fileupload size-limit setters exist and throw a clear StrutsException reporting the core/jakarta version skew, replacing an opaque deep-stack NoSuchMethodError in downstream runtimes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5632 fix(fileupload): make API-verification guard static Resolve Sonar java:S2696 (instance method writing a static field) by making ensureFileUploadApiVerified() static; verification is JVM-global. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
brianandle
|
210dc86b88 |
WW-5630 - Performance Issue SecurityMemberAccess (#1721)
* WW-5630 - Performance Issue SecurityMemberAccess * Add size bound cache, 50, for Class lookup * Add unit test Code generated by Copilot * WW-5630 - Add additional UT * WW-5630 - Add UT for non-existent class * WW-5630 - Review feedback changes * Cache ClassLoader directly * Use weakKeys and weakValues * Comment on the ClassLookupException * Additional Unit Tests Assistance in coding using co-pilot * WW-5630 - Additional review * Limit outer, Classloader, to 25. Ensure memory bounding. * Limit inner, Classes, to 50. Ensure memory bounding. * Additional UTs With co-pilot assitance |
||
|
Lukasz Lenart
|
9011c32b38 |
WW-5631 Add opt-in @StrutsParameter enforcement to ChainingInterceptor (#1719)
* WW-5631 feat(chaining): add struts.chaining.requireAnnotations constant Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 feat(chaining): default struts.chaining.requireAnnotations=false Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): add annotated/unannotated chaining fixtures Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): add failing @StrutsParameter enforcement tests Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 feat(chaining): enforce @StrutsParameter on target when opted in Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 refactor(chaining): align requireAnnotations parsing with BooleanUtils Use BooleanUtils.toBoolean for the chaining requireAnnotations flag so it accepts the same values (yes/on/1) as the sibling struts.parameters.requireAnnotations switch, and unify the enforcement WARN message prefix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): cover includes interaction and proxied target Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 docs(chaining): document struts.chaining.requireAnnotations Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): cover fail-closed introspection; clarify target==action Add a test asserting nothing is copied when the target action cannot be introspected (fail-closed), and document why isAuthorized is called with target == action for chaining (no ModelDriven exemption). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 fix(chaining): address SonarCloud findings - Mark injected parameterAuthorizer/ognlUtil fields transient (S1948); they are re-injected by the container, not serialized. - Extract per-object copy into copyObjectToAction so the copyStack loop uses no break/continue (S135); fail-closed path now returns from the helper instead of continuing the loop. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
419fb1f5c6 |
WW-5535 test(core): cover wildcard-resolved unannotated methods via real proxy (#1692)
Closes the test gap noted in the WW-5535 research: no integration test
exercised HttpMethodInterceptor against a real DefaultActionProxy resolving
a wildcard action with an unannotated method.
Uses xwork-test-allowed-methods.xml's existing <action name="Wild-*"
method="{1}"> on HttpMethodsTestAction. URL "Wild-execute" resolves to
ActionSupport.execute() (no method-level HTTP annotation); the class-level
@AllowedHttpMethod(POST) must still reject GET end-to-end.
Together with the prior MockActionProxy regression tests, this locks in
both halves of the fix:
- DefaultActionProxy.resolveMethod() sets isMethodSpecified()=true for
wildcard-resolved methods (WW-5535 / #1592)
- HttpMethodInterceptor falls back to class-level annotations when the
resolved method is unannotated (#1690)
|
||
|
ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻
|
213b83f64f |
fix(core): enforce class-level HTTP method annotations for wildcard-resolved unannotated methods (#1690)
The WW-5535 fix (commit
|
||
|
Lukasz Lenart
|
e83487f0e1 |
WW-5627 Gate CookieInterceptor through ParameterAuthorizer (#1681)
* WW-5627 add ParameterAllowlister interface and STRUTS_PARAMETER_ALLOWLISTER constant Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5627 add OgnlParameterAllowlister default implementation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5627 register ParameterAllowlister bean in struts-default DI * WW-5627 delegate ParametersInterceptor OGNL allowlisting to OgnlParameterAllowlister Also register ParameterAllowlister in DefaultConfiguration bootstrap factories so it is available in test containers (parallel to how ParameterAuthorizer was already registered there). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5627 test(cookie): failing test for unannotated setter skip Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5627 gate CookieInterceptor cookie injection through ParameterAuthorizer Adds a 5-arg `populateCookieValueIntoStack(name, value, map, stack, action)` hook that runs cookie writes through `ParameterAuthorizer.isAuthorized` and primes `ThreadAllowlist` via `ParameterAllowlister` for nested paths, then delegates to the legacy 4-arg form. The 4-arg form is `@Deprecated(since="7.2.0")` but its body is unchanged, so existing subclass overrides automatically receive only authorized cookies. Default-config behavior is preserved because the authorizer short-circuits when `requireAnnotations=false`. Existing `CookieInterceptorTest` instantiates `new CookieInterceptor()` rather than going through the container, leaving the new injected fields null. Wires explicit pass-through lambdas through a `disableAuthorizationGate(...)` helper so those tests continue to exercise default-config behavior. * WW-5627 cover CookieInterceptor authorization matrix in CookieInterceptorAnnotationTest * WW-5627 docs(cookie): document new 5-arg extension hook and deprecation * WW-5627 wire OgnlParameterAllowlister in StrutsParameterAnnotationTest fixture * WW-5627 address SonarCloud findings on PR #1681 - S1948: mark transient on the new ParameterAuthorizer/ParameterAllowlister fields in CookieInterceptor and ParametersInterceptor (the host classes are Serializable; the injected services are not). - S1874: suppress the deprecation warning on the new 5-arg populateCookieValueIntoStack — the delegation to the deprecated 4-arg form is the contract that lets existing subclass overrides participate. - S3776: extract `allowlistViaPropertyDescriptor` and `allowlistViaPublicField` from `OgnlParameterAllowlister.allowlistAuthorizedPath` to drop cognitive complexity below the threshold. - S1068: remove the unused `mapping` test fixture field. * WW-5627 clarify ParameterAllowlister contract and tidy ParametersInterceptor Rename `ParameterAllowlister#allowlistAuthorizedPath` to `primeAllowlistForPath` to make the contract explicit: the SAM is a side-effect-only priming hook that runs after `ParameterAuthorizer#isAuthorized` has already decided. A no-op return means "no priming needed or possible", never "rejected". The interface name stays channel-agnostic; only the impl class (`OgnlParameterAllowlister`) binds the priming to OGNL's `ThreadAllowlist`. Add a `LOG.debug` in `OgnlParameterAllowlister` for the case where authorization passed but no `@StrutsParameter` could be located on the root property (e.g. `ModelDriven` models without per-property annotations) so the authorize-vs-prime gap is observable instead of surfacing later as an opaque OGNL traversal failure. Drop the dead `performOgnlAllowlisting` pass-through and its unused `paramDepth` parameter from `ParametersInterceptor` — the depth check is already enforced inside `OgnlParameterAllowlister.primeAllowlistForPath`, so the outer guard was a redundant computation. No behavior change. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
09d03286f8 |
WW-5626 per-property authorization for Jackson REST handlers (#1674)
* WW-5626 spike: validate Jackson per-property authorization mechanism Validates that the Approach C design is feasible before committing to a detailed implementation plan. Wraps each SettableBeanProperty via BeanDeserializerModifier; intercepts deserializeAndSet to authorize against a path built from a ThreadLocal Deque; uses skipChildren() to discard unauthorized values; uses [0] suffix for collection/map/array elements to match ParametersInterceptor depth semantics. Findings: - Delegating base class via 'protected delegate' field is the right pattern - addOrReplaceProperty(prop, true) is the correct builder API - Reject-at-parent skips all nested deserialization (better security than two-phase copy: setter side effects on unauthorized properties never fire) - JavaType#isCollectionLikeType/isMapLikeType/isArrayType detects the indexed-path case Spike is kept under .../spike/ as a learning artifact; it will be replaced by production code + tests in subsequent commits. * WW-5626 add ParameterAuthorizationContext for deserializer-level authorization Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5626 address review feedback on ParameterAuthorizationContext Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5626 add AuthorizationAwareContentTypeHandler marker interface * WW-5626 add AuthorizingSettableBeanProperty for Jackson per-property authorization * WW-5626 add ParameterAuthorizingModule installing the property wrapper on Jackson mappers * WW-5626 register ParameterAuthorizingModule on default Jackson REST handlers * WW-5626 use AuthorizationAwareContentTypeHandler path when handler supports it * WW-5626 add integration tests proving the new Jackson authorization path is used * WW-5626 deprecate XStreamHandler in favor of JacksonXmlHandler * WW-5626 remove Jackson auth spike; replaced by production tests * WW-5626 make JuneauXmlHandler authorization-aware via post-parse walk Implements AuthorizationAwareContentTypeHandler. When ParameterAuthorizationContext is active (set by ContentTypeInterceptor when requireAnnotations=true), the handler walks the parsed result tree and copies only authorized properties to the target, descending into nested beans/collections/maps/arrays with indexed-path semantics ([0] suffix) for parity with ParametersInterceptor. Note: Juneau parses the entire result tree before our walk runs, so setter side effects on transient nested objects can fire even for unauthorized properties — those transient objects are then discarded. This is functionally equivalent to the legacy two-phase copy in ContentTypeInterceptor; only the Jackson handlers achieve the stronger guarantee where unauthorized subtrees are never instantiated at all (they use Jackson's BeanDeserializerModifier + skipChildren). When no context is bound (default config), behavior is unchanged: parser.parse + BeanUtils.copyProperties. * WW-5626 add JuneauXmlHandler integration tests for @StrutsParameter authorization * WW-5626 test(rest): cover JuneauXmlHandler post-parse walk for collections, maps, arrays Sonar reported 51 uncovered new lines in JuneauXmlHandler (48.8% coverage on the post-parse authorization walk — the security-critical code path the branch exists to introduce). Add integration coverage for the previously-uncovered branches: - collection-of-scalars (List<String> tags) - collection-of-beans (List<Address> addresses) - map-of-scalars (Map<String,String> attributes) - array-of-scalars (String[] aliases) - empty collection - malformed XML wrapped as IOException Also drop two unnecessary casts (Sonar S1905) on lines 243/252 — the unchecked conversion happens at the return statement, the explicit casts were redundant under the existing @SuppressWarnings("unchecked"). Add @Override on the inline AnyConstraintMatcher.matches override (Sonar S1161). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5626 test(rest): cover AuthorizingSettableBeanProperty builder-path deserialization Sonar reported 11 uncovered new lines on AuthorizingSettableBeanProperty (66.7% coverage). All 11 are in deserializeSetAndReturn — the alternate Jackson entry point used for builder-pattern deserialization, never triggered by setter-based fixtures like Person. Add an @JsonDeserialize(builder=...) fixture (ImmutablePerson) that forces Jackson to use BuilderBasedDeserializer, which dispatches property writes through deserializeSetAndReturn. Three new tests exercise the path: inactive-context pass-through, top-level authorization, and full rejection. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5626 refactor(rest): extract helpers from ContentTypeInterceptor.intercept Sonar S3776 flagged intercept() at cognitive complexity 16 (limit 15). Extract the body-handling branches into named helpers: - openBodyReader: encoding-aware reader from the request InputStream - applyRequestBody: dispatcher between requireAnnotations on/off paths - applyWithAuthorizationContext: bind + delegate + unbind for AuthorizationAware handlers - applyTwoPhaseDeserialize: legacy fresh-instance + copyAuthorizedProperties path intercept() drops to ~12 lines and reads as a flat sequence: resolve target, delegate body application, invoke. Each helper carries the comment that explains the security model for its branch. Add @Override on the inline AnyConstraintMatcher.matches override (Sonar S1161). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
690c4c2737 |
WW-5626 cleanup follow-ups for @StrutsParameter JSON/REST enforcement (#1673)
* WW-5626 add ParameterAuthorizer#resolveTarget for centralized ModelDriven resolution Move the ValueStack peek logic that derives the target object from action+ModelDriven state out of ParametersInterceptor and into ParameterAuthorizer. Callers that need both authorization and the resolved target (for downstream OGNL allowlisting) can now call resolveTarget once and reuse the result. * WW-5626 delegate ModelDriven target resolution to ParameterAuthorizer Replace the inline ValueStack peek in ParametersInterceptor#isParameterAnnotatedAndAllowlist with a call to ParameterAuthorizer#resolveTarget. The ModelDriven import is no longer needed in this class. * WW-5626 defensively skip non-String JSON keys in authorization filter The (String) cast in filterUnauthorizedKeysRecursive threw ClassCastException for any custom JSONReader producing non-String keys. Replace with an instanceof pattern that debug-logs and skips entries whose key cannot be converted to a parameter path. * WW-5626 add real JacksonJsonHandler integration tests for @StrutsParameter filtering The existing ContentTypeInterceptorTest uses mock ContentTypeHandlers, so its requireAnnotations=true tests verify only that intercept() returns SUCCESS — they assert nothing about which properties were actually filtered. These integration tests use a real JacksonJsonHandler + a real StrutsParameterAuthorizer to verify end-to-end property-level filtering for top-level annotated/unannotated properties and nested properties at varying authorized depths. The SecureRestAction fixture documents a semantic divergence: REST's recursive copy authorizes each path level independently, so depth-0 authorization on the top-level property requires @StrutsParameter on the setter even when nested field access is the actual goal. ParametersInterceptor only requires the getter annotation. This divergence is tracked for the Approach C refactor. * WW-5626 make ParameterAuthorizer#resolveTarget a default method to preserve SAM Making resolveTarget abstract broke ParameterAuthorizer as a functional interface, which the existing JSON and REST plugin tests rely on for lambda-based stubs: interceptor.setParameterAuthorizer((parameterName, target, action) -> true); The default returns the action unchanged — adequate for lambda test stubs whose authorization decisions don't depend on the resolved target. The production implementation (StrutsParameterAuthorizer) overrides this with the proper ModelDriven value-stack peek. |
||
|
quactv
|
c3a887085d |
WW-5624: Enforce @StrutsParameter on JSON/REST body deserialization (#1657)
* WW-5624 fix(security): enforce @StrutsParameter on JSON/REST body deserialization Extract ParameterAuthorizer service from ParametersInterceptor to share @StrutsParameter annotation enforcement across all input channels. The json-plugin (JSONInterceptor) and rest-plugin (ContentTypeInterceptor) previously bypassed @StrutsParameter checks when deserializing request bodies, allowing mass assignment even when struts.parameters.requireAnnotations=true. Changes: - New ParameterAuthorizer interface and DefaultParameterAuthorizer impl - JSONInterceptor: filter unauthorized Map keys before populateObject() - ContentTypeInterceptor: two-phase deserialization (fresh instance then copy authorized properties) when requireAnnotations=true; direct deserialization for backward compat when disabled - OGNL ThreadAllowlist side effects remain in ParametersInterceptor only - Full DI wiring: struts-beans.xml + StrutsBeanSelectionProvider + DefaultConfiguration - 15 new unit tests for ParameterAuthorizer, 2 for JSON plugin, 2 for REST plugin; 32 existing regression tests verified * WW-5624 address review feedback from lukaszlenart on PR #1657 1. Rename DefaultParameterAuthorizer → StrutsParameterAuthorizer per Struts naming convention (inline suggestion) 2. Narrow ModelDriven exemption: require action instanceof ModelDriven before exempting target from @StrutsParameter checks. Prevents non-ModelDriven root objects (e.g. JSONInterceptor.root) from bypassing annotation enforcement. 3. Recursive JSON key filtering: filterUnauthorizedKeys() now recurses into nested Maps and Lists, building dot-notation paths (e.g. "address.city") for path-aware @StrutsParameter(depth=N) checks. 4. Deep REST property copy: copyAuthorizedProperties() now recurses into nested bean types with path-aware authorization. Collections, Maps, primitives, and java.lang/java.time types are copied directly. 5. Null-skip semantics preserved and documented: in two-phase deserialization, null in freshInstance is indistinguishable from "not present in request" — clearing would destroy pre-initialized fields. Kept as intentional design choice with inline documentation. 6. No-arg constructor fallback: when target class lacks a no-arg constructor, falls back to single-phase deserialization with post-scrub of unauthorized properties, preserving backward compat. 7. New regression tests: - Non-ModelDriven target with different object (must not exempt) - Nested JSON keys recursively filtered - Non-action root object still checked by authorizer All 280+ core tests, 124 JSON tests, 76 REST tests pass with 0 regressions. * WW-5624: v3 — fix indexed-path depth parity with ParametersInterceptor Four gaps identified by lukaszlenart's April 10 review are now fully addressed: 1. JSON filterUnauthorizedList: pass prefix+"[0]" instead of bare prefix so that list element properties gain one extra '[' in their path — e.g. "publicPojoListDepthOne[0].key" (depth=2) is now correctly rejected when @StrutsParameter(depth=1), matching ParametersInterceptor semantics. Also recurse into nested List<List<Map>> via an else-if branch. 2. REST copyAuthorizedProperties: add authTarget parameter (always = root action/model, passed unchanged through all recursion levels). isAuthorized() now checks the full path against the root class, so "address.city" is looked up on the action, not on the Address object. 3. REST Collection/Map/array deep authorization: replaced the as-is copy with deepCopyAuthorizedCollection(), deepCopyAuthorizedMap(), and deepCopyAuthorizedArray() helpers — each iterates elements with path+"[0]" prefix, authorizing every complex element individually. No-arg fallback skips the element rather than copying an unfiltered object graph (security fix over plan's original as-is suggestion). 4. REST scrubUnauthorizedProperties: now fully recursive via scrubUnauthorizedPropertiesRecursive() — visits nested beans, collection elements, and map values with authTarget always pointing to the root. Includes identity-based visited-set to guard against circular reference cycles. Tests: core 2920 + json 124 + rest 76 = 3120, 0 failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5624: v3.1 — fix collection type, identity set, isNestedBeanType coverage Three correctness/security issues identified by independent review: 1. deepCopyAuthorizedCollection/deepCopyAuthorizedMap type preservation: Previously always returned ArrayList/LinkedHashMap. If the action field is typed Set<Pojo> or SortedMap<K,V>, writeMethod.invoke would throw IllegalArgumentException. Now: SortedSet→TreeSet, Set→LinkedHashSet, List→ArrayList; SortedMap→TreeMap, Map→LinkedHashMap. 2. scrubUnauthorizedPropertiesRecursive visited-set identity safety: Replaced Set<Integer>+System.identityHashCode (not collision-safe) with Collections.newSetFromMap(new IdentityHashMap<>()) which uses reference equality (==). A hash collision could have caused a valid nested object to be skipped, leaving unauthorized properties un-scrubbed. 3. isNestedBeanType now excludes all standard-library leaf packages: java.util.* non-Collection/Map types (UUID, Currency, Locale, Date), java.time.* (all temporal types, not just Temporal subinterface), java.net.*, java.io.*, java.nio.*. Previously UUID etc. would return true, causing the code to recurse into their internal fields and silently drop the value when no @StrutsParameter annotation matched. Tests: json 124 + rest 76 = 200, 0 failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * WW-5624: v4 — close bulk-copy fallback, reject body when no no-arg ctor Two remaining gaps addressed per lukaszlenart's April 11 review: 1. copyAuthorizedProperties bulk-copy fallback removed: When a nested target bean is null and createFreshInstance fails (no no-arg constructor), the previous code fell back to writeMethod.invoke(target, sourceValue) — copying the whole nested object graph without per-path authorization. Now logs a warning and skips the property entirely (same policy as deepCopyAuthorizedCollection elements with no no-arg constructor). 2. Top-level no-arg constructor fallback changed from scrub to reject: When requireAnnotations=true and the target class has no no-arg constructor, body deserialization is now rejected entirely (handler.toObject is never called). The previous best-effort scrub path could not guarantee that all nested unauthorized properties were nulled out. scrubUnauthorizedProperties and its recursive helper are removed as dead code. Tests: rest 76, 0 failures. --------- Co-authored-by: tranquac <tranquac@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
quactv
|
8d6f13904f |
fix(core): WW-5623 HTML-encode form action in PostbackResult to prevent XSS (#1653)
* fix(core): HTML-encode form action in PostbackResult to prevent XSS PostbackResult.doExecute() embeds finalLocation into a <form action=""> attribute via raw string concatenation without HTML encoding. A double quote in the location breaks out of the attribute, enabling reflected XSS. The response Content-Type is text/html (line 103). This is an encoding inconsistency: form field names and values at lines 218-219 ARE properly URL-encoded via URLEncoder.encode(), but the form action attribute was not encoded at all. Add encodeHtml() to escape &, ", <, > in finalLocation before embedding it in the HTML form tag, consistent with the existing encoding approach for form field values in the same class. * fix(core): WW-5623 use StringEscapeUtils and add regression tests Address review feedback from @lukaszlenart: - Replace custom encodeHtml() with StringEscapeUtils.escapeHtml4() for consistency with the rest of Struts core (DefaultActionProxy, Property, TextProviderHelper all use StringEscapeUtils) - Add 3 focused unit tests in PostbackResultTest: - testFormActionHtmlEscaping: XSS payload with attribute breakout - testFormActionEscapesAllHtmlSpecialChars: covers ", &, <, > - testFormActionCleanLocationUnchanged: regression for clean URLs --------- Co-authored-by: tranquac <tranquac@users.noreply.github.com> |
||
|
aaaZayne
|
f4c6349283 |
introduce private method to remove clones (#1666)
* introduce private method to remove clones * Update naming |
||
|
Lukasz Lenart
|
d276b2dded |
WW-5622 perf(core): optimize Hibernate proxy detection when Hibernate is absent (#1649)
Detect Hibernate availability once at class-load time via Class.forName() and short-circuit all Hibernate-related methods immediately when absent. This eliminates repeated LinkageError/NoClassDefFoundError exceptions that cause significant performance degradation in applications without Hibernate on the classpath. Fixes https://issues.apache.org/jira/browse/WW-5622 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
620fcbd152 |
WW-5621 Harden XML parsers against Entity Expansion (Billion Laughs) attacks (#1642)
Modern JDKs (7u45+) already protect against this attack with a built-in 64K entity expansion limit. These changes add defense-in-depth hardening and remove unnecessary attack surface. - Remove unused parseStringAsXML feature from StringAdapter to eliminate a theoretical XML Entity Expansion vector - Deprecate setParseStringAsXML() and getParseStringAsXML() for removal - Enable SECURE_PROCESSING feature in DigesterDefinitionsReader - Add unit test verifying JDK's entity expansion limit rejects Billion Laughs payloads - Add research document with vulnerability analysis Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
2215b6873c |
WW-5537 Resolve classloader/memory leaks during Tomcat hot deployment (#1632)
* WW-5537 Add InternalDestroyable and ContextAwareDestroyable interfaces Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 ContainerHolder: ThreadLocal with AtomicLong generation counter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 FinalizableReferenceQueue: volatile instance, join, classloader null Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 ScopeInterceptor.clearLocks: add synchronized block Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 CompoundRootAccessor, DefaultFileManager: implement InternalDestroyable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Add InternalDestroyable adapter classes for static cache cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Register InternalDestroyable beans in struts-beans.xml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 JSON plugin: add JSONCacheDestroyable for BeanInfo cache cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Dispatcher.cleanup: refactor into focused methods with InternalDestroyable discovery Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Rewrite DispatcherCleanupTest for InternalDestroyable discovery Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Add log4j-web for proper Log4j2 lifecycle in Servlet container Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Dispatcher.destroyObjectFactory: add early return on null, use pattern matching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Fix @since annotations: 7.1.0 -> 7.2.0 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Add Container.destroy() to clear internal caches on undeploy Container now exposes a destroy() method that clears factories, injectors, constructors, and ThreadLocals. This releases Class<?> keys and JDK DelegatingClassLoader instances that pin the webapp classloader. DefaultConfiguration.destroy() calls container.destroy() and reloadContainer() delegates to destroy() to avoid duplication. Also fixes JSONCacheDestroyable referencing non-existent DefaultJSONWriter (renamed to StrutsJSONWriter). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Fix Container.destroy(): don't clear factories, don't call from reloadContainer factories must remain intact because existing code holds direct references to the Container after destroyConfiguration() and expects it to still resolve dependencies (e.g. during configuration reload). reloadContainer() reverted to clearing packageContexts/loadedFileNames directly — calling destroy() there nulled the container reference and cleared state needed during the bootstrap transition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Restore destroy() call in reloadContainer() The test failures were caused by factories.clear() in Container.destroy(), not by calling destroy() from reloadContainer(). Now that factories.clear() is removed, destroy() is safe to call here — it clears packageContexts, loadedFileNames, and the container's reflection caches in one place. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * WW-5537 Fix Sonar issues: thread-safe FinalizableReferenceQueue, empty method comments - Replace volatile field with AtomicReference in FinalizableReferenceQueue for proper thread safety using getAndSet() - Add comments to empty destroy() implementations in test mocks - Replace deprecated new URL() with URI.toURL() in DispatcherCleanupTest - Add comments to empty listener methods in DispatcherCleanupTest Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
55e268b009 |
WW-2963 default-action-ref fails to find wildcard named actions (#1614)
* WW-2963 fix(core): resolve default-action-ref via wildcard matching When default-action-ref names an action that only exists as a wildcard pattern (e.g., "movie-list" matching "movie-*"), the fallback now tries wildcard matching after the exact map lookup fails. This mirrors the exact→wildcard resolution already used for request action names. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * WW-2963 refactor(core): reduce cognitive complexity of findActionConfigInNamespace Extract default-action-ref resolution into findDefaultActionConfig() and replace the deeply nested if-pyramid with early returns, reducing the nesting depth from 5 to 1 to satisfy Sonar's complexity threshold. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Made-with: Cursor --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
4c94c4f89a |
WW-5549 Fix I18nInterceptor supportedLocale breaking request_locale (#1594)
* fix(i18n): ensure request_locale takes precedence over Accept-Language when supportedLocale is configured When supportedLocale was configured on the I18nInterceptor, the Accept-Language header match in AcceptLanguageLocaleHandler.find() returned early before SessionLocaleHandler/CookieLocaleHandler ever checked their explicit locale parameters (request_locale, request_cookie_locale). This made it impossible to switch locale via request parameters when supportedLocale was set. Changes: - Reorder AcceptLanguageLocaleHandler.find() to check request_only_locale before Accept-Language matching - Reorder SessionLocaleHandler.find() to check request_locale before super - Reorder CookieLocaleHandler.find() to check request_cookie_locale before super - Add isLocaleSupported() helper to validate locales against supportedLocale - Filter all locale sources (params, session, cookies) through supportedLocale - Add 4 tests covering the bug scenario and supportedLocale filtering 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * test(i18n): cover missing supportedLocale locale-selection paths Add regression tests for unsupported request_cookie_locale fallback, stored cookie revalidation, and request_only_locale precedence to lock in WW-5549 behavior across remaining branches. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(i18n): extract locale handlers with deprecated inner wrappers Move locale handler implementations into a dedicated interceptor.i18n package with reusable abstract bases, keep thin deprecated inner wrappers in I18nInterceptor for one release-cycle compatibility, and document the LocaleHandler contract. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(i18n): validate request_only_locale against supportedLocale and fix Accept-Language fallback RequestLocaleHandler.find() now checks isLocaleSupported() before returning, preventing unsupported locales from slipping through via the request_only_locale parameter. AcceptLanguageLocaleHandler.find() now returns the first Accept-Language locale when supportedLocale is empty, fixing ACCEPT_LANGUAGE storage mode with no filter configured. Also includes refactoring: deprecated inner classes collapsed with LocaleHandlerAdapter, shouldStore field encapsulated via disableStore(), logger pattern standardized to private static final, and class-level JavaDoc added to handler classes. Made-with: Cursor --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
Lukasz Lenart
|
4d2eb93835 |
fix(core): correct isMethodSpecified() for wildcard-resolved methods (#1592)
DefaultActionProxy.resolveMethod() unconditionally set methodSpecified=false
when the method was not passed explicitly, including when it was resolved from
ActionConfig (e.g., wildcard substitution like method="{1}"). This caused
HttpMethodInterceptor to skip method-level annotation checks for wildcard
actions, falling back to class-level annotations instead.
Move methodSpecified=false inside the inner branch that defaults to "execute",
so config-resolved methods (including wildcard-substituted ones) correctly
report isMethodSpecified()=true. Update Javadoc to reflect the corrected
semantics.
Fixes [WW-5535](https://issues.apache.org/jira/browse/WW-5535)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Lukasz Lenart
|
ca740ed8fb |
WW-5514 Add StrutsProxyService for proxy detection and resolution (#1586)
* feat(proxy): WW-5514 add StrutsProxyService for proxy detection and resolution Introduces a configurable ProxyService interface and StrutsProxyService implementation for detecting and resolving Spring AOP/Hibernate proxies. Key changes: - Add ProxyService interface with isProxy, ultimateTargetClass, and resolveTargetMember methods - Add StrutsProxyService implementation using configurable caches - Add ProxyCacheFactory and StrutsProxyCacheFactory for cache management - Integrate ProxyService into ChainingInterceptor, ParametersInterceptor, and SecurityMemberAccess - Add integration test with Spring AOP proxied action chaining - Add configuration constants for proxy cache type and size The StrutsProxyService correctly handles: - Spring CGLIB proxies (class-based) - Spring JDK dynamic proxies (interface-based) - Hibernate entity proxies - Member resolution for allowlist checking Fixes WW-5514 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * test(proxy): WW-5514 add ProxyService integration tests for Spring proxies Add integration tests to SpringProxyUtilTest that verify the new ProxyService works correctly with real Spring AOP proxies, alongside the existing deprecated ProxyUtil tests. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(proxy): WW-5514 address PR review feedback for proxy caches Remove targetClassCache from StrutsProxyService to avoid memory leak (object-keyed cache reintroduced from PR #1578). Change default proxy cache type to wtlfu to align with all other caches. Switch deprecated ProxyUtil static caches to BASIC to remove hard Caffeine dependency. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
dependabot[bot]
|
d2810d42f0 |
build(deps): bump org.apache.commons:commons-fileupload2-jakarta-servlet6 (#1584)
Bumps org.apache.commons:commons-fileupload2-jakarta-servlet6 from 2.0.0-M4 to 2.0.0-M5. --- updated-dependencies: - dependency-name: org.apache.commons:commons-fileupload2-jakarta-servlet6 dependency-version: 2.0.0-M5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
Lukasz Lenart
|
c90bb70c25 |
feat(ui): WW-3429 add configurable checkbox hidden field prefix (#1570)
Add struts.ui.checkbox.hiddenPrefix constant to allow configuring the checkbox hidden field prefix, addressing HTML validation warnings about double underscores while maintaining backward compatibility. Changes: - Add STRUTS_UI_CHECKBOX_HIDDEN_PREFIX constant to StrutsConstants - Add default value __checkbox_ to default.properties - Update Checkbox component to inject and pass prefix to templates - Update CheckboxInterceptor to use configurable prefix - Update simple/checkbox.ftl and html5/checkbox.ftl templates - Update CheckboxHandler in javatemplates plugin - Add tests for configurable prefix functionality - Fix bug in CheckboxHandler where value was incorrectly prefixed Configuration example: struts.ui.checkbox.hiddenPrefix=struts_checkbox_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Lukasz Lenart
|
a0a213f7c5 |
feat(security): WW-5294 add warning when JSP tags accessed directly (#1569)
Add security warning to TagUtils.getStack() that logs when JSP tags are rendered outside of action scope (direct JSP access). This helps developers identify potential security issues where JSPs are accessed directly without going through the Struts action flow. The warning message includes a link to the security documentation at https://struts.apache.org/security/#never-expose-jsp-files-directly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com> |
||
|
dependabot[bot]
|
e3d09dfc47 |
WW-5613 build(deps): bump ognl:ognl from 3.4.8 to 3.4.10 (#1567)
OgntUtil has been extended to properly pass root object if needed Bumps [ognl:ognl](https://github.com/orphan-oss/ognl) from 3.4.8 to 3.4.10. - [Release notes](https://github.com/orphan-oss/ognl/releases) - [Commits](https://github.com/orphan-oss/ognl/commits) --- updated-dependencies: - dependency-name: ognl:ognl dependency-version: 3.4.10 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |