Skip to content
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

Add trusted types tests for setAttribute that are not sinks. #50295

Merged
merged 1 commit into from
Jan 28, 2025
Merged

Add trusted types tests for setAttribute that are not sinks. #50295

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
34 changes: 34 additions & 0 deletions trusted-types/set-event-handlers-content-attributes.tentative.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,40 @@
}, `${element.tagName}.setAttributeNS(${attr.name}, "${input_string}") calls default policy`);
});
}

[
// The following names correspond to event handler content attributes, but
// not for the div element. The current version of the spec says we should
// still run the default policy on them.
Copy link
Member

Choose a reason for hiding this comment

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

This also seems a bit weird on reflection, but I guess it's okay?

@domenic as you have dabbled a bit in event handler attributes I wonder what you make of the situation with Trusted Types (and soon the Sanitizer API). I guess there's these four classes:

  1. Elements and event handler content attributes defined for them
  2. Elements and event handler content attributes not defined for them (a couple are only defined for the body or media elements)
  3. Elements and event handler content attributes that don't exist, but the event handler attribute does exist somewhere
  4. Elements and event handler content attributes that don't exist, and there's no event handler attribute either

It seems that we're currently leaning towards treating 1 and 2 as something we need to apply policy toward and 3 and 4 as cases we can ignore.

Copy link
Member

Choose a reason for hiding this comment

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

To be really specific here, I believe 1 and 2 are consistent across browsers as being protected, 3 is something this test prohibits but implementations actually do protect against[1]. 4. is something that all browsers agree aren't protected.

[1] I'm not familiar with Firefox's implementation here but it seems to protect onreadystatechange for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding 3, Firefox protects "onreadystatechange" but not "onopen", so that's probably an implementation bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fred-wang Did you / Can you please file a bug to make sure this isn't lost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mozfreddyb Sure I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1944504 for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding 2, one simple refinement could be to not do the event handler check if the element is not in the HTML, SVG or MathML namespace: w3c/trusted-types#520

"onafterprint", // body or frameset elements.
"onwaitingforkey", // audio or video elements.
"onbegin", // SVG animation element.
].forEach(attrName => {
promise_test(async t => {
t.add_cleanup(resetSeenSinkName);
let element = document.createElement("div");
element.setAttribute(attrName, input_string);
assert_equals(seenSinkName, `Element ${attrName}`);
assert_equals(element.getAttribute(attrName), output_string);
}, `DIV.setAttribute("${attrName}", "${input_string}") calls default policy`);
});

// The following names do not correspond to any event handler content
// attributes on a DOM element, so default policy should not apply.
[
"onreadystatechange", // XMLHttpRequest, Document
"onopen", // EventSource, RTCDataChannel, WebSocket, etc
"ondoesnotexist", // starts with "on" but not defined in any spec.
].forEach(attrName => {
promise_test(async t => {
t.add_cleanup(resetSeenSinkName);
let element = document.createElement("div");
element.setAttribute(attrName, input_string);
assert_equals(seenSinkName, undefined);
assert_equals(element.getAttribute(attrName), input_string);
}, `DIV.setAttribute("${attrName}", "${input_string}") does not call default policy`);
});

});
</script>
</body>