Copilot commented on code in PR #1865:
URL: https://github.com/apache/struts/pull/1865#discussion_r3850064775
##########
core/src/main/java/org/apache/struts2/components/UIBean.java:
##########
@@ -903,6 +917,69 @@ public void evaluateParams() {
}
evaluateExtraParams();
+
+ // must run after evaluateExtraParams(): that is where TextField
resolves attributes.type,
+ // and the control type decides which constraints are legal
+ addConstraintAttributes(form);
+ }
+
+ /**
+ * 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
+ */
+ protected void addConstraintAttributes(Form form) {
+ if (!html5ConstraintsEnabled || form == null || htmlConstraintProvider
== null) {
+ return;
+ }
+ String fieldName = (String) getAttributes().get("name");
+ if (fieldName == null) {
+ return;
+ }
+ try {
+ Map<String, String> constraints =
htmlConstraintProvider.constraintsFor(
+ form.getFieldValidators(fieldName), getControlType(),
stack.peek());
Review Comment:
`stack.peek()` is not necessarily the action: `ModelDrivenInterceptor`
pushes the model on top of the ValueStack. Model-driven forms therefore resolve
validator messages and i18n bundles against the model rather than the action
used by server-side validation. Obtain the action from the current
`ActionInvocation`, passing null when no invocation exists.
##########
core/src/main/java/org/apache/struts2/components/UIBean.java:
##########
@@ -903,6 +917,69 @@ public void evaluateParams() {
}
evaluateExtraParams();
+
+ // must run after evaluateExtraParams(): that is where TextField
resolves attributes.type,
+ // and the control type decides which constraints are legal
+ addConstraintAttributes(form);
+ }
+
+ /**
+ * 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
+ */
+ protected void addConstraintAttributes(Form form) {
+ if (!html5ConstraintsEnabled || form == null || htmlConstraintProvider
== null) {
+ return;
+ }
+ String fieldName = (String) getAttributes().get("name");
+ if (fieldName == null) {
+ return;
+ }
+ 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);
+ }
Review Comment:
Catching the exception does not make this path fail-safe when
`ValidatorSupport.getMessage` throws after pushing the action/validator: its
pops are not in a `finally`, so subsequent rendering continues with a corrupted
ValueStack. Capture the stack depth before derivation and restore it in a
`finally`, or make `ValidatorSupport` guarantee balanced cleanup.
##########
core/src/main/java/org/apache/struts2/components/EcmaScriptSafeRegex.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.components;
+
+/**
+ * Decides whether a Java regular expression can be handed to a browser as an
HTML5 {@code pattern}
+ * attribute without changing meaning.
+ * <p>
+ * This is an allowlist by design. A denylist of Java-only constructs would
violate the
+ * never-false-reject rule the first time it missed one, because a missed
construct becomes a pattern
+ * the browser interprets differently and the user cannot get past. Anything
not provably common to
+ * both engines is rejected, and the field simply gets no client-side check.
+ *
+ * @since 7.4.0
+ */
+public final class EcmaScriptSafeRegex {
+
+ /**
+ * Escapes with identical meaning in both engines.
+ * <p>
+ * {@code \s} and {@code \S} are deliberately absent. Java's {@code \s} is
ASCII-only by default
+ * while ECMAScript's is the wider Unicode set, so {@code ^\S+$} accepts a
value containing NBSP
+ * on the server and rejects it in the browser. {@code \d} and {@code \w}
are safe — both engines
+ * are ASCII-only for those, and JavaScript never widens them.
+ */
+ private static final String ALLOWED_ESCAPES =
"dDwWbBnrtf\\.*+?()[]{}|^$/-";
Review Comment:
Java 17 treats `\b`/`\B` boundaries as Unicode-aware, while the browser's
ECMAScript boundary is based on ASCII word characters. For example, Java 17
accepts `^\bäiti\b$` for `äiti`, but the emitted HTML pattern rejects it,
violating the never-false-reject rule. Remove both escapes from the safe subset
(JDK 19 changed Java's behavior, but Struts still targets Java 17).
##########
core/src/main/java/org/apache/struts2/components/HtmlConstraintProvider.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.components;
+
+import org.apache.struts2.validator.Validator;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Maps a field's validators onto the HTML attributes a theme should render
for it.
+ * <p>
+ * The default implementation is deliberately conservative — see {@link
StrutsHtmlConstraintProvider}.
+ * Applications wanting a best-effort mapping (an {@code email} validator
becoming
+ * {@code type="email"}, say) should register their own implementation instead.
Review Comment:
The advertised `type="email"` replacement cannot work with the current
rendering path. `html5/text.ftl` emits `type="text"` before `constraints.ftl`
renders the provider map, producing a duplicate `type`; HTML keeps the first
value. Either restrict this contract to post-rendered constraint attributes or
merge provider overrides into the normal attribute model before the template
emits `type`.
##########
core/src/main/java/org/apache/struts2/components/UIBean.java:
##########
@@ -903,6 +917,69 @@ public void evaluateParams() {
}
evaluateExtraParams();
+
+ // must run after evaluateExtraParams(): that is where TextField
resolves attributes.type,
+ // and the control type decides which constraints are legal
+ addConstraintAttributes(form);
+ }
+
+ /**
+ * 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
+ */
+ protected void addConstraintAttributes(Form form) {
+ if (!html5ConstraintsEnabled || form == null || htmlConstraintProvider
== null) {
+ return;
+ }
+ String fieldName = (String) getAttributes().get("name");
+ if (fieldName == null) {
+ return;
+ }
+ 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);
}
Review Comment:
HTML attribute names are ASCII case-insensitive, but this exact-key check
misses dynamic attributes such as `MAXLENGTH` or `Min`. Because derived
attributes render before dynamic ones, the browser keeps the derived duplicate
and the developer's value does not win as documented. Compare dynamic keys
case-insensitively.
##########
core/src/main/java/org/apache/struts2/components/HtmlControlType.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.components;
+
+import java.util.EnumSet;
+import java.util.Locale;
+import java.util.Set;
+
+/**
+ * The kind of HTML form control a {@link UIBean} renders, used to decide
which HTML5 constraint
+ * attributes are legal on it.
+ * <p>
+ * This models the <em>control</em> rather than the {@code type} attribute,
because {@code textarea}
+ * and {@code select} have no {@code type} attribute yet still accept {@code
required}.
+ *
+ * @since 7.4.0
+ */
+public enum HtmlControlType {
+
+ TEXT, SEARCH, TEL, PASSWORD, EMAIL, URL,
+ NUMBER, RANGE,
+ DATE, MONTH, WEEK, TIME, DATETIME_LOCAL,
+ CHECKBOX, RADIO, FILE, HIDDEN, SELECT,
Review Comment:
`CHECKBOX` and `HIDDEN` are public control kinds, but the built-in
`Checkbox` and `Hidden` components inherit `OTHER`; only the six new overrides
ever expose concrete types. A replacement provider therefore cannot distinguish
these controls, undermining the swappable mapping policy. Return their actual
control types and keep the conservative behavior in
`StrutsHtmlConstraintProvider`.
##########
core/src/main/java/org/apache/struts2/components/StrutsHtmlConstraintProvider.java:
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+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;
+import org.apache.struts2.validator.validators.RequiredStringValidator;
+import org.apache.struts2.validator.validators.StringLengthFieldValidator;
+
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Default {@link HtmlConstraintProvider}.
+ * <p>
+ * Governed by one rule: never false-reject. A constraint is emitted only when
the browser cannot
+ * reject input the server would accept. In particular this implementation
<em>never sets or changes
+ * an input's {@code type}</em> — switching a field to {@code type="number"}
would reject
+ * {@code 1234,50}, which the framework's locale-aware conversion accepts in a
comma-decimal locale,
+ * and the browsers' {@code email}/{@code url} grammars differ from the
framework's validators.
+ * Range constraints are therefore emitted only on a control the developer
already made numeric.
+ *
+ * @since 7.4.0
+ */
+public class StrutsHtmlConstraintProvider implements HtmlConstraintProvider {
+
+ /**
+ * The HTML5 boolean attribute; its canonical serialisation repeats the
attribute name as the value.
+ */
+ private static final String REQUIRED = "required";
+
+ @Override
+ public Map<String, String> constraintsFor(List<Validator> validators,
HtmlControlType control, Object action) {
+ Map<String, String> attributes = new LinkedHashMap<>();
+ if (validators == null || validators.isEmpty() || control == null) {
+ return attributes;
+ }
+ for (Validator validator : validators) {
+ addConstraints(attributes, validator, control);
+ addMessage(attributes, validator, action);
+ }
+ return attributes;
+ }
+
+ protected void addConstraints(Map<String, String> attributes, Validator
validator, HtmlControlType 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) {
+ addPattern(attributes, regexValidator, control);
+ } else if (validator instanceof DoubleRangeFieldValidator
doubleValidator) {
+ addDoubleRange(attributes, doubleValidator, control);
+ } else if (validator instanceof RangeValidatorSupport<?>
rangeValidator) {
+ addRange(attributes, rangeValidator, control);
+ }
+ }
+
+ /**
+ * {@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);
+ }
+
+ protected void addLength(Map<String, String> attributes,
StringLengthFieldValidator validator, HtmlControlType control) {
+ // with trim=true the server measures the trimmed value, so a
maxlength taken from it would
+ // stop the user typing input the server would have accepted
+ if (!control.supportsLength() || validator.isTrim()) {
+ return;
+ }
+ if (validator.getMinLength() > -1) {
+ attributes.put("minlength",
String.valueOf(validator.getMinLength()));
+ }
+ if (validator.getMaxLength() > -1) {
+ attributes.put("maxlength",
String.valueOf(validator.getMaxLength()));
+ }
+ }
+
+ protected void addPattern(Map<String, String> attributes,
RegexFieldValidator validator, HtmlControlType control) {
+ // HTML pattern accepts no flags, so a case-insensitive rule cannot be
expressed at all
+ 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);
+ }
+ }
+
+ protected void addRange(Map<String, String> attributes,
RangeValidatorSupport<?> validator, HtmlControlType control) {
+ 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.
+ return;
+ }
+ // 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());
+ }
+
+ protected void addDoubleRange(Map<String, String> attributes,
DoubleRangeFieldValidator validator, HtmlControlType control) {
+ 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.
+ return;
+ }
+ // exclusive bounds have no HTML equivalent; omitting them leaves the
browser more
+ // permissive than the server, which is the safe direction
+ Double minInclusive = validator.getMinInclusive();
+ if (isIntegral(minInclusive)) {
+ putIfPresent(attributes, "min", minInclusive);
+ }
+ putIfPresent(attributes, "max", validator.getMaxInclusive());
+ }
+
+ private boolean isNumericRange(HtmlControlType control) {
+ 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);
+ }
Review Comment:
Converting arbitrary `Number` values to `double` can round a fractional
custom bound to an integer. A `BigDecimal("1.0000000000000000001")` passes this
check, then renders that fractional value as `min`, shifts the HTML step base,
and can reject values accepted by the server. Determine integrality from the
decimal representation instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
