fix 1057, class= attributes are too restrictive

1. <div class="!@#$%"></div> is valid html, and it was accepted by
an earlier version of caja, but now it isn't.  This change removes
the constraint.  This is important because some coders and libs
use @class to store metadata about the node.

2. <input name="foo[]"> is not valid html, but it's commonly used,
and browsers allow it.  This change extends the range of allowed
characters in ids, and adds a browser-expectations test to verify
that browsers do not mangle weird ids.

3. browser-side sanitization didn't handle IDREFS attribute values
correctly.  This change fixes that.  At the moment, IDREFS are not
that useful, but they'll become more useful when the various aria
attributes are supported.

4. cajoler-side sanitizer mangles GLOBAL_NAME attributes, but
browser-side sanitizer doesn't.  This change makes them
both mangle.

5. browser-side and cajoler-side policies were inconsistent
about whether <input name='foo__'> is allowed or not.
This change removes the inconsistency.

6. some minor changes to jsunit.js to make it more convenient
on various browsers.

[ihab.awad, 2009-07-06 22:27:53]

http://codereview.appspot.com/89073/diff/10/1009
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/89073/diff/10/1009#newcode277
Line 277: if (!checkLegalSuffix(value, pos)) { return; }
It would be nice to add a nearby comment referring to
http://www.w3.org/TR/REC-html40/struct/global.html#h-7.5.2, which I presume is
the relevant legal basis.

http://codereview.appspot.com/89073/diff/10/1009#newcode392
Line 392: private boolean checkLegalSuffix(String value, FilePosition pos) {
Why are there not checkLegalSuffix and checkLegalSuffixes, for the one-valued
and many-valued cases, respectively, similar to checkRestrictedName and
checkRestrictedNames?

http://codereview.appspot.com/89073/diff/10/1007
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/89073/diff/10/1007#newcode302
Line 302: public void test1057ClassNames() throws Exception {
1057??

http://codereview.appspot.com/89073/diff/10/1007#newcode313
Line 313: htmlFragment(fromString("<div></div>")),
Right. It's ok that the entire "class" attribute is rejected, but the error
message should complain about the specific part of it that failed, i.e., "bad__"
rather than "bad__ ok". See comment in TemplateCompiler.java.

[ihab.awad, 2009-07-06 22:55:51]

http://codereview.appspot.com/89073/diff/10/1007
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/89073/diff/10/1007#newcode302
Line 302: public void test1057ClassNames() throws Exception {
Ah got it now. As in bug number 1057. It's a good idea imho, but we have not
done this anywhere else in our codebase, so it seems jarring in that context.
Perhaps rename the method and add a URL to the bug report in a comment?

[felix8a, 2009-07-07 12:53:52]

http://codereview.appspot.com/89073/diff/10/1009
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/89073/diff/10/1009#newcode392
Line 392: private boolean checkLegalSuffix(String value, FilePosition pos) {
On 2009/07/06 22:27:53, ihab.awad wrote:
> Why are there not checkLegalSuffix and checkLegalSuffixes, for the one-valued
> and many-valued cases, respectively, similar to checkRestrictedName and
> checkRestrictedNames?

mainly because I'm mimicking what domita.js does.
and since checkRestrictedName doesn't allow spaces in the value,
adding a single-value checkLegalSuffix won't change what
values are legal, it'll just change what error message you get.

but I guess it's better to not rely on the coupling.  ok, I'll fix it
both here and in domita.js

[ihab.awad, 2009-07-07 16:41:01]

http://codereview.appspot.com/89073/diff/10/1009
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/89073/diff/10/1009#newcode392
Line 392: private boolean checkLegalSuffix(String value, FilePosition pos) {
On 2009/07/07 12:53:53, felix8a wrote:
> it'll just change what error message you get.

Yep, true.

> I'll fix it both here and in domita.js

There's no reason to fix domita.js. It's okay for the domita.js checks to be
more brief and less "compiler"-like, as long as all the Java cajoler stuff is
internally consistent and all the run-time Domita stuff is internally
consistent. Fixing domita.js would imply changing a bunch of stuff that's
already there that already works.

[felix8a, 2009-07-07 17:25:49]

On 2009/07/07 16:41:01, ihab.awad wrote:
> http://codereview.appspot.com/89073/diff/10/1009
> File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):
> 
> http://codereview.appspot.com/89073/diff/10/1009#newcode392
> Line 392: private boolean checkLegalSuffix(String value, FilePosition pos) {
> On 2009/07/07 12:53:53, felix8a wrote:
> > it'll just change what error message you get.
> 
> Yep, true.
> 
> > I'll fix it both here and in domita.js
> 
> There's no reason to fix domita.js. It's okay for the domita.js checks to be
> more brief and less "compiler"-like, as long as all the Java cajoler stuff is
> internally consistent and all the run-time Domita stuff is internally
> consistent. Fixing domita.js would imply changing a bunch of stuff that's
> already there that already works.

ok, how about I rename it to checkLegalSuffixes and omit checkLegalSuffix?
I'd like to keep the code structurally similar to domita.js, because I think
the similarity makes it easier to verify they have similar behavior.

[ihab.awad, 2009-07-08 17:16:39]

The diffs in the last patch set are *huge* and caused by MarkM's recent
cajita.js refactoring showing up as "diffs". Is it possible to do an "svn up"
and then re-snapshot?

[felix8a, 2009-07-08 17:27:45]

new snapshot

[felix8a, 2009-07-09 07:52:06]

sorry, I need to snapshot again.
I have an open bug that overlaps this in a trivial way.
currently, el.className='' has no effect,
and there isn't a good reason for that to fail.
new snapshot changes an if statement and adds another domita test.

[ihab.awad, 2009-07-09 19:18:51]

A few comments -- will discuss further over chat and report back to the list.

http://codereview.appspot.com/89073/diff/3037/3040
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/89073/diff/3037/3040#newcode773
Line 773: '([^' + XML_SPACE + ']+)([' + XML_SPACE + ']|$)', 'g');
Plase move the above 4 lines down closer to the point of first use.

http://codereview.appspot.com/89073/diff/3037/3040#newcode849
Line 849: function(m0, m1, m2) {
Please give these parameters descriptive names. Like m0 -> "_" (unused); m1 ->
"idref", m2 -> "trailingWhitespace"?

http://codereview.appspot.com/89073/diff/3037/3040#newcode852
Line 852: === m1.substring(m1.length - idSuffix.length)) {
Please parcel up the above 3 lines into an "endsWith(aString, aSuffix)" function
and use it.

http://codereview.appspot.com/89073/diff/3037/3040#newcode892
Line 892: return null;
class = cdata-list. Ok.

http://codereview.appspot.com/89073/diff/3037/3040#newcode893
Line 893: case html4.atype.GLOBAL_NAME:
What is the justification for suffixing a GLOBAL_NAME (or any kind of "name" for
that matter) with an ID suffix? And if you suffix a GLOBAL_NAME, why not also a
LOCAL_NAME? Do you understand the source of the distinction in our schemata
between GLOBAL_NAME and LOCAL_NAME? The W3C page
http://www.w3.org/TR/html401/index/attributes.html, which is referenced from our
html4-attributes-defs.json, is not helpful in that regard.

http://codereview.appspot.com/89073/diff/3037/3040#newcode907
Line 907: return null;
Ok, good fix.

http://codereview.appspot.com/89073/diff/3037/3040#newcode910
Line 910: if (value && !illegalSuffix.test(value) && isXmlName(value)) {
Ok, good fix.

http://codereview.appspot.com/89073/diff/3037/3040#newcode2012
Line 2012: switch (atype) {
If you are going to add 'idSuffix' to GLOBAL_NAME attributes in
rewriteAttribute(), shouldn't you be stripping them off here?

http://codereview.appspot.com/89073/diff/3037/3040#newcode2026
Line 2026: function(m0, m1, m2) {
Please rename params; see above.

http://codereview.appspot.com/89073/diff/3037/3040#newcode2029
Line 2029: === m1.substring(m1.length - idSuffix.length)) {
Please use "endsWith"; see above.

http://codereview.appspot.com/89073/diff/3037/3041
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/89073/diff/3037/3041#newcode292
Line 292: if (!checkIllegalSuffixes(value, pos)) { return; }
Why remove illegal suffixes? We append an ID suffix where appropriate, and that
should be robust enough.

http://codereview.appspot.com/89073/diff/3037/3041#newcode297
Line 297: if (!checkIllegalSuffix(value, pos)) { return; }
Here and in 2 other places below: Why remove illegal suffixes?

http://codereview.appspot.com/89073/diff/3037/3041#newcode432
Line 432: return false;
Lookin' good. :)

[felix8a, 2009-07-20 00:40:20]

ok, finally popped my stack enough.
will re-snapshot.

On 2009/07/09 19:18:51, ihab.awad wrote:
> A few comments -- will discuss further over chat and report back to the list.
> 
> http://codereview.appspot.com/89073/diff/3037/3040
> File src/com/google/caja/plugin/domita.js (right):
> 
> http://codereview.appspot.com/89073/diff/3037/3040#newcode773
> Line 773: '([^' + XML_SPACE + ']+)([' + XML_SPACE + ']|$)', 'g');
> Plase move the above 4 lines down closer to the point of first use.

ok

> http://codereview.appspot.com/89073/diff/3037/3040#newcode849
> Line 849: function(m0, m1, m2) {
> Please give these parameters descriptive names. Like m0 -> "_" (unused); m1 ->
> "idref", m2 -> "trailingWhitespace"?

ok

> http://codereview.appspot.com/89073/diff/3037/3040#newcode852
> Line 852: === m1.substring(m1.length - idSuffix.length)) {
> Please parcel up the above 3 lines into an "endsWith(aString, aSuffix)"
function
> and use it.

wrote something simpler.
 
> http://codereview.appspot.com/89073/diff/3037/3040#newcode893
> Line 893: case html4.atype.GLOBAL_NAME:
> What is the justification for suffixing a GLOBAL_NAME (or any kind of "name"
for
> that matter) with an ID suffix? And if you suffix a GLOBAL_NAME, why not also
a
> LOCAL_NAME? Do you understand the source of the distinction in our schemata
> between GLOBAL_NAME and LOCAL_NAME? The W3C page
> http://www.w3.org/TR/html401/index/attributes.html, which is referenced from
our
> html4-attributes-defs.json, is not helpful in that regard.

I'm mirroring how TemplateCompiler behaves.
GLOBAL_NAMEs are things like <form name=xxx>,
and the reason for suffixing those is that IE exposes
those names as global vars, if there isn't already a
var declaration.

however, suffixing them screws up anchors, because
<a name=xxx> is also a GLOBAL_NAME.

I don't know yet how to fix anchors without opening
up the global namespace in IE.  I think there's an
open bug on this.

> http://codereview.appspot.com/89073/diff/3037/3040#newcode2012
> Line 2012: switch (atype) {
> If you are going to add 'idSuffix' to GLOBAL_NAME attributes in
> rewriteAttribute(), shouldn't you be stripping them off here?

yeah, fixed.

> http://codereview.appspot.com/89073/diff/3037/3041
> File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):
> 
> http://codereview.appspot.com/89073/diff/3037/3041#newcode292
> Line 292: if (!checkIllegalSuffixes(value, pos)) { return; }
> Why remove illegal suffixes? We append an ID suffix where appropriate, and
that
> should be robust enough.

In this case, we're not appending an idSuffix.

> http://codereview.appspot.com/89073/diff/3037/3041#newcode297
> Line 297: if (!checkIllegalSuffix(value, pos)) { return; }
> Here and in 2 other places below: Why remove illegal suffixes?

I'm wary of potential for abuse if the mangle/unmangle symmetry
ever accidentally gets screwed up.  given that I had to fix some cases
mangle/unmangle asymmetry, and still missed one, the paranoia
seems justified.

also it seems confusing to have different restrictions for only some
types of ID.

[ihab.awad, 2009-07-20 21:19:42]

LGTM++

http://codereview.appspot.com/89073/diff/5006/6008
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/89073/diff/5006/6008#newcode817
Line 817: }
Peaches and cream.

http://codereview.appspot.com/89073/diff/5006/6008#newcode2039
Line 2039: }
I know this is not within the scope of your immediate changes but, for
consistency, can you please mod the above lines to use unsuffix() as well?

http://codereview.appspot.com/89073/diff/5006/6009
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/89073/diff/5006/6009#newcode317
Line 317: if (!checkIllegalSuffix(value, pos)) { return; }
Didn't you mean checkIllegalSuffix*es*(...)?

http://codereview.appspot.com/89073/diff/5006/6007
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/89073/diff/5006/6007#newcode435
Line 435: //     new Block());
How about just checking for an AssertionFailure exception from assertSafeHtml or
something? In other words, given that there's a TODO here anyway, it's better to
have *a* test using a blunt instrument than comment out the test entirely?

[felix8a, 2009-08-05 23:30:45]

new snapshot:
simplified some code.
added browser-expectations test for nonstandard id chars.
allow some commonly used nonstandard id chars (eg "a$b[]")

[MikeSamuel, 2009-09-17 15:32:22]

On 2009/08/05 23:30:45, felix8a wrote:
> new snapshot:
> simplified some code.
> added browser-expectations test for nonstandard id chars.
> allow some commonly used nonstandard id chars (eg "a$b[]")

What needs to happen for this change to make it into trunk?

[MikeSamuel, 2009-09-22 05:01:21]

Ihab, do you mind if I take over as reviewer for this?

[MikeSamuel, 2009-09-23 23:29:42]

Resending.  Rietveld was misbehaving the first time I sent these, so I thought
I'd retry to get them in a more readable form.

http://codereview.appspot.com/89073/diff/10001/10008
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/89073/diff/10001/10008#newcode815
Line 815: if (typeof str !== 'string') return fail;
ok, so str can't change shape since you typecheck

http://codereview.appspot.com/89073/diff/10001/10008#newcode817
Line 817: if (0 < n && str.substring(n) === suffix) {
0 < n should be 0 <= n since str is a suffix of str for all str.

http://codereview.appspot.com/89073/diff/10001/10008#newcode828
Line 828: function virtualizeAttributeValue(attrType, realValue) {
do we need to force realValue to a string or do all clients do that.

http://codereview.appspot.com/89073/diff/10001/10008#newcode837
Line 837: return unsuffix(id, idSuffix, '') + spaces;
can we normalize spaces.  instead of returning
unsuffix(...) + spaces, maybe return unsuffix(...) + ' '.

http://codereview.appspot.com/89073/diff/10001/10008#newcode910
Line 910: function(_, id, spaces) { return id + idSuffix + spaces; });
Same as at 837.

http://codereview.appspot.com/89073/diff/10001/10007
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/89073/diff/10001/10007#newcode553
Line 553: }
Can you add an assertMessagesLessSevereThan(MessageLevel.WARNING) to
the end of the new tests.
If there should be warnings, you can use assertMessage(true, ...) to
check them and remove them from the queue.
I want to make sure that any warnings about sketchy classes stay visible.

[felix8a, 2009-09-24 20:49:58]

updated snapshot.

http://codereview.appspot.com/89073/diff/10001/10008
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/89073/diff/10001/10008#newcode817
Line 817: if (0 < n && str.substring(n) === suffix) {
On 2009/09/23 23:29:43, MikeSamuel wrote:
> 0 < n should be 0 <= n since str is a suffix of str for all str.

the way unsuffix is used, it should never return '' unless fail is also ''.
I'll fix the comment to clarify that.

http://codereview.appspot.com/89073/diff/10001/10008#newcode828
Line 828: function virtualizeAttributeValue(attrType, realValue) {
On 2009/09/23 23:29:43, MikeSamuel wrote:
> do we need to force realValue to a string or do all clients do that.

I was thinking that attribute values don't necessarily need to be
strings, so this leaves the value unchanged when no transformation
needs to be done.  but the DOM spec says they are always strings,
and I can't think of a case where they aren't.

ok, adding a coercion.

http://codereview.appspot.com/89073/diff/10001/10008#newcode837
Line 837: return unsuffix(id, idSuffix, '') + spaces;
On 2009/09/23 23:29:43, MikeSamuel wrote:
> can we normalize spaces.  instead of returning
> unsuffix(...) + spaces, maybe return unsuffix(...) + ' '.

ok

http://codereview.appspot.com/89073/diff/10001/10008#newcode910
Line 910: function(_, id, spaces) { return id + idSuffix + spaces; });
On 2009/09/23 23:29:43, MikeSamuel wrote:
> Same as at 837.

ok

http://codereview.appspot.com/89073/diff/10001/10007
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/89073/diff/10001/10007#newcode553
Line 553: }
On 2009/09/23 23:29:43, MikeSamuel wrote:
> Can you add an assertMessagesLessSevereThan(MessageLevel.WARNING) to
> the end of the new tests.
> If there should be warnings, you can use assertMessage(true, ...) to
> check them and remove them from the queue.
> I want to make sure that any warnings about sketchy classes stay visible.

ok.

[MikeSamuel, 2009-09-24 21:15:07]

LGTM

[felix8a, 2009-09-24 21:17:10]

On 2009/09/24 21:15:07, MikeSamuel wrote:
> LGTM

@r3743