This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch feature/WW-5695-html5-constraint-validation in repository https://gitbox.apache.org/repos/asf/struts.git
commit 1245212af23e785b53825a11848a0173258d5aeb Author: Lukasz Lenart <[email protected]> AuthorDate: Tue Aug 25 00:56:57 2026 +0200 WW-5695 fix(components): close false-reject and duplicate-attribute gaps in HTML5 constraints Final whole-branch review found five issues in the constraint-derivation path added for the html5 theme's HTML5 constraint validation: - A fractional min (e.g. minInclusive=6000.1 from a double validator) becomes the HTML step base on type="number"/"range", and with the default step="1" the browser then rejects whole numbers the server accepts. addRange and addDoubleRange now only emit min when the bound is integral; max is unaffected since it does not participate in the step base. - addRequiredField only matched RADIO/FILE, but no component ever returned either control type, making it dead code. Radio and File now override getControlType(), the only two controls where an unselected/empty submission omits the parameter entirely and so agrees with the server. Checkbox and Hidden deliberately stay OTHER: CheckboxInterceptor substitutes "false" for an unticked box, so required there would false-reject. - A derived constraint could duplicate a developer-set attribute (maxlength from a declared tag attribute, min/max from a dynamic one on a numeric textfield), producing invalid markup with the attribute repeated. addConstraintAttributes now drops any derived key already present as a declared or dynamic attribute, except "required" against the declared half: requiredLabel stores an unrelated boolean under that same key to draw a label asterisk, and must never suppress a genuine required constraint. - Form.getFieldValidators reaches AnnotationActionValidatorManager, which dereferences the current ActionInvocation unconditionally - a path that used to need the opt-in validate="true" and now runs for every html5 form. Rendering outside action scope, a null validator, or a broken ${} in a validator message unbalancing the value stack in ValidatorSupport.getMessage would all turn a working page into a 500 for a purely decorative feature. addConstraintAttributes now catches broadly and logs the field name. - Two provider tests could never fail: the CreditCardValidator and EmailValidator exclusion tests returned early at earlier guards (case-sensitivity, then isTrimed()) before ever reaching the exclusion they claimed to cover. Both now set caseSensitive/trim so they actually exercise it. Also adds tests for the integral-min guard, the maxlength duplicate suppression, and pins the data-msg-* escaping against a message containing a quote and an angle bracket. Co-Authored-By: Claude Opus 5 <[email protected]> --- .../java/org/apache/struts2/components/File.java | 5 ++ .../java/org/apache/struts2/components/Radio.java | 5 ++ .../components/StrutsHtmlConstraintProvider.java | 25 +++++++++- .../java/org/apache/struts2/components/UIBean.java | 47 ++++++++++++++++-- .../struts2/components/ConstraintAction.java | 20 ++++++++ .../apache/struts2/components/ControlTypeTest.java | 14 +++++- .../StrutsHtmlConstraintProviderTest.java | 56 +++++++++++++++++++--- .../views/jsp/ui/Html5ConstraintRenderingTest.java | 42 ++++++++++++++++ .../components/ConstraintAction-validation.xml | 12 +++++ 9 files changed, 212 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/components/File.java b/core/src/main/java/org/apache/struts2/components/File.java index e9317afba..58bfe42ce 100644 --- a/core/src/main/java/org/apache/struts2/components/File.java +++ b/core/src/main/java/org/apache/struts2/components/File.java @@ -62,6 +62,11 @@ public class File extends UIBean { return TEMPLATE; } + @Override + protected HtmlControlType getControlType() { + return HtmlControlType.FILE; + } + public void evaluateParams() { super.evaluateParams(); diff --git a/core/src/main/java/org/apache/struts2/components/Radio.java b/core/src/main/java/org/apache/struts2/components/Radio.java index d0d3eb146..bc6bf7606 100644 --- a/core/src/main/java/org/apache/struts2/components/Radio.java +++ b/core/src/main/java/org/apache/struts2/components/Radio.java @@ -74,4 +74,9 @@ public class Radio extends ListUIBean { return true; } + @Override + protected HtmlControlType getControlType() { + return HtmlControlType.RADIO; + } + } diff --git a/core/src/main/java/org/apache/struts2/components/StrutsHtmlConstraintProvider.java b/core/src/main/java/org/apache/struts2/components/StrutsHtmlConstraintProvider.java index 5261927be..124def445 100644 --- a/core/src/main/java/org/apache/struts2/components/StrutsHtmlConstraintProvider.java +++ b/core/src/main/java/org/apache/struts2/components/StrutsHtmlConstraintProvider.java @@ -143,7 +143,12 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { // Deliberately deferred; DateRangeFieldValidator therefore emits nothing for now. return; } - putIfPresent(attributes, "min", validator.getMin()); + // min is guarded by isIntegral; see the comment on that method. The shipped Integer/Short/Long + // range validators always pass it, but a custom RangeValidatorSupport<Double> would not. + Object min = validator.getMin(); + if (isIntegral(min)) { + putIfPresent(attributes, "min", min); + } putIfPresent(attributes, "max", validator.getMax()); } @@ -156,7 +161,10 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { } // exclusive bounds have no HTML equivalent; omitting them leaves the browser more // permissive than the server, which is the safe direction - putIfPresent(attributes, "min", validator.getMinInclusive()); + Double minInclusive = validator.getMinInclusive(); + if (isIntegral(minInclusive)) { + putIfPresent(attributes, "min", minInclusive); + } putIfPresent(attributes, "max", validator.getMaxInclusive()); } @@ -164,6 +172,19 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { return control.supportsRange() && (control == HtmlControlType.NUMBER || control == HtmlControlType.RANGE); } + /** + * A fractional {@code min} moves the HTML step base off zero, and with the default {@code step="1"} + * the browser then rejects whole numbers the server accepts. {@code max} does not participate in the + * step base, so only {@code min} needs this guard. + */ + private boolean isIntegral(Object value) { + if (!(value instanceof java.lang.Number number)) { + return false; + } + double asDouble = number.doubleValue(); + return !Double.isNaN(asDouble) && !Double.isInfinite(asDouble) && asDouble == Math.floor(asDouble); + } + protected void addMessage(Map<String, String> attributes, Validator validator, Object action) { if (action == null) { return; diff --git a/core/src/main/java/org/apache/struts2/components/UIBean.java b/core/src/main/java/org/apache/struts2/components/UIBean.java index b5c2d6e2a..6e73392e8 100644 --- a/core/src/main/java/org/apache/struts2/components/UIBean.java +++ b/core/src/main/java/org/apache/struts2/components/UIBean.java @@ -925,6 +925,17 @@ public abstract class UIBean extends Component { /** * Derives HTML5 constraint attributes for this field from the action's validators. + * <p> + * This reaches {@link Form#getFieldValidators(String)}, which resolves the action's validators via + * {@code AnnotationActionValidatorManager}, which in turn dereferences the current + * {@code ActionInvocation} unconditionally. Before this feature that path only ran under the opt-in + * {@code validate="true"}; with constraint derivation gated only by + * {@code struts.ui.html5.constraints}, every {@code html5}-themed form now runs it, including one + * rendered outside action scope (a direct JSP include from a plain servlet, say) — which would NPE. + * A stray {@code null} in the validator list, and a broken {@code ${}} in a validator message + * unbalancing the value stack in {@code ValidatorSupport.getMessage}, land in the same call. This + * feature is purely decorative — a missing constraint attribute costs nothing, a 500 costs the page — + * so the broad catch here is deliberate rather than a mistake. * * @since 7.4.0 */ @@ -936,11 +947,39 @@ public abstract class UIBean extends Component { if (fieldName == null) { return; } - Map<String, String> constraints = htmlConstraintProvider.constraintsFor( - form.getFieldValidators(fieldName), getControlType(), stack.peek()); - if (!constraints.isEmpty()) { - addParameter("constraints", constraints); + try { + Map<String, String> constraints = htmlConstraintProvider.constraintsFor( + form.getFieldValidators(fieldName), getControlType(), stack.peek()); + if (constraints.isEmpty()) { + return; + } + constraints = new LinkedHashMap<>(constraints); + constraints.keySet().removeIf(this::isAlreadyRendered); + if (!constraints.isEmpty()) { + addParameter("constraints", constraints); + } + } catch (Exception e) { + LOG.warn("Failed to derive HTML5 constraint attributes for field [{}], skipping", fieldName, e); + } + } + + /** + * True when the developer already supplied this attribute explicitly — as a declared tag attribute + * (e.g. {@code maxlength}) or a dynamic one (e.g. {@code min} on a numeric textfield, which is not a + * declared attribute of any component) — so a derived constraint of the same name must not be + * rendered a second time. The developer's own value always wins. + * <p> + * {@code required} is deliberately excluded from the declared-attribute half of this check: + * {@code requiredLabel} stores an unrelated boolean under the same {@code attributes.required} key, + * purely to draw a label asterisk in the xhtml theme, and that must never suppress a genuine + * {@code required} constraint derived from a {@code required}/{@code requiredstring} validator. A + * {@code required} attribute the developer typed by hand as a dynamic attribute still wins. + */ + private boolean isAlreadyRendered(String attributeName) { + if (dynamicAttributes.containsKey(attributeName)) { + return true; } + return !"required".equals(attributeName) && getAttributes().containsKey(attributeName); } /** diff --git a/core/src/test/java/org/apache/struts2/components/ConstraintAction.java b/core/src/test/java/org/apache/struts2/components/ConstraintAction.java index ce02d21f8..2a1f9b4f3 100644 --- a/core/src/test/java/org/apache/struts2/components/ConstraintAction.java +++ b/core/src/test/java/org/apache/struts2/components/ConstraintAction.java @@ -24,6 +24,8 @@ import org.apache.struts2.interceptor.parameter.StrutsParameter; public class ConstraintAction extends ActionSupport { private String username; + private String comment; + private String bio; public String getUsername() { return username; @@ -33,4 +35,22 @@ public class ConstraintAction extends ActionSupport { public void setUsername(String username) { this.username = username; } + + public String getComment() { + return comment; + } + + @StrutsParameter + public void setComment(String comment) { + this.comment = comment; + } + + public String getBio() { + return bio; + } + + @StrutsParameter + public void setBio(String bio) { + this.bio = bio; + } } diff --git a/core/src/test/java/org/apache/struts2/components/ControlTypeTest.java b/core/src/test/java/org/apache/struts2/components/ControlTypeTest.java index d0d0d82ab..c43cfe6b0 100644 --- a/core/src/test/java/org/apache/struts2/components/ControlTypeTest.java +++ b/core/src/test/java/org/apache/struts2/components/ControlTypeTest.java @@ -54,9 +54,21 @@ public class ControlTypeTest extends AbstractUITagTest { assertEquals(HtmlControlType.SELECT, select.getControlType()); } + public void testRadioIsRadio() { + Radio radio = new Radio(stack, request, response); + assertEquals(HtmlControlType.RADIO, radio.getControlType()); + } + + public void testFileIsFile() { + File file = new File(stack, request, response); + assertEquals(HtmlControlType.FILE, file.getControlType()); + } + public void testControlsWithoutAnOverrideAreUnknown() { + // CheckboxInterceptor substitutes "false" for an unticked box, so the server accepts what + // a browser "required" would block — that is a real false reject, and the reason Checkbox + // and Hidden deliberately have no getControlType() override. assertEquals(HtmlControlType.OTHER, new Checkbox(stack, request, response).getControlType()); assertEquals(HtmlControlType.OTHER, new Hidden(stack, request, response).getControlType()); - assertEquals(HtmlControlType.OTHER, new File(stack, request, response).getControlType()); } } diff --git a/core/src/test/java/org/apache/struts2/components/StrutsHtmlConstraintProviderTest.java b/core/src/test/java/org/apache/struts2/components/StrutsHtmlConstraintProviderTest.java index 00eeb208c..76eade5a4 100644 --- a/core/src/test/java/org/apache/struts2/components/StrutsHtmlConstraintProviderTest.java +++ b/core/src/test/java/org/apache/struts2/components/StrutsHtmlConstraintProviderTest.java @@ -194,8 +194,12 @@ public class StrutsHtmlConstraintProviderTest { @Test public void creditCardValidatorNeverContributesAPatternConstraint() { // CreditCardValidator strips all whitespace before matching, so its regex cannot be - // expressed as a browser pattern without also stripping whitespace client-side + // expressed as a browser pattern without also stripping whitespace client-side. + // caseSensitive and trim are set explicitly here so this test actually reaches the + // EmailValidator/CreditCardValidator exclusion in addPattern, rather than returning + // earlier at the case-sensitivity guard (the constructor defaults caseSensitive to false). CreditCardValidator validator = new CreditCardValidator(); + validator.setCaseSensitive(true); validator.setTrim(false); assertThat(constraints(validator, HtmlControlType.TEXT)).isEmpty(); @@ -215,16 +219,45 @@ public class StrutsHtmlConstraintProviderTest { @Test public void doubleRangeEmitsInclusiveBoundsOnlyOnANumericControl() { + // integral bounds here, deliberately: a fractional min is covered separately by + // doubleRangeOmitsMinWhenItIsFractionalBecauseItWouldShiftTheStepBase, since it must NOT + // emit min at all (it would shift the HTML step base off zero) DoubleRangeFieldValidator validator = new DoubleRangeFieldValidator(); - validator.setMinInclusive(6000.1); + validator.setMinInclusive(6000.0); validator.setMaxInclusive(10000.1); assertThat(constraints(validator, HtmlControlType.NUMBER)) - .containsEntry("min", "6000.1") + .containsEntry("min", "6000.0") .containsEntry("max", "10000.1"); assertThat(constraints(validator, HtmlControlType.TEXT)).isEmpty(); } + @Test + public void doubleRangeOmitsMinWhenItIsFractionalBecauseItWouldShiftTheStepBase() { + // min becomes the HTML step base, and the default step is 1: min="6000.1" would make the + // browser reject 6002, which DoubleRangeFieldValidator accepts server-side. max does not + // participate in the step base, so it is unaffected. + DoubleRangeFieldValidator validator = new DoubleRangeFieldValidator(); + validator.setMinInclusive(6000.1); + validator.setMaxInclusive(10000.1); + + Map<String, String> result = constraints(validator, HtmlControlType.NUMBER); + + assertThat(result).containsEntry("max", "10000.1"); + assertThat(result).doesNotContainKey("min"); + } + + @Test + public void doubleRangeEmitsMinWhenItIsIntegral() { + DoubleRangeFieldValidator validator = new DoubleRangeFieldValidator(); + validator.setMinInclusive(6000.0); + validator.setMaxInclusive(10000.0); + + assertThat(constraints(validator, HtmlControlType.NUMBER)) + .containsEntry("min", "6000.0") + .containsEntry("max", "10000.0"); + } + @Test public void dateRangeEmitsNothingBecauseTemporalFormattingIsDeferred() { DateRangeFieldValidator validator = new DateRangeFieldValidator(); @@ -237,10 +270,19 @@ public class StrutsHtmlConstraintProviderTest { @Test public void emailValidatorNeverContributesAConstraint() { // the browser's email grammar differs from EmailValidator's, so honouring it could reject - // an address the server accepts. Guaranteed explicitly by addPattern's EmailValidator - // exclusion now, not incidentally by its constructor setting caseSensitive=false. - assertThat(constraints(new EmailValidator(), HtmlControlType.TEXT)).isEmpty(); - assertThat(constraints(new EmailValidator(), HtmlControlType.EMAIL)).isEmpty(); + // an address the server accepts. caseSensitive and trim are set explicitly here so this + // test actually reaches addPattern's EmailValidator exclusion, rather than returning + // earlier at the case-sensitivity guard or the isTrimed() guard (the constructor defaults + // caseSensitive to false, and trim defaults to true). + EmailValidator textControlValidator = new EmailValidator(); + textControlValidator.setCaseSensitive(true); + textControlValidator.setTrim(false); + assertThat(constraints(textControlValidator, HtmlControlType.TEXT)).isEmpty(); + + EmailValidator emailControlValidator = new EmailValidator(); + emailControlValidator.setCaseSensitive(true); + emailControlValidator.setTrim(false); + assertThat(constraints(emailControlValidator, HtmlControlType.EMAIL)).isEmpty(); } @Test diff --git a/core/src/test/java/org/apache/struts2/views/jsp/ui/Html5ConstraintRenderingTest.java b/core/src/test/java/org/apache/struts2/views/jsp/ui/Html5ConstraintRenderingTest.java index d8139363f..c5979e36d 100644 --- a/core/src/test/java/org/apache/struts2/views/jsp/ui/Html5ConstraintRenderingTest.java +++ b/core/src/test/java/org/apache/struts2/views/jsp/ui/Html5ConstraintRenderingTest.java @@ -54,11 +54,50 @@ public class Html5ConstraintRenderingTest extends AbstractUITagTest { output.contains("required=\"required\"")); } + /** + * text.ftl renders {@code attributes.maxlength} (the developer's own tag attribute) before + * including common-attributes.ftl, which renders the derived {@code attributes.constraints} map. + * Without suppressing the derived duplicate, a stringlength validator on this field would render + * {@code maxlength} twice: once from the tag attribute, once from the constraint. + */ + public void testDeveloperSetMaxlengthSuppressesTheDerivedOne() throws Exception { + String output = renderWithMaxlength("bio", "20"); + + int firstIndex = output.indexOf("maxlength="); + assertTrue("expected a maxlength attribute in: " + output, firstIndex >= 0); + assertEquals("expected exactly one maxlength attribute in: " + output, + firstIndex, output.lastIndexOf("maxlength=")); + assertTrue("expected the developer's own value to win: " + output, + output.contains("maxlength=\"20\"")); + } + + /** + * data-msg-* values pass through TextParseUtil.translateVariables and can carry user-submitted + * content into an HTML attribute. Escaping is applied by FreemarkerManager's HTMLOutputFormat + * configuration, not by the template, so this pins it against regression. + */ + public void testDataMsgAttributesAreHtmlEscaped() throws Exception { + String output = render("true", "comment", null); + + assertTrue("expected the escaped message in: " + output, + output.contains("data-msg-requiredstring=\"Contains "quotes" and <brackets>\"")); + assertFalse("the raw, unescaped message must never appear in: " + output, + output.contains("Contains \"quotes\" and <brackets>")); + } + private String render(String constraintsEnabled) throws Exception { return render(constraintsEnabled, "username", null); } private String render(String constraintsEnabled, String fieldName, String requiredLabel) throws Exception { + return render(constraintsEnabled, fieldName, requiredLabel, null); + } + + private String renderWithMaxlength(String fieldName, String maxlength) throws Exception { + return render("true", fieldName, null, maxlength); + } + + private String render(String constraintsEnabled, String fieldName, String requiredLabel, String maxlength) throws Exception { initDispatcher(new HashMap<String, String>() {{ put("configProviders", TestConfigurationProvider.class.getName()); put(StrutsConstants.STRUTS_UI_HTML5_CONSTRAINTS, constraintsEnabled); @@ -80,6 +119,9 @@ public class Html5ConstraintRenderingTest extends AbstractUITagTest { if (requiredLabel != null) { field.setRequiredLabel(requiredLabel); } + if (maxlength != null) { + field.setMaxlength(maxlength); + } field.doStartTag(); field.doEndTag(); form.doEndTag(); diff --git a/core/src/test/resources/org/apache/struts2/components/ConstraintAction-validation.xml b/core/src/test/resources/org/apache/struts2/components/ConstraintAction-validation.xml index 4301d01bc..30bf5dd43 100644 --- a/core/src/test/resources/org/apache/struts2/components/ConstraintAction-validation.xml +++ b/core/src/test/resources/org/apache/struts2/components/ConstraintAction-validation.xml @@ -28,4 +28,16 @@ <message>username must be at least ${minLength} characters</message> </field-validator> </field> + <field name="comment"> + <field-validator type="requiredstring"> + <message>Contains "quotes" and <brackets></message> + </field-validator> + </field> + <field name="bio"> + <field-validator type="stringlength"> + <param name="trim">false</param> + <param name="maxLength">10</param> + <message>bio must be at most ${maxLength} characters</message> + </field-validator> + </field> </validators>
