Skip to content

Update test assertions when default policy moves an attribute during … #51459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update test assertions when default policy moves an attribute during … #51459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions trusted-types/set-attributes-mutations-in-callback.tentative.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
if (window.applyMutation) {
assert_equals(input, input_string);
window.applyMutation();
return output_string;
}
return output_string;
return input;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, probably we should keep the function as is and only use it for the default policy. Probably renaming it applyDefaultPolicyMaybeApplyingMutation or something like that.

We can have a separate policy that just returns the input, for the sake of creating trusted types.

}
trustedTypes.createPolicy("default", {
createHTML: createTrustedType,
Expand Down Expand Up @@ -165,9 +166,10 @@
testData.attrName),
output_string);
assert_equals(otherElement.attributes.length, 1);
// The attribute is moved to the other element before its value is updated.
assert_equals(otherElement.getAttributeNS(testData.attrNS,
testData.attrName),
output_string);
other_string);
break;
case "Element.setAttributeNode":
case "Element.setAttributeNodeNS":
Expand All @@ -187,21 +189,23 @@
assert_equals(otherElement.attributes.length, 1);
assert_equals(otherElement.getAttributeNS(testData.attrNS,
testData.attrName),
output_string);
input_string);
break;
case "Attr.value":
case "Node.nodeValue":
case "Node.textContent":
// These APIs successfully sets the attribute node value, the default
// policy's callback moved that node on the other element.
// These APIs early return when the element changes, but the default policy's
// callback still sets the attribute on the other element.
returnValue = setterData.runSetter(element, testData.attrNS,
testData.attrName,
input_string, testData.type);
input_string, testData.type, other_string);

assert_equals(element.attributes.length, 0);
assert_equals(otherElement.attributes.length, 1);
// The attribute is moved to the other element, with its initial value of an empty string.
assert_equals(otherElement.getAttributeNS(testData.attrNS,
testData.attrName),
output_string);
other_string);
break;
default:
assert_unreached();
Expand Down
14 changes: 7 additions & 7 deletions trusted-types/support/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,9 @@ const attributeSetterData = [
{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't comment on the actual lines but the 4 code blocks above this mean that move attribute to other element tests for those sinks aren't really doing what they say on the tin.

Because they're not moving the attribute to another element. They're making a brand new atribute and moving that instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't they be something like

      this.lastAttributeNode = findAttribute(element, attrNS, attrName);
      const newAttributeNode = document.createAttributeNS(attrNS, attrName);
      newAttributeNode.value = attrValue;
      return element.setAttributeNode(newAttributeNode);

This would no longer throw an InUseException but that's correct, because it wouldn't be inuse on the element anymore (because it was moved?)

I think we need a separate test for the specifics of the exceptions. Because we also need to test the ordering anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "move attribute node to another element" refers to the action of window.applyMutation which removes the attribute from setterData.lastAttributeNode.ownerElement and attaches it to otherElement.

Now it's true that when the ownerElement is null, we are not moving FROM the element to another element but just attaching it to another element.

And the original attribute node that was attached on the element at the beginning of the test is just not changed.

I think current code is aligned with existing documentation though:

runSetter: a function that runs the API to set the element's attribute to
the specified value. Before being executed, it may need to create an
attribute node. The attribute node before execution (existing or created)
is saved on runSetter.lastAttributeNode.

api:"Attr.value",
acceptNS: true,
acceptTrustedTypeArgumentInIDL: false,
runSetter: function(element, attrNS, attrName, attrValue, type) {
element.setAttributeNS(attrNS, attrName, createTrustedOutput(type, ""));
acceptTrustedTypeArgumentInIDL: false,
runSetter: function(element, attrNS, attrName, attrValue, type, initialAttrValue = "") {
element.setAttributeNS(attrNS, attrName, createTrustedOutput(type, initialAttrValue));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you are now adding an optional initialAttrValue. I think my original idea was to keep runSetter minimal so we can actually test situations when the element has no attributes and each test can decide to attach an attribute or not before running the setter. Arguably, finding the best generic APIs that works in all cases is a bit tricky...

If we do your proposed change, then we should update the documentation and make sure all versions of runSetter are consistent. I would also make the initialAttrValue optional so no attribute is attached by default (for these three cases the attribute would be mandatory though otherwise we don't have any attribute node). Common initialization can be factor out in a helper function.

this.lastAttributeNode = findAttribute(element, attrNS, attrName);
assert_true(!!this.lastAttributeNode);
return (this.lastAttributeNode.value = attrValue);
Expand All @@ -296,8 +296,8 @@ const attributeSetterData = [
api: "Node.nodeValue",
acceptNS: true,
acceptTrustedTypeArgumentInIDL: false,
runSetter: function(element, attrNS, attrName, attrValue, type) {
element.setAttributeNS(attrNS, attrName, createTrustedOutput(type, ""));
runSetter: function(element, attrNS, attrName, attrValue, type, initialAttrValue = "") {
element.setAttributeNS(attrNS, attrName, createTrustedOutput(type, initialAttrValue));
this.lastAttributeNode = findAttribute(element, attrNS, attrName);
assert_true(!!this.lastAttributeNode);
return (this.lastAttributeNode.nodeValue = attrValue);
Expand All @@ -307,8 +307,8 @@ const attributeSetterData = [
api: "Node.textContent",
acceptNS: true,
acceptTrustedTypeArgumentInIDL: false,
runSetter: function(element, attrNS, attrName, attrValue, type) {
element.setAttributeNS(attrNS, attrName, createTrustedOutput(type, ""));
runSetter: function(element, attrNS, attrName, attrValue, type, initialAttrValue = "") {
element.setAttributeNS(attrNS, attrName, createTrustedOutput(type, initialAttrValue));
this.lastAttributeNode = findAttribute(element, attrNS, attrName);
assert_true(!!this.lastAttributeNode);
return (this.lastAttributeNode.textContent = attrValue);
Expand Down