[
https://issues.apache.org/jira/browse/SHINDIG-1628?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13118077#comment-13118077
]
[email protected] commented on SHINDIG-1628:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2025/#review2216
-----------------------------------------------------------
Overall the patch looks pretty good to me -- it's nice to see the cleanup of
the dead code in DefaultIframeUriManager and the duplicate properties in
container.js.
The only issues I see are around the changes in HashLockedDomainService -- I've
added detailed comments for those inline in the code.
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/#comment5148>
It looks like the logic in this method is now based on the logic you
removed from DefaultIframeUriManager.validateRenderingUri(...) -- however that
logic doesn't seem to cover all the cases the old logic used to cover. For
example -- with your new logic it's possible to render a gadget on an unlocked
domain even if the container config requires locked domain for all gadgets or
if the gadget itself requests locked domain in its features or via a transitive
dependency.
I suspect this is this where your follow on work is going to come in and
further augment this logic to cover these cases as well as the additional
functionality your adding -- is that right?
If that's the case then I'd rather see a little more work in this patch to
make it consistent with the old logic and see that committed separate from the
new features -- then your follow on work will be easier for everyone to
understand and review. It seems like this patch is close enough to ready to
commit as is that it shouldn't be too difficult to break up the work.
FWIW -- the old logic updated to use your new method signatures seems right
to me:
public boolean gadgetCanRender(String host, Gadget gadget, String
container) {
container = getContainer(container);
if (enabled) {
if (isGadgetReqestingLocking(gadget) ||
isHostUsingLockedDomain(host) ||
isDomainLockingEnforced(container)) {
String neededHost = getLockedDomain(gadget, container);
return host.equals(neededHost);
}
}
return true;
}
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/#comment5145>
Could the call to checkSuffix be done once when the suffix is loaded into
the map instead of needing to be done for every request?
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/#comment5149>
Could the call to checkSuffix be done once when the suffix is loaded into
the map instead of needing to be done for every request?
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/#comment5146>
This looks like it is supposed to be outside of the:
if (LOG.isLoggable(Level.WARNING)) {
...
}
block
- Jesse
On 2011-09-29 17:03:35, Dan Dumont wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2025/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-09-29 17:03:35)
bq.
bq.
bq. Review request for shindig, Paul Lindner, johnfargo, Ryan Baxter, Jesse
Ciancetta, and Stanton Sievers.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Sorry for the crazy diffs here. Much stuff has moved around.
bq. This is the cleanup part of the patch, I want a few good eyes first before
I move on to the feature work.
bq.
bq. Some highlights:
bq.
bq. * org.apache.shindig.gadgets.uri.DefaultIframeUriManager
bq. Nearly everything relating to locked domains has been moved to
org.apache.shindig.gadgets.LockedDomainService
bq. Removed validation on uri for locked domains from this class. It was
never actually used.
bq.
bq. * org.apache.shindig.gadgets.uri.ProxyUriBase
bq. Removed check for INVALID_DOMAIN, nothing in the code paths leading there
ever set that status.
bq.
bq. * org.apache.shindig.gadgets.uri.UriStatus
bq. Removed INVALID_DOMAIN, it was not used anymore. This class seems more
focused on caching anyway.
bq.
bq. * org.apache.shindig.gadgets.HashLockedDomainService
bq. Implemented new methods added to interface. Renamed some methods for
clarity and java convention.
bq. Augmented some existing implementation from code that used to be in
bq. org.apache.shindig.gadgets.uri.DefaultIframeUriManager
bq.
bq. For documentation purposes:
bq. I looked through what appears to be 3 proxies.
bq. * Content proxy - gadgets.io.getProxyUrl
bq. * makeRequest proxy - gadgets.io.makeRequest
bq. * RPC Proxy - osapi.http.get
bq.
bq. 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.
bq.
bq. 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.
bq.
bq. 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)
bq.
bq.
bq. This addresses bug SHINDIG-1628.
bq. https://issues.apache.org/jira/browse/SHINDIG-1628
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java
1174376
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/LockedDomainService.java
1174376
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
1174376
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java
1174376
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
1174376
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriStatus.java
1174376
bq.
http://svn.apache.org/repos/asf/shindig/trunk/php/test/gadgets/ContainerConfigTest.php
1174376
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js
1174376
bq.
bq. Diff: https://reviews.apache.org/r/2025/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Dan
bq.
bq.
> Locked domain cleanup and shared-domain-locking feature
> -------------------------------------------------------
>
> Key: SHINDIG-1628
> URL: https://issues.apache.org/jira/browse/SHINDIG-1628
> Project: Shindig
> Issue Type: New Feature
> Components: Java
> Reporter: Dan Dumont
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
