----------------------------------------------------------- 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 > >
