[
https://issues.apache.org/jira/browse/SHINDIG-1192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12765777#action_12765777
]
Paul Lindner commented on SHINDIG-1192:
---------------------------------------
can you upload this patch to codereview.appspot.com? Something this big should
have a review..
Thanks for the
> content-rewrite is not compliant with Open Social v0.9, plus some bugs
> ----------------------------------------------------------------------
>
> Key: SHINDIG-1192
> URL: https://issues.apache.org/jira/browse/SHINDIG-1192
> Project: Shindig
> Issue Type: Bug
> Components: Java
> Affects Versions: 1.1-BETA3
> Reporter: Jon Weygandt
> Attachments: fix-1192-bug.patch
>
>
> Description of the issues, and details of the associated patch file:
> Issues:
> 1) Not compliant with 0.9 spec:
> http://www.opensocial.org/Technical-Resources/opensocial-spec-v09/Gadgets-API-Specification.html#rfc.section.3.4
> Shindig uses include-urls, exclude-urls (note: plural) which are regular
> expressions
> Specification use include-url, exclude-url which are case insensitive
> matches, plus it allows multiple occurrences of these tags.
> Note: no effort to support minify for this patch
> Fix: Alter the code to honor these tags. For backward compatibility the
> old "plural" tags are also accepted.
> 2) Throughout Shindig parameters "debug" and "nocache" have been used and
> honored for various requests. For the rewriters it is missing. "nocache" is
> obvious. "debug" is not used today, but could be used to support the minify
> options of the future.
> Fix: Alter the rewriters to properly pass the debug and nocache from the
> HttpRequest or GadgetContext to the end URL.
> 3) The concat rewriter takes a list of URLs and rewrites them so that it does
> not exceed some max length. However the test was done after the appending
> which could lead to a URL that was too long.
> Fix: Altered the algorithm so it will check the predicted length prior to
> appending.
> 4) The concat rewriter did not handle a gadget authors link that was
> originally too long.
> Fix: Since concat is for the benefit of the gadget author (as opposed to
> security), original links that are too long are simply passed through without
> being rewritten.
> 5) The ConcatProxyServlet is vulnerable to response splitting:
> http://www.google.com/search?q=response+splitting+vulnerability&ie=utf-8
> Fix: Check for \r \n in header
> 6) The concat rewriter (or concat servlet) did not do caching header
> correctly.
> Fix: Borrowing from how the proxy rewriters did it, added the refresh
> parameter to the concat URL
> Enhancements
> 1) Because: this is optional feature; and this costs resources server side, I
> felt that to allow a gadget author to include additional resources that were
> explicitly excluded by the original server config, or to allow the gadget
> author to extend the expires time beyond the original server config times
> would be an issue. Therefore added a configuration parameter to shindig
> properties "shindig.content-rewrite.only-allow-excludes=false", which when
> set to true will: disallow additional include-url(s); will honor the original
> exclude-urls; disallow an increase in expires time; not allow additional
> extensions to be cached.
> 2) There was some initial effort to allow the rewriters to be "guiced" into
> the code by using a protected method to create a proxying link rewriter,
> HTMLContentRewriter.createLinkRewriter() &
> CssRequestRewriter.createLinkRewriter(). However with 3 different types of
> link rewriters used across 4 files, plus who knows where else it will be
> used. This pattern of protected methods is not scalable. Chose to use a
> factory pattern for the 3 link rewriters.
> Specific Files:
> java/common/conf/shindig.properties
> New property
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
> Support for "nocache"
> Fixes the response splitting vulnerability.
> Also if "refresh" is not present, will explicitly send nocache headers.
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriterFactory.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/DefaultSanitizingProxyingLinkRewriterFactory.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriterFactory.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DefaultConcatLinkRewriterFactory.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingLinkRewriterFactory.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DefaultProxyingLinkRewriterFactory.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingLinkRewriter.java
> An interface for the factory, default implementation of the factory, and
> the rewriter.
> Sanitizing and Concat were generally extracted from their "inner class"
> definitions.
> Fixed Concat to always include refresh on the URL
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeatureFactory.java
> Needed to handle additional include/exclude tags, and new
> only-allow-excludes option.
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
> Reuses some algorithms from the original, but is best reviewed as new
> code implementing the algorithms necessary to support the original and new
> features.
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/HTMLContentRewriter.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssRequestRewriter.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingRequestRewriter.java
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
> Refactored to use new factory pattern.
> java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java
> Needed a new accessor method
> Unit Test Files:
> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java
> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingRequestRewriterTest.java
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingLinkRewriterTest.java
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeatureTestCase.java
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeatureTestCaseOS9.java
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/HTMLContentRewriterTest.java
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssRequestRewriterTest.java
> java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseRewriterTestCase.java
> java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritestyle2-expected.html
> java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritebasic-expected.css
> java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritescriptbasic.html
> In General:
> Mods for use of the new Factory Pattern
> Tests for debug and nocache
> Test for only-allow-excludes
> Concat now uses "refresh", added to tests
> ContentRewriterFeatureTestCaseOS9.java
> New file based on ContentRewriterFeatureTestCase.java, but uses the
> OpenSocial v 0.9 specification to perform the tests.
> HTMLContentRewriterTest.java
> rewritescriptbasic.html
> Added test for the splitting of concat rewrites into multiple concats.
> HTMLContentRewriterTest.java
> CssRequestRewriterTest.java
> BaseRewriterTestCase.java
> Somehow the refresh time was "HTTP", rather than a valid integer! I
> changed this to a valid integer, and fixed the test standards. Additional
> added tests to specifically test when refresh is not a valid integer.
> Added tests to ensure the ConcatRewriter will properly split the rewrite
> into different URLs.
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
