You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@struts.apache.org by "aeonframework (via GitHub)" <gi...@apache.org> on 2026/10/14 19:27:28 UTC
[PR] WW-5772 Remove inline event handlers from optiontransferselect, inputtransferselect, updownselect and combobox [struts]
aeonframework opened a new pull request, #2012:
URL: https://github.com/apache/struts/pull/2012
Fixes [WW-5772](https://issues.apache.org/jira/browse/WW-5772)
### Problem
The `optiontransferselect`, `inputtransferselect` and `updownselect` tags render every button with an `onclick` handler that the framework builds itself. The `combobox` tag renders its select with an inline `onChange`. An application that wants a nonce-based Content-Security-Policy without `'unsafe-inline'` therefore cannot use these tags, even though `<s:script>` already applies the nonce to the scripts the same templates emit.
The generated `onclick` code also contains the `headerKey`/`doubleHeaderKey` values as JavaScript string literals that are only HTML-escaped. The browser decodes `'` back to `'` before it runs the handler, so a header key that contains a quote breaks the handler.
### Approach
- **Transfer selects and updownselect.** Each button now carries its action, the target select id(s) and the header key(s) in `data-struts-*` attributes, which FreeMarker HTML-escapes. `optiontransferselect.js` (also used by updownselect) and `inputtransferselect.js` each register one delegated click listener per page, in the capture phase, guarded against double registration. The listener calls the same existing functions with the same arguments as the old inline handlers, header-key/no-header-key branches included. `optiontransferselect.ftl` folds its duplicated per-button branches into one `transferButton` macro, about 100 lines shorter. A developer-supplied `*Onclick` attribute is still rendered as `onclick` and still runs after the framework action.
- **Combobox (simple and html5).** The select carries `data-struts-combobox="<escapedId>"`. The `change` listener is attached from an `<@s.script>` block, which carries the nonce like the `autoPopulate_<id>` function the template already emits. The script binds every select with that marker that is not bound yet. Repeated comboboxes with the same id (e.g. inside an iterator) therefore each get exactly one handler, and fragments loaded over AJAX work too: jQuery `.html()` evaluates scripts through a temporary element in `<head>`, so `document.currentScript` cannot be used.
- The old global JS functions are unchanged, so custom themes that copied the old templates keep working.
### Why it is systematic
Each of these tags had the same pattern: framework-built JavaScript in event-handler attributes. The patch removes that pattern from them entirely rather than escaping individual values, so neither the CSP blocker nor the quoting problem can come back through these templates.
### Upgrade note (please add to the release notes / migration guide)
The new markup needs the new `optiontransferselect.js` and `inputtransferselect.js`.
- An application that serves its own copy of the static files (`struts.serve.static=false` or a custom `struts.ui.staticContentPath`) must update those two files together with the templates. Otherwise the transfer buttons silently do nothing.
- The static URL is not versioned and browsers cache it for one day, so a browser holding the old copy right after an upgrade sees inactive buttons until the cache expires.
- Code that calls `button.onclick()` directly no longer triggers the framework action. Real clicks and `element.click()` still do.
### Tests
- `./mvnw -pl core test` on JDK 17: 3466 tests, 0 failures, 0 errors, 0 skipped.
- New tests in `OptionTransferSelectTagTest` and `UpDownSelectTagTest` check that framework buttons render no `onclick` and that a header key containing a quote (`it's-left`) lands HTML-escaped in a `data-*` attribute. The rendering fixtures are updated. On the base templates, 51 of the 57 tests in the four affected tag test classes fail.
- Manual check in headless Chromium, under `<meta http-equiv="Content-Security-Policy" content="script-src 'nonce-…'">` with no `'unsafe-inline'`: zero CSP violations. These all worked:
- optiontransferselect add-to-right, add-all-to-left and select-all, with the header options kept in place;
- updownselect move-up and select-all;
- inputtransferselect add, and move-up stopping at the header key;
- two comboboxes with the same id in two forms, each populating only its own field;
- a combobox inserted with `innerHTML` and its scripts evaluated jQuery-style, which populated its field.
### Follow-ups (separate tickets; this PR does not make the themes fully CSP-clean)
- xhtml/css_xhtml `form.ftl`: default `onreset="clearErrorMessages…"` and the `onsubmit="return validateForm_…"` for client-side validation
- xhtml `tooltip.ftl`: `domTT_activate` `onmouseover` when `javascriptTooltip` is on
- `doubleselect`: the `onchange="<id>Redirect(…)"` built in `DoubleSelect.java`
This change was prepared with AI assistance and reviewed and tested by me.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5772 Remove inline event handlers from optiontransferselect, inputtransferselect, updownselect and combobox [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #2012:
URL: https://github.com/apache/struts/pull/2012#discussion_r4279923722
##########
core/src/main/resources/template/simple/optiontransferselect.ftl:
##########
@@ -22,6 +22,18 @@
<@s.script src="${base}${attributes.staticContentPath}/optiontransferselect.js" />
<#assign temporaryVariable = stack.setValue("#optiontransferselect_js_included", "true") /><#t/>
Review Comment:
The new markup only works with the new `optiontransferselect.js`, but this URL isn't versioned. A browser holding the cached old script (static content is cached for a day), or an app serving its own copy, renders buttons with no `onclick` and no listener, so clicks silently do nothing. Same for `inputtransferselect.ftl` and `updownselect.ftl`.
The upgrade note in WW-5772 covers it for now; a cache-busting parameter on these URLs is tracked separately in [WW-5773](https://issues.apache.org/jira/browse/WW-5773).
##########
core/src/main/resources/template/simple/combobox.ftl:
##########
@@ -98,4 +98,17 @@
</option><#lt/>
</...@s.iterator>
</select>
+<@s.script>
Review Comment:
Combobox takes a different route from the transfer selects: a per-instance inline script (with the nonce) rather than a static delegated listener. That works under a nonce-based CSP, but:
- a hash-based or static-scripts-only policy still blocks it,
- this block is duplicated verbatim in `html5/combobox.ftl`,
- N comboboxes in an iterator emit N blocks, each running `querySelectorAll` over the page.
Fine to land as is; a static `combobox.js` with one delegated `change` listener is tracked in [WW-5774](https://issues.apache.org/jira/browse/WW-5774).
##########
core/src/test/java/org/apache/struts2/views/jsp/ui/OptionTransferSelectTagTest.java:
##########
@@ -1142,6 +1142,34 @@ public void testDisableSomeButtons_clearTagStateSet() throws Exception {
strutsBodyTagsAreReflectionEqual(tag, freshTag));
}
+ public void testButtonsCarryHeaderKeysAsDataAttributesWithoutInlineHandlers() throws Exception {
Review Comment:
Nice targeted test. There's no equivalent for `inputtransferselect` (`data-struts-header-key` on move up/down) or for `combobox` (no inline `onChange`, marker attribute, nonce on the binding script), so regressions there are only caught by the fixture files. Tracked in [WW-5775](https://issues.apache.org/jira/browse/WW-5775).
##########
core/src/main/resources/org/apache/struts2/static/optiontransferselect.js:
##########
@@ -189,3 +189,96 @@ function swapOptions(objTargetElement, first, second) {
opt[first] = temp2;
opt[second] = temp;
}
+
+/*
+ * Buttons rendered by the optiontransferselect and updownselect tags carry their action, target select ids and header
+ * keys in data-* attributes instead of inline onclick handlers, so the tags work under a Content-Security-Policy
+ * without 'unsafe-inline':
+ * data-struts-updownselect="moveUp|moveDown|selectAll", data-struts-target, optional data-struts-header-key
+ * data-struts-optiontransferselect="leftUp|leftDown|rightUp|rightDown|addToLeft|addToRight|addAllToLeft|
+ * addAllToRight|selectAll", data-struts-left, data-struts-right, optional data-struts-header-key and
+ * data-struts-double-header-key
+ * The listener is registered once per page, in the capture phase, so the framework action still runs before a
+ * developer-supplied onclick attribute on the same button and is not stopped by an ancestor's stopPropagation().
+ */
+(function () {
+ if (window.strutsOptionTransferSelectListener) {
+ return;
+ }
+ window.strutsOptionTransferSelectListener = true;
+
+ function headerKey(button, name) {
+ return button.hasAttribute(name) ? button.getAttribute(name) : undefined;
Review Comment:
Not introduced here, but worth knowing since the ticket says a header key with a quote now works: the key still goes through `compile()`, which builds `new RegExp("^" + ptn + "$")` without escaping. A key like `[none` makes every button throw `Invalid regular expression`, and `(none)` doesn't match itself, so the header option gets moved/selected with the rest. The old inline handlers had the same behaviour, so no change needed in this PR.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5772 Remove inline event handlers from optiontransferselect, inputtransferselect, updownselect and combobox [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart merged PR #2012:
URL: https://github.com/apache/struts/pull/2012
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org