Repository navigation
Let autocomplete selections and ChoiceFilter auto-submit work under the default CSP (WICKET-6938) - #1667
Let autocomplete selections and ChoiceFilter auto-submit work under the default CSP (WICKET-6938)#1667aeonframework wants to merge 5 commits into
Conversation
ChoiceFilter with autoSubmit wrote onchange="this.form.submit();" into the markup. Wicket's default Content Security Policy (strict(): script-src 'strict-dynamic' 'nonce-...') blocks inline event handler attributes, so under the default settings the filter silently stopped submitting on change. It now registers the same handler with an OnEventHeaderItem, as FilterForm#enableFocusTracking already does for focus tracking, so the script carries the CSP nonce. The markup is unchanged apart from the removed attribute: the select already rendered its markup id for focus tracking. Generated-by: an AI coding assistant; reviewed and tested by the author.
The autocompleter ran AbstractAutoCompleteRenderer#getOnSelectJavaScriptExpression by reading the onselect attribute of the chosen item and passing it to eval(). Wicket's default Content Security Policy does not allow 'unsafe-eval', so under the default settings that eval throws and selecting an item with an expression does nothing; the only way to use the hook was to weaken the CSP for the whole application. The hook also invites building JavaScript from item data by string concatenation, as its own javadoc examples do, which the markup escaping of the attribute does not protect once eval() decodes it. The autocompleter now dispatches a cancelable, bubbling wicket-autocomplete-select event on the input. detail.item is the chosen element and detail.value the text value; a listener may replace detail.value, or call preventDefault() to leave the input unchanged (what returning null from the expression did). Data a listener needs is written with the new AbstractAutoCompleteRenderer#getChoiceData as escaped data-* attributes whose names are restricted to lowercase letters, digits and dashes. The listener is registered from renderHead() and so carries the CSP nonce; nothing is evaluated. getOnSelectJavaScriptExpression is deprecated for removal and keeps working where the application allows 'unsafe-eval': its result becomes the initial detail.value of the event. Applications that do not override it see no difference. Migration: move the expression into a wicket-autocomplete-select listener and expose the values it needs through getChoiceData. Removing the method and the eval() fallback on master is a separate follow-up. WICKET-6938 Generated-by: an AI coding assistant; reviewed and tested by the author.
|
Hi, gentle ping on this one. It fixes WICKET-6938 (CSP-safe autocomplete). Could a maintainer take a look and approve the CI workflows when there is time? Happy to make any changes. |
|
Hi, bumping this once more in case it got lost. It fixes WICKET-6938 (CSP-safe autocomplete). The CI workflows still need a maintainer approval to run. Happy to make any changes. |
reiern70
left a comment
There was a problem hiding this comment.
@aeonframework thanks for this, and sorry for the wait. I went through both commits and ran them merged onto current master: the wicket-extensions tests and the QUnit suites for both engines pass, and the new tests fail against the old code. The approach looks right to me: a cancelable, bubbling event plus data-* attributes is the natural CSP-safe replacement for the eval() hook, and the ChoiceFilter change follows what FilterForm#enableFocusTracking already does.
Points, most important first:
-
Backport to 10.x. Both problems exist on
wicket-10.xas well (the sameeval()inhandleSelection, the same inlineonchange), and the change only adds API, so we would like it there too. Could you open a second PR againstwicket-10.xwith both commits? The ChoiceFilter commit cherry-picks cleanly. The autocomplete commit conflicts in three places:wicket-autocomplete.js: 10.x still usesvarinhandleSelection.Gruntfile.js: 10.x has no vanilla engine and runs the extensions pages against jQuery 4.0.0 and 3.7.1, soautocomplete.htmlloses itsvanillabranch and gets the two jQuery URLs instead.JAVASCRIPTTESTING.mddoes not exist on 10.x; drop that hunk.
-
Version. 11.0.0 is already staged for its release vote, so this misses it. With the backport the API first ships in 10.12.0, so please use
@since 10.12.0ongetChoiceDataandsince = "10.12.0"on the deprecation, on both branches. -
Removal follow-up. Once 11.0.0 is released it becomes the japicmp baseline for master, and removing a protected method of
AbstractAutoCompleteRendereris binary-incompatible. So "remove on master after this lands" can't happen in 11.x; it belongs in the next major. Deprecating withforRemoval = truenow is still right, please just drop or reword that follow-up. -
Javadoc: listener and Ajax re-renders. The class example attaches the listener to
document.getElementById('address'). When the field is re-rendered via Ajax the element is replaced and the listener is lost, unless the header item that registers it is contributed again. Please make clear that it should be registered from the field's ownrenderHead()(which runs again on re-render), or, since the event bubbles, show a listener ondocumentthat checksevent.target. It would also help to mention the event onAutoCompleteTextField/AutoCompleteBehavior, which is where users look first; today it is only documented on the renderer. -
An empty value does not clear the field. Both callers check
if (value), so a listener that setsdetail.value = ''leaves the input unchanged instead of clearing it. That is inherited from the old code, and changing it would also change what deprecated expressions returning''do, so I would just document it: an empty value leaves the input unchanged, likepreventDefault(). -
Deprecated expression under the default CSP (optional). If an application still overrides
getOnSelectJavaScriptExpressionand runs under the default CSP,eval()throws before the event is dispatched, so a new listener never runs and the popup stays open, as it does today. Catching the error inevalOnSelect, logging it withWicket.Log.errorand falling back toinputwould let an application that is halfway through the migration keep working. -
Nits.
getChoiceDataJavadoc: dashed names surface in camelCase indataset(zip-codebecomesdataset.zipCode). One sentence would save users a surprise.ChoiceFilter: thebind()override that callssetOutputMarkupId(true)duplicates whatenableFocusTracking(choice)does right after. Harmless; keep it if you want the behavior to stand on its own.- WICKET-6938 is about the autocomplete only. The ChoiceFilter commit is a related CSP fix with no issue reference; a short GitHub issue for it would put it in the release notes.
With 1 to 3 addressed this looks good to me; 4 and 5 are documentation, 6 and 7 are up to you.
getChoiceData and the deprecation of getOnSelectJavaScriptExpression are backported to wicket-10.x, so both name 10.12.0, the first release that has them, on master too.
The class example added the listener to the input found by id. An Ajax update that re-renders the field replaces that element and the listener is lost with it. The example now registers the listener from the renderHead() of the field, which runs again on every re-render, and names the alternative of a single listener on document, which the bubbling event reaches as well. The documentation also says that an empty detail.value leaves the input unchanged, like preventDefault(): the autocompleter only copies a truthy value, as it did for the result of the deprecated expression. The behaviour itself is unchanged. getChoiceData says how a dashed name is read back from dataset (zip-code becomes dataset.zipCode), and AutoCompleteTextField and AutoCompleteBehavior point to the event, since that is where an application looks for it. WICKET-6938
An application that still overrides getOnSelectJavaScriptExpression and runs under the default Content Security Policy, which does not allow 'unsafe-eval', had eval() throw before the wicket-autocomplete-select event was dispatched. Selecting a choice then did nothing at all, as before this change. The autocompleter now catches the error, logs it with Wicket.Log.error and continues with the text value of the choice, so the selection works, the event reaches its listeners and the log shows that the expression has to be moved into a listener. Where 'unsafe-eval' is allowed and the expression runs, nothing changes. WICKET-6938
|
Thanks a lot for the careful review @reiern70! All points are addressed, as new commits on top here (no force-push), and the backport is ready.
Tested on both branches: the autocomplete and filter tests in wicket-extensions, plus the Grunt/QUnit run (jshint and qunit, both engines on master and both jQuery versions on 10.x), all green. |
Resolves WICKET-6938.
Problem
Wicket's default Content Security Policy (
strict():script-src 'strict-dynamic' 'nonce-…') blocks two things wicket-extensions still relies on:AbstractAutoCompleteRenderer#getOnSelectJavaScriptExpressionis written into anonselectattribute of each choice, andwicket-autocomplete.jsruns it witheval(). Without'unsafe-eval'that throws, so selecting an item with an expression does nothing, and the only way to use the hook is to weaken the CSP for the whole application. The hook also invites building JavaScript from item data by string concatenation, as its own javadoc examples do; the markup escaping of the attribute does not protect that data onceeval()decodes it.autoSubmitthe select is rendered withonchange="this.form.submit();", an inline handler the CSP blocks, so the filter silently stops submitting on change.Approach
Two commits, each standing on its own:
OnEventHeaderItem, the wayFilterForm#enableFocusTrackingalready does, so the script carries the CSP nonce. The markup only loses the attribute.wicket-autocomplete-selectCustomEventon the input.detail.itemis the chosen element anddetail.valuethe text value; a listener may replacedetail.value, or callpreventDefault()to leave the input unchanged (what returningnullfrom the expression did). Data a listener needs comes from the newAbstractAutoCompleteRenderer#getChoiceData(T), written as markup-escapeddata-*attributes whose names must match[a-z][a-z0-9-]*. The listener is registered fromrenderHead(), so it carries the nonce and nothing is evaluated. The class javadoc shows a complete listener.getOnSelectJavaScriptExpressionis@Deprecated(since = "10.12.0", forRemoval = true)and keeps working where'unsafe-eval'is allowed: its result becomes the initialdetail.value, and the expression still sees the text value asinputandarguments[0]. Applications that do not override it see no change. If the expression throws (for example because the CSP blockseval()), the error is logged withWicket.Log.errorand the text value is used, so the event is still dispatched.Migration: move the expression into a
wicket-autocomplete-selectlistener and expose the values it needs throughgetChoiceData.Backport to
wicket-10.x: #1728.Tests
mvn clean verify -Pjs-test -pl wicket-extensions,testing/wicket-js-tests -amd(JDK 21, aftermvn clean install -DskipTests -Pfast): BUILD SUCCESS. wicket-extensions 114/114, wicket-examples 67 run with 0 failures (14 skipped), QUnit 162/162 (jQuery) and 161/161 (vanilla). japicmp and javadoc ran clean.ChoiceFilterTest: checks there is no inlineonchangeand that the header registers the change handler. It fails on the base code.AbstractAutoCompleteRendererTest: checksgetChoiceDataescaping, skipping ofnullvalues, and rejection of invalid attribute names.autocomplete-test.js+autocomplete.html, wired intoGruntfile.jsfor both engines and listed inJAVASCRIPTTESTING.md. It covers the event and its detail, a listener replacing the value, a listener cancelling the selection, and a deprecated expression still getting the text value asarguments[0]. Against the basewicket-autocomplete.jsits 3 event assertions fail.Follow-ups
getOnSelectJavaScriptExpressionand theeval()fallback (removing a protected method ofAbstractAutoCompleteRendereris binary-incompatible).new Function(), which also needs'unsafe-eval'.This change was prepared with AI assistance and reviewed and tested by me.