On Wed, Mar 30, 2011 at 01:47, <[email protected]> wrote: > On 2011/03/29 20:38:29, xtof wrote: > >> > >> > - Implement the public Image(String url,...) constructors in >> > terms of the Image(SafeUri url, ...) ones, using >> > fromUntrustedString. >> > >> > I'm really not sure about that last one, as the getUrl would >> > still have to be a String for backwards compat', and the >> > DOM-level accesses are all String-based too. >> > >> I see. Since ClippedState is strictly internal, it probably >> doesn't make a big difference. I think for the time being it >> would also be fine to leave as it is currently, but replace >> fromTrustedString with fromUntrustedString and add some comments >> about what's going on. Just to avoid the impression >> that the code guarantees something that it really can't guarantee. >> > > Thinking a bit more about it, how about keeping the use of > fromTrustedString, not introducing fromUntrustedString, and instead just > documenting that we're using fromTrustedString here for backwards > compatibility only, and cannot guarantee the URL safety? > The advantage of using a separate method is that it helps code reviews: When I do a security code review, I need to find all the instances of what are effectively unsafe casts to SafeUri where the programmer intended to make the promise that the value is safe; I need to review the code to check that assumption. In the "legacy" unsafe cast, there really isn't anything to review -- it just means there are code paths that are going to be safe until the API is fully refactored. I guess I'd want to review those too, but with a different angle. Using two different methods helps the review, since I can just use the IDE's show call hierarchy function to find them (or grep).
...and adding docs to all the String-based (public) methods about the > caller having to care about security and either sanitize the URI or use > the SafeUri-based overload? > (note however that no such doc was added to setHTML when setSafeHtml was > introduced) I think the difference to SafeHtml is that here, we're actually changing the internal implementation to use SafeUri, but we still have an external method that takes a plain String. For widgets that support setHTML(SafeHtml), and setHTML(String), the situation is a bit simpler since each typically either passes the value down to an inner widget that takes SafeHtml/String, respectively, or ends up calling setInnerHTML, which just takes String. If we for example had a complicated widget that is refactored to use SafeHtmlBuilder intenally, but still exposes a setHTML(String) method, we'd probably have to similarly introduce a such an "unsafe cast" method. BTW, I'm still working on the ENTIRE_URL_ATTRIBUTE cl. Hoping to get that out this morning. cheers, --xtof > > http://gwt-code-reviews.appspot.com/1380806/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
