[
https://issues.apache.org/jira/browse/WW-5772?focusedWorklogId=1047579&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1047579
]
ASF GitHub Bot logged work on WW-5772:
--------------------------------------
Author: ASF GitHub Bot
Created on: 15/Oct/26 06:48
Start Date: 15/Oct/26 06:48
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1047579)
Time Spent: 20m (was: 10m)
> Remove framework-generated inline event handlers from transfer-select,
> updownselect and combobox tags (CSP)
> -----------------------------------------------------------------------------------------------------------
>
> Key: WW-5772
> URL: https://issues.apache.org/jira/browse/WW-5772
> Project: Struts 2
> Issue Type: Improvement
> Components: Core Tags
> Reporter: Aaron Elijah Mars
> Priority: Minor
> Fix For: 7.5.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> h3. 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.
> h3. 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.
> h3. 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.
> h3. 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.
> h3. 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 message was sent by Atlassian Jira
(v8.20.10#820010)
