Security updates to RPC

Add security features to the RPC layer, as described here:
http://wiki.opensocial.org/index.php?title=PubSub.next_Proposals#Secure_rpc.

[Paul Lindner, 2010-08-30 06:46:55]

patch applied, some concerns.  thanks!

http://codereview.appspot.com/1983044/diff/1/3
File features/src/main/javascript/features/rpc/rpc.js (left):

http://codereview.appspot.com/1983044/diff/1/3#oldcode550
features/src/main/javascript/features/rpc/rpc.js:550: if (!gadgets.util) {
Lots of opt_* here, time to consider opt_params?

http://codereview.appspot.com/1983044/diff/1/3
File features/src/main/javascript/features/rpc/rpc.js (right):

http://codereview.appspot.com/1983044/diff/1/3#newcode207
features/src/main/javascript/features/rpc/rpc.js:207: var mainPageUnloading =
false,
This sounds really bad, can you elaborate on this

http://codereview.appspot.com/1983044/diff/1/3#newcode488
features/src/main/javascript/features/rpc/rpc.js:488: // make URL absolute if
necessary
Is this necessary since we make the URL absolute in the code that calls
setRelayUrl()

http://codereview.appspot.com/1983044/diff/1/3#newcode991
features/src/main/javascript/features/rpc/rpc.js:991: FORGED_MSG : FORGED_MSG
these are error responses.  Might be better to have common prefix so someone
knows what these are for..

[Javier Pedemonte, 2010-08-30 14:42:22]

On 2010/08/30 06:46:55, Paul Lindner wrote:
> http://codereview.appspot.com/1983044/diff/1/3#oldcode550
> features/src/main/javascript/features/rpc/rpc.js:550: if (!gadgets.util) {
> Lots of opt_* here, time to consider opt_params?

Sure, setupChildFrame() is an internal function, so changing that is fine. 
However, setupReceiver() (which has similar params) is a public function.  If we
decide to change that one as well, we'll have to stay backwards-compatible with
users who are calling setupReceiver() as currently defined.

> http://codereview.appspot.com/1983044/diff/1/3#newcode207
> features/src/main/javascript/features/rpc/rpc.js:207: var mainPageUnloading =
> false,
> This sounds really bad, can you elaborate on this

Just as stated: setting an unload handler on a page prevents the page from being
cached in memory (but not on disk).  See
http://webkit.org/blog/516/webkit-page-cache-ii-the-unload-event/.

This is only set in the RPC code if the user has set a valid relay URL.  So this
becomes a choice: don't provide a relay URL and get use of RPC, but not fully
secure; or provide a relay, get full security, but it disables the use of the
fast-back cache in the browser.

> http://codereview.appspot.com/1983044/diff/1/3#newcode488
> features/src/main/javascript/features/rpc/rpc.js:488: // make URL absolute if
> necessary
> Is this necessary since we make the URL absolute in the code that calls
> setRelayUrl()

This is not the case for every call.  In shindig-container.js, setRelayUrl() by
default is called with "/gadgets/...".

> http://codereview.appspot.com/1983044/diff/1/3#newcode991
> features/src/main/javascript/features/rpc/rpc.js:991: FORGED_MSG : FORGED_MSG
> these are error responses.  Might be better to have common prefix so someone
> knows what these are for..

Something like SEC_ERROR_FORGED_MSG?

[Paul Lindner, 2010-08-30 18:13:01]

Thanks, +1 to using something like SEC_ERROR_*

Can we use pageShow/pageHide events instead to avoid breaking the memory cache?

I'd also add a reference to the webkit documentation too.

for now the multitude of opt_* params is okay.  At a later time a refactor for
this might be appropriate.

if the url is made absolute in setRelayUrl then we should be able to remove the
duplicate functionality in the layer above then.


On 2010/08/30 14:42:22, Javier Pedemonte wrote:
> On 2010/08/30 06:46:55, Paul Lindner wrote:
> > http://codereview.appspot.com/1983044/diff/1/3#oldcode550
> > features/src/main/javascript/features/rpc/rpc.js:550: if (!gadgets.util) {
> > Lots of opt_* here, time to consider opt_params?
> 
> Sure, setupChildFrame() is an internal function, so changing that is fine. 
> However, setupReceiver() (which has similar params) is a public function.  If
we
> decide to change that one as well, we'll have to stay backwards-compatible
with
> users who are calling setupReceiver() as currently defined.
> 
> > http://codereview.appspot.com/1983044/diff/1/3#newcode207
> > features/src/main/javascript/features/rpc/rpc.js:207: var mainPageUnloading
=
> > false,
> > This sounds really bad, can you elaborate on this
> 
> Just as stated: setting an unload handler on a page prevents the page from
being
> cached in memory (but not on disk).  See
> http://webkit.org/blog/516/webkit-page-cache-ii-the-unload-event/.
> 
> This is only set in the RPC code if the user has set a valid relay URL.  So
this
> becomes a choice: don't provide a relay URL and get use of RPC, but not fully
> secure; or provide a relay, get full security, but it disables the use of the
> fast-back cache in the browser.
> 
> > http://codereview.appspot.com/1983044/diff/1/3#newcode488
> > features/src/main/javascript/features/rpc/rpc.js:488: // make URL absolute
if
> > necessary
> > Is this necessary since we make the URL absolute in the code that calls
> > setRelayUrl()
> 
> This is not the case for every call.  In shindig-container.js, setRelayUrl()
by
> default is called with "/gadgets/...".
> 
> > http://codereview.appspot.com/1983044/diff/1/3#newcode991
> > features/src/main/javascript/features/rpc/rpc.js:991: FORGED_MSG :
FORGED_MSG
> > these are error responses.  Might be better to have common prefix so someone
> > knows what these are for..
> 
> Something like SEC_ERROR_FORGED_MSG?

[Javier Pedemonte, 2010-08-31 00:20:12]

On 2010/08/30 18:13:01, Paul Lindner wrote:
> Can we use pageShow/pageHide events instead to avoid breaking the memory
cache?

No, because we don't really care about the unloading/hiding of the main page. 
We are only concerned about an iframed gadget unloading.  For that, only an
unload event will work.  And attaching an unload event to an iframe has the same
side-effect as attaching the event to the main page:  it disables the memory
cache for the page.

> I'd also add a reference to the webkit documentation too.

OK.

> if the url is made absolute in setRelayUrl then we should be able to remove
the
> duplicate functionality in the layer above then.

Is there some specific code you have in mind here?  I only see a few references
to setRelayUrl(), and only one of them will always pass in an absolute value (in
gadget_holder.js, since it uses shindig.uri).  All the others can take absolute
or relative URLs, depending on what the user passes in or how the iframe is
setup.

I'll have a new patch up shortly.