[
https://issues.apache.org/jira/browse/WW-5702?focusedWorklogId=1041260&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1041260
]
ASF GitHub Bot logged work on WW-5702:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Sep/26 05:50
Start Date: 14/Sep/26 05:50
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1935:
URL: https://github.com/apache/struts/pull/1935
Part of [WW-5702](https://issues.apache.org/jira/browse/WW-5702) — items 2,
5, 6, 8, 9, 10 and 11 (provider hygiene). Follows #1934 (scope gaps); the six
html5 templates (item 12) come last. Docs: apache/struts-site PR linked below.
## What changed
- **Item 2 — `data-msg-*` only on controls that submit a value.** `s:label`
and unrecognised `type=` values (`HtmlControlType.UNSUPPORTED`) get no message.
Select, checkbox and hidden keep the documented script hook — the ticket's
literal rule ("supports at least one constraint") would have dropped it there
too, contradicting the docs.
- **Item 5 — `max` gets the same finiteness guard as `min`.** A `date` range
on a control the developer declared numeric no longer renders
`Date.toString()`; a NaN/infinite double bound is omitted.
- **Item 6 — integrality decided on the decimal representation**
(`BigDecimal(toString()).stripTrailingZeros().scale() <= 0`), which is also
what gets rendered. `1.0000000000000000001` no longer passes as integral and
shifts the step base.
- **Item 8 — a provider's `type` is discarded** before rendering
(case-insensitively), and the `HtmlConstraintProvider` Javadoc no longer
advertises the `type="email"` override that produced a duplicate attribute.
- **Item 9 — dynamic *and* declared attribute names compared
case-insensitively**, as HTML does: a developer's `MAXLENGTH` wins over the
derived `maxlength`; a provider's `Maxlength` yields to a declared `maxlength`.
- **Item 10 — validator type must be a plain attribute name**
(`[A-Za-z0-9_.-]+`) to become part of `data-msg-*`; the name is outside
FreeMarker's escaping. Dots stay legal (`acme.required`), colons don't (XML
namespace prefix).
- **Item 11 — `EcmaScriptSafeRegex` aligned with the mode browsers actually
compile `pattern` in.** HTML uses the `v` (unicode sets) flag, which is
stricter than the `u` flag the class assumed: inside a class `( ) { } / |` must
be escaped, a hyphen is only a range operator between two plain literals,
doubled punctuators are reserved, `[^` is the class opening not a literal;
outside a class `\-` is illegal and a lone `]`/`}` or a stacked quantifier
(`a{2}{3}`) is an error. All of these are literals or legal in Java, so
patterns like the everyday `[a-z0-9._%+-]+@` passed the allowlist and were
silently dropped by the browser. Verified against node 24 and a differential
corpus (every accepted pattern compiles in both engines).
`HtmlControlType.OTHER` is renamed `UNSUPPORTED` — the enum is unreleased,
and the constant is now load-bearing (no constraint, no message); `OTHER`
conflated unknown `type` values with non-submitting components.
## Correction to the ticket
Item 2 says no assertion in `StrutsHtmlConstraintProviderTest` exercises the
message path. One did
(`messageIsEmittedEvenForAValidatorThatContributesNoConstraint`, mock validator
+ real action). The constraint helper passes a null action on purpose: concrete
validators need an injected `ValueStack` for `getMessage`, which a plain unit
test has no container for — so message-path tests use mocks, and the new ones
follow that pattern.
## Tests
`mvn test -DskipAssembly -pl core`: 3358 tests, 0 failures. Every production
change went red first; the two `UIBean` halves of item 9 were each re-run
against the old code to confirm they fail. Three review passes (security,
`/code-review high` twice — the second on the regex rewrite) landed as the
second and third commits.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1041260)
Time Spent: 50m (was: 40m)
> Polish HTML5 constraint derivation: scope gaps and attribute hygiene found
> reviewing WW-5695
> --------------------------------------------------------------------------------------------
>
> Key: WW-5702
> URL: https://issues.apache.org/jira/browse/WW-5702
> Project: Struts 2
> Issue Type: Task
> Components: Core Tags
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Follow-up to WW-5695 (PR #1865), collecting review findings that are real but
> not blocking. None of these is a false-reject; the false-reject class was
> fixed on the WW-5695 branch itself.
> The governing rule for the feature is unchanged: emit a constraint only when
> the browser cannot reject input the server would accept, and never set or
> change an input's {{type}}.
> h3. 1. Derivation is not gated on the theme
> {{UIBean.addConstraintAttributes}} guards on {{struts.ui.html5.constraints}}
> but not on the resolved theme, while only
> {{template/html5/common-attributes.ftl}} renders the map. Enabling the
> constant for a few html5 forms makes every xhtml/simple/css\_xhtml form in
> the application resolve its action's validators and call {{getMessage()}} per
> field to build a map nothing renders. Gate on the resolved theme as well as
> the constant. Note {{ConstraintAttributesTest}} never calls {{setTheme}}, so
> it currently asserts derivation under the default xhtml theme.
> h3. 2. data-msg-* is emitted regardless of control type
> {{StrutsHtmlConstraintProvider.constraintsFor}} calls {{addMessage}} for
> every validator independently of whether {{addConstraints}} derived anything
> and independently of the control. A checkbox whose {{required}} constraint
> was deliberately suppressed still renders {{data-msg-required}}, and
> {{s:label}} renders validation text onto an element that never submits. Gate
> {{addMessage}} on the control supporting at least one constraint.
> The test that should catch this is vacuous:
> {{StrutsHtmlConstraintProviderTest}}'s helper passes a null action, and
> {{addMessage}} returns immediately on null, so no assertion in that class
> exercises the message path at all. Fix the helper to pass a real action.
> h3. 3. Forms with no action attribute resolve validators under an empty
> context
> {{Form.resolveActionValidators}} calls {{findString(action)}}, but {{action}}
> is frequently null: {{ServletUrlRenderer.renderFormUrl}} resolves the name
> from the current invocation into {{attributes.actionName}} without writing it
> back to the field. {{findString(null)}} does not throw -
> {{OgnlTextParser.evaluate}} coerces null to the empty string - and
> {{DefaultActionMapper.getMappingFromActionName("")}} returns a non-null
> mapping, so the {{attributes.actionName}} fallback is unreachable and the
> context becomes the empty string. Alias-scoped
> {{ActionClass-alias-validation.xml}} validators are then silently skipped,
> for the most common form usage.
> This defect is inherited from the deprecated {{getValidators(String)}}, not
> introduced by WW-5695; what is new is that the path now runs for every field
> of every form. Fall back to {{attributes.actionName}} when
> {{findString(action)}} is blank. No test covers an action-less form.
> h3. 4. Visitor-validated nested fields get a message and never a constraint
> {{Form.findFieldValidators}} wraps prefixed-field validators in
> {{Form.FieldVisitorValidatorWrapper}}, which implements only
> {{FieldValidator}}. {{addConstraints}} dispatches on concrete validator
> types, so for the common nested-bean case - a textfield named {{user.name}}
> behind a {{visitor}} validator on {{user}} - no constraint is ever derived,
> while the wrapper's {{getValidatorType()}} returns the literal
> {{field-visitor}} and {{getMessage()}} delegates, so the field renders
> {{data-msg-field-visitor}} and nothing else. Unwrap via
> {{getFieldValidator()}} before dispatching, or skip wrapped validators
> entirely. Same visitor blind spot recorded against WW-3530.
> h3. 5. max is emitted without the type guard applied to min
> {{addRange}} guards {{min}} through {{isIntegral}} but passes {{max}}
> straight to {{putIfPresent}}, which stringifies whatever it is given. A
> date-typed or string-typed range validator on a control the developer
> declared numeric emits a nonsense {{max}}. {{addDoubleRange}} has the same
> asymmetry for NaN and infinite upper bounds. Browsers ignore an unparseable
> bound, so this is invalid markup rather than a false reject.
> h3. 6. isIntegral rounds through double
> {{isIntegral}} tests {{doubleValue()}} while {{putIfPresent}} renders the
> original object, so a custom range validator carrying a BigDecimal such as
> 1.0000000000000000001 passes the guard and then renders that fractional value
> as {{min}}, shifting the HTML step base. No shipped range validator is
> affected - only Short, Int, Long and Date subclasses exist - so this needs a
> custom validator to reach. Determine integrality from the decimal
> representation instead.
> h3. 7. CHECKBOX and HIDDEN are unreachable control types
> The enum declares both but only six components override {{getControlType()}},
> so {{Checkbox}} and {{Hidden}} inherit {{OTHER}} and a replacement provider
> cannot distinguish them. Returning their real types is behaviourally inert
> with the default provider, which ignores both, so this is purely about the
> extension contract being honest.
> h3. 8. The provider javadoc advertises a type override that cannot work
> {{HtmlConstraintProvider}}'s javadoc suggests a replacement provider could
> map an email validator to {{type="email"}}. It cannot: {{TextField}} only
> puts {{type}} into the attribute map when the developer set it, so
> {{isAlreadyRendered}} does not filter it, and {{html5/text.ftl}} has already
> emitted a hardcoded {{type="text"}} by the time the constraint map renders -
> producing a duplicate attribute of which the browser keeps the first. The
> example also contradicts the never-change-type rule stated in
> {{StrutsHtmlConstraintProvider}}. Correct the javadoc, and consider dropping
> {{type}} from the derived map outright.
> h3. 9. Dynamic attribute names are compared case-sensitively
> {{isAlreadyRendered}} does an exact-key lookup in {{dynamicAttributes}}, but
> HTML attribute names are ASCII case-insensitive. A dynamic {{MAXLENGTH}}
> misses the check and renders after the derived {{maxlength}}, so the browser
> keeps the derived one - the opposite of the documented "the developer's own
> value always wins". Compare dynamic keys case-insensitively.
> h3. 10. data-msg attribute names are not sanitised
> The name is {{data-msg-}} concatenated with the validator type. FreeMarker's
> HTML auto-escaping protects the value, not the name, and validator types are
> freely named in validators.xml, so a type containing a space or an equals
> sign splits into a second attribute. Config-controlled, so low risk; a
> character-class guard is one line.
> h3. 11. The allowlist admits patterns the browser refuses to compile
> HTML compiles {{pattern}} with the Unicode flag, under which an identity
> escape outside a character class is restricted to syntax characters plus the
> solidus. An escaped hyphen outside a class is a SyntaxError and the whole
> attribute is then ignored, so a pattern that passes
> {{EcmaScriptSafeRegex.isSafe}} can be emitted and silently do nothing. Safe
> direction - no false reject - but the allowlist should be honest and permit
> the hyphen escape only inside a character class. Related and very low:
> {{isSafe}} clears its in-class state at the first unescaped closing bracket,
> so a literal-bracket class is modelled as class-then-literal, matching
> neither engine.
> h3. 12. Six html5 templates can never render constraints
> {{constraints.ftl}} is reachable only through
> {{html5/common-attributes.ftl}}, which combobox, datetextfield, doubleselect,
> updownselect, optiontransferselect and inputtransferselect do not include.
> datetextfield is the notable one given the enum already models the temporal
> control types. Coverage gap rather than a bug; worth recording alongside the
> deliberately deferred temporal min/max formatting.
> h3. 13. Visitor validators are re-resolved once per field
> {{resolveActionValidators()}} caches the top-level list once per form, but
> the visitor branch inside {{findFieldValidators}} calls the validator manager
> once per field, and the manager caches only the config list - it constructs
> fresh validator instances on every call. An N-field form with one visitor
> validator does N full instantiation passes per render.
> {{FormFieldValidatorsTest}} pins the top-level call count only.
> h3. Suggested test additions
> * a form with no action attribute (item 3 has no coverage today)
> * a real action passed through the provider test helper, so the message path
> stops being invisible (item 2)
> * end-to-end template rendering of pattern, and of required on radio and
> file; only minlength is currently asserted through a template
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
