Re: Update Sanitizing rewriters to use ProxyUriManager, Visitor pattern (issue223095)

[zh...@gmail.com]

New tests are appreciated!


http://codereview.appspot.com/223095/diff/1001/2007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
(right):

http://codereview.appspot.com/223095/diff/1001/2007#newcode186
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java:186:
VisitStatus status = VisitStatus.MODIFY;
Please doc why status is set to MODIFY always and not just when removing
attributes. It seems that the boolean removeAttr can also change
attributes (not very clean...)

Maybe change removeAttr to modifyAttr that return true if attribute was
modified/deleted

http://codereview.appspot.com/223095/diff/1001/2008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java
(right):

http://codereview.appspot.com/223095/diff/1001/2008#newcode35
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java:35:
public class SanitizingProxyingLinkRewriter implements ProxyUriManager {
Is it a rewriter or a uri manager? I think it is confusing to follow the
code without naming consistency

I start to like the original LinkRewriter name better then
UriManager

http://codereview.appspot.com/223095/diff/1001/2001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java
(right):

http://codereview.appspot.com/223095/diff/1001/2001#newcode203
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java:203:
+ "@import
url('http://host.com/proxy?url=www.example.org%2Fwww.evil.com%2Fx.js&"
Where did the gadget=... value is gone?
I thought it would be useful for statistics.

http://codereview.appspot.com/223095/show

Re: Update Sanitizing rewriters to use ProxyUriManager, Visitor pattern (issue223095)

[John Hjelmstad <jo...@gmail.com>]

On Fri, Mar 5, 2010 at 10:27 AM, <zh...@gmail.com> wrote

> New tests are appreciated!
>
>
> http://codereview.appspot.com/223095/diff/1001/2007
> File
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
> (right):
>
> http://codereview.appspot.com/223095/diff/1001/2007#newcode186
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java:186:
> VisitStatus status = VisitStatus.MODIFY;
> Please doc why status is set to MODIFY always and not just when removing
> attributes. It seems that the boolean removeAttr can also change
> attributes (not very clean...)
>
> Maybe change removeAttr to modifyAttr that return true if attribute was
> modified/deleted
>

Agreed -- I missed one use case from the previous class (modifying the
attribute) so have to return MODIFY at all times in the current scheme
(found this during test phase). The impact should be relatively low, in that
all MODIFY does is set a dirty-bit on the DOM, forcing reserialization --
which has negative performance impact only when no other rewriter or Visitor
has modified it.

Even so, I'll fix it up to return a trinary state (NONE, REMOVE, MODIFY)


>
> http://codereview.appspot.com/223095/diff/1001/2008
> File
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java
> (right):
>
> http://codereview.appspot.com/223095/diff/1001/2008#newcode35
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java:35:
> public class SanitizingProxyingLinkRewriter implements ProxyUriManager {
> Is it a rewriter or a uri manager? I think it is confusing to follow the
> code without naming consistency
>
> I start to like the original LinkRewriter name better then
> UriManager
>

Yeah it's actually a UriManager at this point.. I'm just reusing the class.
I'm starting to think it best to just completely replace the "old" classes
with new ones and keep the old infrastructure (for HTMLContentRewriter) in
place. I'll send you a CL to that effect and update these w/ more accurate
class names to avoid confusion.


>
> http://codereview.appspot.com/223095/diff/1001/2001
> File
>
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java
> (right):
>
> http://codereview.appspot.com/223095/diff/1001/2001#newcode203
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java:203:
> + "@import
> url('http://host.com/proxy?url=www.example.org%2Fwww.evil.com%2Fx.js&"
> Where did the gadget=... value is gone?
> I thought it would be useful for statistics.


It's only gone for the test. The PassthruManager that manipulates the URI is
a dummy/test implementation that performs a very simple (one query param,
well-known scheme, authority, and path) transformation for easier testing.
The "real" UriManager that would be used for this is DefaultProxyUriManager
or a subclass, which adds gadget (and container, debug, nocache and the
rest).


>
>
> http://codereview.appspot.com/223095/show
>