Support attribute name virtualization in attribute selectors.

Add a hook to sanitizecss.js to allow rewriting attribute names in
attribute selectors, and use said hook from Domado in order to
support virtualized attributes (div[foo] becomes div[data-caja-foo],
consistently with the rest of our attribute support).

@r5548

[kpreid_google, 2013-08-02 18:14:33]

I think this particular design is terrible, because it introduces a new
interface which is sort-of-but-not the same thing as tagPolicy. Please help me
figure out a better solution.

In the event that we don't see improvements to make, I propose to do this as-is
except to make virtualizeAttrName _optional_ for compatibility with existing
external CSS sanitizer users.

[MikeSamuel, 2013-08-05 17:59:37]

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
src/com/google/caja/plugin/domado.js:2104: var dummyValue =
rewriteAttribute(tagName, attribName, atype, '');
I'm unclear what advantage we get from rewriting a bogus value.

Can this be reduced to lines 2112-2118?

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/san...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:508: var rAttr =
virtualization.virtualizeAttrName(element, vAttr);
Can avoid calling virtualizeAttrName unless atype !== +atype in 517.

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
File tests/com/google/caja/plugin/css-stylesheet-test.js (right):

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
tests/com/google/caja/plugin/css-stylesheet-test.js:74: // Create a test method
that will be called by jsUnit.
Maybe move inside loop or pluralize.

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
tests/com/google/caja/plugin/css-stylesheet-test.js:80: }
cool

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
File tests/com/google/caja/plugin/css-stylesheet-tests.js (left):

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
tests/com/google/caja/plugin/css-stylesheet-tests.js:87: "cssText": "div[zwop] {
color: blue }",
How about testing div[zwop__]?

[kpreid_google, 2013-08-05 19:03:23]

Add a hook to sanitizecss.js to allow rewriting attribute names in
attribute selectors, and use said hook from Domado in order to
support virtualized attributes (div[foo] becomes div[data-caja-foo],
consistently with the rest of our attribute support).

[kpreid_google, 2013-08-05 19:03:41]

new snapshot

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
src/com/google/caja/plugin/domado.js:2104: var dummyValue =
rewriteAttribute(tagName, attribName, atype, '');
On 2013/08/05 17:59:37, MikeSamuel wrote:
> I'm unclear what advantage we get from rewriting a bogus value.

There is no other entry point to handle a single attribute. As noted I'd rather
not have the value involved at all. The alternative here is to interpret the
attribute's 'atype' directly, but I wanted to reduce code duplication — we
already have too many different paths.

> Can this be reduced to lines 2112-2118?

No, because sanitizeOneAttr's job is to decide whether the attribute is
permitted and, if it is, should be virtualized. This requires consulting the
schema and atype.

I'm thinking you're thinking of your draft, which called the hook only if the
schema rejected it, but that is not the correct design, I believe, because this
should be parallel to tagPolicy, which is invoked regardless of the schema.
Fuller parameterizabiliy is better.

What I would prefer, however, is for this _not_ to be a special case method, and
have it subsumed by tagPolicy (or something like that) — but I haven't figured
out how to make that happen. That's the primary design problem I'm concerned
with.

One design I've thought of is to basically invoke tagPolicy with a single
attribute. The problem with that is the case where tagPolicy injects new
attributes solely based on the tag name (e.g. target=_blank). However, we could
somehow extend tagPolicy's interface to be able to ask it to do only
per-attribute sanitization, and also maybe ask it about an attribute-value
pattern rather than a literal value (so it can handle the attribute-value
selectors), but it's unclear to me how to do that cleanly.

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/san...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:508: var rAttr =
virtualization.virtualizeAttrName(element, vAttr);
On 2013/08/05 17:59:37, MikeSamuel wrote:
> Can avoid calling virtualizeAttrName unless atype !== +atype in 517.

This is deliberate; note that at line 518 we permit even if the schema rejects,
so that virtualizeAttrName is the sole arbiter of permission. (I'm open to the
idea that we should change this design, but I'm pointing out that that _is_ the
consistent design.)

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
File tests/com/google/caja/plugin/css-stylesheet-test.js (right):

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
tests/com/google/caja/plugin/css-stylesheet-test.js:74: // Create a test method
that will be called by jsUnit.
On 2013/08/05 17:59:37, MikeSamuel wrote:
> Maybe move inside loop or pluralize.

Belonged to the function above. Fixed.

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
File tests/com/google/caja/plugin/css-stylesheet-tests.js (left):

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
tests/com/google/caja/plugin/css-stylesheet-tests.js:87: "cssText": "div[zwop] {
color: blue }",
On 2013/08/05 17:59:37, MikeSamuel wrote:
> How about testing div[zwop__]?

__ matching is handled in the provided virtualizeAttrName, not inside the
sanitizer — the code path for rejection is tested above using "rejectedfortest"
as the string.

[MikeSamuel, 2013-08-06 15:31:27]

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
src/com/google/caja/plugin/domado.js:2104: var dummyValue =
rewriteAttribute(tagName, attribName, atype, '');
On 2013/08/05 19:03:42, kpreid_google wrote:
> On 2013/08/05 17:59:37, MikeSamuel wrote:
> > I'm unclear what advantage we get from rewriting a bogus value.
> 
> There is no other entry point to handle a single attribute. As noted I'd
rather
> not have the value involved at all. The alternative here is to interpret the
> attribute's 'atype' directly, but I wanted to reduce code duplication — we
> already have too many different paths.
> 
> > Can this be reduced to lines 2112-2118?
> 
> No, because sanitizeOneAttr's job is to decide whether the attribute is
> permitted and, if it is, should be virtualized. This requires consulting the
> schema and atype.
> 
> I'm thinking you're thinking of your draft, which called the hook only if the
> schema rejected it, but that is not the correct design, I believe, because
this
> should be parallel to tagPolicy, which is invoked regardless of the schema.
> Fuller parameterizabiliy is better.
> 
> What I would prefer, however, is for this _not_ to be a special case method,
and
> have it subsumed by tagPolicy (or something like that) — but I haven't figured
> out how to make that happen. That's the primary design problem I'm concerned
> with.
> 
> One design I've thought of is to basically invoke tagPolicy with a single
> attribute. The problem with that is the case where tagPolicy injects new
> attributes solely based on the tag name (e.g. target=_blank). However, we
could
> somehow extend tagPolicy's interface to be able to ask it to do only
> per-attribute sanitization, and also maybe ask it about an attribute-value
> pattern rather than a literal value (so it can handle the attribute-value
> selectors), but it's unclear to me how to do that cleanly.

tagPolicy seems a weird fit.

With tagPolicy, we have

1. A definite element name
2. A list of attributes
3. An attribute value.

and we return

4. An updated list of attributes,
5. possibly including added attributes

Here, we have (1) if we're lucky, a trivial form of (2), and the lack of (3) is
causing grief.  We need back, a trivial form of (4) but definitely need to
distinguish between (4) and (5) which we don't need.

While I agree that it would be good to avoid duplicating parts of the policy,
the level of overlap seems low.

Also, I think we risk conflating *policy* and *fact* if we delegate too much to
tagPolicy.
Whether to allow an attribute is a policy decision, but whether an HTML
attribute's value is a URI is a fact.

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/san...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/san...
src/com/google/caja/plugin/sanitizecss.js:508: var rAttr =
virtualization.virtualizeAttrName(element, vAttr);
On 2013/08/05 19:03:42, kpreid_google wrote:
> On 2013/08/05 17:59:37, MikeSamuel wrote:
> > Can avoid calling virtualizeAttrName unless atype !== +atype in 517.
> 
> This is deliberate; note that at line 518 we permit even if the schema
rejects,
> so that virtualizeAttrName is the sole arbiter of permission. (I'm open to the
> idea that we should change this design, but I'm pointing out that that _is_
the
> consistent design.)

Agreed re "_is_".  I think part of the problem is that I didn't create a clear
distinction between policy and fact when I architected some of these things.

Clearly separating those two things is out of scope for this CL though so I'm
inclined to say it's your call.

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
File tests/com/google/caja/plugin/css-stylesheet-tests.js (left):

https://codereview.appspot.com/12367043/diff/1/tests/com/google/caja/plugin/c...
tests/com/google/caja/plugin/css-stylesheet-tests.js:87: "cssText": "div[zwop] {
color: blue }",
On 2013/08/05 19:03:42, kpreid_google wrote:
> On 2013/08/05 17:59:37, MikeSamuel wrote:
> > How about testing div[zwop__]?
> 
> __ matching is handled in the provided virtualizeAttrName, not inside the
> sanitizer — the code path for rejection is tested above using
"rejectedfortest"
> as the string.

Makes sense.

[kpreid_google, 2013-08-07 17:59:23]

Add a hook to sanitizecss.js to allow rewriting attribute names in
attribute selectors, and use said hook from Domado in order to
support virtualized attributes (div[foo] becomes div[data-caja-foo],
consistently with the rest of our attribute support).

[kpreid_google, 2013-08-07 18:02:53]

New snapshot:
- virtualizeAttrName is now _optional_ for interface compatibility.
- funny characters are now escaped — issue 1617 should be fixed by this.

Also, I actually ran the tests. Unfortunately, it fails because the cajoler does
not implement the new CSS functionality.

I'll work on adding that, but feel free to review the JS code here like it was
going to be submitted.

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/12367043/diff/1/src/com/google/caja/plugin/dom...
src/com/google/caja/plugin/domado.js:2104: var dummyValue =
rewriteAttribute(tagName, attribName, atype, '');
On 2013/08/06 15:31:28, MikeSamuel wrote:
> While I agree that it would be good to avoid duplicating parts of the policy,
> the level of overlap seems low.

Agreed.

> Also, I think we risk conflating *policy* and *fact* if we delegate too
> much to tagPolicy.
> Whether to allow an attribute is a policy decision, but whether an
> HTML attribute's value is a URI is a fact.

Agreed. I hope the current code makes a reasonable distinction there.

The current design of HTML schema unfortunately conflates policy and fact, in
that they're all bundled into html4-defs; this is something I need to fix for
iframes. Also, relevant here, a rejected attribute is not distinguished from an
unknown one.

[MikeSamuel, 2013-08-07 18:33:52]

LGTM

https://codereview.appspot.com/12367043/diff/15001/src/com/google/caja/plugin...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/12367043/diff/15001/src/com/google/caja/plugin...
src/com/google/caja/plugin/sanitizecss.js:610: attrs += '[' +
rAttr.replace(/[^\w-]/g, '\\$&') + op + value +
Cool

[kpreid_google, 2013-08-12 19:37:48]

Add a hook to sanitizecss.js to allow rewriting attribute names in
attribute selectors, and use said hook from Domado in order to
support virtualized attributes (div[foo] becomes div[data-caja-foo],
consistently with the rest of our attribute support).

[kpreid_google, 2013-08-12 19:39:03]

New snapshot disables the new tests for ES5/3 mode, because changing the cajoler
looked more trouble than it's worth. No changes other than in tests/; please
review.

[ihab.awad, 2013-08-12 21:16:25]

lgtm

https://codereview.appspot.com/12367043/diff/22001/tests/com/google/caja/plug...
File tests/com/google/caja/plugin/css-stylesheet-test.js (right):

https://codereview.appspot.com/12367043/diff/22001/tests/com/google/caja/plug...
tests/com/google/caja/plugin/css-stylesheet-test.js:42: return 'data-caja-' +
attrName;
Ah.