Closed Bug 1944504 Opened 1 year ago Closed 1 year ago

Trusted Types checks for onmessage and onreadystatechange attributes

Categories

(Core :: DOM: Events, enhancement)

Product:
Core
Component:
DOM: Events
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
137 Branch
Tracking Flags:
Tracking Status
firefox137 --- fixed

People

(Reporter: fwang, Assigned: fwang)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Bug 1944504 - Add support for the onmessage content attribute on the body and frameset elements. r=smaug
48 bytes, text/x-phabricator-request
Details | Review
Bug 1944504 - Don't treat onreadystatechange/onvisibilitychange as HTML content attributes. r=smaug
48 bytes, text/x-phabricator-request
Details | Review

Tests:

  • set-event-handlers-content-attributes.tentative.html
  • TrustedTypePolicyFactory-getAttributeType-event-handler-content-attributes.tentative.html

Spec:

Implementation:

onmessage is an event handler attribute of body/frameset but we use the EventNameType_None flag. Probably the onmessage content attribute does not work on body right now and there is a FIXME message:
https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/dom/events/EventNameList.h#300

// XXXbz Should the onmessage attribute on <body> really not work?  If so, do we
// need a different macro to flag things like that (IDL, but not content
// attributes on body/frameset), or is just using EventNameType_None enough?
WINDOW_EVENT(message, eMessage, EventNameType_None, eBasicEventClass)

onreadystatechange is a document event and not an event handler attribute of any DOM element, however we use the EventNameType_HTMLXUL flag. Because nsContentUtils does not make a distinction for DOCUMENT_ONLY_EVENT (or WINDOW_ONLY_EVENT etc), it is currently wrongly treated as a event handler content attribute for TT.

https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/dom/events/EventNameList.h#361

DOCUMENT_ONLY_EVENT(readystatechange, eReadyStateChange, EventNameType_HTMLXUL,
                    eBasicEventClass)

See also https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/dom/events/EventNameList.h#19

Probably we would need a nsContentUtils::EventHandlerContentAttributeName that checks an extra flag. Defining FORWARDED_EVENT to set the flag a seems to be what we want, but we should make sure that still works for "HTMLMediaElement" (seems to be using EVENT macro) or "SVGAnimationElement" (seems to be using NON_IDL_EVENT macro).

Component: DOM: Security → DOM: Events

WINDOW_ONLY_EVENT and DOCUMENT_ONLY_EVENT are not supposed to be used for event
names corresponding to event handler content attributes. However, the
EventNameType_HTML bit is set for onreadystatechange and onvisibilitychange
attributes, and this bit is used here:

  • EventNameType_All: Only used in TrustedTypeUtils.cpp to treat attributes as
    TrustedScript sinks for attribute mutation and
    TrustedTypePolicyFactory.getAttributeType().
  • EventNameType_HTML: Only used in nsIContent::IsEventAttributeName() (via its
    Internal version) to register event handlers on HTML elements and treat
    attributes as JavaScript in the XML serializer.
  • EventNameType_HTMLXUL: Not used outside EventNameList.h

This patch stops treating onreadystatechange/onvisibilitychange as HTML content
attributes and only treat them as IDL attributes on the Document object:
https://html.spec.whatwg.org/multipage/webappapis.html#event-handlers-on-elements,-document-objects,-and-window-objects

TODO: add intent to unship.

Keywords: dev-doc-needed
Assignee: nobody → fwang
Attachment #9462701 - Attachment description: WIP: Bug 1944504 - Add support for the onmessage content attribute on the body and frameset elements. r=smaug → Bug 1944504 - Add support for the onmessage content attribute on the body and frameset elements. r=smaug
Status: NEW → ASSIGNED
Attachment #9462713 - Attachment description: WIP: Bug 1944504 - Don't treat onreadystatechange/onvisibilitychange as HTML content attributes. r=smaug → Bug 1944504 - Don't treat onreadystatechange/onvisibilitychange as HTML content attributes. r=smaug

Probably the onmessage content attribute does not work on body right now and there is a FIXME message:

Testcase: data:text/html,<body onmessage="alert('Hello World!')" onload="this.dispatchEvent(new Event('message'))">

Probably we would need a nsContentUtils::EventHandlerContentAttributeName that checks an extra flag. Defining FORWARDED_EVENT to set the flag a seems to be what we want, but we should make sure that still works for "HTMLMediaElement" (seems to be using EVENT macro) or "SVGAnimationElement" (seems to be using NON_IDL_EVENT macro).

So it turned out this suggestion was wrong, I think we just don't need the EventNameType_HTML bit (probably EventNameType_XUL either).

Testcase: data:text/html,<div id="x" onreadystatechange="alert('Unexpected event!')"><script>x.dispatchEvent(new Event("readystatechange"))</script>

Type: defect → enhancement
Pushed by fwang@igalia.com: https://hg.mozilla.org/integration/autoland/rev/c8dfc7a544b4 Add support for the onmessage content attribute on the body and frameset elements. r=smaug https://hg.mozilla.org/integration/autoland/rev/fba1faefa13a Don't treat onreadystatechange/onvisibilitychange as HTML content attributes. r=smaug https://hg.mozilla.org/integration/autoland/rev/06c418a8e2bb apply code formatting via Lando
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/50519 for changes under testing/web-platform/tests

https://hg.mozilla.org/mozilla-central/rev/c8dfc7a544b4
https://hg.mozilla.org/mozilla-central/rev/fba1faefa13a
https://hg.mozilla.org/mozilla-central/rev/06c418a8e2bb

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox137: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 137 Branch
Upstream PR merged by moz-wptsync-bot

I'm attempting to update compatibility data for APIs affected by trusted types. Can you explain what this change means for web application developers? (i.e. most changes I've been documented have clear IDL indicating that a property or method parameter can now take a Trusted type, but I can't tell what developers should do differently because of this change.

Can you explain? More particularly, what the compatibility data and docs changes might look like.

Flags: needinfo?(fwang)

(In reply to Hamish Willee from comment #8)

I'm attempting to update compatibility data for APIs affected by trusted types. Can you explain what this change means for web application developers? (i.e. most changes I've been documented have clear IDL indicating that a property or method parameter can now take a Trusted type, but I can't tell what developers should do differently because of this change.

Can you explain? More particularly, what the compatibility data and docs changes might look like.

Hi. I think the dev-doc-needed request was just for the following intent-to-ship:

https://groups.google.com/a/mozilla.org/g/dev-platform/c/M6k6vwMfzKQ
https://groups.google.com/a/mozilla.org/g/dev-platform/c/LJC3FnNbmfI

They indeed do affect Trusted Types (because we run Trusted Types check when setting an "onevent" content attribute on an element that really supports it ; and because that also affects the corresponding result for trustedTypes.getAttributeType(element, "onevent")) but that's really a minor change, so probably not worth documenting.

Flags: needinfo?(fwang)

Thanks Fred, I think the TLDR is that you're saying that there is nothing to document in this. Is that right?

I'm not sure I understand the rest of your response, but I THINK that what you're saying is that trustedTypes.getAttributeType()

  • now "works" for onmessage attribute, and presumably returns null?
  • no longer works for onreadystatechange/onvisibilitychange attributes on elements. We don't document the existence of this handler, so I think nothing to say.

If so then doing nothing makes sense - we don't need to update the docs that an attribute that doesn't require a trusted type will now be handled properly.

Flags: needinfo?(fwang)

Yes. What I'm saying is that what has changed is the behavior of <body onmessage="willnowrun()"> and <div onreadystatechange="wontrunanymore()"> when your dispatch a "message" or "readyonstatechange" event of them. I'll let you judge whether that's important enough to document.

Regarding how that affects TrustedTypes, I would say these changes don't need to be documented. But here there are for completeness:

  • trustedTypes.getAttributeType("body", "onmessage") will now return "TrustedScript" while trustedTypes.getAttributeType("div", "onreadystatechange") will now return null (it was the other way around before these two patches)

  • But more interestingly, document.body.setAttribute("onmessage", "unsafeHandler()") will run trusted type checks on "unsafeHandler()" while myDiv.setAttribute("onreadystatechange", "unsafeHandler()") won't run such checks any more (again it was the other way around before these two patches).

(same changes apply for other HTML elements than div, for "onvisibilitychange", and for other ways of setting attribute like setAttributeNS, setNamedItem, ...)

Flags: needinfo?(fwang)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: