http://gwt-code-reviews.appspot.com/1384801/diff/3003/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/3003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode80
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:80:
public void testTemplateWithCssAttribute() {
On 2011/03/14 18:25:38, skybrian wrote:
On 2011/03/14 17:03:18, jlabanca wrote:
> The & character is valid because it can be used in a URL:
> background:url(http://url?image=123112&size=1024)
I'm wondering if this is more correct:
background:url(http://url?image=123112&size=1024);
Similarly, can we use other HTML entities like " within a
SafeCssProperties
string?
I'm out of my element here. Is our assertion that users should not use
single or double quotes? And will %26 escaping URLs might be better, it
certainly isn't invalid to use & (I think). I'm not quite sure what I
should be testing here since we aren't actually providing a
SafeCssProperties escaping feature yet.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/cell/client/IconCellDecorator.java
File user/src/com/google/gwt/cell/client/IconCellDecorator.java (right):
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/cell/client/IconCellDecorator.java#newcode196
user/src/com/google/gwt/cell/client/IconCellDecorator.java:196: String
cssString = direction + ":0px;";
On 2011/03/14 18:25:38, skybrian wrote:
To remove a bit of duplication, maybe make this variable have type
SafeCssProperties?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/cell/client/IconCellDecorator.java#newcode203
user/src/com/google/gwt/cell/client/IconCellDecorator.java:203:
cssString += "margin-top:-" + halfHeight + "px;";
On 2011/03/14 18:25:38, skybrian wrote:
We should have some way of concatenating two variables of type
SafeCssProperties.
Done.
I added SafeCssPropertiesBuilder and used it here and in other places.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java
File user/src/com/google/gwt/safecss/shared/SafeCssProperties.java
(right):
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java#newcode107
user/src/com/google/gwt/safecss/shared/SafeCssProperties.java:107: * -
Some implementations of SafeCssProperties would in principle be able to
On 2011/03/14 21:07:51, xtof wrote:
This sentence got formatted wrong?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/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/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode185
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:185:
// SafeCss is safe in a CSS context, so we can use the string without
On 2011/03/14 21:07:51, xtof wrote:
I think we should HTML escape the values of style= attributes; this
will prevent
the attribute-escape bug that's mentioned in the SafeHtmlProperties
documentation.
Unless I'm totally confused, the browser will first HTML-unescape the
style
attribute and then pass it to the CSS parser, i.e. HTML escaping
quotes and
single quotes (and <> markup) will not change behavior, but will make
sure the
HTML attribute is correctly parsed in its entirety.
Done.
I didn't realize the browser would unescape it. Escaping seems to work
fine.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode264
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:264:
*
On 2011/03/14 21:07:51, xtof wrote:
Trailing blank, here and below...
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode284
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:284:
* Verify that the parameter type if used in the correct context. Safe
On 2011/03/14 21:07:51, xtof wrote:
"is used"?
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode328
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:328:
// Warn against using unsafe parameters in a CSS attribute context.
On 2011/03/14 21:07:51, xtof wrote:
It's worth noting that even if a SafeCssProperties is used, the
template isn't
100% guaranteed to be safe, because we don't know if the template
variable
appears in a "composable" CSS context. For example,
@Template("<div style=\"background:url('{0}')\">"
SafeHtml div(SafeCssProperties safeCss);
will not result in a compiler warning, and _could_ potentially mask an
exploitable bug -- since CSS quotes in the parameter are "inside out",
a CSS
property escape could happen, e.g if the value of CSS is something
like,
http://foo%27; background:url(javascript:evil()); font'
Now, it's very unlikely that the code that constructs safeCss will
allow this,
but the potential exists.
There are a few ways to clean this up:
- Augment the stream parser to dive into CSS and determine CSS context
(I'm not
sure how hard this is; presumably not easy, otherwise they'd done it)
- Only allow template variables in CSS_ATTRIBUTE context to appear at
the very
beginning of the style attribute (without warning). This by definition
is a
composable CSS context we can be sure of. This smells a bit awkward,
but might
be an overall reasonable solution.
Options 2 sounds more reasonable. I think digging into option 1 would
require us to encode a lot of complicated rules.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/shared/SafeHtml.java
File user/src/com/google/gwt/safehtml/shared/SafeHtml.java (right):
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/shared/SafeHtml.java#newcode63
user/src/com/google/gwt/safehtml/shared/SafeHtml.java:63: * Notes
regarding serialization: - It may be reasonable to allow
On 2011/03/14 21:07:51, xtof wrote:
This got formatted funny as well at some point.
Done.
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/user/cellview/client/CellTree.java
File user/src/com/google/gwt/user/cellview/client/CellTree.java (right):
http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/user/cellview/client/CellTree.java#newcode954
user/src/com/google/gwt/user/cellview/client/CellTree.java:954:
cssBuilder.append("width: " + res.getWidth() + "px;");
On 2011/03/14 21:07:51, xtof wrote:
I wonder if code like this might be a bit more readable if
SafeCssPropertiesBuilder was used. Perhaps to avoid excessive
verbosity, we
could have
safeCssBuilder.appendFromTrustedString("width:" + .. + "px;")
as a shorthand for
safeCssBuilder.append(SafeCssUtils.fromTrustedString(...));
The advantage is that the code can then be written such that each
append adds a
single CSS property, and to code review for correctness I have to only
eyeball
each .append individually rather than having to consider the whole
builder.
Perhaps not a big deal in practice though.
Done.
http://gwt-code-reviews.appspot.com/1384801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
