Embed and object tags should not be rewritten, input src and body background should be rewritten

The AbsolutePathReferenceVisitor and the ProxyingContentVisitor should bypass the embed and the object tags for now.
The src attribute of input and background attribute of body should be rewritten.

[anupama, 2010-07-13 13:53:02]

http://codereview.appspot.com/1806044/diff/1/5
File
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(left):

http://codereview.appspot.com/1806044/diff/1/5#oldcode45
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:45:
"embed", "src",
I think you should retain these, in order to not affect existing
ProxyingContentRewriter users. Gagan, what do you think?

[gagan.goku, 2010-07-13 13:53:26]

http://codereview.appspot.com/1806044/diff/1/2
File common/conf/shindig.properties (right):

http://codereview.appspot.com/1806044/diff/1/2#newcode51
common/conf/shindig.properties:51:
shindig.content-rewrite.include-tags=link,script,input,body,img,style
small nitpick: maybe sort by name everywhere (the order does not really matter,
but makes it easier to read)

http://codereview.appspot.com/1806044/diff/1/4
File
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java
(left):

http://codereview.appspot.com/1806044/diff/1/4#oldcode105
gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:105:
public void absolutifyTagObject() throws Exception {
Probably you should test that object tag is being ignored. Something like:

  Element obj = img = elem("object", "src", "dummy_url");
  assertEquals(VisitStatus.BYPASS, getVisitStatus(obj));

should do the trick.

[gagan.goku, 2010-07-13 14:00:00]

Although currently in shindig only accel uses ProxyingContentRewriter, there
might be other ppl using this rewriter. I'm not sure how to deal with this
sort of functionality removal.
Maybe we can mention this clearly when we send this mail out to shindig dev.
Also, if any1 is using it, they have a bug already, so its better that their
next release will fix a hidden bug :)

On Tue, Jul 13, 2010 at 7:23 PM, <anupama@google.com> wrote:

>
> http://codereview.appspot.com/1806044/diff/1/5
> File
>
>
> gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
> (left):
>
> http://codereview.appspot.com/1806044/diff/1/5#oldcode45
>
>
gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:45:
> "embed", "src",
> I think you should retain these, in order to not affect existing
> ProxyingContentRewriter users. Gagan, what do you think?
>
>
> http://codereview.appspot.com/1806044/show
>

[Kuntal Loya, 2010-07-13 14:21:10]

Tags in order, added test for bypassing object by AbsolutePathReferenceVisotor.

[gagan.goku, 2010-07-13 14:31:21]

lgtm

[anupama.dutta, 2010-07-13 14:38:42]

There seem to be other "embed" handling classes such as
ProxyingLinkRewriter.java,RewriterTestBase.java, HTMLContentRewriter.java etc. I
was wondering if we should only add body, input to shindig.properties and not
remove the existing ones. The java files changes should still be fine since
these are subsets of the shindig.properties lists. What do you think?

[gagan.goku, 2010-07-13 19:02:19]

Nice catch :) Then i think as you suggested, not removing tags from
shindig.properties file is the right thing to do.


On Tue, Jul 13, 2010 at 8:08 PM, <anupama.dutta@gmail.com> wrote:

> There seem to be other "embed" handling classes such as
> ProxyingLinkRewriter.java,RewriterTestBase.java,
> HTMLContentRewriter.java etc. I was wondering if we should only add
> body, input to shindig.properties and not remove the existing ones. The
> java files changes should still be fine since these are subsets of the
> shindig.properties lists. What do you think?
>
>
> http://codereview.appspot.com/1806044/show
>

[Kuntal Loya, 2010-07-15 09:12:34]

From what I noticed, the ProxyingLinkRewriter was not doing any "embed" handling
and whatever handling the HTMLContentRewriter is doing, is independent of the
shindig.properties.
Regarding the test RewriterTestBase, it is running fine even with the change, as
it again doesn't depend on the shindig.properties.

Now I am not very sure if the change in shindig.properties should be there or
not and to what extent does it affect the rewriters.


On 2010/07/13 19:02:19, gagan.goku wrote:
> Nice catch :) Then i think as you suggested, not removing tags from
> shindig.properties file is the right thing to do.
> 
> 
> On Tue, Jul 13, 2010 at 8:08 PM, <mailto:anupama.dutta@gmail.com> wrote:
> 
> > There seem to be other "embed" handling classes such as
> > ProxyingLinkRewriter.java,RewriterTestBase.java,
> > HTMLContentRewriter.java etc. I was wondering if we should only add
> > body, input to shindig.properties and not remove the existing ones. The
> > java files changes should still be fine since these are subsets of the
> > shindig.properties lists. What do you think?
> >
> >
> > http://codereview.appspot.com/1806044/show
> >
>

[anupama.dutta, 2010-07-16 09:46:36]

What you said could be fine - but just to be sure, could you anyways check
this point specifically with John, when you send this out for his review?

Thanks,
Anupama.

On Thu, Jul 15, 2010 at 2:42 PM, <kuntal.loya@gmail.com> wrote:

> From what I noticed, the ProxyingLinkRewriter was not doing any "embed"
> handling and whatever handling the HTMLContentRewriter is doing, is
> independent of the shindig.properties.
> Regarding the test RewriterTestBase, it is running fine even with the
> change, as it again doesn't depend on the shindig.properties.
>
> Now I am not very sure if the change in shindig.properties should be
> there or not and to what extent does it affect the rewriters.
>
>
>
> On 2010/07/13 19:02:19, gagan.goku wrote:
>
>> Nice catch :) Then i think as you suggested, not removing tags from
>> shindig.properties file is the right thing to do.
>>
>
>
>  On Tue, Jul 13, 2010 at 8:08 PM, <mailto:anupama.dutta@gmail.com>
>>
> wrote:
>
>  > There seem to be other "embed" handling classes such as
>> > ProxyingLinkRewriter.java,RewriterTestBase.java,
>> > HTMLContentRewriter.java etc. I was wondering if we should only add
>> > body, input to shindig.properties and not remove the existing ones.
>>
> The
>
>> > java files changes should still be fine since these are subsets of
>>
> the
>
>> > shindig.properties lists. What do you think?
>> >
>> >
>> > http://codereview.appspot.com/1806044/show
>> >
>>
>
>
>
>
> http://codereview.appspot.com/1806044/show
>



-- 
Anupama

[gagan.goku, 2010-07-16 13:36:52]

Ditto. This change is in good enough shape to send to shindig dev@
Ps: Sorry for the late reply :(

[Kuntal Loya, 2010-07-16 13:38:45]

The AbsolutePathReferenceVisitor and the ProxyingContentVisitor should bypass
the embed and the object tags for now.
The src attribute of input and background attribute of body should be rewritten.

[chirag, 2010-07-16 16:06:49]

The Caja UrlPolicy document has a nice list of HTML attributes that can be
rewritten.

http://code.google.com/p/google-caja/wiki/UrlPolicy

On 2010/07/16 13:38:45, Kuntal Loya wrote:
> The AbsolutePathReferenceVisitor and the ProxyingContentVisitor should bypass
> the embed and the object tags for now.
> The src attribute of input and background attribute of body should be
rewritten.

[Jasvir, 2010-07-16 16:20:52]

In addition to Chirag's pointer, programmatically, if you're looking for html
attributes that take URIs, the attributes returned by
com.google.caja.lang.html.HtmlSchema that take a uri have .getType() ===
HTML.Attribute.Type.URI.

The default list is derived from the w3c spec and I'd rather avoid recreating
the list ad hoc. 

On 2010/07/16 16:06:49, chirag wrote:
> The Caja UrlPolicy document has a nice list of HTML attributes that can be
> rewritten.
> 
> http://code.google.com/p/google-caja/wiki/UrlPolicy
> 
> On 2010/07/16 13:38:45, Kuntal Loya wrote:
> > The AbsolutePathReferenceVisitor and the ProxyingContentVisitor should
bypass
> > the embed and the object tags for now.
> > The src attribute of input and background attribute of body should be
> rewritten.

[gagan.goku, 2010-07-16 16:51:20]

Thanks for the pointer Jasvir.
Seems we want the attributes with type = URI and uriEffect = SAME_DOCUMENT.

If i do a grep over the html4-attributes-defs.json file
in caja\src\com\google\caja\lang\html directory, i find the following
attributes:
body background
object classid
object codebase
applet codebase
object data
link href
img longdesc
frame longdesc
iframe longdesc
head profile
script src
input src
frame src
iframe src
img src

Sadly some (or most) of them (like longdesc etc) seem to be badly supported
by browsers, and due to unexpected bugs we might not want to handle all of
these.
But maybe a blacklist of attributes will work better. So we go over every
node and each of its attributes (or prepopulate a map of allowed node and
attribute) to find attributes matching our criterion and rewrite them.

Thoughts ?

On Fri, Jul 16, 2010 at 9:50 PM, <jasvir@gmail.com> wrote:

> In addition to Chirag's pointer, programmatically, if you're looking for
> html attributes that take URIs, the attributes returned by
> com.google.caja.lang.html.HtmlSchema that take a uri have .getType() ===
> HTML.Attribute.Type.URI.
>
> The default list is derived from the w3c spec and I'd rather avoid
> recreating the list ad hoc.
>
>
> On 2010/07/16 16:06:49, chirag wrote:
>
>> The Caja UrlPolicy document has a nice list of HTML attributes that
>>
> can be
>
>> rewritten.
>>
>
>  http://code.google.com/p/google-caja/wiki/UrlPolicy
>>
>
>  On 2010/07/16 13:38:45, Kuntal Loya wrote:
>> > The AbsolutePathReferenceVisitor and the ProxyingContentVisitor
>>
> should bypass
>
>> > the embed and the object tags for now.
>> > The src attribute of input and background attribute of body should
>>
> be
>
>> rewritten.
>>
>
>
>
> http://codereview.appspot.com/1806044/show
>

[anupama.dutta, 2010-07-20 10:06:32]

On 2010/07/16 16:51:20, gagan.goku wrote:
> Thanks for the pointer Jasvir.
> Seems we want the attributes with type = URI and uriEffect = SAME_DOCUMENT.
> 
> If i do a grep over the html4-attributes-defs.json file
> in caja\src\com\google\caja\lang\html directory, i find the following
> attributes:
> body background
> object classid
> object codebase
> applet codebase
> object data
> link href
> img longdesc
> frame longdesc
> iframe longdesc
> head profile
> script src
> input src
> frame src
> iframe src
> img src
> 
> Sadly some (or most) of them (like longdesc etc) seem to be badly supported
> by browsers, and due to unexpected bugs we might not want to handle all of
> these.
> But maybe a blacklist of attributes will work better. So we go over every
> node and each of its attributes (or prepopulate a map of allowed node and
> attribute) to find attributes matching our criterion and rewrite them.
> 
> Thoughts ?


Agreed that this change to use the complete w3c spec list from the class pointed
to by Jasvir, would be a good change. Kuntal, could you file a jira issue for
tracking this?

I think we should go ahead with the current change for now, and make the
transition to the larger list in a subsequent change. Jasvir, Chirag - do you
think this will be fine?

Thanks,
Anupama.

[Kuntal Loya, 2010-07-21 06:44:49]

Created a JIRA issue - https://issues.apache.org/jira/browse/SHINDIG-1390


On 2010/07/20 10:06:32, anupama.dutta wrote:
> On 2010/07/16 16:51:20, gagan.goku wrote:
> > Thanks for the pointer Jasvir.
> > Seems we want the attributes with type = URI and uriEffect = SAME_DOCUMENT.
> > 
> > If i do a grep over the html4-attributes-defs.json file
> > in caja\src\com\google\caja\lang\html directory, i find the following
> > attributes:
> > body background
> > object classid
> > object codebase
> > applet codebase
> > object data
> > link href
> > img longdesc
> > frame longdesc
> > iframe longdesc
> > head profile
> > script src
> > input src
> > frame src
> > iframe src
> > img src
> > 
> > Sadly some (or most) of them (like longdesc etc) seem to be badly supported
> > by browsers, and due to unexpected bugs we might not want to handle all of
> > these.
> > But maybe a blacklist of attributes will work better. So we go over every
> > node and each of its attributes (or prepopulate a map of allowed node and
> > attribute) to find attributes matching our criterion and rewrite them.
> > 
> > Thoughts ?
> 
> 
> Agreed that this change to use the complete w3c spec list from the class
pointed
> to by Jasvir, would be a good change. Kuntal, could you file a jira issue for
> tracking this?
> 
> I think we should go ahead with the current change for now, and make the
> transition to the larger list in a subsequent change. Jasvir, Chirag - do you
> think this will be fine?
> 
> Thanks,
> Anupama.

[johnfargo, 2010-07-26 22:25:34]

Thanks Kuntal. Jas, any further thoughts on the matter, ie re: browser support
vs. spec designation?

On 2010/07/21 06:44:49, Kuntal Loya wrote:
> Created a JIRA issue - https://issues.apache.org/jira/browse/SHINDIG-1390
> 
> 
> On 2010/07/20 10:06:32, anupama.dutta wrote:
> > On 2010/07/16 16:51:20, gagan.goku wrote:
> > > Thanks for the pointer Jasvir.
> > > Seems we want the attributes with type = URI and uriEffect =
SAME_DOCUMENT.
> > > 
> > > If i do a grep over the html4-attributes-defs.json file
> > > in caja\src\com\google\caja\lang\html directory, i find the following
> > > attributes:
> > > body background
> > > object classid
> > > object codebase
> > > applet codebase
> > > object data
> > > link href
> > > img longdesc
> > > frame longdesc
> > > iframe longdesc
> > > head profile
> > > script src
> > > input src
> > > frame src
> > > iframe src
> > > img src
> > > 
> > > Sadly some (or most) of them (like longdesc etc) seem to be badly
supported
> > > by browsers, and due to unexpected bugs we might not want to handle all of
> > > these.
> > > But maybe a blacklist of attributes will work better. So we go over every
> > > node and each of its attributes (or prepopulate a map of allowed node and
> > > attribute) to find attributes matching our criterion and rewrite them.
> > > 
> > > Thoughts ?
> > 
> > 
> > Agreed that this change to use the complete w3c spec list from the class
> pointed
> > to by Jasvir, would be a good change. Kuntal, could you file a jira issue
for
> > tracking this?
> > 
> > I think we should go ahead with the current change for now, and make the
> > transition to the larger list in a subsequent change. Jasvir, Chirag - do
you
> > think this will be fine?
> > 
> > Thanks,
> > Anupama.

[jas_nagras.com, 2010-07-27 08:09:53]

On Mon, Jul 26, 2010 at 3:25 PM, <johnfargo@gmail.com> wrote:

> Thanks Kuntal. Jas, any further thoughts on the matter, ie re: browser
> support vs. spec designation?


Codereview isn't showing me the latest patch to comment on so commenting in
email.

I chimed in on the review mostly because I'd like to avoid the unnecessary
and error prone creation of a second list of html attributes.  Looking at
the CL more closely, I'd recommend against using Map<String, String> in:

   public final static Map<String, String> RESOURCE_TAGS =
     ImmutableMap.of(

since an element could contain multiple html attributes that need to be
rewritten.  Fr'instance iframe contains longdesc and src attributes - both
of which may be loaded by the browser and as such should be proxied.  Using
a Map will not be able to map iframe to both longdesc and src.  How about
something like the following which gives you the full html attribute list:

    HtmlSchema htmlSchema =
HtmlSchema.getDefault(DevNullMessageQueue.singleton());
    List<Pair<String, String>> result = Lists.newLinkedList();
    for (AttribKey attribName: htmlSchema.getAttributeNames()) {
      ElKey elName = attribName.el;
      Attribute attrib = htmlSchema.lookupAttribute(attribName);
      UriEffect effect = attrib.getUriEffect();

      // Collate all html attributes that contain a urls to resources that a
      // browser may load directly eg. includes img.src but not a.name or
a.href
      if (HTML.Attribute.Type.URI.equals(attrib.getType()) &&
          (UriEffect.SAME_DOCUMENT.equals(effect) ||
          UriEffect.NEW_DOCUMENT.equals(effect))) {
        result.add(Pair.of(elName.localName, attribName.localName));
      }
    }



On 2010/07/21 06:44:49, Kuntal Loya wrote:
>
>> Created a JIRA issue -
>>
> https://issues.apache.org/jira/browse/SHINDIG-1390
>
>
>  On 2010/07/20 10:06:32, anupama.dutta wrote:
>> > On 2010/07/16 16:51:20, gagan.goku wrote:
>> > > Thanks for the pointer Jasvir.
>> > > Seems we want the attributes with type = URI and uriEffect =
>>
> SAME_DOCUMENT.
>
>> > >
>> > > If i do a grep over the html4-attributes-defs.json file
>> > > in caja\src\com\google\caja\lang\html directory, i find the
>>
> following
>
>> > > attributes:
>> > > body background
>> > > object classid
>> > > object codebase
>> > > applet codebase
>> > > object data
>> > > link href
>> > > img longdesc
>> > > frame longdesc
>> > > iframe longdesc
>> > > head profile
>> > > script src
>> > > input src
>> > > frame src
>> > > iframe src
>> > > img src
>> > >
>> > > Sadly some (or most) of them (like longdesc etc) seem to be badly
>>
> supported
>
>> > > by browsers, and due to unexpected bugs we might not want to
>>
> handle all of
>
>> > > these.
>> > > But maybe a blacklist of attributes will work better. So we go
>>
> over every
>
>> > > node and each of its attributes (or prepopulate a map of allowed
>>
> node and
>
>> > > attribute) to find attributes matching our criterion and rewrite
>>
> them.
>
>> > >
>> > > Thoughts ?
>> >
>> >
>> > Agreed that this change to use the complete w3c spec list from the
>>
> class
>
>> pointed
>> > to by Jasvir, would be a good change. Kuntal, could you file a jira
>>
> issue for
>
>> > tracking this?
>> >
>> > I think we should go ahead with the current change for now, and make
>>
> the
>
>> > transition to the larger list in a subsequent change. Jasvir, Chirag
>>
> - do you
>
>> > think this will be fine?
>> >
>> > Thanks,
>> > Anupama.
>>
>
>
>
> http://codereview.appspot.com/1806044/show
>

[Jasvir, 2010-07-30 21:23:07]

LGTM after discussion on the list.

PS. Codereview still gives me:
Error fetching
http://svn.apache.org/repos/asf/shindig/trunk/java/java/common/conf/shindig.p...:
HTTP status 404

[johnfargo, 2010-07-30 23:26:24]

On Fri, Jul 30, 2010 at 2:23 PM, <jasvir@gmail.com> wrote:

> LGTM after discussion on the list.
>
> PS. Codereview still gives me:
> Error fetching
>
>
http://svn.apache.org/repos/asf/shindig/trunk/java/java/common/conf/shindig.p...
> :
> HTTP status 404
>
>
Source "Base" is set incorrectly.


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

[Kuntal Loya, 2010-07-31 04:17:02]

On 2010/07/30 23:26:24, johnfargo wrote:
> On Fri, Jul 30, 2010 at 2:23 PM, <mailto:jasvir@gmail.com> wrote:
> 
> > LGTM after discussion on the list.
> >
> > PS. Codereview still gives me:
> > Error fetching
> >
> >
>
http://svn.apache.org/repos/asf/shindig/trunk/java/java/common/conf/shindig.p...
> > :
> > HTTP status 404
> >
> >
> Source "Base" is set incorrectly.

You were right, Updated it. Diffs are working now.

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

[johnfargo, 2010-08-02 22:39:51]

LGTM, committing.

On 2010/07/31 04:17:02, Kuntal Loya wrote:
> On 2010/07/30 23:26:24, johnfargo wrote:
> > On Fri, Jul 30, 2010 at 2:23 PM, <mailto:jasvir@gmail.com> wrote:
> > 
> > > LGTM after discussion on the list.
> > >
> > > PS. Codereview still gives me:
> > > Error fetching
> > >
> > >
> >
>
http://svn.apache.org/repos/asf/shindig/trunk/java/java/common/conf/shindig.p...
> > > :
> > > HTTP status 404
> > >
> > >
> > Source "Base" is set incorrectly.
> 
> You were right, Updated it. Diffs are working now.
> 
> > 
> > 
> > >
> > > http://codereview.appspot.com/1806044/show
> > >
> >

[johnfargo, 2010-08-02 22:46:25]

Committed as r981700.

On 2010/08/02 22:39:51, johnfargo wrote:
> LGTM, committing.
> 
> On 2010/07/31 04:17:02, Kuntal Loya wrote:
> > On 2010/07/30 23:26:24, johnfargo wrote:
> > > On Fri, Jul 30, 2010 at 2:23 PM, <mailto:jasvir@gmail.com> wrote:
> > > 
> > > > LGTM after discussion on the list.
> > > >
> > > > PS. Codereview still gives me:
> > > > Error fetching
> > > >
> > > >
> > >
> >
>
http://svn.apache.org/repos/asf/shindig/trunk/java/java/common/conf/shindig.p...
> > > > :
> > > > HTTP status 404
> > > >
> > > >
> > > Source "Base" is set incorrectly.
> > 
> > You were right, Updated it. Diffs are working now.
> > 
> > > 
> > > 
> > > >
> > > > http://codereview.appspot.com/1806044/show
> > > >
> > >