Element::IsEventHandlerAttribute

[Daniel Vogelheim]

Hello DOMsters,

I have a question on Element::IsEventHandlerAttribute: This "approximates" whether an attribute is considered an event handler, by checking for the "on" prefix. We use the same heuristic in Trusted Types, where this causes problems by overestimating event handlers. I'm presently trying to fix this, by extracting event handlers from IDL files at build time and then checking against that list at runtime. (In progress CL here; which turns out to be surprisingly little code.)

- Should I fix IsEventHandlerAttribute as well? (Or actually, fix it only there, and then call it from the TT check as well?)

- Heuristics like this have a habit of becoming entrenched when people (purposely or not) start relying on its side effects. Is this observable from user scripts? If so, is it safe to modify? Why is that even there? :)

Thanks,

Daniel

[Mason Freed]

Hi Daniel,

Thanks for the email. So you're right that's an "approximation", but I believe it's a fairly well understood precedent, and is a "perfect" approximation. I.e. I don't believe there are any attributes that start with "on" that aren't event handlers, are there?

Can you clarify how this is becoming an issue for Trusted Types? Perhaps you need to handle developer-created attributes or something?

I'm not against making this more formal, I just want to understand the request.

Thanks,

Mason

[Daniel Vogelheim]

Hi Mason,

There are no *builtin* attributes where this mis-classifies attributes, but Trusted Types users are apparently running into this issue with frameworks that use JS-defined properties. Our TT implementation will then (erroneously) reject assignment to them. (On second thought, maybe the base DOM attribute implementation is different from the TT one. I suspect a custom property defined on a DOM element might not fall under the Element::IsEventHandlerAttribute case.)

The bug that tracks this for TT is here (example cases in comments 3, 4, 8, 9, 10, 11). Google's ISE team - which deploys TT at G - listed this as one of their most important blockers. Examples they gave include the Material Web Components JS libraries (onIcon / offIcon). Another example they came across is LitHTML, where they included an explicit work-around for on-named properties. Another example given is the Angular test suite, which apparently defines a "one" property somewhere.

As said: I'm not sure if this would apply to any actual uses of Element::IsEventHandlerAttribute (and if not, it would make little sense to modify it). But it apparently has hit Trusted Types usage in several instances.

Daniel

[Mason Freed]

Ahh interesting, ok I can see why it becomes an issue for TT. I think your approach (parsing IDL files for event handler attributes) sounds very reasonable. As you mentioned, IsEventHandlerAttribute likely doesn't "need" this, since it's really only used in a few places to avoid serializing things that contain scripts. But I don't think it'd hurt (and might help you?) to add a DCHECK within IsEventHandlerAttribute that makes sure the existing "on" definition and your more explicit list overlap perfectly. I'm also ok merging the two functions to just use the same code - I don't think that's a hot code path. Up to you. I'm happy to review patches.

Thanks,

Mason

[Daniel Vogelheim]

Thanks. To close the loop on this: I've decided to leave Element::IsEventHandlerAttribute alone. I've added a DCHECK for the "on"-prefix in the Trusted Type-relevant code paths. Just to be sure, I've also added a test that runs all attributes it finds on a "div" element through a check whether "on*" indeed correctly predicts event handler-ness.

The resulting changes in core/dom are minimal, but I'll add you for the owners' review.

[Mason Freed]

Sounds great, thanks!