mirror of
https://github.com/apache/struts.git
synced 2026-09-14 09:55:11 +00:00
Lukasz LenartandClaude Opus 5
c752860e32
* WW-5695 docs(validation): design HTML5 constraint validation and retire the JS validator Design covering the replacement of the generated JavaScript client-side validator with native HTML5 constraint attributes derived from the action's validators. Governing rule is never to false-reject: a constraint is emitted only when the browser cannot reject input the server would accept. That rules out changing an input's type, since type="number" refuses "1234,50" which locale-aware conversion accepts, and browser email/url regexes diverge from the framework's. Struts only adds constraints safe for the type already present. Also records why WW-2975 is superseded rather than fixed: constraints riding on each input remove the central tagNames list its root cause depends on. Covers WW-5694 (deprecate, 7.4.0), WW-5695 (constraints, 7.4.0) and WW-5696 (remove, 8.0.0). The implementation plan drawn from this covers 7.4.0 only. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 docs(validation): correct the UIBean hook point and control-type resolution Reading the code turned up two errors in the approved design. evaluateExtraParams() is the last statement of evaluateParams(), and that is where TextField sets attributes.type. The hook cannot sit next to the tagNames block as written, because at that point no text field has a resolved type and every one of them would look like OTHER. It moves to the end of the method. attributes.type is set by TextField and nothing else on the input path, so the control type cannot come from the attribute map alone. Adds a getControlType() component hook with four overrides; Checkbox, Radio, File and Hidden fall through to OTHER, which emits nothing and is correct for all four. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 docs(validation): add the 7.4.0 implementation plan Nine tasks covering WW-5694 (deprecation, task 1, separately mergeable) and WW-5695 (tasks 2-9). WW-5696's 8.0.0 removal is out of scope. Syncs three points where writing the code diverged from the design: - constraintsFor takes the action as a third parameter; getMessage needs it to resolve the i18n text behind the data-msg-* attributes. - The validator list is memoised on Form component fields rather than the attributes map, which is exposed to templates. - Temporal min/max is deferred. DateRangeFieldValidator emits nothing for now; honouring it needs per-control ISO formatting worth doing deliberately. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5694 refactor(validation): deprecate JavaScript client-side validation Marks the form tag's validate attribute and the machinery behind it for removal in 8.0.0. Annotations and documentation only; no behaviour change. Follows the WW-5510 precedent: annotate both the component and the JSP tag, and put the notice inside the START SNIPPET block so the website picks it up. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5694 docs(validation): scope the deprecation banner to the validate attribute The javadoc SNIPPET block published on struts.apache.org read as a blanket deprecation of <s:form> itself, with no replacement mentioned. Reword and move the banner to the end of the javadoc snippet so it explicitly scopes the deprecation to the validate attribute and the client-side JS behind it; the form tag itself is not deprecated. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 feat(components): add HtmlControlType Models the kind of form control a UIBean renders, so constraint derivation can ask which HTML5 attributes are legal rather than string-matching a type attribute. Models the control, not the attribute, because textarea and select have no type yet still accept required. from() never throws; unknown input becomes OTHER, which supports nothing. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 docs(validation): require the licence header on every new file in the plan The plan's code blocks omit the ASF header for brevity, and a task reviewer caught an implementer transcribing that omission into a real test file. apache-rat-plugin:check runs at prepare-package, so mvn test -DskipAssembly — the command every task in the plan uses to verify itself — never runs the licence check. A missing header passes every task-level gate and fails CI. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 chore(components): add the missing licence header to HtmlControlTypeTest The task brief's code block omitted the ASF licence header; every sibling test in this package carries it, and apache-rat:check (bound to prepare-package) would fail on it in a full build. mvn test -DskipAssembly never reaches that phase, so the earlier green run was blind to it. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 feat(components): add ECMAScript-safe regex detection Decides whether a Java regex can become an HTML5 pattern attribute without changing meaning. Allowlist by design: a denylist would violate the never-false-reject rule the first time it missed a construct, and a regex the browser reads differently is a rejection the user cannot get past. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 fix(validation): drop \s and \S from the ECMAScript-safe escape allowlist A task reviewer found a real semantic divergence the design had certified as portable. Java's \s is ASCII-only by default; ECMAScript's is always the wider Unicode set. For a rule as ordinary as ^\S+$, a value containing NBSP satisfies Java's \S and fails the browser's — so the server accepts input the browser silently refuses to submit, which is exactly the failure this allowlist exists to prevent. \d and \w are safe: both engines are ASCII-only for those by default and JavaScript never widens them, so the fix is scoped to s/S. The spec listed \s among the safe escapes and the plan's ALLOWED_ESCAPES transcribed it. Both corrected, plus a regression test. The governing rule outranks its own example list. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 fix(components): drop \s and \S from the portable-escape allowlist Java's \s is ASCII-only by default; ECMAScript's is the wider Unicode whitespace set (NBSP and friends). ^\S+$ therefore accepted a value containing NBSP on the server while the browser's pattern attribute rejected it silently - the exact false-true failure this class exists to prevent. \d and \w stay allowed: both engines are ASCII-only for those and JavaScript never widens them. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 feat(components): map validators onto HTML5 constraint attributes Adds HtmlConstraintProvider and its conservative default implementation, which never sets or changes an input's type: type=number would reject 1234,50 that locale-aware conversion accepts, and browser email/url grammars differ from the framework's validators. Range constraints therefore land only on a control the developer already made numeric. Registered as a swappable bean so applications wanting a best-effort mapping can replace the policy. Co-Authored-By: Claude Opus 5 <[email protected]> * 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]> * WW-5695 feat(components): add Form.getFieldValidators with per-render caching getValidators re-runs the action-mapping lookup and the validator-manager resolution on every call, so a twenty-field form would do twenty of them. The new method resolves once per form render and filters per field. Memoised on component fields rather than the attributes map: a Form component is built per render, and the attributes map is exposed to templates. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 docs(validation): make the plan's memoisation test actually test memoisation A task reviewer found the plan's own testRepeatedCallsAreConsistent vacuous: resolution is deterministic, so comparing result sizes across two calls passes identically against an implementation with no cache at all. Memoisation is the entire purpose of getFieldValidators, so nothing would have caught it silently regressing to a full resolution per field. Replaced with a test that counts resolutions across two different field names, and recorded the second half of the harness trap: createMocks() never sets a config on the MockActionProxy, so the validator manager NPEs without an explicit setConfig. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 test(components): make FormFieldValidatorsTest prove memoisation testRepeatedCallsAreConsistent only compared list sizes across two calls to the same field, which a non-memoised implementation would also satisfy since resolution is deterministic -- it never observed how many times resolution actually ran. Replace it with a test that mocks ActionValidatorManager and asserts getValidators(...) is invoked exactly once across lookups of two different fields on the same Form instance, which is the actual behaviour getFieldValidators promises. Verified the new test fails (2 invocations instead of 1) when the resolveActionValidators() early-return is temporarily neutralised, then passes again with it restored. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 feat(components): resolve the HTML control type per component attributes.type is set by TextField and nothing else on the input path, so the control type cannot come from the attribute map alone. Adds getControlType() with four overrides; Checkbox, Radio, File and Hidden fall through to OTHER, which emits nothing and is correct for all four. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 feat(components): derive constraint attributes during tag evaluation Adds struts.ui.html5.constraints, default false, and wires UIBean to the constraint provider behind it. The hook sits at the end of evaluateParams rather than beside the tagNames block, because evaluateExtraParams is where TextField resolves attributes.type and it runs last; hooking earlier would make every text field look like OTHER. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 test(components): pin the constraint hook's evaluateExtraParams ordering Adds a discriminating test: a stringlength validator on an explicit type="number" control must not emit minlength, since minlength is only legal once TextField.evaluateExtraParams() has resolved the control type. The prior two tests could not tell a correctly-placed hook from one hoisted to the tagNames block, because an untyped field resolves to TEXT either way. Also rewords the javadoc/properties comment for struts.ui.html5.constraints to stop committing to a specific future version number; the release version is chosen at release time, not baked into 7.4.0-SNAPSHOT. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 feat(html5): render derived constraint attributes Included from common-attributes.ftl so every html5 input picks the attributes up without per-template edits. Values rely on FreeMarker's auto-escaping (HTMLOutputFormat + ENABLE_IF_DEFAULT_AUTO_ESCAPING_POLICY, set globally by FreemarkerManager): pattern and the data-msg-* text are author-controlled and land inside an attribute, and the ?html builtin is rejected at parse time under this configuration ("legacy escaping ... not allowed when auto-escaping is on with a markup output format") because ${...} is already escaped. Verified against the actual template with a standalone FreeMarker render using the same Configuration: '"><script>&' comes out as "><script>&. Covers the regression that matters most - requiredLabel draws an asterisk and must never emit a required attribute. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 docs(validation): drop the ?html builtin from the template guidance The design and plan both prescribed ${value?html} for the constraints template. That is a parse error in this repo: FreemarkerManager sets ENABLE_IF_DEFAULT_AUTO_ESCAPING_POLICY with HTMLOutputFormat, so values are escaped by configuration and FreeMarker rejects ?html as a double-escape. Zero templates in the tree use it. The escaping requirement itself stands and is easy to mistake for absent when reading the template, so both documents now say why there is no visible escape and warn against disabling auto-escaping for this file. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 fix(html5): stop constraints.ftl leaking a newline into every input FreeMarker only strips a line's trailing newline when that line contains nothing but FTL tags. The <#if>/<#list>/</#if> block was on one physical line together with the interpolated attribute text, so it failed that test and the newline was emitted on every html5 input render - including when struts.ui.html5.constraints is off, breaking the "changes nothing when off" guarantee. Reformatted to the multi-line idiom already used by the sibling accesskey block in common-attributes.ftl and by dynamic-attributes.ftl, with </#list> and </#if> alone on their own lines. Added testRendersExactMarkupWhenTheConstantIsOff, an assertEquals on the full rendered string (not a contains check), which is the only kind of assertion that can catch a stray whitespace byte; the existing suite's normalize() strips whitespace entirely and could not have caught this. Verified it discriminates: reverting constraints.ftl to the single-line form made it fail on the trailing newline, restoring the multi-line form made it pass again. Co-Authored-By: Claude Opus 5 <[email protected]> * 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]> * WW-5695 docs(design): correct the spec and plan to match the post-fix behaviour The design spec's mapping table and the plan's Task 4/6 code listings still described the pre-fix behaviour: required and requiredstring both emitting unconditionally, no EmailValidator/CreditCardValidator pattern exclusion, no regex trim gate, and Radio/File deliberately left without a getControlType() override. These are now the only remaining wrong description of the feature and the input to the follow-up ticket WW-5696, so bring both back in line with StrutsHtmlConstraintProvider as it stands, including the new integral-min guard and duplicate-attribute suppression. Narrative sections are left alone; only the tables and code blocks are corrected, with notes marking what changed and why. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 refactor(components): address SonarQube findings No behaviour change; the core suite stays at 3257 tests, 0 failures. Split EcmaScriptSafeRegex.isSafe into a positioning loop and an isPortable predicate. The allowlist is the security-relevant half, and it now reads as one method answering one question, which is the thing a reviewer has to audit. Dropping the ++i mutation out of the loop header makes the "an escape consumes two characters" rule explicit rather than a side effect buried in a condition. Also: @deprecated Javadoc on the four members deprecated in this branch, each naming the html5 replacement; a constant for the repeated actionClass and required literals; renamed the local in TextField.getControlType that shadowed the field of the same name but held the evaluated value rather than the raw OGNL expression; and three test nits. FormTag.populateParams gets @SuppressWarnings("removal") -- the tag has to keep forwarding validate until both sides go in 8.0.0. Two findings are deliberately not taken. S1133 ("remove this deprecated code someday") fires on every @Deprecated and deprecation is the point of the branch. S8924 (static import for verify) is wrong here: AbstractUITagTest inherits verify(URL), which shadows a static import and fails to compile -- noted in a comment so the next reader does not retry it. Co-Authored-By: Claude Opus 5 <[email protected]> * WW-5695 fix(components): correct three constraint-derivation defects found in review Drop \b and \B from the portable-escape allowlist. Their meaning is not stable across the JDKs Struts supports: up to Java 18 the boundary was decided by Character.isLetterOrDigit, so it was Unicode-aware while \w stayed ASCII, and JDK 19 made the two consistent. Pattern.matches("^\\bäiti\\b$", "äiti") is therefore true on Java 17 and false on Java 21, while ECMAScript's always-ASCII boundary rejects it in every browser. On the Java 17 baseline that is a false reject, and no version check could rescue it - one validation.xml would have to mean two different things depending on the JVM. Resolve the action from the ActionInvocation rather than stack.peek(). The top of the stack is not the action whenever something has been pushed over it: ModelDrivenInterceptor pushes the model, an <s:iterator> around the field pushes the current element. ValidationInterceptor validates invocation.getAction(), and ValidatorSupport.getMessage builds its DelegatingValidatorContext from whatever it is handed, so peeking resolved data-msg-* keys against the wrong resource bundle. Restore the value stack in a finally. Derivation runs against the request-scoped stack - DefaultActionValidatorManager.getValidators hands each validator ActionContext.getValueStack() - and getMessage's pops sit outside any finally, so a message that fails to resolve leaves frames behind. Because the catch here deliberately swallows the failure to keep a decorative feature from costing the page, every tag rendered afterwards would then have resolved its OGNL against the wrong root, with nothing in the output to say so. Each of the three regression tests was confirmed to fail against the unfixed code, for its own reason. Co-Authored-By: Claude Opus 5 <[email protected]> --------- Co-authored-by: Claude Opus 5 <[email protected]>