Factor out a replaceable tagPolicy to enable customization of the sanitizer.

This exposes a hook named 'tagPolicy' in the sanitizer so that the
sanitizer can be customized to accept or transform particular tags,
for example, to accept <iframe> tags with a specific SRC, or transform
<embed> tags into <iframe> tags.
 
makeTagPolicy, sanitizeAttribs, and sanitizeWithPolicy are factored 
out so that they can be easily reused in custom tagPolicy functions.

[MikeSamuel, 2011-10-06 12:30:22]

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:1: // Copied from r4631 of the
google-caja project, then modified for MapShop.
Cruft?

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:132: * {@ updoc
Please consistently neuter the JSDoc tags.  \@updoc seems fine.

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:180: * {@ updoc
here too

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:379: * @param {Function} tagPolicy
A function that takes (tagName, attribs[]),
Maybe Function -> function(string, Array)Array? or whatever?

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:402: }
Ok.  Looks like
    if (eflags & html4.eflags.UNSAFE) {
has moved to makeTagPolicy.

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:550: attribs[i + 1] = value ||
null;
What is the `|| null` for?  Does this conflate <input checked=""> and <input>?

[kpy, 2011-10-06 13:52:02]

Thanks!  All done, please take another look.

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:1: // Copied from r4631 of the
google-caja project, then modified for MapShop.
On 2011/10/06 12:30:22, MikeSamuel wrote:
> Cruft?

Removed.

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:132: * {@ updoc
On 2011/10/06 12:30:22, MikeSamuel wrote:
> Please consistently neuter the JSDoc tags.  \@updoc seems fine.

Oops, sorry.  Cleaned this up.

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:180: * {@ updoc
On 2011/10/06 12:30:22, MikeSamuel wrote:
> here too

Done.

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:379: * @param {Function} tagPolicy
A function that takes (tagName, attribs[]),
On 2011/10/06 12:30:22, MikeSamuel wrote:
> Maybe Function -> function(string, Array)Array? or whatever?

Done (also reworded the doc string somewhat, have another look).

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:402: }
On 2011/10/06 12:30:22, MikeSamuel wrote:
> Ok.  Looks like
>     if (eflags & html4.eflags.UNSAFE) {
> has moved to makeTagPolicy.

Yup.  This is so that tagPolicy can be customized to allow specific exceptions.

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:550: attribs[i + 1] = value ||
null;
On 2011/10/06 12:30:22, MikeSamuel wrote:
> What is the `|| null` for?

Yes, this causes attributes with value = null, undefined, or '' to disappear.

> Does this conflate <input checked=""> and <input>?

Yes, and I think this is correct behaviour.  <input checked> is equivalent to
<input checked="checked">, not <input checked="">.

http://www.w3.org/TR/html4/intro/sgmltut.html#h-3.3.4.2

[MikeSamuel, 2011-10-06 23:09:18]

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:550: attribs[i + 1] = value ||
null;
On 2011/10/06 13:52:02, kpy wrote:
> On 2011/10/06 12:30:22, MikeSamuel wrote:
> > What is the `|| null` for?
> 
> Yes, this causes attributes with value = null, undefined, or '' to disappear.
> 
> > Does this conflate <input checked=""> and <input>?
> 
> Yes, and I think this is correct behaviour.  <input checked> is equivalent to
> <input checked="checked">, not <input checked="">.
> 
> http://www.w3.org/TR/html4/intro/sgmltut.html#h-3.3.4.2

<input type="checkbox" checked="">
shows a checked checkbox on all the browsers I tried.

http://www.w3.org/TR/html5/common-microsyntaxes.html#boolean-attributes says

2.5.2 Boolean attributes

"""
A number of attributes are boolean attributes. The presence of a boolean
attribute on an element represents the true value, and the absence of the
attribute represents the false value.
"""

[kpy, 2011-10-07 18:15:38]

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/4641064/diff/11002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:550: attribs[i + 1] = value ||
null;
Oh, looks like you're right!  Fixed.

http://codereview.appspot.com/4641064/diff/21001/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/4641064/diff/21001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:536: if (value && '#' ===
value.charAt(0)) {
I realized I don't really understand what's going on here.  This code preserves
the current behaviour, but why is it correct to pass in a #-prefixed string to
opt_nmTokenPolicy and then expect it to return a non-#-prefixed string?  If
opt_nmTokenPolicy is the identity function, won't this result in usemap="#foo"
turning into usemap="##foo"?

[MikeSamuel, 2011-10-09 07:02:36]

LGTM modulo a fix for the bug you identified.

http://codereview.appspot.com/4641064/diff/21001/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/4641064/diff/21001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:536: if (value && '#' ===
value.charAt(0)) {
On 2011/10/07 18:15:38, kpy wrote:
> I realized I don't really understand what's going on here.  This code
preserves
> the current behaviour, but why is it correct to pass in a #-prefixed string to
> opt_nmTokenPolicy and then expect it to return a non-#-prefixed string?  If
> opt_nmTokenPolicy is the identity function, won't this result in usemap="#foo"
> turning into usemap="##foo"?

I think this is buggy.

If the URI starts with '#' then it is an in-document reference, so scrolls to an
element with the ID or NAME attribute that is value.substring(1).  Line 537
should probably do a substring(1).
The name policy then would return a name or ID which would have to be properly
prefixed to make a URI reference.

[kpy, 2011-10-19 00:18:42]

http://codereview.appspot.com/4641064/diff/21001/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/4641064/diff/21001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:536: if (value && '#' ===
value.charAt(0)) {
On 2011/10/09 07:02:36, MikeSamuel wrote:
> On 2011/10/07 18:15:38, kpy wrote:
> > I realized I don't really understand what's going on here.  This code
> preserves
> > the current behaviour, but why is it correct to pass in a #-prefixed string
to
> > opt_nmTokenPolicy and then expect it to return a non-#-prefixed string?  If
> > opt_nmTokenPolicy is the identity function, won't this result in
usemap="#foo"
> > turning into usemap="##foo"?
> 
> I think this is buggy.
> 
> If the URI starts with '#' then it is an in-document reference, so scrolls to
an
> element with the ID or NAME attribute that is value.substring(1).  Line 537
> should probably do a substring(1).
> The name policy then would return a name or ID which would have to be properly
> prefixed to make a URI reference.

Done.

[MikeSamuel, 2011-10-31 16:16:09]

On 2011/10/19 17:42:00, zestyping wrote:

Took over at http://codereview.appspot.com/5305081