Repository navigation
Conversation
CSPDirective had no require-trusted-types-for and no trusted-types, so an application could not have the browser enforce Trusted Types through the CSP settings. Both Ajax engines, wicket-autocomplete.js, progressbar.js and MultiFileUploadField.js also assigned plain strings to DOM sinks (innerHTML, script text and src, DOMParser, and the XML parsing of the Ajax response in the jQuery engine), so on a page that enforced Trusted Types every Ajax update failed. The two directives are now available with value checks, and CSPHeaderConfiguration.requireTrustedTypes() adds both for the policy name TRUSTED_TYPES_POLICY. No preset includes them. The sinks go through Wicket.TrustedTypes, which on first use creates a policy named wicket that passes the markup and scripts of the server on unchanged. Without the Trusted Types API, or when the page does not allow the policy, the values stay strings as before. MultiFileUploadField.js now writes file names as text nodes instead of escaped markup. An application that does not add the directives sees no change in the rendered page. The one difference without them is in MultiFileUploadField.js: MultiSelector.getOnlyFileNames() now returns an array of names and getOnlyFileName() no longer escapes, which code that calls or overrides these functions would notice. Under enforcement two limits remain. Handlers that reach Wicket.Ajax as strings are compiled with new Function, which the browser blocks, so they do not run; Wicket renders its own handlers as functions. With the jQuery engine a script element inside replaced component markup makes the update fail, because jQuery 4.0.0 evaluates it from a string. The user guide names both.
papegaaij
left a comment
papegaaij
left a comment
There was a problem hiding this comment.
Thank you for this contribution! Trusted Types support is something we'd really like to have in Wicket, and this is thorough work: both Ajax engines, the extensions scripts, a QUnit page that actually enforces the policy, and the user guide. I really like the direction.
Before we can merge it, two requirements have to hold.
1. With the option disabled, which is the default for now, nothing may change.
At the moment the JavaScript creates the wicket policy in every browser that supports Trusted Types, whether or not the application called requireTrustedTypes(). Applications that never opted in therefore already get TrustedHTML objects instead of strings, the replaced text xml converter and the changed MultiSelector functions. Please make the client side opt-in as well, so that the policy is only created and values are only wrapped when the application enabled the option. The inline comments have the details.
2. With the option enabled, Wicket must work, with no exceptions.
The one limitation we accept is the eval of the onselect expression in wicket-autocomplete.js. The other limitations in the guide have to go:
- A
<script>element inside replaced component markup, inline or external, must also work with the jQuery engine. That is the default engine, so this limitation would hit most applications that enable the option. - Please make sure nothing in Wicket's own modules hands string handlers, preconditions or dynamic extra parameters to
Wicket.Ajax, so Wicket code never reaches thenew Functionpath. The Java side renders them as functions. Application JavaScript that passes strings is the application's concern, and the guide can say so.
To show the feature works as intended, please enable it in the examples: add .requireTrustedTypes() to the CSP configuration in WicketExampleApplication and in the example applications that set up their own CSP, such as MyAuthenticatedWebApplication, and make sure the examples still work. Any script used by Wicket or by the examples that breaks under enforcement needs fixing as part of this PR.
One note on scope: custom Wicket Ajax references (setWicketAjaxReference(...)) don't need to be supported. An application that ships its own copy is responsible for keeping it in sync with the full API, so it's fine for wicket-autocomplete.js and progressbar.js to rely on Wicket.TrustedTypes.
On the tests, which are already a good base, I'd like to see:
- a test for the disabled mode in a browser that supports Trusted Types, showing that values stay strings, for example with a custom
jQuery.htmlPrefilterthat does string operations; - an end-to-end Ajax response with
<header-contribution>,<priority-evaluate>and<evaluate>under enforcement, not only a component replacement; - the script-in-markup test running for both engines;
- Java tests for calling
requireTrustedTypes()twice, and for combining it withstrict()andreporting().
| policy = { createHTML: identity, createScript: identity, createScriptURL: identity }; | ||
| if (window.trustedTypes && window.trustedTypes.createPolicy) { | ||
| try { | ||
| policy = window.trustedTypes.createPolicy('wicket', policy); |
There was a problem hiding this comment.
This runs in every browser that supports Trusted Types, including when the application did not call requireTrustedTypes(). With the option disabled, that has visible effects:
- With jQuery 4,
toMarkuppasses aTrustedHTMLobject to jQuery, which hands it tojQuery.htmlPrefilter. Any override of that hook that does string operations throws. jquery-migrate 4.0.2, which Wicket ships, is one example. Itsself-closed-tagspatch is off by default, but applications that still have<div/>-style markup enable it, and it callshtml.replace(...)(jquery-migrate-4.0.2.js:1017). Every Ajax update then fails withhtml.replace is not a function. - An application that enforces Trusted Types through its own header with a
defaultpolicy now has Wicket's markup bypass that policy. - An application whose header restricts policy names without allowing
wicketgets an error logged on every page, and with a report-only header a violation report on every page.
Could the policy be created, and values wrapped, only when the application enabled the option? For instance, the server could render a flag when the CSP contains trusted-types wicket. With the flag off, html(), script() and scriptURL() should return their input unchanged, and the text xml converter should not be installed. The same applies to wicket-ajax.js.
| * response, so the policy 'wicket' passes them on unchanged. They stay strings where the | ||
| * browser has no Trusted Types or the page does not allow the policy. | ||
| */ | ||
| Wicket.TrustedTypes = (function () { |
There was a problem hiding this comment.
Smaller point: html(), script() and scriptURL() turn any string into a trusted value, and any script on the page can call them. For an application that enables the option, this weakens what Trusted Types offers, because the set of code that can create trusted values is no longer small and auditable. Wicket trusts the markup and scripts the server sends, so routing the Ajax response through this policy is fine. Please at least document on this object that it must only be given markup and scripts that come from the server. The guide should also say that, with the option enabled, application code must not pass untrusted strings to Wicket.DOM.replace, Wicket.DOM.add or Wicket.Xml.parse.
| const cleanedText = text.trim(); | ||
|
|
||
| const $newElement = jQuery(cleanedText); | ||
| const $newElement = jQuery(toMarkup(cleanedText)); |
There was a problem hiding this comment.
With the option enabled, this has to work for markup that contains <script> elements, inline or external. Right now jQuery evaluates them from a string and the update fails, and this is the default engine.
One way to solve this: parse the markup the way wicket-ajax.js does, with a template element and reviveScripts, and insert the nodes with the DOM API instead of through jQuery's manipulation methods, which evaluate scripts themselves. Removed elements would still need jQuery's data and event cleanup. That would also make toMarkup and its trailing-text handling unnecessary. The same applies to add below.
| }); | ||
|
|
||
| // jQuery 4.0.0 evaluates the scripts of the markup it inserts itself, from a string | ||
| test.if('Wicket.DOM.replace - inline and external scripts in the markup', typeof(jQuery) === 'undefined', assert => { |
There was a problem hiding this comment.
Once the jQuery engine handles scripts in markup, this test should run for both engines.
| .add(CSPDirective.TRUSTED_TYPES, "myPolicy"); | ||
| ---- | ||
|
|
||
| Ajax call listeners and dynamic extra parameters that reach _Wicket.Ajax_ as strings instead of functions are not executed under these directives. With the default jQuery based Ajax implementation jQuery 4 is required, and a _script_ element inside the markup of a component makes the Ajax update of that component fail, because jQuery evaluates such elements from a string. The jQuery-free implementation (_WicketAjaxResourceReference_) executes them. |
There was a problem hiding this comment.
With the option enabled, the only limitation we want to keep is the eval of the autocomplete's onselect expression in wicket-autocomplete.js, so this paragraph should name only that one. It can also note that application JavaScript passing handlers to Wicket.Ajax as strings won't run. The script element limitation should go away with the engine change.
| { | ||
| var files = inputElement.files; | ||
| var retVal = ""; | ||
| var retVal = []; |
There was a problem hiding this comment.
This changes the result of getOnlyFileNames() from a string to an array, and getOnlyFileName() below no longer escapes. Both are overridable functions of MultiSelector, so applications that never enable Trusted Types will see the change. Could you keep both functions as they are, deprecated if you like, and add a new function that returns the unescaped names as an array for addListRow to use? Building text nodes is the right approach; it's only the existing contract that should stay.
| public void checkValueForDirective(CSPRenderable value, | ||
| List<CSPRenderable> existingDirectiveValues) | ||
| { | ||
| if (!(value instanceof FixedCSPValue) |
There was a problem hiding this comment.
add(TRUSTED_TYPES, CSPDirectiveSrcValue.NONE) is rejected here, and only the string "'none'" is accepted; trustedTypesDoesNotSupportOtherValues even asserts that. Every other directive uses CSPDirectiveSrcValue.NONE for 'none', so please accept it here too.
| */ | ||
| public CSPHeaderConfiguration requireTrustedTypes() | ||
| { | ||
| return add(REQUIRE_TRUSTED_TYPES_FOR, "'script'").add(TRUSTED_TYPES, TRUSTED_TYPES_POLICY); |
There was a problem hiding this comment.
Calling this twice throws an IllegalArgumentException, because require-trusted-types-for accepts only one value. If both a library and the application call it, the application fails at startup. Could it be idempotent, skipping values that are already present?
CSPDirectivedoes not knowrequire-trusted-types-forandtrusted-types, and both Ajax engines and three component scripts hand strings to DOM sinks, so every Ajax update fails on a page that enforces Trusted Types. This adds the two directives with the opt-inCSPHeaderConfiguration.requireTrustedTypes(), routes the sinks throughWicket.TrustedTypes, whosewicketpolicy passes server markup and scripts on unchanged, and writes the file names ofMultiFileUploadField.jsas text nodes. The new QUnit pagetrusted-types.htmlcovers each sink under enforcement and runs with both engines.No preset includes the directives, and without them the rendered page does not change. Under enforcement two limits remain: handlers given as strings still go through
new Functionand do not run, and with the jQuery engine ascriptelement inside replaced component markup fails the update, because jQuery 4.0.0 evaluates it from a string. Tested in headless Chrome.