http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java
File user/src/com/google/gwt/safecss/shared/SafeStyles.java (right):
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java#newcode71
user/src/com/google/gwt/safecss/shared/SafeStyles.java:71: * attribute:
On 2011/04/06 21:51:17, xtof wrote:
Maybe instead say, "comply with this type's contract" (here and
elsewhere) --
these examples are not inherently unsafe, they just don't comply with
the type
contract (specifically, the composability requirement).
With that in mind, perhaps move the examples after the paragraph about
composability?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java#newcode88
user/src/com/google/gwt/safecss/shared/SafeStyles.java:88: * {@code
http://trackingurl.com)"} is appended to {@code background:url("}, the
On 2011/04/06 21:51:17, xtof wrote:
Hmm, so generally the SafeHtml contracts don't guarantee that the
resulting HTML
is side-effect free; in particular when we sanitize untrusted URIs
we'll leave
them alone as long as they have a whitelisted scheme. I.e. we'd allow
<img
src='{0}'> where the value of {0} is http://trackingurl.com/
Complete side effect freedom in this sense would be much harder to
enforce (we'd
have to somehow have a way to configure filters that know which
domains are
"trusted" and which ones are third-party).
Perhaps a better example would be to use "javascript:evil()" as a url?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java
File user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java#newcode42
user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java:42: * any
escaping to it.
On 2011/04/06 21:51:17, xtof wrote:
Perhaps, "any escaping or sanitization"?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode131
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:131:
* <li>If the parameter is not of type {@link SafeStyles}, the result
is then
On 2011/04/06 21:51:17, xtof wrote:
I think this comment should remain unchanged; we always HTML escape
the
attribute value even if it came from a SafeStyles.
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode176
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:176:
// TODO(xtof): Throw an exception if using SafeHtml within an attribute.
On 2011/04/06 21:51:17, xtof wrote:
I think you can remove this TODO, isn't this handled now in your new
checks at
the start of emitParameterExpression?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode280
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:280:
* definitely isn't what the user intended to do.
On 2011/04/06 21:51:17, xtof wrote:
Maybe "user" -> "developer"?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode315
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:315:
logger.log(TreeLogger.WARN,
I added a comment explaining that we've already checked that the user
did not try to use SafeStyles in a non-CSS context.
The reason for the split checks is that each Safe type is only valid in
one context, so it makes sense to check that separately. Otherwise, we
would have to have a !SafeHtml/!SafeStyles check in every context where
those classes aren't supported.
Conversely, some contexts have or will have a Safe value that is
appropriate for that context. Adding the warning in the switch
statement allows us to do check if the user could have used a Safe class
specific to the current context. If we move the warnings out of the
switch statement, then we would have to check the current context to see
if the user could have chosen a better Safe value, which would basically
add a duplicate (subset) switch statement.
On 2011/04/06 21:51:17, xtof wrote:
I find the code is a bit hard to understand with the checks related to
CSS_ATTRIBUTE contexts partially done here, and partially done up
above in line
270ff. Might it be more readable if you move the checks from above to
here
(possibly splitting the two cases)? Not sure on this; might make the
code more
verbose...
Otherwise, a comment noting the precondition established by the check
in 270ff
might help.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/user/cellview/client/CellBrowser.java
File user/src/com/google/gwt/user/cellview/client/CellBrowser.java
(right):
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/user/cellview/client/CellBrowser.java#newcode728
user/src/com/google/gwt/user/cellview/client/CellBrowser.java:728:
private static final SafeHtml LEAF_IMAGE = SafeHtmlUtils
On 2011/04/06 21:51:17, xtof wrote:
Maybe break after = ?
Done. Blame auto format.
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
(right):
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode60
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:60:
SafeHtml templateWithCssAttribute(int height /* generates a compiled
time warning */,
On 2011/04/06 21:51:17, xtof wrote:
"compile time"
Done.
I also realized that the tests now fail because SafeStyles can only be
used at the start of the attribute. I updated the tests accordingly.
http://gwt-code-reviews.appspot.com/1384801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
