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.

[Thomas Broyer]

On Thursday, November 3, 2016 at 5:33:31 PM UTC+1, Colin Alworth wrote:

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).

You forgot one major point here: this patch is proposing a new API (new overloads to existing methods), without actually changing the current API (at least in terms of its contract).

So it's not about changing "every call sites for SafeHtml/UriUtils API", it's about being able to take advantage of a new behavior by selectively calling a new API.

If you want that behavior everywhere, then yes you'll have to update all call sites; but the patch doesn't change that in any way (except what you change your code *to*)

All those existing usage of SafeHtml you cited (SafeHtmlTemplates, UiBinder, I18N and HasSafeHtml) would be unchanged and keeping their current behavior (and only 2 of them deal with SafeUri: SafeHtmlTemplates and UiBinder). And the only two cases where UriUtils is called by those (vs. receiving a SafeUri value from the "user") are discouraged and generate a warning (and would continue to only use the default whitelist and reject "tel:" URIs; if you'd like to allow "tel:" URIs there, you'd have to change to SafeUri anyway, and then the responsibility of converting a String to a SafeUri falls on you, so no change to UriUtils or other APIs/generators is actually *required*):

https://github.com/gwtproject/gwt/blob/3a41a8067767965ff72f4e49eb015f587fcfee16/user/src/com/google/gwt/uibinder/attributeparsers/SafeUriAttributeParser.java#L41

https://github.com/gwtproject/gwt/blob/3a41a8067767965ff72f4e49eb015f587fcfee16/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#L183 (warning at: https://github.com/gwtproject/gwt/blob/3a41a8067767965ff72f4e49eb015f587fcfee16/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#L364)

After the security review, we're not in a situation of "I think GWT's behavior should be changed (and this will affect everyone in many places)", we're rather in a situation of "I'd like to add this new feature to GWT; and this is something I could do in my own code" (delegating to the existing behavior after I checked whether the new behavior I want would apply).

To put it in precise terms for this patch, it's a matter of changing UriUtils.fromString(…) to either TelAwareUriUtils.fromString(…) (application-specific factory that eventually delegate to UriUtils.fromString) or UriUtils.fromString(…, Collections.singleton("tel")) (new proposed API). That TelAwareUriUtils would look like:

public class TelAwareUriUtils {

  public static SafeUri fromString(String uri) {

    if (isTelUri(uri)) {

      return UriUtils.fromTrustedString(uri);

    }

    return UriUtils.fromString(uri);

  }

  // add similar isSafeUri and sanitizeUri methods

  private boolean isTelUri(String uri) {

    return uri.regionMatches(true, 0, "tel:", 0, 4); // this is a case-insensitive startsWith

  }

}

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.

+1, but we're not in that exact situation here. 

[Goktug Gokdogan]

FWIW, I agree with tbroyer's assessment and the suggestion to use a helper sounds reasonable.

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/cede6c7f-7428-42f0-bc42-8b1cb03cbecf%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Colin Alworth]

I agree entirely, just trying to see this as a potential contributor with its associated steep learning curve, and address the question of "Should we stop adding new features?".

we're rather in a situation of "I'd like to add this new feature to GWT; and this is something I could do in my own code"

If (and I stress if) we are prepared to stop accepting features requests that come with technically correct code, we should not make that decision lightly. There is little doubt that this would be an improvement to GWT, and something that generally would make GWT apps better - why should we not take it?

My (and your) alternative of using an external safe-* project adds boilerplate for using gwt-user features that are considered up-to-date and well supported. That is the main basis for my +1, that it isn't sufficient to say "well, if you want this browser API, which could be easily supported out of the box, here are the hoops you jump through". Find/Replace all use of UriUtils leads to prefixed MyCompanyUriUtils for each class in the GWT that could/should be better, and does make updating harder, or encourages forked copies of GWT which then can be difficult to rebase and keep up to date. 

In contrast, transforming a user into a contributor, even if only a fraction of them stay with us long term, improves the overall health of the project.

The one technical objection I see is that including Collections.emptySet() and its transitive tree might add some weight for small projects that avoid any java collections. String[] might be preferred, but with a minor runtime cost last I checked. 

If it is a technically bad patch (style, increases compile time/size in an unacceptable way, insufficiently tested/documented, insecure, etc), it should be rejected. If it could be done in user code (and just about any feature request *could* be done), this probably isn't enough to reject it, unless we think it is a fringe case which other users are unlikely to avail themselves of. But if it improves the sdk in a way that will benefit users, and make it clear that GWT is in it for the long haul, improving every release, and open to developer contributions? Why should we stand in the way of that?

(Again, mostly devil's advocate, trying to address the huge number of concerns we've had over the last year or four of "contributing to GWT is too hard" or "GWT doesn't move forward and stay current with modern APIs". This is a token changeset to address this, and probably not perfect to demonstrate these thoughts, but I just wanted to make the point that we will be sending a message whichever direction we take with this changeset.)

[Jens]

IMHO it's a small enough addition that totally makes sense and it's nothing that would block any future direction of GWT. I don't see any real reason to not accept the contribution.

The points mentioned are either clunky (pull in a complete different library just to get a single small feature or use some workarounds), won't happen anytime soon (replacing GWT SafeHtml with safe-html-types or wrap it around) or are simply pointless (J2CL not released, no fully worked out / discussed future plan for GWT 3.0). 

The only valid point might be code size increase and runtime performance, but thats a valid point for every contribution and should simply be worked out until it's acceptable.

IMHO contributions shouldn't be handled too picky as long as there is no concrete plan for GWT 3.0. Nowadays we have GWT 2.8 and a contributor wants to improve it, that's it, very simple. I'll probably give +1 after rethinking the code size / performance point and a maintainer for SafeHtml should give +2 once the code is fine.

-- J.