Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(7469)

Issue 130079: [SHINDIG-1192] content-rewrite is not compliant with Open Social v0.9, plus some bugs

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 6 months ago by Jon Weygandt
Modified:
9 years, 4 months ago
Reviewers:
shindig.remailer
Base URL:
http://svn.apache.org/repos/asf/incubator/shindig/trunk/
Visibility:
Public.

Description

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.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1559 lines, -318 lines) Patch
java/common/conf/shindig.properties View 1 chunk +1 line, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/DefaultSanitizingProxyingLinkRewriterFactory.java View 1 chunk +51 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java View 4 chunks +12 lines, -41 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java View 1 chunk +37 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriterFactory.java View 1 chunk +35 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingRequestRewriter.java View 3 chunks +11 lines, -12 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java View 1 chunk +121 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriterFactory.java View 1 chunk +35 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java View 4 chunks +244 lines, -69 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeatureFactory.java View 6 chunks +7 lines, -5 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CssRequestRewriter.java View 3 chunks +6 lines, -10 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DefaultConcatLinkRewriterFactory.java View 1 chunk +32 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DefaultProxyingLinkRewriterFactory.java View 1 chunk +47 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/HTMLContentRewriter.java View 14 chunks +34 lines, -78 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingLinkRewriter.java View 3 chunks +30 lines, -20 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingLinkRewriterFactory.java View 1 chunk +35 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java View 2 chunks +14 lines, -7 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java View 1 chunk +1 line, -1 line 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java View 1 chunk +7 lines, -0 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java View 7 chunks +66 lines, -3 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingRequestRewriterTest.java View 2 chunks +22 lines, -2 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseRewriterTestCase.java View 5 chunks +82 lines, -22 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeatureTestCase.java View 9 chunks +56 lines, -13 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeatureTestCaseOS9.java View 1 chunk +262 lines, -0 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssRequestRewriterTest.java View 3 chunks +57 lines, -4 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/HTMLContentRewriterTest.java View 10 chunks +194 lines, -16 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ProxyingLinkRewriterTest.java View 1 chunk +37 lines, -11 lines 0 comments Download
java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritebasic-expected.css View 1 chunk +3 lines, -3 lines 0 comments Download
java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritescriptbasic.html View 1 chunk +19 lines, -0 lines 0 comments Download
java/gadgets/src/test/resources/org/apache/shindig/gadgets/rewrite/rewritestyle2-expected.html View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 1
Jon Weygandt
14 years, 6 months ago (2009-10-14 21:35:52 UTC) #1

          
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b