Skip to content

Commit

Permalink
polymer-resin: test and fix handling of orphan text in shadow DOM mode
Browse files Browse the repository at this point in the history
----

Background:

I narrowed down the failure of gr-tooltip-test in
https://gerrit-review.googlesource.com/c/106190

The failure only manifests in shadow DOM mode which is why it was not
showing up in local runs.

https://gerrit.googlesource.com/gerrit/+/master/polygerrit-ui/app/test/index.html#122
  for (let file of elements) {
    file = elementsPath + file;
    testFiles.push(file);
    testFiles.push(file + '?dom=shadow');
  }
takes the cross product of (*.html) x (normal DOM, shadow DOM).

When testing locally, I was not appending ?dom=shadow.

----

I. Run tests in shadow DOM mode

This adds another wct_test_suite that runs the same tests as the
first but with an incantation that causes it to run via shadow DOM.

I can't find any docs on settings={"dom": "shadow"}, but it appears
in other BUILD files and, until I fixed the test JS files, all the
shadow DOM tests were failing.

----

II. Fixup tests to work in shadow DOM mode.

The tests used fixture(...) to load a test fixture, but then assumed
that DOM searches from that node would reach into the content.

That is not the case with the shadow DOM where every custom element
has a disconnected fragment that represents its content.

The relevant parts of
https://www.polymer-project.org/1.0/docs/devguide/local-dom#dom-api
that are necessary to understand the test changes are

> Work with local DOM

> Every Polymer element has a this.root property which is the root of
> its local DOM tree. You can manipulate the tree using Polymer.dom
> methods:

>   // Append to local DOM
>   var toLocal = document.createElement('div');
>   Polymer.dom(this.root).appendChild(toLocal);

> For locating dynamically-created nodes in your element's local DOM,
> the $$ method provides a shorthand for
>   Polymer.dom(this.root).querySelector():

>   this.$$(selector)
> $$ returns the first node in the local DOM that matches selector.

----

III. Fixing rootless nodes.

When vetting text nodes, Polymer-resin looks at the parent element.

In
   <template>
     [[foo]]
   </template>
foo specifies a text node that appears at the top level of a
custom element's content tree.

In shadow DOM mode, the parent of the text node is a document
fragment, not an element.

I changed polymer-resin to allow text nodes when there is no
parent element because in that case there is no direct path from
text node addition to parsing as JS, CSS, etc.

TODO:
I have not yet tested this in the context:

  <script is="my-custom-element"></script>

but custom-builtin elements are deprecated in Polymer2.0.
It is unclear that text in a customized-builtin script
element's shadow DOM content is JavaScript.

Testing:
I added the separate shadow DOM tests, ran all tests,
and observed that all the new tests were broken, so
I concluded that the new test did exercise a different
point in the Polymer configuration space.

I iteratively reworked test code to get tests green
using Polymer.dom and abbreviations.
During this I noticed that there was a test that was
not installing polymer-resin.  I fixed this and
`grep -c`ed to check that there were no others.
After several rounds of this there were two failing
tests that failed in shadow mode: src-attra-as-text, tooltip.
I ran both interactively and noted that both involved text
nodes with a null parentElement.
The tooltip-test was derived from gerrit's gr-tooltip-test so
I concluded that fixing these tests would probably address
the remaining failure in gerrit's test suite.

I tweaked polymer-resin and reran all tests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=157460289
  • Loading branch information
msamuel authored and mikesamuel committed May 30, 2017
1 parent 0dcd499 commit d625532
Show file tree
Hide file tree
Showing 15 changed files with 52 additions and 47 deletions.
2 changes: 1 addition & 1 deletion a-tag-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ suite(
});

function getA(id) {
return toCheck.querySelector('#' + id);
return toCheck.$$('#' + id);
}

test('innocuous_string', function() {
Expand Down
8 changes: 4 additions & 4 deletions attr-property-aliasing-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ suite(

setup(function () {
buttons = fixture('attr-property-aliasing-fixture');
propertyButton = buttons.querySelector('.property-button');
attributeButton = buttons.querySelector('.attribute-button');
propertyButton = buttons.$$('.property-button');
attributeButton = buttons.$$('.attribute-button');
customButton = buttons
.querySelector('.custom-button')
.querySelector('button');
.$$('.custom-button')
.$$('button');
});


Expand Down
17 changes: 10 additions & 7 deletions computed-value-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,31 +28,34 @@ suite(

function () {
var computedValueFixture;
var links;

setup(function (done) {
computedValueFixture = fixture('computed-value-fixture');
computedValueFixture.links = links;
flush(done); // Don't run tests until dom-repeat terminates
computedValueFixture.links = linkContent;
flush(function () {
// Don't run tests until dom-repeat terminates
links = Polymer.dom(computedValueFixture.root).querySelectorAll('a');
done();
});
});

var links = [
{ "url": "http://example.com/#frag", text: "example" },
{ "url": "javascript:alert(1)", text: "XSS" }
var linkContent = [
{ url: "http://example.com/#frag", text: "example" },
{ url: "javascript:alert(1)", text: "XSS" }
];

function trim(s) {
return (s || '').replace(/^\s+|\s+$/g, '');
}

test('urls', function() {
var links = computedValueFixture.querySelectorAll('a');
assert.equal(2, links.length);
assert.equal('http://example.com/', links[0].href);
assert.equal(goog.html.SafeUrl.INNOCUOUS_STRING, links[1].href);
});

test('text', function() {
var links = computedValueFixture.querySelectorAll('a');
assert.equal(2, links.length);
assert.equal('example (example.com)', trim(links[0].textContent));
assert.equal('XSS ()', trim(links[1].textContent));
Expand Down
6 changes: 3 additions & 3 deletions custom-element-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ suite(
// Grab elements from under the given <custom-tag>'s shadow root
// so that the tests below can easily inspect their attributes
// and properties.
var links = customTag.querySelectorAll('a');
var links = Polymer.dom(customTag.root).querySelectorAll('a');
decomposed = {
outerDiv: customTag.querySelector('div'),
outerDiv: customTag.$$('div'),
dynLink: links[0], // The <a href="[[...]]"><img ...> ([[...]])</a>
img: customTag.querySelector('img'),
img: customTag.$$('img'),
staticLink: links[1] // The <a href="javascript:...">fixed text</a>
};
});
Expand Down
4 changes: 2 additions & 2 deletions enum-attribute-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ suite(
});

test('blank', function() {
var link = enumAttrFixture.querySelector('a');
var link = enumAttrFixture.$$('a');
enumAttrFixture.x = '_blank';

assert.equal('_blank', link.target);
});

test('evil_payload', function() {
var link = enumAttrFixture.querySelector('a');
var link = enumAttrFixture.$$('a');
enumAttrFixture.x = 'login-form';

assert.equal('zClosurez', link.target);
Expand Down
4 changes: 2 additions & 2 deletions identifier-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ suite(

setup(function () {
identifierFixture = fixture('identifier-test-fixture');
input = identifierFixture.querySelector('input');
label = identifierFixture.querySelector('label');
input = identifierFixture.$$('input');
label = identifierFixture.$$('label');
});

function assertId(want, inputValue) {
Expand Down
6 changes: 3 additions & 3 deletions one-attr-binding-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,22 @@ suite(
});

test('innocuous_string', function() {
var link = oneAttrFixture.querySelector('a');
var link = oneAttrFixture.$$('a');
oneAttrFixture.x = 'http://example.com/foo';

assert.equal('http://example.com/foo', link.href);
});

test('safe_url', function() {
var link = oneAttrFixture.querySelector('a');
var link = oneAttrFixture.$$('a');
oneAttrFixture.x = goog.html.SafeUrl.fromConstant(
goog.string.Const.from('javascript:safe()'));

assert.equal('javascript:safe()', link.href);
});

test('evil_payload', function() {
var link = oneAttrFixture.querySelector('a');
var link = oneAttrFixture.$$('a');
oneAttrFixture.x = 'javascript:evil()';

assert.equal(
Expand Down
6 changes: 3 additions & 3 deletions one-late-attr-binding-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ suite(
oneLateAttrFixture.items = ['http://example.com/foo'];
flush(
function () {
var link = oneLateAttrFixture.querySelector('a');
var link = oneLateAttrFixture.$$('a');
assert.equal('http://example.com/foo', link.href);
done();
});
Expand All @@ -44,7 +44,7 @@ suite(
];
flush(
function () {
var link = oneLateAttrFixture.querySelector('a');
var link = oneLateAttrFixture.$$('a');
assert.equal('javascript:safe()', link.href);
done();
});
Expand All @@ -53,7 +53,7 @@ suite(
test('evil_payload', function(done) {
oneLateAttrFixture.items = ['javascript:evil()'];
flush(function () {
var link = oneLateAttrFixture.querySelector('a');
var link = oneLateAttrFixture.$$('a');
assert.equal(
goog.html.SafeUrl.INNOCUOUS_STRING,
link.href);
Expand Down
16 changes: 8 additions & 8 deletions polymer-resin.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,13 +374,13 @@ security.polymer_resin.install = function (opt_config) {
// Whitelist and handle text node interpolation by checking
// the content type of the parent node.
var parentElement = node.parentElement;
var allowText = !parentElement;
if (parentElement
&& parentElement.nodeType === goog.dom.NodeType.ELEMENT) {
var parentElementName = parentElement.localName;
var parentClassification = security.polymer_resin.classifyElement(
parentElementName,
/** @type{!Function} */(parentElement.constructor));
var allowText = false;
switch (parentClassification) {
case security.polymer_resin.CustomElementClassification.BUILTIN:
case security.polymer_resin.CustomElementClassification.LEGACY:
Expand All @@ -395,13 +395,13 @@ security.polymer_resin.install = function (opt_config) {
allowText = true;
break;
}
if (allowText) {
return (
!!(value && value.implementsGoogStringTypedString)
? (/** @type {!goog.string.TypedString} */(value))
.getTypedStringValue()
: String(value));
}
}
if (allowText) {
return (
!!(value && value.implementsGoogStringTypedString)
? (/** @type {!goog.string.TypedString} */(value))
.getTypedStringValue()
: String(value));
}
}

Expand Down
1 change: 1 addition & 0 deletions src-attr-as-text-test.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<script src="/components/web-component-tester/browser.js"></script>
<link rel="import" href="/components/polymer/polymer.html" />
<script src="polymer-resin.js"></script>
<script>security.polymer_resin.install();</script>
<script src="src-attr-as-text-test.js"></script>
<title>Src Attr As Text Test</title>
</head>
Expand Down
6 changes: 3 additions & 3 deletions src-attr-as-text-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ suite(
test('innocuous_string', function() {
srcAttrAsTextFixture.src = 'Java joe\'s';
assert.equal('I bought a coffee at Java joe\'s then I dropped it.',
srcAttrAsTextFixture.textContent);
Polymer.dom(srcAttrAsTextFixture.root).textContent);
});

test('bad_url_as_text', function() {
srcAttrAsTextFixture.src = 'javascript:joe(\'s\')';
assert.equal(
'I bought a coffee at javascript:joe(\'s\') then I dropped it.',
srcAttrAsTextFixture.textContent);
Polymer.dom(srcAttrAsTextFixture.root).textContent);
});

test('typed_string_is_unwrapped', function() {
Expand All @@ -48,7 +48,7 @@ suite(
// computeFinalAnnotationValue. This seems different from that
// seen by other test cases. Why is it?
// assert.equal('I bought a coffee at safe/value then I dropped it.',
// srcAttrAsTextFixture.textContent);
// Polymer.dom(srcAttrAsTextFixture.root).textContent);
});

});
10 changes: 5 additions & 5 deletions standalone/polymer-resin-debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -2950,9 +2950,9 @@ security.polymer_resin.install = function(opt_config) {
var nodeType = node.nodeType;
if (nodeType !== goog.dom.NodeType.ELEMENT) {
if (nodeType === goog.dom.NodeType.TEXT) {
var parentElement = node.parentElement;
var parentElement = node.parentElement, allowText = !parentElement;
if (parentElement && parentElement.nodeType === goog.dom.NodeType.ELEMENT) {
var parentElementName = parentElement.localName, allowText = !1;
var parentElementName = parentElement.localName;
switch(security.polymer_resin.classifyElement(parentElementName, parentElement.constructor)) {
case security.polymer_resin.CustomElementClassification.BUILTIN:
case security.polymer_resin.CustomElementClassification.LEGACY:
Expand All @@ -2962,9 +2962,9 @@ security.polymer_resin.install = function(opt_config) {
case security.polymer_resin.CustomElementClassification.CUSTOM:
allowText = !0;
}
if (allowText) {
return value && value.implementsGoogStringTypedString ? value.getTypedStringValue() : String(value);
}
}
if (allowText) {
return value && value.implementsGoogStringTypedString ? value.getTypedStringValue() : String(value);
}
}
security.polymer_resin.reportHandler_ && security.polymer_resin.reportHandler_(!0, "Failed to sanitize %s %s%s node to value %O", node.parentElement && node.parentElement.nodeName, "#text", "", value);
Expand Down
6 changes: 3 additions & 3 deletions text-node-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ suite(

setup(function () {
nodes = fixture('text-node-test');
divElement = nodes.querySelector('div');
objectElement = nodes.querySelector('object');
scriptElement = nodes.querySelector('script');
divElement = nodes.$$('div');
objectElement = nodes.$$('object');
scriptElement = nodes.$$('script');
});


Expand Down
1 change: 1 addition & 0 deletions tooltip-test.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<script src="/components/web-component-tester/browser.js"></script>
<link rel="import" href="/components/polymer/polymer.html" />
<script src="polymer-resin.js"></script>
<script>security.polymer_resin.install();</script>
<script src="tooltip-test.js"></script>
<title>Tooltip Tests</title>
</head>
Expand Down
6 changes: 3 additions & 3 deletions tooltip-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ suite(
function () {
var testFixture;
var spanElement;
var textNode;

var evilDone = false;
goog.exportSymbol('tooltip_tests.doEvil', function () {
Expand All @@ -29,7 +28,7 @@ suite(

setup(function () {
testFixture = fixture('tooltip-test-fixture');
spanElement = testFixture.querySelector('span');
spanElement = testFixture.$$('span');
});


Expand All @@ -47,7 +46,8 @@ suite(
testFixture.content = 'Hello, World!';

var textNodeValue;
for (var child = testFixture.firstChild; child;
for (var child = Polymer.dom(testFixture.root).firstChild;
child;
child = child.nextSibling) {
if (child.nodeType == Node.TEXT_NODE
&& /\S/.test(child.nodeValue)) {
Expand Down

0 comments on commit d625532

Please sign in to comment.