About a proposed to change to SafeHtml's UriUtils

[Thomas Broyer]

[cc gwt-steering; please reply to gwt-contrib]

Hi there,

There's a patch currently in review of which I don't quite know what to do: https://gwt-review.googlesource.com/12940

It proposes a change to c.g.g.safehtml.shared.UriUtils to allow whitelisting URL schemes in addition to the default whitelist (http, https, ftp, and mailto), with the (original) purpose of considering "tel:" URIs as "safe". Actually, it started out as simply whitelisting the "tel:" scheme, but then we had a "security review" that asked not to do it.

As I see it:

• Now that GWT 2.8 has been released; GWT 2.x is moving to "maintenance mode" (one could argue this is not the case yet, as we still don't have a clear plan for GWT 3.x and work on it hasn't yet started).
• Google has rewritten c.g.g.safehtml for use outside GWT (but is still GWT-compatible) at https://github.com/google/safe-html-types Given that Google will slowly be moving their projects from GWT to J2Cl, we should expect them to invest in safe-html-types and deprecate (from their point of view at least) c.g.g.safehtml.
• As a corollary, there's little to no reason to port c.g.g.safehtml to J2Cl. We'll probably rather end up deprecating it, and/or rewriting it as a wrapper around safe-html-types to make it J2Cl-compatible (for a smoother transition).
• There are easy workarounds for whitelisting custom URL schemes today: one could use Google's safe-html-types and "wrap" the sanitized value into a UriUtils.fromTrustedString(), or use String#regionMatches to case-insensitively compare the URL prefix with the custom scheme(s) and fallback to UriUtils.sanitizeUri() / UriUtils.fromString(). Given that with the proposed change, call sites would have to explicitly whitelist new custom schemes (i.e. require changes to call sites), they could just as well be changed to do that extra work (possibly through custom factories). The patch author probably already has such code in place already (the patch was actually initially proposed more than one year ago).

As such, I'm worried that we (and I include everyone, including the contributor, and the webappsec team at Google) may be investing into an API that's bound to be deprecated in the (near) future; and it doesn't seem worth the effort.

I'm also worried that this change may have an impact in the final code size (for everyone, not only those using the new API, which would obviously not be a problem), and possibly runtime performance (I doubt it, but who knows?). Making sure this isn't the case (or is acceptable) means spending even more time on the matter.

I'm a bit reluctant to giving a +2 (or even a +1) to the patch (after some more changes anyway) for the above reasons, actually thinking I may rather reject it entirely with a -2. I don't want to take such a decision unilaterally though.

[Colin Alworth]

With 3.0 on the horizon, we've promised consistency of a sort in 2.x, and without 3.0 actually in sight, 2.x is going to need to see active development. Encouraging a third generation of url tools is not a bad thing, but only switching over half-way leaves something to be desired.

I'll play devil's advocate a bit - I'm not addressing the patch specifically (since I haven't fully read all of it or the discussion around it, and if it was a bad patch, I'm sure it would have been shot down on its own merits) but the thinking to use around what goes into the 2.x branch:

 * (point 1) We're not ready for maintenance mode, at least until we have timelines and completed APIs for GWT 3. If we are, we should be forbidding all merge requests to gwt-user that don't fix critical bugs. 

 * (point 2 and 3) Can't argue with new tools arriving that solve the problem better, especially if they are going into 3 as a cross-platform way of solving the problem (gwt/java/j2cl). Obvious caveat here (even for the devil's advocate) that with the release of 3, breaking changes off of 2 are acceptable and expected.

 * (point 4) Without transitioning the existing GWT Safe* tools, SafeHtmlTemplates is stuck in the past, as is UiBinder, I18n Messages, and any HasSafeHtml widget (both in GWT and in the general ecosystem). This is a lot to leave behind in the name of "but the tool didn't belong in GWT in the first place". We've had our chance to properly deprecate it for the 2.8 release, and if our past timelines are any measure, it is going to be at least a year before we finally ship a release with SafeHtml, and all that use it, deprecated. And once we've reached that point, unless gwt-user depends on (or rebases, etc) safe-html-types or any other successor, all of the above downstream classes which use SafeHtml are left effectively broken (not unlike the unported Generators and Linkers we have today). 

Obviously ClientBundle/GssResource isn't actually deprecated, but we have officially said that new tools should not use the features that it takes advantage of. SafeHtml takes this a step further - GssResource got several upgrades within the 2.8 release (unlike other generators), despite it being deprecated, but with SafeHtml going out, it takes out features within many GWT Widgets themselves. There will be no officially supported way to correctly assign safe html content into an HTML widget.

Now yes, a bit of hyperbole there - you can of course (as Thomas noted in his email) simply asString() the not-gwt SafeHtml and use it, or provide your own wrapper, but depending on your project size (and GWT users out there have some big ones) that could be hundreds or thousands of sites to fix in code. Yet another change would be needed for any widgets that the project has which implement HasSafeHtml (so it can still be used in UiBinder), UiBinder @UiFields or getters (which return SafeUri for use in its embedded SafeHtml handling), Messages (to wrap any incoming url). This ignores any use of Messages/Constants in a SafeHtml-using uibinder itself - I'm not seeing a clear path there without the use of default methods in I18n interfaces (which admittedly, would probably work, but isn't going to be pretty).

---

Speaking for myself again, I am not prepared to outright "-2" any contribution to gwt-user that isn't a fix for a critical bug. I don't think we're ready today to deprecate everything inside of gwt-user that isn't JRE emulation or jsinterop-based. Ideally of course aspects of GWT will be spun out as community projects and maintained separately, but until we pull it off (or even start), any new GWT user will assume that anything in gwt-user is the best basis for a GWT project. Actively abandoning most of gwt-user is not going to help the already difficult situation we're in there.