Re: OpenSocial 1.0 spec patch: Security policy and EL escaping (issue154162)
le...@google.com
http://codereview.appspot.com/154162/diff/1/2
File OpenSocial-Templating.xml (right):
http://codereview.appspot.com/154162/diff/1/2#newcode122
OpenSocial-Templating.xml:122: The Security Policy feature is used by an
app to request permissions under
app -> application
http://codereview.appspot.com/154162/diff/1/2#newcode130
OpenSocial-Templating.xml:130: <Param name="el_escaping"
>auto</Param>
Remove extra space between " and >
http://codereview.appspot.com/154162/diff/1/2#newcode138
OpenSocial-Templating.xml:138: Permissions are specified by using
<Param > elements to identify
A few lines above, you write "with Param elements" without the angle
brackets. Please to be consistent.
http://codereview.appspot.com/154162/diff/1/2#newcode140
OpenSocial-Templating.xml:140: A container may override any
app-requested permission with its own security policy.
app -> application
I wonder if this makes sense. If an application expects to be rendered
with "none" escaping, and the container doesn't support it, it's
possible that the application cannot be properly displayed at all. We
should explicitly say what happens when an unsupported policy is asked
for.
http://codereview.appspot.com/154162/diff/1/2#newcode147
OpenSocial-Templating.xml:147: <t>safe - Allow a filtered subset of
markup within the EL statement. Strip out any markup deemed unsafe. It
is up to the container to define what subset of markup is safe.</t>
+1 on dropping "safe" for now - this can currently be supported with
<os:Html>
http://codereview.appspot.com/154162/diff/1/2#newcode148
OpenSocial-Templating.xml:148: <t>none - No escaping rules applied to EL
statements. String responses will be emitted in their raw form,
inclusive of markup.</t>
This sentence is a bit awkward. How about:
none - Disables automatic escaping. EL expression results will be
emitted raw, any markup will be rendered.
http://codereview.appspot.com/154162
goosemanjack
completeness.
Lev Epshteyn
--
You received this message because you are subscribed to the Google Groups "OpenSocial and Gadgets Specification Discussion" group.
To post to this group, send email to opensocial-an...@googlegroups.com.
To unsubscribe from this group, send email to opensocial-and-gadg...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en.
Evan Gilbert
Tosh Meston
> >> opensocial-and-gadg...@googlegroups.com<opensocial-and-gad gets-spec%2Bunsu...@googlegroups.com>
> >> .
> >> For more options, visit this group at
> >>http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en.
>
> > --
> > You received this message because you are subscribed to the Google Groups
> > "OpenSocial and Gadgets Specification Discussion" group.
> > To post to this group, send email to
> > opensocial-an...@googlegroups.com.
> > To unsubscribe from this group, send email to
> > opensocial-and-gadg...@googlegroups.com<opensocial-and-gad gets-spec%2Bunsu...@googlegroups.com>
Adam Winer
apologies for not looking at this.
I'm mostly +1, but am -1 on some details of the EL functions:
+<list style="hanging">
+ <t hangText="os:htmlEncode">
+Encode HTML markup entities. At minimum escape greater-than and
less-than symbols and quotation marks to their encoded
equivilents.</t>
Typo, equivilents -> equivalents.
+ <t hangText="os:htmlDecode">
+Unescape an HTML-encoded string
+ </t>
+ <t hangText="os:urlEncode">
+URL-encode the contained string
+ </t>
This isn't specific enough to allow compatible implementation. Are we
encoding a full URL? Or just a parameter? And what character set is
used for encoding?
For example, if we have:
<a href="http://example.com?foo=${os:urlEncode(someExpression)}&debug=1"/>
... then it's essential to encode '&' in someExpression to %26.
But if there's:
<a href="${os:urlEncode(someExpression)}"/>
Then you'd absolutely have to *not* encode '&' to %26.
Context-sensitivity here is undesirable so we either need to pick one
or have two functions (os:urlEncode and os:urlParamEncode). I'd
recommend having just os:urlParamEncode, or if we need both behaviors
then two functions.
Also, the character set matters - should we simply say UTF-8, or
should we use the charset of the generated content?
+ <t hangText="os:urlDecode">
+URL-decode the contained string
+ </t>
+ <t hangText="os:filterUrl">
+Apply rules to make the URL safe for inclusion in markup.
Recommendations are to disallow javascript and apply anti-phishing
mechanisms.
+ </t>
+ <t hangText="os:jsStringEscape">
+Escape string for inclusion within JavaScript block inside quotes.
There is an optional second parameter to specify that the string
should be escaped for inclusion within single quotes when set to true.
Why not just always escape both ' and ", and save the need for the
extra parameter? Boolean parameters are a royal pain - there's no
obvious way to remember whether you use true for ' or for ", so it's
easy to get wrong, and hard to visually audit.
+ <figure>
+ <artwork xml:space="preserve">
+var x = "${os:jsStringEscape(someVar)}";
+var y = '${os:jsStringEscape(someVar, true)}';
+</artwork>
I'm also -0 on the need for "decode" functions; I'm fine with having
them there for symmetry, but don't specifically know of use cases.
Apologies if that's been discussed and I've missed it.
> To unsubscribe from this group, send email to opensocial-and-gadg...@googlegroups.com.
Lev Epshteyn
goosemanjack
to mirror the behavior of JavaScript window.escape function. Adam,
you're also correct on your other comments. I'll update the patch to
reflect your feedback.
On Jan 5, 9:30 am, Lev Epshteyn <le...@google.com> wrote:
> Generally like Adam's suggestions. (spefically, I agree we need to encode
> URL params much more often than whole URLs, and that decoding is not
> necessary).
>
> On Sun, Dec 20, 2009 at 9:03 PM, Adam Winer <awi...@gmail.com> wrote:
> > I've been out of town for the past week, and generally busy -
> > apologies for not looking at this.
>
> > I'm mostly +1, but am -1 on some details of the EL functions:
>
> > +<list style="hanging">
> > + <t hangText="os:htmlEncode">
> > +Encode HTML markup entities. At minimum escape greater-than and
> > less-than symbols and quotation marks to their encoded
> > equivilents.</t>
>
> > Typo, equivilents -> equivalents.
>
> > + <t hangText="os:htmlDecode">
> > +Unescape an HTML-encoded string
> > + </t>
> > + <t hangText="os:urlEncode">
> > +URL-encode the contained string
> > + </t>
>
> > This isn't specific enough to allow compatible implementation. Are we
> > encoding a full URL? Or just a parameter? And what character set is
> > used for encoding?
>
> > For example, if we have:
>
> > <a href="http://example.com?foo=${os:urlEncode(someExpression)}&debug=1<http://example.com?foo=$%7Bos:urlEncode%28someExpression%29%7D&debug=1>
> > On Wed, Dec 16, 2009 at 5:13 PM, Tosh Meston <tosh.mes...@gmail.com>
> > >> >> opensocial-and-gadg...@googlegroups.com<opensocial-and-gadgets-spec%2Bunsu...@googlegroups.com><opensocial-and-gad
> > gets-spec%2Bunsu...@googlegroups.com<gets-spec%252Buns...@googlegroups.com>
>
> > >> >> .
> > >> >> For more options, visit this group at
> > >> >>http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en.
>
> > >> > --
> > >> > You received this message because you are subscribed to the Google
> > Groups
> > >> > "OpenSocial and Gadgets Specification Discussion" group.
> > >> > To post to this group, send email to
> > >> > opensocial-an...@googlegroups.com.
> > >> > To unsubscribe from this group, send email to
> > >> > opensocial-and-gadg...@googlegroups.com<opensocial-and-gadgets-spec%2Bunsu...@googlegroups.com><opensocial-and-gad
> > gets-spec%2Bunsu...@googlegroups.com<gets-spec%252Buns...@googlegroups.com>
goosemanjack
and left the "decode" methods in for now. I've come across scenarios
where sometime get double encoded while being carried across the
various server hops. The decode functions have proven useful in these
scenarios where we're compensating for some external encoding bug.
--
clc
goosemanjack
pointed out, window.escape has been deprecated.
Lane, should I be committing this patch as well?
Lane LiaBraaten
To unsubscribe from this group, send email to opensocial-and-gadg...@googlegroups.com.
goosemanjack
> > <opensocial-and-gadgets-spec%2Bunsu...@googlegroups.com<opensocial-and-gadgets-spec%252Buns...@googlegroups.com>
> > ><opensocial-and-gad
> > > > > > gets-spec%2Bunsu...@googlegroups.com<gets-spec%252Buns...@googlegroups.com>
> > <gets-spec%252Buns...@googlegroups.com<gets-spec%25252Bun...@googlegroups.com>
>
> > > > > > >> >> .
> > > > > > >> >> For more options, visit this group at
>
> >http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en.
>
> > > > > > >> > --
> > > > > > >> > You received this message because you are subscribed to the
> > Google
> > > > > > Groups
> > > > > > >> > "OpenSocial and Gadgets Specification Discussion" group.
> > > > > > >> > To post to this group, send email to
> > > > > > >> > opensocial-an...@googlegroups.com.
> > > > > > >> > To unsubscribe from this group, send email to
> > > > > > >> > opensocial-and-gadg...@googlegroups.com<opensocial-and-gadgets-spec%2Bunsu...@googlegroups.com>
> > <opensocial-and-gadgets-spec%2Bunsu...@googlegroups.com<opensocial-and-gadgets-spec%252Buns...@googlegroups.com>
> > ><opensocial-and-gad
> > > > > > gets-spec%2Bunsu...@googlegroups.com<gets-spec%252Buns...@googlegroups.com>
> > <gets-spec%252Buns...@googlegroups.com<gets-spec%25252Bun...@googlegroups.com>
>
> ...
>
> read more »
