uri normalization breaks several uris

When using a PluginEnvironment that allows URIs without rewriting,
the cajoler will turn this:
  <a href="mailto:a@b"></a>
  <a href="http://a.b/c;_d=e"></a>
into this:
  <a href="mailto:/a%40b"></a>
  <a href="http://a.b/c%3b%5fd%3de"></a>
which doesn't mean the same thing at all.

What's happening is the TemplateCompiler gets
a URI String from the PluginEnvironment,
then runs that String through normalizeUri,
which is a little buggy and mangles the URI

I don't see any particular reason to mistrust the
string returned by the PluginEnvironment.
So rather than trying to fix normalizeUri,
I've removed the call to normalizeUri.

I've also fixed the error message for invalid uris.

[MikeSamuel, 2009-07-07 18:20:34]

http://codereview.appspot.com/88118/diff/1/3
File src/com/google/caja/lexer/escaping/UriUtil.java (right):

http://codereview.appspot.com/88118/diff/1/3#newcode103
Line 103: if (path.length() != 0 || authority != null) {
Checking whether the authority is null is not the right way to check whether a
URL is opaque.  That depends on the scheme.

Can we assume that it is opaque iff if the scheme is in a set of known opaque
URL schemes?
Maybe construct a case-insensitive TreeSet<String> containing ("mailto", "data",
"javascript").

[felix8a, 2009-07-07 19:02:07]

On 2009/07/07 18:20:34, MikeSamuel wrote:
> http://codereview.appspot.com/88118/diff/1/3
> File src/com/google/caja/lexer/escaping/UriUtil.java (right):
> 
> http://codereview.appspot.com/88118/diff/1/3#newcode103
> Line 103: if (path.length() != 0 || authority != null) {
> Checking whether the authority is null is not the right way to check whether a
> URL is opaque.  That depends on the scheme.
> 
> Can we assume that it is opaque iff if the scheme is in a set of known opaque
> URL schemes?
> Maybe construct a case-insensitive TreeSet<String> containing ("mailto",
"data",
> "javascript").

according to
http://java.sun.com/j2se/1.5.0/docs/api/java/net/URI.html

a URI is opaque if
1. it's absolute, ie, it specifies a scheme, and
2. the scheme-specific part does not begin with '/'

I was working off that description, but now that I
think about it, the language may be misleading,
since it implies that "http:foo" is opaque.

I'll check the rfc and browser behavior.

[ihab.awad, 2009-07-08 18:21:35]

Felix, please let me know when this is ready for review.

[felix8a, 2009-07-14 11:02:43]

ok, looking at this some more, there are a lot more cases
where URIs get mangled by the TemplateCompiler.

the PluginEnvironment returns a URI string,
and then TemplateCompiler mangles it with normalizeUri,
which is buggy in a couple ways.

it seems to me that TemplateCompiler is being unreasonably
paranoid.  it should be able to trust the result from PluginEnvironment.
so, new snapshot will just remove the normalization.

[felix8a, 2009-07-15 08:36:10]

I missed a testcase that broke due to the normalization change.
new snapshot.

[ihab.awad, 2009-07-16 00:27:26]

Looks good, but see comments. What do you think?

http://codereview.appspot.com/88118/diff/2001/2005
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/88118/diff/2001/2005#newcode147
Line 147: "<form action='test:///testFormRewritten'"
The default form action URL, as used here, is being added by
TemplateCompiler.java; see line 225++:

  if (a.getType() == HTML.Attribute.Type.URI) {
    safeValue = Nodes.getFilePositionFor(el)
        .source().getUri().toString();
    // ...
  }

This code is generic for any URI-valued attribute; it simply selects what it
considers a "safe value".

Should we be calling the PluginEnvironment instead to get what *it* thinks is a
"safe" URI? Should that be [yet] an[other] callback on the PluginEnvironment?
Wouldn't that be more consistent with your "trust the PluginEnvironment
assertion?

It seems that a URI back to the original source code of a plugin (which is what
the existing code does) is only "safe" if the PluginEnvironment thinks so,
right?

[felix8a, 2009-07-16 19:43:47]

On 2009/07/16 00:27:26, ihab.awad wrote:
> Looks good, but see comments. What do you think?
> 
> http://codereview.appspot.com/88118/diff/2001/2005
> File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):
> 
> http://codereview.appspot.com/88118/diff/2001/2005#newcode147
> Line 147: "<form action='test:///testFormRewritten'"
> The default form action URL, as used here, is being added by
> TemplateCompiler.java; see line 225++:
> 
>   if (a.getType() == HTML.Attribute.Type.URI) {
>     safeValue = Nodes.getFilePositionFor(el)
>         .source().getUri().toString();
>     // ...
>   }
> 
> This code is generic for any URI-valued attribute; it simply selects what it
> considers a "safe value".
> 
> Should we be calling the PluginEnvironment instead to get what *it* thinks is
a
> "safe" URI? Should that be [yet] an[other] callback on the PluginEnvironment?
> Wouldn't that be more consistent with your "trust the PluginEnvironment
> assertion?
> 
> It seems that a URI back to the original source code of a plugin (which is
what
> the existing code does) is only "safe" if the PluginEnvironment thinks so,
> right?

yeah, but the safeValue does get sent through PluginEnvironment rewriteUri.
if a PluginEnvironment wants a safeValue other than the automatic one,
it can do the substitution then, right?

[ihab.awad, 2009-07-17 22:46:04]

LGTM++

Optional whether or not you want to implement PluginEnvironment.getSafeUri().