You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@shindig.apache.org by "Jon Weygandt (JIRA)" <ji...@apache.org> on 2009/10/14 23:34:31 UTC
[jira] Created: (SHINDIG-1192) content-rewrite is not compliant
with Open Social v0.9, plus some bugs
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
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.
[jira] Commented: (SHINDIG-1192) content-rewrite is not compliant
with Open Social v0.9, plus some bugs
Posted by "Paul Lindner (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Commented: (SHINDIG-1192) content-rewrite is not compliant
with Open Social v0.9, plus some bugs
Posted by "Jon Weygandt (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12766090#action_12766090 ]
Jon Weygandt commented on SHINDIG-1192:
---------------------------------------
The code review is on http://codereview.appspot.com/130079
Am I doing something wrong? I set the reviewer to
"Shindig.remailer@gmail.com", but never see any notification in this
group.
> 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.
[jira] Commented: (SHINDIG-1192) content-rewrite is not compliant
with Open Social v0.9, plus some bugs
Posted by "Paul Lindner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12770168#action_12770168 ]
Paul Lindner commented on SHINDIG-1192:
---------------------------------------
I'm going to try to carve out some time to look this over.
Overall it seems quite good and fixes a number of big issues.
> 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.
[jira] Commented: (SHINDIG-1192) content-rewrite is not compliant
with Open Social v0.9, plus some bugs
Posted by "Paul Lindner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12773950#action_12773950 ]
Paul Lindner commented on SHINDIG-1192:
---------------------------------------
lgtm, will apply today..
> 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.
[jira] Updated: (SHINDIG-1192) content-rewrite is not compliant
with Open Social v0.9, plus some bugs
Posted by "Jon Weygandt (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1192?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jon Weygandt updated SHINDIG-1192:
----------------------------------
Attachment: fix-1192-bug.patch
> 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.
[jira] Resolved: (SHINDIG-1192) content-rewrite is not compliant
with Open Social v0.9, plus some bugs
Posted by "Paul Lindner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-1192?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Paul Lindner resolved SHINDIG-1192.
-----------------------------------
Resolution: Fixed
Fix Version/s: 1.1-BETA4
looks good..
will get committed when svn.apache.org comes back online..
> 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
> Fix For: 1.1-BETA4
>
> 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.