Gmail's linkifier rewrites URLs server-side as the draft is stored, so the
four link lines in the 6.11.0 vote draft arrived wrapped in
https://www.google.com/url?q=...&source=gmail&ust=... and the 72-column
wrap was reflowed. All three ways of passing the body through the Gmail
tool were tested and all are rewritten; body-only is the least-bad, and
supplying htmlBody is worse (HTML-only mail, or a plain part whose visible
text is the wrapped URL).
Record the constraint and make the workaround structural: the skill now
produces the draft *and* a file with the exact body, so the release
manager fixes it with one select-all-paste instead of hand-editing four
URLs. Also flag re-running the draft-update tool on an already-corrected
draft, which re-mangles it.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* docs: design for a creating-release-vote-mail skill
Specifies a sibling to creating-version-notes that drafts the
[VOTE] Apache Struts X.Y.Z mail once the Version Notes page, GitHub
release and staged artifacts are published.
Derived from the 7.2.1 and 6.10.0 vote mails: the body is a plain-text
rendering of the Version Notes page, the ASF vote boilerplate is frozen
in a template, and the mail goes to dev@ only with private@ in Bcc.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: implementation plan for creating-release-vote-mail
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: revise vote-mail spec from baseline testing
Three baseline agents drafted the 7.3.0 vote mail with no skill present.
Two-thirds of the specified content taught nothing - rendering from the
page, precondition checks, security truncation, checkboxes, subject and
Bcc were all done correctly unassisted, so they become cross-references.
Reverses two decisions on the evidence: Rejected requests are included
(3/3 agents reproduced them, as does the page's own framing) and the
staging URL aligns on content/groups/staging/ with the page and the
[TEST] mail.
Adds the rule that a vote carries no security information on any
channel, after a baseline kept dev@ neutral and then routed severities,
bulletin ids and reporter detail into a private@ companion mail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: add creating-release-vote-mail skill
Drafts the [VOTE] Apache Struts X.Y.Z mail as a rendering of the
published Version Notes page. Scoped to the five failures three baseline
agents actually made without it: security detail routed through a
private companion mail, a body grown beyond the page, user@ on Cc,
sending rather than drafting, and a paragraph inserted into the frozen
vote boilerplate.
Everything the baselines already got right - rendering from the page,
precondition checks, security truncation, checkboxes, subject, Bcc - is
cross-referenced to creating-version-notes and creating-security-bulletins
rather than restated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: close the vote-mail skill loop after GREEN runs
Template now states that a page section's own introductory sentence
comes with it, after a verification run kept the Rejected requests
preamble on judgement the skeleton did not support.
Chains creating-version-notes to the vote as its next step, and replaces
the pre-testing plan with the record of the RED/GREEN/REFACTOR cycle -
including the contamination lesson: baselines must run in a worktree
that does not contain the design document.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: record why private@ is on the vote mail
It is there for reach - not every PMC member follows dev@ and PMC votes
are the binding ones - not for confidentiality. Stating this closes the
loophole where an agent reads private@ as a confidential channel and
routes advisory detail through it, which is what the disclosure baseline
did.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* docs: refine creating-version-notes from the 7.3.0 release run
Seven changes, each from something the 7.3.0 run got wrong or had to invent.
Breaking changes were far too long: the old guidance held up a paragraph-length
example and asked for what replaces the old behaviour, which produces prose the
ticket already carries. It is now one sentence plus the ticket link, whose job is
to tell a reader whether to open the ticket.
Reconciliation knew only "listed" or "dropped", so a ticket resolved Won't Do got
silently dropped. Such tickets now go under a Rejected requests section - not into
a type section, where they would read as delivered. Deprecations get their own
section for the same reason: nothing stops working yet.
Security summaries are no longer reproduced verbatim when the ticket itself names
the bypass or the leak; they are truncated at a clause boundary and the cut is
reported to the release manager.
The GitHub release range is now fixed at source by passing previous_tag_name
rather than generating wrong notes and pruning them, and creating a release that
does not exist yet is covered alongside editing one that does.
Also adds the test-build announcement mail as the closing step, the full page
section order, split verification by set-diff, and the note that content_file is
rejected for paths outside the repository.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: address the test-build announcement to both dev@ and user@
The recipient list was left as the release manager's call because 6.11.0 went to
dev@ and 7.3.0 to user@. Both were mistakes: the announcement goes to both lists,
Bcc private@. A build announced to one list reaches half the people who could
find a problem while the vote is open.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* docs: add creating-version-notes skill and page template
Covers Version Notes pages on the cwiki for every maintenance line - 6.x and
7.x share one structure, so the release line changes the data, not the process.
The published pages show that cloning the previous release's page reliably
leaves a half-updated link, differently each time, so the skill starts from a
template instead. The template corrects three defects the published pages
carry: a malformed code-macro parameter, hard-coded macro ids duplicated
across releases, and trailing empty divs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: extend creating-version-notes with the Migration Guide step
Exercising the skill on Version Notes 6.11.0 surfaced four rules it was missing:
- The page is a child of Migration Guide (13981), which is also the index; a page
not listed there is unreachable. Records how to update that section safely and
why its version diff renders empty even when the edit landed.
- Reconcile through the ticket's linked PR files. WW-5630 reads "Performance Issue
SecurityMemberAccess" but was fixed in ConfigParseUtil, so grepping commit
subjects or the class in the title wrongly concludes the backport is missing.
- Patch-level dependency bumps ship untick eted by design, so a pom version ahead
of the ticket text is expected rather than a reconciliation gap.
- The Staging Repository block is included on every line, not an open decision.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: cover the GitHub release notes in creating-version-notes
A release also has GitHub release notes at the STRUTS_X_Y_Z tag, and the
generated body needs two corrections before it is fit to publish.
Entries are split into What's Changed and a nested Dependencies section by
whether they carry a WW ticket, not by author: a Dependabot PR with a ticket is
release content and stays above, while an untick eted dependency bump from anyone
moves down.
More importantly, the generated Full Changelog range is not trustworthy. GitHub
picks the previous tag by reachability, and our release branches get renamed and
re-imported, so it reaches too far back - for 6.11.0 it chose STRUTS_6_8_0 and
listed 88 entries that had already shipped in 6.9.0 and 6.10.0. The range must be
verified with git log PREV..THIS, which works across unrelated histories.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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>
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>
* 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>
* 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.
* docs: add creating-security-bulletins skill and bulletin template
Captures the editorial process for S2-XXX security bulletins, which has
been implicit until now, as a companion to the existing
triaging-security-reports skill. Triage establishes what is true; this
covers what the published page is allowed to say.
The load-bearing part is the disclosure budget. Earlier bulletins
explained causes and mitigations in enough detail to build working
exploits, and the project moved away from that; this writes the rule
down and extends it past Problem to Backward compatibility and
Workaround, which is where a carefully guarded advisory tends to leak.
Also records conventions that were previously tacit:
- Affected Software lists voted releases only, never a build that failed
its test period, and never a range inferred from git tags
- ratings match a definition on the Security Bulletins page, which is the
only authority since the four-level naming postdates older advisories
- workarounds are verified in source or not published, including the
claim that none exists
- behavioural claims are derived from the fix diff rather than its commit
message, and the fix is confirmed merged before publication
- who is *not* affected is stated explicitly, since scoping shrinks the
population that has to act
bulletin-template.md is the source of truth for page structure and
carries the storage-format skeleton; the restricted wiki template becomes
a rendered copy of it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: require running a supplied PoC and finding the control case
Two gaps surfaced by triaging a real report against the skill.
The claim table told the triager to trace a PoC through the code but
never to run it, even when the reporter supplied a runnable one. Reading
and inferring is weaker evidence than executing, and the report that
exposed this shipped a JUnit test and the exact command to run it.
More importantly, the skill never said to look for the control case. A
single odd behaviour is nearly always arguable as intended -- a type that
opts into dynamic binding can be read as the developer asking for it. What
settles the question is the sibling that behaves correctly under the same
input: when one dispatch path rejects an unannotated member and its
neighbour does not, the control is incomplete rather than by design, and
that divergence is the finding. Two independent triages of the same report
both relied on this argument, and neither the skill nor THREAT_MODEL.md
prompted for it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* ci(owasp): cap job timeout and disable NVD auto-update in check step
The OWASP job intermittently failed with no reason other than timeouts.
Root cause is the unreliable NIST NVD feed (see dependency-check#8633):
keyless NVD downloads are heavily rate-limited and stall.
Two fixes:
- Add timeout-minutes: 30 so a hung NVD download fails fast instead of
dragging to the 6h GitHub Actions default.
- Add -DautoUpdate=false to the check step so it reads only the cache
populated by the preceding update-only step. Previously the check step
carried neither the mirror datafeed URL nor the API key, so on any cache
staleness/miss it synced directly against NIST - the unreliable path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* ci(owasp): fall back to NVD mirror when the API update fails
The NIST NVD API is unreliable even with an API key (retries exhausted,
see dependency-check#8633). Previously the mirror datafeed was used only
when no API key was present, so apache/struts always took the flaky API
path and never the mirror.
Make the API update step continue-on-error and run the mirror update as a
fallback when the API step fails (or when no API key is configured).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* OWASP + Github workflow updates
* Library updates
* Dependancy suppression cleanup
* #1667 PR Review updates, use NIST_NVD_API_KEY when available else use mirror for forks not configured, remove workflow_call for now since we don't on call
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>
* WW-4858 fix(json): evaluate name allowlist at leaf keys only
The JSON population filter walked the object tree and applied every name
check at every node before recursing. Accepted name patterns and the
ParameterNameAware callback target the full dotted binding path, so gating
an intermediate node (e.g. "bean") against a leaf-specific rule dropped the
entire subtree before the leaf ("bean.stringField") was ever evaluated —
diverging from ParametersInterceptor, which only evaluates complete leaf
names. For arrays it also meant the accepted allowlist judged the container
name instead of the element path.
Split the per-key gate: length, excluded patterns, @StrutsParameter
authorization and property filters stay per-node (exclusion is prefix-safe
and authorization is intentionally hierarchical); accepted patterns and
ParameterNameAware move to leaf keys only, including scalar array elements
at their indexed path ("items[0]"). This reproduces the flat-path semantics
exactly. Excluded/include-property behavior is unchanged.
Tests: nested-object leaf populates under a leaf-targeting accepted pattern
and a ParameterNameAware action that rejects the intermediate node; accepted
patterns now apply to the array element path; nested include-property
filtering still works.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 fix(json): apply per-node checks to scalar array elements
Scalar list elements were gated only by the leaf name-allowlist and value
checks, skipping the per-node checks (length, excluded patterns,
@StrutsParameter authorization, property filters). That left the JSON path
more permissive than ParametersInterceptor, which evaluates all of these
against the full indexed name "items[0]".
Apply isAcceptableNode(elementPrefix, ...) to scalar list elements so an
element is gated exactly as the flat path gates "items[0]". Note this makes
scalar-list @StrutsParameter authorization use the element path (depth 1,
read method) rather than only the container (depth 0), matching the flat
path.
Tests: excluded name pattern and @StrutsParameter authorization now apply at
the list element path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 refactor(json): extract keyTypeName helper to lower cognitive complexity
Move the non-String-key logging ternary out of filterUnacceptableKeysRecursive
into a keyTypeName helper. Pure extraction, no behavior change; drops the
method's cognitive complexity from 17 to 14, under Sonar's S3776 threshold.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5604 Add CdiProxyService to detect Weld client proxies
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5604 Register CdiProxyService as the active ProxyService
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5604 Address review: positive allowlist test, guard Weld member check, fix javadoc
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5604 Add WELD_AVAILABLE guard and weld-api version property
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5604 Cover null, non-proxy, non-method and Weld-absent paths
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5604 Remove unreachable guard and cover unwrap fallbacks
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 docs: add Bootstrap 5.3.x sample-app migration design
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 docs: add Bootstrap 5 migration implementation plan
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 build: add Bootstrap 5, Bootstrap Icons, showcase jQuery webjars
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(showcase): serve Bootstrap 5 and jQuery via webjars
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): serve html5 demo Bootstrap CSS via webjar
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(showcase): migrate navbar and top-level pages to Bootstrap 5
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): migrate leftover Bootstrap 2 icon and fixed-navbar classes
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(showcase): migrate tag-demo pages to Bootstrap 5
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(showcase): migrate validation-demo pages to Bootstrap 5
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(showcase): migrate fileupload and conversion pages to Bootstrap 5
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(showcase): migrate wait, token and empmanager pages to Bootstrap 5
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): remove BS3 carets and well class
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): use ms-auto for right nav and add nav-link to Home
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(rest-showcase): serve Bootstrap 5 CSS via webjars
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 feat(rest-showcase): migrate JSP markup to Bootstrap 5
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(rest-showcase): migrate legacy BS2/BS3 grid classes to Bootstrap 5
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): drop obsolete css/js excludePattern so webjar JS is served
The old struts.action.excludePattern whitelisted the vendored /styles/*.css
and /js/*.js layout for container serving. After moving Bootstrap/jQuery to
webjars under /static/webjars/**, the '.*/js/.*\.js' entry matched the webjar
JS path (e.g. bootstrap.bundle.min.js) and excluded it from Struts' static
handler, so it fell through to the container and 404'd. Remaining webapp assets
(prettify.js, main.css) are served via default-servlet fall-through.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(samples): migrate remaining BS2/BS3 classes (tables, buttons, progress, forms, navbar)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 chore(showcase): comment out verbose debug loggers
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): migrate Bootstrap 5 markup in ftl/vm/html templates
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 test(showcase): disable JS in FreeMarkerManagerTest for Bootstrap 5
HtmlUnit's JS engine cannot parse Bootstrap 5's ES6 (bootstrap.bundle.min.js
uses 'class'), and the decorator now serves it, so the default WebClient threw
on script error. The test only asserts server-rendered FreeMarker output, so
JavaScript is disabled (matching Html5TagExampleTest).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): replace dead Bootstrap 2 form and alert classes
Migrate new-person.ftl form off BS2 control-group/controls/form-actions to
Bootstrap 5 (mb-3, form-label, form-control), and replace the dead alert-error
class with alert-danger across the showcase pages.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 test(showcase): make integration tests tolerate Bootstrap 5 under HtmlUnit
HtmlUnit 5.2.0 cannot parse Bootstrap 5's minified ES6 (bootstrap.bundle.min.js
uses 'class'), which broke the showcase HtmlUnit integration tests once the
decorator started serving the bundle.
- Add ParameterUtils.createWebClient() which disables throwExceptionOnScriptError,
and route all integration tests through it (they assert server-rendered output,
not Bootstrap's client-side behaviour).
- Load bootstrap.bundle.min.js with 'defer' so a page's own inline scripts (e.g.
the async chat demo) still execute before HtmlUnit hits the bundle's parse
error; defer is also the recommended real-browser loading strategy.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 test(showcase): poll for async chat result instead of fixed sleep
AsyncTest relied on a fixed Thread.sleep(4000) for the server-push chat
round-trip, which is not enough on slower/newer JVMs (reproduced failing on
JDK 25). Poll the result element for up to ~30s via waitForBackgroundJavaScript
instead, making the test robust across JVMs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 test(showcase): drive AsyncTest via HTTP instead of HtmlUnit
The browser-driven chat test was flaky on JDK 25 in CI: HtmlUnit's handling of
the async server-push long-poll timed out (message never rendered), even with
polling. The test's purpose is to validate the Servlet 3 async endpoints, which
needs no browser or JavaScript. Rewrite it to POST /async/sendMessage and read
/async/receiveNewMessages directly over HTTP and assert the JSON, making it
deterministic and independent of Bootstrap/HtmlUnit JS parsing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): drop progressbar role from wait progress bar
Resolves a SonarCloud accessibility finding (S6819) introduced by the Bootstrap 5
migration. The BS5 progress component is styled on .progress/.progress-bar divs;
the role/aria attributes were newly added (the BS3 original had none), so removing
them clears the finding while keeping the Bootstrap 5 styling.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): correct BS5 alert wrapper and drop stray table tag
Address Copilot review on PR #1793:
- decorators/main.jsp: the dismissible wrapper was a second .alert with no
variant while the variant class (alert-danger, etc.) stayed on the inner
<ul>, producing a nested, uncoloured alert box. Move the alert* classes
onto the wrapper and strip them from the <ul> so the wrapper is the single
alert container.
- orders-edit.jsp: remove the stray, unclosed <table> start tag before
</s:form> (pre-existing invalid markup carried over during the migration).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): remove BS3 navbar-header wrapper and orphan dropdown-submenu
Clean up two leftover Bootstrap 3 artifacts in the showcase decorator navbar:
- Drop the `navbar-header` wrapper (no BS5 CSS behind it) and make the brand
and toggler direct children of the `.container-fluid` flex container
(justify-content: space-between), with the brand first per BS5 convention.
- Remove the empty, unclosed `<li class="dropdown-submenu">` orphan before the
first item in the Examples menu; BS5 has no dropdown-submenu feature.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): replace soft-deprecated navbar-light with data-bs-theme
navbar-light is soft-deprecated in Bootstrap 5.3. Switch the showcase navbar
to the current data-bs-theme="light" idiom; bg-light is retained for the
background.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(showcase): normalize page-header replacement to border-bottom utilities
A handful of showcase pages replaced the BS3 page-header with a bare <div>
while the rest used <div class="border-bottom pb-2 mb-3">. Normalize those 35
header wrappers to the same border-bottom pb-2 mb-3 utilities so all showcase
page headers render consistently.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5653 fix(rest-showcase): add mb-3 to page-header replacement
Align rest-showcase order page headers with the showcase standard by using
border-bottom pb-2 mb-3 (was border-bottom pb-2), so header spacing is
consistent across both sample apps.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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>
* 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>
* Use ConcurrentHashMap for XSLT template cache and add double-check locking
* Prevent noCache from polluting shared template cache; add dedup and noCache regression tests
* WW-5650 revert StrutsJSONReader to plain single-use instance fields
* WW-5650 revert StrutsJSONWriter to plain single-use instance fields
* WW-5650 obtain a fresh JSONUtil per request in JSONInterceptor
* WW-5650 resolve JSONUtil lazily only on JSON request paths
Move getJSONUtil() into the JSON and JSON-RPC branches of intercept() so
requests with a non-JSON content type no longer construct and discard an
unused JSONUtil/reader/writer graph. Also trim a stray trailing blank line
in StrutsJSONWriter.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5650 test(json): assert JSONWriter bean stays prototype-scoped
Guards the response-side invariant from WW-5644: StrutsJSONWriter now uses
plain instance fields and is not thread-safe, so cross-request safety relies
solely on the writer bean being prototype-scoped. Assert distinct instances
per container lookup so a future switch to singleton scope fails the build.
Addresses review feedback on #1782 without adding a getWriter() accessor
purely for tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* 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.
* 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>
The disclosure rules only forbade publishing exploit/PoC code, so a
contributor who opens a public PR that fixes or hints at a suspected
vulnerability reads them as satisfied — the fix itself telegraphs the
weakness before a fixed release exists.
Add a dedicated "Do not disclose through a pull request, commit, or issue"
section directing reporters to email security@struts.apache.org first, and
extend the PoC rule in Report Quality Rules to state that a fix, patch, or
hardening change is a public disclosure in the same way a PoC is. Aligns
SECURITY.md with the rule already stated in CLAUDE.md/AGENTS.md.
🤖 Generated by AI Assistant
Add two tests to JSONInterceptorTest exercising the nested-object path for
the name/value filtering added in WW-4858:
- testAcceptedNamePatternRejectsNestedKey: accepted name patterns are raw
full-match regexes with no hierarchy expansion, so the intermediate node
("bean") must itself match an accepted pattern or the whole subtree is
dropped before the leaf is visited.
- testIncludePropertiesAppliedToNestedInputWhenEnabled: include patterns do
expand across the hierarchy, so "bean.stringField" also matches the
intermediate "bean" and the nested leaf populates while the excluded
sibling "bean.intField" is dropped.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(json): confine StrutsJSONWriter write state to the writing thread
JSONUtil obtains its JSONWriter once via @Inject and reuses that same
instance across every concurrent response handled by that JSONResult/
JSONInterceptor configuration. StrutsJSONWriter kept its output buffer,
cyclic-reference stack, root object, and expression-path state
(buf/stack/root/buildExpr/exprStack/excludeProperties/
includeProperties/excludeNullProperties) as plain instance fields, all
reset in place at the start of write().
Two concurrent write() calls on the same instance therefore race on
that reset: one call's in-progress buffer can be wiped and overwritten
by a second, unrelated concurrent call before the first call reads it
back via buf.toString(), so one request's serialized JSON can be
returned as a completely different, concurrently-served request's
response body.
Move buf/stack/root/buildExpr/exprStack/excludeProperties/
includeProperties/excludeNullProperties into a WriteState confined to
a ThreadLocal, scoped to a single write() call. Method signatures and
behavior are otherwise unchanged so existing StrutsJSONWriter
subclasses keep working; ignoreHierarchy/dateFormat/enumAsBean/
excludeProxyProperties stay as plain instance fields since they are
set to the same value on every call for a given writer configuration
and are safe to share.
* test(json): raise writer concurrency test to 16 threads for reliable repro
Verified independently that the 2-thread version can miss the race on
machines with more cores than contending threads (with no CPU
contention, the OS scheduler has no need to preempt either thread
mid-call, so the corruption window is rarely hit): 0 reproductions in
8 reruns against unpatched code on a 10-core machine. Sixteen threads
reproduced it reliably (44,646/320,000 corrupted responses against
unpatched StrutsJSONWriter), and confirmed zero corruption against the
fix under the same load.
---------
Co-authored-by: g0w6y <g0w6y@users.noreply.github.com>
* fix(json): confine StrutsJSONReader parse state to the parsing thread
JSONInterceptor obtains its JSONReader once via @Inject and reuses that
same instance across every concurrent request handled by that
interceptor. StrutsJSONReader kept its parse cursor, token buffer and
nesting-depth counter (used to enforce maxDepth/maxElements/
maxStringLength/maxKeyLength) as plain instance fields, so two
concurrent read() calls on the same instance tore each other's state:
one request's depth counter could be decremented by an unrelated
concurrent request finishing its own parse, letting payloads deeper
than the configured maxDepth through, and the shared character cursor
and string/number buffer let fragments of one request's JSON body leak
into a different, concurrently-parsed request's result.
Move the cursor, current character, token, buffer and depth into a
ParseState confined to a ThreadLocal, scoped to a single read() call.
Method signatures and behavior are otherwise unchanged so existing
StrutsJSONReader subclasses keep working; the limit fields
(maxElements/maxDepth/maxStringLength/maxKeyLength) stay as plain
instance fields since they are set to the same value on every call for
a given interceptor configuration and are safe to share.
* test(json): raise reader concurrency test to 16 threads for reliable repro
Verified independently that the 2-thread version can miss the race on
machines with more cores than contending threads (with no CPU
contention, the OS scheduler has no need to preempt either thread
mid-call, so the corruption window is rarely hit): 0 reproductions in
8 reruns against unpatched code on a 10-core machine. Sixteen threads
reproduced both symptoms reliably against unpatched StrutsJSONReader
(81 cross-thread data leaks and 79 maxDepth bypasses out of 160,000
attempts), and confirmed zero of either against the fix under the
same load. Combined the two prior tests into one, since both symptoms
come from the same shared parse state and are naturally checked
together per thread.
---------
Co-authored-by: g0w6y <g0w6y@users.noreply.github.com>
* WW-4858 docs(json): design for honoring parameter filtering during JSON population
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 docs(json): implementation plan for JSON parameter filtering
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 feat(json): enforce excluded/accepted name patterns on JSON population
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 feat(json): enforce param-name max length on JSON population
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 feat(json): honor ParameterNameAware and ParameterValueAware on JSON population
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 feat(json): add opt-in excluded/accepted value patterns on JSON population
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 feat(json): opt-in applying excludeProperties/includeProperties to JSON input
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-4858 test(json): cover nested and list-element paths; clarify filter comments
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5641 docs: design spec for JSON writer/reader override regression
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5641 docs: implementation plan for JSON writer/reader override fix
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* WW-5641 fix: run JSON bean-selection from struts-deferred.xml
The JSON plugin declared <bean-selection> in struts-plugin.xml, which runs
at plugin-parse time, before the application struts.xml is folded in. That
froze the JSONWriter/JSONReader default binding to StrutsJSONWriter/Reader,
so struts.json.writer / struts.json.reader overrides were ignored.
Move the element to struts-deferred.xml, which Dispatcher loads last (after
the app config and core's StrutsBeanSelectionProvider), so the alias honors
the override. Mirrors the velocity plugin. JSONUtil is unchanged from main.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* 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>
* Add draft security threat model (THREAT_MODEL.md)
Generated-by: Claude Opus 4.8 (1M context)
* SECURITY.md: link to THREAT_MODEL.md for scanner/triager discoverability
Generated-by: Claude Opus 4.8 (1M context)
* Fix dangling §14 refs and tighten provenance in threat model
Address code-review findings on the THREAT_MODEL.md draft:
- Add the missing §14 Q-env and Q-egress open questions, so every
*(inferred)* claim that cites them now resolves (restores the
"each inferred claim has a matching §14 question" invariant).
- Tag the two previously bare *(inferred)* claims (examples/showcase,
on-path attacker) with their matching question IDs.
- Soften the §1 header: drop the ASF Security team / PMC authorship
attribution on an unratified draft; state it is drafted for PMC review.
- Correct the OGNL Java Security Manager wording: SecurityManager is
deprecated for removal since JDK 17 and permanently disabled in JDK 24,
not simply "does not work on JDK 21+".
- AGENTS.md: point the Assess step at THREAT_MODEL.md's disposition guide
so the triage wrapper links the model directly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Lukasz Lenart <lukaszlenart@apache.org>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* 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>
* 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.
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>
* 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>
* 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
* 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>
Add an agent skill for triaging privately-disclosed security reports:
research each claim from source without trusting the reporter, verify
effective runtime defaults (config overrides field initializers), avoid
introducing unverified facts into responses, and frame findings as
vulnerability vs. operator responsibility.
Developed test-first: a baseline run produced contradictory, unverified
claims about defaults; the skill closes that gap and was verified to also
avoid over-correcting into reflexive rejection of valid reports.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The `cooldown` block expects `default-days`, not `default`. Using the
wrong key fails schema validation, causing Dependabot to silently fall
back to the previously valid config — which still targets
`release/struts-6-8-x` instead of `support/struts-6-x-x`.
Also adds a 3-day cooldown to the `main` maven entry for consistency.
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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)
The WW-5535 fix (commit 4d2eb93) corrected isMethodSpecified() for wildcard-resolved
methods but introduced a structural gap in HttpMethodInterceptor.intercept().
The if/else-if structure made the class-level annotation check unreachable whenever
isMethodSpecified()=true and the resolved method carries no method-level annotation:
if (isMethodSpecified()) {
if (isAnnotatedBy(method)) { ... }
// falls through silently
} else if (isAnnotatedBy(class)) { ... } // never reached
return invocation.invoke(); // no enforcement
Fix: convert else-if to standalone if so the class-level check is always evaluated
as a fallback when the method itself has no annotation. Method-level annotations
still take precedence (checked first).
Add two regression tests covering the wildcard-resolved unannotated method scenario.
Co-authored-by: g0w6y <g0w6y@users.noreply.github.com>
* 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>
* chore(agents): defines a new AGENTS.md focused on reporting vulnerabilities
* chore(security): makes SECURITY.md the source of truth
* Fix typos in SECURITY.md and minor grammar correction
* chore(security): clarify public GitHub repo PoC counts as disclosure
Per @ppkarwasz review on #1680: expand the PoC bullet to make explicit
that pushing a PoC to a public GitHub repo, gist, fork, or branch is
public disclosure, and note that private repos require granting access
to each PMC member individually.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: René Gielen <rgielen@apache.org>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
* 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>
* 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.
* 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>
* 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>
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>
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>
* 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>
Add configurable limits to the JSON plugin to prevent denial-of-service
attacks via malicious payloads (deeply nested objects, huge arrays, long
strings).
Changes:
- Extract JSONReader interface from class, create StrutsJSONReader impl
with maxElements, maxDepth, maxStringLength, maxKeyLength enforcement
- Rename DefaultJSONWriter to StrutsJSONWriter (Struts* naming convention)
- Add JSONBeanSelectionProvider for bean aliasing via constants
- Update JSONUtil with @Inject for reader/writer, add instance
deserializeInput() with maxLength check, deprecate static deserialize()
- Wire limits into JSONInterceptor with @Inject from constants
- Register beans and defaults in struts-plugin.xml
Default limits: 10K elements, 64 depth, 2MB length, 256KB strings, 512 keys.
All configurable via struts.xml constants or per-action interceptor params.
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
- Add production deployment warnings to showcase and rest-showcase READMEs
- Convert README.txt to README.md with proper Markdown formatting
- Restrict ViewSourceAction config parameter to XML files within webapp path
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Replace e.printStackTrace() with System.err.println() to properly
log errors to stderr without stack trace noise in CLI tools.
Issue: JRException handling in JasperReports compilation
Co-authored-by: Senrian <sen@senrian.com>
* 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>
- Add serialization support for LocalDate, LocalDateTime, LocalTime,
ZonedDateTime, OffsetDateTime, and Instant in DefaultJSONWriter
- Add deserialization support for the same types in JSONPopulator
- Support @JSON(format=...) custom formats for all temporal types
- Fix Instant custom-format serialization requiring UTC zone
- Add Calendar serialization/deserialization via temporal bridge
- Add comprehensive tests for all temporal types including custom
formats, malformed input, and null handling
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Moved to user-wide ~/.claude/agents/ to make it available across all
projects. The agent is now project-agnostic and auto-detects build tools.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* 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>
Apply recommendations from CLAUDE.md benchmarking study (1,188 tests
across 3 models): remove redundant generic instructions, reframe
prohibitions as positive directives, trim inferable content, and
keep only project-specific knowledge that Claude cannot derive from
the codebase itself. Reduces file from 142 to 64 lines.
Key changes:
- Remove Common Pitfalls (negative framing, generic, duplicated)
- Remove Available Tools section (redundant with system prompt)
- Trim build commands to project-specific flags only
- Collapse Technology Stack into one-line overview
- Reframe security directives from "never do X" to "do Y instead"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude <noreply@anthropic.com>
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>
* 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>
The duplicate @Action name detection in PackageBasedActionConfigBuilder
was embedded inside a conditional block that only ran when execute() was
NOT annotated with @Action. This meant two methods could map to the same
action name silently when execute() had an @Action annotation, with one
overwriting the other non-deterministically.
Extract the duplicate check to run unconditionally before the conditional
block, so it applies to all annotated methods regardless of whether
execute() is annotated.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude <noreply@anthropic.com>
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>
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>
description: Execute and analyze Maven tests for Apache Struts. Use PROACTIVELY when code changes, user mentions testing, asks to run tests, check test coverage, or validate changes. MUST BE USED for all Maven test execution (mvn test), test result analysis, and coverage reports.
tools: bash_tool, view, str_replace
model: sonnet
color: green
---
# Apache Struts Test Runner Agent
## Identity
You are a specialized test execution and analysis expert for Apache Struts projects. You understand the framework's testing patterns, Maven module structure, and can intelligently execute tests, analyze results, and provide actionable feedback on test coverage and quality.
## Core Capabilities
### 1. Intelligent Test Execution
- **Module-aware testing**: Execute tests in specific Maven modules (`core/`, `plugins/`, `apps/`, `jakarta/`)
- **Pattern-based test selection**: Run tests matching specific patterns or components
- **Incremental testing**: Execute only tests affected by recent changes
- **Performance testing**: Measure test execution times and identify slow tests
- **Parallel execution**: Optimize test runs using Maven parallel execution
### 2. Test Analysis and Reporting
- **Coverage analysis**: Analyze test coverage across different components
- **Failure analysis**: Categorize test failures and provide remediation guidance
- **Security test validation**: Ensure security-related tests are comprehensive
- **Integration test coordination**: Manage complex integration test scenarios
- **Regression detection**: Identify potential regressions in test results
### 3. Struts-Specific Testing Expertise
- **Action testing patterns**: Validate ActionSupport and POJO action tests
- **Security tests**: Vulnerability and attack prevention tests
- **Performance tests**: Load and stress testing
- **Configuration tests**: XML and annotation validation
### 3. CI/CD Integration
- Pre-commit hook validation
- Pull request test automation
- Release candidate testing
- Performance regression detection
## Best Practices
### 1. Test Execution Efficiency
- Always use `-DskipAssembly` to avoid building documentation/examples
- Use pattern matching to run relevant tests only
- Leverage parallel execution for large test suites
- Cache dependencies to reduce setup time
### 2. Test Quality Assurance
- Ensure security tests cover all attack vectors
- Validate test coverage meets minimum thresholds
- Monitor test execution trends for performance regressions
- Maintain test isolation and repeatability
### 3. Failure Handling
- Categorize failures by severity and impact
- Provide clear remediation guidance
- Track failure patterns across builds
- Implement automatic retry for flaky tests
Remember: Testing is crucial for Struts applications due to the framework's security sensitivity. Always prioritize security tests and ensure comprehensive coverage of OGNL evaluation paths and parameter handling logic.
description: Use when opening the formal release vote for a Struts release candidate on any maintenance line (6.x, 7.x) - composing and drafting the [VOTE] Apache Struts X.Y.Z mail to dev@ once the Version Notes page, GitHub release and staged artifacts are published.
---
# Creating a Release Vote Mail
## Overview
The `[VOTE]` mail opens the formal release vote. It is four links wrapped in frozen ASF
boilerplate, around a plain-text rendering of the release's Version Notes page.
**Core principle:** the mail is a *rendering* of the page, not a second account of the release.
**This is the step after `creating-version-notes`.** That skill produces the page, the GitHub
release and the `[TEST]` announcement; this one consumes all three. If they do not exist yet,
you are in the wrong skill.
[`vote-mail-template.md`](vote-mail-template.md) is the source of truth for the artifact.
## The mail is exactly these parts, in this order
1. The two-sentence opener
2. The page's `Breaking changes`, `Deprecations` and `Rejected requests`, where present
3. The page's issue-type sections, in page order
4. The four link lines
5. The vote boilerplate
6. The sign-off
**A part not on this list is not in the mail, and one mail is produced, not two.** Every
section is the page's content; the opener is the only prose you write.
The pull here is toward helpfulness — an upgrade-notes section derived from the fix commits, a
summary of what changed for integrators, a companion note to a subset of recipients. All of it
is real work that belongs somewhere else. A vote is a judgement on the staged artifacts, and
the page is what describes them.
## The vote carries no security information
No severity, no CVE, no S2-XXX, no bulletin link, no attack description, no reporter or
coordination detail. **That disclosure happens after the vote passes and the version is
released.**
**The restriction is on the vote, not on the audience.** Routing advisory detail through
`private@`, a Cc, an attachment or a companion mail is the same violation as putting it in the
body — a second mail sent to open the vote is part of the vote. "The recipients already hold
this information" is not an exemption; the vote is simply not the vehicle.
Neutral ticket summaries carried from the page are not security information. Keep them exactly
as the page has them, including where the page truncates one at a clause boundary.
**REQUIRED BACKGROUND:**`creating-security-bulletins` governs what may be said, and when.
## Recipients
```
To: dev@struts.apache.org
Bcc: private@struts.apache.org
```
**`user@struts.apache.org` must not appear in any header — not To, not Cc, not Bcc.** The
`[TEST]` mail one step earlier goes to `dev@` and `user@`, which is right for it: it asks
people to test. This mail asks people to *vote*, and a vote invitation on the user list
solicits votes that are not binding and scatters the tally across two lists.
Cc is not a compromise. If a release manager asks you to include the user list, the answer is
that the `[TEST]` mail already did.
**`private@` is on the mail for reach, not for confidentiality.** Not every PMC member follows
`dev@`, and PMC votes are the binding ones, so `private@` is what guarantees the binding voters
see the call. Nothing goes there that could not go to `dev@` — its presence is a delivery
decision, and it is not an exemption from the rule above.
It goes on **Bcc, not Cc**: on the 7.1.1 and 6.8.0 votes it was on Cc, and reply-all `+1`s
landed on the private PMC list. Bcc gives the same reach while keeping the tally in one thread
on `dev@`.
Subject is exactly `[VOTE] Apache Struts X.Y.Z`.
## Draft it, do not send it
Create a Gmail draft with To, Bcc, Subject and body set, and write the same body to a file whose
path you hand over. **Never send.**
| Rationalization | Reality |
|---|---|
| "The release manager authorised whatever I produce" | Authorisation to compose is not authorisation to transmit. |
| "Every fact is verified; review would catch nothing" | Sending is not a quality gate, it is a commitment. Verification does not confer it. |
| "A draft doesn't open the vote, which defeats the request" | Correct, and that is the right outcome when the release manager is not there to send it. |
| "The 72-hour clock is the reason for the hurry" | A vote opened on the wrong artifacts costs far more than the hours saved. |
Sending opens a binding vote on a permanently archived public list and commits the PMC to the
artifacts as staged.
### Gmail mangles the links — hand over a paste-ready body
Gmail's linkifier rewrites URLs server-side as the draft is stored, so the four link lines
arrive as `https://www.google.com/url?q=...&source=gmail&ust=...` and the 72-column wrap is
reflowed. There is no way to pass the body through the Gmail tool that avoids it:
The canonical skeleton for the `[VOTE] Apache Struts X.Y.Z` mail that opens a release vote.
Companion to [`SKILL.md`](SKILL.md), which covers *how* to fill the slots; this file covers
*what the mail contains*.
**This file is the source of truth.** Start every vote mail from the skeleton below.
## Slots
| Slot | What goes in it |
|---|---|
| `<X.Y.Z>` | The release being voted on, dotted — subject, opening sentence, Version Notes URL, dist path |
| `<X_Y_Z>` | The same version underscored, for the `STRUTS_` git tag only |
| `<SHAPE SENTENCE>` | See below — authored per release |
| Page sections | Breaking changes, Deprecations, Rejected requests, and the issue-type sections, copied from the Version Notes page. Omit any the page omits. |
**Where the page introduces a section with a sentence of its own, that sentence comes with it.**
The skeleton below shows sections as bare lists, but `Rejected requests` on the 7.3.0 page opens
with *"Two long-standing requests were closed as Won't Do in this cycle. They are listed here so
the decision is visible rather than silent."* Without it the section reads as two unexplained
ticket ids.
## The shape sentence is authored per release
The opener is two sentences. The first is fixed. The second describes the *shape* of the issue
list — how the release is composed — and is written from the list in front of you.
| The page has | Second sentence |
|---|---|
| No Breaking changes | `With this release the following issues were addressed:` |
description: Use when drafting, updating, or reviewing an S2-XXX security bulletin on the Struts cwiki, when preparing bulletin text ahead of a CVE request, or when deciding how much detail about a fixed vulnerability is safe to publish.
---
# Creating Security Bulletins
## Overview
An S2-XXX bulletin exists to tell an operator **what to upgrade and why** — not to explain the defect. Every sentence that helps a defender must be weighed against how much it helps someone building an exploit.
**Core principle:** every field is either traced to source you read this session, or a visible placeholder. Never a plausible guess.
**Process authority:** [`SECURITY.md`](../../../SECURITY.md) governs disclosure. This skill governs *what the page says and how it is written*.
**REQUIRED BACKGROUND:** the claims you put in a bulletin come from triage. Use `triaging-security-reports` to establish them before writing.
## The Iron Rule
```
NO FIELD IN A BULLETIN WITHOUT A SOURCE YOU READ THIS SESSION,
OR A VISIBLE PLACEHOLDER.
```
Applies to the severity rating, the affected versions, and above all the Workaround. "There is no workaround" is a factual claim about absence — the hardest kind to get right, and the most common thing to assert without checking.
## Page structure
Sections in order, matching the existing published bulletins:
`Summary` (in an `excerpt` macro) → field table → `Problem` → `Solution` → `Backward compatibility` → `Workaround`
Field table rows, in order:
| Row | Content |
|---|---|
| Who should read this | Usually `All Struts 2 developers and users`; narrow it only when exposure is genuinely conditional |
| Impact of vulnerability | A short impact phrase, not a paragraph — *Remote Code Execution*, *Denial of service*, *Disclosure of Data, Denial of Service, Server Side Request Forgery*. Hedging is accepted where warranted (*Possible Remote Code Execution vulnerability*) |
| Maximum security rating | Low / Moderate / Important / Critical — see the rating scale below |
| Recommendation | `Upgrade to Struts X.Y.Z at least`. Name **every** maintenance line that carries the fix (`Upgrade to Struts 6.8.0 or 7.1.1 at least`), and add the required action where upgrading alone is not enough (`… and use Action File Upload Interceptor`) |
| Affected Software | Officially released versions only (see below); bullet one range per maintenance line, linking the EOL announcement for end-of-life ranges |
| Reporters | Credit the reporter — they earned it, and it costs nothing. Include their organisation where they gave one (`Steven Seeley of Source Incite`); obfuscate any email (`pwntester at github dot com`) |
| CVE Identifier | Placeholder until assigned (see below) |
**Match the house voice — from the *recent* bulletins only.** Read the two or three most recently published ones before writing. They are far terser than a triage write-up: `Problem` is one to three sentences, and every affected feature is **linked to its page on struts.apache.org** so an operator can go straight to the documentation. Where a bulletin resembles an earlier one, the Summary says so and links it.
**Do not take the older bulletins as a precedent for how much to disclose.** Earlier advisories explained causes and mitigations in far more detail, and that detail was used to build working exploits. The project deliberately stopped. An old bulletin naming the exact construct that triggers the flaw is evidence of the practice this skill exists to prevent, not licence to repeat it — mine them for structure and tone, never for depth.
## Affected Software: released versions only
**List only versions that passed a PMC release vote.** A build that was cut, failed its test period, and was superseded never reached users as a release — listing it implies an official artifact was vulnerable and drags a phantom version into every downstream CVE record and scanner database.
Verify before writing. Do not infer the range from the tags in git: a tag exists for builds that were never voted through. Ask, or check the release announcements.
**Deriving the lower bound** — one method, both bounds:
1. Find when the vulnerable code entered, with `git log -S'<the vulnerable construct>' -- <path>`. Do not assume it arrived with the feature that made it reachable; a defect often predates the control that was supposed to bound it.
2. Map that commit to the first *release* containing it.
3. If step 2 can't be settled from what you have, write a visible placeholder naming what must be confirmed — never a guessed version number.
## The rating scale is published — apply it, don't invent one
The definitions live on **[Security Bulletins](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61758)** (page `61758`), and they answer one question: *how worried should I be about this vulnerability?*
**That page is the only authority.** The four-level naming was introduced comparatively recently, so bulletins published before it use other wording and inconsistent capitalisation. Never infer the vocabulary or calibrate a rating from an older bulletin — match a definition on page `61758`, and take comparisons only from advisories published since the scale existed.
| Rating | Applies when |
|---|---|
| **Critical** | A remote attacker can get Struts to execute arbitrary code — exploitable automatically, regardless of whether the developer followed the Security Guide |
| **Important** | Compromise of the application's **data or availability**; also easy RCE that depends on the developer having mistreated user input |
| **Moderate** | There is **significant mitigation**: the flaw does not affect likely configurations, or the configuration is not widely used, or the attacker must be authenticated |
| **Low** | Everything else — believed **extremely hard to exploit**, or the exploit yields minimal consequences |
Two traps in applying it:
- **Low is not "narrow".** A flaw that is trivial to trigger and causes real damage is not Low merely because a setting gates it. Reserve Low for hard-to-exploit *or* minimal-consequence.
- **The Moderate clause is "not widely used", not "opt-in".** A gate only mitigates if few deployments pass through it. S2-068 needed file upload enabled and was still rated **Important**, because file upload is ordinary. Ask how many real deployments the precondition actually excludes.
- **Availability counts as Important.** Denial of service is not automatically a lesser class — S2-068 was disk exhaustion, rated Important. It drops to Moderate only where a mitigation clause genuinely applies.
**Exploitation status belongs on the page, not in the rating.** The scale measures the flaw itself, so it has no slot for "a public reproduction already exists." When a defect was disclosed publicly before the fix shipped, or a working reproduction is already public, say so in plain words — downstream consumers are told by their own regulators to prioritise on real risk and active exploitation, not on a severity class alone. It costs nothing: the reproduction is already out.
## CVE placeholder
CVEs are requested **after** the fixed release is out and accepted. Until then the row carries a placeholder that cannot be mistaken for a real identifier:
```
CVE-YYYY-NNNNN (to be assigned before publication)
```
Never leave a cloned page's real CVE in place. Never invent a well-formed-looking number.
One CVE per independently fixable issue — separate fixes get separate bulletins and separate CVEs, per [CNA rules 4.1.10](https://www.cve.org/ResourcesSupport/AllResources/CNARules).
## The disclosure budget
**The budget covers every prose section — `Problem`, `Backward compatibility`, and `Workaround` alike.**`Problem` is the section authors guard; `Backward compatibility` is the one that leaks, because describing what changed about the fixed behaviour describes the defect. A note saying which inputs are handled differently now points straight at the code path that was rewritten. Apply the table below to all three sections, and write the BC note in terms of what an application might *observe*, never what the fix altered internally.
Write the shortest true description that lets an operator judge whether they are exposed. One to three sentences, as in the published bulletins.
| Safe to publish | Never publish before the fix is out |
|---|---|
| Impact categories and consequence | Class, method, or field names |
| The component in plain words, linked to its documentation | `file:line` references |
| Whether a configured control fails to apply | Commit hashes, PR or Jira numbers |
| That state is shared / input is unvalidated | The triggering request shape or payload |
| Which released versions are affected | Reproduction steps, PoC, timing conditions |
**Write for an operator, not a reviewer.** S2-068 describes an exploited disk-exhaustion bug in one sentence — *"If support for file upload is enabled, file leak in multipart request processing causes disk exhaustion."* That is the register: the feature, the failure, the consequence. Naming the class turns a bulletin into a starting point.
## State who is *not* affected
An operator's first question is "does this reach me?" Answer it on the page, or every reader has to assume it does.
The house form is **one sentence, linked to the feature's documentation** — S2-067 does it in a single line:
> **Note**: applications not using [FileUploadInterceptor](https://struts.apache.org/core-developers/file-upload-interceptor) are safe.
or folded into the opening clause, as S2-068 does with *"If support for file upload is enabled, …"*. Say it whenever exposure is conditional — an optional plugin the application chooses to ship, a setting that must be switched on, an endpoint that must be mapped, or an unaffected sibling path that lets a reader stop reading. Add "earlier releases are not affected" when there is a clean prior baseline.
Keep it at the level of a deployment decision ("uses the plugin", "exposes such an endpoint"), not a code path. Scoping *reduces* net disclosure: it shrinks the population that has to care, and it costs an attacker nothing they could not learn from the dependency list.
## Fix provenance
A bulletin promises a fixed release and describes post-fix behaviour as settled fact. Both claims rest on a specific change.
**Record which commit or PR each behavioural claim rests on**, in the version comment or your notes — not on the page.
**Confirm that change is merged into the release branch before publishing.** A patch under private review may be revised or dropped; a bulletin describing behaviour that never shipped is worse than a late bulletin. Bulletins are routinely drafted while the fix is still embargoed and unmerged — that is normal, and it is exactly why the merge state must be re-checked at publication time rather than at drafting time.
**Derive BC notes from the fix diff, not from its commit message.** A commit summary that calls the behaviour unchanged can still carry an observable difference its author did not think worth mentioning. Read the diff.
**`Backward compatibility` is also where a breaking upgrade is announced**, and the announcement has to be blunt. S2-067 told users the fix was *not* backward compatible, that they had to rewrite their actions onto a new mechanism, and that staying on the old one left them vulnerable. Where the fix is transparent, the house sentence is simply *"This change is backward compatible."*
## Workaround: verify or say nothing
Three valid outcomes, in order of preference:
1.**A verified configuration or operational change.** Trace it in source and confirm it actually removes reachability. Give the change, not the mechanism. It need not be a Struts setting — S2-068 offers a sized or dedicated temp volume, and pointing at the relevant section of the Security Guide is a legitimate workaround in itself.
2.**Upgrade only** — when you checked and found nothing.
3.**Verified absence.** The house value is a bare `n/a` (S2-066, S2-067); spell it out when the reason is worth stating.
Never ship a workaround you reasoned about but did not confirm. A wrong workaround leaves operators believing they are protected and discredits every other field on the page.
**The tension to decide deliberately:** a workaround usually reveals which path is affected. That is often the right trade — it is why the bulletin exists — but it is a decision to make and surface, not one to make silently. Say which way you went and why.
## Re-read the page immediately before you write to it
Bulletins are drafted by more than one person, often within the same hour. Content you read earlier may have moved on — a backport range added, a placeholder resolved, a section rewritten.
**Fetch the current version immediately before every write, and compare the returned version number against the one you read.** If it advanced, re-read, merge your change onto the newer content, and write that. Writing from a stale copy silently discards someone else's work with no warning and no conflict error.
After writing, diff your new version against the one you meant to build on. The diff should show only your intended change. If it shows deletions you did not intend, restore from history and redo the edit on top.
## Restrictions
Bulletins stay restricted until the coordinated publication date.
**Check restrictions before the edit and again after.** An API update should not disturb them, but "should not" is not verification, and an accidentally public pre-release bulletin is an unrecoverable disclosure.
Expected on the Struts wiki: read and update limited to the author plus `struts-committers`.
## Start from the template, never from a previous bulletin
**[`bulletin-template.md`](bulletin-template.md)** — the field reference, per-section guidance, pre-publication checklist, and a storage-format skeleton ready to POST to the Confluence API. **It is the source of truth.**
A rendered copy exists on the wiki as a restricted child of *Security Bulletins* for authors who prefer to copy a page; when the two disagree, fix the wiki page from the file. Whichever route you take, confirm the new page carries the same restrictions before typing anything into it, and give the `excerpt` macro a fresh `ac:macro-id` — a copied page inherits the template's, and two bulletins must not share one.
**If you inherit a page cloned from a previous bulletin instead**, assume every field is inherited and wrong until you have replaced it. The residue that survives a careless edit:
- The previous bulletin's real CVE identifier
- Its affected versions, rating, and reporter credit
- Its workaround — describing a mitigation for an unrelated defect
- The `excerpt` macro's `ac:macro-id`, now **duplicated across two pages** — generate a fresh UUID
Read the whole page and rewrite it; do not patch the fields you happen to notice.
## Red Flags — STOP
- About to write a Workaround you have not traced in source
- About to write "no workaround exists" without having looked
- Affected Software copied from a git tag list rather than confirmed releases
- A CVE number on the page that you did not receive from the CVE assignment process
- Naming a class, method, or file in `Problem` "because it's already public in the PR"
- Copying the disclosure depth of an older bulletin — that depth is the reason this budget exists
- Calibrating a rating against a bulletin published before the four-level scale existed
- Guarding `Problem` carefully and then describing the fix's internals in `Backward compatibility`
- Writing a BC note from the fix's commit message without reading the diff
- Publishing while the fix is still unmerged, or without re-checking that it landed
- No statement of who is *not* affected, when exposure depends on a plugin or an opt-in setting
- Writing a page from content you read earlier in the session without re-fetching it first
- Publishing without re-checking restrictions
- A severity rating chosen by feel, or by reachability alone, without checking it against the published scale
- Rating something Low because the feature is opt-in — opt-in is the definition of Moderate
## Common Mistakes
| Mistake | Reality |
|---|---|
| "The PR is public, so detail costs nothing" | A bulletin is indexed, permanent, and read by people who never see the PR. Aggregation is the harm. |
| "An older bulletin explained the cause in detail" | Those explanations were used to build exploits. The practice was stopped deliberately — don't restore it. |
| "An older bulletin rated something like this X" | The rating scale postdates it. Match a definition on page 61758 instead. |
| "Listing the failed build is more honest" | It is less accurate. That build was never a release; listing it misdirects every downstream consumer. |
| "Disabling the feature is an obvious workaround" | Obvious ≠ verified. Confirm the feature is genuinely on the only reachable path. |
| "The rating is roughly right" | Ratings drive upgrade urgency. Read the published definitions and match one, don't approximate. |
| "It needs an opt-in feature, so it's Low" | That is the Moderate mitigation clause. Low means hard to exploit or minimal consequence. |
| "Restrictions were set when the page was created" | Verify after every edit. The cost of being wrong once is total. |
| "I'll fill in the CVE later" | Only if the placeholder is unmistakable. A blank or a stale number ships as fact. |
| "The BC note is just a compatibility courtesy" | It describes what the fix changed, which describes the defect. Same budget as `Problem`. |
| "The commit message says behaviour is unchanged" | Commit summaries understate. Read the diff and decide for yourself. |
| "Naming the plugin narrows it for an attacker too" | They can read your dependency list. Scoping spares every operator who isn't exposed. |
| "The patch is reviewed, so the release will contain it" | Reviewed is not merged. Re-check at publication, not at drafting. |
| "Copying the last bulletin is quicker than the template" | It is how another advisory's CVE ships on your page. Copy the template. |
| "I read the page a few minutes ago" | Someone else may have written to it since. Re-fetch, then write. There is no conflict warning. |
for authors who prefer to copy a page. **This file is the source of truth** — when the
two disagree, fix the wiki page from here.
**Draft bulletins stay restricted** (read and update limited to the author plus
`struts-committers`) until the coordinated publication date. Check restrictions before
an edit and again after it: an accidentally public pre-release bulletin is an
unrecoverable disclosure.
## Fields
| Row | What goes in it |
|---|---|
| Who should read this | Usually `All Struts 2 developers and users`. Narrow it only when exposure is genuinely conditional. |
| Impact of vulnerability | A short impact phrase, not a paragraph — `Remote Code Execution`, `Denial of service`. Hedge where warranted (`Possible Remote Code Execution vulnerability`). |
| Maximum security rating | `Low` / `Moderate` / `Important` / `Critical`, matching a definition on the [Security Bulletins](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61758) page. That page is the only authority — the four-level naming postdates many older bulletins, so never calibrate against one. |
| Recommendation | `Upgrade to Struts X.Y.Z at least`. Name **every** maintenance line carrying the fix, and add the required action where upgrading alone is not enough. |
| Affected Software | Officially released versions only. One bullet per maintenance line; link the EOL announcement for end-of-life ranges. |
| Reporters | Credit the reporter. Include their organisation where they gave one; obfuscate any email address. |
| CVE Identifier | `CVE-YYYY-NNNNN (to be assigned before publication)` until the real identifier arrives. One CVE per independently fixable issue. |
### Affected Software
List only versions that passed a PMC release vote. A build that was cut, failed its
test period and was superseded never reached users — listing it implies an official
artifact was vulnerable and drags a phantom version into every downstream CVE record
and scanner database. Do not read the range off git tags; tags exist for builds that
were never voted through.
To find the lower bound: locate when the vulnerable construct entered with
`git log -S`, then map that commit to the first release containing it. A defect often
predates the control that was supposed to bound it, so do not assume it arrived with
the feature that made it reachable. If the mapping cannot be settled, write a visible
placeholder naming what must be confirmed — never a guessed version number.
## Problem
One to three sentences. Write for an operator, not a reviewer: the feature, the
failure, the consequence.
| Safe to publish | Never publish before the fix is out |
|---|---|
| Impact categories and consequence | Class, method, or field names |
| The component in plain words, linked to its documentation | `file:line` references |
| Whether a configured control fails to apply | Commit hashes, PR or Jira numbers |
| That state is shared / input is unvalidated | The triggering request shape or payload |
| Which released versions are affected | Reproduction steps, PoC, timing conditions |
Older bulletins explained causes and mitigations in far more detail, and that detail
was used to build working exploits. The project deliberately stopped. **Mine the
archive for structure and tone, never for depth.**
**Then say who is not affected.** An operator's first question is "does this reach
me?" — answer it, or every reader must assume it does. One sentence, linked to the
feature's documentation, either as a trailing note or folded into the opening clause.
Name the optional plugin, the setting that must be switched on, the endpoint that must
be mapped, or the unaffected sibling path. Add "earlier releases are not affected"
where there is a clean prior baseline.
Keep it at the level of a deployment decision, not a code path. Scoping *reduces* net
disclosure: it shrinks the population that has to care, and costs an attacker nothing
they could not read off a dependency list.
## Solution
`Upgrade to Struts X.Y.Z at least.` Repeat for each maintenance line, and link the
migration guide where the fix requires one.
## Backward compatibility
**Subject to the same disclosure budget as Problem.** This is the section that leaks:
describing what changed about the fixed behaviour describes the defect. Write it in
terms of what an application might *observe*, never what the fix altered internally,
and derive it from the fix diff rather than the commit message — a summary calling the
behaviour unchanged can still carry an observable difference.
It is also where a **breaking** upgrade is announced, and that announcement has to be
blunt: what must be rewritten, and what staying put costs. Where the fix is
transparent, the house sentence is simply `This change is backward compatible.`
## Workaround
Three valid outcomes, in order of preference:
1.**A verified configuration or operational change** — traced in source and confirmed
to remove reachability. Give the change, not the mechanism. It need not be a Struts
setting; container and reverse-proxy limits count, as does pointing at the relevant
section of the Security Guide.
2.**Upgrade only**, when you checked and found nothing.
3.**Verified absence.** The house value is a bare `n/a`; spell it out when the reason
is worth stating.
Never ship a workaround you reasoned about but did not confirm — it leaves operators
believing they are protected and discredits every other field on the page. "No
workaround exists" is a claim about absence and needs checking too.
A workaround usually reveals which path is affected. That is often the right trade,
but make it deliberately and record which way you went.
## Before publishing
- [ ] Every placeholder is replaced, and no guidance text survives on the page.
- [ ] The CVE identifier is real, not the placeholder.
- [ ] Affected Software lists voted releases only, and covers every maintenance line.
- [ ] The rating matches a published definition rather than an approximation.
- [ ] The workaround was verified in source, or its absence was.
- [ ] Problem, Backward compatibility and Workaround name no class, file, commit, PR
or payload.
- [ ] The fix is **merged** into the release branch — reviewed is not merged; re-check
now, not at drafting time.
- [ ] The fixed release is out and accepted.
- [ ] Restrictions are lifted only at the coordinated publication moment.
## Storage-format skeleton
Ready to POST to the Confluence API. Give the `excerpt` macro a **fresh**
`ac:macro-id` each time — two bulletins must not share one.
description: Use when preparing, updating, or reviewing the release documentation for a Struts release or release candidate on any maintenance line (6.x, 7.x) - the Version Notes page on the cwiki, its Migration Guide entry, the GitHub release notes, and the test-build announcement mail.
---
# Creating Version Notes
## Overview
A Version Notes page answers one question for a user deciding whether to upgrade: **what changed in this release, and what will break.** Almost all of it is a mechanical rendering of a JIRA fix version onto fixed boilerplate.
**Core principle:** the mechanical parts must be *derived*, never retyped; the two judgement parts — Breaking changes, and how a security fix is described — are the only places you author prose.
**One skill covers every maintenance line.** 6.x and 7.x pages share an identical structure. The line changes the data (version, prior page, JIRA ids), never the process.
## The Iron Rule
```
START FROM THE TEMPLATE. NEVER CLONE THE PREVIOUS VERSION NOTES PAGE.
```
Cloning is how the published pages acquired their defects, and it fails differently every time:
| Page | Inherited defect |
|---|---|
| Version Notes 6.9.0 | Issue Detail links **"JIRA Release Notes 6.8.0"** — label and `version=` id both from 6.8.0 |
| Version Notes 6.10.0 | Issue List links **"Struts 6.9.0 DONE"** — label names the previous release, against a `filter=` id different from the one the 6.9.0 page used |
| Both series | Maven Dependency code macro carries `ac:name=""` instead of `ac:name="language"` |
Half-updated links are the signature failure: the number gets fixed and the label doesn't, or the reverse. They survive review because the link still works — it just points at, or claims to be, the wrong release.
**[`version-notes-template.md`](version-notes-template.md) is the source of truth**: field guidance, storage-format skeleton with those defects corrected, and the pre-publication checklist.
## Collect every input before writing
Each row is derived from a named source. A value you cannot source is a visible placeholder, never a guess.
| Input | Where it comes from |
|---|---|
| Version | The release being voted or announced |
| Parent page | Always **Migration Guide** (page id `13981`) — every Version Notes page is a child of it |
| Prior notes page title | The previous **released** version in the same series — see below |
| JIRA version id | Numeric id behind `ReleaseNote.jspa?version=` — from the WW project's versions, **not** the version name |
| DONE filter id | The saved JIRA filter for this release; a new release needs a new filter |
| Issue list | `project = WW AND fixVersion = <version>`, grouped by type |
| Breaking changes | Authored — see below |
| Staging Repository block | An explicit decision — see below |
## The issue list
Group under `<h2>` per issue type, in this order, omitting any type with no issues:
**Bug → New Feature → Improvement → Task → Dependency**
Within a section, order by issue key ascending. Each entry is `[WW-XXXX] - <the JIRA summary verbatim>`.
**Reconcile against what actually merged.** The JIRA query is the starting point, not the answer. Three mismatches to check:
- A ticket resolved `Fixed` whose change did not make the release branch — it must not be listed as delivered.
- A ticket resolved **`Won't Do`** or otherwise not `Fixed` — it belongs under `Rejected requests`, not in a type section and not dropped. Check the resolution, not just the status: both `Closed` and `Resolved` sit in the Done category.
- Work that shipped under a ticket assigned to a different fix version — the notes under-report the release.
A ticket with no commit in the range is not automatically wrong. Check its **component** first: `IDEA Plugin`, `Example Applications` and similar live in other repositories and are still legitimately part of the release.
**Reconcile through the ticket's linked PR, reading the files it changed.** Do not grep commit subjects, and do not go looking for the class named in the ticket title: a title often names the *symptom* while the fix lives elsewhere. WW-5630 reads "Performance Issue SecurityMemberAccess" and was fixed in `ConfigParseUtil`; searching for the former concludes, wrongly, that the backport is missing. Squash-merges also rewrite hashes, so the merge commit id from the PR need not appear on the branch.
**Untick eted patch-level dependency bumps are not a gap.** Dependabot PRs for patch updates are merged directly and deliberately get no ticket, so they get no entry — there is nothing to link. Expect the pom to show a higher patch version than the ticket text says: 6.11.0 shipped jackson 2.22.1 while WW-5648 reads "2.21.4 to 2.22.0". That is correct, not an omission. Minor and major bumps do get a ticket and do get listed.
Where a ticket's summary was written for triage rather than for users, the page may carry a clearer summary — but then it is authored text, and the link must still resolve to that ticket.
## Only released versions belong in the chain
The prior-notes link forms a chain through the series, and it **skips versions that were cut but never released**. Version Notes 7.2.1 links back to 7.1.1, not to the withdrawn 7.2.0.
When a release is superseded before it ships, its content does not disappear — the successor absorbs it. 7.2.1 carries the Breaking changes for the whole 7.2.x cycle. Check what the predecessor covered before assuming your issue list is complete.
This is the same discipline `creating-security-bulletins` applies to Affected Software, for the same reason: naming a version that never reached users misdirects everyone downstream.
## Page section order
Beyond the boilerplate, sections appear in this order, each omitted when empty:
**Breaking changes → Deprecations → Rejected requests → Bug → New Feature → Improvement → Task → Dependency → Issue Detail → Issue List → Other resources**
The first three are authored; the issue-type sections are derived from JIRA.
## Breaking changes
Present only when the release has them — a maintenance release usually does not.
Each item is **one sentence plus its ticket link**:
```
<what changed, in terms of what an application sees> [WW-XXXX].
```
> Annotated wildcard actions are matched most-specific-first, so action selection can differ [WW-3784].
> `JSONInterceptor` uses a fresh reader and writer per request, so custom ones must not hold state between requests [WW-5650].
The sentence exists so a reader can judge **whether to open the ticket**, not so they can avoid opening it. The ticket carries the detail — API signatures, migration steps, the config that changes. Naming the affected type or setting is enough; enumerating what replaces it is the ticket's job.
Derive each item from the fix diff rather than the ticket title, and write only what you confirmed. A change you suspect is breaking but could not pin down is one to raise with the release manager, not to describe vaguely.
## Deprecations
Where a release deprecates public API, list it separately from Breaking changes — nothing stops working yet, so mixing the two overstates the upgrade cost. Same one-line shape, naming the replacement where there is one:
> `ConversionRule.COLLECTION` and the `Collection_` key prefix are deprecated; use `ConversionRule.ELEMENT` and `Element_` instead [WW-5656].
## Rejected requests
A ticket resolved **`Won't Do`** (or otherwise not `Fixed`) against this fix version is still news: someone asked for it and the project decided against it.
- **Do not put it in a type section.** Under Improvement or New Feature it reads as delivered.
- **Do not silently drop it either.** The decision is the value.
- List it under `Rejected requests`, saying it will not be implemented and, where the release manager gave one, the reason.
> [WW-2635] - Flash scope - will not be implemented; the proposed mechanism could introduce a security risk.
Note the JIRA-generated release notes linked from the page *will* still include these tickets under their type. Clearing the fix version in JIRA is the only way to change that, and is the release manager's call.
## Security fixes in a release
A release usually ships before its bulletin publishes and before a CVE exists. The Version Notes then list a **public, neutrally-framed** ticket for a defect whose advisory is still restricted.
- List the ticket as you would any other. It is already public; omitting it under-reports the release.
- **Do not add security framing the bulletin has not published yet** — no severity, no attack description, no S2-XXX or CVE number that has not been assigned and published.
- Once the bulletin is public, the notes may link it.
**Where the ticket's own summary describes the defect, list the neutral part of it.** "List the summary verbatim" assumes a neutrally-worded ticket, and security tickets often are not. WW-5643 reads *"StrutsJSONReader parse state shared across concurrent requests — maxDepth bypass and cross-request data leak"*; the page carried it up to "concurrent requests" and stopped. The trailing clause is the bulletin's job.
Truncate at the clause boundary — never paraphrase into something the ticket does not say, and never alter the ticket link. Then **tell the release manager which summaries you cut and why**: whether an already-public JIRA summary should be reproduced in full is their call, not yours, and it has to be made before the page goes up rather than edited afterwards.
**REQUIRED BACKGROUND:** where the wording of a security-relevant entry is in question, `creating-security-bulletins` governs what may be said and when.
## The Staging Repository block
**Include it.** The block points readers at ASF Nexus staging so they can test the artifacts before the vote closes, and it stays on the page afterwards.
Older 6.x pages (6.9.0, 6.10.0) lack it while the 7.x pages carry it. That is an artefact of cloning within each series, not a difference between the lines — 6.11.0 carries it.
## Link the new page from the Migration Guide
The page is not finished when it is created. **[Migration Guide](https://cwiki.apache.org/confluence/spaces/WW/pages/13981/Migration+Guide) (id `13981`) is both the parent page and the index**, and a Version Notes page that is not listed there is unreachable by anyone browsing.
Add an entry at the **top** of the list under the `<h2>` for the matching line — `Version Notes 7.x`, `Version Notes 6.x`, and so on. The lists are newest-first, and the entry is a page link carrying no body text:
**Update the section, not the whole page.**`confluence_update_page_section` on the exact heading replaces only that section's body; its boundary is the next `<h2>`, so the section body includes the `<h3>` migration-guide link that follows the list. Supply that `<h3>` and its paragraph in the replacement content or they are dropped.
**Verify against raw storage, not the diff.** A version diff of this page renders empty even for a real change, because the markdown view discards `ac:link` bodies. Fetch the new version with `convert_to_markdown=false` and confirm the new entry is present, the prior entries survive in order, and the trailing `<h3>` appears exactly once.
This applies to **every** section update, including ones on the Version Notes page itself — shortening `Breaking changes` carries the same risk of swallowing the `Deprecations` heading that follows it. After any section write, confirm the sections below it are still present exactly once. Where the page has no `ac:link` in it, the cheaper markdown fetch is enough to see the headings.
## Writing pages through the API
`content_file` is rejected for any path outside the repository — a scratchpad path fails as path traversal. Draft wherever you like, but **pass the body as inline `content`** when creating or updating a page.
The response carries the new version number. On a page you have just written, that number is its own check: a create followed by one update should report version 2, so anything higher means someone else wrote in between.
## The GitHub release notes
A release also has a GitHub release at the `STRUTS_X_Y_Z` tag, kept as a **pre-release** while the vote runs. GitHub's generated body is a starting point that needs two corrections before it is fit to publish.
### Name the previous tag yourself
**Never let GitHub choose the range.** It picks the previous tag by reachability, and Struts release branches get renamed and re-imported, so older tags are frequently *not* ancestors of the new one and the heuristic reaches too far back. For 6.11.0 it chose `STRUTS_6_8_0` and produced ~101 entries, 88 of which had already shipped in 6.9.0 and 6.10.0.
Generate the body with the previous release named explicitly, and it comes out right the first time:
```bash
gh api -X POST repos/apache/struts/releases/generate-notes \
**If you inherit a body GitHub generated on its own**, check the `**Full Changelog**: .../compare/<PREVIOUS>...<THIS>` line first, and regenerate as above rather than pruning by hand. When pruning is unavoidable, drop `## New Contributors` too if the contribution it cites falls outside the range — but keep it when the contributors are genuinely new in this range.
### Split the entries
Two sections, `### Dependencies` nested under `## What's Changed`, before any `## New Contributors`:
| Entry | Section |
|---|---|
| Carries a `WW-XXXX` ticket — whoever authored it | `## What's Changed` |
| A human PR that is not a dependency change (ci, chore, release prep) | `## What's Changed` |
| A dependency bump with **no** ticket | `### Dependencies` |
**The discriminator is the ticket, not the author.** A Dependabot PR carrying a ticket stays in What's Changed, because a ticketed bump is release content and appears in the Version Notes Dependency section. A human PR that is purely a dependency change (`Removes unused jaxb-core dependency`) belongs under Dependencies. Both cases occur in the 6.9.0 release.
A PR that mixes a dependency change with something else — CVE-driven library updates *plus* a CI tweak — stays in What's Changed. Dependencies is for entries that are nothing but a bump.
Preserve the generated relative order within each section, and keep the entry lines byte-identical — they carry the author and PR links GitHub rendered. Split with a script rather than by retyping, then **prove nothing was lost**:
Pass `--prerelease` either way, so a release still under vote is not silently promoted, and `--verify-tag` on create so a typo in the tag fails instead of creating one.
## The test-build announcement
Once the Version Notes page and the GitHub release are both up, the release manager announces the test build so people can exercise the staged artifacts during the vote. **Draft it last** — every link in it points at something the earlier steps produced.
Subject is `[TEST] Apache Struts X.Y.Z test build is ready`. Send it to **both** lists, Bcc the private one:
```
To: dev@struts.apache.org, user@struts.apache.org
Bcc: private@struts.apache.org
```
Both audiences want it — committers to check the staged artifacts, users to test against their own applications — and a build announced to only one of them reaches half the people who could find a problem during the vote.
The body is fixed apart from four substitutions:
```
Hello,
This is a minor release of Struts <LINE> which contains <WHAT>, and it
shouldn't break your code<RISK>. Please take your time and test the bits
- any help is appreciated. Please report any problems you will spot.
| `<WHAT>` | What the issue list actually contains — `mostly bug fixes` for 6.11.0, `a few improvements and bug fixes` for 7.3.0 |
| `<RISK>` | Empty when the release has no Breaking changes; ` but it contains significant changes` when it does. 6.11.0 had none and said nothing; 7.3.0 had seven and said so |
| Tag / paths | Tag underscored (`STRUTS_7_3_0`), dist path and page title dotted (`7.3.0`) |
Do not take the recipients from a previous announcement: 6.11.0 went to `dev@` alone and 7.3.0 to `user@` alone, and both were mistakes. Address every announcement to the two lists above.
Keep the security posture of the pages: the mail links the release notes, it does not summarise what is in them, so no severity, CVE or S2-XXX reaches it either.
**The vote is the next step, and it is a different mail.** Once the test build is announced, `creating-release-vote-mail` composes the `[VOTE] Apache Struts X.Y.Z` call — to `dev@` alone, rendered from the page this skill produced. Do not draft it from here: its recipients, subject and body all differ from the announcement above.
## Re-read the page immediately before you write to it
Confluence has no conflict warning. Fetch the current version immediately before every write and compare the version number against the one you read; if it advanced, re-read, merge onto the newer content, and write that.
After writing, diff against the version you meant to build on. The diff should show only your intended change.
## Red Flags — STOP
- Starting from a copy of the previous release's page
- A version number or JIRA id typed rather than derived
- A link whose label and its id name different releases
- The prior-notes link pointing at a version that was cut but never released
- Publishing the issue list straight from JIRA without reconciling against the release branch
- Concluding a backport is missing from a commit-subject grep, or from the class named in the ticket title
- Treating an untick eted patch dependency bump as a reconciliation gap
- Dropping a `Won't Do` ticket, or listing it under Improvement or New Feature as though it shipped
- A Breaking changes item that runs past one sentence, or restates what the ticket already explains
- Reproducing a security ticket's summary in full when it names the bypass or the leak
- Letting GitHub pick the previous tag instead of passing `previous_tag_name`
- Regrouping release entries by retyping them instead of scripting the split and diffing the result
- A severity, CVE, or S2-XXX reference on the page that has not been published
- Breaking changes assembled by pasting ticket summaries
- Creating the page without adding it to the Migration Guide index
- Trusting an empty version diff on the Migration Guide as proof the edit landed
- Publishing GitHub release notes without checking which tag the Full Changelog compares against
- Splitting the GitHub sections by author instead of by whether the entry carries a ticket
- Editing a GitHub release under vote without `--prerelease`
- Writing from page content read earlier in the session without re-fetching
## Common Mistakes
| Mistake | Reality |
|---|---|
| "Copying last release's page is faster" | It is how "JIRA Release Notes 6.8.0" shipped on the 6.9.0 page. Copy the template. |
| "I updated the link, it's fine" | Check the label too. Every observed defect is a half-updated link. |
| "`version=` takes the version number" | It takes JIRA's numeric version id. Look it up. |
| "The DONE filter can be reused" | A reused filter shows the previous release's issues under this release's heading. |
| "JIRA is the release contents" | JIRA is the claim. The release branch is the fact. Reconcile. |
| "No commit mentions the ticket, so it wasn't backported" | Read the linked PR's changed files. Titles name symptoms, and squash-merges rewrite hashes. |
| "The pom version doesn't match the ticket, that's a gap" | Patch bumps ship untick eted by design. Only ticketed bumps get an entry. |
| "The page is created, so the work is done" | It is invisible until listed on the Migration Guide. |
| "The version diff is empty, so nothing changed" | The diff renders markdown, which drops `ac:link` bodies. Check raw storage. |
| "GitHub generated the changelog, so the range is right" | It guesses the previous tag by reachability. Renamed branches make it reach too far back. Verify with `git log PREV..THIS`. |
| "Dependabot authored it, so it goes under Dependencies" | Ticketed bumps stay in What's Changed. The ticket decides, not the author. |
| "A Won't Do ticket isn't part of the release" | The decision is news. It goes under Rejected requests, not into a type section and not into the bin. |
| "More detail in Breaking changes is safer" | One sentence plus the ticket link. The reader opens the ticket for detail; the page exists to tell them whether to. |
| "The summary is public in JIRA, so I can repeat it" | Not when it names the bypass or the leak and the bulletin is unpublished. Truncate, and say you did. |
| "GitHub will work out the previous tag" | Pass `previous_tag_name` and it is right the first time. |
| "The fix is public, so I can describe the vulnerability" | The ticket being public does not publish the advisory. Neutral framing until the bulletin ships. |
| "Breaking changes are the tickets typed as breaking" | They are the changes that break an application. Author them. |
| "7.x needs different handling from 6.x" | Same structure, same process. Only the data differs. |
The canonical skeleton and per-field guidance for a Struts **Version Notes X.Y.Z** page
on the [Apache Struts 2 Wiki](https://cwiki.apache.org/confluence/spaces/WW) (space `WW`).
Companion to [`SKILL.md`](SKILL.md), which covers *how* to establish the values;
this file covers *what the page contains*.
**This file is the source of truth.** Start every page from the skeleton below, never
from a copy of the previous release's page — see the Iron Rule in `SKILL.md`.
## Fields
| Field | What goes in it |
|---|---|
| Version | The release being announced, e.g. `6.11.0`. Appears in the intro sentence, the page title, the Maven snippet, and both JIRA link labels. |
| Parent page | Always `Migration Guide`, page id `13981`. Create the page as its child, and add it to that page's index — see `SKILL.md`. |
| Prior notes page | Title of the previous **released** version's page in the same series, e.g. `Version Notes 6.10.0`. Skip versions that were cut but never released. |
| JIRA version id | The numeric id for `ReleaseNote.jspa?version=`. Obtain from the WW project's versions — it is not the version name. `6.10.0` is `12357065`, `7.2.1` is `12355751`. |
| DONE filter id | Saved-filter id for `issues/?filter=`, labelled `Struts X.Y.Z DONE`. Each release needs its own; a reused id lists the wrong release. |
| TODO filter id | Constant across releases: `12351174`, labelled `Struts x.x.x TODO`. |
| Issue sections | One `<h2>` per issue type present, ordered **Bug → New Feature → Improvement → Task → Dependency**, entries sorted by key ascending. |
| Breaking changes | Optional. Authored, **one sentence plus the ticket link** per item. Omit the section when the release has none. |
| Deprecations | Optional. Same one-line shape, for public API deprecated but still working. |
| Rejected requests | Optional. Tickets resolved `Won't Do` against this fix version — never in a type section. |
| Staging Repository | Always included, on every line — see `SKILL.md`. |
## Corrected storage format
Three defects present in the published pages are fixed here. Keep them fixed:
1.**`ac:name="language"` on the code macros.** The published Maven Dependency and
Staging Repository macros carry `ac:name=""` with the value `xml`, which is a
malformed parameter. The Archetype Catalog macro on the same pages has it right.
2.**No `ac:macro-id` attributes.** The published pages share hard-coded macro ids
across releases and across series because they were cloned. Omit the attribute and
let Confluence assign one on save.
3.**No trailing empty `<div>`s.** Every published page ends with two empty divs
carrying inline `font-size: 24.0px` styling. They render as stray whitespace.
```xml
<p><ac:emoticonac:name="tick"/> These are the notes for the Struts version X.Y.Z distribution.</p>
<p><ac:emoticonac:name="tick"/> For prior notes in this release series, see <ac:link><ri:pageri:content-title="Version Notes PRIOR"/></ac:link></p>
<p>If you are a Maven user, you might want to get started using the <ac:link><ri:pageri:content-title="Struts 2 Maven Archetypes"/><ac:plain-text-link-body><![CDATA[Maven Archetype]]></ac:plain-text-link-body></ac:link>.</p>
<!-- OPTIONAL: omit the whole section when the release has no breaking changes -->
<h2>Breaking changes</h2>
<ulstyle="list-style-type: square;">
<li>ONE SENTENCE: WHAT AN APPLICATION SEES DIFFERENTLY [<ahref="https://issues.apache.org/jira/browse/WW-XXXX">WW-XXXX</a>].</li>
</ul>
<!-- OPTIONAL: public API deprecated but still working -->
<h2>Deprecations</h2>
<ulstyle="list-style-type: square;">
<li><code>WHAT</code> is deprecated; use <code>REPLACEMENT</code> instead [<ahref="https://issues.apache.org/jira/browse/WW-XXXX">WW-XXXX</a>].</li>
</ul>
<!-- OPTIONAL: tickets resolved Won't Do against this fix version -->
<h2>Rejected requests</h2>
<p>Two long-standing requests were closed as <em>Won't Do</em> in this cycle. They are listed here so the decision is visible rather than silent.</p>
<ulstyle="list-style-type: square;">
<li>[<ahref="https://issues.apache.org/jira/browse/WW-XXXX">WW-XXXX</a>] - SUMMARY - will not be implemented; REASON WHERE THE RELEASE MANAGER GAVE ONE.</li>
- [ ] Each Breaking changes and Deprecations item is one sentence plus its ticket link.
- [ ] Staging Repository block present.
- [ ] No unpublished severity, CVE, or S2-XXX reference anywhere on the page, and any security summary truncated at a clause boundary was reported to the release manager.
- [ ] Page created as a child of Migration Guide (`13981`).
- [ ]**Listed at the top of the matching `Version Notes N.x` section on the Migration Guide**, and that edit verified against raw storage — the version diff renders empty even when the change landed.
- [ ] Page re-fetched immediately before every write.
## GitHub release notes
- [ ] Body generated with `previous_tag_name` named explicitly, not left to GitHub's guess.
- [ ] Entry count sane against `git log PREV..THIS`.
- [ ] Original body saved first when editing an existing release, so it can be restored.
- [ ] Entries split by **ticket, not author**: ticketed → `## What's Changed`; untick eted dependency bumps → `### Dependencies`; mixed PRs stay in What's Changed.
- [ ] Generated order and entry text preserved within each section.
- [ ] Split verified by diffing the sorted entry lists before and after — empty output.
- [ ]`--prerelease` passed while the vote is open; `--verify-tag` when creating.
## Test-build announcement
- [ ] Drafted **after** the Version Notes page and GitHub release exist — it links both.
- [ ] Subject `[TEST] Apache Struts X.Y.Z test build is ready`.
- [ ] Addressed to **both**`dev@struts.apache.org` and `user@struts.apache.org`, Bcc `private@struts.apache.org`.
- [ ] Risk clause matches reality: silent when there are no Breaking changes, "but it contains significant changes" when there are.
- [ ] Tag underscored in the release link, version dotted in the dist path and page title.
description: Use when a vulnerability or security report arrives for triage, when assessing a CVE/RCE/OGNL/injection claim against the code, or when drafting a reply to a security researcher — to research the claim from source without trusting the reporter and without fabricating your own facts.
---
# Triaging Security Reports
## Overview
A security report is a **claim to be tested, not a finding to be confirmed or rebutted**. The reporter may be right, wrong, partially right, or right about the symptom and wrong about the cause. Your job is to independently re-derive the truth from current source.
**Core principle:** Every factual statement that ends up in your assessment or reply — the reporter's claims *and your own* — must be traced to current source code before you write it down. The most common failure is not believing the reporter; it is **inventing supporting facts to justify a verdict you already reached.**
**Process authority:** [`SECURITY.md`](../../../SECURITY.md) is the source of truth for the disclosure process (private handling, assessment checklist, reporting rules). Read it. This skill governs *how you research and respond*, not the process itself.
## The Iron Rule
```
NO CLAIM IN A SECURITY RESPONSE WITHOUT A FILE:LINE YOU READ THIS SESSION.
```
Applies to the verdict, every mitigation you cite, and every "default" you state. If you can't point to the line, you can't write the sentence.
## Research: report-blind, not report-led
Read the report once to know what to investigate. Then **research as if you were auditing that area cold** — do not let the report's framing drive your search.
For each claim, independently verify:
| Reporter asserts | You must verify from source |
|---|---|
| A line number ("bug is at X:392") | Read that line **and its call path** — is it even reachable as described? |
| A severity / CVSS | Re-derive from actual exploitability, not their number |
| "No mitigation / no gate exists" | Search for gates, filters, allowlists, authorizers *yourself* — absence claims are the most often wrong |
| "Default configuration" | Check the **effective runtime default**, not one source (see trap below) |
| "Same as CVE-XXXX" | Confirm the mechanism actually matches; analogy ≠ equivalence |
| A working PoC | **Run it if it is runnable**, then trace whether the payload survives every filter on the path |
If the report has **no reproducible PoC against a default config**, that is itself a triage outcome — say so per `SECURITY.md`.
## Find the control case
A single odd behaviour is ambiguous — it can nearly always be read as intended. What settles it is the **sibling that behaves correctly under the same input**.
Before writing a verdict, find the case that ought to differ and check it: the annotated property beside the unannotated one, the ordinary setter beside the dynamic one, the sibling path the same control does cover. Behave alike and you are probably looking at a design decision. Diverge, and the control is incomplete — that divergence *is* the finding.
Prefer an executed differential to an argued one. An existing test that passes beside the reporter's failing one is the strongest evidence a triage can produce.
## The effective-default trap
A Java field initializer and the shipped config can disagree. Reading only one produces a confident, wrong claim.
```java
privatebooleanrequireAnnotations=false;// field initializer
```
```properties
struts.parameters.requireAnnotations=true # default.properties OVERRIDES it
```
**The effective default is `true`.** Always trace the full chain: field initializer → `@Inject` setter → `default.properties` → any struts.xml override. State the *effective runtime* value, and cite the file that actually wins.
## Vulnerability vs. operator responsibility
"In the default configuration" is a crutch — drop it. Decide the real question:
- **Is it a vulnerability?** Then it's a vulnerability whether or not it's the default. Handle it privately per `SECURITY.md`.
- **Does it require an operator to opt into an insecure configuration?** A documented, opt-in setting (e.g. `cookiesName=*`, `devMode=true`) that works as advertised is the operator's responsibility, provided the docs carry the warning. Say "X works as documented; the operator owns the security implications of enabling it" — not "not a vuln *in the default config*."
- **Is the RCE/escalation only reachable via application code the framework can't constrain?** (e.g. an action that moves an uploaded file to a web root.) Then it's an application concern, not a framework vulnerability — state that boundary explicitly.
## Drafting the reply
- Lead with the verdict and the *reason*, both grounded in file:line.
- Cite a source for every mitigation you mention. If you didn't verify it this session, delete the sentence.
- Prefer "works as documented / operator responsibility" framing over "default configuration."
- **Don't over-promise.** Before pledging a hardening change, check it doesn't already exist (it often does) and that you intend to actually do it.
- Acknowledge anything the reporter got right (e.g. correct CVE-fix verification) — it builds the relationship and signals you actually read it.
- Keep it private: no public issue, PR, Jira, or list thread before triage. Never open a PR that is itself the security fix (see [`CLAUDE.md`](../../../CLAUDE.md)).
## Red Flags — STOP
- About to write "this is mitigated by X" — did you read X's line *this session*?
- About to state a "default" from a field initializer — did you check `default.properties`?
- Citing the reporter's line number without having traced its call path.
- Asserting "no gate / no check exists" without having grepped for it.
- Two of your own claims contradict each other → at least one is unverified. Stop and verify both.
- Promising a fix/warning "we'll add" without checking it isn't already there.
- Writing "not a vulnerability in the default configuration" → reframe as vuln-or-not + operator responsibility.
## Common Mistakes
| Mistake | Reality |
|---|---|
| "Reporter cited line 392, so that's the bug site" | A line is only a bug if it's *reachable* as described. Trace callers. |
| "The field defaults to false, so the gate is off by default" | `default.properties` may override it to true. Check the effective value. |
| "I'll add a mitigation to strengthen the rejection" | An unverified mitigation that's wrong discredits the whole response. Verify or omit. |
| "It rejects the payload, obviously" | Confirm the specific PoC string fails the specific filter (e.g. full-match regex `ACCEPTED_PATTERN`). |
| "We should add a startup warning" | Grep first — the warning frequently already exists. |
| "Not a vuln in default config" | Either it's a vuln or it's operator-owned opt-in. The default-config hedge muddies both. |
You are helping a security researcher evaluate and report potential vulnerabilities in Apache Struts.
[`SECURITY.md`](SECURITY.md) is the source of truth for the Apache Struts vulnerability reporting process. **Read it first and follow it.** This file is a short
LLM-facing wrapper around that policy; it does not replace it.
## Workflow
Before drafting any report, opening an issue, posting publicly, or reaching a security conclusion:
1.**Pre-reporting checks** — complete every step in [`SECURITY.md` § Before Reporting](SECURITY.md#before-reporting):
- read the Struts security policy,
- read the Struts security guidelines,
- check previously disclosed vulnerabilities and Security Bulletins.
2.**Assess** the finding against the questions in [`SECURITY.md` § Assessment](SECURITY.md#assessment), and route it through the disposition guide in
[`THREAT_MODEL.md`](THREAT_MODEL.md) (scope, trust boundaries, in-model security properties, and the known-non-findings list). If the answers do not still
point to a likely new framework vulnerability, stop and explain — do not draft a new report.
3.**Report privately** to `security@struts.apache.org` following [`SECURITY.md` § Private Report Requirements](SECURITY.md#private-report-requirements) and
Do not open a public GitHub issue, Jira issue, pull request, mailing list thread, or discussion for a suspected vulnerability before private triage.
## Rules for AI Agents
- **Never submit a pull request that fixes a suspected vulnerability.** Before opening any PR, verify the change is not a security patch — OGNL injection,
parameter filtering bypass, file upload exploit, authentication or authorization bypass, RCE, SSRF, path traversal, deserialization, XSS in framework
components, etc. If it is, stop and direct the researcher to report it privately to `security@struts.apache.org` instead. Vulnerability fixes go through
the private security process, not public PRs.
- Do not speculate beyond what can be demonstrated. If severity is uncertain, say so explicitly.
- If the issue turns out to be application misconfiguration, an already-disclosed CVE, or a non-Struts problem, stop and explain — do not draft a new report.
@@ -6,38 +6,19 @@ For detailed procedures, use the specialized agents and commands in `.claude/age
## Project Overview
Apache Struts is a mature MVC web application framework for Java (originally WebWork 2). Current version: *
*7.2.0-SNAPSHOT**.
Apache Struts is a mature MVC web application framework for Java (originally WebWork 2). Current version: **7.2.0-SNAPSHOT**. Uses OGNL for value stack expressions and FreeMarker for UI tag templates.
### Build Commands
```bash
# Full build with tests
mvn clean install
# Run all tests (faster, skips assembly)
# Run tests (skip assembly for speed)
mvn test -DskipAssembly
# Run single test class
mvn test -DskipAssembly -Dtest=MyClassTest
# Single test in specific module
mvn test -DskipAssembly -pl core -Dtest=MyClassTest#testMethodName
# Run single test method
mvn test -DskipAssembly -Dtest=MyClassTest#testMethodName
Run `/security_scan` for comprehensive security analysis.
## Security Reports & Scans
For any security-related activity — vulnerability scans, security analysis, drafting security reports — **[`SECURITY.md`](SECURITY.md) is the source of truth**.
Read it first and follow its pre-reporting checks, assessment checklist, and reporting requirements. Reports must be sent privately to
`security@struts.apache.org`; do not open a public GitHub issue, Jira issue, pull request, or mailing list thread for a suspected vulnerability before private
triage. [`AGENTS.md`](AGENTS.md) is a shorter LLM-facing wrapper around the same process.
## Testing
**Priority order for running tests:**
1.**JetBrains MCP** (in IntelliJ): `mcp__jetbrains__execute_run_configuration`
2.**test-runner agent**: `Task` tool with `subagent_type="test-runner"`
3.**Direct Maven**: `mvn test -DskipAssembly -Dtest=TestClassName`
Tests use JUnit 5 with AssertJ assertions and Mockito for mocking.
1. Never use `File.createTempFile()` without controlling the directory
2. Always clean up temporary files (track and delete in finally blocks)
3. Test error paths and cleanup behavior, not just happy paths
4. Don't catch generic `Exception` - catch specific types
5. Use `protected` visibility for methods subclasses may override
- **Never submit a PR that fixes a suspected vulnerability.** Before opening a PR, verify the change is not a security patch (OGNL injection, parameter
filtering bypass, file upload exploit, auth bypass, RCE, SSRF, path traversal, deserialization, XSS in framework components, etc.). If it is, stop and report
it privately to `security@struts.apache.org` — see [`SECURITY.md`](SECURITY.md).
## Do not disclose through a pull request, commit, or issue
**A fix is a disclosure.** Opening a public pull request, pushing a commit, branch, or
fork, or filing a public Jira/GitHub issue that **fixes, describes, or hints at** a
suspected vulnerability reveals where the weakness is — often with a working roadmap to
exploit it — before a fixed release exists. This holds even if you never attach a
proof-of-concept, and even if you believe the impact is low or you are "just hardening"
the code.
If you have found, or suspect you have found, a security problem:
- **Do not** open a public PR, commit, branch, fork, Jira issue, or mailing-list thread
for it.
- **Do** email [security@struts.apache.org](mailto:security@struts.apache.org) first and
wait for the PMC to triage it and agree how the fix will be handled — the fix is
typically prepared privately and landed alongside the advisory and release.
If you notice a possible security issue while working on an unrelated bug or PR, stop and
email the private list before pushing the change. **When in doubt, treat it as
security-sensitive and email the list** — a private report that turns out to be a
non-issue costs far less than a public change that turns out to be exploitable.
## Before Reporting
Before sending a vulnerability report, run through the following checks. They exist to prevent duplicate reports, public disclosure of untriaged issues,
and reports for behavior that is already documented as insecure configuration.
### 1. Read this policy
Confirm:
- which Struts versions are currently supported (see [Supported Versions](#supported-versions)),
- where reports must be sent (see [Reporting New Security Issues](#reporting-new-security-issues-with-the-apache-struts)),
- which reports do not belong on the private security list.
### 2. Read the Struts security guidelines
Review the [Struts security guidance](https://struts.apache.org/security/) and determine whether the finding is already covered by documented secure
configuration or application guidance, including but not limited to:
- Config Browser Plugin exposure,
- direct JSP access,
-`devMode` is required to exploit the vulnerability,
-`@StrutsParameter` usage and parameter annotation requirements,
- unsafe setters or getters exposed to request parameters,
- use of incoming values in localization or forced OGNL evaluation,
- raw JSP EL expressions,
- custom error pages,
- Dynamic Method Invocation and Strict Method Invocation,
- accepted and excluded parameter patterns,
- Fetch Metadata, COOP, and COEP protections,
- OGNL sandboxing, allowlists, excluded classes/packages, and OGNL Guard settings.
If the behavior is caused by an application ignoring documented security guidance, that is not an Apache Struts framework vulnerability.
### 3. Check previously disclosed vulnerabilities
Compare the finding against already disclosed Struts vulnerabilities — affected versions, impact ratings, mitigations, and fixed versions:
If the finding overlaps with a known vulnerability, link to the existing bulletin, advisory, CVE, or release notes instead of drafting a new report.
## Assessment
Before drafting a report, confirm:
1. Is the affected version supported?
2. Is the behavior in Apache Struts framework code, rather than only in an application using Struts?
3. Is it already documented as insecure configuration or unsupported usage?
4. Is it a duplicate of a previously disclosed vulnerability or Security Bulletin?
5. Can the impact be demonstrated with a minimal, self-contained reproduction?
Only proceed with a private report when these answers still point to a likely new vulnerability in the framework.
## Private Report Requirements
A useful private report includes:
- affected Struts version or version range,
- affected component or module,
- required application configuration, if any,
- minimal reproduction steps,
- expected behavior,
- actual behavior,
- demonstrated security impact,
- whether authentication or special privileges are required,
- proposed fix or mitigation, if known.
Do not speculate beyond what can be demonstrated. If severity is uncertain, say so explicitly.
## Report Quality Rules
- One vulnerability per report.
- Keep reproduction steps minimal and self-contained.
- Do not include unrelated findings.
- Do not publish exploit details or proof-of-concept code publicly before the Struts project has triaged the issue. **A fix, patch, or hardening change is a
public disclosure in the same way a PoC is** — see [Do not disclose through a pull request, commit, or issue](#do-not-disclose-through-a-pull-request-commit-or-issue).
**Pushing a PoC to a public GitHub repository, gist, fork, or branch counts as public disclosure** — even a "test" or throwaway repo. Private repositories
are acceptable for sharing a PoC, but access must be granted individually to each PMC member who will triage the report.
- Do not send ordinary bugs, usage questions, or generic denial-of-service concerns to the private security list.
- If the issue is not a vulnerability in Apache Struts source code, use the appropriate public support or issue channel instead.
exposing it in production is operator misconfiguration. `OUT-OF-MODEL: non-default-config`.
- **"I can enumerate / pass arbitrary parameters."** Parameter binding is the point of
the framework; in-model only when it crosses the default annotation/allowlist
protections.
- **"Generic DoS: I streamed a huge body / hammered a URL."** Not accepted per
`SECURITY.md`; only super-linear amplification inside framework code is considered.
- **Duplicate of a disclosed S2-series bulletin/CVE** — closed by reference.
- **Dependency-tail CVEs** (a transitive jar, e.g. a logging or XML library) from an
SCA scan — triage upstream unless Struts' own code reaches the vulnerable path with
untrusted input.
## §12 Conditions that would change this model
- A change to the default-hardening set (e.g. a new secure-by-default knob, or a
default flipped) — re-baseline §5a/§8/§11a.
- A new request-facing surface, a new bundled plugin, or a new expression/templating
integration with its own trust surface.
- A change to how OGNL evaluation, the allowlist, or parameter binding works.
- A report that cannot be routed to a §13 disposition → revise §8/§9.
## §13 Triage dispositions
| Disposition | Meaning | Licensed by |
| --- | --- | --- |
| `VALID` | A §8 property breaks via an untrusted HTTP client on a current-version, default-hardened app. | §8, §6, §7 |
| `VALID-HARDENING` | A §11 misuse is too easy, or a default could be tightened. | §11/§5a |
| `OUT-OF-MODEL: application-responsibility` | Requires a developer anti-pattern (unsafe setter, raw EL, forced eval, direct JSP) or the app's own authn/authz. | §3/§10 |
| `OUT-OF-MODEL: non-default-config` | Only manifests with `devMode`, a dev-only plugin, DMI, or a disabled default protection. | §5a |
<p>The Struts Showcase demonstrates a variety of use cases and tag usages. Essentially, the application exercises various framework features in isolation. The Showcase is not meant as a "best practices" example.</p>
<p>For more "by example" solutions, see the <a href="https://github.com/apache/struts-examples" class="btn btn-primary btn-large">Struts Examples »</a> pages.</p>
<p>For more "by example" solutions, see the <a href="https://github.com/apache/struts-examples" class="btn btn-primary btn-lg">Struts Examples »</a> pages.</p>
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.