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 fcdc518e5f40749a34299850fba2f36ef2c75b6e Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Aug 24 20:46:48 2026 +0200 WW-5695 fix(components): close three false-reject and injection gaps in HtmlConstraintProvider Review found two Critical defects inherited from the task brief and three Important gaps: - addPattern ignored RegexFieldValidator's trim=true default: the server matches the trimmed value while HTML pattern matches the raw one, so "abc " would pass server-side and be blocked client-side. Now guarded on isTrimed(), and EmailValidator/CreditCardValidator (whose matching diverges from their raw regex) are excluded outright. - required was emitted wherever RequiredFieldValidator's null-or-empty check is looser than the browser's required: an empty text input, a select with an empty header option, and an unticked checkbox (CheckboxInterceptor substitutes "false") all pass server-side but would be blocked client-side. Split into addRequiredString (safe on any text-entry control, since requiredstring rejects blank too) and addRequiredField (safe only on RADIO and FILE, the only controls that omit the parameter entirely when empty). - The bean was registered in struts-beans.xml but never aliased to its container-default name, so an @Inject HtmlConstraintProvider would not resolve. Added STRUTS_HTML_CONSTRAINT_PROVIDER to StrutsConstants, its default.properties entry, and the alias() call in StrutsBeanSelectionProvider, following the UrlRenderer model. - Added regression coverage: the temporal-range early return, the new trim/EmailValidator/CreditCardValidator pattern exclusions, and the -1 length sentinel, none of which had a covering test before. Co-Authored-By: Claude Opus 5 <[email protected]> --- .../java/org/apache/struts2/StrutsConstants.java | 8 ++ .../components/StrutsHtmlConstraintProvider.java | 55 ++++++++++--- .../config/StrutsBeanSelectionProvider.java | 2 + .../org/apache/struts2/default.properties | 3 + .../StrutsHtmlConstraintProviderTest.java | 96 ++++++++++++++++++++-- 5 files changed, 146 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 9c1245248..3276ca7c2 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -218,6 +218,14 @@ public final class StrutsConstants { */ public static final String STRUTS_UI_ESCAPE_HTML_BODY = "struts.ui.escapeHtmlBody"; + /** + * The {@link org.apache.struts2.components.HtmlConstraintProvider} implementation used to derive + * HTML5 constraint attributes from an action's validators. + * + * @since 7.4.0 + */ + public static final String STRUTS_HTML_CONSTRAINT_PROVIDER = "struts.htmlConstraintProvider"; + /** * The maximum size of a multipart request (file upload) */ 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 c4f5e151e..5261927be 100644 --- a/core/src/main/java/org/apache/struts2/components/StrutsHtmlConstraintProvider.java +++ b/core/src/main/java/org/apache/struts2/components/StrutsHtmlConstraintProvider.java @@ -19,7 +19,9 @@ package org.apache.struts2.components; import org.apache.struts2.validator.Validator; +import org.apache.struts2.validator.validators.CreditCardValidator; import org.apache.struts2.validator.validators.DoubleRangeFieldValidator; +import org.apache.struts2.validator.validators.EmailValidator; import org.apache.struts2.validator.validators.RangeValidatorSupport; import org.apache.struts2.validator.validators.RegexFieldValidator; import org.apache.struts2.validator.validators.RequiredFieldValidator; @@ -58,8 +60,10 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { } protected void addConstraints(Map<String, String> attributes, Validator validator, HtmlControlType control) { - if (validator instanceof RequiredFieldValidator || validator instanceof RequiredStringValidator) { - addRequired(attributes, control); + if (validator instanceof RequiredStringValidator) { + addRequiredString(attributes, control); + } else if (validator instanceof RequiredFieldValidator) { + addRequiredField(attributes, control); } else if (validator instanceof StringLengthFieldValidator lengthValidator) { addLength(attributes, lengthValidator, control); } else if (validator instanceof RegexFieldValidator regexValidator) { @@ -71,8 +75,26 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { } } - protected void addRequired(Map<String, String> attributes, HtmlControlType control) { - if (control == HtmlControlType.OTHER) { + /** + * {@code requiredstring} fails on null, empty and (by default) blank, so the browser's + * {@code required} can only reject what the server would also reject. Safe on any text-entry control. + */ + protected void addRequiredString(Map<String, String> attributes, HtmlControlType control) { + if (!control.supportsLength()) { + return; + } + attributes.put("required", "required"); + } + + /** + * {@code required} fails only on null, an empty array or an empty collection. A control that submits + * an empty string rather than omitting the parameter therefore passes server-side while the browser + * blocks it — an empty text input, a select with an empty-valued header option, and an unticked + * checkbox (CheckboxInterceptor substitutes "false") are all in that group. Only RADIO and FILE omit + * the parameter entirely when empty, so only they agree with the browser. + */ + protected void addRequiredField(Map<String, String> attributes, HtmlControlType control) { + if (control != HtmlControlType.RADIO && control != HtmlControlType.FILE) { return; } attributes.put("required", "required"); @@ -97,6 +119,17 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { if (!control.supportsPattern() || !validator.isCaseSensitive()) { return; } + // trim defaults to true, and the server matches the trimmed value while pattern matches the + // raw one: "[a-z]+" would accept "abc " server-side and be blocked by the browser + if (validator.isTrimed()) { + return; + } + // Both extend RegexFieldValidator but do not match their regex against the raw value: + // CreditCardValidator strips all whitespace first, and both carry grammars the browser + // does not share. Neither is expressible as a pattern. + if (validator instanceof EmailValidator || validator instanceof CreditCardValidator) { + return; + } String regex = validator.getRegex(); if (EcmaScriptSafeRegex.isSafe(regex)) { attributes.put("pattern", regex); @@ -104,10 +137,7 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { } protected void addRange(Map<String, String> attributes, RangeValidatorSupport<?> validator, HtmlControlType control) { - if (!control.supportsRange()) { - return; - } - if (control != HtmlControlType.NUMBER && control != HtmlControlType.RANGE) { + if (!isNumericRange(control)) { // Temporal controls support ranges too, but min/max there need per-control ISO // formatting (date -> yyyy-MM-dd, month -> yyyy-MM, week -> yyyy-'W'ww, time -> HH:mm). // Deliberately deferred; DateRangeFieldValidator therefore emits nothing for now. @@ -118,10 +148,7 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { } protected void addDoubleRange(Map<String, String> attributes, DoubleRangeFieldValidator validator, HtmlControlType control) { - if (!control.supportsRange()) { - return; - } - if (control != HtmlControlType.NUMBER && control != HtmlControlType.RANGE) { + if (!isNumericRange(control)) { // Temporal controls support ranges too, but min/max there need per-control ISO // formatting (date -> yyyy-MM-dd, month -> yyyy-MM, week -> yyyy-'W'ww, time -> HH:mm). // Deliberately deferred; DateRangeFieldValidator therefore emits nothing for now. @@ -133,6 +160,10 @@ public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider { putIfPresent(attributes, "max", validator.getMaxInclusive()); } + private boolean isNumericRange(HtmlControlType control) { + return control.supportsRange() && (control == HtmlControlType.NUMBER || control == HtmlControlType.RANGE); + } + 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/config/StrutsBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java index f37cb4752..b0dda639b 100644 --- a/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java @@ -29,6 +29,7 @@ import org.apache.struts2.StrutsConstants; import org.apache.struts2.text.TextProvider; import org.apache.struts2.text.TextProviderFactory; import org.apache.struts2.UnknownHandlerManager; +import org.apache.struts2.components.HtmlConstraintProvider; import org.apache.struts2.components.UrlRenderer; import org.apache.struts2.components.date.DateFormatter; import org.apache.struts2.conversion.ConversionAnnotationProcessor; @@ -424,6 +425,7 @@ public class StrutsBeanSelectionProvider extends AbstractBeanSelectionProvider { alias(MultiPartRequest.class, StrutsConstants.STRUTS_MULTIPART_PARSER, builder, props, Scope.PROTOTYPE); alias(FreemarkerManager.class, StrutsConstants.STRUTS_FREEMARKER_MANAGER_CLASSNAME, builder, props); alias(UrlRenderer.class, StrutsConstants.STRUTS_URL_RENDERER, builder, props); + alias(HtmlConstraintProvider.class, StrutsConstants.STRUTS_HTML_CONSTRAINT_PROVIDER, builder, props); alias(ActionValidatorManager.class, StrutsConstants.STRUTS_ACTIONVALIDATORMANAGER, builder, props); alias(ValueStackFactory.class, StrutsConstants.STRUTS_VALUESTACKFACTORY, builder, props); alias(ReflectionProvider.class, StrutsConstants.STRUTS_REFLECTIONPROVIDER, builder, props); diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index a14c84bcc..d4f5fccf8 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -176,6 +176,9 @@ struts.ui.templateSuffix=ftl ### and this take precedence over the global flag # struts.ui.escapeHtmlBody=true +### The HtmlConstraintProvider implementation used to derive HTML5 constraint attributes +struts.htmlConstraintProvider=struts + ### Configuration reloading ### This will cause the configuration to reload struts.xml when it is changed # struts.configuration.xml.reload=false 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 f6d965c2b..00eeb208c 100644 --- a/core/src/test/java/org/apache/struts2/components/StrutsHtmlConstraintProviderTest.java +++ b/core/src/test/java/org/apache/struts2/components/StrutsHtmlConstraintProviderTest.java @@ -20,6 +20,8 @@ package org.apache.struts2.components; import org.apache.struts2.ActionSupport; import org.apache.struts2.validator.Validator; +import org.apache.struts2.validator.validators.CreditCardValidator; +import org.apache.struts2.validator.validators.DateRangeFieldValidator; import org.apache.struts2.validator.validators.DoubleRangeFieldValidator; import org.apache.struts2.validator.validators.EmailValidator; import org.apache.struts2.validator.validators.IntRangeFieldValidator; @@ -30,6 +32,7 @@ import org.apache.struts2.validator.validators.StringLengthFieldValidator; import org.junit.Before; import org.junit.Test; +import java.util.Date; import java.util.List; import java.util.Map; @@ -54,14 +57,48 @@ public class StrutsHtmlConstraintProviderTest { } @Test - public void requiredValidatorEmitsRequired() { - assertThat(constraints(new RequiredFieldValidator(), HtmlControlType.TEXT)) + public void requiredStringEmitsRequiredEvenThoughServerIsStricter() { + assertThat(constraints(new RequiredStringValidator(), HtmlControlType.TEXT)) .containsEntry("required", "required"); } @Test - public void requiredStringEmitsRequiredEvenThoughServerIsStricter() { - assertThat(constraints(new RequiredStringValidator(), HtmlControlType.TEXT)) + public void requiredStringEmitsRequiredOnTextarea() { + assertThat(constraints(new RequiredStringValidator(), HtmlControlType.TEXTAREA)) + .containsEntry("required", "required"); + } + + @Test + public void requiredFieldEmitsNothingOnATextControlBecauseEmptyStringWouldPassServerSide() { + // an empty text input submits name="", which RequiredFieldValidator accepts (it only + // rejects null / empty array / empty collection) — required here would false-reject + assertThat(constraints(new RequiredFieldValidator(), HtmlControlType.TEXT)).isEmpty(); + } + + @Test + public void requiredFieldEmitsNothingOnACheckboxBecauseUncheckedSubstitutesFalse() { + // CheckboxInterceptor substitutes "false" for an unticked box, so the field is never + // null server-side and an unticked required checkbox would still pass validation + assertThat(constraints(new RequiredFieldValidator(), HtmlControlType.CHECKBOX)).isEmpty(); + } + + @Test + public void requiredFieldEmitsNothingOnASelectBecauseAnEmptyOptionWouldPassServerSide() { + // a select with an empty-valued header option submits "", which passes server-side + assertThat(constraints(new RequiredFieldValidator(), HtmlControlType.SELECT)).isEmpty(); + } + + @Test + public void requiredFieldEmitsRequiredOnRadioBecauseNoSelectionOmitsTheParameter() { + // an unselected radio group omits the parameter entirely, agreeing with the server + assertThat(constraints(new RequiredFieldValidator(), HtmlControlType.RADIO)) + .containsEntry("required", "required"); + } + + @Test + public void requiredFieldEmitsRequiredOnFileBecauseNoSelectionOmitsTheParameter() { + // an empty file input omits the parameter entirely, agreeing with the server + assertThat(constraints(new RequiredFieldValidator(), HtmlControlType.FILE)) .containsEntry("required", "required"); } @@ -98,11 +135,25 @@ public class StrutsHtmlConstraintProviderTest { assertThat(constraints(validator, HtmlControlType.NUMBER)).isEmpty(); } + @Test + public void stringLengthOmitsMinlengthWhenOnlyMaxLengthIsSet() { + StringLengthFieldValidator validator = new StringLengthFieldValidator(); + validator.setTrim(false); + validator.setMaxLength(10); + + Map<String, String> result = constraints(validator, HtmlControlType.TEXT); + + // minLength defaults to the -1 sentinel (unset), which must not become "minlength=-1" + assertThat(result).containsEntry("maxlength", "10"); + assertThat(result).doesNotContainKey("minlength"); + } + @Test public void regexEmitsPatternWhenPortableAndCaseSensitive() { RegexFieldValidator validator = new RegexFieldValidator(); validator.setRegex("[a-z]+"); validator.setCaseSensitive(true); + validator.setTrim(false); assertThat(constraints(validator, HtmlControlType.TEXT)) .containsEntry("pattern", "[a-z]+"); @@ -113,6 +164,7 @@ public class StrutsHtmlConstraintProviderTest { RegexFieldValidator validator = new RegexFieldValidator(); validator.setRegex("[a-z]+"); validator.setCaseSensitive(false); + validator.setTrim(false); // HTML pattern accepts no flags, so a case-insensitive rule cannot be expressed assertThat(constraints(validator, HtmlControlType.TEXT)).isEmpty(); @@ -123,6 +175,28 @@ public class StrutsHtmlConstraintProviderTest { RegexFieldValidator validator = new RegexFieldValidator(); validator.setRegex("\\p{Alpha}+"); validator.setCaseSensitive(true); + validator.setTrim(false); + + assertThat(constraints(validator, HtmlControlType.TEXT)).isEmpty(); + } + + @Test + public void regexEmitsNothingWhenTrimming() { + // trim defaults to true: the server matches the trimmed value while pattern matches the + // raw one, so "abc " would pass server-side and be blocked by the browser + RegexFieldValidator validator = new RegexFieldValidator(); + validator.setRegex("[a-z]+"); + validator.setCaseSensitive(true); + + assertThat(constraints(validator, HtmlControlType.TEXT)).isEmpty(); + } + + @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 + CreditCardValidator validator = new CreditCardValidator(); + validator.setTrim(false); assertThat(constraints(validator, HtmlControlType.TEXT)).isEmpty(); } @@ -151,10 +225,20 @@ public class StrutsHtmlConstraintProviderTest { assertThat(constraints(validator, HtmlControlType.TEXT)).isEmpty(); } + @Test + public void dateRangeEmitsNothingBecauseTemporalFormattingIsDeferred() { + DateRangeFieldValidator validator = new DateRangeFieldValidator(); + validator.setMin(new Date(0)); + validator.setMax(new Date(1_000_000L)); + + assertThat(constraints(validator, HtmlControlType.DATE)).isEmpty(); + } + @Test public void emailValidatorNeverContributesAConstraint() { - // the browser's email grammar differs from EmailValidator's, so honouring it - // could reject an address the server accepts + // 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(); }
