You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Dan Dumont <dd...@us.ibm.com> on 2011/10/03 16:26:22 UTC
Re: Review Request: Locked domain cleanup and shared-domain-locking feature
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2025/
-----------------------------------------------------------
(Updated 2011-10-03 14:26:22.765398)
Review request for shindig, Paul Lindner, johnfargo, Ryan Baxter, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Updated change after Ryan's last commit. Some test changes and minor rewiring to account for the blacklist replacement.
Summary
-------
Sorry for the crazy diffs here. Much stuff has moved around.
This is the cleanup part of the patch, I want a few good eyes first before I move on to the feature work.
Some highlights:
* org.apache.shindig.gadgets.uri.DefaultIframeUriManager
Nearly everything relating to locked domains has been moved to org.apache.shindig.gadgets.LockedDomainService
Removed validation on uri for locked domains from this class. It was never actually used.
* org.apache.shindig.gadgets.uri.ProxyUriBase
Removed check for INVALID_DOMAIN, nothing in the code paths leading there ever set that status.
* org.apache.shindig.gadgets.uri.UriStatus
Removed INVALID_DOMAIN, it was not used anymore. This class seems more focused on caching anyway.
* org.apache.shindig.gadgets.HashLockedDomainService
Implemented new methods added to interface. Renamed some methods for clarity and java convention.
Augmented some existing implementation from code that used to be in
org.apache.shindig.gadgets.uri.DefaultIframeUriManager
For documentation purposes:
I looked through what appears to be 3 proxies.
* Content proxy - gadgets.io.getProxyUrl
* makeRequest proxy - gadgets.io.makeRequest
* RPC Proxy - osapi.http.get
Content proxy denies all requests to a locked domain by default. It's assumed that it's configured on a url that would ensure it is only used for things like image or sctipt tags, etc.
makeRequest does not appear to do any locked domain checking to make sure the gadget is valid for the locked domain. While it's reasonable to assume a malicious gadget will not use the locked domain url of another gadget, it's possible it could craft a request to the proxy on its own locked domain and forge the gadget passed in to appear as another gadget. I'll be making changes to this proxy to include locked domain validation.
RPC Proxy appears to be made from the container on behalf of a gadget, the gadget passed in should be legitimate. I have not tried to make this request on a locked domain to see if the proxy will respond. (Gadget pretending to be the container making the request)
This addresses bug SHINDIG-1628.
https://issues.apache.org/jira/browse/SHINDIG-1628
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RendererTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestServletTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletTestFixture.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManagerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriManagerTestBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/ContainerConfigTest.php 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/LockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriStatus.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/HashLockedDomainServiceTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/Renderer.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/LockedDomainService.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo1.xml PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo2.xml PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shared-script-frame/shared-script-frame-container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetException.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java 1178412
Diff: https://reviews.apache.org/r/2025/diff
Testing
-------
Thanks,
Dan
Re: Review Request: Locked domain cleanup and shared-domain-locking feature
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2025/
-----------------------------------------------------------
(Updated 2011-10-03 19:58:37.102473)
Review request for shindig, Paul Lindner, johnfargo, Ryan Baxter, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
removed @override annotations so java 1.5 won't break.
Summary
-------
Sorry for the crazy diffs here. Much stuff has moved around.
This is the cleanup part of the patch, I want a few good eyes first before I move on to the feature work.
Some highlights:
* org.apache.shindig.gadgets.uri.DefaultIframeUriManager
Nearly everything relating to locked domains has been moved to org.apache.shindig.gadgets.LockedDomainService
Removed validation on uri for locked domains from this class. It was never actually used.
* org.apache.shindig.gadgets.uri.ProxyUriBase
Removed check for INVALID_DOMAIN, nothing in the code paths leading there ever set that status.
* org.apache.shindig.gadgets.uri.UriStatus
Removed INVALID_DOMAIN, it was not used anymore. This class seems more focused on caching anyway.
* org.apache.shindig.gadgets.HashLockedDomainService
Implemented new methods added to interface. Renamed some methods for clarity and java convention.
Augmented some existing implementation from code that used to be in
org.apache.shindig.gadgets.uri.DefaultIframeUriManager
For documentation purposes:
I looked through what appears to be 3 proxies.
* Content proxy - gadgets.io.getProxyUrl
* makeRequest proxy - gadgets.io.makeRequest
* RPC Proxy - osapi.http.get
Content proxy denies all requests to a locked domain by default. It's assumed that it's configured on a url that would ensure it is only used for things like image or sctipt tags, etc.
makeRequest does not appear to do any locked domain checking to make sure the gadget is valid for the locked domain. While it's reasonable to assume a malicious gadget will not use the locked domain url of another gadget, it's possible it could craft a request to the proxy on its own locked domain and forge the gadget passed in to appear as another gadget. I'll be making changes to this proxy to include locked domain validation.
RPC Proxy appears to be made from the container on behalf of a gadget, the gadget passed in should be legitimate. I have not tried to make this request on a locked domain to see if the proxy will respond. (Gadget pretending to be the container making the request)
This addresses bug SHINDIG-1628.
https://issues.apache.org/jira/browse/SHINDIG-1628
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo1.xml PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo2.xml PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shared-script-frame/shared-script-frame-container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetException.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/LockedDomainService.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/Renderer.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/LockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriStatus.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/HashLockedDomainServiceTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RendererTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestServletTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletTestFixture.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManagerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriManagerTestBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/ContainerConfigTest.php 1178412
Diff: https://reviews.apache.org/r/2025/diff
Testing
-------
Thanks,
Dan
Re: Review Request: Locked domain cleanup and shared-domain-locking feature
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2025/
-----------------------------------------------------------
(Updated 2011-10-03 19:18:41.510554)
Review request for shindig, Paul Lindner, johnfargo, Ryan Baxter, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Updates for comments from Jesse
Summary
-------
Sorry for the crazy diffs here. Much stuff has moved around.
This is the cleanup part of the patch, I want a few good eyes first before I move on to the feature work.
Some highlights:
* org.apache.shindig.gadgets.uri.DefaultIframeUriManager
Nearly everything relating to locked domains has been moved to org.apache.shindig.gadgets.LockedDomainService
Removed validation on uri for locked domains from this class. It was never actually used.
* org.apache.shindig.gadgets.uri.ProxyUriBase
Removed check for INVALID_DOMAIN, nothing in the code paths leading there ever set that status.
* org.apache.shindig.gadgets.uri.UriStatus
Removed INVALID_DOMAIN, it was not used anymore. This class seems more focused on caching anyway.
* org.apache.shindig.gadgets.HashLockedDomainService
Implemented new methods added to interface. Renamed some methods for clarity and java convention.
Augmented some existing implementation from code that used to be in
org.apache.shindig.gadgets.uri.DefaultIframeUriManager
For documentation purposes:
I looked through what appears to be 3 proxies.
* Content proxy - gadgets.io.getProxyUrl
* makeRequest proxy - gadgets.io.makeRequest
* RPC Proxy - osapi.http.get
Content proxy denies all requests to a locked domain by default. It's assumed that it's configured on a url that would ensure it is only used for things like image or sctipt tags, etc.
makeRequest does not appear to do any locked domain checking to make sure the gadget is valid for the locked domain. While it's reasonable to assume a malicious gadget will not use the locked domain url of another gadget, it's possible it could craft a request to the proxy on its own locked domain and forge the gadget passed in to appear as another gadget. I'll be making changes to this proxy to include locked domain validation.
RPC Proxy appears to be made from the container on behalf of a gadget, the gadget passed in should be legitimate. I have not tried to make this request on a locked domain to see if the proxy will respond. (Gadget pretending to be the container making the request)
This addresses bug SHINDIG-1628.
https://issues.apache.org/jira/browse/SHINDIG-1628
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo1.xml PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo2.xml PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shared-script-frame/shared-script-frame-container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetException.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/LockedDomainService.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/Renderer.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/LockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriStatus.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/HashLockedDomainServiceTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RendererTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestServletTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletTestFixture.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManagerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriManagerTestBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/ContainerConfigTest.php 1178412
Diff: https://reviews.apache.org/r/2025/diff
Testing
-------
Thanks,
Dan
Re: Review Request: Locked domain cleanup and shared-domain-locking feature
Posted by Ryan Baxter <rb...@gmail.com>.
> On 2011-10-03 19:04:01, Jesse Ciancetta wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java, line 315
> > <https://reviews.apache.org/r/2025/diff/6/?file=47438#file47438line315>
> >
> > You could consider using a StringBuilder here instead of a StringBuffer
Committed revision 1178561. Thanks Dan and Jesse.
Dan please close the review and the JIRA.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2025/#review2269
-----------------------------------------------------------
On 2011-10-03 19:58:37, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2025/
> -----------------------------------------------------------
>
> (Updated 2011-10-03 19:58:37)
>
>
> Review request for shindig, Paul Lindner, johnfargo, Ryan Baxter, Jesse Ciancetta, and Stanton Sievers.
>
>
> Summary
> -------
>
> Sorry for the crazy diffs here. Much stuff has moved around.
> This is the cleanup part of the patch, I want a few good eyes first before I move on to the feature work.
>
> Some highlights:
>
> * org.apache.shindig.gadgets.uri.DefaultIframeUriManager
> Nearly everything relating to locked domains has been moved to org.apache.shindig.gadgets.LockedDomainService
> Removed validation on uri for locked domains from this class. It was never actually used.
>
> * org.apache.shindig.gadgets.uri.ProxyUriBase
> Removed check for INVALID_DOMAIN, nothing in the code paths leading there ever set that status.
>
> * org.apache.shindig.gadgets.uri.UriStatus
> Removed INVALID_DOMAIN, it was not used anymore. This class seems more focused on caching anyway.
>
> * org.apache.shindig.gadgets.HashLockedDomainService
> Implemented new methods added to interface. Renamed some methods for clarity and java convention.
> Augmented some existing implementation from code that used to be in
> org.apache.shindig.gadgets.uri.DefaultIframeUriManager
>
> For documentation purposes:
> I looked through what appears to be 3 proxies.
> * Content proxy - gadgets.io.getProxyUrl
> * makeRequest proxy - gadgets.io.makeRequest
> * RPC Proxy - osapi.http.get
>
> Content proxy denies all requests to a locked domain by default. It's assumed that it's configured on a url that would ensure it is only used for things like image or sctipt tags, etc.
>
> makeRequest does not appear to do any locked domain checking to make sure the gadget is valid for the locked domain. While it's reasonable to assume a malicious gadget will not use the locked domain url of another gadget, it's possible it could craft a request to the proxy on its own locked domain and forge the gadget passed in to appear as another gadget. I'll be making changes to this proxy to include locked domain validation.
>
> RPC Proxy appears to be made from the container on behalf of a gadget, the gadget passed in should be legitimate. I have not tried to make this request on a locked domain to see if the proxy will respond. (Gadget pretending to be the container making the request)
>
>
> This addresses bug SHINDIG-1628.
> https://issues.apache.org/jira/browse/SHINDIG-1628
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo1.xml PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo2.xml PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shared-script-frame/shared-script-frame-container.js 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetException.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/LockedDomainService.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGenerator.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/Renderer.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGenerator.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/LockedDomainPrefixGenerator.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriStatus.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/HashLockedDomainServiceTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGeneratorTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RendererTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestServletTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletTestFixture.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManagerTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGeneratorTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriManagerTestBase.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/ContainerConfigTest.php 1178412
>
> Diff: https://reviews.apache.org/r/2025/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Locked domain cleanup and shared-domain-locking feature
Posted by Jesse Ciancetta <jc...@mitre.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2025/#review2269
-----------------------------------------------------------
Ship it!
I found two small nits that you could take or leave which I noted in the code -- other than that it looks good to me.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java
<https://reviews.apache.org/r/2025/#comment5244>
It might be good to log the exception here
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java
<https://reviews.apache.org/r/2025/#comment5245>
You could consider using a StringBuilder here instead of a StringBuffer
- Jesse
On 2011-10-03 14:30:19, Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2025/
> -----------------------------------------------------------
>
> (Updated 2011-10-03 14:30:19)
>
>
> Review request for shindig, Paul Lindner, johnfargo, Ryan Baxter, Jesse Ciancetta, and Stanton Sievers.
>
>
> Summary
> -------
>
> Sorry for the crazy diffs here. Much stuff has moved around.
> This is the cleanup part of the patch, I want a few good eyes first before I move on to the feature work.
>
> Some highlights:
>
> * org.apache.shindig.gadgets.uri.DefaultIframeUriManager
> Nearly everything relating to locked domains has been moved to org.apache.shindig.gadgets.LockedDomainService
> Removed validation on uri for locked domains from this class. It was never actually used.
>
> * org.apache.shindig.gadgets.uri.ProxyUriBase
> Removed check for INVALID_DOMAIN, nothing in the code paths leading there ever set that status.
>
> * org.apache.shindig.gadgets.uri.UriStatus
> Removed INVALID_DOMAIN, it was not used anymore. This class seems more focused on caching anyway.
>
> * org.apache.shindig.gadgets.HashLockedDomainService
> Implemented new methods added to interface. Renamed some methods for clarity and java convention.
> Augmented some existing implementation from code that used to be in
> org.apache.shindig.gadgets.uri.DefaultIframeUriManager
>
> For documentation purposes:
> I looked through what appears to be 3 proxies.
> * Content proxy - gadgets.io.getProxyUrl
> * makeRequest proxy - gadgets.io.makeRequest
> * RPC Proxy - osapi.http.get
>
> Content proxy denies all requests to a locked domain by default. It's assumed that it's configured on a url that would ensure it is only used for things like image or sctipt tags, etc.
>
> makeRequest does not appear to do any locked domain checking to make sure the gadget is valid for the locked domain. While it's reasonable to assume a malicious gadget will not use the locked domain url of another gadget, it's possible it could craft a request to the proxy on its own locked domain and forge the gadget passed in to appear as another gadget. I'll be making changes to this proxy to include locked domain validation.
>
> RPC Proxy appears to be made from the container on behalf of a gadget, the gadget passed in should be legitimate. I have not tried to make this request on a locked domain to see if the proxy will respond. (Gadget pretending to be the container making the request)
>
>
> This addresses bug SHINDIG-1628.
> https://issues.apache.org/jira/browse/SHINDIG-1628
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriManagerTestBase.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/ContainerConfigTest.php 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGeneratorTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestServletTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletTestFixture.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManagerTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RendererTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGenerator.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/LockedDomainPrefixGenerator.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriStatus.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/HashLockedDomainServiceTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGeneratorTest.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetException.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/LockedDomainService.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGenerator.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/Renderer.java 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo2.xml PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shared-script-frame/shared-script-frame-container.js 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1178412
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo1.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/2025/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan
>
>
Re: Review Request: Locked domain cleanup and shared-domain-locking feature
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2025/
-----------------------------------------------------------
(Updated 2011-10-03 14:30:19.987345)
Review request for shindig, Paul Lindner, johnfargo, Ryan Baxter, Jesse Ciancetta, and Stanton Sievers.
Changes
-------
Found and removed a trailing whitespace
Summary
-------
Sorry for the crazy diffs here. Much stuff has moved around.
This is the cleanup part of the patch, I want a few good eyes first before I move on to the feature work.
Some highlights:
* org.apache.shindig.gadgets.uri.DefaultIframeUriManager
Nearly everything relating to locked domains has been moved to org.apache.shindig.gadgets.LockedDomainService
Removed validation on uri for locked domains from this class. It was never actually used.
* org.apache.shindig.gadgets.uri.ProxyUriBase
Removed check for INVALID_DOMAIN, nothing in the code paths leading there ever set that status.
* org.apache.shindig.gadgets.uri.UriStatus
Removed INVALID_DOMAIN, it was not used anymore. This class seems more focused on caching anyway.
* org.apache.shindig.gadgets.HashLockedDomainService
Implemented new methods added to interface. Renamed some methods for clarity and java convention.
Augmented some existing implementation from code that used to be in
org.apache.shindig.gadgets.uri.DefaultIframeUriManager
For documentation purposes:
I looked through what appears to be 3 proxies.
* Content proxy - gadgets.io.getProxyUrl
* makeRequest proxy - gadgets.io.makeRequest
* RPC Proxy - osapi.http.get
Content proxy denies all requests to a locked domain by default. It's assumed that it's configured on a url that would ensure it is only used for things like image or sctipt tags, etc.
makeRequest does not appear to do any locked domain checking to make sure the gadget is valid for the locked domain. While it's reasonable to assume a malicious gadget will not use the locked domain url of another gadget, it's possible it could craft a request to the proxy on its own locked domain and forge the gadget passed in to appear as another gadget. I'll be making changes to this proxy to include locked domain validation.
RPC Proxy appears to be made from the container on behalf of a gadget, the gadget passed in should be legitimate. I have not tried to make this request on a locked domain to see if the proxy will respond. (Gadget pretending to be the container making the request)
This addresses bug SHINDIG-1628.
https://issues.apache.org/jira/browse/SHINDIG-1628
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriManagerTestBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/ContainerConfigTest.php 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestServletTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ServletTestFixture.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManagerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RendererTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/LockedDomainPrefixGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriStatus.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/HashLockedDomainServiceTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGeneratorTest.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetException.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/LockedDomainService.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGenerator.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/Renderer.java 1178412
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo2.xml PRE-CREATION
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shared-script-frame/shared-script-frame-container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1178412
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/SharedLockedDomainDemo1.xml PRE-CREATION
Diff: https://reviews.apache.org/r/2025/diff
Testing
-------
Thanks,
Dan