[
https://issues.apache.org/jira/browse/WW-5702?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart reassigned WW-5702:
---------------------------------
Assignee: Lukasz Lenart
> 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
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
>
> 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)
