OpenSocial 1.0 spec patch: Security policy and EL escaping

Adding Security Policy and permission to control EL escaping behavior

[Lane LiaBraaten, 2009-12-09 21:23:06]

I'm +1 on this after the comments are addressed.

-Lane

http://codereview.appspot.com/154162/diff/1/2
File OpenSocial-Templating.xml (right):

http://codereview.appspot.com/154162/diff/1/2#newcode129
OpenSocial-Templating.xml:129: <Optional feature="SecurityProfile" >
Can a gadget optionally depend on this?  It seems like they need to either
expect output to be escaped or expect it to not be escaped.

http://codereview.appspot.com/154162/diff/1/2#newcode129
OpenSocial-Templating.xml:129: <Optional feature="SecurityProfile" >
SecurityProfile --> security-policy

http://codereview.appspot.com/154162/diff/1/2#newcode130
OpenSocial-Templating.xml:130: <Param name="el_escaping"
>auto</Param>
auto --> html

http://codereview.appspot.com/154162/diff/1/2#newcode146
OpenSocial-Templating.xml:146: <t>html - Automatically HTML encode all EL
statements.</t>
Per Evan's comments:
MUST: HTML escape all elements and attributes
SHOULD: Prevent any EL statement from executing JavaScript code.

http://codereview.appspot.com/154162/diff/1/2#newcode146
OpenSocial-Templating.xml:146: <t>html - Automatically HTML encode all EL
statements.</t>
mention that html is the default.

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>
I think we can drop 'safe' mode for this iteration.  All or nothing seems like a
reasonable place to start.

http://codereview.appspot.com/154162/diff/1/2#newcode152
OpenSocial-Templating.xml:152: Escaping rules specified with the "el_escaping"
parameter are applied to all views.  An app may request a different escaping
policy for a specific view by specifying the view name as a suffix with a
preceeding colon to the el_escaping name.  For example, to specify auto escaping
on the '''profile''' view and no escaping for all other views, the following is
used:
change triple quotes to single quotes

http://codereview.appspot.com/154162/diff/1/2#newcode156
OpenSocial-Templating.xml:156: &lt;Optional feature="SecurityProfile" &gt;
SecurityProfile --> security-policy

http://codereview.appspot.com/154162/diff/1/2#newcode157
OpenSocial-Templating.xml:157: &lt;Param name="el_escaping:profile"
&gt;auto&lt;/Param&gt;
auto --> html

http://codereview.appspot.com/154162/diff/1/2#newcode165
OpenSocial-Templating.xml:165: Containers SHOULD apply HTML escaping by default
to all views that are not otherwise constrained or protected by a security
mechanism (ex: jail domained IFrame, trusted parter, pre-filtered content).
parter --> partner

http://codereview.appspot.com/154162/diff/1/2#newcode274
OpenSocial-Templating.xml:274: <list style="symbols">
This will probably look better as a style="hanging" table:

<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>
...
</list>

http://codereview.appspot.com/154162/diff/1/2#newcode290
OpenSocial-Templating.xml:290: 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.
javascript --> JavaScript

[goosemanjack, 2009-12-11 20:44:11]

I'm out of town right now, but will try to get an updated patch that
addresses the comments.  We'll shoot for over the weekend.


On Wed, Dec 9, 2009 at 4:23 PM, <lliabraa@google.com> wrote:

> I'm +1 on this after the comments are addressed.
>
> -Lane
>
>
> http://codereview.appspot.com/154162/diff/1/2
> File OpenSocial-Templating.xml (right):
>
> http://codereview.appspot.com/154162/diff/1/2#newcode129
> OpenSocial-Templating.xml:129: &lt;Optional feature="SecurityProfile"
> &gt;
> Can a gadget optionally depend on this?  It seems like they need to
> either expect output to be escaped or expect it to not be escaped.
>
> http://codereview.appspot.com/154162/diff/1/2#newcode129
> OpenSocial-Templating.xml:129: &lt;Optional feature="SecurityProfile"
> &gt;
> SecurityProfile --> security-policy
>
> http://codereview.appspot.com/154162/diff/1/2#newcode130
> OpenSocial-Templating.xml:130: &lt;Param name="el_escaping"
> &gt;auto&lt;/Param&gt;
> auto --> html
>
> http://codereview.appspot.com/154162/diff/1/2#newcode146
> OpenSocial-Templating.xml:146: <t>html - Automatically HTML encode all
> EL statements.</t>
> Per Evan's comments:
> MUST: HTML escape all elements and attributes
> SHOULD: Prevent any EL statement from executing JavaScript code.
>
> http://codereview.appspot.com/154162/diff/1/2#newcode146
> OpenSocial-Templating.xml:146: <t>html - Automatically HTML encode all
> EL statements.</t>
> mention that html is the default.
>
> 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>
> I think we can drop 'safe' mode for this iteration.  All or nothing
> seems like a reasonable place to start.
>
> http://codereview.appspot.com/154162/diff/1/2#newcode152
> OpenSocial-Templating.xml:152: Escaping rules specified with the
> "el_escaping" parameter are applied to all views.  An app may request a
> different escaping policy for a specific view by specifying the view
> name as a suffix with a preceeding colon to the el_escaping name.  For
> example, to specify auto escaping on the '''profile''' view and no
> escaping for all other views, the following is used:
> change triple quotes to single quotes
>
> http://codereview.appspot.com/154162/diff/1/2#newcode156
> OpenSocial-Templating.xml:156: &lt;Optional feature="SecurityProfile"
> &gt;
> SecurityProfile --> security-policy
>
> http://codereview.appspot.com/154162/diff/1/2#newcode157
> OpenSocial-Templating.xml:157: &lt;Param name="el_escaping:profile"
> &gt;auto&lt;/Param&gt;
> auto --> html
>
> http://codereview.appspot.com/154162/diff/1/2#newcode165
> OpenSocial-Templating.xml:165: Containers SHOULD apply HTML escaping by
> default to all views that are not otherwise constrained or protected by
> a security mechanism (ex: jail domained IFrame, trusted parter,
> pre-filtered content).
> parter --> partner
>
> http://codereview.appspot.com/154162/diff/1/2#newcode274
> OpenSocial-Templating.xml:274: <list style="symbols">
> This will probably look better as a style="hanging" table:
>
> <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>
> ...
> </list>
>
> http://codereview.appspot.com/154162/diff/1/2#newcode290
> OpenSocial-Templating.xml:290: 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.
> javascript --> JavaScript
>
> http://codereview.appspot.com/154162
>

[levik, 2009-12-14 20:14:06]

Agree with Lane's comments, plus some of my own mostly minor nits.

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.

[goosemanjack, 2009-12-15 17:28:42]

Updated to address reviewer comments

[goosemanjack, 2010-01-11 18:46:17]

Updated to clarify built-in function behavior and remove error in jsStringEscape

[uidude, 2010-01-12 16:38:23]

http://codereview.appspot.com/154162/diff/7001/8001
File OpenSocial-Templating.xml (right):

http://codereview.appspot.com/154162/diff/7001/8001#newcode286
OpenSocial-Templating.xml:286: URL-encode the contained string.  The behavior
should mirror the JavaScript function window.escape.
encodeURIComponent (& decodeURIComponent) possibly? escape is being depreceated
and apparently doesn't work well for non-ASCII characters.

[goosemanjack, 2010-01-12 22:17:01]

Using encodeURIComponent instead of deprecated window.escape